From 5534f3b1afd49d5309e6285116a1a4ce449fc447 Mon Sep 17 00:00:00 2001 From: Levalicious <24482560+Levalicious@users.noreply.github.com> Date: Thu, 8 Oct 2026 01:48:08 -0400 Subject: [PATCH] feat(api): error envelope + validate-first atomicity (policy R1/R2/R4) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Implements docs/api-error-policy.md: errors are signal. - ToolError + stable codes; the request handler converts them into visible structured tool errors (isError + JSON envelope) — protocol errors are now reserved for genuine server faults (R1) - validate-first, all-or-nothing batches for create_entities, create_relations, add_observations, sequentialthinking (R2; fixes the mid-loop partial application in add_observations) - silent drops become errors (R4): missing relation endpoints (ENDPOINT_MISSING), all-missing open_nodes, unknown type, unknown get_neighbors/random_walk starts, invalid regex, kb_load file errors, unlinked thoughts - search NL-guard returns the same envelope (NO_MATCHES + suggestion) - tests: 13 protocol-error assertions converted to envelope checks; new policy block (atomicity proofs, envelope shape); fuzz now shows protoErr=0 with every rejection visible as toolErr - full suite 183/183 Co-Authored-By: Claude Code --- docs/api-error-policy.md | 89 +++++++++++ server.ts | 288 +++++++++++++++++++++++++++------ tests/memory-server.test.ts | 309 ++++++++++++++++++++++++------------ tests/test-utils.ts | 30 ++++ 4 files changed, 568 insertions(+), 148 deletions(-) create mode 100644 docs/api-error-policy.md diff --git a/docs/api-error-policy.md b/docs/api-error-policy.md new file mode 100644 index 0000000..72eec41 --- /dev/null +++ b/docs/api-error-policy.md @@ -0,0 +1,89 @@ +# API Error & Result Policy + +Status: **adopted 2026-10-08** (implementation ongoing — see [Status](#status)). + +This document is the contract for how every MCP tool reports failure and +success. It replaces the original "never fail, funnel the caller into +unfailing completion" approach: with RL-era models, errors are signal — a +silent no-op or a hidden protocol error teaches the caller nothing and lets +wrong beliefs persist (KB: `BGS_API_ErrorInconsistency`, +`Inventory_APISilentDrops_2026_10_08`). + +## Rules + +**R1 — Visible failures.** +Every failure the caller can act on is returned as a tool-level error: +`isError: true` with a JSON envelope body: + +```json +{ + "error": { + "tool": "create_relations", + "code": "ENDPOINT_MISSING", + "message": "2 relation(s) reference nonexistent entities; nothing was created (details below).", + "details": [ + { "from": "A", "to": "Ghost", "relationType": "USES", "code": "ENDPOINT_MISSING", + "missing": ["to"], "message": "endpoint(s) not found: to" } + ] + } +} +``` + +Stable codes: `ENTITY_NOT_FOUND`, `TYPE_NOT_FOUND`, `ENDPOINT_MISSING`, +`COLLISION`, `LIMIT_EXCEEDED`, `INVALID_REGEX`, `INVALID_FILE`, +`NO_MATCHES`, `VALIDATION_FAILED`. JSON-RPC protocol errors are reserved for +genuine server faults and are never used for caller-actionable failures. + +**R2 — Validate-first, atomic.** +A mutation validates the entire request under the write lock before touching +state, and is all-or-nothing: a batch containing a failing item changes +nothing, and the error lists every offending item in one round-trip. + +**R3 — Per-item ledger.** +Every mutating success enumerates per-item outcomes +(`created` / `existing` / `skippedDuplicates` / `notFound` / `deleted` / +`added` / `alreadyPresent`). There is no bare "successfully" response. + +**R4 — No silent drops.** +An item that cannot be satisfied errors (R1) unless the caller's intent is +provably satisfied — duplicates are "already there", deleting an absent +thing is "already absent". Those cases succeed **and are reported** (R3). + +**R5 — Pagination integrity.** +Cursors identify the result set they came from (a fingerprint now; a store +txid once the owner daemon lands). A stale/mismatched cursor returns a +`CURSOR_STALE` error instead of a silently wrong page. + +## Per-op policy + +| Tool | On failure | On success | +|---|---|---| +| `create_entities` | same-name-different-data → `COLLISION` (atomic, all offenders); limit breaches → `LIMIT_EXCEEDED` | `{created, existing}` — exact duplicates are success | +| `create_relations` | any endpoint missing → `ENDPOINT_MISSING` (atomic, all offenders) | `{created, skippedDuplicates}` | +| `add_observations` | unknown entity → `ENTITY_NOT_FOUND`; limit breaches → `LIMIT_EXCEEDED` (atomic, all offenders) | per-entity `{entityName, addedObservations, alreadyPresent}` | +| `delete_entities` / `delete_relations` / `delete_observations` | nothing errors — absence is `notFound` (R4) | `{deleted, notFound}` ledgers | +| `sequentialthinking` | unknown `previousCtxId` → `ENTITY_NOT_FOUND` (chain integrity); limits → `LIMIT_EXCEEDED` | `{ctxId, linkedTo}` | +| `kb_load` | bad extension / unreadable file → `INVALID_FILE`; entity & relation stages per their rows | per-stage ledger (document, stats, entity/relation outcomes) | +| `open_nodes` | ALL requested names missing → `ENTITY_NOT_FOUND` | graph result + `missing[]` for partial misses | +| `get_neighbors` | unknown start → `ENTITY_NOT_FOUND` | neighbors (depth 0 = immediate) | +| `search_nodes` | invalid ERE → `INVALID_REGEX`; literal query, zero matches → `NO_MATCHES` | paginated graph | +| `get_entities_by_type` | type absent from the data-derived type set → `TYPE_NOT_FOUND` | entities of that type | +| `random_walk` | unknown start → `ENTITY_NOT_FOUND` | `{entity, path, modeUsed, stopReason}` | +| `find_path` | — (β-contract already reports `targetReached` / `budgetExhausted` / `note`) | unchanged — the reference pattern for R1-style reporting | +| reads (`get_entity_types`, `get_relation_types`, `get_stats`, `get_orphaned_entities`, `validate_graph`, `decode_timestamp`) | unchanged | unchanged | + +Decisions of record (Lev, 2026-10-08): duplicates are ledger reports, not +errors; the unified surface is `isError` with protocol errors reserved for +faults; Q1 = atomic-fail on missing endpoints; Q2/Q3 = error. + +## Status + +- **This change (envelope PR):** R1 (envelope + codes + dispatch conversion), + R2 (validate-first atomicity for `create_entities`, `create_relations`, + `add_observations`, `sequentialthinking`), R4 (drops→errors: + `ENDPOINT_MISSING`, all-missing `open_nodes`, `TYPE_NOT_FOUND`, unknown + starts), `INVALID_FILE`, `INVALID_REGEX`, `NO_MATCHES` envelope. +- **Ledger PR (next):** R3 result ledgers + report fields (`missing[]`, + `modeUsed`, `alreadyPresent`), R5 cursor fingerprinting. +- **v4 line (`feat/libsegstore`):** inherits this policy at reconciliation; + kbd4's batch=txn=commit model is the natural home for R2 atomicity. diff --git a/server.ts b/server.ts index 5054455..e2731a0 100644 --- a/server.ts +++ b/server.ts @@ -19,14 +19,48 @@ import { toolDurationHistogram, traced, tracer } from './src/tracing.js'; /** * Result envelope for a single tool dispatch. Mirrors the MCP CallToolResult * shape we return from every case; `isError: true` signals a tool-level error - * that the LLM should see (vs. a thrown error which becomes a hidden JSON-RPC - * protocol error). + * that the LLM should see. Caller-actionable failures are thrown as + * {@link ToolError} and converted into that visible shape by the request + * handler; any other exception propagates as a protocol error and is reserved + * for genuine server faults. */ type ToolDispatchResult = { content: Array<{ type: 'text'; text: string }>; isError?: boolean; }; +/** + * Stable, machine-readable error codes carried in the tool-error envelope. + * This list is part of the public contract (docs/api-error-policy.md). + */ +export type ToolErrorCode = + | 'ENTITY_NOT_FOUND' + | 'TYPE_NOT_FOUND' + | 'ENDPOINT_MISSING' + | 'COLLISION' + | 'LIMIT_EXCEEDED' + | 'INVALID_REGEX' + | 'INVALID_FILE' + | 'NO_MATCHES' + | 'VALIDATION_FAILED'; + +/** + * A failure the CALLER can act on. The request handler converts these into a + * VISIBLE, structured tool error (`isError: true` with a JSON envelope) — + * never a protocol error. Protocol errors stay reserved for genuine server + * faults. See docs/api-error-policy.md (R1). + */ +export class ToolError extends Error { + constructor( + public code: ToolErrorCode, + message: string, + public details?: unknown, + ) { + super(message); + this.name = 'ToolError'; + } +} + /** * True if the query contains any regex metacharacter. Used to gate the * `search_nodes` natural-language warning: a pure literal that returns no @@ -581,22 +615,43 @@ export class KnowledgeGraphManager { } async createEntities(entities: Entity[]): Promise { - // Validate observation limits (can do outside lock) + // Validate observation limits across the WHOLE batch first, collecting + // every violation rather than stopping at the first (R2: validate-first; + // the caller gets all offenders in one round-trip). + const limitViolations: { entity: string; code: ToolErrorCode; message: string }[] = []; for (const entity of entities) { if (entity.observations.length > 2) { - throw new Error(`Entity "${entity.name}" has ${entity.observations.length} observations. Maximum allowed is 2.`); + limitViolations.push({ + entity: entity.name, + code: 'LIMIT_EXCEEDED', + message: `has ${entity.observations.length} observations; maximum allowed is 2`, + }); } - for (const obs of entity.observations) { + entity.observations.forEach((obs, index) => { if (obs.length > 140) { - throw new Error(`Observation in entity "${entity.name}" exceeds 140 characters (${obs.length} chars): "${obs.substring(0, 50)}..."`); + limitViolations.push({ + entity: entity.name, + code: 'LIMIT_EXCEEDED', + message: `observation #${index + 1} is ${obs.length} chars (140 max, ${obs.length - 140} over)`, + }); } - } + }); + } + if (limitViolations.length > 0) { + throw new ToolError( + 'LIMIT_EXCEEDED', + `${limitViolations.length} limit violation(s) in the batch; nothing was created (details below).`, + limitViolations, + ); } return this.withWriteLock(() => { const now = BigInt(Date.now()); - const newEntities: Entity[] = []; + // Pass 1 — classify every entity before writing anything. A collision + // fails the whole batch (R2: atomic) and lists every offender. + const toCreate: Entity[] = []; + const collisions: { entity: string; code: ToolErrorCode; message: string }[] = []; for (const e of entities) { const existingOffset = this.db.lookup(e.name); if (existingOffset !== 0n) { @@ -604,10 +659,27 @@ export class KnowledgeGraphManager { const sameType = existing.entityType === e.entityType; const sameObs = existing.observations.length === e.observations.length && existing.observations.every((o, i) => o === e.observations[i]); - if (sameType && sameObs) continue; - throw new Error(`Entity "${e.name}" already exists with different data (type: "${existing.entityType}" vs "${e.entityType}", observations: ${existing.observations.length} vs ${e.observations.length})`); + if (sameType && sameObs) continue; // exact duplicate: intent satisfied + collisions.push({ + entity: e.name, + code: 'COLLISION', + message: `exists with different data (type: "${existing.entityType}" vs "${e.entityType}", observations: ${existing.observations.length} vs ${e.observations.length})`, + }); + } else { + toCreate.push(e); } + } + if (collisions.length > 0) { + throw new ToolError( + 'COLLISION', + `${collisions.length} entity name collision(s); nothing was created (details below).`, + collisions, + ); + } + // Pass 2 — create. + const newEntities: Entity[] = []; + for (const e of toCreate) { const offset = this.db.createEntity(e.name, e.entityType, now); for (const obs of e.observations) { this.db.addObservation(offset, obs, now); @@ -628,12 +700,40 @@ export class KnowledgeGraphManager { async createRelations(relations: Relation[]): Promise { return this.withWriteLock(() => { const now = BigInt(Date.now()); - const newRelations: Relation[] = []; + // Pass 1 — every endpoint must exist; missing endpoints fail the whole + // batch (R2/R4: a relation to a nonexistent entity is a state desync, + // never a silent drop). + const missing: { from: string; to: string; relationType: string; code: ToolErrorCode; missing: string[]; message: string }[] = []; + for (const r of relations) { + const missingEnds: string[] = []; + if (this.db.lookup(r.from) === 0n) missingEnds.push('from'); + if (this.db.lookup(r.to) === 0n) missingEnds.push('to'); + if (missingEnds.length > 0) { + missing.push({ + from: r.from, + to: r.to, + relationType: r.relationType, + code: 'ENDPOINT_MISSING', + missing: missingEnds, + message: `endpoint(s) not found: ${missingEnds.join(', ')}`, + }); + } + } + if (missing.length > 0) { + throw new ToolError( + 'ENDPOINT_MISSING', + `${missing.length} relation(s) reference nonexistent entities; nothing was created (details below).`, + missing, + ); + } + + // Pass 2 — create. Duplicates are already-satisfied intent and are + // skipped (they become part of the result ledger under policy R3). + const newRelations: Relation[] = []; for (const r of relations) { const fromOffset = this.db.lookup(r.from); const toOffset = this.db.lookup(r.to); - if (fromOffset === 0n || toOffset === 0n) continue; const isDuplicate = this.db.edges(fromOffset).some(e => e.direction === DIR_FORWARD && e.target === toOffset && e.relType === r.relationType @@ -651,33 +751,58 @@ export class KnowledgeGraphManager { async addObservations(observations: { entityName: string; contents: string[] }[]): Promise<{ entityName: string; addedObservations: string[] }[]> { return this.withWriteLock(() => { - const results: { entityName: string; addedObservations: string[] }[] = []; + // Pass 1 — validate the WHOLE batch before touching state (R2: atomic; + // the old code validated inside the write loop, so a late failure left + // earlier items applied). Collect every violation for one round-trip. + const pending: { entityName: string; offset: bigint; newObservations: string[] }[] = []; + const violations: { entityName: string; code: ToolErrorCode; message: string }[] = []; for (const o of observations) { const offset = this.db.lookup(o.entityName); if (offset === 0n) { - throw new Error(`Entity with name ${o.entityName} not found`); + violations.push({ entityName: o.entityName, code: 'ENTITY_NOT_FOUND', message: 'entity does not exist' }); + continue; } - for (const obs of o.contents) { + o.contents.forEach((obs, index) => { if (obs.length > 140) { - throw new Error(`Observation for "${o.entityName}" exceeds 140 characters (${obs.length} chars): "${obs.substring(0, 50)}..."`); + violations.push({ + entityName: o.entityName, + code: 'LIMIT_EXCEEDED', + message: `observation #${index + 1} is ${obs.length} chars (140 max, ${obs.length - 140} over)`, + }); } - } + }); const existingObs = this.db.readEntity(offset).observations; const newObservations = o.contents.filter(content => !existingObs.includes(content)); - if (existingObs.length + newObservations.length > 2) { - throw new Error(`Adding ${newObservations.length} observations to "${o.entityName}" would exceed limit of 2 (currently has ${existingObs.length}).`); + violations.push({ + entityName: o.entityName, + code: 'LIMIT_EXCEEDED', + message: `adding ${newObservations.length} observation(s) would give ${existingObs.length + newObservations.length} total; maximum is 2`, + }); } + pending.push({ entityName: o.entityName, offset, newObservations }); + } - const now = BigInt(Date.now()); - for (const obs of newObservations) { - this.db.addObservation(offset, obs, now); - } + if (violations.length > 0) { + const codes = new Set(violations.map(v => v.code)); + throw new ToolError( + codes.size === 1 ? violations[0].code : 'VALIDATION_FAILED', + `${violations.length} observation(s) failed validation; nothing was written (details below).`, + violations, + ); + } - results.push({ entityName: o.entityName, addedObservations: newObservations }); + // Pass 2 — apply. + const now = BigInt(Date.now()); + const results: { entityName: string; addedObservations: string[] }[] = []; + for (const p of pending) { + for (const obs of p.newObservations) { + this.db.addObservation(p.offset, obs, now); + } + results.push({ entityName: p.entityName, addedObservations: p.newObservations }); } return results; @@ -742,7 +867,7 @@ export class KnowledgeGraphManager { // query is never rejected by a divergent JS RegExp dialect, and an invalid one // is rejected consistently. Preserves the "Invalid regex pattern" contract. if (!this.db.regexValid(query)) { - throw new Error(`Invalid regex pattern: ${query}`); + throw new ToolError('INVALID_REGEX', `Invalid regex pattern: ${query}`); } return traced( @@ -789,6 +914,17 @@ export class KnowledgeGraphManager { offsetByName.set(name, offset); } + // Q2 (docs/api-error-policy.md): when NONE of the requested names exist, + // the request is a typo/desync — error instead of returning an empty + // graph (a partial miss is reported via the R3 ledger). + if (names.length > 0 && filteredEntities.length === 0) { + throw new ToolError( + 'ENTITY_NOT_FOUND', + `None of the ${names.length} requested entities exist.`, + { missing: names }, + ); + } + const filteredEntityNames = new Set(filteredEntities.map(e => e.name)); const filteredRelations: Relation[] = []; @@ -838,7 +974,7 @@ export class KnowledgeGraphManager { const startOffset = this.db.lookup(entityName); if (startOffset === 0n) { span.setAttribute('kb.traversal.start_found', false); - return []; + throw new ToolError('ENTITY_NOT_FOUND', `Start entity not found: ${entityName}`); } span.setAttribute('kb.traversal.start_found', true); @@ -989,7 +1125,17 @@ export class KnowledgeGraphManager { async getEntitiesByType(entityType: string, sortBy?: EntitySortField, sortDir?: SortDirection): Promise { return this.withReadLock(() => { - const filtered = this.getAllEntities().filter(e => e.entityType === entityType); + const all = this.getAllEntities(); + // Q3 (docs/api-error-policy.md): the type set is data-derived, so an + // absent type can never have entities — a miss is a typo/desync, not an + // empty result. + if (!all.some(e => e.entityType === entityType)) { + throw new ToolError( + 'TYPE_NOT_FOUND', + `Entity type "${entityType}" is not present in the graph. See get_entity_types for the full list.`, + ); + } + const filtered = all.filter(e => e.entityType === entityType); const rankMaps = this.getRankMapsUnlocked(); return sortEntities(filtered, sortBy, sortDir, rankMaps); }); @@ -1136,7 +1282,7 @@ export class KnowledgeGraphManager { (span) => this.withReadLock(() => { const startOffset = this.db.lookup(start); if (startOffset === 0n) { - throw new Error(`Start entity not found: ${start}`); + throw new ToolError('ENTITY_NOT_FOUND', `Start entity not found: ${start}`); } // Seeded walk: hash the string seed to a u64 the C RNG can use. A @@ -1203,33 +1349,45 @@ export class KnowledgeGraphManager { } async addThought(observations: string[], previousCtxId?: string): Promise<{ ctxId: string }> { - // Validate observations (can do outside lock) + // Validate observation limits across the whole request first. + const violations: { code: ToolErrorCode; message: string }[] = []; if (observations.length > 2) { - throw new Error(`Thought has ${observations.length} observations. Maximum allowed is 2.`); + violations.push({ code: 'LIMIT_EXCEEDED', message: `Thought has ${observations.length} observations; maximum allowed is 2` }); } - for (const obs of observations) { + observations.forEach((obs, index) => { if (obs.length > 140) { - throw new Error(`Observation exceeds 140 characters (${obs.length} chars): "${obs.substring(0, 50)}..."`); + violations.push({ code: 'LIMIT_EXCEEDED', message: `observation #${index + 1} is ${obs.length} chars (140 max, ${obs.length - 140} over)` }); } + }); + if (violations.length > 0) { + throw new ToolError('LIMIT_EXCEEDED', `${violations.length} limit violation(s); nothing was created (details below).`, violations); } return this.withWriteLock(() => { const now = BigInt(Date.now()); - const ctxId = randomBytes(12).toString('hex'); + // Validate the chain link BEFORE creating the thought (R2: atomic; a + // silently-unlinked thought is exactly the FORGOT_EVIDENCE failure the + // chain exists to prevent). + let prevOffset = 0n; + if (previousCtxId) { + prevOffset = this.db.lookup(previousCtxId); + if (prevOffset === 0n) { + throw new ToolError('ENTITY_NOT_FOUND', `previousCtxId "${previousCtxId}" does not exist; refusing to create an unlinked thought.`); + } + } + + const ctxId = randomBytes(12).toString('hex'); const offset = this.db.createEntity(ctxId, 'Thought', now); for (const obs of observations) { this.db.addObservation(offset, obs, now); } if (previousCtxId) { - const prevOffset = this.db.lookup(previousCtxId); - if (prevOffset !== 0n) { - // prev --follows--> new, and new --preceded_by--> prev. C creates - // both directed edges per relation and owns the refcounts. - this.db.createRelation(prevOffset, offset, 'follows', now); - this.db.createRelation(offset, prevOffset, 'preceded_by', now); - } + // prev --follows--> new, and new --preceded_by--> prev. C creates + // both directed edges per relation and owns the refcounts. + this.db.createRelation(prevOffset, offset, 'follows', now); + this.db.createRelation(offset, prevOffset, 'preceded_by', now); } return { ctxId }; @@ -1629,9 +1787,12 @@ The file MUST be plaintext (.txt, .tex, .md, source code, etc.). For PDFs, use p /** * Dispatch a single tool call. Extracted from the request handler so the * handler can wrap it with span/metric instrumentation without duplicating - * the per-tool logic. Returns the MCP `CallToolResult` shape; thrown errors - * become JSON-RPC protocol errors (hidden from the model), while - * `{ isError: true }` returns are visible tool-level errors. + * the per-tool logic. Returns the MCP `CallToolResult` shape. + * + * Error policy (docs/api-error-policy.md): caller-actionable failures are + * thrown as `ToolError`s — the request handler converts them into VISIBLE + * structured tool errors (`isError: true` + JSON envelope). Any other throw + * remains a JSON-RPC protocol error and is reserved for server faults. */ async function dispatch(name: string, args: Record): Promise { switch (name) { @@ -1677,11 +1838,16 @@ The file MUST be plaintext (.txt, .tex, .md, source code, etc.). For PDFs, use p const suggestion = suggested && suggested !== query ? ` For multiple terms try ${JSON.stringify(suggested)}.` : ''; + const envelope = { + error: { + tool: 'search_nodes', + code: 'NO_MATCHES', + message: `No matches for ${JSON.stringify(query)}. search_nodes uses POSIX Extended Regular Expressions (ERE), case-sensitive — not natural language, and not JS/PCRE regex (use [0-9] not \\d, [[:alpha:]] not \\w; no lookahead or backreferences).${suggestion} You can also browse with get_entities_by_type, get_neighbors, or random_walk.`, + ...(suggestion && { details: { suggestedRegex: suggested } }), + }, + }; return { - content: [{ - type: "text", - text: `No matches for ${JSON.stringify(query)}. search_nodes uses POSIX Extended Regular Expressions (ERE), case-sensitive — not natural language, and not JS/PCRE regex (use [0-9] not \\d, [[:alpha:]] not \\w; no lookahead or backreferences).${suggestion} You can also browse with get_entities_by_type, get_neighbors, or random_walk.`, - }], + content: [{ type: "text", text: JSON.stringify(envelope, null, 2) }], isError: true, }; } @@ -1787,14 +1953,18 @@ The file MUST be plaintext (.txt, .tex, .md, source code, etc.). For PDFs, use p const filePath = args.filePath as string; // Validate extension - validateExtension(filePath); + try { + validateExtension(filePath); + } catch (err: unknown) { + throw new ToolError('INVALID_FILE', err instanceof Error ? err.message : String(err)); + } // Read file let text: string; try { text = fs.readFileSync(filePath, 'utf-8'); } catch (err: unknown) { - throw new Error(`Failed to read file: ${err instanceof Error ? err.message : String(err)}`); + throw new ToolError('INVALID_FILE', `Failed to read file: ${err instanceof Error ? err.message : String(err)}`); } // Derive title @@ -1887,6 +2057,26 @@ The file MUST be plaintext (.txt, .tex, .md, source code, etc.). For PDFs, use p } return result; } catch (err) { + if (err instanceof ToolError) { + // Policy R1 (docs/api-error-policy.md): caller-actionable failures + // surface as VISIBLE tool errors with a structured envelope — + // never as protocol errors. + errorType = `tool_error:${err.code}`; + span.setAttribute('error.type', errorType); + span.setStatus({ code: SpanStatusCode.ERROR, message: err.message }); + const envelope = { + error: { + tool: name, + code: err.code, + message: err.message, + ...(err.details !== undefined && { details: err.details }), + }, + }; + return { + content: [{ type: "text", text: JSON.stringify(envelope, null, 2) }], + isError: true, + }; + } errorType = (err as Error)?.constructor?.name ?? 'Error'; span.setAttribute('error.type', errorType); span.recordException(err as Error); diff --git a/tests/memory-server.test.ts b/tests/memory-server.test.ts index 5017c5a..52ff6b5 100644 --- a/tests/memory-server.test.ts +++ b/tests/memory-server.test.ts @@ -3,7 +3,7 @@ import { promises as fs } from 'fs'; import path from 'path'; import os from 'os'; import { createServer, type Entity, type Relation, type Neighbor } from '../server.js'; -import { createTestClient, callTool, callToolRaw, type PaginatedGraph, type PaginatedResult, type FindPathResult } from './test-utils.js'; +import { createTestClient, callTool, callToolRaw, parseToolError, type PaginatedGraph, type PaginatedResult, type FindPathResult } from './test-utils.js'; describe('MCP Memory Server E2E Tests', () => { let testDir: string; @@ -64,49 +64,49 @@ describe('MCP Memory Server E2E Tests', () => { expect(result[0].name).toBe('Bob'); }); - it('should error on duplicate name with different data', async () => { + it('should error on duplicate name with different data (COLLISION, visible)', async () => { await callTool(client, 'create_entities', { entities: [{ name: 'Alice', entityType: 'Person', observations: ['First'] }] }); - // Same name, different type — should error - await expect( - callTool(client, 'create_entities', { - entities: [{ name: 'Alice', entityType: 'Organization', observations: ['First'] }] - }) - ).rejects.toThrow(/already exists/); + // Same name, different type — visible tool error (policy R1) + const rawType = await callToolRaw(client, 'create_entities', { + entities: [{ name: 'Alice', entityType: 'Organization', observations: ['First'] }] + }); + expect(rawType.isError).toBe(true); + expect(parseToolError(rawType).code).toBe('COLLISION'); - // Same name, different observations — should error - await expect( - callTool(client, 'create_entities', { - entities: [{ name: 'Alice', entityType: 'Person', observations: ['Different'] }] - }) - ).rejects.toThrow(/already exists/); + // Same name, different observations — also visible + const rawObs = await callToolRaw(client, 'create_entities', { + entities: [{ name: 'Alice', entityType: 'Person', observations: ['Different'] }] + }); + expect(rawObs.isError).toBe(true); + expect(parseToolError(rawObs).code).toBe('COLLISION'); }); - it('should reject entities with more than 2 observations', async () => { - await expect( - callTool(client, 'create_entities', { - entities: [{ - name: 'TooMany', - entityType: 'Test', - observations: ['One', 'Two', 'Three'] - }] - }) - ).rejects.toThrow(/Maximum allowed is 2/); + it('should reject entities with more than 2 observations (LIMIT_EXCEEDED, visible)', async () => { + const raw = await callToolRaw(client, 'create_entities', { + entities: [{ + name: 'TooMany', + entityType: 'Test', + observations: ['One', 'Two', 'Three'] + }] + }); + expect(raw.isError).toBe(true); + expect(parseToolError(raw).code).toBe('LIMIT_EXCEEDED'); }); - it('should reject observations longer than 140 characters', async () => { + it('should reject observations longer than 140 characters (LIMIT_EXCEEDED, visible)', async () => { const longObservation = 'x'.repeat(141); - await expect( - callTool(client, 'create_entities', { - entities: [{ - name: 'LongObs', - entityType: 'Test', - observations: [longObservation] - }] - }) - ).rejects.toThrow(/exceeds 140 characters/); + const raw = await callToolRaw(client, 'create_entities', { + entities: [{ + name: 'LongObs', + entityType: 'Test', + observations: [longObservation] + }] + }); + expect(raw.isError).toBe(true); + expect(parseToolError(raw).code).toBe('LIMIT_EXCEEDED'); }); it('should delete entities and their relations', async () => { @@ -158,16 +158,16 @@ describe('MCP Memory Server E2E Tests', () => { expect(result[0].addedObservations).toEqual(['New']); }); - it('should reject adding observations that would exceed limit', async () => { + it('should reject adding observations that would exceed limit (LIMIT_EXCEEDED, visible)', async () => { await callTool(client, 'add_observations', { observations: [{ entityName: 'TestEntity', contents: ['One', 'Two'] }] }); - await expect( - callTool(client, 'add_observations', { - observations: [{ entityName: 'TestEntity', contents: ['Three'] }] - }) - ).rejects.toThrow(/would exceed limit of 2/); + const raw = await callToolRaw(client, 'add_observations', { + observations: [{ entityName: 'TestEntity', contents: ['Three'] }] + }); + expect(raw.isError).toBe(true); + expect(parseToolError(raw).code).toBe('LIMIT_EXCEEDED'); }); it('should delete specific observations', async () => { @@ -300,10 +300,10 @@ describe('MCP Memory Server E2E Tests', () => { expect(result.entities.items[0].name).toBe('TypeScript'); }); - it('should reject invalid regex', async () => { - await expect( - callTool(client, 'search_nodes', { query: '[invalid' }) - ).rejects.toThrow(/Invalid regex pattern/); + it('should reject invalid regex (INVALID_REGEX, visible)', async () => { + const raw = await callToolRaw(client, 'search_nodes', { query: '[invalid' }); + expect(raw.isError).toBe(true); + expect(parseToolError(raw).code).toBe('INVALID_REGEX'); }); it('accepts ERE-valid patterns that JS RegExp rejects (validator uses the C ERE engine)', async () => { @@ -315,11 +315,11 @@ describe('MCP Memory Server E2E Tests', () => { }); it('rejects ERE-invalid patterns even when JS RegExp accepts them', async () => { - // '(?:x)' is a valid JS non-capturing group but invalid POSIX ERE — must throw, - // not silently return zero matches. - await expect( - callTool(client, 'search_nodes', { query: '(?:x)' }) - ).rejects.toThrow(/Invalid regex pattern/); + // '(?:x)' is a valid JS non-capturing group but invalid POSIX ERE — must + // be a visible error, not silently zero matches. + const raw = await callToolRaw(client, 'search_nodes', { query: '(?:x)' }); + expect(raw.isError).toBe(true); + expect(parseToolError(raw).code).toBe('INVALID_REGEX'); }); it('trigram path: regex-extractable queries return the same results as before', async () => { @@ -495,11 +495,12 @@ describe('MCP Memory Server E2E Tests', () => { }); expect(raw.isError).toBe(true); - const text = raw.content[0]?.text ?? ''; - expect(text).toContain('knowledge graph features'); - expect(text).toContain('regex'); + const err = parseToolError(raw); + expect(err.code).toBe('NO_MATCHES'); + expect(err.message).toContain('knowledge graph features'); + expect(err.message).toContain('regex'); // Auto-suggested |-joined regex should appear. - expect(text).toContain('knowledge|graph|features'); + expect(err.message).toContain('knowledge|graph|features'); }); it('does NOT flag a regex query with anchors that simply misses', async () => { @@ -539,11 +540,12 @@ describe('MCP Memory Server E2E Tests', () => { }); expect(raw.isError).toBe(true); - const text = raw.content[0]?.text ?? ''; - expect(text).toContain('Slef'); - expect(text).toContain('regex'); + const err = parseToolError(raw); + expect(err.code).toBe('NO_MATCHES'); + expect(err.message).toContain('Slef'); + expect(err.message).toContain('regex'); // No multi-term suggestion because there's only one term. - expect(text).not.toContain('"Slef|'); + expect(err.message).not.toContain('"Slef|'); }); // Note: a "walker bias" test is intentionally omitted because the guard @@ -1201,29 +1203,25 @@ describe('MCP Memory Server E2E Tests', () => { expect(neighbors.items.some(n => n.name === second.ctxId)).toBe(true); }); - it('should ignore invalid previousCtxId gracefully', async () => { - const result = await callTool(client, 'sequentialthinking', { + it('should refuse an invalid previousCtxId (visible ENTITY_NOT_FOUND)', async () => { + // Policy R4: a silently-unlinked thought is exactly the evidence-loss + // failure the chain exists to prevent — an unknown link is an error. + const raw = await callToolRaw(client, 'sequentialthinking', { previousCtxId: 'nonexistent_thought', observations: ['Orphaned thought'] - }) as { ctxId: string }; - - expect(result.ctxId).toMatch(/^[0-9a-f]{24}$/); - - // Verify no neighbors (no valid relations were created) - const neighbors = await callTool(client, 'get_neighbors', { - entityName: result.ctxId, - depth: 1 - }) as PaginatedResult; - - expect(neighbors.items).toHaveLength(0); + }); + expect(raw.isError).toBe(true); + const err = parseToolError(raw); + expect(err.code).toBe('ENTITY_NOT_FOUND'); + expect(err.message).toContain('nonexistent_thought'); }); - it('should enforce observation limits on thoughts', async () => { - await expect( - callTool(client, 'sequentialthinking', { - observations: ['One', 'Two', 'Three'] - }) - ).rejects.toThrow(/Maximum allowed is 2/); + it('should enforce observation limits on thoughts (LIMIT_EXCEEDED, visible)', async () => { + const raw = await callToolRaw(client, 'sequentialthinking', { + observations: ['One', 'Two', 'Three'] + }); + expect(raw.isError).toBe(true); + expect(parseToolError(raw).code).toBe('LIMIT_EXCEEDED'); }); it('should set mtime and obsMtime on thought entities', async () => { @@ -1360,10 +1358,10 @@ describe('MCP Memory Server E2E Tests', () => { expect(result1.path).toEqual(result2.path); }); - it('should throw on non-existent start entity', async () => { - await expect( - callTool(client, 'random_walk', { start: 'NonExistent', depth: 2 }) - ).rejects.toThrow(/not found/); + it('should return a visible ENTITY_NOT_FOUND for a non-existent start entity', async () => { + const raw = await callToolRaw(client, 'random_walk', { start: 'NonExistent', depth: 2 }); + expect(raw.isError).toBe(true); + expect(parseToolError(raw).code).toBe('ENTITY_NOT_FOUND'); }); it('should accept mode=uniform and produce a valid walk', async () => { @@ -1965,28 +1963,30 @@ describe('MCP Memory Server E2E Tests', () => { docFile = path.join(testDir, 'test-doc.txt'); }); - it('should reject non-plaintext extensions', async () => { + it('should reject non-plaintext extensions (INVALID_FILE, visible)', async () => { const pdfPath = path.join(testDir, 'test.pdf'); await fs.writeFile(pdfPath, 'fake pdf content'); - await expect( - callTool(client, 'kb_load', { filePath: pdfPath }) - ).rejects.toThrow(/Unsupported file extension/); + const raw = await callToolRaw(client, 'kb_load', { filePath: pdfPath }); + expect(raw.isError).toBe(true); + const err = parseToolError(raw); + expect(err.code).toBe('INVALID_FILE'); + expect(err.message).toContain('Unsupported file extension'); }); - it('should reject files with no extension', async () => { + it('should reject files with no extension (INVALID_FILE, visible)', async () => { const noExtPath = path.join(testDir, 'noext'); await fs.writeFile(noExtPath, 'some content'); - await expect( - callTool(client, 'kb_load', { filePath: noExtPath }) - ).rejects.toThrow(/no extension/); + const raw = await callToolRaw(client, 'kb_load', { filePath: noExtPath }); + expect(raw.isError).toBe(true); + expect(parseToolError(raw).code).toBe('INVALID_FILE'); }); - it('should reject missing files', async () => { - await expect( - callTool(client, 'kb_load', { filePath: path.join(testDir, 'nonexistent.txt') }) - ).rejects.toThrow(/Failed to read file/); + it('should reject missing files (INVALID_FILE, visible)', async () => { + const raw = await callToolRaw(client, 'kb_load', { filePath: path.join(testDir, 'nonexistent.txt') }); + expect(raw.isError).toBe(true); + expect(parseToolError(raw).code).toBe('INVALID_FILE'); }); it('should load a small document and create entities + relations', async () => { @@ -2095,14 +2095,14 @@ describe('MCP Memory Server E2E Tests', () => { await fs.writeFile(docFile, 'Short doc for dedup testing purposes here.'); await callTool(client, 'kb_load', { filePath: docFile }); - // Second load with different content but same title — Document entity - // already exists with entityType 'Document' and no observations, - // so it gets silently skipped. But the index entities already exist - // with different observations, so it should error. + // Second load with different content but same title: the Document entity + // is an exact duplicate (intent satisfied), but the index entities collide + // with different observations → visible COLLISION (policy R1), and the + // failed load is atomic per op. await fs.writeFile(docFile, 'Completely different content for dedup testing now.'); - await expect( - callTool(client, 'kb_load', { filePath: docFile }) - ).rejects.toThrow(/already exists/); + const raw = await callToolRaw(client, 'kb_load', { filePath: docFile }); + expect(raw.isError).toBe(true); + expect(parseToolError(raw).code).toBe('COLLISION'); }); it('should enforce observation length limits', async () => { @@ -2137,4 +2137,115 @@ describe('MCP Memory Server E2E Tests', () => { } }); }); + + describe('API error policy (R1/R2/R4)', () => { + it('create_entities collision is atomic: nothing in the batch is created', async () => { + await callTool(client, 'create_entities', { + entities: [{ name: 'PolicyDup', entityType: 'PolicyTag', observations: ['base'] }], + }); + + const raw = await callToolRaw(client, 'create_entities', { + entities: [ + { name: 'PolicyFresh', entityType: 'PolicyTag', observations: ['new'] }, + { name: 'PolicyDup', entityType: 'PolicyTag', observations: ['changed'] }, + ], + }); + + expect(raw.isError).toBe(true); + const err = parseToolError(raw); + expect(err.code).toBe('COLLISION'); + expect(Array.isArray(err.details)).toBe(true); + + // Atomicity: PolicyFresh must NOT exist. Anchored regex → plain empty + // result (a literal query would route through the NL guard instead). + const fresh = await callTool(client, 'search_nodes', { query: '^PolicyFresh$' }) as PaginatedGraph; + expect(fresh.entities.items).toEqual([]); + }); + + it('create_relations with a missing endpoint errors and writes nothing', async () => { + await callTool(client, 'create_entities', { + entities: [ + { name: 'PolicyX', entityType: 'PolicyTag', observations: [] }, + { name: 'PolicyY', entityType: 'PolicyTag', observations: [] }, + ], + }); + + const raw = await callToolRaw(client, 'create_relations', { + relations: [ + { from: 'PolicyX', to: 'PolicyY', relationType: 'LINKS' }, + { from: 'PolicyX', to: 'PolicyGhost', relationType: 'LINKS' }, + ], + }); + + expect(raw.isError).toBe(true); + expect(parseToolError(raw).code).toBe('ENDPOINT_MISSING'); + + // Atomicity: the valid relation in the same batch was not created. + const after = await callTool(client, 'open_nodes', { names: ['PolicyX'] }) as PaginatedGraph; + expect(after.relations.items).toEqual([]); + }); + + it('add_observations validates the whole batch first (atomic)', async () => { + await callTool(client, 'create_entities', { + entities: [{ name: 'PolicyObs', entityType: 'PolicyTag', observations: [] }], + }); + + const raw = await callToolRaw(client, 'add_observations', { + observations: [ + { entityName: 'PolicyObs', contents: ['would-have-been-added'] }, + { entityName: 'PolicyGhost', contents: ['x'] }, + ], + }); + + expect(raw.isError).toBe(true); + expect(parseToolError(raw).code).toBe('ENTITY_NOT_FOUND'); + + // Atomicity: the first item must not have been applied. + const after = await callTool(client, 'open_nodes', { names: ['PolicyObs'] }) as PaginatedGraph; + expect(after.entities.items[0]?.observations).toEqual([]); + }); + + it('add_observations limit errors carry exact char counts', async () => { + await callTool(client, 'create_entities', { + entities: [{ name: 'PolicyObs2', entityType: 'PolicyTag', observations: [] }], + }); + + const raw = await callToolRaw(client, 'add_observations', { + observations: [{ entityName: 'PolicyObs2', contents: ['y'.repeat(151)] }], + }); + + expect(raw.isError).toBe(true); + const err = parseToolError(raw); + expect(err.code).toBe('LIMIT_EXCEEDED'); + // Precise counts live in the per-item details: "151 chars (…11 over)". + expect(JSON.stringify(err.details)).toContain('151'); + }); + + it('open_nodes errors when ALL requested names are missing', async () => { + const raw = await callToolRaw(client, 'open_nodes', { names: ['NoSuchOne', 'NoSuchTwo'] }); + expect(raw.isError).toBe(true); + const err = parseToolError(raw); + expect(err.code).toBe('ENTITY_NOT_FOUND'); + expect(err.details).toEqual({ missing: ['NoSuchOne', 'NoSuchTwo'] }); + }); + + it('get_entities_by_type errors on an unknown type', async () => { + const raw = await callToolRaw(client, 'get_entities_by_type', { entityType: 'NoSuchType' }); + expect(raw.isError).toBe(true); + expect(parseToolError(raw).code).toBe('TYPE_NOT_FOUND'); + }); + + it('get_neighbors errors on an unknown start entity', async () => { + const raw = await callToolRaw(client, 'get_neighbors', { entityName: 'NoSuchEntity' }); + expect(raw.isError).toBe(true); + expect(parseToolError(raw).code).toBe('ENTITY_NOT_FOUND'); + }); + + it('error envelopes carry the tool name and a message naming the offender', async () => { + const raw = await callToolRaw(client, 'get_neighbors', { entityName: 'NoSuchEntity' }); + const err = parseToolError(raw); + expect(err.tool).toBe('get_neighbors'); + expect(err.message).toContain('NoSuchEntity'); + }); + }); }); diff --git a/tests/test-utils.ts b/tests/test-utils.ts index 60e3961..ea99ebe 100644 --- a/tests/test-utils.ts +++ b/tests/test-utils.ts @@ -147,3 +147,33 @@ export async function callToolRaw( isError: (result as { isError?: boolean }).isError, }; } + +/** + * The structured body of a tool-level error, per policy R1 + * (docs/api-error-policy.md): `{ tool, code, message, details? }`. + */ +export interface ToolErrorEnvelope { + tool: string; + code: string; + message: string; + details?: unknown; +} + +/** + * Parse the JSON envelope out of a `callToolRaw` result. Throws if the result + * isn't a tool-error envelope, so tests fail loudly when a call unexpectedly + * succeeds or returns something unstructured. + */ +export function parseToolError(raw: RawToolResult): ToolErrorEnvelope { + const text = raw.content[0]?.text ?? ''; + let parsed: { error?: ToolErrorEnvelope }; + try { + parsed = JSON.parse(text) as { error?: ToolErrorEnvelope }; + } catch { + throw new Error(`Expected a JSON tool-error envelope, got non-JSON: ${text.slice(0, 200)}`); + } + if (!parsed.error) { + throw new Error(`Expected a tool-error envelope, got: ${text.slice(0, 200)}`); + } + return parsed.error; +}