diff --git a/CHANGELOG.md b/CHANGELOG.md index a1c1e39..9f29806 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -32,6 +32,13 @@ the open-source home for EverMe CLI and agent plugins. ### Plugins +- Fix the agent SDK's dead transport-retry path by distinguishing semantic + reads from non-idempotent writes. `mem_context` and `mem_search` now retry + transient transport failures, HTTP 429, and HTTP 5xx once within the + original timeout budget; memory writes remain single-attempt. +- Add redacted structured SDK/MCP failure diagnostics (`classification`, + `causeCode`, `httpStatus`, `requestId`, `attempts`, `retryable`, and + `elapsedMs`) without exposing request queries, bodies, URLs, or tokens. - Open-source the plugin workspace under `plugins/`. - Include `@everme/agent-sdk`, the shared JavaScript client and helper package. - Include `@everme/memory-mcp`, the generic MCP memory server. diff --git a/docs/contracts.md b/docs/contracts.md index fcca2df..0dcd9d7 100644 --- a/docs/contracts.md +++ b/docs/contracts.md @@ -131,6 +131,42 @@ failures return MCP tool errors with `isError: true`; resource-read failures are thrown as MCP JSON-RPC errors so hosts can distinguish them from successful markdown content. +### MCP tool error diagnostics + +For backward compatibility, a tool failure still returns a short redacted text +line in `content[0].text`. When the failure reaches the MCP adapter through the +agent SDK, the same result also contains `structuredContent.error`: + +```json +{ + "classification": "http", + "causeCode": "HTTP_503", + "httpStatus": 503, + "requestId": "req_example", + "attempts": 2, + "retryable": true, + "elapsedMs": 152 +} +``` + +The fields are additive and machine-readable: + +| Field | Meaning | +|---|---| +| `classification` | Redacted failure class such as `transport`, `timeout`, `http`, `rate_limit`, `auth`, or `application`. | +| `causeCode` | Sanitized transport, HTTP, or EverMe code. It never contains native exception text. | +| `httpStatus` | HTTP status, or `0` when no response was received. | +| `requestId` | Sanitized upstream correlation id when available. | +| `attempts` | Number of actual network attempts made. | +| `retryable` | Whether another read attempt is semantically safe; writes report `false`. | +| `elapsedMs` | Total elapsed time across attempts and retry delay. | + +Diagnostics never include the memory query, request body, URL, token, or raw +fetch cause. `mem_context` and `mem_search` are semantic reads even though the +wire method is POST: they retry transient transport failures, HTTP 429, and +HTTP 5xx once within the original timeout budget. Memory writes are +non-idempotent and remain single-attempt. + ## MCP Tools The stable tool names are: diff --git a/plugins/agent-sdk/README.md b/plugins/agent-sdk/README.md index b4ea1b8..e0fa4e9 100644 --- a/plugins/agent-sdk/README.md +++ b/plugins/agent-sdk/README.md @@ -19,7 +19,7 @@ Without a shared SDK each plugin would reimplement the EverMe wire protocol — ```js import { // HTTP layer - createClient, EvermeError, redactError, + createClient, EvermeError, redactError, REQUEST_SEMANTICS, // Agent-memory realtime writes saveAgentMemory, AGENT_MEMORY_ROLES, AGENT_MEMORY_TOOL_CALL_TYPES, // Search / context @@ -51,9 +51,12 @@ The envelope every endpoint follows: ## Concurrency / safety contracts -- **Retry**: `execWithRetry` retries transport failures once — but only for GET/HEAD. POST writes (`/mem/agent-memory`) surface the transport error so the caller can decide; retrying a POST after a mid-flight drop can duplicate writes. +- **Request semantics**: GET/HEAD default to `safe_read`. The POST read endpoints `/mem/search` and `/mem/context` opt in explicitly through `REQUEST_SEMANTICS.SAFE_READ`. All other requests are `non_idempotent_write`; a caller cannot make a write retry merely by mislabelling it. +- **Retry**: safe reads make at most two attempts for transient transport failures, HTTP 429, and HTTP 5xx. Both attempts and any `Retry-After` delay share the original timeout budget. A timeout that consumes that budget is surfaced after one attempt rather than starting another full timeout. +- **Write safety**: POST writes such as `/mem/agent-memory` are attempted exactly once. A lost response can hide a server-side success, so blind retry can duplicate memory until every write path has a supported idempotency key. +- **Structured failures**: `EvermeError` includes `classification`, `causeCode`, `httpStatus`, `requestId`, `attempts`, `retryable`, and `elapsedMs`. These fields never include the request URL, query, body, token, or the native fetch cause message. - **Redaction**: `redactError` scrubs `evt_*`, `emk_*`, `X-Amz-Signature/Credential/Security-Token`, and AWS access key ids. Apply at every error sink before passing to host stderr / model context. -- **Timeouts**: `EvermeError{type:"timeout"}` is thrown so callers can branch on it (e.g. degrade to fallback) rather than retrying as if it were a transport blip. Body-read timeouts are caught too — a stuck body no longer silently parses as `null`. +- **Timeouts**: `EvermeError{type:"timeout", classification:"timeout"}` is thrown so callers can branch on it (e.g. degrade to fallback). Body-read timeouts are caught too — a stuck body no longer silently parses as `null`. The public CLI/MCP/token redaction contract is documented in [`../../docs/contracts.md`](../../docs/contracts.md). @@ -64,7 +67,9 @@ The public CLI/MCP/token redaction contract is documented in npm test ``` -Covers HTTP envelope, retry gating, config precedence, message normalization, agent-memory shaping. +Covers HTTP envelope, safe-read retry gating, write single-attempt safety, +structured error metadata, config precedence, message normalization, and +agent-memory shaping. ## License diff --git a/plugins/agent-sdk/index.js b/plugins/agent-sdk/index.js index b1f7e07..2d619f2 100644 --- a/plugins/agent-sdk/index.js +++ b/plugins/agent-sdk/index.js @@ -26,7 +26,7 @@ * dedicated module rather than reviving the multi-layer buffer. */ -export { createClient, EvermeError, redactError } from "./src/client.js"; +export { createClient, EvermeError, redactError, REQUEST_SEMANTICS } from "./src/client.js"; export { saveAgentMemory, convertAgentMessage, diff --git a/plugins/agent-sdk/src/client.js b/plugins/agent-sdk/src/client.js index 9f108df..c27a032 100644 --- a/plugins/agent-sdk/src/client.js +++ b/plugins/agent-sdk/src/client.js @@ -9,8 +9,8 @@ * - Two upload-flow timeouts: regular requests use TIMEOUT_MS (30s), * S3 multipart uploads use UPLOAD_TIMEOUT_MS (120s) — passed in by * upload.js when needed. - * - One soft retry on transient transport failures. The caller (assemble - * / save / search) decides whether to surface the error or degrade. + * - One soft retry, inside the original timeout budget, for explicitly + * safe reads. Non-idempotent writes are always attempted once. */ import { setTimeout as sleep } from "node:timers/promises"; @@ -18,6 +18,15 @@ import { TIMEOUT_MS } from "./config.js"; const noop = { info() {}, warn() {} }; +export const REQUEST_SEMANTICS = Object.freeze({ + SAFE_READ: "safe_read", + NON_IDEMPOTENT_WRITE: "non_idempotent_write", +}); + +const SAFE_READ_POST_PATHS = new Set(["/mem/search", "/mem/context"]); +const MAX_SAFE_READ_ATTEMPTS = 2; +const RETRY_DELAY_MS = 150; + /** * Token regex — used by redactError to scrub a literal token if it * accidentally lands in an error message (defense-in-depth, the same @@ -71,13 +80,36 @@ export function redactError(msg) { * surfaced from the envelope's requestId field for support correlation. */ export class EvermeError extends Error { - constructor({ message, status = 0, code = 0, requestId = "", type = "upstream" }) { + constructor({ + message, + status = 0, + code = 0, + requestId = "", + type = "upstream", + classification = type === "timeout" ? "timeout" : type === "auth" ? "auth" : "application", + causeCode = "", + attempts = 1, + retryable = false, + elapsedMs = 0, + retryAfterMs = 0, + }) { super(redactError(message)); this.name = "EvermeError"; this.httpStatus = status; this.code = code; this.requestId = requestId; this.type = type; + this.classification = classification; + this.causeCode = sanitizeCauseCode(causeCode); + this.attempts = attempts; + this.retryable = Boolean(retryable); + this.elapsedMs = elapsedMs; + Object.defineProperty(this, "retryAfterMs", { + value: Math.max(0, Number(retryAfterMs) || 0), + writable: true, + enumerable: false, + configurable: true, + }); } } @@ -86,6 +118,7 @@ export class EvermeError extends Error { * individual methods (e.g. inject a fake fetch via cfg). */ export function createClient(cfg, log = noop) { + const fetchImpl = cfg.fetch ?? globalThis.fetch; const headers = () => ({ "Content-Type": "application/json", Accept: "application/json", @@ -98,14 +131,20 @@ export function createClient(cfg, log = noop) { * `body` may be undefined for GET. Returns the envelope's result or * throws an EvermeError. */ - async function request(method, path, body, { timeoutMs = TIMEOUT_MS, query } = {}) { + async function request( + method, + path, + body, + { timeoutMs = TIMEOUT_MS, query, requestSemantics } = {}, + ) { const url = buildUrl(cfg.baseUrl, path, query); + const semantics = resolveRequestSemantics(method, path, requestSemantics); const init = { method, headers: headers(), body: body == null ? undefined : JSON.stringify(body), }; - return execWithRetry(url, init, timeoutMs, log); + return execWithRetry(url, init, timeoutMs, log, semantics, fetchImpl); } /** @@ -124,7 +163,7 @@ export function createClient(cfg, log = noop) { const headers = contentType ? { "Content-Type": contentType } : undefined; let res; try { - res = await fetch(uploadUrl, { + res = await fetchImpl(uploadUrl, { method: "POST", body, headers, @@ -203,50 +242,102 @@ function buildUrl(base, path, query) { return q ? `${base}${path}?${q}` : `${base}${path}`; } -async function execWithRetry(url, init, timeoutMs, log) { - try { - return await execOnce(url, init, timeoutMs); - } catch (err) { - if (err instanceof EvermeError) { - // Application-level errors don't get retried — only transport. - throw err; +function resolveRequestSemantics(method, path, requested) { + const normalizedMethod = String(method || "GET").toUpperCase(); + if (normalizedMethod === "GET" || normalizedMethod === "HEAD") { + return REQUEST_SEMANTICS.SAFE_READ; + } + if ( + requested === REQUEST_SEMANTICS.SAFE_READ && + normalizedMethod === "POST" && + SAFE_READ_POST_PATHS.has(path) + ) { + return REQUEST_SEMANTICS.SAFE_READ; + } + return REQUEST_SEMANTICS.NON_IDEMPOTENT_WRITE; +} + +async function execWithRetry(url, init, timeoutMs, log, semantics, fetchImpl) { + const startedAt = Date.now(); + const totalBudgetMs = Math.max(1, Number(timeoutMs) || TIMEOUT_MS); + const deadline = startedAt + totalBudgetMs; + const maxAttempts = + semantics === REQUEST_SEMANTICS.SAFE_READ ? MAX_SAFE_READ_ATTEMPTS : 1; + let attempts = 0; + + while (attempts < maxAttempts) { + const remainingMs = Math.max(0, deadline - Date.now()); + if (remainingMs <= 0) { + throw finalizeError( + new EvermeError({ + message: `timed out after ${totalBudgetMs}ms`, + type: "timeout", + classification: "timeout", + causeCode: "TIMEOUT", + retryable: true, + }), + attempts || 1, + startedAt, + semantics, + ); } - // Only GET is safe to retry on a transport error. POST /mem/agent-memory - // and other writes can succeed server-side after the response is lost - // mid-flight; retrying duplicates the write. Idempotency keys would let - // us retry safely, but the current backend contract doesn't accept one - // on every write path, so we err on the side of surfacing the error. - const method = (init?.method || "GET").toUpperCase(); - if (method !== "GET" && method !== "HEAD") { - throw err; + + attempts += 1; + try { + return await execOnce(url, init, remainingMs, fetchImpl); + } catch (caught) { + const err = normalizeRequestError(caught); + const canRetry = + semantics === REQUEST_SEMANTICS.SAFE_READ && + err.retryable && + attempts < maxAttempts; + if (!canRetry) { + throw finalizeError(err, attempts, startedAt, semantics); + } + + const delayMs = Math.max(RETRY_DELAY_MS, err.retryAfterMs || 0); + if (Date.now() + delayMs >= deadline) { + throw finalizeError(err, attempts, startedAt, semantics); + } + log.warn?.( + `[everme] safe read retry ${attempts + 1}/${maxAttempts} after ` + + `${err.classification}:${err.causeCode || "UNKNOWN"}`, + ); + await sleep(delayMs); } - log.warn?.(`[everme] ${method} failed, retrying once: ${redactError(err?.message)}`); - await sleep(150); - return execOnce(url, init, timeoutMs); } + + throw finalizeError( + new EvermeError({ + message: "request attempts exhausted", + classification: "transport", + causeCode: "ATTEMPTS_EXHAUSTED", + retryable: true, + }), + attempts, + startedAt, + semantics, + ); } -async function execOnce(url, init, timeoutMs) { +async function execOnce(url, init, timeoutMs, fetchImpl) { const ac = new AbortController(); const t = setTimeout(() => ac.abort(), timeoutMs); let res; let text = ""; try { try { - res = await fetch(url, { ...init, signal: ac.signal }); + res = await fetchImpl(url, { ...init, signal: ac.signal }); } catch (err) { - // Throw EvermeError (not plain Error) so execWithRetry's - // `instanceof EvermeError` short-circuit fires for timeouts — - // previously a timeout was caught as a transport error and - // retried, doubling the user-visible wait AND losing the - // type:"timeout" semantic that callers (assemble degrade, - // dispose) rely on. const aborted = ac.signal.aborted; throw new EvermeError({ message: aborted ? `timed out after ${timeoutMs}ms` - : redactError(err?.message || String(err)), + : "transport request failed", type: aborted ? "timeout" : "upstream", + classification: aborted ? "timeout" : "transport", + causeCode: aborted ? "TIMEOUT" : transportCauseCode(err), + retryable: true, }); } // Body read inherits the same AbortController so a stuck body still @@ -261,8 +352,11 @@ async function execOnce(url, init, timeoutMs) { throw new EvermeError({ message: aborted ? `timed out reading body after ${timeoutMs}ms` - : redactError(`body read failed: ${err?.message || String(err)}`), + : "transport body read failed", type: aborted ? "timeout" : "upstream", + classification: aborted ? "timeout" : "transport", + causeCode: aborted ? "TIMEOUT" : transportCauseCode(err, "BODY_READ_FAILED"), + retryable: true, }); } } finally { @@ -273,11 +367,33 @@ async function execOnce(url, init, timeoutMs) { try { env = text ? JSON.parse(text) : {}; } catch { - // Non-JSON response (load shedder, proxy 502 page, etc). + // Non-JSON response (load shedder, proxy 502 page, etc). Do not + // include the response body in the public error: it is untrusted and + // may contain reflected request data. throw new EvermeError({ - message: `HTTP ${res.status}${text ? " — " + text.slice(0, 200) : ""}`, + message: `HTTP ${res.status} returned a non-JSON response`, status: res.status, type: res.status === 401 || res.status === 403 ? "auth" : "upstream", + classification: classifyHttpStatus(res.status), + causeCode: `HTTP_${res.status}`, + retryable: isRetryableHttpStatus(res.status), + retryAfterMs: parseRetryAfterMs(res.headers?.get?.("Retry-After")), + }); + } + + if (!res.ok) { + const code = Number(env?.status) || 0; + const isAuth = res.status === 401 || res.status === 403 || isAuthCode(code); + throw new EvermeError({ + message: env?.error || `HTTP ${res.status}`, + status: res.status, + code, + requestId: env?.requestId, + type: isAuth ? "auth" : "upstream", + classification: isAuth ? "auth" : classifyHttpStatus(res.status), + causeCode: code ? `EVERME_${code}` : `HTTP_${res.status}`, + retryable: isRetryableHttpStatus(res.status), + retryAfterMs: parseRetryAfterMs(res.headers?.get?.("Retry-After")), }); } @@ -286,12 +402,69 @@ async function execOnce(url, init, timeoutMs) { } // Envelope-encoded failure or missing status. const code = Number(env?.status) || 0; - const errType = code >= 30000 && code < 30300 && code !== 30104 ? "auth" : "upstream"; + const errType = isAuthCode(code) ? "auth" : "upstream"; throw new EvermeError({ message: env?.error || `HTTP ${res.status}`, status: res.status, code, requestId: env?.requestId, type: errType, + classification: errType === "auth" ? "auth" : "application", + causeCode: code ? `EVERME_${code}` : "EVERME_APPLICATION_ERROR", + retryable: false, }); } + +function normalizeRequestError(err) { + if (err instanceof EvermeError) return err; + return new EvermeError({ + message: "transport request failed", + type: "upstream", + classification: "transport", + causeCode: transportCauseCode(err), + retryable: true, + }); +} + +function finalizeError(err, attempts, startedAt, semantics) { + err.attempts = Math.max(1, attempts || 1); + err.elapsedMs = Math.max(0, Date.now() - startedAt); + err.retryable = semantics === REQUEST_SEMANTICS.SAFE_READ && Boolean(err.retryable); + return err; +} + +function transportCauseCode(err, fallback = "FETCH_FAILED") { + return sanitizeCauseCode(err?.cause?.code || err?.code || fallback) || fallback; +} + +function sanitizeCauseCode(value) { + if (value == null || value === "") return ""; + const normalized = String(value) + .toUpperCase() + .replace(/[^A-Z0-9_]/g, "_") + .slice(0, 64); + return normalized || "UNKNOWN"; +} + +function classifyHttpStatus(status) { + if (status === 401 || status === 403) return "auth"; + if (status === 429) return "rate_limit"; + return "http"; +} + +function isAuthCode(code) { + return code >= 30000 && code < 30300 && code !== 30104; +} + +function isRetryableHttpStatus(status) { + return status === 429 || (status >= 500 && status <= 599); +} + +function parseRetryAfterMs(value) { + if (!value) return 0; + const seconds = Number(value); + if (Number.isFinite(seconds) && seconds >= 0) return Math.round(seconds * 1000); + const when = Date.parse(value); + if (!Number.isFinite(when)) return 0; + return Math.max(0, when - Date.now()); +} diff --git a/plugins/agent-sdk/src/search.js b/plugins/agent-sdk/src/search.js index c3a324d..00499de 100644 --- a/plugins/agent-sdk/src/search.js +++ b/plugins/agent-sdk/src/search.js @@ -15,6 +15,8 @@ * Both endpoints accept evt_ via Bearer (MemAuth + mem:search/mem:read). */ +import { REQUEST_SEMANTICS } from "./client.js"; + const noop = { info() {}, warn() {} }; // Mirror the backend's max search-query limit: /mem/search rejects a @@ -59,8 +61,10 @@ export async function searchMemory(client, params, log = noop) { ? { memoryTypes: params.memoryTypes } : {}), }; - log.info?.(`[everme] POST /mem/search topK=${body.topK} q="${truncate(body.query, 60)}"`); - const res = await client.request("POST", "/mem/search", body); + log.info?.(`[everme] POST /mem/search topK=${body.topK} queryChars=${body.query.length}`); + const res = await client.request("POST", "/mem/search", body, { + requestSemantics: REQUEST_SEMANTICS.SAFE_READ, + }); return { memories: res?.items ?? [], profiles: res?.profiles ?? [], @@ -81,7 +85,9 @@ export async function searchMemory(client, params, log = noop) { export async function getContext(client, _query, opts = {}, log = noop) { const body = opts.forceRefresh ? { forceRefresh: true } : {}; log.info?.(`[everme] POST /mem/context forceRefresh=${!!opts.forceRefresh}`); - const res = await client.request("POST", "/mem/context", body); + const res = await client.request("POST", "/mem/context", body, { + requestSemantics: REQUEST_SEMANTICS.SAFE_READ, + }); if (typeof res?.context === "string" && res.context) { return { context: res.context, memoryCount: res.memoryCount ?? estimateCount(res) }; } @@ -127,8 +133,3 @@ function estimateCount(res) { const arr = res.items || []; return Array.isArray(arr) ? arr.length : 0; } - -function truncate(s, n) { - s = String(s || ""); - return s.length > n ? s.slice(0, n) + "…" : s; -} diff --git a/plugins/agent-sdk/tests/client.test.js b/plugins/agent-sdk/tests/client.test.js index e6d3fdb..f5b20ea 100644 --- a/plugins/agent-sdk/tests/client.test.js +++ b/plugins/agent-sdk/tests/client.test.js @@ -3,6 +3,34 @@ import assert from "node:assert/strict"; import http from "node:http"; import { createClient, EvermeError } from "../src/client.js"; +const token = "evt_aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa"; + +function jsonResponse(body, status = 200, headers = {}) { + return new Response(JSON.stringify(body), { + status, + headers: { "Content-Type": "application/json", ...headers }, + }); +} + +function successResponse(result = { ok: true }) { + return jsonResponse({ error: "ok", status: 0, result, requestId: "req-ok" }); +} + +function fetchFailure(code, message = "fetch failed") { + const err = new TypeError(message); + err.cause = Object.assign(new Error("internal transport detail must stay private"), { code }); + return err; +} + +function clientWithFetch(fetchImpl) { + return createClient({ + baseUrl: "https://gateway.invalid/api/v1", + agentId: "agt_test", + agentToken: token, + fetch: fetchImpl, + }); +} + /** * Helper: spin up a tiny HTTP server, return its base URL + a * "respond" closure tests can use to set the next reply. @@ -116,4 +144,186 @@ describe("client", () => { await c.request("GET", "/agents", undefined, { query: { platform: "claude-code" } }); assert.equal(s.getLastRequest().url, "/agents?platform=claude-code"); }); + + test("retries a safe GET once after a socket reset", async (t) => { + t.after(async () => s.close()); + let calls = 0; + const c = clientWithFetch(async () => { + calls += 1; + if (calls === 1) throw fetchFailure("ECONNRESET"); + return successResponse({ recovered: true }); + }); + + const out = await c.request("GET", "/healthz"); + + assert.deepEqual(out, { recovered: true }); + assert.equal(calls, 2); + }); + + test("retries an explicitly safe POST read once after a transport failure", async (t) => { + t.after(async () => s.close()); + let calls = 0; + const c = clientWithFetch(async () => { + calls += 1; + if (calls === 1) throw fetchFailure("UND_ERR_SOCKET"); + return successResponse({ items: [] }); + }); + + const out = await c.request("POST", "/mem/search", { query: "q" }, { + requestSemantics: "safe_read", + }); + + assert.deepEqual(out, { items: [] }); + assert.equal(calls, 2); + }); + + test("never retries a non-idempotent write, even if a caller mislabels it", async (t) => { + t.after(async () => s.close()); + let calls = 0; + const c = clientWithFetch(async () => { + calls += 1; + throw fetchFailure("ECONNRESET"); + }); + + await assert.rejects( + c.request("POST", "/mem/agent-memory", { messages: [] }, { + requestSemantics: "safe_read", + }), + (err) => err instanceof EvermeError && err.attempts === 1, + ); + assert.equal(calls, 1); + }); + + test("returns structured redacted metadata after DNS retries are exhausted", async (t) => { + t.after(async () => s.close()); + let calls = 0; + const c = clientWithFetch(async () => { + calls += 1; + throw fetchFailure("ENOTFOUND", "fetch failed"); + }); + + await assert.rejects(c.request("GET", "/healthz"), (err) => { + assert.ok(err instanceof EvermeError); + assert.equal(err.classification, "transport"); + assert.equal(err.causeCode, "ENOTFOUND"); + assert.equal(err.attempts, 2); + assert.equal(err.retryable, true); + assert.equal(typeof err.elapsedMs, "number"); + assert.doesNotMatch(JSON.stringify(err), /internal transport detail/); + return true; + }); + assert.equal(calls, 2); + }); + + test("keeps a timeout inside one shared budget and reports it without a retry storm", async (t) => { + t.after(async () => s.close()); + let calls = 0; + const c = clientWithFetch((_url, { signal }) => { + calls += 1; + return new Promise((resolve, reject) => { + signal.addEventListener("abort", () => { + const err = new Error("aborted"); + err.name = "AbortError"; + reject(err); + }, { once: true }); + }); + }); + + const startedAt = Date.now(); + await assert.rejects( + c.request("GET", "/slow", undefined, { timeoutMs: 25 }), + (err) => + err instanceof EvermeError && + err.classification === "timeout" && + err.causeCode === "TIMEOUT" && + err.attempts === 1 && + err.retryable === true, + ); + assert.equal(calls, 1); + assert.ok(Date.now() - startedAt < 250, "timeout retry budget must stay bounded"); + }); + + test("retries a safe read once on HTTP 429", async (t) => { + t.after(async () => s.close()); + let calls = 0; + const c = clientWithFetch(async () => { + calls += 1; + if (calls === 1) { + return jsonResponse( + { error: "rate limited", status: 42900, requestId: "req-rate" }, + 429, + { "Retry-After": "0" }, + ); + } + return successResponse({ recovered: true }); + }); + + const out = await c.request("POST", "/mem/context", {}, { + requestSemantics: "safe_read", + }); + assert.deepEqual(out, { recovered: true }); + assert.equal(calls, 2); + }); + + test("retries a safe read once on HTTP 5xx", async (t) => { + t.after(async () => s.close()); + let calls = 0; + const c = clientWithFetch(async () => { + calls += 1; + if (calls === 1) { + return jsonResponse({ error: "unavailable", status: 0, requestId: "req-503" }, 503); + } + return successResponse({ recovered: true }); + }); + + const out = await c.request("GET", "/healthz"); + assert.deepEqual(out, { recovered: true }); + assert.equal(calls, 2); + }); + + test("does not retry HTTP 401 and preserves the request id", async (t) => { + t.after(async () => s.close()); + let calls = 0; + const c = clientWithFetch(async () => { + calls += 1; + return jsonResponse( + { error: "invalid token", status: 30001, requestId: "req-auth" }, + 401, + ); + }); + + await assert.rejects(c.request("GET", "/healthz"), (err) => { + assert.ok(err instanceof EvermeError); + assert.equal(err.type, "auth"); + assert.equal(err.classification, "auth"); + assert.equal(err.requestId, "req-auth"); + assert.equal(err.attempts, 1); + assert.equal(err.retryable, false); + return true; + }); + assert.equal(calls, 1); + }); + + test("preserves envelope auth classification on non-401 HTTP errors", async (t) => { + t.after(async () => s.close()); + let calls = 0; + const c = clientWithFetch(async () => { + calls += 1; + return jsonResponse( + { error: "invalid token", status: 30001, requestId: "req-auth-400" }, + 400, + ); + }); + + await assert.rejects(c.request("GET", "/healthz"), (err) => { + assert.ok(err instanceof EvermeError); + assert.equal(err.type, "auth"); + assert.equal(err.classification, "auth"); + assert.equal(err.causeCode, "EVERME_30001"); + assert.equal(err.requestId, "req-auth-400"); + assert.equal(err.attempts, 1); + return true; + }); + assert.equal(calls, 1); + }); }); diff --git a/plugins/agent-sdk/tests/search.test.js b/plugins/agent-sdk/tests/search.test.js index 63f95bc..8367841 100644 --- a/plugins/agent-sdk/tests/search.test.js +++ b/plugins/agent-sdk/tests/search.test.js @@ -1,13 +1,13 @@ import { test, describe } from "node:test"; import assert from "node:assert/strict"; -import { searchMemory, QUERY_MAX_CHARS } from "../src/search.js"; +import { getContext, searchMemory, QUERY_MAX_CHARS } from "../src/search.js"; describe("searchMemory", () => { test("posts query + topK to /mem/search and renames items → memories", async () => { const calls = []; const client = { - async request(method, path, body) { - calls.push({ method, path, body }); + async request(method, path, body, options) { + calls.push({ method, path, body, options }); return { items: [{ summary: "hit" }], profiles: [], requestId: "req-1" }; }, }; @@ -19,10 +19,28 @@ describe("searchMemory", () => { assert.equal(calls[0].path, "/mem/search"); assert.equal(calls[0].body.query, "what do I prefer"); assert.equal(calls[0].body.topK, 3); + assert.equal(calls[0].options.requestSemantics, "safe_read"); assert.equal(res.memories.length, 1); assert.equal(res.requestId, "req-1"); }); + test("marks /mem/context as a safe semantic read", async () => { + const calls = []; + const client = { + async request(method, path, body, options) { + calls.push({ method, path, body, options }); + return { context: "remembered", memoryCount: 1 }; + }, + }; + + const res = await getContext(client, "unused"); + + assert.deepEqual(res, { context: "remembered", memoryCount: 1 }); + assert.equal(calls.length, 1); + assert.equal(calls[0].path, "/mem/context"); + assert.equal(calls[0].options.requestSemantics, "safe_read"); + }); + test("clamps an over-long query to the backend's MaxSearchQueryRunes", async () => { // The backend rejects query > 1024 runes; the SDK is the single // chokepoint to /mem/search, so an oversized prompt from any caller @@ -49,4 +67,17 @@ describe("searchMemory", () => { await searchMemory(client, { query: ok }); assert.equal(sent.query, ok, "exactly-at-limit query is untouched"); }); + + test("logs search metadata without persisting the query text", async () => { + const logs = []; + const client = { async request() { return { items: [] }; } }; + const query = "private regression phrase"; + + await searchMemory(client, { query, topK: 7 }, { info: (line) => logs.push(line), warn() {} }); + + assert.equal(logs.length, 1); + assert.doesNotMatch(logs[0], /private regression phrase/); + assert.match(logs[0], /topK=7/); + assert.match(logs[0], new RegExp(`queryChars=${query.length}`)); + }); }); diff --git a/plugins/memory-mcp/README.md b/plugins/memory-mcp/README.md index ed53b6a..c580926 100644 --- a/plugins/memory-mcp/README.md +++ b/plugins/memory-mcp/README.md @@ -30,6 +30,25 @@ Read-only MCP resources are also exposed for hosts that support The public MCP contract is documented in [`../../docs/contracts.md`](../../docs/contracts.md). +Tool failures keep the backward-compatible `isError: true` text response and +also expose redacted diagnostics at `structuredContent.error`: + +```json +{ + "classification": "transport", + "causeCode": "ECONNRESET", + "httpStatus": 0, + "requestId": "", + "attempts": 2, + "retryable": true, + "elapsedMs": 154 +} +``` + +The diagnostic object never includes the memory query, request body, token, +URL, or native fetch error text. Read tools use the SDK's bounded safe-read +retry; write tools remain single-attempt. + ## Wire it into a host's mcpServers config ```json @@ -52,7 +71,7 @@ The public MCP contract is documented in ## Architecture -Thin adapter on top of [`@everme/agent-sdk`](https://www.npmjs.com/package/@everme/agent-sdk). This package owns the MCP framing (stdio + JSON-RPC), tool list, and dispatch. Everything else — HTTP client, realtime agent-memory writes, search/context calls, retry, redaction — comes from the SDK. +Thin adapter on top of [`@everme/agent-sdk`](https://www.npmjs.com/package/@everme/agent-sdk). This package owns the MCP framing (stdio + JSON-RPC), tool list, dispatch, and structured MCP error projection. Everything else — HTTP client, realtime agent-memory writes, search/context calls, retry, and redaction — comes from the SDK. ## Tests diff --git a/plugins/memory-mcp/src/mcp.js b/plugins/memory-mcp/src/mcp.js index 687ba24..d7954e9 100644 --- a/plugins/memory-mcp/src/mcp.js +++ b/plugins/memory-mcp/src/mcp.js @@ -480,7 +480,7 @@ export function createMcpServer({ logger } = {}) { // — so the scrub MUST run here, not just in EvermeError. const safe = redactError(err?.message || String(err)); log.warn?.(`[everme-mcp] tool ${name} failed: ${safe}`); - return errResp(safe); + return errResp(safe, structuredErrorDetail(err)); } }); @@ -757,9 +757,57 @@ function okMarkdown(text) { // errResp accepts pre-redacted text. Callers MUST run redactError on // any non-EvermeError input before passing it in (the catch block // above does this; if you add another caller, do the same). -function errResp(msg) { - return { +// +// SDK-originated failures also carry a structuredContent.error object. +// The text line remains for older hosts; structuredContent lets newer +// hosts and automation ledgers classify failures without parsing prose. +function errResp(msg, errorDetail) { + const response = { isError: true, content: [{ type: "text", text: `error: ${msg}` }], }; + if (errorDetail) response.structuredContent = { error: errorDetail }; + return response; +} + +function structuredErrorDetail(err) { + if (!err || typeof err !== "object") return null; + const classification = safeLabel( + err.classification || + (err.type === "timeout" ? "timeout" : err.type === "auth" ? "auth" : "internal"), + "internal", + ).toLowerCase(); + const causeCode = safeLabel(err.causeCode || "UNEXPECTED_ERROR", "UNEXPECTED_ERROR"); + const httpStatus = boundedInteger(err.httpStatus, 0, 999, 0); + const attempts = boundedInteger(err.attempts, 1, 10, 1); + const elapsedMs = boundedInteger(err.elapsedMs, 0, Number.MAX_SAFE_INTEGER, 0); + const requestId = safeOpaqueId(err.requestId); + return { + classification, + causeCode, + httpStatus, + requestId, + attempts, + retryable: Boolean(err.retryable), + elapsedMs, + }; +} + +function safeLabel(value, fallback) { + const normalized = String(value ?? "") + .toUpperCase() + .replace(/[^A-Z0-9_]/g, "_") + .slice(0, 64); + return normalized || fallback; +} + +function safeOpaqueId(value) { + const safe = redactError(String(value ?? "")).slice(0, 128); + return /^[A-Za-z0-9._:-]*$/.test(safe) ? safe : ""; +} + +function boundedInteger(value, min, max, fallback) { + const n = Number(value); + if (!Number.isFinite(n)) return fallback; + return Math.min(max, Math.max(min, Math.round(n))); } diff --git a/plugins/memory-mcp/tests/resources.test.js b/plugins/memory-mcp/tests/resources.test.js index fcefe09..b0b9b49 100644 --- a/plugins/memory-mcp/tests/resources.test.js +++ b/plugins/memory-mcp/tests/resources.test.js @@ -666,6 +666,83 @@ describe("mem_context / mem_search tools return raw markdown (no JSON envelope)" }); }); +describe("MCP tool failures preserve structured SDK diagnostics", { skip: !sdkAvailable && "SDK not installed" }, () => { + test("safe reads retry once while writes remain single-attempt", async () => { + const http = await import("node:http"); + const { Client } = await import("@modelcontextprotocol/sdk/client/index.js"); + const { InMemoryTransport } = await import("@modelcontextprotocol/sdk/inMemory.js"); + + let contextCalls = 0; + let writeCalls = 0; + const mockServer = http.createServer((req, res) => { + if (req.url === "/api/v1/mem/context" && req.method === "POST") { + contextCalls += 1; + } else if (req.url === "/api/v1/mem/agent-memory" && req.method === "POST") { + writeCalls += 1; + } else { + res.writeHead(404); + res.end(); + return; + } + res.writeHead(503, { "Content-Type": "application/json" }); + res.end(JSON.stringify({ + status: 0, + requestId: "req-test-outage", + error: "temporarily unavailable", + })); + }); + await new Promise((resolve) => mockServer.listen(0, "127.0.0.1", resolve)); + const port = mockServer.address().port; + + process.env.EVERME_API_BASE = `http://127.0.0.1:${port}`; + process.env.EVERME_AGENT_ID = "agt_x"; + process.env.EVERME_AGENT_TOKEN = "evt_aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa"; + const { createMcpServer } = await import("../src/mcp.js"); + const { server, dispose } = createMcpServer(); + const [c, s] = InMemoryTransport.createLinkedPair(); + const client = new Client({ name: "t", version: "0" }, { capabilities: {} }); + await Promise.all([server.connect(s), client.connect(c)]); + + try { + const readResult = await client.callTool({ + name: "mem_context", + arguments: { query: "test input" }, + }); + assert.equal(readResult.isError, true); + assert.equal(contextCalls, 2, "semantic read must use the one-retry budget"); + assert.deepEqual(readResult.structuredContent?.error, { + classification: "http", + causeCode: "HTTP_503", + httpStatus: 503, + requestId: "req-test-outage", + attempts: 2, + retryable: true, + elapsedMs: readResult.structuredContent?.error?.elapsedMs, + }); + assert.equal(typeof readResult.structuredContent.error.elapsedMs, "number"); + assert.doesNotMatch(readResult.content[0].text, /test input/, + "the query must never be copied into the error result"); + + const writeResult = await client.callTool({ + name: "mem_save_turn", + arguments: { + messages: [{ role: "user", content: "test input" }], + flush: false, + }, + }); + assert.equal(writeResult.isError, true); + assert.equal(writeCalls, 1, "non-idempotent write must never be retried"); + assert.equal(writeResult.structuredContent?.error?.attempts, 1); + assert.equal(writeResult.structuredContent?.error?.retryable, false); + } finally { + await client.close(); + await server.close(); + await dispose(); + await new Promise((resolve) => mockServer.close(resolve)); + } + }); +}); + describe("mem_save_fact dispatch (profile write path)", { skip: !sdkAvailable && "SDK not installed" }, () => { // mem_save_fact must hit /mem/personal (profile + episodic), NOT // /mem/agent-memory (case/skill). Routing it to the agent path would