From 3a6d348f4d60cb9e17ec552cd396e24d961c3f97 Mon Sep 17 00:00:00 2001 From: Cordelia-2434 Date: Sun, 18 Oct 2026 11:45:23 -0400 Subject: [PATCH 1/5] fix(mcpl): fetch_reference's failures say what happened to the reference and what calling again can do Every failure of the reference fetcher is now two texts: a clause about the reference ("the "srv" server answered HTTP 503", "it claims 10000 bytes, over the 1000 bytes this call allows", ...) and a sentence saying what calling again can do: try later, wait for a fresh reference, pass a larger max_bytes (no more than the server's budget has left), ask whoever runs the host, or nothing. fetch_reference reads `fetch_reference couldn't fetch "": . `. save_recent_image gives both where a reference can't be fetched, through #518's refusal(). The eager and script-demand log lines read the clause. - FetchOutcome becomes a union whose failure side carries both texts, so a failure without a remedy doesn't compile. #518's Refusal {reason, ofReference} becomes {error, remedy}: each clause has its own subject. - Each refusal before dialing says what could change it: a server this host no longer has (whoever runs it can add it back), a server with no address (nothing), a claim past the host's own ceiling (no max_bytes or setting lifts it). - The scheme check reads the uri's own protocol, so a ws: or wss: reference on its server's origin is refused before dialing. The unreachable plaintext-from-a-secure-origin branch goes; an http: reference from a server dialed over wss: is refused by the origin comparison, and a test pins that. - A server address that doesn't parse is refused as having no address. - The budget's stop partway shows the server's running total, as the next refusal does, since other fetches may have spent part of it. - The save hook logs why it returns null; a save that throws is a storage failure, logged, not the fetch's own error. - A thrown fetch's clause takes its cause's words, and an AggregateError's attempts' when it has none of its own. - #518's changelog entry, not yet released, quotes the no-address refusal's old words, so it now quotes these, and this change's own entry doesn't repeat them. Co-Authored-By: Claude Opus 5.5 --- changelog.d/fetch-reference-words.changed.md | 6 + .../no-address-server-refusal.fixed.md | 6 +- src/framework.ts | 20 +- src/mcpl/reference-fetcher.ts | 235 +++++++++++----- src/mcpl/references.ts | 10 +- test/mcpl-reference-binding.test.ts | 18 +- test/mcpl-reference-fetcher.test.ts | 259 ++++++++++++++---- test/save-recent-image.test.ts | 23 +- test/tool-result-reference-uri.test.ts | 2 +- 9 files changed, 436 insertions(+), 143 deletions(-) create mode 100644 changelog.d/fetch-reference-words.changed.md diff --git a/changelog.d/fetch-reference-words.changed.md b/changelog.d/fetch-reference-words.changed.md new file mode 100644 index 000000000..51ae5c1c5 --- /dev/null +++ b/changelog.d/fetch-reference-words.changed.md @@ -0,0 +1,6 @@ +- `fetch_reference`'s failures now say what happened to the reference and what calling again can do: `fetch_reference couldn't fetch "": . `, where they gave the fetcher's reason alone, such as `HTTP 404`, `fetch timed out or aborted`, `claimed bytes exceeds fetch ceiling ` or `third-party origin ( != dialed ); not fetched by this host`. Where `save_recent_image` says a reference can't be fetched, it gives the same two. + - **What happened** names the server and what it answered (`the "" server answered HTTP 503`), the limit a fetch met (`it claims bytes, over the bytes this call allows`, `the download had reached bytes, past …`), what kept a fetch from dialing (`it points to , not to the "" server's own address (), and this host fetches only from the server itself`), or that what arrived didn't match the reference's digest or couldn't be stored. The eager fetch's and a script's fetch's log lines read the same words. + - **What calling again can do**, such as trying again later (after a timeout, a failed request, 408, 429, or a 5xx other than 501 and 505), waiting for a fresh reference to it (an expired or unbound reference, and the other 4xx), passing a larger `max_bytes` (naming the size it needs, and no more than the server's fetch budget has left), asking whoever runs the host (who can check its credentials for the server after a 401 or 403, raise or renew a spent budget, or add back a server it no longer has), or nothing, as for an address on another origin, a server this host doesn't connect to over a network, or a claim past the 256 MiB this host fetches at most, which no `max_bytes` or setting lifts. + - **The budget's texts** read `this host's fetch budget for the "" server is used up ( of bytes)` and, partway, `this host's fetch budget for the "" server ran out during the download ( of bytes), so it was stopped and nothing was kept`, where they read `per-server fetch budget exhausted (/ bytes)`, the one partway after `aborted: `. + - **A `ws:` or `wss:` reference** on its server's own origin is refused for its scheme before anything is dialed, where the fetch dialed it and failed as a network error. A server whose address doesn't parse is refused as one with no address. + - **The host's log** says why a fetched payload couldn't be stored: no workspace, no writable mount, or the write's error. A save that throws is that failure too, logged, where its message reached the agent as the fetch's own error. diff --git a/changelog.d/no-address-server-refusal.fixed.md b/changelog.d/no-address-server-refusal.fixed.md index d78b3e834..bfbea0ee9 100644 --- a/changelog.d/no-address-server-refusal.fixed.md +++ b/changelog.d/no-address-server-refusal.fixed.md @@ -1,8 +1,8 @@ - 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, + `unknown server: `, it now says `this host doesn't connect to the + "" 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` the transport won't dial: one that isn't a `ws://` or `wss://` url, or that has a diff --git a/src/framework.ts b/src/framework.ts index cb39c3006..75566c83a 100644 --- a/src/framework.ts +++ b/src/framework.ts @@ -4920,11 +4920,19 @@ export class AgentFramework { } return { url: cfg.url, token: cfg.token, autofetch: cfg.autofetch }; }, + // A null tells the agent only that the payload couldn't be stored; + // why, which can name the host's paths, goes to the log. async (fileName, data, mimeType) => { const workspace = this.getWorkspaceModule(); - if (!workspace) return null; + if (!workspace) { + console.error(`[refs] couldn't store ${fileName}: this host has no workspace`); + return null; + } const mount = this.firstWritableMountName(workspace); - if (!mount) return null; + if (!mount) { + console.error(`[refs] couldn't store ${fileName}: no workspace mount is writable`); + return null; + } const path = `${mount}/refs/${fileName}`; const res = await workspace.writeBinary(path, data, mimeType); if (res.success) return path; @@ -4934,6 +4942,7 @@ export class AgentFramework { console.error(`[refs] ${path} is in the workspace's Chronicle tree, but the rest of its write failed: ${res.error ?? 'unknown error'}`); return path; } + console.error(`[refs] couldn't store ${path}: ${res.error ?? 'unknown error'}`); return null; }, ); @@ -23684,7 +23693,10 @@ export class AgentFramework { result = outcome.ok ? { success: true, data: JSON.stringify({ path: outcome.path, bytes: outcome.bytes, mime_type: outcome.mimeType, digest_verified: outcome.digestVerified ?? 'no digest claimed' }) } - : { success: false, isError: true, error: outcome.error }; + // The fetcher says what happened as a clause about the reference, + // and what calling again can do as a sentence of its own. + : { success: false, isError: true, + error: `fetch_reference couldn't fetch "${refId}": ${outcome.error}. ${outcome.remedy}` }; } this.emitTrace({ type: result.isError ? 'tool:failed' : 'tool:completed', @@ -23937,7 +23949,7 @@ export class AgentFramework { "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.' : refusal - ? `It can't be fetched either — ${refusal.reason}.` + ? `It can't be fetched either — ${refusal.error}. ${refusal.remedy}` : 'Fetch it with fetch_reference and save the materialized file instead.'), ); } diff --git a/src/mcpl/reference-fetcher.ts b/src/mcpl/reference-fetcher.ts index 3d9a9261e..daddc6116 100644 --- a/src/mcpl/reference-fetcher.ts +++ b/src/mcpl/reference-fetcher.ts @@ -8,8 +8,10 @@ * not fetch third-party origins at all (deferred with declared reference * origins); the connection credential is applied as an Authorization * header by this code alone and never surfaces to model, script, or URI. - * - **scheme allowlist (§7.1, vector 11):** https; http only when the dialed - * origin itself is plaintext (a ws:// server references its own origin). + * - **scheme allowlist (§7.1, vector 11):** the uri's own scheme is http: or + * https:, and since origins compare with their schemes, http only from a + * server dialed in plaintext (ws://), https only from one dialed over TLS + * (wss://). * - **no redirect traversal (§7.2, vector 12):** any 3xx refuses. * - **actual-byte ceiling (§7.3 list, vector 10):** streamed with abort the * moment real octets exceed the ceiling, regardless of claimed sizeBytes. @@ -30,9 +32,11 @@ */ import { createHash } from 'node:crypto'; import type { ReferenceRecord } from './references.js'; +import { boundedForLog, describeCause } from './errors.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). */ +/** Per-fetch hard ceiling on actual bytes. A call can lower it for itself + * (fetch_reference's max_bytes, which it clamps to this); nothing raises it. */ export const DEFAULT_FETCH_MAX_BYTES = 256 * 1024 * 1024; /** Eager (autofetch) threshold: at most what could have shipped inline. */ export const DEFAULT_EAGER_FETCH_MAX_BYTES = 256 * 1024; @@ -41,13 +45,30 @@ export const DEFAULT_EAGER_FETCH_MAX_BYTES = 256 * 1024; * kept. */ export const DEFAULT_MAX_TOTAL_BYTES = 1024 * 1024 * 1024; const FETCH_TIMEOUT_MS = 60_000; -/** What a refusal by the per-server budget adds. The total lives as long as - * this fetcher, one per framework, so only someone running the host renews - * it, by starting a new one, or raises the budget (the server's config's - * autofetch.maxTotalBytes, which each fetch reads afresh). */ -const BUDGET_LASTS = "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 EAGER_FETCH_TIMEOUT_MS = 10_000; +// What calling again can do, as each failure's remedy says it. The fetcher +// doesn't know where a reference came from (a tool result, a push, a channel +// message), so none names a tool, and fetch_reference's max_bytes is named +// only where a larger one can fetch it. +const NOTHING_TO_DO = "Calling again won't change that."; +/** The registry keys a record by its uri, so a fresh reference to the same + * uri arrives as this very id and refreshes its record. */ +const FRESH_REFERENCE = "Once a fresh reference to it arrives, fetching may work; until then, fetching it again won't."; +const TRY_LATER = 'Trying again later may work.'; +const CREDENTIALS = "Calling again won't change that; whoever runs this host can check its credentials for the server."; +const FRESH_OR_CREDENTIALS = "Once a fresh reference to it arrives, fetching may work; if that's refused too, whoever runs this host can check its credentials for the server."; +/** The total lives as long as this fetcher, one per framework, so only + * someone running the host renews it, by starting a new one, or raises the + * budget (the server's config's autofetch.maxTotalBytes, which each fetch + * reads afresh). */ +const BUDGET = "The budget doesn't renew on its own, so calling again won't change that unless whoever runs this host raises or renews it."; +/** The host's own ceiling: fetch_reference clamps max_bytes to it, and no + * config sets another. */ +const HOST_CEILING = "No max_bytes or setting lifts that limit, so calling again won't change it."; +/** The resolver reads the host's server configs as they are at each fetch. */ +const SERVER_BACK = "Calling again won't change that unless whoever runs this host adds that server back."; + export interface ResolvedServerBinding { /** 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 @@ -64,28 +85,75 @@ export interface ResolvedServerBinding { * 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. */ +/** Why a fetch failed, or would be refused before dialing. */ export interface Refusal { - reason: string; - ofReference: boolean; + /** What happened, as a clause that completes "couldn't fetch : ", + * whatever it's about: fetch_reference's sentence, save_recent_image's + * and the host's log lines all read it. */ + error: string; + /** What calling again can do, as a sentence. fetch_reference and + * save_recent_image show it; the eager and script-demand fetches log the + * error alone. */ + remedy: string; +} + +/** A fetch's result: the saved copy, or why it failed, which every failure + * says with both a clause and a remedy. The fields the other side lacks are + * declared undefined, so a caller can read them before checking ok. */ +export type FetchOutcome = + | { + ok: true; + /** Mount-prefixed workspace path of the saved payload. */ + path: string; + bytes?: number; + mimeType?: string; + digestVerified?: boolean; + error?: undefined; + remedy?: undefined; + } + | ({ ok: false; path?: undefined; bytes?: undefined; mimeType?: undefined; digestVerified?: undefined } & Refusal); + +const refused = (error: string, remedy: string): { refusal: Refusal } => ({ refusal: { error, remedy } }); +const fail = (error: string, remedy: string): FetchOutcome => ({ ok: false, error, remedy }); + +/** What calling again can do about a server's HTTP status. A timeout, a rate + * limit or a server's passing trouble can clear, though not 501 or 505, which + * say the server doesn't support the request. A 401 is about this host's + * credentials for the server, which a fresh reference doesn't change, and a + * 403 can be either those or this reference. The rest of 4xx is about this + * reference, which a fresh one may not share. */ +function statusRemedy(status: number): string { + if (status === 501 || status === 505) return NOTHING_TO_DO; + if (status === 408 || status === 429 || status >= 500) return TRY_LATER; + if (status === 401) return CREDENTIALS; + if (status === 403) return FRESH_OR_CREDENTIALS; + return FRESH_REFERENCE; } -/** fetch_reference's words for a refusal. */ -function refusalText(refusal: Refusal): string { - return refusal.ofReference ? `reference ${refusal.reason}` : refusal.reason; +/** What a download stopped at a call's own ceiling, below the host's, can do. + * Its whole size is unknown, only that it's past what arrived, and a fetch + * again reads it from the start, within the host's ceiling and what the + * server's budget has left after this one's bytes. */ +function largerMaxBytes(received: number, left: number, server: string): string { + const most = Math.min(DEFAULT_FETCH_MAX_BYTES, left); + if (received <= most) return `It's at least ${received} bytes; a larger max_bytes, up to ${most}, may fetch it.`; + return left < DEFAULT_FETCH_MAX_BYTES + ? `It's at least ${received} bytes, more than the ${left} bytes left of this host's fetch budget for ${server}. ${BUDGET}` + : `It's at least ${received} bytes, more than the ${DEFAULT_FETCH_MAX_BYTES} bytes this host fetches at most. ${HOST_CEILING}`; } -export interface FetchOutcome { - ok: boolean; - /** Mount-prefixed workspace path of the saved payload (on success). */ - path?: string; - bytes?: number; - mimeType?: string; - digestVerified?: boolean; - error?: string; +/** What a thrown fetch says went wrong. fetch's own "fetch failed" says only + * that, so its cause's words come first. A cause from trying each of a + * host's addresses (an AggregateError) has no words of its own, so its + * attempts' stand in, then its code. */ +function whyFetchFailed(e: unknown): string { + const source = (e as { cause?: unknown } | null | undefined)?.cause ?? e; + const own = describeCause(source); + if (own !== '') return own; + const attempts = (source as { errors?: unknown } | null | undefined)?.errors; + if (Array.isArray(attempts) && attempts.length > 0) return attempts.map(describeCause).join('; '); + const code = (source as { code?: unknown } | null | undefined)?.code; + return typeof code === 'string' ? code : describeCause(e); } /** Magic-byte sniff for the formats this pipeline actually moves. The @@ -167,22 +235,23 @@ export class ReferenceFetcher { * 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; + let refusal: { refusal: Refusal } | undefined; for (const serverId of record.serverIds) { const binding = this.resolveServer(serverId); if (!binding) { - refusal ??= { reason: `unknown server: ${serverId}`, ofReference: false }; + refusal ??= refused(`this host no longer has the "${serverId}" server it came from`, SERVER_BACK); continue; } if (binding === 'no-address') { - refusal ??= { reason: noAddressReason(serverId), ofReference: false }; + refusal ??= refused(noAddressReason(serverId), NOTHING_TO_DO); continue; } - const offOrigin = this.originCheck(record, binding); + const offOrigin = this.originCheck(record, binding, serverId); if (offOrigin === null) return { serverId, binding }; - refusal ??= { reason: offOrigin, ofReference: false }; + refusal ??= offOrigin; } - return { refusal: refusal ?? { reason: UNBOUND_REASON, ofReference: true } }; + // A later delivery of the same uri by a server binds this record. + return refusal ?? refused(`it's ${UNBOUND_REASON}`, FRESH_REFERENCE); } /** Every check the fetch makes before it dials, in its order: the server @@ -199,7 +268,7 @@ export class ReferenceFetcher { const { serverId, binding } = server; const t = record.testimony; const expiry = referenceExpiry(t); - if (expiry) return { refusal: { reason: EXPIRY_REASON[expiry], ofReference: true } }; + if (expiry) return refused(`the reference is ${EXPIRY_REASON[expiry]}`, FRESH_REFERENCE); const ceiling = opts?.maxBytes ?? DEFAULT_FETCH_MAX_BYTES; // The budget is checked before the ceiling, here and while reading: a @@ -208,7 +277,7 @@ export class ReferenceFetcher { 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 } }; + return refused(`this host's fetch budget for the "${serverId}" server is used up (${used} of ${budget} bytes)`, BUDGET); } // 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, @@ -216,14 +285,17 @@ export class ReferenceFetcher { // 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 refused( + `it claims ${t.sizeBytes} bytes, more than the ${budget - used} bytes left of this host's fetch budget for the "${serverId}" server`, + BUDGET, + ); } - return { refusal: { reason: `claimed ${t.sizeBytes} bytes exceeds fetch ceiling ${ceiling}`, ofReference: false } }; + // Within what's left of the budget, so a ceiling of the call's own, + // below the host's, is what a larger max_bytes lifts. + return t.sizeBytes <= DEFAULT_FETCH_MAX_BYTES + ? refused(`it claims ${t.sizeBytes} bytes, over the ${ceiling} bytes this call allows`, + `Pass max_bytes of at least ${t.sizeBytes} to fetch it.`) + : refused(`it claims ${t.sizeBytes} bytes, over the ${DEFAULT_FETCH_MAX_BYTES} bytes this host fetches at most`, HOST_CEILING); } return { serverId, binding, ceiling, budget }; } @@ -239,19 +311,27 @@ export class ReferenceFetcher { return 'refusal' in check ? check.refusal : null; } - /** §6.1/§7.1 policy check; returns a refusal reason or null. */ - private originCheck(record: ReferenceRecord, binding: ResolvedServerBinding): string | null { + /** §6.1/§7.1 policy check; returns a refusal or null. */ + private originCheck(record: ReferenceRecord, binding: ResolvedServerBinding, serverId: string): { refusal: Refusal } | null { const refOrigin = normalizedOrigin(record.testimony.uri); + if (!refOrigin) return refused("its address doesn't parse as a URL", NOTHING_TO_DO); + // A server's address that doesn't parse leaves nothing to dial, as for a + // server with none. The framework's resolver gives only an address its + // transport dials, so this guards other resolvers. const dialedOrigin = normalizedOrigin(binding.url); - if (!refOrigin) return 'reference uri does not parse'; - if (!dialedOrigin) return 'server url does not parse'; - const scheme = refOrigin.split('//')[0]; - if (scheme !== 'https:' && scheme !== 'http:') return `scheme not allowed: ${scheme}`; - if (refOrigin !== dialedOrigin) { - return `third-party origin (${refOrigin} != dialed ${dialedOrigin}); not fetched by this host`; + if (!dialedOrigin) return refused(noAddressReason(serverId), NOTHING_TO_DO); + // The uri's own scheme, not its origin's: an origin reads ws: as http: + // and wss: as https:, and fetch can't dial a ws: or wss: uri, which would + // fail only once dialed, as a network error a later try can't mend. + const scheme = new URL(record.testimony.uri).protocol; + if (scheme !== 'https:' && scheme !== 'http:') { + return refused(`its address uses ${scheme}, and this host fetches only http: and https: addresses`, NOTHING_TO_DO); } - if (scheme === 'http:' && dialedOrigin.startsWith('https:')) { - return 'plaintext reference from a secure origin'; + // Origins compare with their schemes, so this also refuses an http: + // reference from a server dialed over TLS (wss://). + if (refOrigin !== dialedOrigin) { + return refused(`it points to ${refOrigin}, not to the "${serverId}" server's own address (${dialedOrigin}), ` + + 'and this host fetches only from the server itself', NOTHING_TO_DO); } return null; } @@ -273,20 +353,22 @@ export class ReferenceFetcher { private async doFetch(record: ReferenceRecord, opts?: { maxBytes?: number; timeoutMs?: number }): Promise { const t = record.testimony; const check = this.beforeDial(record, opts); - if ('refusal' in check) return { ok: false, error: refusalText(check.refusal) }; + if ('refusal' in check) return { ok: false, ...check.refusal }; const { serverId, binding, ceiling, budget } = check; + const server = `the "${serverId}" server`; + const timeoutMs = opts?.timeoutMs ?? FETCH_TIMEOUT_MS; const controller = new AbortController(); - const timer = setTimeout(() => controller.abort(), opts?.timeoutMs ?? FETCH_TIMEOUT_MS); + const timer = setTimeout(() => controller.abort(), timeoutMs); try { const headers: Record = {}; if (binding.token) headers.authorization = `Bearer ${binding.token}`; const resp = await fetch(t.uri, { headers, redirect: 'manual', signal: controller.signal }); if (resp.status >= 300 && resp.status < 400) { - return { ok: false, error: `redirect refused (HTTP ${resp.status}) — no redirect traversal` }; + return fail(`${server} answered HTTP ${resp.status}, a redirect, and this host doesn't follow redirects`, NOTHING_TO_DO); } - if (!resp.ok) return { ok: false, error: `HTTP ${resp.status}` }; - if (!resp.body) return { ok: false, error: 'no response body' }; + if (!resp.ok) return fail(`${server} answered HTTP ${resp.status}`, statusRemedy(resp.status)); + if (!resp.body) return fail(`${server} answered without any content`, FRESH_REFERENCE); const hash = createHash('sha256'); const chunks: Buffer[] = []; @@ -302,11 +384,18 @@ export class ReferenceFetcher { this.totals.set(serverId, total); if (total > budget) { controller.abort(); - return { ok: false, error: `aborted: per-server fetch budget exhausted (${total}/${budget} bytes); ${BUDGET_LASTS}` }; + // The server's running total, as the next fetch's refusal gives it: + // other fetches through the server may have spent part of it. + return fail(`this host's fetch budget for ${server} ran out during the download (${total} of ${budget} bytes), ` + + 'so it was stopped and nothing was kept', BUDGET); } if (received > ceiling) { controller.abort(); - return { ok: false, error: `aborted: actual bytes exceeded ceiling at ${received}` }; + return ceiling >= DEFAULT_FETCH_MAX_BYTES + ? fail(`the download had reached ${received} bytes, past the ${DEFAULT_FETCH_MAX_BYTES} bytes this host fetches at most, ` + + 'so it was stopped and nothing was kept', HOST_CEILING) + : fail(`the download had reached ${received} bytes, past the ${ceiling} bytes this call allows, ` + + 'so it was stopped and nothing was kept', largerMaxBytes(received, budget - total, server)); } const buf = Buffer.from(value); hash.update(buf); @@ -319,7 +408,11 @@ export class ReferenceFetcher { const got = 'sha256:' + hash.digest('base64url'); if (got !== t.digest) { // Vector 5: never presented as the described content. - return { ok: false, error: `digest mismatch (claimed ${t.digest.slice(0, 20)}…, got ${got.slice(0, 20)}…)` }; + return fail( + `the bytes that arrived don't match the reference's digest (${t.digest.slice(0, 20)}…, against ${got.slice(0, 20)}…), ` + + "so they weren't kept", + "Trying again may work; if it fails the same way, this reference can't be fetched here.", + ); } digestVerified = true; } @@ -334,8 +427,21 @@ export class ReferenceFetcher { const observedMime = sniffMime(data) ?? headerMime ?? claimedMime ?? 'application/octet-stream'; const ext = EXT_BY_MIME.get(observedMime) ?? EXT_BY_MIME.get(headerMime ?? '') ?? EXT_BY_MIME.get(claimedMime ?? '') ?? 'bin'; // §7.3: host-generated storage name; the server's `name` is display-only. - const path = await this.saveFile(`${record.refId}.${ext}`, data, observedMime); - if (path === null) return { ok: false, error: 'no writable workspace mount to store the payload' }; + // A null from the save hook covers no workspace, no writable mount and + // a failed write, and the hook logs which. A throw is the save's failure + // too, not the fetch's, so it's logged here. + const fileName = `${record.refId}.${ext}`; + let path: string | null; + try { + path = await this.saveFile(fileName, data, observedMime); + } catch (e) { + console.error(`[refs] couldn't store ${fileName}: ${describeCause(e)}`); + path = null; + } + if (path === null) { + return fail("it was fetched, but this host couldn't store it in its workspace, so nothing was kept", + 'Calling again is likely to fail the same way; whoever runs this host can see why in its log.'); + } record.fetchedPath = path; record.verifiedBytes = data.length; @@ -343,8 +449,13 @@ export class ReferenceFetcher { record.digestVerified = digestVerified; return { ok: true, path, bytes: data.length, mimeType: observedMime, digestVerified }; } catch (e) { - const msg = (e as Error).name === 'AbortError' ? 'fetch timed out or aborted' : (e as Error).message; - return { ok: false, error: msg }; + // The timer is the only abort that throws: the budget and the ceiling + // abort the download and return at once. + if ((e as Error)?.name === 'AbortError') { + const seconds = timeoutMs / 1000; + return fail(`it took longer than ${seconds} second${seconds === 1 ? '' : 's'}`, TRY_LATER); + } + return fail(`the request to ${server} failed: ${boundedForLog(whyFetchFailed(e))}`, TRY_LATER); } finally { clearTimeout(timer); } diff --git a/src/mcpl/references.ts b/src/mcpl/references.ts index e37bab3f0..acff9ecb8 100644 --- a/src/mcpl/references.ts +++ b/src/mcpl/references.ts @@ -223,12 +223,12 @@ export const EXPIRY_REASON = { export const UNBOUND_REASON = "not tied to any server, so there's nowhere 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. */ + * host doesn't connect to over a network, such as a stdio server, as a + * clause about the reference; 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`; + return `this host doesn't connect to the "${serverId}" server over a network, so there's nowhere to fetch it from`; } /** Strip control characters (C0, DEL, C1) and every bidi control (marks, diff --git a/test/mcpl-reference-binding.test.ts b/test/mcpl-reference-binding.test.ts index c12dc95a6..678a20060 100644 --- a/test/mcpl-reference-binding.test.ts +++ b/test/mcpl-reference-binding.test.ts @@ -174,31 +174,35 @@ test("fetch_reference reaches a push event's reference through its server's own const fetched = await fetchReference(own!.refId); assert.equal(fetched.success, true, fetched.error); assert.equal(dialed, 1); + // fetch_reference's sentence: the fetcher's clause, then what calling again can do. + const refusedWith = (refId: string, clause: string, remedy = "Calling again won't change that.") => + `fetch_reference couldn't fetch "${refId}": ${clause}. ${remedy}`; const refused = await fetchReference(elsewhere!.refId); assert.equal(refused.success, false); - assert.match(refused.error ?? '', /third-party origin/); + assert.match(refused.error ?? '', /^fetch_reference couldn't fetch "ref_\S+": it points to \S+, not to the "\S+" server's own address /); const noAddress = await fetchReference(stdio!.refId); assert.equal(noAddress.success, false); - assert.equal(noAddress.error, noAddressReason('stdio-srv')); + assert.equal(noAddress.error, refusedWith(stdio!.refId, noAddressReason('stdio-srv'))); const notDialed = await fetchReference(spawned!.refId); assert.equal(notDialed.success, false); - assert.equal(notDialed.error, noAddressReason('spawned-srv')); + assert.equal(notDialed.error, refusedWith(spawned!.refId, noAddressReason('spawned-srv'))); const unknown = await fetchReference(gone!.refId); assert.equal(unknown.success, false); - assert.equal(unknown.error, 'unknown server: gone-srv'); + assert.equal(unknown.error, refusedWith(gone!.refId, 'this host no longer has the "gone-srv" server it came from', + "Calling again won't change that unless whoever runs this host adds that server back.")); 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')); + assert.equal(spawnedByTransport.error, refusedWith(stdioUrl!.refId, 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(undialable.error, refusedWith(wsHttp!.refId, noAddressReason('ws-http-srv'))); const fragment = await fetchReference(wsFragment!.refId); assert.equal(fragment.success, false); - assert.equal(fragment.error, noAddressReason('ws-fragment-srv')); + assert.equal(fragment.error, refusedWith(wsFragment!.refId, noAddressReason('ws-fragment-srv'))); assert.equal(dialed, 2, 'nothing dialed for the last three'); }); diff --git a/test/mcpl-reference-fetcher.test.ts b/test/mcpl-reference-fetcher.test.ts index 4e201885b..9646d2f2e 100644 --- a/test/mcpl-reference-fetcher.test.ts +++ b/test/mcpl-reference-fetcher.test.ts @@ -8,7 +8,7 @@ import test from 'node:test'; import assert from 'node:assert/strict'; import { createServer } from 'node:http'; import { createHash } from 'node:crypto'; -import { ReferenceFetcher, normalizedOrigin, type ServerResolution } from '../src/mcpl/reference-fetcher.js'; +import { DEFAULT_FETCH_MAX_BYTES, 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'; @@ -54,6 +54,9 @@ const server = createServer((req, res) => { // claims small in testimony, streams big (vector 10) res.writeHead(200, { 'content-type': 'application/octet-stream' }); res.end(Buffer.alloc(200_000, 7)); + } else if (url.pathname.startsWith('/status/')) { + // answers with the status its path names, and nothing else + res.writeHead(Number(url.pathname.slice('/status/'.length))); res.end(); } else { res.writeHead(404); res.end(); } @@ -76,6 +79,14 @@ function makeRecord(reg: ReferenceRegistry, uri: string, extra: Record, { kind: 'reference' }>).testimony, serverId); } +// What a failure says calling again can do, in the fetcher's words. +const NOTHING_TO_DO = "Calling again won't change that."; +const FRESH_REFERENCE = "Once a fresh reference to it arrives, fetching may work; until then, fetching it again won't."; +const TRY_LATER = 'Trying again later may work.'; +const BUDGET = "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 HOST_CEILING = "No max_bytes or setting lifts that limit, so calling again won't change it."; +const SERVER_BACK = "Calling again won't change that unless whoever runs this host adds that server back."; + test('happy path: origin-bound fetch, bearer auth applied host-side, digest verified', async () => { const reg = new ReferenceRegistry(); const f = makeFetcher({ srv: { url: `ws://127.0.0.1:${port}`, token: 'sekrit' } }); @@ -97,7 +108,8 @@ test('vector 5: digest mismatch is never presented', async () => { const rec = makeRecord(reg, `${ORIGIN}/ok`, { digest: 'sha256:' + 'B'.repeat(43) }); const out = await f.fetch(rec); assert.equal(out.ok, false); - assert.match(out.error!, /digest mismatch/); + assert.match(out.error!, /^the bytes that arrived don't match the reference's digest \(sha256:B{13}…, against sha256:[\w-]{13}…\), so they weren't kept$/); + assert.equal(out.remedy, "Trying again may work; if it fails the same way, this reference can't be fetched here."); assert.equal(rec.fetchedPath, undefined); }); @@ -107,7 +119,10 @@ test('vector 10: stream past the ceiling aborts on actual bytes', async () => { const rec = makeRecord(reg, `${ORIGIN}/liar`, { sizeBytes: 100 }); const out = await f.fetch(rec, { maxBytes: 50_000 }); assert.equal(out.ok, false); - assert.match(out.error!, /exceeded ceiling/); + const [, received] = /^the download had reached (\d+) bytes, past the 50000 bytes this call allows, so it was stopped and nothing was kept$/.exec(out.error!) ?? []; + assert.ok(received, out.error); + // Its whole size is unknown, only that it's past what arrived, and the budget has room for the host's ceiling. + assert.equal(out.remedy, `It's at least ${received} bytes; a larger max_bytes, up to ${DEFAULT_FETCH_MAX_BYTES}, may fetch it.`); }); test('vector 4: a claim already over the ceiling refuses without dialing', async () => { @@ -116,7 +131,13 @@ test('vector 4: a claim already over the ceiling refuses without dialing', async const rec = makeRecord(reg, `${ORIGIN}/ok`, { sizeBytes: 10_000_000 }); const out = await f.fetch(rec, { maxBytes: 1000 }); assert.equal(out.ok, false); - assert.match(out.error!, /exceeds fetch ceiling/); + assert.equal(out.error, 'it claims 10000000 bytes, over the 1000 bytes this call allows'); + assert.equal(out.remedy, 'Pass max_bytes of at least 10000000 to fetch it.'); + // Past the host's own ceiling, no max_bytes can fetch it. + const huge = await f.fetch(makeRecord(reg, `${ORIGIN}/ok?huge`, { sizeBytes: DEFAULT_FETCH_MAX_BYTES + 1 }), { maxBytes: 1000 }); + assert.equal(huge.error, `it claims ${DEFAULT_FETCH_MAX_BYTES + 1} bytes, over the ${DEFAULT_FETCH_MAX_BYTES} bytes this host fetches at most`); + assert.equal(huge.remedy, HOST_CEILING); + assert.equal(hits.get('/ok?huge') ?? 0, 0, 'nothing dialed'); }); test('vector 11: non-http(s) scheme refused', async () => { @@ -125,7 +146,22 @@ test('vector 11: non-http(s) scheme refused', async () => { const rec = makeRecord(reg, 'file:///etc/passwd'); const out = await f.fetch(rec); assert.equal(out.ok, false); - assert.match(out.error!, /scheme not allowed|third-party/); + assert.equal(out.error, 'its address uses file:, and this host fetches only http: and https: addresses'); + assert.equal(out.remedy, NOTHING_TO_DO); +}); + +test("a ws: or wss: reference on its server's own origin is refused for its scheme, with nothing dialed", async () => { + // An origin reads ws: as http: and wss: as https:, so each is on the dialed + // origin. fetch can't dial either, and would fail only once dialed. + const reg = new ReferenceRegistry(); + for (const [scheme, dialed] of [['ws:', `ws://127.0.0.1:${port}`], ['wss:', 'wss://ws-ref.example.invalid']] as const) { + const f = makeFetcher({ srv: { url: dialed } }); + const host = scheme === 'ws:' ? `127.0.0.1:${port}` : 'ws-ref.example.invalid'; + const out = await f.fetch(makeRecord(reg, `${scheme}//${host}/ok?scheme=${scheme}`)); + assert.equal(out.error, `its address uses ${scheme}, and this host fetches only http: and https: addresses`, scheme); + assert.equal(out.remedy, NOTHING_TO_DO, scheme); + } + assert.equal(hits.get('/ok?scheme=ws:') ?? 0, 0, 'nothing dialed'); }); test('vector 12: redirects refused, no traversal', async () => { @@ -134,7 +170,8 @@ test('vector 12: redirects refused, no traversal', async () => { const rec = makeRecord(reg, `${ORIGIN}/redirect`); const out = await f.fetch(rec); assert.equal(out.ok, false); - assert.match(out.error!, /redirect refused/); + assert.equal(out.error, `the "srv" server answered HTTP 302, a redirect, and this host doesn't follow redirects`); + assert.equal(out.remedy, NOTHING_TO_DO); }); test('third-party origin refused (v1 policy: dialed origin only)', async () => { @@ -143,20 +180,35 @@ test('third-party origin refused (v1 policy: dialed origin only)', async () => { const rec = makeRecord(reg, 'https://cdn.example.com/thing.wav'); const out = await f.fetch(rec); assert.equal(out.ok, false); - assert.match(out.error!, /third-party origin/); + assert.equal(out.error, + `it points to https://cdn.example.com, not to the "srv" server's own address (${ORIGIN}), and this host fetches only from the server itself`); + assert.equal(out.remedy, NOTHING_TO_DO); +}); + +test('an http: reference from a server dialed over TLS is refused as another origin, with nothing dialed', async () => { + // The same host and port, so only the scheme differs: the origin check, which + // compares origins with their schemes, is all that refuses it. + const reg = new ReferenceRegistry(); + const f = makeFetcher({ srv: { url: `wss://127.0.0.1:${port}` } }); + const rec = makeRecord(reg, `${ORIGIN}/ok?plaintext-from-tls`); + const out = await f.fetch(rec); + assert.equal(out.error, + `it points to ${ORIGIN}, not to the "srv" server's own address (https://127.0.0.1:${port}), and this host fetches only from the server itself`); + assert.equal(out.remedy, NOTHING_TO_DO); + assert.equal(hits.get('/ok?plaintext-from-tls') ?? 0, 0, 'nothing dialed'); }); test('vector 16: expired (and unparseable-expiry) references refuse locally, and take no eager slot', async () => { const reg = new ReferenceRegistry(); const f = makeFetcher({ srv: { url: ORIGIN } }); - for (const [expiresAt, why] of [['2001-01-01T00:00:00Z', /^reference expired$/], ['not-a-date', /^reference treated as expired, since its expiry time can't be read$/]] as const) { + for (const [expiresAt, why] of [['2001-01-01T00:00:00Z', /^the reference is expired$/], ['not-a-date', /^the reference is treated as expired, since its expiry time can't be read$/]] as const) { // Small, sized and from the dialed origin: only its expiry keeps it from the eager default. const rec = makeRecord(reg, `${ORIGIN}/ok?e=${expiresAt}`, { expiresAt, sizeBytes: PAYLOAD.length }); assert.equal(f.isEagerEligible(rec), false, `${expiresAt}: the eager default would only be refused`); const out = await f.fetch(rec); assert.equal(out.ok, false); - assert.match(out.error!, /expired/); assert.match(out.error!, why); + assert.equal(out.remedy, FRESH_REFERENCE); } }); @@ -171,7 +223,7 @@ test('vector 16: an expiry rejected as a field refuses locally, with nothing dia assert.equal(f.isEagerEligible(rec), false, `${label}: the eager default would only be refused`); const out = await f.fetch(rec); assert.equal(out.ok, false, label); - assert.equal(out.error, "reference treated as expired, since its expiry time isn't in a form this host accepts", label); + assert.equal(out.error, "the reference is treated as expired, since its expiry time isn't in a form this host accepts", label); } assert.equal(saved.length, before, 'nothing saved'); assert.equal(lastAuth, 'untouched', 'the origin was never dialed'); @@ -378,11 +430,10 @@ for (const expired of [true, false]) { }); } -/** A refusal by the budget says it lasts, so a second try is no use unless - * whoever runs the host raises or renews it. */ -const BUDGET_LASTS = "the budget doesn't renew on its own, so calling again won't change that unless whoever runs this host raises or renews it"; /** The budget's stop partway, with the server's running total and the budget. */ -const STOPPED_BY_BUDGET = new RegExp(`^aborted: per-server fetch budget exhausted \\((\\d+)/(\\d+) bytes\\); ${BUDGET_LASTS}$`); +const STOPPED_BY_BUDGET = /^this host's fetch budget for the "srv" server ran out during the download \((\d+) of (\d+) bytes\), so it was stopped and nothing was kept$/; +/** The budget's refusal before dialing, with what's used of it. */ +const usedUp = (used: number | string, budget: number | string) => `this host's fetch budget for the "srv" server is used up (${used} of ${budget} bytes)`; test('per-server budget exhausts cumulatively, each fetch counting once', async () => { const reg = new ReferenceRegistry(); @@ -394,7 +445,10 @@ test('per-server budget exhausts cumulatively, each fetch counting once', async assert.equal(b.ok, true, b.error); const c = await f.fetch(makeRecord(reg, `${ORIGIN}/ok?n=3`)); assert.equal(c.ok, false); - assert.equal(c.error, `per-server fetch budget exhausted (${2 * PAYLOAD.length}/${2 * PAYLOAD.length} bytes); ${BUDGET_LASTS}`); + assert.equal(c.error, usedUp(2 * PAYLOAD.length, 2 * PAYLOAD.length)); + // A refusal by the budget says it lasts, so a second try is no use unless + // whoever runs the host raises or renews it. + assert.equal(c.remedy, BUDGET); }); test("a download the budget stops says so, and what arrived counts toward the next fetch's refusal", async () => { @@ -405,8 +459,9 @@ test("a download the budget stops says so, and what arrived counts toward the ne const [, total, budget] = STOPPED_BY_BUDGET.exec(out.error!) ?? []; assert.ok(Number(total) > 1000, out.error); assert.equal(budget, '1000'); + assert.equal(out.remedy, BUDGET); const next = await f.fetch(makeRecord(reg, `${ORIGIN}/ok?after-budget-abort`)); - assert.equal(next.error, `per-server fetch budget exhausted (${total}/1000 bytes); ${BUDGET_LASTS}`); + assert.equal(next.error, usedUp(total, 1000)); }); test('a budget crossed on a later chunk stops the download there, with every chunk counted', async () => { @@ -433,14 +488,15 @@ test('concurrent fetches from one server share its budget, and none takes what a for (const o of refused) assert.match(o.error!, STOPPED_BY_BUDGET); // Every byte read counts, so a fifth is refused before it dials. const fifth = await f.fetch(makeRecord(reg, `${ORIGIN}/ok?shared-5`)); - assert.match(fifth.error!, /^per-server fetch budget exhausted/); + assert.match(fifth.error!, /^this host's fetch budget for the "srv" server is used up /); }); test("bytes the workspace couldn't store still count against the budget", async () => { const reg = new ReferenceRegistry(); const unsaved = new ReferenceFetcher(() => ({ url: ORIGIN, autofetch: { maxTotalBytes: PAYLOAD.length + 100 } }), async () => null); const notStored = await unsaved.fetch(makeRecord(reg, `${ORIGIN}/ok?unsaved-1`)); - assert.equal(notStored.error, 'no writable workspace mount to store the payload'); + assert.equal(notStored.error, "it was fetched, but this host couldn't store it in its workspace, so nothing was kept"); + assert.equal(notStored.remedy, 'Calling again is likely to fail the same way; whoever runs this host can see why in its log.'); assert.match((await unsaved.fetch(makeRecord(reg, `${ORIGIN}/ok?unsaved-2`))).error!, STOPPED_BY_BUDGET); }); @@ -448,7 +504,7 @@ test("bytes that don't match their digest still count against the budget", async const reg = new ReferenceRegistry(); const f = makeFetcher({ srv: { url: ORIGIN, autofetch: { maxTotalBytes: PAYLOAD.length + 100 } } }); const mismatch = await f.fetch(makeRecord(reg, `${ORIGIN}/ok?mismatch-charged`, { digest: 'sha256:' + 'B'.repeat(43) })); - assert.match(mismatch.error!, /^digest mismatch/); + assert.match(mismatch.error!, /^the bytes that arrived don't match the reference's digest /); const after = await f.fetch(makeRecord(reg, `${ORIGIN}/ok?after-mismatch`)); // The stop shows the server's total, the mismatched payload included, not // this download's own bytes. @@ -463,7 +519,8 @@ test('bytes that arrived before a timeout still count against the budget', async // 1000 bytes arrive at once, and the rest only after the fetch gives up. // The timeout leaves a second for the headers and the first 1000 bytes. const slow = await f.fetch(makeRecord(reg, `${ORIGIN}/drip?first=1000&rest=1000&wait=60000`), { timeoutMs: 1000 }); - assert.equal(slow.error, 'fetch timed out or aborted'); + assert.equal(slow.error, 'it took longer than 1 second'); + assert.equal(slow.remedy, TRY_LATER); assert.match((await f.fetch(makeRecord(reg, `${ORIGIN}/ok?after-timeout`))).error!, STOPPED_BY_BUDGET); }); @@ -473,7 +530,10 @@ test('bytes past the ceiling still count against the budget', async () => { // 1200 bytes arrive before the pause: past max_bytes, within the budget. A // payload alone fits what's left only if what was read went uncounted. const out = await f.fetch(makeRecord(reg, `${ORIGIN}/drip?first=1200&rest=5000&wait=10000`), { maxBytes: 1000 }); - assert.match(out.error!, /^aborted: actual bytes exceeded ceiling at \d+$/); + const [, received] = /^the download had reached (\d+) bytes, past the 1000 bytes this call allows, so it was stopped and nothing was kept$/.exec(out.error!) ?? []; + assert.ok(received, out.error); + // A larger max_bytes can read no more than the budget has left after these bytes. + assert.equal(out.remedy, `It's at least ${received} bytes; a larger max_bytes, up to ${PAYLOAD.length + 500 - Number(received)}, may fetch it.`); assert.match((await f.fetch(makeRecord(reg, `${ORIGIN}/ok?after-ceiling`))).error!, STOPPED_BY_BUDGET); }); @@ -501,11 +561,13 @@ test("a claim over max_bytes and over what's left of the budget is refused for t assert.equal((await f.fetch(makeRecord(reg, `${ORIGIN}/ok?spend-most`))).ok, true); // 100 bytes are left: a larger max_bytes wouldn't let this claim through. const out = await f.fetch(makeRecord(reg, `${ORIGIN}/ok?claim-past-both`, { sizeBytes: 10_000 }), { maxBytes: 1000 }); - assert.equal(out.error, `claimed 10000 bytes exceeds what's left of the per-server fetch budget (${PAYLOAD.length}/${PAYLOAD.length + 100} bytes); ${BUDGET_LASTS}`); + assert.equal(out.error, `it claims 10000 bytes, more than the 100 bytes left of this host's fetch budget for the "srv" server`); + assert.equal(out.remedy, BUDGET); // Within what's left, the same claim is refused for the ceiling, which a larger max_bytes lifts. const roomy = makeFetcher({ srv: { url: ORIGIN } }); const ceilingOnly = await roomy.fetch(makeRecord(reg, `${ORIGIN}/ok?claim-past-ceiling`, { sizeBytes: 10_000 }), { maxBytes: 1000 }); - assert.equal(ceilingOnly.error, 'claimed 10000 bytes exceeds fetch ceiling 1000'); + assert.equal(ceilingOnly.error, 'it claims 10000 bytes, over the 1000 bytes this call allows'); + assert.equal(ceilingOnly.remedy, 'Pass max_bytes of at least 10000 to fetch it.'); }); test("a claim over what's left of the budget but within max_bytes still dials, since a claim can be wrong", async () => { @@ -522,7 +584,7 @@ test('a claim over max_bytes, once the budget is spent, is refused for the budge const f = makeFetcher({ srv: { url: ORIGIN, autofetch: { maxTotalBytes: PAYLOAD.length } } }); assert.equal((await f.fetch(makeRecord(reg, `${ORIGIN}/ok?spend`))).ok, true); const out = await f.fetch(makeRecord(reg, `${ORIGIN}/ok?claim-over`, { sizeBytes: 10_000 }), { maxBytes: 1000 }); - assert.equal(out.error, `per-server fetch budget exhausted (${PAYLOAD.length}/${PAYLOAD.length} bytes); ${BUDGET_LASTS}`); + assert.equal(out.error, usedUp(PAYLOAD.length, PAYLOAD.length)); }); test('eager eligibility: small + sized + connection origin only', () => { @@ -590,7 +652,8 @@ test('fetch_reference answers an unbound reference with the reason its stub give const { result } = await answered; assert.equal(result.success, false); assert.equal(result.isError, true); - assert.equal(result.error, `reference ${UNBOUND_REASON}`); + // The fetcher's clause and remedy, in fetch_reference's sentence. + assert.equal(result.error, `fetch_reference couldn't fetch "${record.refId}": it's ${UNBOUND_REASON}. ${FRESH_REFERENCE}`); assert.equal(hits.get('/ok?unbound') ?? 0, 0, 'nothing dialed'); }); @@ -602,7 +665,8 @@ test('a reference both unbound and expired is refused as unbound, the reason its assert.equal(referenceExpiry(t), 'passed'); const out = await makeFetcher({ srv: { url: ORIGIN } }).fetch(reg.register(t)); assert.equal(out.ok, false); - assert.equal(out.error, `reference ${UNBOUND_REASON}`); + assert.equal(out.error, `it's ${UNBOUND_REASON}`); + assert.equal(out.remedy, FRESH_REFERENCE); assert.equal(hits.get('/ok?unbound-expired') ?? 0, 0, 'nothing dialed'); }); @@ -639,15 +703,16 @@ test('a uri on none of its servers\' origins is refused with the first server\'s assert.equal(f.isEagerEligible(record), false); const out = await f.fetch(record); assert.equal(out.ok, false); - assert.match(out.error ?? '', new RegExp(`^third-party origin \\(http://127\\.0\\.0\\.1:\\d+ != dialed https://${order[0]}\\.example\\.invalid\\)`)); + assert.equal(out.error, + `it points to ${ORIGIN}, not to the "${order[0]}" server's own address (https://${order[0]}.example.invalid), and this host fetches only from the server itself`); assert.equal(hits.get(`/ok?none-on-origin=${order[0]}`) ?? 0, 0, 'nothing dialed'); } }); test('a server unknown to the host 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 [ - [['gone', 'a'], /^unknown server: gone$/], - [['a', 'gone'], /^third-party origin \(http:\/\/127\.0\.0\.1:\d+ != dialed https:\/\/a\.example\.invalid\)/], + [['gone', 'a'], /^this host no longer has the "gone" server it came from$/], + [['a', 'gone'], /^it points to http:\/\/127\.0\.0\.1:\d+, not to the "a" server's own address \(https:\/\/a\.example\.invalid\)/], ] as const) { const reg = new ReferenceRegistry(); const f = makeFetcher({ a: { url: 'https://a.example.invalid' } }); @@ -682,7 +747,10 @@ test('a server unknown to the host is its refusal while it is the only one, and const f = makeFetcher({ owner: { url: `ws://127.0.0.1:${port}` } }); const uri = `${ORIGIN}/ok?unknown-first`; const record = makeRecord(reg, uri, {}, 'gone'); - assert.equal((await f.fetch(record)).error, 'unknown server: gone'); + const gone = await f.fetch(record); + assert.equal(gone.error, 'this host no longer has the "gone" server it came from'); + // The resolver reads the host's configs at each fetch, so the server's return would change it. + assert.equal(gone.remedy, SERVER_BACK); makeRecord(reg, uri, {}, 'owner'); assert.deepEqual(record.serverIds, ['gone', 'owner']); const out = await f.fetch(record); @@ -695,8 +763,8 @@ test('a server with no address is refused in a phrase naming it, and passed over 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.deepEqual(f.refusal(record), { error: noAddressReason('stdio-srv'), remedy: NOTHING_TO_DO }); + assert.equal((await f.fetch(record)).error, "this host doesn't connect to the \"stdio-srv\" server 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'); @@ -708,15 +776,15 @@ test('a server with no address is refused in a phrase naming it, and passed over 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\)/], + [['stdio-srv', 'a'], /^this host doesn't connect to the "stdio-srv" server over a network/], + [['a', 'stdio-srv'], /^it points to http:\/\/127\.0\.0\.1:\d+, not to the "a" server's own address \(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(f.refusal(record)?.error ?? '', 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'); } @@ -732,28 +800,30 @@ test('refusal() answers what the fetch would refuse before dialing, case by case 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]> = [ + // Each with what it says happened, and what it says calling again can do. + const cases: Array<[string, ReturnType, string, string]> = [ // 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'], + ['unbound', reg.register((classifyBlock({ type: 'resource', uri: `${ORIGIN}/ok?case=unbound`, ...small }) as Extract, { kind: 'reference' }>).testimony), + `it's ${UNBOUND_REASON}`, FRESH_REFERENCE], + ['unknown server', makeRecord(reg, `${ORIGIN}/ok?case=unknown`, small, 'gone'), 'this host no longer has the "gone" server it came from', SERVER_BACK], + ['no address', makeRecord(reg, `${ORIGIN}/ok?case=no-address`, small, 'stdio-srv'), noAddressReason('stdio-srv'), NOTHING_TO_DO], + ['off the origin', makeRecord(reg, 'https://cdn.example.invalid/case-origin', small, 'srv'), + `it points to https://cdn.example.invalid, not to the "srv" server's own address (${ORIGIN}), and this host fetches only from the server itself`, NOTHING_TO_DO], + ['expired', makeRecord(reg, `${ORIGIN}/ok?case=expired`, { ...small, expiresAt: '2001-01-01T00:00:00Z' }, 'srv'), `the reference is ${EXPIRY_REASON.passed}`, FRESH_REFERENCE], + ['budget spent', makeRecord(reg, `${ORIGIN}/ok?case=budget`, small, 'spent'), 'this host\'s fetch budget for the "spent" server is used up (0 of 0 bytes)', BUDGET], + ['over the ceiling', makeRecord(reg, `${ORIGIN}/ok?case=ceiling`, { sizeBytes: 512 * 1024 * 1024 }, 'roomy'), + 'it claims 536870912 bytes, over the 268435456 bytes this host fetches at most', HOST_CEILING], ['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}`], + 'it claims 2147483648 bytes, more than the 1073741824 bytes left of this host\'s fetch budget for the "roomy" server', BUDGET], ]; - for (const [what, record, error] of cases) { + for (const [what, record, error, remedy] 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; + assert.deepEqual(refusal, { error, remedy }, what); 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.deepEqual({ error: out.error, remedy: out.remedy }, refusal, `${what}: the fetch answers what refusal() said`); } 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. @@ -777,13 +847,13 @@ test('a reference with two faults is refused for the first the fetch checks: the 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}`], + ['expired, budget spent, over the ceiling', makeRecord(reg, `${ORIGIN}/ok?two=2`, { expiresAt: past, sizeBytes: huge }, 'spent'), `the reference is ${EXPIRY_REASON.passed}`], + ['budget spent, over the ceiling', makeRecord(reg, `${ORIGIN}/ok?two=3`, { sizeBytes: huge }, 'spent'), 'this host\'s fetch budget for the "spent" server is used up (0 of 0 bytes)'], ]; 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(refusal.error, error, what); assert.equal((await f.fetch(record)).error, error, what); } }); @@ -855,7 +925,7 @@ test('a fetch is charged to the budget of the server it dials, not of the first assert.equal(first.ok, true, first.error); // That spent the owner's budget, so its own next reference is refused for it. const next = await f.fetch(makeRecord(reg, `${ORIGIN}/ok?charged-own`, {}, 'owner')); - assert.equal(next.error, `per-server fetch budget exhausted (${PAYLOAD.length}/${PAYLOAD.length} bytes); ${BUDGET_LASTS}`); + assert.equal(next.error, `this host's fetch budget for the "owner" server is used up (${PAYLOAD.length} of ${PAYLOAD.length} bytes)`); assert.equal(hits.get('/ok?charged-own') ?? 0, 0, 'nothing dialed'); }); @@ -885,3 +955,86 @@ test('vector 13: magic bytes beat a lying Content-Type header for storage type', assert.equal(out.mimeType, 'audio/wav', 'sniffed type must win'); assert.ok(rec.fetchedPath!.endsWith('.wav'), rec.fetchedPath); }); + +test("each HTTP status says what calling again can do: later, a fresh reference, the host's credentials, or nothing", async () => { + const reg = new ReferenceRegistry(); + const f = makeFetcher({ srv: { url: ORIGIN } }); + const expect: Array<[number, string]> = [ + // Passing trouble, which can clear. + [408, TRY_LATER], [429, TRY_LATER], [500, TRY_LATER], [503, TRY_LATER], + // The server doesn't support the request, which later won't change. + [501, NOTHING_TO_DO], [505, NOTHING_TO_DO], + // This host's credentials for the server, which a fresh reference keeps. + [401, "Calling again won't change that; whoever runs this host can check its credentials for the server."], + [403, "Once a fresh reference to it arrives, fetching may work; if that's refused too, whoever runs this host can check its credentials for the server."], + // About this reference, which a fresh one may not share. + [404, FRESH_REFERENCE], [410, FRESH_REFERENCE], + ]; + for (const [status, remedy] of expect) { + const out = await f.fetch(makeRecord(reg, `${ORIGIN}/status/${status}`)); + assert.equal(out.error, `the "srv" server answered HTTP ${status}`, String(status)); + assert.equal(out.remedy, remedy, String(status)); + } + // A success with no body at all. + const empty = await f.fetch(makeRecord(reg, `${ORIGIN}/status/204`)); + assert.equal(empty.error, 'the "srv" server answered without any content'); + assert.equal(empty.remedy, FRESH_REFERENCE); +}); + +test('a fetch that meets no listener says the request failed, with its cause, and that a later try may work', async (t) => { + const reg = new ReferenceRegistry(); + // A port just freed refuses: fetch's "fetch failed" carries the refusal as its cause. + const freed = createServer(); + await new Promise((r) => freed.listen(0, '127.0.0.1', r)); + const deadPort = (freed.address() as { port: number }).port; + await new Promise((r) => freed.close(() => r())); + const f = makeFetcher({ dead: { url: `ws://127.0.0.1:${deadPort}` } }); + const out = await f.fetch(makeRecord(reg, `http://127.0.0.1:${deadPort}/x`, {}, 'dead')); + assert.equal(out.error, `the request to the "dead" server failed: connect ECONNREFUSED 127.0.0.1:${deadPort}`); + assert.equal(out.remedy, TRY_LATER); + // A host with several addresses is refused at each, and its cause, an + // AggregateError, has no words of its own: each attempt's stand in. + t.mock.method(globalThis, 'fetch', async () => { + const attempts = [new Error('connect ECONNREFUSED ::1:4321'), new Error('connect ECONNREFUSED 127.0.0.1:4321')]; + throw new TypeError('fetch failed', { cause: Object.assign(new AggregateError(attempts), { code: 'ECONNREFUSED' }) }); + }); + const both = await makeFetcher({ dual: { url: 'ws://dual.example.invalid:4321' } }) + .fetch(makeRecord(reg, 'http://dual.example.invalid:4321/x', {}, 'dual')); + assert.equal(both.error, 'the request to the "dual" server failed: connect ECONNREFUSED ::1:4321; connect ECONNREFUSED 127.0.0.1:4321'); + assert.equal(both.remedy, TRY_LATER); +}); + +test("a save that throws is the workspace's failure, logged for whoever runs the host, not the fetch's", async (t) => { + const errors = t.mock.method(console, 'error', () => {}); + const reg = new ReferenceRegistry(); + const f = new ReferenceFetcher(() => ({ url: ORIGIN }), async () => { throw new Error('disk on fire'); }); + const rec = makeRecord(reg, `${ORIGIN}/ok?save-throws`); + const out = await f.fetch(rec); + assert.equal(out.error, "it was fetched, but this host couldn't store it in its workspace, so nothing was kept"); + assert.equal(out.remedy, 'Calling again is likely to fail the same way; whoever runs this host can see why in its log.'); + assert.deepEqual(errors.mock.calls.map((c) => String(c.arguments[0])), [`[refs] couldn't store ${rec.refId}.wav: disk on fire`]); + assert.equal(rec.fetchedPath, undefined); +}); + +test("an address that doesn't parse is refused before anything is dialed: the reference's, or the server's as no address", async () => { + const reg = new ReferenceRegistry(); + const bad = await makeFetcher({ srv: { url: ORIGIN } }).fetch(makeRecord(reg, 'not a url at all')); + assert.equal(bad.error, "its address doesn't parse as a URL"); + assert.equal(bad.remedy, NOTHING_TO_DO); + const misconfigured = await makeFetcher({ srv: { url: 'not a url either' } }).fetch(makeRecord(reg, `${ORIGIN}/ok?misconfigured`)); + assert.equal(misconfigured.error, noAddressReason('srv')); + assert.equal(misconfigured.remedy, NOTHING_TO_DO); + assert.equal(hits.get('/ok?misconfigured') ?? 0, 0, 'nothing dialed'); +}); + +test("past what's left of the server's budget, a download stopped at max_bytes is told the budget is the limit", async () => { + const reg = new ReferenceRegistry(); + const f = makeFetcher({ srv: { url: ORIGIN, autofetch: { maxTotalBytes: 1500 } } }); + // 1200 bytes arrive before the pause: past max_bytes, within the budget, + // which then has less left than what's known of the payload. + const out = await f.fetch(makeRecord(reg, `${ORIGIN}/drip?first=1200&rest=5000&wait=10000`), { maxBytes: 1000 }); + const [, received] = /^the download had reached (\d+) bytes, past the 1000 bytes this call allows, so it was stopped and nothing was kept$/.exec(out.error!) ?? []; + assert.ok(received, out.error); + assert.equal(out.remedy, + `It's at least ${received} bytes, more than the ${1500 - Number(received)} bytes left of this host's fetch budget for the "srv" server. ${BUDGET}`); +}); diff --git a/test/save-recent-image.test.ts b/test/save-recent-image.test.ts index 3b7eec8b8..91bc553c8 100644 --- a/test/save-recent-image.test.ts +++ b/test/save-recent-image.test.ts @@ -582,7 +582,7 @@ describe('save_recent_image provenance (issue #104)', () => { const refused = await waitForToolResult(h.framework, 'call_save0'); assert.strictEqual(refused.isError, true); assert.match(refused.content, new RegExp(`index 0 is a reference \\(${expired.refId}, image/png, from`)); - assert.match(refused.content, /It can't be fetched either — expired\./); + assert.match(refused.content, /It can't be fetched either — the reference is expired\. Once a fresh reference to it arrives, fetching may work; until then, fetching it again won't\./); assert.ok(!refused.content.includes('fetch_reference'), refused.content); const keptRefusal = await waitForToolResult(h.framework, 'call_save1'); assert.strictEqual(keptRefusal.isError, true); @@ -628,7 +628,9 @@ describe('save_recent_image provenance (issue #104)', () => { const refused = await waitForToolResult(h.framework, 'call_save0'); assert.strictEqual(refused.isError, true); assert.match(refused.content, new RegExp(`index 0 is a reference \\(${unbound.refId}, image/png, from`)); - assert.ok(refused.content.includes(`It can't be fetched either — ${UNBOUND_REASON}.`), refused.content); + assert.ok(refused.content.includes( + `It can't be fetched either — it's ${UNBOUND_REASON}. Once a fresh reference to it arrives, fetching may work; until then, fetching it again won't.`, + ), refused.content); assert.ok(!refused.content.includes('fetch_reference'), refused.content); const boundRefusal = await waitForToolResult(h.framework, 'call_save1'); assert.strictEqual(boundRefusal.isError, true); @@ -682,13 +684,18 @@ describe('save_recent_image provenance (issue #104)', () => { }, }); 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"; + // The fetcher's clause, then what calling again can do, as fetch_reference would say them. + const nothing = "Calling again won't change that."; 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}.`], + ['call_save0', noAddress.refId, `It can't be fetched either — ${noAddressReason('stdio')}. ${nothing}`], + ['call_save1', offOrigin.refId, "It can't be fetched either — it points to https://cdn.example.invalid, not to the \"vst\" server's own address " + + `(https://example.invalid), and this host fetches only from the server itself. ${nothing}`], + ['call_save2', gone.refId, "It can't be fetched either — this host no longer has the \"gone\" server it came from. " + + "Calling again won't change that unless whoever runs this host adds that server back."], + ['call_save3', big.refId, "It can't be fetched either — it claims 536870912 bytes, over the 268435456 bytes this host fetches at most. " + + "No max_bytes or setting lifts that limit, so calling again won't change it."], + ['call_save4', spent.refId, "It can't be fetched either — this host's fetch budget for the \"spent\" server is used up (0 of 0 bytes). " + + "The budget doesn't renew on its own, so calling again won't change that unless whoever runs this host raises or renews it."], ] as const; for (const [call, refId, reason] of expected) { const refused = await waitForToolResult(h.framework, call); diff --git a/test/tool-result-reference-uri.test.ts b/test/tool-result-reference-uri.test.ts index c16402d29..6f9f82dbb 100644 --- a/test/tool-result-reference-uri.test.ts +++ b/test/tool-result-reference-uri.test.ts @@ -337,6 +337,6 @@ describe('a script given such data', () => { } assert.ok(!text.includes(SIGNATURE), text); assert.match(text, /^zz-text\n\[ref_[^\]]+\] zz\.pdf — application\/pdf — from tool result/); - assert.ok(logged.some((line) => /\[rfc005\] script-demand fetch of ref_\S+ failed: reference not tied to any server, so there's nowhere to fetch it from/.test(line)), logged.join('\n')); + assert.ok(logged.some((line) => /\[rfc005\] script-demand fetch of ref_\S+ failed: it's not tied to any server, so there's nowhere to fetch it from$/.test(line)), logged.join('\n')); }); }); From de7741c51743e80dc979830734d2c7915251f51c Mon Sep 17 00:00:00 2001 From: Cordelia-2434 Date: Sun, 18 Oct 2026 11:45:23 -0400 Subject: [PATCH 2/5] test(mcpl): the save hook's log says why a fetched payload wasn't stored, and whyFetchFailed drops a branch nothing reaches The storage failure's remedy tells the agent whoever runs the host can see why in its log, so a test now pins each null the framework's save hook returns to its log line: no workspace, no writable mount, the write's error. It runs through the framework's own fetcher and fetch_reference. whyFetchFailed's fallback to an AggregateError's code had no case that reaches it (an AggregateError from net carries its attempts), so it goes: with no words and no attempts, the outer error's words stand. Co-Authored-By: Claude Opus 5.5 --- src/mcpl/reference-fetcher.ts | 6 ++--- test/mcpl-reference-fetcher.test.ts | 35 +++++++++++++++++++++++++++++ 2 files changed, 37 insertions(+), 4 deletions(-) diff --git a/src/mcpl/reference-fetcher.ts b/src/mcpl/reference-fetcher.ts index daddc6116..95ed37197 100644 --- a/src/mcpl/reference-fetcher.ts +++ b/src/mcpl/reference-fetcher.ts @@ -145,15 +145,13 @@ function largerMaxBytes(received: number, left: number, server: string): string /** What a thrown fetch says went wrong. fetch's own "fetch failed" says only * that, so its cause's words come first. A cause from trying each of a * host's addresses (an AggregateError) has no words of its own, so its - * attempts' stand in, then its code. */ + * attempts' stand in. */ function whyFetchFailed(e: unknown): string { const source = (e as { cause?: unknown } | null | undefined)?.cause ?? e; const own = describeCause(source); if (own !== '') return own; const attempts = (source as { errors?: unknown } | null | undefined)?.errors; - if (Array.isArray(attempts) && attempts.length > 0) return attempts.map(describeCause).join('; '); - const code = (source as { code?: unknown } | null | undefined)?.code; - return typeof code === 'string' ? code : describeCause(e); + return Array.isArray(attempts) && attempts.length > 0 ? attempts.map(describeCause).join('; ') : describeCause(e); } /** Magic-byte sniff for the formats this pipeline actually moves. The diff --git a/test/mcpl-reference-fetcher.test.ts b/test/mcpl-reference-fetcher.test.ts index 9646d2f2e..45b900aac 100644 --- a/test/mcpl-reference-fetcher.test.ts +++ b/test/mcpl-reference-fetcher.test.ts @@ -1016,6 +1016,41 @@ test("a save that throws is the workspace's failure, logged for whoever runs the assert.equal(rec.fetchedPath, undefined); }); +test("a payload fetched but not stored says so, and the host's log says why: no workspace, no writable mount, or the write's error", async (t) => { + const errors = t.mock.method(console, 'error', () => {}); + type Answer = { result: { success: boolean; error?: string } }; + const cases: Array<[string, Record, (file: string) => string]> = [ + ['no workspace', { getWorkspaceModule: () => undefined }, (file) => `[refs] couldn't store ${file}: this host has no workspace`], + ['no writable mount', { getWorkspaceModule: () => ({}), firstWritableMountName: () => null }, + (file) => `[refs] couldn't store ${file}: no workspace mount is writable`], + ['a failed write', { + getWorkspaceModule: () => ({ writeBinary: async () => ({ success: false, error: 'EROFS: read-only file system' }) }), + firstWritableMountName: () => 'scratch', + }, (file) => `[refs] couldn't store scratch/refs/${file}: EROFS: read-only file system`], + ]; + for (const [what, workspace, logLine] of cases) { + // The framework's own fetcher and save hook, with this file's server as an MCPL server's address. + 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([['srv', { id: 'srv', url: `ws://127.0.0.1:${port}` }]]), + emitTrace: () => {}, + ...workspace, + }); + // Bytes no sniffer knows, with no type, so the file is saved as .bin. + const record = makeRecord(referenceRegistry, `${ORIGIN}/untyped?unstored=${encodeURIComponent(what)}`); + const answered = new Promise((resolve) => { framework.pushEvent = (event) => resolve(event as Answer); }); + framework.dispatchFetchReferenceToolCall('prime', { id: `call_${what}`, name: 'fetch_reference', input: { ref_id: record.refId } }); + const { result } = await answered; + assert.equal(result.error, `fetch_reference couldn't fetch "${record.refId}": it was fetched, but this host couldn't store it in its workspace, ` + + 'so nothing was kept. Calling again is likely to fail the same way; whoever runs this host can see why in its log.', what); + const logged = errors.mock.calls.map((c) => String(c.arguments[0])); + assert.ok(logged.includes(logLine(`${record.refId}.bin`)), `${what}: ${logged.join('\n')}`); + } +}); + test("an address that doesn't parse is refused before anything is dialed: the reference's, or the server's as no address", async () => { const reg = new ReferenceRegistry(); const bad = await makeFetcher({ srv: { url: ORIGIN } }).fetch(makeRecord(reg, 'not a url at all')); From b9e51557e008a613425fa16d53b62f417a5c0f09 Mon Sep 17 00:00:00 2001 From: Cordelia-2434 Date: Sun, 18 Oct 2026 12:21:08 -0400 Subject: [PATCH 3/5] fix(mcpl): fetch failures name the limit nothing lifts first, and a failed request's remedy holds for every cause What an internal review of this change found: - A thrown fetch's remedy now holds whatever the cause: "Trying again later may work; if it fails the same way, whoever runs this host can check its connection to the server." A cause that repeats on every try (a port fetch refuses, a name that doesn't resolve, a certificate this host doesn't trust) was told only that a later try may work. - A claim past the host's own 256 MiB is refused for that first, before the server's budget, since nothing lifts it, not even a budget whoever runs the host raises. A download past it is told the same. - A server address that doesn't parse gets a clause of its own, where it read as a server this host doesn't connect to over a network. - The origin clause names the server's address as it's configured (wss://...), not its normalized origin (https://...); the comparison still reads ws as http and wss as https. - A digest that doesn't match tells the agent that fetching may work once a fresh reference arrives, which refreshes this very record, where it said the reference can't be fetched here. - The save-throw log line bounds the cause, as the agent's text does. - Comments: noAddressReason's and beforeDial's say what each covers, and the ws: scheme check says fetch refuses such a uri before connecting. Tests: the refusal-order test puts a claim past the host's ceiling before a spent budget; the request-failure tests, the origin clauses and the digest remedy take the new words; the save-throw test's cause carries a newline; the two ceiling-remedy tests pin the reached count between max_bytes and the bytes sent before the pause. Co-Authored-By: Claude Opus 5.5 --- changelog.d/fetch-reference-words.changed.md | 4 +- src/mcpl/reference-fetcher.ts | 56 ++++++++++++-------- src/mcpl/references.ts | 7 +-- test/mcpl-reference-fetcher.test.ts | 32 ++++++----- test/save-recent-image.test.ts | 2 +- 5 files changed, 62 insertions(+), 39 deletions(-) diff --git a/changelog.d/fetch-reference-words.changed.md b/changelog.d/fetch-reference-words.changed.md index 51ae5c1c5..fbf48fe0f 100644 --- a/changelog.d/fetch-reference-words.changed.md +++ b/changelog.d/fetch-reference-words.changed.md @@ -1,6 +1,6 @@ - `fetch_reference`'s failures now say what happened to the reference and what calling again can do: `fetch_reference couldn't fetch "": . `, where they gave the fetcher's reason alone, such as `HTTP 404`, `fetch timed out or aborted`, `claimed bytes exceeds fetch ceiling ` or `third-party origin ( != dialed ); not fetched by this host`. Where `save_recent_image` says a reference can't be fetched, it gives the same two. - **What happened** names the server and what it answered (`the "" server answered HTTP 503`), the limit a fetch met (`it claims bytes, over the bytes this call allows`, `the download had reached bytes, past …`), what kept a fetch from dialing (`it points to , not to the "" server's own address (), and this host fetches only from the server itself`), or that what arrived didn't match the reference's digest or couldn't be stored. The eager fetch's and a script's fetch's log lines read the same words. - - **What calling again can do**, such as trying again later (after a timeout, a failed request, 408, 429, or a 5xx other than 501 and 505), waiting for a fresh reference to it (an expired or unbound reference, and the other 4xx), passing a larger `max_bytes` (naming the size it needs, and no more than the server's fetch budget has left), asking whoever runs the host (who can check its credentials for the server after a 401 or 403, raise or renew a spent budget, or add back a server it no longer has), or nothing, as for an address on another origin, a server this host doesn't connect to over a network, or a claim past the 256 MiB this host fetches at most, which no `max_bytes` or setting lifts. + - **What calling again can do**, such as trying again later (after a timeout, 408, 429, or a 5xx other than 501 and 505, and after a failed request, with whoever runs the host to check its connection to the server if it fails the same way), waiting for a fresh reference to it (an expired or unbound reference, and the other 4xx), passing a larger `max_bytes` (naming the size it needs, and no more than the server's fetch budget has left), asking whoever runs the host (who can check its credentials for the server after a 401 or 403, raise or renew a spent budget, or add back a server it no longer has), or nothing, as for an address on another origin, a server this host doesn't connect to over a network, or a claim past the 256 MiB this host fetches at most, which no `max_bytes` or setting lifts. A claim past those 256 MiB is refused for them first, whatever the server's budget has left. - **The budget's texts** read `this host's fetch budget for the "" server is used up ( of bytes)` and, partway, `this host's fetch budget for the "" server ran out during the download ( of bytes), so it was stopped and nothing was kept`, where they read `per-server fetch budget exhausted (/ bytes)`, the one partway after `aborted: `. - - **A `ws:` or `wss:` reference** on its server's own origin is refused for its scheme before anything is dialed, where the fetch dialed it and failed as a network error. A server whose address doesn't parse is refused as one with no address. + - **A `ws:` or `wss:` reference** on its server's own origin is refused for its scheme before anything is dialed, where `fetch` refused it before connecting and the agent read `fetch failed`. A server whose address doesn't parse is refused, saying so. - **The host's log** says why a fetched payload couldn't be stored: no workspace, no writable mount, or the write's error. A save that throws is that failure too, logged, where its message reached the agent as the fetch's own error. diff --git a/src/mcpl/reference-fetcher.ts b/src/mcpl/reference-fetcher.ts index 95ed37197..f5a537660 100644 --- a/src/mcpl/reference-fetcher.ts +++ b/src/mcpl/reference-fetcher.ts @@ -56,6 +56,11 @@ const NOTHING_TO_DO = "Calling again won't change that."; * uri arrives as this very id and refreshes its record. */ const FRESH_REFERENCE = "Once a fresh reference to it arrives, fetching may work; until then, fetching it again won't."; const TRY_LATER = 'Trying again later may work.'; +/** A request that failed before an answer: a refused or reset connection + * can clear, and a cause that repeats on every try (a port fetch refuses, a + * name that doesn't resolve, a certificate this host doesn't trust) is + * whoever runs the host's to change, so the remedy names both. */ +const REQUEST_FAILED = "Trying again later may work; if it fails the same way, whoever runs this host can check its connection to the server."; const CREDENTIALS = "Calling again won't change that; whoever runs this host can check its credentials for the server."; const FRESH_OR_CREDENTIALS = "Once a fresh reference to it arrives, fetching may work; if that's refused too, whoever runs this host can check its credentials for the server."; /** The total lives as long as this fetcher, one per framework, so only @@ -137,7 +142,8 @@ function statusRemedy(status: number): string { function largerMaxBytes(received: number, left: number, server: string): string { const most = Math.min(DEFAULT_FETCH_MAX_BYTES, left); if (received <= most) return `It's at least ${received} bytes; a larger max_bytes, up to ${most}, may fetch it.`; - return left < DEFAULT_FETCH_MAX_BYTES + // Past the host's own ceiling nothing lifts it, whatever the budget has left. + return received <= DEFAULT_FETCH_MAX_BYTES ? `It's at least ${received} bytes, more than the ${left} bytes left of this host's fetch budget for ${server}. ${BUDGET}` : `It's at least ${received} bytes, more than the ${DEFAULT_FETCH_MAX_BYTES} bytes this host fetches at most. ${HOST_CEILING}`; } @@ -253,8 +259,10 @@ 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. For a record + * (none, unknown, no address, an address of its own or the reference's + * that doesn't parse, a scheme other than http: or https:, off the uri's + * origin), the expiry, a claimed size over the host's own ceiling, the + * server's spent budget, a claimed size over a call's 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 @@ -269,8 +277,13 @@ export class ReferenceFetcher { if (expiry) return refused(`the reference is ${EXPIRY_REASON[expiry]}`, FRESH_REFERENCE); 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 + // A claim past the host's own ceiling is refused first: nothing lifts + // it, not even a budget whoever runs the host raises. + if (t.sizeBytes !== undefined && t.sizeBytes > DEFAULT_FETCH_MAX_BYTES) { + return refused(`it claims ${t.sizeBytes} bytes, over the ${DEFAULT_FETCH_MAX_BYTES} bytes this host fetches at most`, HOST_CEILING); + } + // The budget is checked before a call's ceiling, here and while reading: + // a larger max_bytes lifts that 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; @@ -288,12 +301,10 @@ export class ReferenceFetcher { BUDGET, ); } - // Within what's left of the budget, so a ceiling of the call's own, - // below the host's, is what a larger max_bytes lifts. - return t.sizeBytes <= DEFAULT_FETCH_MAX_BYTES - ? refused(`it claims ${t.sizeBytes} bytes, over the ${ceiling} bytes this call allows`, - `Pass max_bytes of at least ${t.sizeBytes} to fetch it.`) - : refused(`it claims ${t.sizeBytes} bytes, over the ${DEFAULT_FETCH_MAX_BYTES} bytes this host fetches at most`, HOST_CEILING); + // Within what's left of the budget and the host's ceiling, so a + // ceiling of the call's own is what a larger max_bytes lifts. + return refused(`it claims ${t.sizeBytes} bytes, over the ${ceiling} bytes this call allows`, + `Pass max_bytes of at least ${t.sizeBytes} to fetch it.`); } return { serverId, binding, ceiling, budget }; } @@ -313,14 +324,17 @@ export class ReferenceFetcher { private originCheck(record: ReferenceRecord, binding: ResolvedServerBinding, serverId: string): { refusal: Refusal } | null { const refOrigin = normalizedOrigin(record.testimony.uri); if (!refOrigin) return refused("its address doesn't parse as a URL", NOTHING_TO_DO); - // A server's address that doesn't parse leaves nothing to dial, as for a - // server with none. The framework's resolver gives only an address its - // transport dials, so this guards other resolvers. + // A server's address that doesn't parse leaves nothing to dial. The + // framework's resolver gives only an address its transport dials, so this + // guards other resolvers. const dialedOrigin = normalizedOrigin(binding.url); - if (!dialedOrigin) return refused(noAddressReason(serverId), NOTHING_TO_DO); + if (!dialedOrigin) { + return refused(`the "${serverId}" server's address doesn't parse as a URL, so there's nowhere to fetch it from`, NOTHING_TO_DO); + } // The uri's own scheme, not its origin's: an origin reads ws: as http: - // and wss: as https:, and fetch can't dial a ws: or wss: uri, which would - // fail only once dialed, as a network error a later try can't mend. + // and wss: as https:, and fetch refuses a ws: or wss: uri ('unknown + // scheme') before connecting, which would read as a failed request a + // later try can mend. const scheme = new URL(record.testimony.uri).protocol; if (scheme !== 'https:' && scheme !== 'http:') { return refused(`its address uses ${scheme}, and this host fetches only http: and https: addresses`, NOTHING_TO_DO); @@ -328,7 +342,7 @@ export class ReferenceFetcher { // Origins compare with their schemes, so this also refuses an http: // reference from a server dialed over TLS (wss://). if (refOrigin !== dialedOrigin) { - return refused(`it points to ${refOrigin}, not to the "${serverId}" server's own address (${dialedOrigin}), ` + + return refused(`it points to ${refOrigin}, not to the "${serverId}" server's own address (${new URL(binding.url).origin}), ` + 'and this host fetches only from the server itself', NOTHING_TO_DO); } return null; @@ -409,7 +423,7 @@ export class ReferenceFetcher { return fail( `the bytes that arrived don't match the reference's digest (${t.digest.slice(0, 20)}…, against ${got.slice(0, 20)}…), ` + "so they weren't kept", - "Trying again may work; if it fails the same way, this reference can't be fetched here.", + "Trying again may work; if it fails the same way, fetching may work once a fresh reference to it arrives.", ); } digestVerified = true; @@ -433,7 +447,7 @@ export class ReferenceFetcher { try { path = await this.saveFile(fileName, data, observedMime); } catch (e) { - console.error(`[refs] couldn't store ${fileName}: ${describeCause(e)}`); + console.error(`[refs] couldn't store ${fileName}: ${boundedForLog(describeCause(e))}`); path = null; } if (path === null) { @@ -453,7 +467,7 @@ export class ReferenceFetcher { const seconds = timeoutMs / 1000; return fail(`it took longer than ${seconds} second${seconds === 1 ? '' : 's'}`, TRY_LATER); } - return fail(`the request to ${server} failed: ${boundedForLog(whyFetchFailed(e))}`, TRY_LATER); + return fail(`the request to ${server} failed: ${boundedForLog(whyFetchFailed(e))}`, REQUEST_FAILED); } finally { clearTimeout(timer); } diff --git a/src/mcpl/references.ts b/src/mcpl/references.ts index acff9ecb8..f33879c58 100644 --- a/src/mcpl/references.ts +++ b/src/mcpl/references.ts @@ -224,9 +224,10 @@ export const UNBOUND_REASON = "not tied to any server, so there's nowhere to fet /** 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, as a - * clause about the reference; 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. */ + * clause that completes "couldn't fetch : " and says so of the 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 `this host doesn't connect to the "${serverId}" server over a network, so there's nowhere to fetch it from`; } diff --git a/test/mcpl-reference-fetcher.test.ts b/test/mcpl-reference-fetcher.test.ts index 45b900aac..aeb3df1a6 100644 --- a/test/mcpl-reference-fetcher.test.ts +++ b/test/mcpl-reference-fetcher.test.ts @@ -83,6 +83,7 @@ function makeRecord(reg: ReferenceRegistry, uri: string, extra: Record { const out = await f.fetch(rec); assert.equal(out.ok, false); assert.match(out.error!, /^the bytes that arrived don't match the reference's digest \(sha256:B{13}…, against sha256:[\w-]{13}…\), so they weren't kept$/); - assert.equal(out.remedy, "Trying again may work; if it fails the same way, this reference can't be fetched here."); + assert.equal(out.remedy, "Trying again may work; if it fails the same way, fetching may work once a fresh reference to it arrives."); assert.equal(rec.fetchedPath, undefined); }); @@ -193,7 +194,7 @@ test('an http: reference from a server dialed over TLS is refused as another ori const rec = makeRecord(reg, `${ORIGIN}/ok?plaintext-from-tls`); const out = await f.fetch(rec); assert.equal(out.error, - `it points to ${ORIGIN}, not to the "srv" server's own address (https://127.0.0.1:${port}), and this host fetches only from the server itself`); + `it points to ${ORIGIN}, not to the "srv" server's own address (wss://127.0.0.1:${port}), and this host fetches only from the server itself`); assert.equal(out.remedy, NOTHING_TO_DO); assert.equal(hits.get('/ok?plaintext-from-tls') ?? 0, 0, 'nothing dialed'); }); @@ -532,6 +533,8 @@ test('bytes past the ceiling still count against the budget', async () => { const out = await f.fetch(makeRecord(reg, `${ORIGIN}/drip?first=1200&rest=5000&wait=10000`), { maxBytes: 1000 }); const [, received] = /^the download had reached (\d+) bytes, past the 1000 bytes this call allows, so it was stopped and nothing was kept$/.exec(out.error!) ?? []; assert.ok(received, out.error); + // Past max_bytes, and no more than the 1200 bytes sent before the pause, however they were chunked. + assert.ok(Number(received) > 1000 && Number(received) <= 1200, received); // A larger max_bytes can read no more than the budget has left after these bytes. assert.equal(out.remedy, `It's at least ${received} bytes; a larger max_bytes, up to ${PAYLOAD.length + 500 - Number(received)}, may fetch it.`); assert.match((await f.fetch(makeRecord(reg, `${ORIGIN}/ok?after-ceiling`))).error!, STOPPED_BY_BUDGET); @@ -808,13 +811,14 @@ test('refusal() answers what the fetch would refuse before dialing, case by case ['unknown server', makeRecord(reg, `${ORIGIN}/ok?case=unknown`, small, 'gone'), 'this host no longer has the "gone" server it came from', SERVER_BACK], ['no address', makeRecord(reg, `${ORIGIN}/ok?case=no-address`, small, 'stdio-srv'), noAddressReason('stdio-srv'), NOTHING_TO_DO], ['off the origin', makeRecord(reg, 'https://cdn.example.invalid/case-origin', small, 'srv'), - `it points to https://cdn.example.invalid, not to the "srv" server's own address (${ORIGIN}), and this host fetches only from the server itself`, NOTHING_TO_DO], + `it points to https://cdn.example.invalid, not to the "srv" server's own address (ws://127.0.0.1:${port}), and this host fetches only from the server itself`, NOTHING_TO_DO], ['expired', makeRecord(reg, `${ORIGIN}/ok?case=expired`, { ...small, expiresAt: '2001-01-01T00:00:00Z' }, 'srv'), `the reference is ${EXPIRY_REASON.passed}`, FRESH_REFERENCE], ['budget spent', makeRecord(reg, `${ORIGIN}/ok?case=budget`, small, 'spent'), 'this host\'s fetch budget for the "spent" server is used up (0 of 0 bytes)', BUDGET], ['over the ceiling', makeRecord(reg, `${ORIGIN}/ok?case=ceiling`, { sizeBytes: 512 * 1024 * 1024 }, 'roomy'), 'it claims 536870912 bytes, over the 268435456 bytes this host fetches at most', HOST_CEILING], - ['over the ceiling and what is left of the budget', makeRecord(reg, `${ORIGIN}/ok?case=both`, { sizeBytes: 2 * 1024 * 1024 * 1024 }, 'roomy'), - 'it claims 2147483648 bytes, more than the 1073741824 bytes left of this host\'s fetch budget for the "roomy" server', BUDGET], + // Past the host's own ceiling, whatever the budget has left, since no budget lifts it. + ['over the host ceiling and what is left of the budget', makeRecord(reg, `${ORIGIN}/ok?case=both`, { sizeBytes: 2 * 1024 * 1024 * 1024 }, 'roomy'), + 'it claims 2147483648 bytes, over the 268435456 bytes this host fetches at most', HOST_CEILING], ]; for (const [what, record, error, remedy] of cases) { const refusal = f.refusal(record); @@ -840,7 +844,7 @@ test('refusal() answers what the fetch would refuse before dialing, case by case 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 () => { +test('a reference with two faults is refused for the first the fetch checks: the server, then the expiry, a claim past the host\'s ceiling, the spent budget', 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'; @@ -848,7 +852,9 @@ test('a reference with two faults is refused for the first the fetch checks: the 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'), `the reference is ${EXPIRY_REASON.passed}`], - ['budget spent, over the ceiling', makeRecord(reg, `${ORIGIN}/ok?two=3`, { sizeBytes: huge }, 'spent'), 'this host\'s fetch budget for the "spent" server is used up (0 of 0 bytes)'], + // Nothing lifts the host's ceiling, so it comes before a budget whoever runs the host can raise. + ['over the host ceiling, budget spent', makeRecord(reg, `${ORIGIN}/ok?two=3`, { sizeBytes: huge }, 'spent'), 'it claims 536870912 bytes, over the 268435456 bytes this host fetches at most'], + ['budget spent, within the ceiling', makeRecord(reg, `${ORIGIN}/ok?two=4`, { sizeBytes: 1000 }, 'spent'), 'this host\'s fetch budget for the "spent" server is used up (0 of 0 bytes)'], ]; for (const [what, record, error] of cases) { const refusal = f.refusal(record); @@ -991,7 +997,7 @@ test('a fetch that meets no listener says the request failed, with its cause, an const f = makeFetcher({ dead: { url: `ws://127.0.0.1:${deadPort}` } }); const out = await f.fetch(makeRecord(reg, `http://127.0.0.1:${deadPort}/x`, {}, 'dead')); assert.equal(out.error, `the request to the "dead" server failed: connect ECONNREFUSED 127.0.0.1:${deadPort}`); - assert.equal(out.remedy, TRY_LATER); + assert.equal(out.remedy, REQUEST_FAILED); // A host with several addresses is refused at each, and its cause, an // AggregateError, has no words of its own: each attempt's stand in. t.mock.method(globalThis, 'fetch', async () => { @@ -1001,18 +1007,19 @@ test('a fetch that meets no listener says the request failed, with its cause, an const both = await makeFetcher({ dual: { url: 'ws://dual.example.invalid:4321' } }) .fetch(makeRecord(reg, 'http://dual.example.invalid:4321/x', {}, 'dual')); assert.equal(both.error, 'the request to the "dual" server failed: connect ECONNREFUSED ::1:4321; connect ECONNREFUSED 127.0.0.1:4321'); - assert.equal(both.remedy, TRY_LATER); + assert.equal(both.remedy, REQUEST_FAILED); }); test("a save that throws is the workspace's failure, logged for whoever runs the host, not the fetch's", async (t) => { const errors = t.mock.method(console, 'error', () => {}); const reg = new ReferenceRegistry(); - const f = new ReferenceFetcher(() => ({ url: ORIGIN }), async () => { throw new Error('disk on fire'); }); + // Its words take one line of the log, as the agent's text would. + const f = new ReferenceFetcher(() => ({ url: ORIGIN }), async () => { throw new Error('disk on fire\nand smoking'); }); const rec = makeRecord(reg, `${ORIGIN}/ok?save-throws`); const out = await f.fetch(rec); assert.equal(out.error, "it was fetched, but this host couldn't store it in its workspace, so nothing was kept"); assert.equal(out.remedy, 'Calling again is likely to fail the same way; whoever runs this host can see why in its log.'); - assert.deepEqual(errors.mock.calls.map((c) => String(c.arguments[0])), [`[refs] couldn't store ${rec.refId}.wav: disk on fire`]); + assert.deepEqual(errors.mock.calls.map((c) => String(c.arguments[0])), [`[refs] couldn't store ${rec.refId}.wav: disk on fire?and smoking`]); assert.equal(rec.fetchedPath, undefined); }); @@ -1057,7 +1064,7 @@ test("an address that doesn't parse is refused before anything is dialed: the re assert.equal(bad.error, "its address doesn't parse as a URL"); assert.equal(bad.remedy, NOTHING_TO_DO); const misconfigured = await makeFetcher({ srv: { url: 'not a url either' } }).fetch(makeRecord(reg, `${ORIGIN}/ok?misconfigured`)); - assert.equal(misconfigured.error, noAddressReason('srv')); + assert.equal(misconfigured.error, "the \"srv\" server's address doesn't parse as a URL, so there's nowhere to fetch it from"); assert.equal(misconfigured.remedy, NOTHING_TO_DO); assert.equal(hits.get('/ok?misconfigured') ?? 0, 0, 'nothing dialed'); }); @@ -1070,6 +1077,7 @@ test("past what's left of the server's budget, a download stopped at max_bytes i const out = await f.fetch(makeRecord(reg, `${ORIGIN}/drip?first=1200&rest=5000&wait=10000`), { maxBytes: 1000 }); const [, received] = /^the download had reached (\d+) bytes, past the 1000 bytes this call allows, so it was stopped and nothing was kept$/.exec(out.error!) ?? []; assert.ok(received, out.error); + assert.ok(Number(received) > 1000 && Number(received) <= 1200, received); assert.equal(out.remedy, `It's at least ${received} bytes, more than the ${1500 - Number(received)} bytes left of this host's fetch budget for the "srv" server. ${BUDGET}`); }); diff --git a/test/save-recent-image.test.ts b/test/save-recent-image.test.ts index 91bc553c8..8993db243 100644 --- a/test/save-recent-image.test.ts +++ b/test/save-recent-image.test.ts @@ -689,7 +689,7 @@ describe('save_recent_image provenance (issue #104)', () => { const expected = [ ['call_save0', noAddress.refId, `It can't be fetched either — ${noAddressReason('stdio')}. ${nothing}`], ['call_save1', offOrigin.refId, "It can't be fetched either — it points to https://cdn.example.invalid, not to the \"vst\" server's own address " - + `(https://example.invalid), and this host fetches only from the server itself. ${nothing}`], + + `(wss://example.invalid), and this host fetches only from the server itself. ${nothing}`], ['call_save2', gone.refId, "It can't be fetched either — this host no longer has the \"gone\" server it came from. " + "Calling again won't change that unless whoever runs this host adds that server back."], ['call_save3', big.refId, "It can't be fetched either — it claims 536870912 bytes, over the 268435456 bytes this host fetches at most. " From fb480d977e8899887c6d78d25f5150e363674279 Mon Sep 17 00:00:00 2001 From: Cordelia-2434 Date: Sun, 18 Oct 2026 12:35:44 -0400 Subject: [PATCH 4/5] fix(mcpl): refuse a reference address carrying a user name or password before dialing A reference uri such as http://user:secret@host/x passed the origin check, since an origin drops the userinfo, and reached fetch, which refuses such a url before connecting with a message that holds the whole uri. That message became the clause fetch_reference gave the agent, and the eager pass's log line, so the password reached the agent's context and the host's log, with a remedy inviting a retry that can't work. Such a uri is now refused in the origin check, before the scheme check, with a clause that doesn't repeat it and a remedy saying calling again won't change that; refusal() and the eager check refuse it too. A user name or password in a uri is a credential embedded in it, which RFC-005 section 6.1 forbids a server to mint and lets a host refuse (vector 6); this host neither sends nor repeats it. statusRemedy: 409 and 423, a conflict or lock the server's state can clear, and 425, a request the server calls too early, now say a later try may work, where they said only a fresh reference could change the answer. Tests: a reference with a user name and password, and one with a user name alone, each claiming a size within the eager limit, are refused by refusal(), the eager check and fetch, with nothing dialed; the status table gains 409, 423 and 425. Changelog: a fixed entry for the refusal, and the changed entry lists 409, 423 and 425 among the statuses where a later try may work. Co-Authored-By: Claude Opus 5.5 --- .../fetch-reference-credentials.fixed.md | 1 + changelog.d/fetch-reference-words.changed.md | 2 +- src/mcpl/reference-fetcher.ts | 19 +++++++++++++----- test/mcpl-reference-fetcher.test.ts | 20 +++++++++++++++++++ 4 files changed, 36 insertions(+), 6 deletions(-) create mode 100644 changelog.d/fetch-reference-credentials.fixed.md diff --git a/changelog.d/fetch-reference-credentials.fixed.md b/changelog.d/fetch-reference-credentials.fixed.md new file mode 100644 index 000000000..3c8bb2ec9 --- /dev/null +++ b/changelog.d/fetch-reference-credentials.fixed.md @@ -0,0 +1 @@ +- A reference whose address carries a user name or password is refused before anything is dialed, in words that don't repeat them. `fetch` refused such an address before connecting, with a message holding the whole address, password included, which `fetch_reference` gave the agent as its error, and the host's log carried it when the eager fetch tried it. diff --git a/changelog.d/fetch-reference-words.changed.md b/changelog.d/fetch-reference-words.changed.md index fbf48fe0f..4165a5d9c 100644 --- a/changelog.d/fetch-reference-words.changed.md +++ b/changelog.d/fetch-reference-words.changed.md @@ -1,6 +1,6 @@ - `fetch_reference`'s failures now say what happened to the reference and what calling again can do: `fetch_reference couldn't fetch "": . `, where they gave the fetcher's reason alone, such as `HTTP 404`, `fetch timed out or aborted`, `claimed bytes exceeds fetch ceiling ` or `third-party origin ( != dialed ); not fetched by this host`. Where `save_recent_image` says a reference can't be fetched, it gives the same two. - **What happened** names the server and what it answered (`the "" server answered HTTP 503`), the limit a fetch met (`it claims bytes, over the bytes this call allows`, `the download had reached bytes, past …`), what kept a fetch from dialing (`it points to , not to the "" server's own address (), and this host fetches only from the server itself`), or that what arrived didn't match the reference's digest or couldn't be stored. The eager fetch's and a script's fetch's log lines read the same words. - - **What calling again can do**, such as trying again later (after a timeout, 408, 429, or a 5xx other than 501 and 505, and after a failed request, with whoever runs the host to check its connection to the server if it fails the same way), waiting for a fresh reference to it (an expired or unbound reference, and the other 4xx), passing a larger `max_bytes` (naming the size it needs, and no more than the server's fetch budget has left), asking whoever runs the host (who can check its credentials for the server after a 401 or 403, raise or renew a spent budget, or add back a server it no longer has), or nothing, as for an address on another origin, a server this host doesn't connect to over a network, or a claim past the 256 MiB this host fetches at most, which no `max_bytes` or setting lifts. A claim past those 256 MiB is refused for them first, whatever the server's budget has left. + - **What calling again can do**, such as trying again later (after a timeout, 408, 409, 423, 425, 429, or a 5xx other than 501 and 505, and after a failed request, with whoever runs the host to check its connection to the server if it fails the same way), waiting for a fresh reference to it (an expired or unbound reference, and the other 4xx), passing a larger `max_bytes` (naming the size it needs, and no more than the server's fetch budget has left), asking whoever runs the host (who can check its credentials for the server after a 401 or 403, raise or renew a spent budget, or add back a server it no longer has), or nothing, as for an address on another origin, a server this host doesn't connect to over a network, or a claim past the 256 MiB this host fetches at most, which no `max_bytes` or setting lifts. A claim past those 256 MiB is refused for them first, whatever the server's budget has left. - **The budget's texts** read `this host's fetch budget for the "" server is used up ( of bytes)` and, partway, `this host's fetch budget for the "" server ran out during the download ( of bytes), so it was stopped and nothing was kept`, where they read `per-server fetch budget exhausted (/ bytes)`, the one partway after `aborted: `. - **A `ws:` or `wss:` reference** on its server's own origin is refused for its scheme before anything is dialed, where `fetch` refused it before connecting and the agent read `fetch failed`. A server whose address doesn't parse is refused, saying so. - **The host's log** says why a fetched payload couldn't be stored: no workspace, no writable mount, or the write's error. A save that throws is that failure too, logged, where its message reached the agent as the fetch's own error. diff --git a/src/mcpl/reference-fetcher.ts b/src/mcpl/reference-fetcher.ts index f5a537660..d023fcbae 100644 --- a/src/mcpl/reference-fetcher.ts +++ b/src/mcpl/reference-fetcher.ts @@ -121,7 +121,8 @@ export type FetchOutcome = const refused = (error: string, remedy: string): { refusal: Refusal } => ({ refusal: { error, remedy } }); const fail = (error: string, remedy: string): FetchOutcome => ({ ok: false, error, remedy }); -/** What calling again can do about a server's HTTP status. A timeout, a rate +/** What calling again can do about a server's HTTP status. A timeout, a + * conflict or lock the server's state can clear, a request too early, a rate * limit or a server's passing trouble can clear, though not 501 or 505, which * say the server doesn't support the request. A 401 is about this host's * credentials for the server, which a fresh reference doesn't change, and a @@ -129,7 +130,7 @@ const fail = (error: string, remedy: string): FetchOutcome => ({ ok: false, erro * reference, which a fresh one may not share. */ function statusRemedy(status: number): string { if (status === 501 || status === 505) return NOTHING_TO_DO; - if (status === 408 || status === 429 || status >= 500) return TRY_LATER; + if (status === 408 || status === 409 || status === 423 || status === 425 || status === 429 || status >= 500) return TRY_LATER; if (status === 401) return CREDENTIALS; if (status === 403) return FRESH_OR_CREDENTIALS; return FRESH_REFERENCE; @@ -260,8 +261,8 @@ export class ReferenceFetcher { /** Every check the fetch makes before it dials, in its order: the server * (none, unknown, no address, an address of its own or the reference's - * that doesn't parse, a scheme other than http: or https:, off the uri's - * origin), the expiry, a claimed size over the host's own ceiling, the + * that doesn't parse, a reference address carrying a user name or + * password, a scheme other than http: or https:, off the uri's origin), the expiry, a claimed size over the host's own ceiling, the * server's spent budget, a claimed size over a call's 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 @@ -331,11 +332,19 @@ export class ReferenceFetcher { if (!dialedOrigin) { return refused(`the "${serverId}" server's address doesn't parse as a URL, so there's nowhere to fetch it from`, NOTHING_TO_DO); } + const refUrl = new URL(record.testimony.uri); + // A user name or password in the uri is a credential embedded in it, + // which RFC-005 §6.1 forbids a server to mint and lets a host refuse + // (vector 6). This host neither sends nor repeats it: fetch refuses such + // a url with a message that holds it whole. + if (refUrl.username !== '' || refUrl.password !== '') { + return refused("its address carries a user name or password, which this host doesn't send", NOTHING_TO_DO); + } // The uri's own scheme, not its origin's: an origin reads ws: as http: // and wss: as https:, and fetch refuses a ws: or wss: uri ('unknown // scheme') before connecting, which would read as a failed request a // later try can mend. - const scheme = new URL(record.testimony.uri).protocol; + const scheme = refUrl.protocol; if (scheme !== 'https:' && scheme !== 'http:') { return refused(`its address uses ${scheme}, and this host fetches only http: and https: addresses`, NOTHING_TO_DO); } diff --git a/test/mcpl-reference-fetcher.test.ts b/test/mcpl-reference-fetcher.test.ts index aeb3df1a6..67cdb48ee 100644 --- a/test/mcpl-reference-fetcher.test.ts +++ b/test/mcpl-reference-fetcher.test.ts @@ -186,6 +186,24 @@ test('third-party origin refused (v1 policy: dialed origin only)', async () => { assert.equal(out.remedy, NOTHING_TO_DO); }); +test('a reference whose address carries a user name or password is refused before dialing, without repeating them', async () => { + // fetch refuses such a url with a message that holds it whole, so the + // credential would reach the agent's context and the host's log. + const reg = new ReferenceRegistry(); + const f = makeFetcher({ srv: { url: `ws://127.0.0.1:${port}` } }); + const clause = "its address carries a user name or password, which this host doesn't send"; + for (const uri of [`http://user:s3cret@127.0.0.1:${port}/ok?userinfo`, `http://justauser@127.0.0.1:${port}/ok?userinfo-name`]) { + // A claimed size within the eager limit, so only the refusal keeps the eager fetch away. + const rec = makeRecord(reg, uri, { sizeBytes: 1000 }); + assert.deepEqual(f.refusal(rec), { error: clause, remedy: NOTHING_TO_DO }, uri); + assert.equal(f.isEagerEligible(rec), false, uri); + const out = await f.fetch(rec); + assert.equal(out.error, clause, uri); + assert.equal(out.remedy, NOTHING_TO_DO, uri); + } + assert.equal((hits.get('/ok?userinfo') ?? 0) + (hits.get('/ok?userinfo-name') ?? 0), 0, 'nothing dialed'); +}); + test('an http: reference from a server dialed over TLS is refused as another origin, with nothing dialed', async () => { // The same host and port, so only the scheme differs: the origin check, which // compares origins with their schemes, is all that refuses it. @@ -968,6 +986,8 @@ test("each HTTP status says what calling again can do: later, a fresh reference, const expect: Array<[number, string]> = [ // Passing trouble, which can clear. [408, TRY_LATER], [429, TRY_LATER], [500, TRY_LATER], [503, TRY_LATER], + // A conflict or a lock the server's state can clear, and a request it calls too early. + [409, TRY_LATER], [423, TRY_LATER], [425, TRY_LATER], // The server doesn't support the request, which later won't change. [501, NOTHING_TO_DO], [505, NOTHING_TO_DO], // This host's credentials for the server, which a fresh reference keeps. From ab59c2d760fb96477d3c09738467b7818d72b483 Mon Sep 17 00:00:00 2001 From: Cordelia-2434 Date: Sun, 18 Oct 2026 13:00:28 -0400 Subject: [PATCH 5/5] fix(mcpl): stop a download past the host's ceiling before the budget, name a server's address as configured, and tell an unparseable server address apart What a second internal review found, with two smaller points from a reader: - The read loop checks the host's own 256 MiB first, then the server's budget, then a call's max_bytes, the order the checks before dialing use. A chunk that carried a download past both the host's ceiling and the budget was told about the budget, though no budget lifts the ceiling. largerMaxBytes now only ever sees a download within the host's ceiling, so its branch for one past it goes. - The origin clause names the server's address by its scheme and host as configured. URL's own origin is "null" for a scheme that isn't special, which the origin comparison accepts, so the clause could read "(null)". Scheme and host also leave out any userinfo, path or query. - A server address that doesn't parse tells the agent that whoever runs the host can correct it, where it said calling again won't change that. - Comments: Refusal.error's doc names the log lines that read the clause; beforeDial's and statusRemedy's are rewrapped and reworded. - Tests: a password with no user name is refused before dialing like the other two shapes; a server configured on a scheme that isn't special, with userinfo, shows as its scheme and host; a no-address server with a claim past the host's ceiling is refused for the server; the save-throw test's cause runs past 200 code points, pinning the log line's bound; an unparseable address's test title says what each case now reads. - Changelog: the changed entry says a download past the host's ceiling is stopped for it first, and what an unparseable server address now says. Co-Authored-By: Claude Opus 5.5 --- changelog.d/fetch-reference-words.changed.md | 4 +- src/mcpl/reference-fetcher.ts | 69 ++++++++++++-------- test/mcpl-reference-fetcher.test.ts | 22 +++++-- 3 files changed, 59 insertions(+), 36 deletions(-) diff --git a/changelog.d/fetch-reference-words.changed.md b/changelog.d/fetch-reference-words.changed.md index 4165a5d9c..fbad821d9 100644 --- a/changelog.d/fetch-reference-words.changed.md +++ b/changelog.d/fetch-reference-words.changed.md @@ -1,6 +1,6 @@ - `fetch_reference`'s failures now say what happened to the reference and what calling again can do: `fetch_reference couldn't fetch "": . `, where they gave the fetcher's reason alone, such as `HTTP 404`, `fetch timed out or aborted`, `claimed bytes exceeds fetch ceiling ` or `third-party origin ( != dialed ); not fetched by this host`. Where `save_recent_image` says a reference can't be fetched, it gives the same two. - **What happened** names the server and what it answered (`the "" server answered HTTP 503`), the limit a fetch met (`it claims bytes, over the bytes this call allows`, `the download had reached bytes, past …`), what kept a fetch from dialing (`it points to , not to the "" server's own address (), and this host fetches only from the server itself`), or that what arrived didn't match the reference's digest or couldn't be stored. The eager fetch's and a script's fetch's log lines read the same words. - - **What calling again can do**, such as trying again later (after a timeout, 408, 409, 423, 425, 429, or a 5xx other than 501 and 505, and after a failed request, with whoever runs the host to check its connection to the server if it fails the same way), waiting for a fresh reference to it (an expired or unbound reference, and the other 4xx), passing a larger `max_bytes` (naming the size it needs, and no more than the server's fetch budget has left), asking whoever runs the host (who can check its credentials for the server after a 401 or 403, raise or renew a spent budget, or add back a server it no longer has), or nothing, as for an address on another origin, a server this host doesn't connect to over a network, or a claim past the 256 MiB this host fetches at most, which no `max_bytes` or setting lifts. A claim past those 256 MiB is refused for them first, whatever the server's budget has left. + - **What calling again can do**, such as trying again later (after a timeout, 408, 409, 423, 425, 429, or a 5xx other than 501 and 505, and after a failed request, with whoever runs the host to check its connection to the server if it fails the same way), waiting for a fresh reference to it (an expired or unbound reference, and the other 4xx), passing a larger `max_bytes` (naming the size it needs, and no more than the server's fetch budget has left), asking whoever runs the host (who can check its credentials for the server after a 401 or 403, raise or renew a spent budget, or add back a server it no longer has), or nothing, as for an address on another origin, a server this host doesn't connect to over a network, or a claim past the 256 MiB this host fetches at most, which no `max_bytes` or setting lifts. A claim past those 256 MiB is refused for them first, and a download past them is stopped for them first, whatever the server's budget has left. - **The budget's texts** read `this host's fetch budget for the "" server is used up ( of bytes)` and, partway, `this host's fetch budget for the "" server ran out during the download ( of bytes), so it was stopped and nothing was kept`, where they read `per-server fetch budget exhausted (/ bytes)`, the one partway after `aborted: `. - - **A `ws:` or `wss:` reference** on its server's own origin is refused for its scheme before anything is dialed, where `fetch` refused it before connecting and the agent read `fetch failed`. A server whose address doesn't parse is refused, saying so. + - **A `ws:` or `wss:` reference** on its server's own origin is refused for its scheme before anything is dialed, where `fetch` refused it before connecting and the agent read `fetch failed`. A server whose address doesn't parse is refused, saying so, and that whoever runs the host can correct it. - **The host's log** says why a fetched payload couldn't be stored: no workspace, no writable mount, or the write's error. A save that throws is that failure too, logged, where its message reached the agent as the fetch's own error. diff --git a/src/mcpl/reference-fetcher.ts b/src/mcpl/reference-fetcher.ts index d023fcbae..03a7257b1 100644 --- a/src/mcpl/reference-fetcher.ts +++ b/src/mcpl/reference-fetcher.ts @@ -93,8 +93,8 @@ export type ServerResolution = ResolvedServerBinding | 'no-address'; /** Why a fetch failed, or would be refused before dialing. */ export interface Refusal { /** What happened, as a clause that completes "couldn't fetch : ", - * whatever it's about: fetch_reference's sentence, save_recent_image's - * and the host's log lines all read it. */ + * whatever it's about: fetch_reference's sentence, save_recent_image's, + * and the eager and script-demand fetches' log lines read it. */ error: string; /** What calling again can do, as a sentence. fetch_reference and * save_recent_image show it; the eager and script-demand fetches log the @@ -121,13 +121,14 @@ export type FetchOutcome = const refused = (error: string, remedy: string): { refusal: Refusal } => ({ refusal: { error, remedy } }); const fail = (error: string, remedy: string): FetchOutcome => ({ ok: false, error, remedy }); -/** What calling again can do about a server's HTTP status. A timeout, a - * conflict or lock the server's state can clear, a request too early, a rate - * limit or a server's passing trouble can clear, though not 501 or 505, which - * say the server doesn't support the request. A 401 is about this host's - * credentials for the server, which a fresh reference doesn't change, and a - * 403 can be either those or this reference. The rest of 4xx is about this - * reference, which a fresh one may not share. */ +/** What calling again can do about a server's HTTP status. A later try can + * work after a timeout, a conflict or lock the server's state can clear, a + * request it calls too early, a rate limit or a server's passing trouble, + * though not after 501 or 505, which say the server doesn't support the + * request. A 401 is about this host's credentials for the server, which a + * fresh reference doesn't change, and a 403 can be either those or this + * reference. The rest of 4xx is about this reference, which a fresh one may + * not share. */ function statusRemedy(status: number): string { if (status === 501 || status === 505) return NOTHING_TO_DO; if (status === 408 || status === 409 || status === 423 || status === 425 || status === 429 || status >= 500) return TRY_LATER; @@ -139,14 +140,13 @@ function statusRemedy(status: number): string { /** What a download stopped at a call's own ceiling, below the host's, can do. * Its whole size is unknown, only that it's past what arrived, and a fetch * again reads it from the start, within the host's ceiling and what the - * server's budget has left after this one's bytes. */ + * server's budget has left after this one's bytes. The read loop stops a + * download past the host's ceiling before it asks, so what arrived is within + * it, and only the budget can be what binds. */ function largerMaxBytes(received: number, left: number, server: string): string { const most = Math.min(DEFAULT_FETCH_MAX_BYTES, left); if (received <= most) return `It's at least ${received} bytes; a larger max_bytes, up to ${most}, may fetch it.`; - // Past the host's own ceiling nothing lifts it, whatever the budget has left. - return received <= DEFAULT_FETCH_MAX_BYTES - ? `It's at least ${received} bytes, more than the ${left} bytes left of this host's fetch budget for ${server}. ${BUDGET}` - : `It's at least ${received} bytes, more than the ${DEFAULT_FETCH_MAX_BYTES} bytes this host fetches at most. ${HOST_CEILING}`; + return `It's at least ${received} bytes, more than the ${left} bytes left of this host's fetch budget for ${server}. ${BUDGET}`; } /** What a thrown fetch says went wrong. fetch's own "fetch failed" says only @@ -191,6 +191,15 @@ const EXT_BY_MIME = new Map(Object.entries({ 'application/pdf': 'pdf', 'application/zip': 'zip', })); +/** A server's address as it's configured, its scheme and host, for a + * clause: never its userinfo, path or query. URL's own origin is 'null' for + * a scheme that isn't special, which normalizedOrigin accepts. The caller + * has already parsed it. */ +function configuredOrigin(url: string): string { + const u = new URL(url); + return `${u.protocol}//${u.host}`; +} + /** ws→http / wss→https with default ports elided, for origin comparison. */ export function normalizedOrigin(url: string): string | null { try { @@ -262,12 +271,13 @@ export class ReferenceFetcher { /** Every check the fetch makes before it dials, in its order: the server * (none, unknown, no address, an address of its own or the reference's * that doesn't parse, a reference address carrying a user name or - * password, a scheme other than http: or https:, off the uri's origin), the expiry, a claimed size over the host's own ceiling, the - * server's spent budget, a claimed size over a call's 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. */ + * password, a scheme other than http: or https:, off the uri's origin), + * the expiry, a claimed size over the host's own ceiling, the server's + * spent budget, a claimed size over a call's 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); @@ -330,7 +340,8 @@ export class ReferenceFetcher { // guards other resolvers. const dialedOrigin = normalizedOrigin(binding.url); if (!dialedOrigin) { - return refused(`the "${serverId}" server's address doesn't parse as a URL, so there's nowhere to fetch it from`, NOTHING_TO_DO); + return refused(`the "${serverId}" server's address doesn't parse as a URL, so there's nowhere to fetch it from`, + "Calling again won't change that unless whoever runs this host corrects the server's address."); } const refUrl = new URL(record.testimony.uri); // A user name or password in the uri is a credential embedded in it, @@ -351,7 +362,7 @@ export class ReferenceFetcher { // Origins compare with their schemes, so this also refuses an http: // reference from a server dialed over TLS (wss://). if (refOrigin !== dialedOrigin) { - return refused(`it points to ${refOrigin}, not to the "${serverId}" server's own address (${new URL(binding.url).origin}), ` + + return refused(`it points to ${refOrigin}, not to the "${serverId}" server's own address (${configuredOrigin(binding.url)}), ` + 'and this host fetches only from the server itself', NOTHING_TO_DO); } return null; @@ -403,6 +414,13 @@ export class ReferenceFetcher { // this server read the same total, so none spends what another took. const total = (this.totals.get(serverId) ?? 0) + value.byteLength; this.totals.set(serverId, total); + // The limit nothing lifts first, as before dialing: the host's + // ceiling, then the budget, then a call's own ceiling. + if (received > DEFAULT_FETCH_MAX_BYTES) { + controller.abort(); + return fail(`the download had reached ${received} bytes, past the ${DEFAULT_FETCH_MAX_BYTES} bytes this host fetches at most, ` + + 'so it was stopped and nothing was kept', HOST_CEILING); + } if (total > budget) { controller.abort(); // The server's running total, as the next fetch's refusal gives it: @@ -412,11 +430,8 @@ export class ReferenceFetcher { } if (received > ceiling) { controller.abort(); - return ceiling >= DEFAULT_FETCH_MAX_BYTES - ? fail(`the download had reached ${received} bytes, past the ${DEFAULT_FETCH_MAX_BYTES} bytes this host fetches at most, ` + - 'so it was stopped and nothing was kept', HOST_CEILING) - : fail(`the download had reached ${received} bytes, past the ${ceiling} bytes this call allows, ` + - 'so it was stopped and nothing was kept', largerMaxBytes(received, budget - total, server)); + return fail(`the download had reached ${received} bytes, past the ${ceiling} bytes this call allows, ` + + 'so it was stopped and nothing was kept', largerMaxBytes(received, budget - total, server)); } const buf = Buffer.from(value); hash.update(buf); diff --git a/test/mcpl-reference-fetcher.test.ts b/test/mcpl-reference-fetcher.test.ts index 67cdb48ee..dbbb930f4 100644 --- a/test/mcpl-reference-fetcher.test.ts +++ b/test/mcpl-reference-fetcher.test.ts @@ -192,7 +192,9 @@ test('a reference whose address carries a user name or password is refused befor const reg = new ReferenceRegistry(); const f = makeFetcher({ srv: { url: `ws://127.0.0.1:${port}` } }); const clause = "its address carries a user name or password, which this host doesn't send"; - for (const uri of [`http://user:s3cret@127.0.0.1:${port}/ok?userinfo`, `http://justauser@127.0.0.1:${port}/ok?userinfo-name`]) { + // A user name with a password, a user name alone, and a password alone. + for (const uri of [`http://user:s3cret@127.0.0.1:${port}/ok?userinfo`, `http://justauser@127.0.0.1:${port}/ok?userinfo-name`, + `http://:s3cret@127.0.0.1:${port}/ok?userinfo-password`]) { // A claimed size within the eager limit, so only the refusal keeps the eager fetch away. const rec = makeRecord(reg, uri, { sizeBytes: 1000 }); assert.deepEqual(f.refusal(rec), { error: clause, remedy: NOTHING_TO_DO }, uri); @@ -201,7 +203,7 @@ test('a reference whose address carries a user name or password is refused befor assert.equal(out.error, clause, uri); assert.equal(out.remedy, NOTHING_TO_DO, uri); } - assert.equal((hits.get('/ok?userinfo') ?? 0) + (hits.get('/ok?userinfo-name') ?? 0), 0, 'nothing dialed'); + assert.equal(['/ok?userinfo', '/ok?userinfo-name', '/ok?userinfo-password'].reduce((n, k) => n + (hits.get(k) ?? 0), 0), 0, 'nothing dialed'); }); test('an http: reference from a server dialed over TLS is refused as another origin, with nothing dialed', async () => { @@ -817,6 +819,8 @@ test('refusal() answers what the fetch would refuse before dialing, case by case srv: { url: `ws://127.0.0.1:${port}` }, 'stdio-srv': 'no-address', spent: { url: `ws://127.0.0.1:${port}`, autofetch: { maxTotalBytes: 0 } }, + // A scheme that isn't special, whose URL origin is 'null', and a token in its userinfo. + relay: { url: 'mcpl://tok:sekrit@relay.example:9000/path?key=x' }, // 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 } }, }); @@ -830,6 +834,9 @@ test('refusal() answers what the fetch would refuse before dialing, case by case ['no address', makeRecord(reg, `${ORIGIN}/ok?case=no-address`, small, 'stdio-srv'), noAddressReason('stdio-srv'), NOTHING_TO_DO], ['off the origin', makeRecord(reg, 'https://cdn.example.invalid/case-origin', small, 'srv'), `it points to https://cdn.example.invalid, not to the "srv" server's own address (ws://127.0.0.1:${port}), and this host fetches only from the server itself`, NOTHING_TO_DO], + // The server's address as configured, its scheme and host, without its userinfo, path or query. + ['off the origin of a server on another scheme', makeRecord(reg, 'https://cdn.example.invalid/case-relay', small, 'relay'), + 'it points to https://cdn.example.invalid, not to the "relay" server\'s own address (mcpl://relay.example:9000), and this host fetches only from the server itself', NOTHING_TO_DO], ['expired', makeRecord(reg, `${ORIGIN}/ok?case=expired`, { ...small, expiresAt: '2001-01-01T00:00:00Z' }, 'srv'), `the reference is ${EXPIRY_REASON.passed}`, FRESH_REFERENCE], ['budget spent', makeRecord(reg, `${ORIGIN}/ok?case=budget`, small, 'spent'), 'this host\'s fetch budget for the "spent" server is used up (0 of 0 bytes)', BUDGET], ['over the ceiling', makeRecord(reg, `${ORIGIN}/ok?case=ceiling`, { sizeBytes: 512 * 1024 * 1024 }, 'roomy'), @@ -869,6 +876,7 @@ test('a reference with two faults is refused for the first the fetch checks: the 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')], + ['no address, over the host ceiling', makeRecord(reg, `${ORIGIN}/ok?two=5`, { sizeBytes: huge }, 'stdio-srv'), noAddressReason('stdio-srv')], ['expired, budget spent, over the ceiling', makeRecord(reg, `${ORIGIN}/ok?two=2`, { expiresAt: past, sizeBytes: huge }, 'spent'), `the reference is ${EXPIRY_REASON.passed}`], // Nothing lifts the host's ceiling, so it comes before a budget whoever runs the host can raise. ['over the host ceiling, budget spent', makeRecord(reg, `${ORIGIN}/ok?two=3`, { sizeBytes: huge }, 'spent'), 'it claims 536870912 bytes, over the 268435456 bytes this host fetches at most'], @@ -1033,13 +1041,13 @@ test('a fetch that meets no listener says the request failed, with its cause, an test("a save that throws is the workspace's failure, logged for whoever runs the host, not the fetch's", async (t) => { const errors = t.mock.method(console, 'error', () => {}); const reg = new ReferenceRegistry(); - // Its words take one line of the log, as the agent's text would. - const f = new ReferenceFetcher(() => ({ url: ORIGIN }), async () => { throw new Error('disk on fire\nand smoking'); }); + // Its words take one line of the log, bounded as the agent's text would be. + const f = new ReferenceFetcher(() => ({ url: ORIGIN }), async () => { throw new Error(`disk on fire\n${'x'.repeat(300)}`); }); const rec = makeRecord(reg, `${ORIGIN}/ok?save-throws`); const out = await f.fetch(rec); assert.equal(out.error, "it was fetched, but this host couldn't store it in its workspace, so nothing was kept"); assert.equal(out.remedy, 'Calling again is likely to fail the same way; whoever runs this host can see why in its log.'); - assert.deepEqual(errors.mock.calls.map((c) => String(c.arguments[0])), [`[refs] couldn't store ${rec.refId}.wav: disk on fire?and smoking`]); + assert.deepEqual(errors.mock.calls.map((c) => String(c.arguments[0])), [`[refs] couldn't store ${rec.refId}.wav: disk on fire?${'x'.repeat(187)}…`]); assert.equal(rec.fetchedPath, undefined); }); @@ -1078,14 +1086,14 @@ test("a payload fetched but not stored says so, and the host's log says why: no } }); -test("an address that doesn't parse is refused before anything is dialed: the reference's, or the server's as no address", async () => { +test("an address that doesn't parse is refused before anything is dialed: the reference's, or the server's, each saying which", async () => { const reg = new ReferenceRegistry(); const bad = await makeFetcher({ srv: { url: ORIGIN } }).fetch(makeRecord(reg, 'not a url at all')); assert.equal(bad.error, "its address doesn't parse as a URL"); assert.equal(bad.remedy, NOTHING_TO_DO); const misconfigured = await makeFetcher({ srv: { url: 'not a url either' } }).fetch(makeRecord(reg, `${ORIGIN}/ok?misconfigured`)); assert.equal(misconfigured.error, "the \"srv\" server's address doesn't parse as a URL, so there's nowhere to fetch it from"); - assert.equal(misconfigured.remedy, NOTHING_TO_DO); + assert.equal(misconfigured.remedy, "Calling again won't change that unless whoever runs this host corrects the server's address."); assert.equal(hits.get('/ok?misconfigured') ?? 0, 0, 'nothing dialed'); });