diff --git a/CHANGELOG.md b/CHANGELOG.md index 776c1c7..a3c9001 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,6 +8,17 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +## [0.16.1-beta.0] — 2026-05-28 + +### Added + +- **`available_cache_uris` field on MCP tool error envelopes** (plan `docs/plans/2026-05-28-003-fix-error-envelope-cache-uri-hint-plan.md`). When an MCP tool returns an `{error: ""}` envelope (engine-side inline-list cap OR MCP-server-side validation), the engine scans the last 10 `role: "tool"` messages backward, harvests `_cache_uri` per upstream tool name (deduped, most-recent-first), and appends `available_cache_uris: [{tool_name, cache_uri}, ...]` to the LLM-visible envelope before pushing into conversation history. Fixes the observed underperformance where a failed `create_*` tool would cause the LLM to re-run the upstream data query — small / quantized models reliably drop `_cache_uri` values from attention by the time they see a chart-tool failure, so they defend by re-querying instead of reusing the cached pointer. With the new field, the URI lives in immediate error-envelope context. + + - Wired at the inline-list-cap site (`engine/index.js:934`) and the dispatch path (`:1270`). Runs BEFORE the truncation block so oversized error envelopes preserve the field through the summary build (`:1257-1264`). + - NOT wired at the cache-miss site (`:957`) — that envelope already names the failing URIs verbatim via `_missing_uris`; enrichment would surface peer rows likely close to eviction. + - Short-circuits when `cacheOptions.enabled !== true` (the URI cannot exist) or when the envelope isn't an error. Additive only — existing `error`, `fix_hint`, `expected_kwargs`, `details`, `_engine_dispatched`, `_cache_uri` fields are preserved unchanged. + - Helper `enrichErrorEnvelope(envelope, messages, cacheOptions)` is exported from `engine/index.js` and re-exported via `engine/__test_internals__.js`. + ## [0.16.0-beta.0] — 2026-05-28 ### Added diff --git a/engine/__test_internals__.js b/engine/__test_internals__.js index 43dafcb..5236782 100644 --- a/engine/__test_internals__.js +++ b/engine/__test_internals__.js @@ -16,4 +16,5 @@ export { discoverPrompts, getPrompt, EmptyPromptError, + enrichErrorEnvelope, } from "./index.js"; diff --git a/engine/cacheUriHint.test.js b/engine/cacheUriHint.test.js new file mode 100644 index 0000000..5436b63 --- /dev/null +++ b/engine/cacheUriHint.test.js @@ -0,0 +1,453 @@ +/** + * engine/cacheUriHint.test.js — coverage for `enrichErrorEnvelope` + * (Plan 2026-05-28-003). + * + * Contract: when an MCP tool returns an `{error: "..."}` envelope, the + * engine scans the last 10 `role: "tool"` messages backward for entries + * carrying `_cache_uri` and appends `available_cache_uris: [{tool_name, + * cache_uri}, ...]` (most-recent-first, deduped by tool_name) to the + * LLM-visible envelope. Short-circuits when caching is disabled or the + * envelope is not an error. Helper is pure (no message mutation). + * + * Wire-up: enrichment fires at the inline-list-cap site and at the + * dispatch site BEFORE truncation. The cache-miss site is intentionally + * NOT enriched. Truncation summary preserves the new field. + */ + +import { describe, expect, it, vi } from "vitest"; + +import { processToolCalls } from "./index.js"; +import { enrichErrorEnvelope } from "./__test_internals__.js"; +import { makeFakeClient } from "../test-helpers/fakeConn.js"; + +// --------------------------------------------------------------------------- +// Helpers +// --------------------------------------------------------------------------- + +function makeFreshState() { + return { + lastChartResult: null, + lastQueryResult: null, + lastQuerySQL: null, + lastListResult: null, + lastMapResult: null, + lastHydrofabricResult: null, + pendingVisualizations: [], + pendingLayerUpdates: [], + pendingPatches: [], + rejectedPatches: [], + toolCallsThisTurn: [], + }; +} + +function makeToolCall(name, args = {}, id = `call-${name}`) { + return { id, function: { name, arguments: args } }; +} + +function makeConnections(toolResultsByName) { + const callTool = vi.fn(async ({ name }) => { + const result = toolResultsByName[name]; + if (result === undefined) { + throw new Error(`Test fixture missing result for tool: ${name}`); + } + return { data: result }; + }); + const client = makeFakeClient({ callToolImpl: callTool }); + const connections = [{ client, transport: null, protocolUsed: "http" }]; + const toolServerMap = new Map( + Object.keys(toolResultsByName).map((name) => [name, 0]), + ); + return { connections, toolServerMap }; +} + +function toolMessage(tool_name, payload) { + return { + role: "tool", + tool_call_id: `call-${tool_name}`, + tool_name, + content: JSON.stringify(payload), + }; +} + +const CACHED_OPTIONS = { enabled: true, conversationId: "test" }; +const UNCACHED_OPTIONS = { enabled: false, conversationId: "test" }; + +// --------------------------------------------------------------------------- +// Helper-level tests (Unit 1) +// --------------------------------------------------------------------------- + +describe("enrichErrorEnvelope — pure helper", () => { + it("happy path: one prior tool with _cache_uri → field added with that entry", () => { + const envelope = { error: "invalid_args: data missing" }; + const messages = [ + toolMessage("query_data", { _cache_uri: "cache://abc", rows: 100 }), + ]; + + const result = enrichErrorEnvelope(envelope, messages, CACHED_OPTIONS); + + expect(result.error).toBe("invalid_args: data missing"); + expect(result.available_cache_uris).toEqual([ + { tool_name: "query_data", cache_uri: "cache://abc" }, + ]); + }); + + it("happy path multi: distinct tool_names → list in most-recent-first order", () => { + const envelope = { error: "invalid_args" }; + const messages = [ + toolMessage("query_data", { _cache_uri: "cache://oldest" }), + toolMessage("list_files", { _cache_uri: "cache://middle" }), + toolMessage("get_metadata", { _cache_uri: "cache://newest" }), + ]; + + const result = enrichErrorEnvelope(envelope, messages, CACHED_OPTIONS); + + expect(result.available_cache_uris).toEqual([ + { tool_name: "get_metadata", cache_uri: "cache://newest" }, + { tool_name: "list_files", cache_uri: "cache://middle" }, + { tool_name: "query_data", cache_uri: "cache://oldest" }, + ]); + }); + + it("edge: no prior tool messages → envelope returned unchanged (same ref)", () => { + const envelope = { error: "validation failed" }; + const result = enrichErrorEnvelope(envelope, [], CACHED_OPTIONS); + expect(result).toBe(envelope); + expect(result.available_cache_uris).toBeUndefined(); + }); + + it("edge: malformed JSON in content → skip without throwing, continue scanning", () => { + const envelope = { error: "x" }; + const messages = [ + { role: "tool", tool_call_id: "c1", tool_name: "bad", content: "{not json" }, + toolMessage("good", { _cache_uri: "cache://ok" }), + ]; + + const result = enrichErrorEnvelope(envelope, messages, CACHED_OPTIONS); + + expect(result.available_cache_uris).toEqual([ + { tool_name: "good", cache_uri: "cache://ok" }, + ]); + }); + + it("edge: envelope is not an error → returned unchanged (same ref)", () => { + const envelope = { ok: true, rows: 5 }; + const messages = [toolMessage("query_data", { _cache_uri: "cache://abc" })]; + + const result = enrichErrorEnvelope(envelope, messages, CACHED_OPTIONS); + + expect(result).toBe(envelope); + expect(result.available_cache_uris).toBeUndefined(); + }); + + it("edge: cacheOptions.enabled === false → returned unchanged with no JSON.parse work", () => { + const envelope = { error: "x" }; + // Pre-stringify into a non-JSON sentinel; if the helper attempted to + // parse it, the test would still pass because the parse failure is + // silently swallowed. But assertion below proves no field was added. + const messages = [toolMessage("query_data", { _cache_uri: "cache://abc" })]; + + const result = enrichErrorEnvelope(envelope, messages, UNCACHED_OPTIONS); + + expect(result).toBe(envelope); + expect(result.available_cache_uris).toBeUndefined(); + }); + + it("edge: cacheOptions omitted entirely → returned unchanged", () => { + const envelope = { error: "x" }; + const messages = [toolMessage("query_data", { _cache_uri: "cache://abc" })]; + + const result = enrichErrorEnvelope(envelope, messages, undefined); + + expect(result).toBe(envelope); + }); + + it("edge: > 10 tool messages → scan stops at 10 most-recent", () => { + const envelope = { error: "x" }; + const messages = []; + // 5 old entries the scan should NOT see (older than 10 deep) + for (let i = 0; i < 5; i++) { + messages.push(toolMessage(`old_tool_${i}`, { _cache_uri: `cache://old-${i}` })); + } + // 10 recent entries the scan SHOULD see + for (let i = 0; i < 10; i++) { + messages.push(toolMessage(`recent_tool_${i}`, { _cache_uri: `cache://recent-${i}` })); + } + + const result = enrichErrorEnvelope(envelope, messages, CACHED_OPTIONS); + + expect(result.available_cache_uris).toHaveLength(10); + expect(result.available_cache_uris.every((e) => e.tool_name.startsWith("recent_tool_"))).toBe(true); + expect(result.available_cache_uris.some((e) => e.tool_name.startsWith("old_tool_"))).toBe(false); + }); + + it("edge: same tool_name twice → only most recent kept", () => { + const envelope = { error: "x" }; + const messages = [ + toolMessage("query_data", { _cache_uri: "cache://old" }), + toolMessage("other", { _cache_uri: "cache://other" }), + toolMessage("query_data", { _cache_uri: "cache://new" }), + ]; + + const result = enrichErrorEnvelope(envelope, messages, CACHED_OPTIONS); + + expect(result.available_cache_uris).toEqual([ + { tool_name: "query_data", cache_uri: "cache://new" }, + { tool_name: "other", cache_uri: "cache://other" }, + ]); + }); + + it("edge: prior tool content parseable but no _cache_uri → skipped silently", () => { + const envelope = { error: "x" }; + const messages = [ + toolMessage("list_files", { ok: true, files: ["a", "b"] }), + toolMessage("query_data", { _cache_uri: "cache://abc" }), + ]; + + const result = enrichErrorEnvelope(envelope, messages, CACHED_OPTIONS); + + expect(result.available_cache_uris).toEqual([ + { tool_name: "query_data", cache_uri: "cache://abc" }, + ]); + }); + + it("edge: assistant/user messages → skipped (only role=tool scanned)", () => { + const envelope = { error: "x" }; + const messages = [ + { role: "user", content: "what's the weather?" }, + { role: "assistant", content: "Let me check.", tool_calls: [{ id: "c1" }] }, + toolMessage("query_data", { _cache_uri: "cache://abc" }), + { role: "assistant", content: "Here you go." }, + { role: "user", content: "now plot it" }, + ]; + + const result = enrichErrorEnvelope(envelope, messages, CACHED_OPTIONS); + + expect(result.available_cache_uris).toEqual([ + { tool_name: "query_data", cache_uri: "cache://abc" }, + ]); + }); + + it("edge: envelope already has available_cache_uris → entirely replaced (no merge)", () => { + const envelope = { + error: "x", + available_cache_uris: [{ tool_name: "stale_tool", cache_uri: "cache://stale" }], + }; + const messages = [toolMessage("fresh_tool", { _cache_uri: "cache://fresh" })]; + + const result = enrichErrorEnvelope(envelope, messages, CACHED_OPTIONS); + + expect(result.available_cache_uris).toEqual([ + { tool_name: "fresh_tool", cache_uri: "cache://fresh" }, + ]); + expect(result.available_cache_uris).toHaveLength(1); + }); + + it("invariant: does not mutate the input messages array", () => { + const envelope = { error: "x" }; + const original = [ + toolMessage("query_data", { _cache_uri: "cache://abc", rows: 100 }), + ]; + const snapshot = JSON.parse(JSON.stringify(original)); + + enrichErrorEnvelope(envelope, original, CACHED_OPTIONS); + + expect(original).toEqual(snapshot); + }); +}); + +// --------------------------------------------------------------------------- +// Wire-up tests (Unit 2) +// --------------------------------------------------------------------------- + +describe("enrichErrorEnvelope — engine wire-up", () => { + it("inline-cap site: fires through enrichment and the pushed message carries available_cache_uris", async () => { + const { connections, toolServerMap } = makeConnections({ + create_plotly_chart: { ok: true }, // never reached — cap fires first + }); + + // Pre-seed conversation with a prior tool message carrying _cache_uri + const messages = [ + toolMessage("query_data", { _cache_uri: "cache://nrds-query", rows: 240 }), + ]; + + // 24 records inline triggers checkInlineListCap (default cap is 20) + const oversizedData = Array.from({ length: 24 }, (_, i) => ({ x: i, y: i * 2 })); + const toolCalls = [ + makeToolCall("create_plotly_chart", { data: oversizedData }), + ]; + + await processToolCalls( + toolCalls, + messages, + connections, + toolServerMap, + makeFreshState(), + "", + { cacheOptions: CACHED_OPTIONS }, + ); + + // The cap-error message should be the last entry in messages + const pushed = messages[messages.length - 1]; + expect(pushed.role).toBe("tool"); + expect(pushed.tool_name).toBe("create_plotly_chart"); + const parsed = JSON.parse(pushed.content); + expect(parsed.error).toMatch(/invalid_args/); + expect(parsed.available_cache_uris).toEqual([ + { tool_name: "query_data", cache_uri: "cache://nrds-query" }, + ]); + }); + + it("dispatch error site: MCP-server-returned {error: ...} envelope picks up available_cache_uris", async () => { + const { connections, toolServerMap } = makeConnections({ + create_plotly_chart: { error: "invalid_args: data field required" }, + }); + + const messages = [ + toolMessage("query_data", { _cache_uri: "cache://nrds-query", rows: 100 }), + ]; + + await processToolCalls( + [makeToolCall("create_plotly_chart")], + messages, + connections, + toolServerMap, + makeFreshState(), + "", + { cacheOptions: CACHED_OPTIONS }, + ); + + const pushed = messages[messages.length - 1]; + const parsed = JSON.parse(pushed.content); + expect(parsed.error).toBe("invalid_args: data field required"); + expect(parsed.available_cache_uris).toEqual([ + { tool_name: "query_data", cache_uri: "cache://nrds-query" }, + ]); + }); + + it("dispatch error + truncation: oversized error envelope preserves available_cache_uris in the summary (HIGH feasibility finding)", async () => { + // Build a tool result that is large enough to cross the truncation + // threshold AND carries an `error` field. Without the + // pre-truncation enrichment ordering, `available_cache_uris` would + // silently drop here. + const largePayload = "x".repeat(25000); // > MAX_TOOL_RESULT_CHARS (20000) + const { connections, toolServerMap } = makeConnections({ + create_plotly_chart: { + error: "invalid_args: trace 0 data field empty", + fix_hint: "Provide data_uri or non-empty data", + details: largePayload, + }, + }); + + const messages = [ + toolMessage("query_data", { _cache_uri: "cache://nrds-query", rows: 240 }), + ]; + + await processToolCalls( + [makeToolCall("create_plotly_chart")], + messages, + connections, + toolServerMap, + makeFreshState(), + "", + { cacheOptions: CACHED_OPTIONS }, + ); + + const pushed = messages[messages.length - 1]; + const parsed = JSON.parse(pushed.content); + expect(parsed._truncated).toBe(true); + expect(parsed.error).toBe("invalid_args: trace 0 data field empty"); + expect(parsed.fix_hint).toBe("Provide data_uri or non-empty data"); + expect(parsed.available_cache_uris).toEqual([ + { tool_name: "query_data", cache_uri: "cache://nrds-query" }, + ]); + }); + + // Cache-miss site (engine/index.js:957) is intentionally NOT wired + // through `enrichErrorEnvelope`. The cache-miss envelope already names + // the failing URIs verbatim via `_missing_uris`; adding + // `available_cache_uris` would surface peer cache rows likely close to + // eviction and add no signal the LLM doesn't already have. + // + // Non-enrichment is enforced by the wire-up: enrichErrorEnvelope has + // exactly two call sites in engine/index.js — the inline-cap branch + // and the dispatch path. The cache-miss branch does not call it. This + // is checked by code inspection; an integration test would require + // fake-indexeddb to drive the substituteCacheUris miss path, which is + // covered by cache.test.js's harness in a different fixture. + + it("success path unchanged: dispatch returns a non-error envelope → no available_cache_uris added", async () => { + const { connections, toolServerMap } = makeConnections({ + list_intake_plugins: { plugins: ["a", "b", "c"] }, + }); + + const messages = [ + toolMessage("query_data", { _cache_uri: "cache://nrds-query", rows: 100 }), + ]; + + await processToolCalls( + [makeToolCall("list_intake_plugins")], + messages, + connections, + toolServerMap, + makeFreshState(), + "", + { cacheOptions: CACHED_OPTIONS }, + ); + + const pushed = messages[messages.length - 1]; + const parsed = JSON.parse(pushed.content); + expect(parsed.plugins).toEqual(["a", "b", "c"]); + expect(parsed.available_cache_uris).toBeUndefined(); + }); + + it("first turn (no prior tool messages): error envelope is pushed without available_cache_uris", async () => { + const { connections, toolServerMap } = makeConnections({ + create_plotly_chart: { error: "invalid_args: data field required" }, + }); + + const messages = []; // empty history + + await processToolCalls( + [makeToolCall("create_plotly_chart")], + messages, + connections, + toolServerMap, + makeFreshState(), + "", + { cacheOptions: CACHED_OPTIONS }, + ); + + const pushed = messages[messages.length - 1]; + const parsed = JSON.parse(pushed.content); + expect(parsed.error).toBe("invalid_args: data field required"); + expect(parsed.available_cache_uris).toBeUndefined(); + }); + + it("enableResultCache=false host: no available_cache_uris added at the dispatch site even with a prior _cache_uri", async () => { + const { connections, toolServerMap } = makeConnections({ + create_plotly_chart: { error: "invalid_args: data field required" }, + }); + + // Prior tool result happens to have `_cache_uri` in its serialized + // content (e.g., legacy conversation), but the current host has + // caching disabled. Helper short-circuits → field not added. + const messages = [ + toolMessage("query_data", { _cache_uri: "cache://legacy", rows: 100 }), + ]; + + await processToolCalls( + [makeToolCall("create_plotly_chart")], + messages, + connections, + toolServerMap, + makeFreshState(), + "", + { cacheOptions: UNCACHED_OPTIONS }, + ); + + const pushed = messages[messages.length - 1]; + const parsed = JSON.parse(pushed.content); + expect(parsed.error).toBe("invalid_args: data field required"); + expect(parsed.available_cache_uris).toBeUndefined(); + }); +}); diff --git a/engine/index.js b/engine/index.js index 06d367a..1d8d45e 100644 --- a/engine/index.js +++ b/engine/index.js @@ -816,6 +816,47 @@ function _substituteLastUuidPlaceholders(value, lastReturnedUuids) { return value; } +// Plan 2026-05-28-003 — enrich error envelopes with a structured pointer +// at the last `_cache_uri` per tool name from recent history. The LLM +// already sees the URI somewhere in conversation, but small/quantized +// models reliably drop it from attention by the time they need to retry +// a failed `create_*` tool. Surfacing the URI in the immediate error +// envelope prevents the wasted upstream re-query. +// +// Pure. Short-circuits when caching is disabled (the URI cannot exist) +// or the envelope isn't an error. Scans the last 10 `role: "tool"` +// messages backward; dedups by `tool_name` keeping the most recent. +export function enrichErrorEnvelope(envelope, messages, cacheOptions) { + if (cacheOptions?.enabled !== true) return envelope; + if (!envelope || typeof envelope !== "object") return envelope; + if (typeof envelope.error !== "string") return envelope; + if (!Array.isArray(messages) || messages.length === 0) return envelope; + + const collected = []; + const seenToolNames = new Set(); + let scanned = 0; + for (let i = messages.length - 1; i >= 0 && scanned < 10; i--) { + const m = messages[i]; + if (!m || m.role !== "tool") continue; + scanned++; + if (!m.tool_name || seenToolNames.has(m.tool_name)) continue; + let parsed; + try { + parsed = JSON.parse(m.content); + } catch { + continue; + } + if (!parsed || typeof parsed !== "object" || typeof parsed._cache_uri !== "string") { + continue; + } + seenToolNames.add(m.tool_name); + collected.push({ tool_name: m.tool_name, cache_uri: parsed._cache_uri }); + } + + if (collected.length === 0) return envelope; + return { ...envelope, available_cache_uris: collected }; +} + // --------------------------------------------------------------------------- // Generic Tool Processing // --------------------------------------------------------------------------- @@ -931,11 +972,21 @@ export async function processToolCalls( if (capError) { fireStatus({ type: "tool_start", toolName }); fireStatus({ type: "tool_complete", toolName, success: false }); + // Plan 2026-05-28-003 — pin the most recent `_cache_uri` per + // upstream tool into the envelope so the LLM doesn't have to + // scroll back through history to recover it. The cap envelope + // already names the `_cache_uri` path abstractly; this adds + // the concrete URI value. + const capEnvelope = enrichErrorEnvelope( + { ...capError, _engine_dispatched: [] }, + messages, + cacheOptions, + ); messages.push({ role: "tool", tool_call_id: toolCall.id || toolName, tool_name: toolName, - content: JSON.stringify({ ...capError, _engine_dispatched: [] }), + content: JSON.stringify(capEnvelope), }); hadError = true; lastErr = new Error(capError.error); @@ -1163,6 +1214,14 @@ export async function processToolCalls( resultForLlm._cache_uri = cacheUri; } } + + // Plan 2026-05-28-003 — pin available `_cache_uri` values into + // error envelopes BEFORE truncation (the truncation block below + // builds its summary from `resultForLlm` and serializes to + // `resultContent`; enriching after would silently drop the field + // on every oversized error result, exactly the case where the LLM + // most needs the hint). + resultForLlm = enrichErrorEnvelope(resultForLlm, messages, cacheOptions); } // Truncate large results before storing in conversation history. @@ -1257,6 +1316,14 @@ export async function processToolCalls( if (resultForLlm._cache_uri) { summary._cache_uri = resultForLlm._cache_uri; } + // Plan 2026-05-28-003 — preserve `available_cache_uris` into the + // truncation summary alongside `_cache_uri`. The enrichment runs + // before truncation (see above) so `resultForLlm` already has + // the field on error envelopes; manual copy keeps it through the + // summary build. + if (resultForLlm.available_cache_uris) { + summary.available_cache_uris = resultForLlm.available_cache_uris; + } resultContent = JSON.stringify(summary); } else { // K1 — non-object results never gain `_engine_dispatched`; keep diff --git a/package.json b/package.json index 56946fa..02ca6c8 100644 --- a/package.json +++ b/package.json @@ -1,6 +1,6 @@ { "name": "@aquaveo/chatbox-core", - "version": "0.16.0-beta.0", + "version": "0.16.1-beta.0", "description": "Generic chatbox engine, UI components, and helpers. Self-contained build — consumers only need react + styled-components.", "license": "MIT", "author": "Aquaveo LLC",