diff --git a/CHANGELOG.md b/CHANGELOG.md index d681d05..452dd80 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -250,6 +250,18 @@ This project follows [Semantic Versioning](https://semver.org/spec/v2.0.0.html). generator refuses to draw a line that would be clipped or to accept a transcript line that is a near-miss of the format. +- **A rejected request body now uses the protocol's error shape.** FastAPI raises + `RequestValidationError` before a route runs, so it never passed through + `AMPError`: a malformed body answered `{"detail": [...]}` while every other error + answered `{"error": {"code", "message", "details"}}`. The API reference + documented `422 VALIDATION_ERROR` all along, and both SDKs read `error.code` - so + a caller that sent a bad field got a generic "HTTP error 422" and no idea which + field was wrong, which is the same failure the single error shape was introduced + to remove. The field errors now ride in `error.details.errors`, a rejected value + that is not JSON-serialisable cannot turn the handler into a 500, and the + conformance suite gained a vector so any implementation is held to the same + shape (39 vectors now). + ### Changed - **Every endpoint returns one error shape.** `PATCH /memories/{id}` answered a conflict with `{"detail": ...}` while `DELETE` answered with diff --git a/README.md b/README.md index c1a2d8f..3047637 100644 --- a/README.md +++ b/README.md @@ -102,7 +102,7 @@ needs no dependencies at all (Node 18+ has `fetch`). | **Specification** | The `MemoryCell` schema, the decay formula, the lifecycle state machine, the threat model | [`spec/v0.1.0/`](spec/v0.1.0/) · [explained in plain English](docs/spec-explained.md) | | **Reference server** | FastAPI, with your choice of storage: embedded ChromaDB, or PostgreSQL + `pgvector`. Includes an MCP server, so an LLM client can use memory as tools | [`server/`](server/) · [API reference](docs/api-reference.md) | | **Client SDKs** | Python (sync, async, LangChain) and Node (zero dependencies) | [`sdk/`](sdk/) | -| **Conformance suite** | 38 checks to run against your own implementation. It imports nothing from this repo's server, so it judges your implementation rather than comparing it to ours | [`conformance/`](conformance/) | +| **Conformance suite** | 39 checks to run against your own implementation. It imports nothing from this repo's server, so it judges your implementation rather than comparing it to ours | [`conformance/`](conformance/) | | **OpenAPI contract** | Generated from the server and committed, so an API change shows up as a reviewable diff | [`spec/v0.1.0/openapi.json`](spec/v0.1.0/openapi.json) | Implementing AMP yourself? `amp-conformance --base-url http://your-server` is the @@ -181,7 +181,7 @@ Worth reading before you build on this: developed on has no PostgreSQL, so the storage contract suite runs against a real `pgvector` container in CI and skips locally. -248 server tests, 34 Python SDK tests, 24 Node tests and 38 conformance vectors run +248 server tests, 34 Python SDK tests, 24 Node tests and 39 conformance vectors run in CI across Python 3.11/3.12 and Node 18/20/22, with lint, format and type gates on every package. diff --git a/conformance/README.md b/conformance/README.md index afde675..e062b85 100644 --- a/conformance/README.md +++ b/conformance/README.md @@ -49,7 +49,7 @@ into CI. |---|---|---| | `schema` | no | Standalone `MemoryCell` documents against `spec/v0.1.0/memory-cell.schema.json`, both documents that must validate and documents that must not. | | `decay` | no | `decay_score = importance x confidence x e^(-decay_rate x delta_days)` recomputed from `spec/v0.1.0/lifecycle.md`, including the stale threshold at `0.3` and the half-life. | -| `http_contract` | yes | Status codes and error bodies: a missing agent identity is `401`, an unknown cell is `403` and never `404`, only an archived cell can be deleted, an archived cell cannot return to `active`, and a write cannot reach `deleted`. | +| `http_contract` | yes | Status codes and error bodies: a missing agent identity is `401`, an unknown cell is `403` and never `404`, only an archived cell can be deleted, an archived cell cannot return to `active`, a write cannot reach `deleted`, and a body the schema rejects answers the same `{\"error\": {\"code\", \"message\", \"details\"}}` envelope as everything else rather than a framework's default shape. | | `access_control` | yes | The read and write decision for every agent in a policy matrix, checked through three surfaces at once: `GET` (read), `PATCH` (write) and `POST /memories/search` (read through the ranking path). | | `spec_capabilities` | yes | The declarations at `GET /spec`, against the server's own numbers: an advertised `max_cell_size_bytes` has to be the actual maximum, an advertised `manual_run_endpoint` has to be a route that exists, and `/spec` and `/health` have to agree on the version. | diff --git a/conformance/amp_conformance/vectors/http_contract.json b/conformance/amp_conformance/vectors/http_contract.json index a314372..5a049fa 100644 --- a/conformance/amp_conformance/vectors/http_contract.json +++ b/conformance/amp_conformance/vectors/http_contract.json @@ -265,6 +265,25 @@ "expect": {"status": 409, "error_code": "INVALID_TRANSITION"} } ] + }, + { + "id": "rejected-body-uses-the-error-envelope", + "steps": [ + { + "request": { + "method": "POST", + "path": "/memories", + "headers": {"X-AMP-Agent-ID": "agent-conformance"}, + "json": {"type": "semantic"} + }, + "expect": { + "status": 422, + "error_code": "VALIDATION_ERROR", + "has_fields": ["error"], + "json_equals": {"error.code": "VALIDATION_ERROR"} + } + } + ] } ] -} \ No newline at end of file +} diff --git a/docs/api-reference.md b/docs/api-reference.md index 6082e1f..1edc284 100644 --- a/docs/api-reference.md +++ b/docs/api-reference.md @@ -735,4 +735,8 @@ All error responses use the following structure: |-------|-------------| | `error.code` | Machine-readable error code in `SCREAMING_SNAKE_CASE` | | `error.message` | Human-readable description | -| `error.details` | Optional structured context (field errors, etc.) | +| `error.details` | Optional structured context. The one field-errors case is a body the schema rejects: `422 VALIDATION_ERROR` carries `details.errors`, a list of `{type, loc, msg, input}` entries, so a caller can see which field was wrong | + +A request body that fails validation never reaches a route, so it is worth saying +explicitly: it uses this same envelope. `error.code` there is `VALIDATION_ERROR` +and the per-field detail is in `error.details.errors`. diff --git a/docs/hn-submission.md b/docs/hn-submission.md index 83a942b..12dc7e9 100644 --- a/docs/hn-submission.md +++ b/docs/hn-submission.md @@ -24,7 +24,7 @@ AMP decouples memory from agent frameworks by defining: We've launched the v0.1.0 specification along with: - **Reference Server (Python/FastAPI):** ChromaDB by default with no infrastructure, or PostgreSQL + pgvector (`AMP_STORAGE_BACKEND=postgres`) if you already run one. It exposes MCP tools (`amp_remember`, `amp_recall`, ...), so you can connect it directly to Claude Desktop out of the box. - **Python SDK (`amp-client`)** with sync, async, and LangChain memory integrations, plus a **zero-dependency Node client**. Neither is published yet; both install from the repo. -- **A conformance suite** (38 vectors) you can run against your own implementation: `amp-conformance --base-url https://your-server.example.com`. It imports nothing from the reference server, and one category measures a server against its own advertised numbers. +- **A conformance suite** (39 vectors) you can run against your own implementation: `amp-conformance --base-url https://your-server.example.com`. It imports nothing from the reference server, and one category measures a server against its own advertised numbers. - **Multi-Agent Demo:** an example showing CustomerService and Billing agents sharing memory context, while a Marketing agent is blocked by cell-level access policies. We'd love to hear your feedback on the schema design and protocol specification: diff --git a/docs/release-notes-v0.1.0.md b/docs/release-notes-v0.1.0.md index eb44094..75cdc0e 100644 --- a/docs/release-notes-v0.1.0.md +++ b/docs/release-notes-v0.1.0.md @@ -53,7 +53,7 @@ released together. - Bundled MCP server (`amp-mcp`) so the server can be used as MCP tools. **Conformance suite (`conformance/`)** -- 38 vectors an implementation can run against its own server: +- 39 vectors an implementation can run against its own server: `amp-conformance --base-url https://your-server.example.com`. - It imports nothing from the reference server and carries the normative JSON Schema, so it judges an implementation rather than comparing it to this one. @@ -74,7 +74,7 @@ released together. **Examples** under `examples/`: a quickstart, an MCP `claude-desktop` config, and a multi-agent demo where one agent is blocked by cell-level access policy. -241 server tests, 31 Python SDK tests, 22 Node tests and 38 conformance vectors. +248 server tests, 34 Python SDK tests, 24 Node tests and 39 conformance vectors. CI runs lint, format and type gates across every package, Python 3.11 and 3.12, Node 18, 20 and 22, the storage contract suite against a real PostgreSQL service container, and the conformance suite against a live server. diff --git a/server/amp_server/errors.py b/server/amp_server/errors.py index 2915ee3..4bd58a9 100644 --- a/server/amp_server/errors.py +++ b/server/amp_server/errors.py @@ -75,6 +75,21 @@ def unauthenticated() -> AMPError: ) +def validation_error(field_errors: list[dict[str, Any]]) -> AMPError: + """A request body the schema rejects (spec §8.1). + + The per-field detail goes in `details.errors`, which is what `details` is for: + a caller needs to know which field was wrong, and the protocol needs one error + shape on every response. + """ + return AMPError( + 422, + "VALIDATION_ERROR", + "Request validation failed", + details={"errors": field_errors}, + ) + + def rate_limited(retry_after_seconds: int) -> AMPError: """Too many scoring edits on one cell (RFC §5: decay-score manipulation). diff --git a/server/amp_server/main.py b/server/amp_server/main.py index 83bc834..c031647 100644 --- a/server/amp_server/main.py +++ b/server/amp_server/main.py @@ -3,13 +3,17 @@ from __future__ import annotations import asyncio +import json import logging import os +import re from contextlib import asynccontextmanager from datetime import UTC, datetime from typing import Any from fastapi import APIRouter, Depends, FastAPI, Header, Query, Request +from fastapi.encoders import jsonable_encoder +from fastapi.exceptions import RequestValidationError from fastapi.responses import JSONResponse, Response from amp_server.access_control import check_read_access, check_write_access @@ -23,6 +27,7 @@ missing_agent_id, rate_limited, unauthenticated, + validation_error, ) from amp_server.lifecycle import LifecycleEngine from amp_server.limits import MAX_CELL_SIZE_BYTES @@ -227,7 +232,71 @@ async def lifespan(app: FastAPI): # App & router # --------------------------------------------------------------------------- -app = FastAPI( + +def document_the_error_envelope(schema: dict[str, Any]) -> dict[str, Any]: + """Point every 422 in a generated contract at the body this server returns. + + FastAPI documents its own validation error shape (`{"detail": [...]}`) on every + route that can reject input, which is not what comes back here - see + `_request_validation_handler`. The committed contract is meant to be the truth a + client generator builds from, so the 422 is rewritten to the protocol's envelope + in one place instead of being declared route by route. + """ + for operations in schema.get("paths", {}).values(): + for operation in operations.values(): + responses = operation.get("responses", {}) + if "422" in responses: + responses["422"] = { + "description": ( + "Request validation failed; error.code is VALIDATION_ERROR " + "and error.details.errors lists the fields" + ), + "content": { + "application/json": { + "schema": {"$ref": "#/components/schemas/ErrorResponse"} + } + }, + } + _drop_unreferenced_schemas(schema) + return schema + + +def _drop_unreferenced_schemas(schema: dict[str, Any]) -> None: + """Remove component schemas nothing points at any more. + + Rewriting the 422 leaves FastAPI's own `HTTPValidationError` and the + `ValidationError` it is built from defined but unused. Nothing breaks with them + there, and they are still worth removing: the contract is what a client + generator reads, and a component describing a body this server never sends is + how a wrong error type ends up in somebody's client. + + Refs are collected from the whole document, not from the components block - + most of them live in the paths - and the loop repeats because dropping one + schema can orphan another. + """ + schemas = schema.get("components", {}).get("schemas", {}) + while True: + referenced = { + match.group(1) + for match in re.finditer( + r"#/components/schemas/([A-Za-z0-9_.-]+)", json.dumps(schema) + ) + } + orphans = [name for name in schemas if name not in referenced] + if not orphans: + return + for name in orphans: + del schemas[name] + + +class AMPFastAPI(FastAPI): + """FastAPI, with a contract that matches the errors this server sends.""" + + def openapi(self) -> dict[str, Any]: + return document_the_error_envelope(super().openapi()) + + +app = AMPFastAPI( title="AMP Server", version=AMP_VERSION, description="Agent Memory Protocol reference server implementation", @@ -257,6 +326,26 @@ async def _amp_error_handler(request: Request, exc: AMPError) -> JSONResponse: ) +@app.exception_handler(RequestValidationError) +async def _request_validation_handler( + request: Request, exc: RequestValidationError +) -> JSONResponse: + """Answer a rejected request body with the protocol's own error shape. + + FastAPI raises this before a route runs, so it never reached the AMPError + handler above: a malformed body came back as `{"detail": [...]}` while every + other error used `{"error": {...}}`. Both SDKs read `error.code`, so a caller + that sent a bad field got "HTTP error 422" and no idea which field - the same + class of bug the single error shape was introduced to remove. The field errors + move into `details`, and `jsonable_encoder` keeps a rejected value that is not + JSON-serialisable (a bytes body, say) from turning this handler into a 500. + """ + return JSONResponse( + status_code=422, + content=validation_error(jsonable_encoder(exc.errors())).to_response(), + ) + + # The error envelope is part of the protocol, so it belongs in the contract # rather than being left out of it: without these, `openapi.json` would document # only the success path and a generated client would have nothing to type its diff --git a/server/tests/test_error_shape.py b/server/tests/test_error_shape.py index 0056afe..22bd2b6 100644 --- a/server/tests/test_error_shape.py +++ b/server/tests/test_error_shape.py @@ -75,6 +75,46 @@ async def test_invalid_transition_uses_the_error_envelope(app_client): _assert_error_envelope(resp.json(), "INVALID_TRANSITION") +@pytest.mark.asyncio +async def test_a_rejected_body_uses_the_error_envelope(app_client): + """The framework's own validation error is part of the wire contract too. + + A malformed body never reaches a route, so it never passed through AMPError - + it answered with FastAPI's `{"detail": [...]}` while everything else used + `{"error": {...}}`. Both SDKs read `error.code`, so a caller that sent a bad + field got a generic "HTTP error 422" and no idea which field was wrong. + """ + resp = await app_client.post( + "/amp/v1/memories", headers=_HEADERS, json={"type": "semantic"} + ) + + assert resp.status_code == 422 + _assert_error_envelope(resp.json(), "VALIDATION_ERROR") + assert "detail" not in resp.json() + + +@pytest.mark.asyncio +async def test_a_rejected_body_names_the_field_that_was_wrong(app_client): + """`details` carries the field errors, which is what `details` is for.""" + resp = await app_client.post( + "/amp/v1/memories", headers=_HEADERS, json={"type": "semantic"} + ) + + errors = resp.json()["error"]["details"]["errors"] + locations = {".".join(str(part) for part in error["loc"]) for error in errors} + assert "body.content" in locations + assert "body.identity" in locations + + +@pytest.mark.asyncio +async def test_an_out_of_range_page_size_uses_the_error_envelope(app_client): + """Every route that validates a parameter answers the same way.""" + resp = await app_client.get("/amp/v1/memories?limit=1000000", headers=_HEADERS) + + assert resp.status_code == 422 + _assert_error_envelope(resp.json(), "VALIDATION_ERROR") + + def test_every_error_helper_serialises_to_the_same_envelope(): """The helpers are the only source of protocol errors, so they must agree.""" cases = [ diff --git a/spec/v0.1.0/openapi.json b/spec/v0.1.0/openapi.json index b304fe9..8c8135c 100644 --- a/spec/v0.1.0/openapi.json +++ b/spec/v0.1.0/openapi.json @@ -45,19 +45,6 @@ "title": "ExtractionMethod", "type": "string" }, - "HTTPValidationError": { - "properties": { - "detail": { - "items": { - "$ref": "#/components/schemas/ValidationError" - }, - "title": "Detail", - "type": "array" - } - }, - "title": "HTTPValidationError", - "type": "object" - }, "LifecycleStatus": { "enum": [ "active", @@ -469,46 +456,6 @@ ], "title": "SourceType", "type": "string" - }, - "ValidationError": { - "properties": { - "ctx": { - "title": "Context", - "type": "object" - }, - "input": { - "title": "Input" - }, - "loc": { - "items": { - "anyOf": [ - { - "type": "string" - }, - { - "type": "integer" - } - ] - }, - "title": "Location", - "type": "array" - }, - "msg": { - "title": "Message", - "type": "string" - }, - "type": { - "title": "Error Type", - "type": "string" - } - }, - "required": [ - "loc", - "msg", - "type" - ], - "title": "ValidationError", - "type": "object" } } }, @@ -590,11 +537,11 @@ "content": { "application/json": { "schema": { - "$ref": "#/components/schemas/HTTPValidationError" + "$ref": "#/components/schemas/ErrorResponse" } } }, - "description": "Validation Error" + "description": "Request validation failed; error.code is VALIDATION_ERROR and error.details.errors lists the fields" } }, "summary": "Run Lifecycle Now" @@ -736,11 +683,11 @@ "content": { "application/json": { "schema": { - "$ref": "#/components/schemas/HTTPValidationError" + "$ref": "#/components/schemas/ErrorResponse" } } }, - "description": "Validation Error" + "description": "Request validation failed; error.code is VALIDATION_ERROR and error.details.errors lists the fields" } }, "summary": "Query Memories" @@ -828,11 +775,11 @@ "content": { "application/json": { "schema": { - "$ref": "#/components/schemas/HTTPValidationError" + "$ref": "#/components/schemas/ErrorResponse" } } }, - "description": "Validation Error" + "description": "Request validation failed; error.code is VALIDATION_ERROR and error.details.errors lists the fields" } }, "summary": "Create Memory" @@ -974,11 +921,11 @@ "content": { "application/json": { "schema": { - "$ref": "#/components/schemas/HTTPValidationError" + "$ref": "#/components/schemas/ErrorResponse" } } }, - "description": "Validation Error" + "description": "Request validation failed; error.code is VALIDATION_ERROR and error.details.errors lists the fields" } }, "summary": "Query Memories" @@ -1058,11 +1005,11 @@ "content": { "application/json": { "schema": { - "$ref": "#/components/schemas/HTTPValidationError" + "$ref": "#/components/schemas/ErrorResponse" } } }, - "description": "Validation Error" + "description": "Request validation failed; error.code is VALIDATION_ERROR and error.details.errors lists the fields" } }, "summary": "Search Memories" @@ -1152,11 +1099,11 @@ "content": { "application/json": { "schema": { - "$ref": "#/components/schemas/HTTPValidationError" + "$ref": "#/components/schemas/ErrorResponse" } } }, - "description": "Validation Error" + "description": "Request validation failed; error.code is VALIDATION_ERROR and error.details.errors lists the fields" } }, "summary": "Delete Memory" @@ -1243,11 +1190,11 @@ "content": { "application/json": { "schema": { - "$ref": "#/components/schemas/HTTPValidationError" + "$ref": "#/components/schemas/ErrorResponse" } } }, - "description": "Validation Error" + "description": "Request validation failed; error.code is VALIDATION_ERROR and error.details.errors lists the fields" } }, "summary": "Get Memory" @@ -1364,11 +1311,11 @@ "content": { "application/json": { "schema": { - "$ref": "#/components/schemas/HTTPValidationError" + "$ref": "#/components/schemas/ErrorResponse" } } }, - "description": "Validation Error" + "description": "Request validation failed; error.code is VALIDATION_ERROR and error.details.errors lists the fields" }, "429": { "content": {