diff --git a/README.md b/README.md index 20a0013..17516f1 100644 --- a/README.md +++ b/README.md @@ -54,6 +54,29 @@ Sign in at [app.xtrace.ai](https://app.xtrace.ai) and grab two values from **Set Both are required on every request. See the [full docs](https://docs.xtrace.ai/guides/authentication) for storage best practices. +### Auth header form + +By default the client sends the API key as `Authorization: Bearer `. If +your deployment authenticates with an `x-api-key` header instead, opt in with +`authMode`: + +```ts +const client = new MemoryClient({ + apiKey: process.env.XTRACE_API_KEY!, + orgId: process.env.XTRACE_ORG_ID!, + authMode: "x-api-key", // sends `x-api-key: `; omits `Authorization` +}); +``` + +`X-Org-Id` is sent in both modes. `authMode` defaults to `"bearer"`, so existing +code is unaffected. + +> [!NOTE] +> The scheme is a wire-format choice only — it carries **no rate-limit +> advantage**. Both `bearer` and `x-api-key` are throttled against the same +> `(org_id, key_hash)` rate bucket, so switching does not buy you extra quota. +> `bearer` is the default; pick `x-api-key` only if your deployment requires it. + ## TypeScript SDK ```ts @@ -88,6 +111,16 @@ if (sync.status === "succeeded") { console.log(sync.result?.memories_created); } +// When an ingest supersedes an existing fact, the result maps old id → new id +// (`result.memories_superseded_by`). Resolve a superseded fact to its +// replacement Memory without reading that map yourself: +const oldId = "mem_old"; // an id you held before this ingest +const replacement = await client.memories.resolveSuperseded(done.result!, oldId); +// → the new Memory, or null if `oldId` wasn't superseded in this ingest. + +// Or resolve everything this ingest superseded in one call (Map): +const replacements = await client.memories.resolveAllSuperseded(done.result!); + // Search — scope by what you pass (user_id / group_ids / agent_id / app_id all AND-narrow) const results = await client.memories.search({ query: "what does the user like to eat?", @@ -100,18 +133,111 @@ const { prompt } = await client.memories.recall({ pools: [{ user_id: "alice" }, { group_ids: ["grp_tokyo2026"] }], }); +// Recall + heavy artifact bodies in one round-trip — `include` is forwarded to +// every pool, so `details.full_content` is populated on the rows it returns. +const { memories } = await client.memories.recall({ + query: "the trip itinerary", + pools: [{ user_id: "alice" }, { group_ids: ["grp_tokyo2026"] }], + include: ["full_content"], +}); + // List with auto-pagination for await (const memory of client.memories.list({ user_id: "alice" })) { console.log(memory.text); } +// Search with auto-pagination — the search twin of list(); threads the cursor +// for you across pages until the server says has_more: false +for await (const memory of client.memories.searchAll({ query: "what does the user like to eat?", user_id: "alice" })) { + console.log(memory.text); +} + // Get one const memory = await client.memories.get(results.data[0]!.id); +// Edit group membership — the only in-place mutation on a memory. +// Pass add_group_ids and/or remove_group_ids; at least one is required. +// Resolves to the full updated Memory (group_ids reflects the edit). +const reTagged = await client.memories.patch(memory.id, { + add_group_ids: ["grp_tokyo2026"], + remove_group_ids: ["grp_archive"], +}); +console.log(reTagged.group_ids); + // Delete (hard — the point is removed; get/list/search no longer return it, a second delete 404s) await client.memories.delete(memory.id); ``` +The patch body is typed as `MemoryPatchRequest`. An empty patch (`{}`, or both +arrays empty) throws synchronously — the server would reject it with +`422 empty_patch`, so the SDK saves the round-trip: + +```ts +import type { MemoryPatchRequest } from "@xtraceai/memory"; + +const changes: MemoryPatchRequest = { add_group_ids: ["grp_tokyo2026"] }; +await client.memories.patch("mem_123", changes); +``` + +## Filtering + +Scope axes (`user_id` / `agent_id` / `app_id` / `group_ids`) are top-level +fields on `search`. For anything richer — ranges, set membership, negation, +existence, or filtering on your own indexed payload keys — pass a filter to +`search({ filters })`. Use the typed `f` builder instead of hand-writing the +wire JSON: it's discoverable, and it makes a silently-wrong query impossible. + +```ts +import { f } from "@xtraceai/memory"; + +// (agent_id == "bot") AND (0.5 <= score < 0.9) AND (plan in ["a", "b"]) +const results = await client.memories.search({ + query: "recent activity", + filters: f.all( + f.eq("agent_id", "bot"), + f.field("score", { $gte: 0.5, $lt: 0.9 }), // a range keeps BOTH operators + f.in("plan", ["a", "b"]), + ), +}); +``` + +`f.field(name, ops)` is the only way to put multiple operators on one field, so +a two-sided range can't silently collapse to one bound. `f.all(...)` merges +clauses on **distinct** fields and **throws** on a duplicate field — combine a +field's operators in a single `f.field` call, or `f.and(...)` to AND two +conditions on the same field explicitly. + +```ts +// single-operator shorthands +f.eq("status", "active"); // { status: { $eq: "active" } } +f.ne("status", "archived"); // { status: { $ne: "archived" } } +f.in("plan", ["pro", "team"]); // { plan: { $in: ["pro", "team"] } } +f.nin("plan", ["free"]); // { plan: { $nin: ["free"] } } +f.exists("conv_id"); // { conv_id: { $exists: true } } +f.exists("conv_id", false); // { conv_id: { $exists: false } } +f.between("score", 0.5, 0.9); // { score: { $between: [0.5, 0.9] } } +f.isNull("agent_id"); // { agent_id: null } ← null = unset + +// boolean composition +f.and(f.eq("a", 1), f.eq("b", 2)); // { AND: [...] } +f.or(f.eq("a", 1), f.eq("b", 2)); // { OR: [...] } +f.not(f.eq("a", 1)); // { NOT: { a: { $eq: 1 } } } +``` + +The builder is **key-agnostic** — it filters any indexed payload key, including +your own metadata keys, exactly the way it filters an entity axis: + +```ts +// `tier` is a customer metadata key; it filters identically to a built-in axis. +filters: f.all(f.eq("agent_id", "bot"), f.eq("tier", "gold")); +``` + +The builder's output is a plain object assignable to `SearchRequest.filters`, so +the raw `Filter` (`Record`) escape hatch still works if you'd +rather hand-write the JSON. (Lifting scope axes such as `user_id` *out of* +`filters` is deprecated — set those as top-level fields — but the operator DSL +over payload keys is fully supported.) + ## Vercel AI SDK integration A separate subpath, `@xtraceai/memory/ai-sdk`, ships two ways to use the SDK with the [Vercel AI SDK](https://ai-sdk.dev). Peer dependencies (`ai`, `zod`) are optional — they're only required if you import from this subpath. @@ -177,10 +303,90 @@ try { } ``` +`error.code` is populated regardless of which envelope the server emits — the +legacy `{ error: { code, message } }`, the spec `{ detail: { code, message } }`, +or FastAPI's plain `{ detail: "..." }` string (which sets `error.message`). A +`422` validation response (`{ detail: [...] }`) surfaces as `Unprocessable` with +`error.code === "validation_error"` and the raw per-field array under +`error.details.validation_errors`: + +```ts +import { Unprocessable } from "@xtraceai/memory"; + +try { + await client.memories.search({ query: "" }); +} catch (err) { + if (err instanceof Unprocessable) { + const fields = err.details?.validation_errors; // raw FastAPI 422 array + } +} +``` + +### Rate limits + +When the server sends `x-ratelimit-limit` / `x-ratelimit-remaining` / +`x-ratelimit-reset` headers, the SDK parses them into a `RateLimitSnapshot`. On +any thrown error, read it from `err.rateLimit` so you can back off proactively: + +```ts +import { RateLimited } from "@xtraceai/memory"; + +try { + await client.memories.search({ query: "recent context" }); +} catch (err) { + if (err instanceof RateLimited && err.rateLimit?.remaining === 0) { + // err.rateLimit.reset is an EPOCH-SECONDS timestamp, not a duration: + // const waitMs = (err.rateLimit.reset * 1000) - Date.now(); + // (err.retryAfter carries the server's Retry-After for a 429) + } +} +``` + +> [!IMPORTANT] +> `reset` is an **epoch-seconds timestamp** (the wall-clock time the window +> rolls over) — **not** a number of seconds to wait. Compute the delay as +> `reset * 1000 - Date.now()`; do not pass `reset` straight to a `setTimeout`. + +Absent headers leave every `RateLimitSnapshot` field `undefined`. Rate-limit +state is exposed per-response — there is no client-level "last seen" global, +because the SDK fans out concurrent requests and a shared snapshot would be +racy. + +Note (v0.3.0): the snapshot is parsed for **every** response, but on the +**success** path it is currently only available on the internal +`HttpClient.request()` return — the public methods (`memories.*`, `groups.*`) +return just the parsed body, so there is no public success-path read yet. A +public success-path rate-limit surface is deferred until there is demand for +it; today, `err.rateLimit` on a thrown error is the supported way to observe +the bucket state. + ## Documentation Full documentation at [docs.xtrace.ai](https://docs.xtrace.ai). +### Type surface & generated reference (maintainers) + +The hand-authored `src/types.ts` is the canonical public type surface +(see `docs/adr/ADR-002`). `src/generated/types.ts` is a spec-derived +**reference** — produced by `npm run gen:types` +(`openapi-typescript spec/memory.json`) and imported by nothing in `src/`. +Do not hand-edit the generated file; regenerate it from the spec instead. + +`npm run check:types-sync` (`gen:types` + `git diff --exit-code` on the +generated file) proves the committed reference is exactly what the current +`spec/memory.json` produces, catching reference drift. It is chained into +`prepublishOnly`, so a publish fails if the generated reference is stale. +After any `spec/memory.json` change, run `npm run gen:types` and commit the +regenerated reference. + +**openapi-typescript version pin:** the reference is generated with +**openapi-typescript `^7.4.0`** (the version installed at generation time was +`7.13.0`). The generator's output format can shift between minor versions, so a +bump can make `check:types-sync` fail even with no spec change. If that happens, +it is not spec drift — run `npm run gen:types`, review the diff, and commit the +regenerated reference. Keep the devDependency pin tight to avoid spurious +failures. + # License MIT — see [LICENSE](LICENSE). diff --git a/docs/adr/ADR-001-sdk-spec-reconciliation-policy.md b/docs/adr/ADR-001-sdk-spec-reconciliation-policy.md new file mode 100644 index 0000000..57640d8 --- /dev/null +++ b/docs/adr/ADR-001-sdk-spec-reconciliation-policy.md @@ -0,0 +1,403 @@ +# ADR-001: Treat the shipping SDK as the canonical 1.0 contract and correct the spec to match + +> **Status**: Accepted | **Date**: 2026-06-12 | **Deciders**: M2 reconciliation (adversarial review passed, rev 2) + +## Context + +`@xtraceai/memory` is approaching a 1.0 release. The 1.0 cut is a contract-locking +event: once published, consumers are entitled to rely on the auth scheme, error +codes, endpoint set, and wire shapes behaving as documented, and breaking any of +them after 1.0 carries a major-version cost. Today the SDK and its companion +OpenAPI document, `spec/memory.json`, disagree in four confirmed places. Locking +1.0 while those disagreements stand would bake silent drift into the contract. + +The disagreements are not symmetric guesses — they have a consistent shape. The +SDK targets a **newer, deployed** API than `spec/memory.json` describes; the spec +**lags** the implementation. Evidence: the SDK already ships `/v1/groups`, +`group_ids`, `recall`, and per-pool `mode` (added through PRs #6–#8) that the +checked-in spec only partially reflects, and one endpoint the spec still documents +was **removed server-side** and deleted from the SDK because the live API rejected +it. So reconciliation is not "make the SDK obey the spec" — in most cases it is the +reverse, plus hardening the SDK where the spec describes a shape we cannot currently +disprove. + +The four divergences (all cross-checked against `src/` and `spec/memory.json`): + +1. **Error envelope.** `src/http.ts:117-127` (`toError`) reads `parsed.error` + (`ApiErrorBody`, `src/types.ts:339-347`), and `src/errors.ts:43-66` + (`errorForStatus`) defaults `code` to `'unknown_error'` when that field is + absent. The spec (`spec/memory.json` `info.description` "Error envelope"; + `ErrorEnvelope`/`ErrorDetail` at L795-833) documents a *different* shape: + `{ "detail": { "code", "message" } }`, with 422 validation failures using the + FastAPI array form `{ "detail": [ { "loc", "msg", "type" } ] }` + (`HTTPValidationError`/`ValidationError`, L835-846, L1554-1585). If the live API + emits the documented `detail` shape, `MemoryError.code` silently degrades to + `'unknown_error'` and `message` to the generic `"Memory API request failed + with status N"` — the HTTP-status-derived subclass is still correct, but the + stable code consumers are told to switch on is lost. + +2. **Auth header.** `src/http.ts:44` sends `Authorization: Bearer `. The spec's + security schemes (`spec/memory.json` L1820-1837; `info.description` + "Authentication") document only `Authorization: Token ` (`BearerToken`, + `bearerFormat: "Token"`) **or** `x-api-key: `, always paired with + `X-Org-Id`. The `Bearer` form matches **neither** documented scheme verbatim — + yet it has shipped since the initial commit, through v0.2.1. A published SDK that + authenticated with a rejected scheme would 401 on every call; it does not. The + strongest evidence that the maintainers probe the live API (not just publish) is + commit `a7da7a9`'s own message — *"the old calls already 4xx against the live + API"* — i.e. they observe real status codes from the deployed server. That same + commit proves the deployed API actively diverges from this spec (it dropped + `metadata`, added `group_ids`; see divergence 5 below). + +3. **Rate-limit headers.** `src/http.ts:77,119` reads only `x-request-id` and + `retry-after`. The spec (`info.description` "Response headers"; 429 descriptions) + documents `RateLimit-Limit`, `RateLimit-Remaining`, and `RateLimit-Reset` on + **every** response — the bucket state for the `(org_id, key_hash)` pair. A + caller has no way to throttle proactively today; it can only react to a 429. + +4. **Update endpoint.** The spec defines `PATCH /v1/memories/{memory_id}` with an + `UpdateRequest` body (`spec/memory.json` L514, L602, L1522-1551). The SDK has no + `update()` method, and `src/memories.ts:230` asserts the API has none. Git + history is decisive: commit `a7da7a9` / PR #6 **removed** `memories.update()` + and `UpdateRequest` with the message *"PATCH /v1/memories/{id} was removed + server-side (returns 405). Corrections flow through ingest."* The spec is simply + stale here. + +5. **`metadata` field (surfaced during review).** The same commit `a7da7a9` + **dropped** `metadata` from `Memory` and `IngestRequest` ("dropped + server-side"), so the SDK no longer models it. But `spec/memory.json`'s + `info.description` still documents `metadata` on both the Memory object and the + ingest contract, **and** the spec's filter DSL section advertises filtering on + "customer metadata keys." This is a fifth divergence in the same class as the + four above, and it has a second-order effect: the additive typed-filter-DSL + feature (B1, a separate project) targets metadata-key filtering against a field + the SDK deliberately removed. It is in scope for *this* policy only as a spec + correction; its impact on the filter DSL design is flagged for that feature's + design doc. + +The forces in tension: + +- **Contract fidelity vs. shipping reality.** The spec is the *documented* contract; + the SDK is the *executed* contract. They cannot both be the 1.0 truth where they + disagree. One must yield. +- **Regression risk vs. spec conformance.** Changing shipping defaults (e.g. + `Bearer → Token`) to match the spec would, if the spec is the stale side, break + every existing consumer. +- **Verification gap.** No live-API credentials are available in this environment + (`XTRACE_API_KEY` / `XTRACE_ORG_ID` unset; `npm run smoke` cannot run). So for the + error-envelope and auth divergences we can reason from shipping evidence and git + history, but cannot *probe* the live wire shape right now. The decision framework + has to be robust to that residual uncertainty rather than gated on resolving it. + +This ADR governs the `spec-contract-alignment` project (roadmap `m2/s5`). A +companion ADR (ADR-002) covers the type-surface source-of-truth question. The +additive search capabilities (typed filter DSL, first-class `include[]`, +`searchAll()`) are feature work, not contract decisions, and are out of scope here. + +## Decision + +**We adopt a single reconciliation policy for the 1.0 cut: where the SDK and +`spec/memory.json` disagree, the proven, shipping SDK behaviour is the canonical +contract, and `spec/memory.json` is corrected to match it. Where the spec describes +a wire shape we cannot currently disprove and tolerating it is cheap, we +additionally harden the SDK to accept *both* shapes rather than guess which the live +API emits. We do not change any shipping default auth, error, or endpoint behaviour +— only additive hardening and spec corrections.** + +Concretely, the policy decides the direction and the risk posture; the four +divergences are *applications* of it (see Consequences → Application), not four +independent decisions. The through-line: + +- **Spec yields to the SDK** on settled facts (auth scheme that demonstrably works; + an endpoint the server already rejects). +- **SDK hardens toward the spec** on unsettled wire shapes that are cheap to tolerate + and carry no regression risk (parse the documented error envelope *in addition to* + the current one; read the documented rate-limit headers *in addition to* + `Retry-After`). +- **No shipping default is altered.** Every change is either purely additive in the + SDK or confined to `spec/memory.json`. + +**Why auth (A2) is hardened differently from errors/rate-limit.** Harden-both is +safe for A1/A3 because *receiving* and tolerating an extra response shape can never +make a request fail. Auth is *request*-side: hardening it means *sending* extra +credentials, and a server that validates "exactly one credential present" could +reject a request carrying both `Authorization: Bearer` **and** `x-api-key`. So the +no-regret property does not transfer automatically. We therefore do **not** send dual +auth headers by default. We do, however, make the spec's primary documented scheme +(`x-api-key`) reachable as an *opt-in* the caller can select — the auth analogue of +harden-both, minus the dual-send risk — so a consumer in an environment where the +gateway prefers `x-api-key` is not stranded. The default remains `Bearer` (proven); +the opt-in is additive and off by default. + +- **Spec edits are additive/annotative, not destructive.** Because we cannot probe the + server we don't control, a spec "correction" must not *assert* something new we + haven't verified. We *add* the SDK's observed form alongside the documented one + (auth) and *annotate* removed operations with the evidence (the 405 observation), + rather than silently rewriting or deleting — preserving history and avoiding + replacing one un-probed claim with another. + +## Rationale + +The policy is the natural consequence of three facts established in Context: the +spec is the *lagging* side, the SDK is *published and working*, and we *cannot probe* +the live API right now. + +### Key Factors + +1. **A published SDK is empirical evidence; a checked-in spec is an assertion.** + The `Bearer` header and the absence of `update()` are not opinions — they are the + behaviour of software that demonstrably transacts with the live API (v0.2.1 ships; + PR #6 removed `update()` *because* the server returned 405). When documented + intent and observed behaviour collide, observed behaviour is the stronger witness + to what 1.0 consumers will actually experience. Correcting the spec to match it + removes drift at the source. + +2. **Asymmetric cost of being wrong.** Consider auth. If we keep `Bearer` and the + live API also accepts it (overwhelmingly likely — it ships), nothing breaks. If + we instead "conform to the spec" by switching to `Token` and the spec is the stale + side, we break **every** existing consumer on the 1.0 upgrade. The downside of + trusting the shipping default is bounded (a spec edit); the downside of trusting + the stale spec is unbounded (a field-wide outage). The policy always chooses the + bounded-risk side. + +3. **Harden-both converts an unknown into a no-regret.** For the error envelope we + genuinely do not know which shape the live API emits — *both* current behaviours + ("right subclass, degraded code") are consistent with shipping. Rather than bet on + one, we parse both. If the live API emits `detail`, we start surfacing real codes + (a strict improvement); if it emits `error`, behaviour is unchanged. The + uncertainty stops being a blocker because no outcome is worse than today. The same + logic makes the rate-limit headers a pure capability add: present → exposed, + absent → `undefined`, never a regression. + +4. **Silent drift violates the story's own success criteria.** `m2/s5` requires that + *every* confirmed divergence be triaged to a recorded decision and that no silent + spec drift remain at the 1.0 cut. "Document the gaps and move on" fails that bar by + construction. A policy that forces each divergence to either change code or change + the spec is the only one that satisfies it. + +## Consequences + +### Positive + +- **No regression risk.** Because no shipping default changes, existing consumers see + identical auth/error/endpoint behaviour on upgrade. Every SDK change is additive. +- **The 1.0 contract becomes self-consistent.** After the spec corrections, a reader + of `spec/memory.json` and a user of the SDK observe the same auth form, the same + endpoint set, and compatible error handling. The story's "no silent drift" bar is met. +- **Error ergonomics strictly improve or stay equal.** Hardening `toError` to read + `detail.code`/`detail.message` (and the 422 array) means stable codes surface + wherever the live API uses the documented shape, with zero downside if it doesn't. +- **Proactive throttling becomes possible.** Surfacing `RateLimit-*` lets callers + pace themselves instead of only reacting to 429s. +- **The verification gap stops blocking the milestone.** The harden-both posture is + explicitly designed to be safe under "we can't probe the live API," so M2 can + proceed without credentials. + +### Negative + +- **`spec/memory.json` diverges from upstream's checked-in copy.** We are editing the + spec on our fork. To bound the epistemic risk (we can't probe the server we don't + control), edits are **additive/annotative**: we *add* the observed `Bearer` form + alongside the documented schemes and *annotate* removed/dropped items (PATCH, + `metadata`) with the evidence — we do not delete or rewrite to assert un-probed + claims. If upstream later "corrects" in the opposite direction (e.g. revives PATCH), + the annotations make the conflict legible rather than silent. *Accepted:* shipping + 1.0 with a spec we know is wrong is worse, and annotations are individually reversible. +- **Harden-both carries a sliver of permanent complexity.** `toError` will understand + two envelope shapes forever, even after the live shape is known. *Accepted:* the + branch is a few lines, fully unit-testable against fixtures of both shapes, and the + cost of removing it later (once probed) is trivial. +- **Residual empirical uncertainty is recorded, not resolved.** We are asserting + `Bearer` works and the `detail` envelope *might* be live without a fresh probe. + *Accepted:* the asymmetric-cost and harden-both reasoning make every residual + unknown a no-regret; a follow-up smoke run when credentials exist (see Validation) + can promote any inference to a fact and prune the tolerated branch. + +### Neutral + +- **`x-api-key` support is deferred, not denied.** The spec's `x-api-key` scheme is a + legitimate alternative; adding it is additive and safe but is sequenced as optional + follow-up scope rather than bundled into the default-auth decision. +- **The `delete()` docstring already encodes the A4 outcome** ("corrections flow + through ingest"), so that part of the contract is consistent in the SDK already; only + the spec needs the matching correction. + +### Application — disposition of the four divergences + +| # | Divergence | Direction | Disposition | Breaking? | +|---|-----------|-----------|-------------|-----------| +| A1 | Error envelope (`{error:…}` vs spec `{detail:…}` / 422 `detail[]`) | SDK hardens toward spec | Parse **both** envelope shapes in `toError`/`errorForStatus`; extract `code`/`message` from whichever is present; surface the 422 `detail[]` array in `details` (the array→`details` mapping shape is a design-doc decision, since `MemoryError.details` is typed `Record`); status-derived subclass unchanged. | No (additive) | +| A2 | Auth header (`Bearer` vs spec `Token`/`x-api-key`) | Spec yields to SDK; SDK gains opt-in | Keep `Bearer` as the canonical **default** (no change to the default path). **Add** an opt-in `x-api-key` auth mode (additive, off by default — the auth analogue of harden-both without dual-send risk). Spec edit is **additive**: document the `Bearer` form *alongside* the existing `Token`/`x-api-key` schemes, not in place of them. | No (additive) | +| A3 | Rate-limit headers (`Retry-After` only vs spec `RateLimit-*`) | SDK adds capability | Additively parse and surface `RateLimit-Limit/Remaining/Reset` alongside the existing `Retry-After` handling. | No (additive) | +| A4 | Update endpoint (spec `PATCH` vs SDK none; server 405) | Spec yields to SDK | **Won't-do** in the SDK (do not re-add `update()`). **Annotate** the `PATCH /v1/memories/{memory_id}` operation + `UpdateRequest` schema in `spec/memory.json` as removed server-side (observed 405, PR #6), documenting that corrections flow through ingest — preserve the entry with a deprecation/removal note rather than silently deleting. | No | +| A5 | `metadata` field (SDK dropped it; spec still documents it + filter DSL) | Spec yields to SDK | **Annotate** the `metadata` references in `spec/memory.json`'s `info.description` (Memory object, ingest contract, filter DSL) as dropped server-side per `a7da7a9`. Hand off the filter-DSL impact to the B1 design doc. | No (spec-only) | + +## Alternatives Considered + +### Alternative 1: Conform the SDK to the spec + +**Description**: Treat `spec/memory.json` as the source of truth. Change the auth +header `Bearer → Token`, re-add `memories.update()` + `UpdateRequest` for the +documented `PATCH`, and rewrite `toError` to read only the `detail` shape. + +**Pros**: +- Produces a spec-faithful SDK with no fork-local spec edits. +- Philosophically clean: "the spec is the contract, the code obeys it." + +**Cons**: +- Inverts the established direction of drift — the spec is the *lagging* side here, + so conforming to it means regressing toward stale truth. +- Re-adds an endpoint the server **already rejects with 405** (PR #6's exact reason + for removal). The SDK would ship a method guaranteed to fail. +- Changing `Bearer → Token` is a live-fire change to a working auth path with no way + to verify it (no credentials). If `Bearer` is what the live API honours, this + breaks every consumer. + +**Why not chosen**: It optimizes for documentary tidiness at the cost of breaking a +shipping, working contract and reviving a known-dead endpoint. The spec being wrong +is not a reason to make the SDK wrong to match. + +### Alternative 2: Document the divergences as known gaps and ship 1.0 anyway + +**Description**: Leave both sides as-is. Add a "known divergences" note to the README +or spec and lock 1.0 with the contradictions intact. + +**Pros**: +- Zero code/spec churn now; fastest path to a 1.0 tag. +- Honest in the narrow sense that the gaps are written down somewhere. + +**Cons**: +- Directly violates `m2/s5`'s success criteria ("no silent spec drift remains at the + 1.0 cut"; every divergence "triaged to a recorded decision"). +- Ships a contract that contradicts itself: consumers reading the spec get + `detail.code` guidance the SDK may not honour, and a `PATCH` endpoint that 405s. +- Locks the drift behind a major-version wall — the costs compound rather than + resolve. + +**Why not chosen**: A "known gap" at a contract-locking event is just deferred drift +with a higher future removal cost. The milestone exists precisely to avoid this. + +### Alternative 3: Block M2 on obtaining live-API credentials and probe everything + +**Description**: Pause until `XTRACE_API_KEY`/`XTRACE_ORG_ID` are available, run +`npm run smoke` against the live API, empirically settle every wire shape, then +reconcile from facts. + +**Pros**: +- Replaces every inference with a measurement — the strongest possible grounding. +- Lets us prune the harden-both branch down to the single real shape. + +**Cons**: +- Credentials are not available in this environment; the milestone would stall + indefinitely on an external dependency. +- Most of the work doesn't need probing: A4 is already settled by git history, A2 by + shipping evidence, and A3 is additive regardless. Only the error envelope is + genuinely uncertain — and harden-both already neutralizes that. + +**Why not chosen**: It makes the whole milestone hostage to one unavailable input to +resolve a question the harden-both posture already de-risks. Probing is the right +*follow-up* (see Validation), not a *precondition*. + +### Alternative 4: Send dual auth headers (`Bearer` + `x-api-key`) by default + +**Description**: Apply harden-both literally to auth — send *both* the proven +`Authorization: Bearer` and the spec's primary `x-api-key` on every request, so the +server accepts whichever it honours. + +**Pros**: +- The most aggressive de-risk: a request authenticates under either scheme without the + caller choosing, making auth a true no-regret the way A1/A3 are. +- Zero config burden on consumers. + +**Cons**: +- Harden-both is only safe when *tolerating* an extra input; here we'd be *sending* + extra credentials. A server that validates "exactly one credential present" (a + common gateway posture) could reject a dual-credential request — turning a working + auth path into a 4xx for everyone. We cannot probe to rule this out. +- It changes the default request shape, violating the policy's "no shipping default is + altered" guarantee. + +**Why not chosen**: It inverts the policy's risk posture on the request side — the one +place we can't afford a wrong guess. The chosen middle path keeps the proven `Bearer` +default and exposes `x-api-key` as an **opt-in** (A2), capturing the de-risk for +callers who need it without imposing dual-send risk on everyone. + +## Implementation Notes + +This ADR sets direction; the design doc and cards carry the detail. In scope here: + +- **A1**: Extend `toError` (`src/http.ts`) and `errorForStatus` (`src/errors.ts`) to + recognize `{detail:{code,message}}` and the 422 `{detail:[{loc,msg,type}]}` array + in addition to the current `{error:{type,code,message}}`. Extraction precedence and + the `details` mapping for the 422 array are design-doc decisions. Unit tests must + cover all three fixtures; this is the canonical TDD target. +- **A2**: Default path unchanged. (1) Edit `spec/memory.json` *additively* — security + schemes (L1820-1837) and the `info.description` "Authentication" block — to document + the `Bearer` form *alongside* the existing `Token`/`x-api-key` schemes, not in place + of them. (2) Add an opt-in `x-api-key` auth mode in the HTTP layer (e.g. a config + flag selecting the header form), additive and off by default — this is a small but + real code change and gets its own card, separate from the default-auth spec edit. +- **A3**: Parse `RateLimit-Limit/Remaining/Reset` in the HTTP layer and surface them + additively. The *exposure surface* (on `RateLimited`/`MemoryError`, and/or a + client-level "last seen" snapshot for successful responses) is a design-doc + decision, since SDK methods currently return bodies, not header-bearing envelopes. +- **A4**: No SDK code change. Edit `spec/memory.json` to *annotate* the + `PATCH /v1/memories/{memory_id}` operation (L514/L602) and the `UpdateRequest` + schema (L1522-1551) as removed server-side (observed 405, PR #6), recording that + corrections flow through ingest — preserve the entry with a removal note rather than + deleting it outright. +- **A5**: Spec-only. Annotate the `metadata` references in `info.description` (Memory + object, ingest contract, filter DSL operator table) as dropped server-side per + `a7da7a9`. The B1 (typed-filter-DSL) design doc owns the question of whether/how + metadata filtering is still meaningful. + +Because every change is additive or spec-only, this work is a **minor** version bump +under the pre-1.0 policy in `RELEASING.md` (no breaking change). Spec-only edits skip +the code pre-commit gate; the A1/A3 code changes must pass `typecheck → test → build`. + +## Validation + +We will know this was the right call if: + +- **No regression signal.** After release, existing consumers report no auth, error, + or endpoint behaviour change (the additive-only guarantee holds). Concretely: the + existing `groups`/`recall`/`ai-sdk` test suites pass unchanged, and the new A1/A3 + tests pass against both-shape fixtures. +- **Self-consistency check passes.** A reader can no longer find a contradiction + between `spec/memory.json` and the SDK on auth scheme, the endpoint set, or error + handling. The four table rows are each either code-additive or spec-corrected; none + remains "documented but unhandled." +- **The probe, when it happens, confirms the inference.** *Revisit trigger:* the first + time `npm run smoke` runs with real credentials, capture the actual auth acceptance, + one real error body, and the response headers. If the live error shape is settled, + prune the unused branch in `toError` and record the fact in this ADR's revision + history. If `Bearer` is somehow *not* accepted, this decision must be reopened + immediately — but the shipping evidence makes that outcome very unlikely. + +## Related Decisions + +- **ADR-002** (companion): type-surface source of truth — hand-authored + `src/types.ts` as canonical vs the generated `src/generated/types.ts`, and the + disposition of declared-but-unwired forward fields. + +## References + +- `spec/memory.json` — `info.description` (auth, error envelope, response headers, + filter DSL); `ErrorDetail`/`ErrorEnvelope` (L795-833); `HTTPValidationError`/ + `ValidationError` (L835-846, L1554-1585); security schemes (L1820-1837); + `PATCH /v1/memories/{memory_id}` (L514, L602); `UpdateRequest` (L1522-1551). +- `src/http.ts` — auth header (L44), `toError` (L117-127), header reads (L77, L119). +- `src/errors.ts` — `errorForStatus` and `'unknown_error'` default (L43-66). +- `src/memories.ts` — `delete()` "no update endpoint" docstring (L230). +- `git show a7da7a9` / PR #6 — removal of `memories.update()` + `UpdateRequest` + (server 405; "corrections flow through ingest"). +- `RELEASING.md` — pre-1.0 versioning (breaking changes bump the minor, L51-52). + +--- + +## Revision History + +| Date | Status | Notes | +|------|--------|-------| +| 2026-06-12 | Proposed | Initial proposal — reconciliation policy for the four spec-contract-alignment divergences (m2/s5). | +| 2026-06-12 | Proposed (rev 2) | Adversarial-review fixes: auth opt-in (`x-api-key`) folded into A2 + dual-send rejected as Alt 4 (B1); spec edits made additive/annotative (S1); cited `a7da7a9` live-4xx evidence (S2); added the `metadata` divergence as A5 (cross-ADR X1). | diff --git a/docs/adr/ADR-002-type-surface-source-of-truth.md b/docs/adr/ADR-002-type-surface-source-of-truth.md new file mode 100644 index 0000000..26db181 --- /dev/null +++ b/docs/adr/ADR-002-type-surface-source-of-truth.md @@ -0,0 +1,347 @@ +# ADR-002: Hand-authored domain types are the canonical 1.0 surface; generated types are a spec-derived reference + +> **Status**: Accepted | **Date**: 2026-06-12 | **Deciders**: M2 reconciliation (adversarial review passed, rev 2) + +## Context + +The SDK ships two parallel type definitions of the same API: + +- **`src/types.ts`** (347 lines) — hand-authored *domain* types: `Memory` (a + discriminated union over `fact`/`artifact`/`episode`), `SearchRequest`, + `RecallParams`, `Group`, `IngestJob`, etc. This is the surface every other module + imports and the one re-exported to consumers. It is ergonomic, documented, and + shaped for how callers think (`Memory`, `recall(params)`), not how HTTP is wired. +- **`src/generated/types.ts`** (1,353 lines) — produced by + `npm run gen:types` (`openapi-typescript spec/memory.json -o src/generated/types.ts`). + It is *operation/path*-shaped (`paths["/v1/memories"]["post"]`, + `operations["ingest_memories_v1_memories_post"]`, `components["schemas"][…]`), + faithfully mirroring the OpenAPI document. It is **imported nowhere** in `src/` — + a grep for `generated/types` returns no hits. It is dead relative to the public + surface. + +Two pieces of in-tree documentation frame this as unfinished business pointing in a +particular direction: + +1. `src/types.ts:1-2` header: *"Hand-written types matching `openapi/memory_v2.json`. + Regenerate the canonical version with `npm run gen:types` once the spec is final."* + This implies the generated file is the eventual canonical surface and the + hand-authored one is interim. +2. The roadmap feature `wire-generated-types` (m2/s5) restates that as a 1.0 step. + +Both contain a concrete defect: **`openapi/memory_v2.json` does not exist.** The +`gen:types` script reads `spec/memory.json`, and that is the only spec file on disk. +So the documented source of truth for the type surface names a phantom file, and the +"regenerate the canonical version" instruction, taken literally, would either fail or +silently generate from a different source than the comment claims. + +A second, related cluster of unfinished type-surface work is the set of +**declared-but-unwired forward fields** — types that exist in `src/types.ts` but have +no request-side trigger or resolution helper behind them: + +- `Memory.expanded?: Record` (`types.ts:74`) — an expansion field + on responses, but there is no `expand` parameter on search/list/recall to request + it, and no spec-documented request trigger for it. +- `PromptTemplate` (`types.ts:283`) — its own doc says it exists so that *"a future + API endpoint can return xmem's preferred template and the SDK renders with it."* + No such endpoint exists; only the client-side `DEFAULT_PROMPT_TEMPLATE` + + per-call override (`recall(..., { template })`) is wired today. +- `IngestJobResult.memories_superseded_by: Record` (`types.ts:119`) — + a typed old-id → new-id map returned by ingest, with no SDK helper to follow the + chain (i.e. resolve a superseded fact to its replacement). + +The forces in tension: + +- **Documentary intent vs structural fit.** The in-tree comments say "make the + generated file canonical," but the generated file is OpenAPI-operation-shaped, not + the domain surface consumers want. Adopting it wholesale would be a different, + worse public API, not a finishing touch on the current one. +- **Drift risk of two parallel definitions.** Two hand-/machine-authored type files + describing the same API can silently disagree. If neither is exercised against the + other, "in sync with the spec" is an unverified claim. +- **Speculative surface vs honest gaps.** The declared-but-unwired fields are a + standing invitation to either build speculative features (an `expand` param, a + template-fetch path) before the server supports them, or to leave typed dead-ends + in the 1.0 surface with no recorded rationale. +- **"Once the spec is final."** The header gates regeneration on spec finality — but + the spec is being actively corrected *as part of this same milestone* (ADR-001 + edits auth, error, and the PATCH endpoint). So "regenerate now and freeze" would + capture a spec mid-correction. + +This ADR covers the type-surface decision for `m2/s5` (the `type-surface-finalization` +project). ADR-001 covers the contract/wire-shape reconciliation; this one covers +which type definitions are canonical and what to do with the unwired forward fields. + +## Decision + +**The hand-authored `src/types.ts` is the canonical public type surface for 1.0. +`src/generated/types.ts` is retained as a spec-derived *reference* artifact, kept in +sync via `npm run gen:types` (source: `spec/memory.json`), not promoted to the public +surface. We correct the stale source-of-truth references, regenerate the reference +file *after* ADR-001's spec corrections land, and add a reproducibility guard +(`gen:types` + `git diff --exit-code`) that keeps the committed reference provably in +sync with the spec — with full hand-authored-vs-spec conformance assertions scoped as +an optional follow-up, not a 1.0 precondition.** + +For the declared-but-unwired forward fields, **we wire only what the live API already +supports today and defer the rest with a recorded rationale:** + +- `memories_superseded_by` → **wire now**: add a client-side helper to resolve the + superseded chain (follow old-id → new-id, fetch the replacement via the existing + `get()`). It needs no new server capability. +- `Memory.expanded` / an `expand` request parameter → **defer**: no spec-documented + request-side trigger exists; we do not invent one. The response field stays typed + (harmless) and is revisited when the API documents `expand`. +- server-supplied `PromptTemplate` → **defer**: no server endpoint returns a template; + the existing client-side default + per-call override already covers today's need. + The type stays as the forward-looking contract its own doc describes. + +## Rationale + +### Key Factors + +1. **Canonical-ness should track what consumers depend on, not what a comment + aspires to.** Every `src/` module and every external consumer imports + `src/types.ts`. The generated file is imported by nothing. Promoting the generated, + operation-shaped types to canonical would not "finish" the current surface — it + would *replace* the domain API (`Memory`, `recall(params)`) with a wire API + (`operations[...]`, `paths[...]`). That is a downgrade in ergonomics disguised as a + stabilization step. The hand-authored file is already the contract; we make that + official instead of pretending the generated file is the destination. + +2. **The "make generated canonical" instruction rests on a false premise.** Its own + pointer, `openapi/memory_v2.json`, does not exist. An instruction that cannot be + followed literally is not a plan; it is a stale TODO. Correcting the reference to + `spec/memory.json` and recording the *actual* relationship (hand-authored = + canonical, generated = reference) replaces aspiration with a decision. + +3. **The generated file is a reproducible, spec-faithful reference — not an automatic + oracle.** Be precise about what keeping it actually buys, because the two surfaces + are structurally different (the generated file is operation/path-shaped; + `src/types.ts` is domain-shaped), so a mechanical diff between them is *not* + available. Two distinct guarantees are on offer, at very different costs: + - **Reproducibility (cheap, adopted):** `npm run gen:types && git diff --exit-code` + in the local/pre-publish gate proves the committed reference is exactly what the + current `spec/memory.json` produces — i.e. nobody hand-edited it and it tracks + spec changes. This catches reference rot, and gives human reviewers a + spec-faithful artifact to diff a hand-authored type change against during review. + It does **not**, by itself, prove the hand-authored `Memory` matches the spec. + - **Conformance (real but costly, optional):** type-level assertions (e.g. + `expectAssignable` for the handful of + wire-critical types) *would* mechanically verify the canonical surface against the + spec-derived schemas. This is genuinely valuable but has authoring/maintenance + cost and is scoped as an optional hardening, not a precondition. + + We adopt reproducibility now and leave conformance assertions as an explicit + follow-up. Even at the reproducibility tier, the generated file earns its keep: a + regenerable, in-tree mirror of the spec is a better review aid and a lower + reintroduction cost than nothing — which is the honest case for keeping it over + deletion. + +4. **Regenerate after the spec is corrected, not before.** ADR-001 edits the spec + (auth docs, the PATCH/UpdateRequest removal). Regenerating `src/generated/types.ts` + *before* those edits would bake the pre-correction spec — including the dead + `UpdateRequest` — into the reference. Sequencing the regen after ADR-001 makes the + reference reflect the reconciled 1.0 spec. + +5. **Wire support, not speculation.** `memories_superseded_by` is data the server + *already returns*; a resolver is pure client-side convenience over an existing + field and the existing `get()`. By contrast, `expand` and a server-supplied + `PromptTemplate` would require the SDK to call request shapes or endpoints the API + does not document — building those now is speculative surface that could be wrong + when the server actually ships them. Wiring the supported field and deferring the + unsupported ones (with the reason written down) is the honest 1.0 posture and + directly satisfies the story's "implemented or explicitly closed with rationale" + bar. + +## Consequences + +### Positive + +- **The public surface stays ergonomic and stable.** Consumers keep `Memory`, + `recall(params)`, etc.; nothing about the imported types changes at 1.0. +- **The generated file gains a purpose.** It goes from dead code to a reproducible, + spec-derived reference (reproducibility guard now; optional conformance assertions + later) — a review aid and conformance on-ramp rather than clutter. +- **Source-of-truth docs become true.** The `openapi/memory_v2.json` phantom is + replaced with `spec/memory.json` everywhere it appears, so a future maintainer + running `gen:types` gets what the comment promises. +- **No speculative dead-ends ship in 1.0.** Each unwired field is either given a real + helper (`memories_superseded_by`) or carries a recorded "deferred because the API + doesn't support it yet" rationale. + +### Negative + +- **We keep two type files indefinitely.** The hand-authored canonical surface must be + kept consistent with the spec by hand, rather than being machine-generated. + *Accepted:* the ergonomic gap between domain and operation types is real; the + reproducibility guard keeps the *reference* honest, and the optional conformance + assertions can later verify the canonical surface against it — recovering most of + the value generation would have given us, without the breaking downgrade. +- **The guard is a new maintenance surface.** The reproducibility guard must be re-run + (and the reference re-committed) whenever the spec changes; the optional conformance + assertions, if added, must track legitimate domain-surface evolution. *Accepted:* + the reproducibility tier is one gate command, and the conformance tier is opt-in and + scoped to wire-critical types, so neither grows unbounded. +- **Deferred fields remain typed but inert.** `Memory.expanded` and `PromptTemplate` + stay in the surface without a live trigger, which a reader might mistake for working + features. *Accepted:* their docstrings already mark them forward-looking, and this + ADR records the deferral explicitly so the gap is intentional, not silent. + +### Neutral + +- **Regeneration is sequenced behind ADR-001.** This creates an ordering dependency in + the sprint (spec corrections → regen) but no functional coupling beyond that. +- **Deleting the generated file remains a future option.** If, after a release or two, + the conformance assertions are never added and the reference is never consulted in + review, a later decision can drop it; this ADR does not foreclose that. + +### Application — disposition of the type-surface items + +| Item | Disposition | Notes | +|------|-------------|-------| +| Canonical surface | Hand-authored `src/types.ts` is canonical; `src/generated/types.ts` is reference | No public-surface change | +| `openapi/memory_v2.json` reference | **Correct** to `spec/memory.json` in `src/types.ts:1-2` (and the roadmap note) | Phantom file → real source | +| `src/generated/types.ts` | **Regenerate** from corrected spec *after* ADR-001 lands; add `gen:types`+`git diff --exit-code` reproducibility guard (conformance assertions optional follow-up) | Reproducible reference | +| `memories_superseded_by` | **Wire now** — client-side superseded-chain resolver over existing `get()` | Additive helper | +| `Memory.expanded` / `expand` param | **Defer** — no documented request trigger | Type stays; rationale recorded | +| server-supplied `PromptTemplate` | **Defer** — no server endpoint; client default suffices | Type stays as forward contract | + +## Alternatives Considered + +### Alternative 1: Adopt the generated types as the canonical public surface + +**Description**: Follow the literal `src/types.ts` header instruction — make +`src/generated/types.ts` the source of truth, re-export it, and rewrite `src/` modules +to consume the operation/path-shaped types, deleting or thinning the hand-authored +file. + +**Pros**: +- One machine-generated source, regenerated from the spec — no hand-maintenance, no + drift by construction. +- Maximally spec-faithful: the public types *are* the spec. + +**Cons**: +- The generated types are OpenAPI-operation-shaped. The public API would become + `operations["search_memories_..."]["requestBody"]` instead of `SearchRequest`, and + the `Memory` discriminated union (which callers rely on for `m.type`-narrowing) + would have to be reconstructed by hand anyway. +- It is a breaking change to the entire public surface immediately before 1.0, for a + *worse* developer experience. +- It bakes the spec — mid-correction under ADR-001 — into the surface. + +**Why not chosen**: It optimizes for "machine-generated" over "good public API" and +would degrade ergonomics while breaking every consumer. The generated shape is a +faithful *mirror of HTTP*, not the *domain model* the SDK exists to provide. + +### Alternative 2: Delete the generated file entirely + +**Description**: Remove `src/generated/types.ts` and the `gen:types` script; commit to +the hand-authored surface as the sole type definition. + +**Pros**: +- Eliminates the two-file question and any drift between them outright. +- Smallest tree; nothing dead to explain. + +**Cons**: +- Throws away a cheap, reproducible spec-derived reference. Even granting (per Key + Factor 3) that a regen-and-diff guard only proves *reproducibility*, not full + conformance, that reference is still the artifact a reviewer diffs a hand-authored + type change against — and it is the substrate the optional conformance assertions + would build on. Deleting it forecloses the cheap path to the stronger guarantee. +- Makes future regeneration (if the spec stabilizes and the team wants machine help) + a from-scratch reintroduction rather than a re-run. + +**Why not chosen**: This is the closest alternative, and on the *honest* (weaker) +value of the kept file it is genuinely competitive — if we only ever ship the +reproducibility guard, the file's marginal value is "a review aid + a cheap on-ramp to +conformance assertions." We keep it because that on-ramp matters: the file is *unused* +today, not *useless*, and retaining a regenerable reference costs one gate command +while preserving the option to add real conformance checks. If, after a release or +two, the conformance assertions are never added and the reference is never consulted +in review, revisit and delete (see Neutral consequences). + +### Alternative 3: Build the speculative forward fields now (`expand`, server templates) + +**Description**: Treat all declared-but-unwired fields as 1.0 work — add an `expand` +parameter wired to `Memory.expanded`, and a template-fetch/cache path for a +server-supplied `PromptTemplate`. + +**Pros**: +- Closes every typed dead-end with a working feature; nothing inert in the surface. + +**Cons**: +- There is no documented server support for either. The SDK would be guessing the + request shape for `expand` and inventing an endpoint for templates — likely to be + wrong when the server actually ships them, then needing a breaking correction. +- Spends 1.0 effort on speculative surface instead of the confirmed gaps. + +**Why not chosen**: Building against an undocumented server contract is exactly the +kind of speculative drift this milestone is meant to eliminate. Deferring with a +recorded rationale is the disciplined choice; `memories_superseded_by` is wired +precisely *because* it needs no new server capability. + +## Implementation Notes + +- **Correct the references**: edit `src/types.ts:1-2` to name `spec/memory.json` + (matching `package.json`'s `gen:types`), and update the roadmap + `wire-generated-types` note. Verify `gen:types` runs clean against `spec/memory.json`. +- **Regenerate** `src/generated/types.ts` *after* ADR-001's spec corrections so the + reference reflects the reconciled 1.0 spec (no dead `UpdateRequest`). +- **Reproducibility guard (now)**: add `gen:types` + `git diff --exit-code` to the + local/pre-publish gate so the committed reference provably tracks `spec/memory.json` + and nobody hand-edits the generated file. This proves *reproducibility*, not full + conformance — state that honestly. +- **Conformance assertions (optional follow-up)**: type-level `expectAssignable` + checks for the wire-critical types, if/when the team wants the stronger guarantee. + Scoped as hardening, not a 1.0 precondition. +- **Superseded-chain helper**: *disposition is wire-now* (it needs no new server + capability). The mechanism — method name, location, and whether it takes an + `IngestJobResult` or an id — is a **design-doc decision**, not fixed here. Pure + addition; TDD with mocked `HttpClient`. +- **Deferrals**: leave `Memory.expanded` and `PromptTemplate` typed; ensure their + docstrings note the deferral, and record the closed-with-rationale outcome on the + roadmap feature. + +All of this is additive or doc/spec-only → a **minor** bump under `RELEASING.md`. + +## Validation + +- **Canonical surface unchanged**: the public type exports are identical before and + after (the existing test suites compile and pass without type edits to consumers). +- **References resolve**: `npm run gen:types` runs without error from + `spec/memory.json`, and no occurrence of `openapi/memory_v2.json` remains in the + tree (`grep` returns nothing). +- **Reproducibility guard works**: hand-editing the generated reference (or a spec + change without regen) fails `gen:types` + `git diff --exit-code`; a clean, + regenerated tree passes. Regeneration after ADR-001 produces a reference with no + `UpdateRequest`. *(If conformance assertions are added later: desyncing a + wire-critical hand-authored type from the spec-derived schema fails the assertion.)* +- **Superseded resolver**: unit tests show a superseded id resolves to its replacement + and a non-superseded id is returned/handled as-is. +- **Deferrals are legible**: a reader of the 1.0 surface can tell `expand` and + server-templates are intentionally deferred (docstring + this ADR), not forgotten. + +## Related Decisions + +- **ADR-001** (companion): SDK↔spec wire-contract reconciliation policy. The spec + corrections it makes are an explicit upstream dependency of the regeneration step + here. + +## References + +- `src/types.ts` — header SoT comment (L1-2); `Memory.expanded` (L74); + `IngestJobResult.memories_superseded_by` (L119); `PromptTemplate` (L283). +- `src/generated/types.ts` — generated, operation/path-shaped, unimported (1,353 lines). +- `package.json` — `gen:types` = `openapi-typescript spec/memory.json -o src/generated/types.ts`. +- `src/memories.ts` — `DEFAULT_PROMPT_TEMPLATE` + `recall(..., { template })` + client-side template path. +- `RELEASING.md` — pre-1.0 minor-bump policy. + +--- + +## Revision History + +| Date | Status | Notes | +|------|--------|-------| +| 2026-06-12 | Proposed | Initial proposal — type-surface source of truth + disposition of declared-but-unwired fields (m2/s5). | +| 2026-06-12 | Proposed (rev 2) | Adversarial-review fixes: replaced the overstated "conformance oracle" with an honest two-tier guard — reproducibility now, conformance assertions optional (B1); re-evaluated the delete alternative against the weaker honest value; moved superseded-helper mechanics to the design doc (S1). | diff --git a/docs/adr/ADR-003-live-verified-supersession-of-adr-001-dispositions.md b/docs/adr/ADR-003-live-verified-supersession-of-adr-001-dispositions.md new file mode 100644 index 0000000..554762c --- /dev/null +++ b/docs/adr/ADR-003-live-verified-supersession-of-adr-001-dispositions.md @@ -0,0 +1,594 @@ +# ADR-003: Supersede ADR-001's un-probed dispositions with live-verified contract facts + +> **Status**: Accepted | **Date**: 2026-06-15 | **Deciders**: autonomous run (gitban-adr-writer ⇄ gitban-adr-reviewer, approved rev 2) | **Supersedes (in part)**: ADR-001 (A1, A3, A4 dispositions; corrects the A2 quota premise) + +## Context + +ADR-001 ("Treat the shipping SDK as the canonical 1.0 contract and correct the +spec to match", *Accepted* 2026-06-12) set a reconciliation policy for four +SDK↔`spec/memory.json` divergences and shipped it in v0.3.0 (sprint M2RECON). +That ADR was decided under an explicit, recorded constraint: **no live-API +credentials existed in the environment** ("Verification gap … cannot *probe* the +live wire shape right now"). Its risk posture — "where the spec describes a wire +shape we cannot currently disprove and tolerating it is cheap, harden the SDK to +accept *both* shapes rather than guess" — was a deliberate response to that gap, +and ADR-001's own **Validation** section recorded the exit condition: + +> *Revisit trigger:* the first time `npm run smoke` runs with real credentials, +> capture the actual auth acceptance, one real error body, and the response +> headers … If `Bearer` is somehow *not* accepted, this decision must be reopened +> immediately. + +Credentials have now been supplied (`.env.local`). The deferred verification +spike (gitban card **2syddu**) was executed on **2026-06-15** against the live +deployment at `https://api.production.xtrace.ai` with a real free-tier key, +read-only/non-mutating calls only (GET list, POST search with a bad body, GET +missing id, PATCH with a probe body, a bad-token 401). The raw bodies and headers +captured there are the evidence base for this ADR. The probe **overturned several +of ADR-001's dispositions** and surfaced one outright SDK bug. + +A note on evidentiary strength up front, because it shapes every disposition +below. The error-envelope captures (401/404/422) are **observed at n=1 per error +class** — one real body each, not a statistical sample — and the **429 envelope +was never captured at all** (forcing one requires exhausting a live bucket, which +the read-only probe did not do). The auth finding required a **second, +controlled re-probe** (2026-06-15, n=8, order-reversed) because the first reading +was a confounded n=1; that re-probe is recorded below and it *reversed* the +initial auth conclusion. The dispositions are calibrated to that strength: we +assert where we measured cleanly, and we *retain* tolerance where we did not. + +1. **A1 — Error envelope (ADR-001: "harden to both shapes").** Every non-2xx the + probe produced — a 401, a 404, and a 422, each **observed at n=1** (one body + per error class, not a sample) — returned the legacy + `{error:{type,code,message,request_id,details?}}` shape. The spec-documented + `{detail:{code,message}}` and the FastAPI 422 `detail:[{loc,msg,type}]` array + were **not produced in these observations**. The live 422 field errors were + carried under `error.details.errors[]` as `{field,message,type}` — not + `{loc,msg,type}`. So the SDK's legacy-first `parseErrorBody` is *correct against + the bodies we saw*, and it is `spec/memory.json`'s "Error envelope" section that + contradicts them. **The 429 envelope was not captured**, so the error-shape + evidence covers three error classes, not the full set. The `{detail}` / + `detail[]` branches ADR-001 added were **not exercised** by these observations. + +2. **A2 — Auth (not decided in ADR-001's table beyond "keep `Bearer` default; + add `x-api-key` opt-in").** Both `Authorization: Bearer` and `x-api-key` + return 200 with the same key, same endpoint, back-to-back. The *first* probe + appeared to show a per-scheme rate-limit asymmetry (`Bearer` → `limit: 3`; + `x-api-key` → `limit: 30`) and was initially read as a **10× quota + difference**. **That reading was wrong.** It was a confounded n=1: the two + schemes were sent in fixed order (Bearer first), and `spec/memory.json` + documents the rate bucket as keyed on `(org_id, key_hash)` — *not* on the auth + scheme — so a scheme-based difference contradicts the documented bucketing. + + A **controlled re-probe** (2026-06-15, n=8) reversed the call order + (`x-api-key` *first* this time) and alternated schemes, capturing + limit/remaining per call: + + | # | scheme | limit | remaining | + |---|--------|-------|-----------| + | 1 | x-api-key (first) | 3 | 2 | + | 2 | bearer | 30 | 29 | + | 3 | x-api-key | 30 | 28 | + | 4 | bearer | 30 | 27 | + | 5 | bearer | 30 | 26 | + | 6 | bearer | 30 | 25 | + | 7 | bearer | 30 | 24 | + | 8 | x-api-key | 30 | 23 | + + Two facts fall out cleanly. First, the `limit: 3` tracks **call position, not + scheme**: this time `x-api-key` was called first and *it* got `limit: 3`, so + the "3" is a first-call / cold-start artifact, not a property of `Bearer`. + Second, `remaining` **decrements continuously (29 → 23) across both schemes**, + which means the rate bucket is **shared** regardless of which header is sent — + exactly the `(org_id, key_hash)` keying the spec describes. **There is no + scheme-based quota advantage.** The original 10× claim is retracted; this is the + process catching its own error, and we record it as such. + +3. **A3 — Rate-limit headers (ADR-001: "additively parse `RateLimit-*`").** The + live server emits `x-ratelimit-limit`, `x-ratelimit-remaining`, + `x-ratelimit-reset` (x-prefixed) on a 200; no bare `RateLimit-*` is present, + and `-reset` is an **epoch timestamp** (e.g. `1781491860`), not delta-seconds. + `parseRateLimit` (`src/http.ts`) reads `RateLimit-Limit/Remaining/Reset`. + `Headers.get()` is case-insensitive, but the `x-` prefix is a literal part of + the name, so the lookup **never matches** and `RateLimitSnapshot` is *always* + `undefined` in production. ADR-001 framed A3 as a no-regret capability add + ("present → exposed, absent → `undefined`, never a regression"); in reality + the capability never fires. This is a **shipped bug**, not a dead branch. + +4. **A4 — PATCH endpoint (ADR-001: "won't-do; server returns 405; annotate spec + as removed").** This is the largest overturn. `PATCH /v1/memories/{id}` with + `{"text":"x"}` returned **422 `empty_patch` — "Provide add_group_ids and/or + remove_group_ids"**, *not* 405. The endpoint **exists**; its current contract + is **group-membership editing** (`add_group_ids` / `remove_group_ids`), not + the spec's old `{text,metadata}` `UpdateRequest`. `spec/memory.json` even + treats PATCH (≈ line 113) as a live, API-key-only operation, contradicting its + own "removed" annotation. ADR-001's A4 rested on git-history inference (PR #6's + commit message: "PATCH … was removed server-side, returns 405"); the live + server falsifies the 405 premise. The *old* `{text,metadata}` `UpdateRequest` + is still genuinely gone — but a *different* PATCH contract is live and + unmodelled by the SDK. + +The forces in tension: + +- **Live fact vs. accepted decision.** ADR-001 is *Accepted* and shipped. Its + dispositions are now contradicted by direct measurement on the deployment it + was reasoning about. The accepted record must not be silently mutated, but the + contract it locks can no longer stand on falsified premises. +- **Epistemic upgrade — and its limits.** ADR-001's posture was correct *for its + evidence state* (blind harden-both under no-probe). Where the probe measured + cleanly, holding that posture now would itself be the error — we would be + shipping a header parse that never matches and disclaiming a method for an + endpoint that demonstrably exists. But the upgrade is uneven: the auth reading + was wrong on the first pass and only the controlled re-probe corrected it, and + the 429 envelope was never seen at all. So the same evidence that *strengthens* + A1 (for 401/404/422) and A3 also tells us where tolerance must be **kept**, not + pruned. +- **Scope of the reopen.** The probe was read-only and non-mutating. It did not + capture the PATCH happy-path response (status code, whether it returns the + updated `Memory`) or a real 429's headers, because both require mutating the org + or exhausting a live bucket. Some facts are settled (the 401/404/422 envelopes, + the rate-limit header prefix, the shared bucket); two probe-shaped holes remain + (A4's success contract; the 429 envelope), and the dispositions are gated and + retained accordingly. + +This ADR governs the remediation of ADR-001's live-checked dispositions. It is +scoped to the roadmap node tracking this rework (`m3/s6`). It supersedes specific +ADR-001 dispositions; it does **not** rewrite ADR-001's history or its still-valid +elements (A5 `metadata`, the no-regression *principle*, and the policy's +direction-of-drift framing all stand). + +Framed honestly, the four reviewed dispositions are not four equally-strong +overturns — they sit on a deliberate evidence gradient, and the unifying intent is +narrow: **correct the contract and docs to the measured wire, no more.** + +- **A1** — a spec correction, well-evidenced across three error classes (401/404/422) + but at n=1 each and with the 429 envelope uncaptured; code unchanged, tolerance + retained. +- **A3** — a shipped no-op bug fix, airtight: the parser provably never matched the + live `x-ratelimit-*` headers. +- **A4** — a premise reversal (PATCH is live, not 405) that is well-evidenced for + the *spec correction* (A4a) and a *method gated on a future probe* (A4b). +- **A2** — a documentation note: the premise that drove an earlier flip was + corrected by the controlled re-probe, so the conservative `Bearer` default stands + and we merely document the shared bucket. + +## Decision + +**We supersede ADR-001's A1, A3, and A4 dispositions, and clarify A2, replacing +each with the live-verified contract fact captured in card 2syddu (as corrected +by the controlled auth re-probe). We treat the live deployment at +`api.production.xtrace.ai` (probed 2026-06-15) as the source of truth wherever it +contradicts an ADR-001 disposition on a *cleanly measured* point, and we correct +`spec/memory.json` to match the live wire, not the lagging documentation. The next +published cut is `0.4.0`, a pre-1.0 minor bump — but a normal additive one: it +carries the new `memories.patch()` method, the A3 bug fix, and the spec +corrections. There is no breaking change, because A2 does not flip any +default (see below).** + +ADR-001 remains the canonical record of the *blind* reconciliation; this ADR is +the *probed* correction layered on top of it. Disposition by disposition: + +- **A1 (supersede the symmetry).** The live error envelope, in every body we + observed (401/404/422, **n=1 per class**), was + `{error:{type,code,message,request_id,details?}}`. We affirm the legacy + `{error}` branch of `parseErrorBody` as the canonical, primary path (it already + is) and **correct `spec/memory.json`'s "Error envelope" section to document + `{error:{…}}`** as the contract — overturning ADR-001's "additive/annotative, + don't assert" constraint *for this section specifically*, because we are no + longer asserting an un-probed claim: we observed it. The spec must also document + that live 422 field errors are carried under `error.details.errors[]` as + `{field,message,type}`. The `{detail:…}` and `detail[]` branches were not + exercised by these observations; we **retain them as documented + belt-and-suspenders** — not least because the **429 envelope was never + captured** and is extrapolated, not measured, so pruning the only tolerant + fallback on the strength of three single observations would be a larger bet than + keeping a few fixture-tested lines. They are demoted from "we don't know which is + live" to "defensive against shapes this deployment was not seen to emit." A + maintainer may prune them once a fuller capture (including the 429 body) exists; + that is not required by, nor authorised against the 429 by, this decision. + +- **A2 (documentation note — keep the `Bearer` default; do NOT flip).** We **keep + `Bearer` as the SDK default `authMode`** — preserving ADR-001's "no shipping + default is altered" guarantee — and instead **document that both schemes + authenticate against the same shared `(org_id, key_hash)` rate bucket, with no + quota difference between them.** The earlier "flip the default to `x-api-key` for + 10× quota" decision is **withdrawn**: the controlled re-probe (n=8, order + reversed) showed the apparent asymmetry was a first-call/cold-start artifact, not + a scheme property, and that `remaining` decrements continuously across both + schemes — confirming the shared bucket the spec already describes. With no quota + reason to flip, the default stays where ADR-001 put it (proven `Bearer`), and + `x-api-key` remains the additive opt-in ADR-001 added. **This A2 disposition + touches neither code nor spec:** `spec/memory.json` already documents + `Bearer`/`Token`/`x-api-key` (ADR-001 A2a) and already describes + `(org_id, key_hash)` bucketing, so A2 here is purely a documentation note (e.g. + in the SDK README / `authMode` doc comment) recording the shared-bucket fact and + that scheme choice carries no rate advantage. **Withdrawing the flip removes the + only breaking change this ADR contemplated.** + +- **A3 (supersede the header names and fix the bug).** We **correct + `parseRateLimit` to read `x-ratelimit-limit/remaining/reset`**, keeping + `RateLimit-*` as a fallback, and we **document that `x-ratelimit-reset` is an + absolute epoch-seconds timestamp**, not delta-seconds — so any consumer-facing + representation must not treat it as a duration. We correct the rate-limit header + section of `spec/memory.json` from `RateLimit-*` to `x-ratelimit-*`. This + promotes A3 from "additive capability" to "bug fix": the snapshot that ADR-001 + intended to expose has never populated in production. Note that this + **(re)defines `RateLimitSnapshot.reset`'s JSDoc semantics from delta-seconds to + epoch-seconds** — a deliberate redefinition, not a silent one. It is doc-only and + carries no compatibility cost because the field has *never populated* in + production (the parse never matched), so no consumer can have been depending on + the old delta-seconds reading. The `RateLimit-*` fallback is retained for the + same reason A1's tolerance is: it costs little and guards against deployments or + revisions that emit the un-prefixed family. + +- **A4 (supersede the won't-do; split into a spec correction now and a + probe-gated method).** We **reverse ADR-001's won't-do**, but in two distinct + parts with different evidence and different timing: + + - **(a) Spec correction — do now, fully evidenced.** We **correct + `spec/memory.json`**: un-annotate PATCH, drop the false "405 / removed server + side" claim, document the live **group-membership contract** + (`add_group_ids` / `remove_group_ids`), and resolve the spec's self- + contradiction (the "removed" note vs the line treating PATCH as a live + API-key-only op). This part rests on the cleanly captured live + `422 empty_patch — "Provide add_group_ids and/or remove_group_ids"`, which + proves both that the endpoint *exists* (not 405) and what its contract *is*. + No further probe is required for (a). + + - **(b) Add `memories.patch()` — gated on an implementation-phase live probe.** + We will add an SDK method that edits group membership via + `add_group_ids` / `remove_group_ids`. But **the live PATCH happy-path response + shape (status code; whether it returns the updated `Memory`) was NOT + captured** — capturing it requires mutating the org with a throwaway memory. + **Precondition:** before the sprint commits to building (b), the implementation + phase **must confirm it has org-mutation authority** — a tenant or memory it + may create *and* delete. If that authority is available, it runs the live + happy-path probe (status code + response body shape on a throwaway memory) and + builds the method's return type from the captured shape. **If org-mutation + authority is not available, ship (a) only and defer (b)** to a follow-up gated + on the same authority; do not guess the return type. + + The old `{text,metadata}` `UpdateRequest` stays genuinely removed — corrections + to memory *content* still flow through ingest/supersede; PATCH is for membership, + not content. + +This ADR records the decisions only. The concrete code/spec/test work it unblocks +is enumerated in Implementation Notes for a follow-on design doc and sprint. + +## Rationale + +The single through-line: **ADR-001 explicitly conditioned its dispositions on the +absence of a live probe and named the probe as the revisit trigger; the probe has +now run, so the trigger fires.** We are not relitigating a sound decision — we are +executing the exit condition ADR-001 wrote for itself. The probe contradicts some +dispositions (A3 outright, A4's 405 premise outright, A1's symmetry for the +classes we saw) and, after a controlled re-probe, *confirms* ADR-001's +conservative choice on others (A2's `Bearer` default). The honest reading is the +point: we correct what we measured cleanly and we keep what we did not. + +### Key Factors + +1. **Measurement beats inference — when the measurement is clean — and ADR-001 + agreed.** ADR-001's whole posture was "reason from shipping evidence and git + history because we cannot probe." Each disposition this ADR overturns was an + *inference* (the `{detail}` shape "might" be live; PATCH "returns 405" per a + commit message). Where a direct observation of the deployment those inferences + described is unambiguous — the header prefix is `x-ratelimit-*` and the parser + never matched (A3); PATCH returns `422 empty_patch`, not 405 (A4a) — it is + strictly stronger evidence and ADR-001's Validation section commits to honouring + it. Continuing to ship a header parser that never matches would be choosing the + weaker witness over the stronger one *after the stronger one arrived*. The A2 + episode is the necessary caveat: a single observation with an unruled-out + confound is *not* a clean measurement, and we treated it accordingly once the + re-probe exposed it. + +2. **The A4 reversal is a capability we were about to permanently disclaim.** + "Won't-do" on PATCH was not neutral: it told future engineers the endpoint is + dead and corrections only flow through ingest. The live contract shows PATCH is + the *only* way to edit group membership without re-ingesting — a real, + currently-unreachable capability. Annotating the spec as "removed" would have + propagated a falsehood into the 1.0 contract and steered consumers away from a + working operation. Reversing it now, before 1.0 locks, is far cheaper than + discovering it post-1.0. + +3. **The process caught its own error on A2 — and that is a virtue, not a + blemish.** The first probe read a per-scheme 10× quota gap and very nearly drove + a default-auth flip (the one breaking change this rework would have carried). A + controlled re-probe — reversed call order, alternating schemes, n=8 — showed the + "gap" was a first-call/cold-start artifact and that the bucket is *shared* across + schemes, exactly as `spec/memory.json`'s `(org_id, key_hash)` keying says. Two + lessons compound here. First, this is precisely why the rest of this ADR is + calibrated to evidentiary strength: an n=1 reading with an unruled-out confound + is not a basis for a breaking change, and we say so. Second, the correct A2 + disposition is the conservative one ADR-001 already chose: keep `Bearer`, + document the shared bucket. The cost of *not* flipping is now known to be zero + (no quota is left on the table), so the prior guarantee ("no shipping default is + altered") stands on its merits, not in spite of evidence. + +4. **A3 was sold as no-regret; it is currently no-op.** ADR-001 justified A3 as + "present → exposed, absent → undefined, never a regression." That framing + assumed the right header names. With the wrong prefix the snapshot is *always* + undefined, so the proactive-throttling benefit ADR-001 claimed simply does not + exist in production. Fixing the prefix is what makes the original A3 promise + true; it is not new scope, it is delivery of un-delivered scope. + +5. **Superseding, not rewriting, preserves institutional memory.** ADR-001's + blind posture was correct for its evidence state and is itself a useful record + ("here is how we reasoned when we couldn't probe, and here is the trigger that + would change it"). Editing ADR-001 in place would erase that reasoning trail. + A superseding ADR keeps both: the original judgement and the corrected one, + with the evidence (card 2syddu) that bridges them. + +## Consequences + +### Positive + +- **The 1.0 contract aligns with the deployment that will serve it.** After the + spec corrections, `spec/memory.json` describes the error envelope, rate-limit + headers, and PATCH contract the live server actually presents — closing the + drift ADR-001 closed *on paper* but, for A1/A3, against the wrong target. +- **No breaking change.** Withdrawing the auth-default flip means `0.4.0` is a + normal additive minor: every existing consumer sees identical default behaviour, + and ADR-001's "no shipping default is altered" guarantee holds intact. +- **Proactive throttling finally works.** Fixing `parseRateLimit` makes + `RateLimitSnapshot` actually populate, delivering the capability ADR-001 + intended. +- **A real, working capability becomes reachable.** `memories.patch()` exposes + group-membership editing that the SDK currently cannot do at all (gated on the + implementation-phase happy-path probe). +- **The auth bucketing is now documented accurately.** Consumers learn that scheme + choice carries no rate advantage — closing the door on the same 10× confusion + this ADR itself nearly fell into. +- **The decision is grounded in observation, honestly scoped.** Every disposition + cites a captured live response (card 2syddu), and where the observation was thin + (n=1, or the uncaptured 429) the ADR says so and retains tolerance rather than + overclaiming. + +### Negative + +- **We are trusting a single deployment, observed thinly, as the contract.** The + probe hit one environment (`api.production.xtrace.ai`) on one date, with the + error envelopes seen at **n=1 per class** and the **429 envelope not seen at + all**. If another deployment or a future server revision emits `{detail}` or + `RateLimit-*`, or if the uncaptured 429 differs, the corrected spec would be + wrong for it. *Accepted:* this is exactly why the `{detail}` branches are + *retained* rather than pruned, why `RateLimit-*` stays as a parse fallback, and + why capturing the 429 envelope is a Validation precondition. We correct the + *primary/documented* contract to the observed shape without discarding the + tolerance that costs little. +- **The A2 retraction shows a single-observation reading can be flat wrong.** The + initial 10× claim survived into a draft before the re-probe overturned it. + *Accepted:* this is a feature of the process, not a defect of the decision — it + is the reason every remaining disposition is scoped to its evidence and the + thinly-seen ones (n=1 envelopes, uncaptured 429) keep their fallbacks rather than + driving irreversible pruning or breaking changes. +- **A4(b) ships gated, not guaranteed.** We commit to adding `memories.patch()` + only if the implementation phase has org-mutation authority and captures the + success response shape. *Accepted:* the hole is an explicit precondition (see + Decision A4 and Validation), not papered over — if the authority or the capture + is unavailable, the spec correction A4(a) still ships and the method defers, so + no return type is ever guessed. +- **`spec/memory.json` diverges further from upstream's checked-in copy.** We are + now *asserting* corrected shapes (A1/A3) rather than only annotating, reversing + ADR-001's additive-only spec posture. *Accepted:* the assertions are + measured, not guessed, which is precisely the condition ADR-001 required before + asserting; the divergence is from a spec we have *proven* stale. + +### Neutral + +- **ADR-001 stays Accepted and unedited.** Its A5 (`metadata`) disposition and + its no-regression *principle* are untouched; A1/A3/A4 are superseded and A2 is + *affirmed* (the re-probe vindicated ADR-001's conservative `Bearer` default). A + cross-reference is added to ADR-001's Related Decisions / Revision History + pointing here (a pointer, not a rewrite of its body). +- **The `{detail}` branches remain in the code** as documented defensive fallbacks + against shapes this deployment was not seen to emit (and against the uncaptured + 429); whether to prune them later is a maintainer cleanup call once a fuller + capture exists, not a decision this ADR forces. + +### Application — disposition of the reviewed items + +| # | ADR-001 disposition | Live finding (card 2syddu, + auth re-probe) | This ADR's disposition | Breaking? | +|---|---------------------|-----------------------------|------------------------|-----------| +| A1 | Harden `parseErrorBody` to both `{error}` and `{detail}`/`detail[]`; spec edits additive | 401/404/422 each **observed n=1** as `{error:{…}}`; `{detail}`/`detail[]` not seen; 422 fields under `error.details.errors[]`; **429 not captured** | `{error}` is the documented contract; **correct spec to `{error:{…}}`**; **retain** `{detail}` branches (n=1 + uncaptured 429) | No (spec + docs; code unchanged) | +| A2 | (implicit) keep `Bearer` default; add `x-api-key` opt-in | First probe read 10× gap (`x-api-key` 30 vs `Bearer` 3); **re-probe (n=8, order reversed) refuted it** — "3" is a first-call artifact, bucket is **shared** `(org_id, key_hash)` | **Keep `Bearer` default (do NOT flip); withdraw the 10× claim.** Document the shared bucket (no scheme quota advantage). Doc-only — no code, no spec change | No (default unchanged) | +| A3 | Additively parse `RateLimit-Limit/Remaining/Reset` | Live emits `x-ratelimit-*`; `-reset` = epoch; current parse **never matches** (always `undefined`) | **Fix `parseRateLimit` → `x-ratelimit-*`** (keep `RateLimit-*` fallback); redefine `RateLimitSnapshot.reset` JSDoc → epoch seconds (doc-only, field never populated); correct spec | No (bug fix; additive surface) | +| A4 | **Won't-do**; annotate spec PATCH as removed (405) | PATCH is **live**: `422 empty_patch`; contract = group-membership (`add_group_ids`/`remove_group_ids`); **happy-path shape not captured** | **(a) correct spec now** (un-annotate, document membership contract; well-evidenced); **(b) add `memories.patch()`** gated on org-mutation authority + live happy-path probe; ship (a) only if (b) can't be probed. Old `{text,metadata}` stays gone | No (additive method) — *but* (b) gated on live probe | + +## Alternatives Considered + +### Alternative 1: Amend ADR-001 in place + +**Description**: Edit ADR-001's Application table, Decision, and Validation +sections directly to reflect the live findings; no new ADR. + +**Pros**: +- One document to read; no cross-referencing between two ADRs. +- The "current truth" lives in a single accepted record. + +**Cons**: +- Destroys the reasoning trail. ADR-001's blind harden-both posture and its + "probe is the trigger" logic are themselves valuable institutional memory; an + in-place edit erases *why* we reasoned that way under no-probe. +- Violates the convention that accepted ADRs are immutable records superseded by + new ones, not retconned — future readers could no longer tell what was decided + on 2026-06-12 vs what changed on 2026-06-15. +- Conflates two distinct evidence states (inferred vs measured) under one date. + +**Why not chosen**: The whole point of the supersession pattern is to preserve the +original judgement *and* the correction. The task is explicit that ADR-001's +history must not be rewritten. + +### Alternative 2: Open a fresh unblock issue and defer the rework to a future milestone + +**Description**: Treat the falsification as "newly discovered work," file it as a +backlog item / GitHub issue, and leave v0.3.0's dispositions in place until a +later milestone picks it up. + +**Pros**: +- No immediate churn; M2RECON closeout stays clean. +- Lets the probe's secondary holes (PATCH happy-path, real 429) be resolved before + any decision. + +**Cons**: +- Leaves a *known* bug (A3: rate-limit snapshot never populates) and a *known* + falsehood (A4: spec says PATCH removed/405) shipping toward 1.0 — exactly the + "silent drift at a contract-locking event" ADR-001 existed to prevent. +- Leaves the live error-envelope and rate-limit-header contract undocumented in the + spec, with no decision record explaining the drift. +- The A4 spec annotation would actively mislead consumers away from a working + endpoint. + +**Why not chosen**: Deferral re-creates the drift ADR-001 was written to kill, and +the cost compounds against the 1.0 wall. The probe's remaining holes are narrow +(one return shape, one 429 header capture) and belong to the *implementation* +phase, not a reason to defer the *decision*. + +### Alternative 3: Flip the default `authMode` to `x-api-key` for a larger quota + +**Description**: Change the SDK default from `Bearer` to `x-api-key` on the +strength of the first probe's apparent 10× rate-limit gap, accepting the resulting +default-behaviour change (and the breaking-change minor bump it forces). This was +the direction an earlier draft of this ADR actually took. + +**Pros**: +- *If* the gap were real, it would move every default-path consumer onto a 10× + larger rate budget with no caller action — a genuine correctness-of-default win. +- `Bearer` would remain reachable via `authMode`, bounding the regression risk. + +**Cons**: +- **Its premise is false.** The controlled re-probe (n=8, order reversed) showed + the apparent gap was a first-call/cold-start artifact tracking call *position*, + not auth scheme, and that `remaining` decrements continuously across both schemes + — the bucket is shared on `(org_id, key_hash)`, exactly as the spec documents. + There is no quota to gain. +- It would have introduced the *only* breaking change in this rework — and a + default-path behaviour change at that — entirely on the basis of a confounded + n=1 observation. + +**Why not chosen**: The re-probe eliminated the rationale outright. With no quota +difference, flipping the default would impose a breaking change for zero benefit +while discarding ADR-001's "no shipping default is altered" guarantee for nothing. +This is the alternative the controlled re-probe was *designed* to test, and it +failed the test — keeping `Bearer` (and merely documenting the shared bucket) is +the adopted A2. + +## Implementation Notes + +This ADR sets direction; a follow-on **design doc** owns the detail (return-type +shape for `patch()`, where `RateLimitSnapshot` is surfaced, the exact spec-section +edits) and a **sprint** carries the cards. In scope to be seeded from here: + +- **SDK code** + - `src/http.ts` `parseRateLimit`: read `x-ratelimit-limit/remaining/reset` + (retain `RateLimit-*` as fallback); treat `-reset` as epoch seconds. Update + `RateLimitSnapshot.reset`'s JSDoc to state epoch-seconds (was delta-seconds — + a deliberate redefinition; doc-only, the field never populated). + - **No change to the default `authMode`.** It stays `bearer` (A2 affirmed). Do + not touch `defaultHttpConfig`/`src/client.ts` auth defaults. + - New `memories.patch()` (`src/memories.ts`): group-membership editing via + `add_group_ids` / `remove_group_ids`. **Gated:** before building, the + implementation phase must confirm org-mutation authority and capture the live + happy-path response (status + body shape) on a throwaway memory. If neither is + available, defer this method and ship only the spec correction (A4a). +- **Spec corrections (`spec/memory.json`)** — *assertive* (observed) for A1/A3, + not merely annotative; A2 needs **no** spec change (the spec already documents + `Bearer`/`Token`/`x-api-key` and `(org_id, key_hash)` bucketing): + - "Error envelope" section → `{error:{type,code,message,request_id,details?}}`; + document 422 fields under `error.details.errors[]` as `{field,message,type}`. + - Rate-limit header section → `x-ratelimit-*`; document `-reset` = epoch seconds. + - PATCH (A4a): un-annotate as removed; document the group-membership contract; + drop the false 405/removed claim; resolve the spec's self-contradiction (the + "removed" note vs the line treating PATCH as a live API-key-only op). +- **Docs (A2)** — a SDK README / `authMode` doc-comment note recording that both + schemes hit the same shared `(org_id, key_hash)` rate bucket and that scheme + choice carries no quota advantage. No code, no spec edit. +- **Tests** — fixtures for the observed `{error}` 401/404/422 bodies (including + `error.details.errors[]`); `parseRateLimit` against `x-ratelimit-*` headers with + an epoch `-reset`, *and* retained coverage for the `RateLimit-*` fallback and the + `{detail}`/`detail[]` branches (kept, not pruned); `memories.patch()` happy/error + paths once the live response shape is captured. +- **Versioning** — next cut is **`0.4.0`**, a *normal additive* pre-1.0 minor (per + `RELEASING.md`): it carries `memories.patch()` (if probed), the A3 bug fix, and + the spec corrections. **No breaking change** — the auth default is unchanged. +- **ADR-001 linkage** — add a Related-Decisions / Revision-History *pointer* to + this ADR in ADR-001 (no body rewrite). + +Every change here is additive, a bug fix, or spec/doc-corrective — there is **no +breaking change**. The A3/`patch()` code changes must pass `typecheck → test → +build`; spec-only and doc-only edits skip the code pre-commit gate. + +## Validation + +We will know this was the right call if: + +- **The corrected spec matches a re-probe.** Re-running the card-2syddu probe + against `api.production.xtrace.ai` after the spec edits finds no contradiction: + the error envelope reads `{error:{…}}`, rate-limit headers are `x-ratelimit-*`, + and `-reset` parses as a future epoch second. +- **`RateLimitSnapshot` populates in production.** After the `parseRateLimit` fix, + a live 200 yields a defined snapshot with `limit`/`remaining`/`reset` from the + `x-ratelimit-*` headers — proving the A3 capability now fires (it never did + before). +- **The shared-bucket finding holds on re-test.** A live re-probe confirms + `remaining` decrements across both `Bearer` and `x-api-key` calls (shared + `(org_id, key_hash)` bucket) with no scheme-based `limit` difference once past the + first call — i.e. the withdrawn 10× claim stays withdrawn and the documented + shared-bucket note is accurate. +- **The org-mutation preconditions are met before `patch()` ships.** *Blocking + precondition:* the implementation phase must (i) confirm it holds org-mutation + authority (a tenant/memory it may create *and* delete), then (ii) using a + **throwaway test memory** capture the live PATCH happy-path — its status code and + whether it returns the updated `Memory` — and (iii) capture a real **429**'s + envelope and headers (the read-only probe could not force one against the live + bucket and did not mutate the org). `memories.patch()` return-type design and any + 429-specific snapshot/error handling are blocked on those captures. If either + authority or the happy-path capture is unavailable, ship the spec correction + (A4a) only and defer the method. If the captured shape differs from what the + design doc assumes, the design doc — not this ADR — is revised. +- **Revisit trigger.** If a future deployment or server revision is observed + emitting `{detail}` or `RateLimit-*` (the shapes we corrected away from), if the + uncaptured 429 envelope turns out to differ from `{error:{…}}`, or if a per-scheme + rate bucket is ever genuinely observed (re-opening the A2 question), reopen: the + single-deployment / thin-observation assumption (Negative #1) would no longer + hold. + +## Related Decisions + +- **ADR-001** (superseded in part, affirmed in part): A1, A3, and A4 dispositions + are superseded by this ADR on live evidence; A2's `Bearer` default is **affirmed** + — the controlled re-probe vindicated ADR-001's conservative choice. ADR-001's + A5 (`metadata`), its no-regression *principle*, and its drift-direction framing + remain valid. ADR-001's Validation section *predicted* this reopen. +- **ADR-002** (type-surface source of truth): unaffected, but `memories.patch()` + adds a method to the hand-authored `src/types.ts` surface ADR-002 governs; + the new request/response types follow ADR-002's source-of-truth rule. +- **gitban card 2syddu** (live-verification spike): the evidence base — raw 401/ + 404/422 bodies (n=1 each), `x-ratelimit-*` header capture, the PATCH 422 + `empty_patch` observation, and the controlled auth re-probe (n=8, order reversed) + that overturned the initial 10× reading — all captured 2026-06-15 against + `api.production.xtrace.ai`. + +## References + +- gitban card **2syddu** — "LIVE VERIFICATION RESULTS (2026-06-15)": raw error + bodies (n=1 per class), rate-limit header capture, PATCH falsification, and the + auth re-probe (n=8) establishing the shared `(org_id, key_hash)` bucket. +- `spec/memory.json` — "Error envelope" / "Response headers" sections; auth + security schemes + `(org_id, key_hash)` bucketing; PATCH + `/v1/memories/{memory_id}` operation (≈ L113) and `UpdateRequest` schema. +- `src/http.ts` — `parseRateLimit` (reads `RateLimit-*`, L156-169); `defaultHttpConfig` + (`authMode` default — **unchanged**, L176-192); auth-header selection (L58-62). +- `src/errors.ts` — `parseErrorBody` multi-envelope precedence (L75-92). +- `RELEASING.md` — pre-1.0 versioning: breaking changes bump the minor (L51-52). +- ADR-001 — `docs/adr/ADR-001-sdk-spec-reconciliation-policy.md` (Decision, + Application table, Validation/revisit trigger). + +--- + +## Revision History + +| Date | Status | Notes | +|------|--------|-------| +| 2026-06-15 | Proposed | Initial nomination — supersede ADR-001's live-falsified A1/A3/A4 dispositions (and flip A2 default authMode → x-api-key) on the evidence of card 2syddu's live probe of api.production.xtrace.ai. | +| 2026-06-15 | Proposed (rev 2) | Adversarial-review fixes after a controlled auth re-probe (n=8, order reversed) **falsified the A2 10× premise** (the apparent gap was a first-call/cold-start artifact; the rate bucket is shared on `(org_id, key_hash)`). **A2 reversed:** keep `Bearer` default (no flip), document the shared bucket — eliminating the only breaking change; `0.4.0` becomes a normal additive minor (M1/M3, B2). **A4 split** into spec-correction-now (A4a) + probe-gated method (A4b) with an explicit org-mutation-authority precondition (S3). Evidence language softened to "observed (n=1 per error class)"; **429 envelope flagged as not captured / extrapolated**, so `{detail}`/`detail[]` and `RateLimit-*` fallbacks are **retained, not pruned** (S1/S2/M1). A3 fix kept; added the `RateLimitSnapshot.reset` JSDoc redefinition note (delta→epoch, doc-only) (M2). | diff --git a/docs/designs/m2-1.0-spec-reconciliation-and-surface.md b/docs/designs/m2-1.0-spec-reconciliation-and-surface.md new file mode 100644 index 0000000..6ae85b8 --- /dev/null +++ b/docs/designs/m2-1.0-spec-reconciliation-and-surface.md @@ -0,0 +1,515 @@ +# Design Doc: M2 — SDK↔spec reconciliation and the 1.0 surface + +> **ADRs**: [ADR-001](../adr/ADR-001-sdk-spec-reconciliation-policy.md) (contract reconciliation), [ADR-002](../adr/ADR-002-type-surface-source-of-truth.md) (type-surface SoT) | **Date**: 2026-06-12 | **Author**: gitban M2 | **Roadmap**: `m2/s5` + +## Overview + +This document is the implementation bridge for the entire M2 milestone — locking a +self-consistent 1.0 contract for `@xtraceai/memory`. It implements two accepted ADRs +plus the additive search capabilities the milestone calls for. + +ADR-001 decided the reconciliation policy: the proven, shipping SDK is the canonical +1.0 contract; `spec/memory.json` is corrected (additively/annotatively) to match it; +and the SDK is hardened to tolerate documented-but-unverified *response* shapes where +that is a no-regret. ADR-002 decided that the hand-authored `src/types.ts` remains the +canonical public surface, the generated `src/generated/types.ts` is a spec-derived +reference with a reproducibility guard, and declared-but-unwired forward fields are +wired only where the live API already supports them. + +The work is **entirely additive** — no shipping default (auth header, error subclass, +endpoint set) changes. Every consumer upgrading sees identical behaviour plus new +opt-in capability. Under the pre-1.0 policy in `RELEASING.md` this is a **minor** bump: +**v0.2.1 → v0.3.0**. The implementation is sequenced so the spec corrections land +before the generated types are regenerated against them. + +## Requirements + +The implementation is complete when: + +1. **Error codes survive both envelope shapes.** `toError` extracts a stable `code` + and `message` from the legacy `{error:{…}}`, the spec `{detail:{code,message}}`, + *and* the 422 `{detail:[{loc,msg,type}]}` array — with the current shape's + behaviour bit-for-bit unchanged. +2. **Rate-limit state is observable.** `RateLimit-Limit/Remaining/Reset` are parsed + and reachable on the per-response `request()` return and on raised errors, without + changing any method's return type in a breaking way. +3. **The spec no longer contradicts the SDK.** `spec/memory.json` documents the + `Bearer` auth form alongside `Token`/`x-api-key`, annotates the removed PATCH + endpoint + `UpdateRequest`, and annotates the dropped `metadata` field — no silent + divergence remains for auth, the endpoint set, or `metadata`. +4. **`x-api-key` auth is reachable as an opt-in**, with `Bearer` remaining the default. +5. **The search surface is symmetric and typed.** A typed filter DSL emits wire-correct + `filters`; `include[]` is threaded through `recall`; and `searchAll()` auto-pages + search exactly as `list()` auto-pages list. +6. **The type-surface source of truth is true and guarded.** No reference to the + phantom `openapi/memory_v2.json` remains; `src/generated/types.ts` is regenerated + from the corrected spec; and a reproducibility guard fails if the committed + reference drifts from `spec/memory.json`. +7. **Declared-but-unwired fields are resolved.** `memories_superseded_by` has a + resolution helper; `Memory.expanded`/`expand` and server-supplied `PromptTemplate` + carry an explicit, documented deferral. +8. **All existing tests pass unchanged**, and every new work item ships with tests + written first (TDD) and a `typecheck → test → build` that is green. + +## Current State + +- **`src/http.ts`** — `HttpClient.request()` returns `{ body, status, requestId }`. + Auth is hard-coded `Authorization: Bearer ` (L44). `toError` (L117-127) + reads only `parsed.error` (`ApiErrorBody`). Only `x-request-id` (L77) and + `retry-after` (L119) headers are read. `defaultHttpConfig` (L129-146) builds the + config; default `baseUrl` is `https://api.production.xtrace.ai`. +- **`src/errors.ts`** — `errorForStatus(status, body, requestId, retryAfter)` maps + status → subclass and defaults `code` to `'unknown_error'`, `message` to a generic + string. `RateLimited` carries `retryAfter`; the base `MemoryError` carries + `status/errorType/code/message/requestId/details`. +- **`src/memories.ts`** — `search()` (L240) POSTs the body through verbatim; + `retrieve()` forces `mode:'compose'`; `recall()` (L298) fans out per-pool searches + (L369) and has no `include`; `list()` (L480) is an async-generator auto-pager; + there is no `searchAll()`. `DEFAULT_PROMPT_TEMPLATE` + `recall(...,{template})` is the + only template path. +- **`src/types.ts`** — header (L1-2) references the nonexistent `openapi/memory_v2.json`. + `Filter = Record` (L160). `SearchRequest.include` (L200) and + `ListQuery.include` (L171) exist; `RecallParams` (L222) has no `include`. + `SearchListEnvelope.extras.context_prompt` (L154) and `ArtifactDetails.full_content` + (L46) are typed. `Memory.expanded` (L74), `PromptTemplate` (L283), and + `IngestJobResult.memories_superseded_by` (L119) are declared but unwired. + `ApiErrorBody` (L339) types the legacy `{error:{…}}` shape. +- **`src/client.ts`** — `MemoryClient` + `MemoryClientOptions`; constructor builds the + `HttpClient` via `defaultHttpConfig`. **`src/generated/types.ts`** — generated, + operation-shaped, imported nowhere. +- **Tests** — vitest, mocked `HttpClient`/`fetch` (`src/recall.test.ts`, + `src/groups.test.ts`, `src/ai-sdk/tools.test.ts`). No CI; the only gate is the local + pre-commit `typecheck → test → build`, which runs only when code is staged. + +## Target State + +``` + MemoryClient(options) + authMode?: 'bearer'|'x-api-key' ──┐ + ▼ + ┌───────────────────────────┐ + │ HttpClient │ + request() returns │ request(): {body,status, │ + {body,status,requestId, │ requestId, rateLimit?} │ + rateLimit?} │ · auth header by authMode│ + │ · parse RateLimit-* hdrs │ + │ · toError → parseErrorBody(detail string|obj|array + {error}) + └──────┬─────────────┬──────┘ + │ │ + ┌────────────▼───┐ ┌─────▼─────────────────┐ + │ errors.ts │ │ memories.ts │ + │ MemoryError + │ │ search / retrieve │ + │ rateLimit? │ │ recall(+include) │ + │ RateLimited │ │ searchAll() ◀ new │ + └────────────────┘ │ resolveSuperseded() ◀ new + └─────┬─────────────────┘ + │ filters + ┌─────────────▼──────────────┐ + │ filter.ts ◀ new (DSL) │ + │ f.eq/in/gt/.../and/or/not │ + │ → Filter (Record) wire │ + └────────────────────────────┘ + +spec/memory.json ──(A2a/A4/A5 additive+annotative edits)──▶ gen:types ──▶ src/generated/types.ts + ▲ reproducibility guard +``` + +After all phases: the spec and SDK agree; the HTTP layer tolerates both error +envelopes and surfaces rate-limit state; `x-api-key` is an opt-in; the search surface +has a typed filter DSL, `include[]` on recall, and `searchAll()`; the type surface SoT +is corrected, regenerated, and guarded; and the superseded chain is resolvable. + +## Design + +### Key Design Decisions + +**KD-1 — A single `parseErrorBody()` with legacy-first precedence (A1).** +Introduce one pure helper (in `errors.ts`) that normalizes any error body to +`{ type?, code?, message?, details? }`: + +```ts +type ParsedError = { type?: string; code?: string; message?: string; details?: Record }; +function parseErrorBody(parsed: unknown): ParsedError | null; +``` + +Precedence, chosen to guarantee zero regression: +1. If `parsed.error` is an object → use it (current `ApiErrorBody` path, untouched). +2. Else if `parsed.detail` is an **array** → 422 validation: synthesize + `code: 'validation_error'` (FastAPI's 422 array has no top-level code), build a + human `message` (e.g. `"Validation failed: field error(s)"`), and preserve the + raw array as `details: { validation_errors: detail }`. +3. Else if `parsed.detail` is a **non-array object** → `{ code: detail.code, + message: detail.message }` (spec `ErrorEnvelope`). +4. Else if `parsed.detail` is a **string** → `{ message: detail }`. This is FastAPI's + *default* error shape (`HTTPException(detail="Not authenticated")` → + `{"detail":"Not authenticated"}`), the common form for 401/403/404 raised outside + the custom handler. Without this branch the only message the server sent is + discarded for the generic fallback. `code` is left to the status default. +5. Else → `null` (so `errorForStatus` applies its existing `'unknown_error'`/generic + defaults — unchanged behaviour). + +Note the array check precedes the object check (a JS array is also `typeof +'object'`), and the string branch is distinct from both. + +`toError` calls `parseErrorBody` instead of reading `parsed.error` directly; +`errorForStatus` is unchanged below the body shape. *Why a synthetic +`validation_error` code:* it gives consumers a stable, switchable code for 422s where +the wire provides none, and the full per-field detail is retained under +`details.validation_errors` — resolving the array→`Record` typing +without widening `MemoryError.details`. + +**KD-2 — Rate-limit state on the two *unambiguous* surfaces only (A3).** +`RateLimit-*` is parsed in the HTTP layer and exposed on the surfaces where it has a +well-defined meaning — deliberately **not** as a client-level "last seen" global. + +```ts +export interface RateLimitSnapshot { + limit?: number; // RateLimit-Limit + remaining?: number; // RateLimit-Remaining + reset?: number; // RateLimit-Reset (seconds) +} +``` +- `HttpClient.request()` returns an extra optional `rateLimit?: RateLimitSnapshot` + (existing destructures of `{body,status,requestId}` are unaffected). This is the + unambiguous per-response channel — each return carries *its own* response's headers. +- `MemoryError` gains an optional `rateLimit?: RateLimitSnapshot` (so a thrown error + carries the bucket state at failure time); `RateLimited` keeps `retryAfter` and also + carries `rateLimit`. The failure path is where proactive backoff actually triggers. + +Parsing: numeric coercion with `Number.isFinite` guards; absent headers → `undefined` +fields (never throws, never a regression). + +*Why no `client.rateLimit` global snapshot:* the SDK fires concurrent requests against +one client (`recall()` fans out N parallel per-pool searches), so a mutable +"last-response-wins" field would be non-deterministic under the most common multi-request +path — a worse-than-useless throttling signal. The per-response return + the error field +cover the real needs without racy shared state. A clean success-path aggregate (an +`onResponse` hook or a `*WithMeta` return) can be added later if demand appears; it is +**out of scope for v0.3.0**. + +**KD-3 — Auth mode is a config enum, default `bearer` (A2b).** +`MemoryClientOptions.authMode?: 'bearer' | 'x-api-key'` (default `'bearer'`), threaded +through `defaultHttpConfig` → `HttpClientConfig.authMode`. In `request()` the header +block becomes conditional: +- `'bearer'` → `Authorization: Bearer ` (current default, byte-identical). +- `'x-api-key'` → `x-api-key: ` (no `Authorization` header). + +`X-Org-Id` / `X-Request-Id` / `Accept` are unchanged in both modes. *Why an enum, not +a boolean:* it leaves room for the spec's `Token` form later without another breaking +option rename. + +**KD-4 — The filter DSL is key-agnostic; this is how A5 is reconciled (B1).** +New module `src/filter.ts`. The DSL builds conditions over **arbitrary field-name +strings**, not a fixed field enum. This is the deliberate resolution of the `metadata` +tension: the SDK dropped the typed `metadata` *field*, but the server still filters on +"any indexed payload field, including customer metadata keys" (spec filter-DSL table). +A key-agnostic builder therefore filters entity axes *and* arbitrary indexed/metadata +keys without reintroducing a typed `metadata` field — it is forward-compatible by +construction. `Filter = Record` stays as the raw escape hatch; the +builder's output is assignable to it. + +```ts +// src/filter.ts — typed fragments + combinators, emitting the wire DSL. +export type Comparable = string | number | boolean; +export interface FieldOps { + $eq?: Comparable; $ne?: Comparable; + $in?: Comparable[]; $nin?: Comparable[]; + $exists?: boolean; + $gt?: Comparable; $gte?: Comparable; $lt?: Comparable; $lte?: Comparable; + $between?: [Comparable, Comparable]; +} +export type Clause = Record + | { AND: Clause[] } | { OR: Clause[] } | { NOT: Clause }; + +export const f = { + // Single-field condition — ALL operators for one field go in ONE call, so a + // range is `f.field('price', { $gt: 10, $lt: 100 })`. This is the only way to + // put multiple operators on a field; it makes the clobber bug below impossible. + field: (name: string, ops: FieldOps): Clause => ({ [name]: { ...ops } }), + // Convenience shorthands (each is a single-operator field condition): + eq: (field: string, v: Comparable): Clause => ({ [field]: { $eq: v } }), + ne: (field: string, v: Comparable): Clause => ({ [field]: { $ne: v } }), + in: (field: string, v: Comparable[]): Clause => ({ [field]: { $in: v } }), + nin: (field: string, v: Comparable[]): Clause => ({ [field]: { $nin: v } }), + exists: (field: string, v = true): Clause => ({ [field]: { $exists: v } }), + between: (field: string, lo: Comparable, hi: Comparable): Clause => ({ [field]: { $between: [lo, hi] } }), + isNull: (field: string): Clause => ({ [field]: null }), // null = unset + and: (...c: Clause[]): Clause => ({ AND: c }), + or: (...c: Clause[]): Clause => ({ OR: c }), + not: (c: Clause): Clause => ({ NOT: c }), + // Implicit-AND of clauses on DISTINCT fields. Throws on a same-field collision + // rather than silently dropping operators (use f.field for multi-op-one-field, + // or f.and(...) to AND two conditions on the same field explicitly). + all: (...c: Clause[]): Filter => { + const out: Record = {}; + for (const clause of c) { + for (const [k, v] of Object.entries(clause)) { + if (k in out) { + throw new Error( + `filter: duplicate field "${k}" in f.all(); combine its operators in a ` + + `single f.field("${k}", {...}), or use f.and(...) to AND them explicitly`, + ); + } + out[k] = v; + } + } + return out; + }, +}; +``` +*Why a single-field `f.field` + a throwing `f.all`:* the earlier shape +(`Object.assign(...)` merging per-operator helpers) shallow-merges, so +`f.gt('price',10)` AND `f.lt('price',100)` would collapse to `{price:{$lt:100}}` — +silently dropping `$gt`. Forcing multi-operator fields through one `f.field` call (and +making `f.all` *fail loudly* on a same-field collision) makes a wrong range +unrepresentable. The output object is passed as `SearchRequest.filters`. *Doc note:* +`filters` is `@deprecated` only for *lifting scope axes* +(`user_id`/`agent_id`/`app_id`) out of it — its operator DSL over payload keys is the +supported, non-deprecated use; the design clarifies that comment. + +**KD-5 — `include[]` threads through recall (full_content only); accessors are the +already-typed fields (B2).** Add `include?: Array<'full_content'>` to `RecallParams`, +forwarded into each per-pool `this.search({ query, mode, limit, include, ...pool })`. +*Why `full_content` only, not `context_prompt`:* `recall()` discards the per-pool +`SearchListEnvelope`s — it dedupes/re-ranks the rows and renders **one** prompt +client-side, returning `{ memories, prompt, scopes }` with no per-pool `extras` channel. +`full_content` is coherent (it enriches the `Memory` rows recall returns); requesting +per-pool `context_prompt` would make the server assemble prompts recall then throws +away. The full `Array<'context_prompt' | 'full_content'>` stays on `SearchRequest` and +`retrieve()`, where the envelope (and its `extras?.context_prompt`) is returned intact — +those are the "first-class accessors" (already-typed `SearchListEnvelope.extras?.context_prompt` +and `ArtifactDetails.full_content`; the gap was *threading*, not new types). `retrieve()` +keeps `mode:'compose'` (unchanged). No removal of `mode`. + +**KD-6 — `searchAll()` mirrors `list()` (B3).** +```ts +async *searchAll(body: SearchRequest, context: RequestContext = {}): AsyncGenerator { + let cursor = body.cursor ?? undefined; + while (true) { + const env = await this.search({ ...body, cursor }, context); // non-mutating: spreads a fresh body each page + for (const m of env.data) yield m; + if (!env.has_more || !env.next_cursor) return; + cursor = env.next_cursor; + } +} +``` +Same shape and termination contract as `list()` — `SearchListEnvelope` already extends +`ListEnvelope` (`has_more` + `next_cursor`). The caller's `body` is never mutated (each +page spreads a fresh object), so a shared `body` reused across calls is safe. + +**KD-7 — Superseded resolver on `Memories`, taking the result + an id (C2a).** +`memories_superseded_by` only exists on an `IngestJobResult`, so the resolver takes +that map context explicitly rather than pretending old ids are globally queryable: +```ts +// returns the replacement Memory, or null if `oldId` was not superseded +async resolveSuperseded(result: IngestJobResult, oldId: string, ctx?: RequestContext): Promise { + const newId = result.memories_superseded_by?.[oldId]; + return newId ? this.get(newId, ctx) : null; +} +``` +Lives on `Memories` (has `http` + `get()`). A batch variant +(`resolveAllSuperseded(result)` → `Map`) is an optional convenience the +sprint may include or defer. + +**KD-8 — Reproducibility guard via an npm script in the gate (C1).** +Add `"check:types-sync": "npm run gen:types && git diff --exit-code -- src/generated/types.ts"` +and chain it into `prepublishOnly`. Document it for the local gate. This proves the +committed reference equals what the current spec produces; it does **not** assert +hand-authored conformance (that is ADR-002's optional follow-up). The `src/types.ts` +header (L1-2) is corrected to reference `spec/memory.json`. + +### Interface Design (summary of new/changed public surface) + +| Surface | Change | Breaking? | +|---|---|---| +| `MemoryClientOptions.authMode?: 'bearer'\|'x-api-key'` | new, default `'bearer'` | No | +| `RateLimitSnapshot` (exported type) | new | No | +| `MemoryError.rateLimit?: RateLimitSnapshot` | new optional field | No | +| `src/filter.ts` → `f`, `Clause`, `FieldOps` (exported) | new module | No | +| `RecallParams.include?: Array<'full_content'>` | new optional field | No | +| `Memories.searchAll(body, ctx)` | new method | No | +| `Memories.resolveSuperseded(result, oldId, ctx?)` | new method | No | +| `HttpClient.request()` return `+ rateLimit?` | additive field | No | + +All new exports are re-exported from `src/index.ts`. + +## Implementation Phases + +### Phase 1 — Spec reconciliation (spec-only, unblocks Phase 5 regen) + +**Goal:** `spec/memory.json` no longer contradicts the SDK on auth, the endpoint set, +or `metadata`. + +**Deliverables:** +- A2a: security schemes (L1820-1837) + `info.description` "Authentication" — **add** the + `Bearer ` form alongside the documented `Token`/`x-api-key` (do not replace). +- A4: annotate `PATCH /v1/memories/{memory_id}` (L514/L602) + `UpdateRequest` (L1522) as + removed server-side (observed 405, PR #6); corrections flow through ingest. Preserve + the entries with a removal/deprecation note. +- A5: annotate `metadata` references in `info.description` (Memory object, ingest + contract, filter-DSL table) as dropped server-side per `a7da7a9`. + +**Test strategy:** spec is data, not code — validation is `npx openapi-typescript +spec/memory.json` parsing clean (proven in Phase 5 regen) + a human diff review. +**Infrastructure:** none. **Documentation:** the spec *is* the doc. +**Dependencies:** none. +**Definition of done:** +- [ ] Spec documents `Bearer` alongside `Token`/`x-api-key`; no scheme removed. +- [ ] PATCH op + `UpdateRequest` annotated as removed (not deleted). +- [ ] `metadata` references annotated as dropped. +- [ ] `npx openapi-typescript spec/memory.json` parses without error. + +### Phase 2 — HTTP error + rate-limit hardening (A1, A3) + +**Goal:** the HTTP layer extracts codes from both error envelopes + 422, and surfaces +`RateLimit-*`. + +**Deliverables:** `parseErrorBody()` + `RateLimitSnapshot` in `src/errors.ts`; +`toError`/`request()` updated in `src/http.ts` (parse headers, populate `rateLimit` on +the return); `MemoryError.rateLimit?`; exports in `src/index.ts`. (No client-level +snapshot — see KD-2.) + +**Test strategy (TDD, mocked fetch) — write these fixtures first:** +- unit: legacy `{error:{…}}` → identical code/message/details as today (regression lock). +- unit: spec `{detail:{code,message}}` → code/message extracted. +- unit: 422 `{detail:[…]}` → `code:'validation_error'`, `details.validation_errors` + holds the array. +- unit: FastAPI string `{detail:"Not authenticated"}` → `message` = that string (the + B2 fix; array-check precedes object-check precedes string-check). +- unit: `RateLimit-*` headers parsed onto the `request()` return + a raised + `RateLimited.rateLimit`; absent headers → `undefined` fields, no throw. + +**Infrastructure:** none. **Documentation:** JSDoc on `RateLimitSnapshot`, +`MemoryError.rateLimit`, README error/rate-limit note. **Dependencies:** none (spec +Phase 1 not required for code). +**Definition of done:** +- [ ] All four error fixtures pass (legacy, detail-object, detail-array, detail-string); + existing error tests unchanged. +- [ ] Rate-limit headers reachable on the `request()` return and on raised errors. +- [ ] `typecheck → test → build` green. + +### Phase 3 — Opt-in `x-api-key` auth mode (A2b) + +**Goal:** callers can select `x-api-key`; `Bearer` stays default. + +**Deliverables:** `authMode` on `MemoryClientOptions`/`HttpClientConfig`/ +`defaultHttpConfig`; conditional header build in `request()`. + +**Test strategy (TDD):** unit asserting the default sends `Authorization: Bearer …` +(regression lock) and `authMode:'x-api-key'` sends `x-api-key: …` with no +`Authorization` header; `X-Org-Id` present in both. +**Infrastructure:** none. **Documentation:** README auth section + JSDoc. +**Dependencies:** Phase 2 (shares the `request()` header block — sequence to avoid churn). +**Definition of done:** +- [ ] Default header byte-identical to today. +- [ ] `x-api-key` mode verified; gate green. + +### Phase 4 — Search capabilities (B1, B2, B3) + +**Goal:** typed filter DSL, `include[]` on recall, and `searchAll()` exist and are typed. + +**Deliverables:** `src/filter.ts` (`f`, `Clause`, `FieldOps`); `RecallParams.include?` +threaded into per-pool search; `Memories.searchAll()`; exports in `src/index.ts`; +clarified `filters` deprecation JSDoc. + +**Test strategy (TDD, mocked HttpClient):** +- unit (B1): each operator + `f.field` multi-op + `and`/`or`/`not` + implicit-AND + (`f.all`) + `isNull` emits the exact wire JSON; a two-operator range via + `f.field('price',{$gt,$lt})` keeps **both** operators; `f.all` with a duplicate field + **throws** (the B1 fix); output assignable to `SearchRequest.filters`; an arbitrary + metadata key filters identically to an entity axis (the A5 reconciliation). +- unit (B2): `recall({…, include:['full_content']})` forwards `include` into every + per-pool search call; `full_content` typed-accessible on returned artifact rows; + `retrieve({…, include:['context_prompt']})` returns `extras.context_prompt`. +- unit (B3): a 3-page mocked search yields all rows in order and stops at + `has_more:false`; single-page stops immediately. + +**Infrastructure:** none. **Documentation:** README "Filtering" + "Pagination" +(searchAll) sections; examples. **Dependencies:** Phase 1 (A5 decision informs the B1 +doc note; B1 code is key-agnostic so not blocked). +**Definition of done:** +- [ ] DSL operator/combinator tests pass; `Filter` escape hatch retained. +- [ ] `include` round-trips through recall. +- [ ] `searchAll()` mirrors `list()` semantics; gate green. + +### Phase 5 — Type-surface finalization (C1, C2a, C2b/c) + +**Goal:** SoT references corrected, generated reference regenerated + guarded, +superseded chain resolvable, deferrals recorded. + +**Deliverables:** fix `src/types.ts:1-2` (→ `spec/memory.json`) and the roadmap +`wire-generated-types` note; regenerate `src/generated/types.ts`; +`check:types-sync` npm script chained into `prepublishOnly`; +`Memories.resolveSuperseded()` (+ optional batch); deferral JSDoc on `Memory.expanded` +and `PromptTemplate` referencing ADR-002. + +**Test strategy (TDD):** +- unit (C2a): superseded `oldId` resolves to the replacement `Memory` (mocked `get()`); + a non-superseded id returns `null`. +- guard (C1): `npm run check:types-sync` passes on a clean tree; a hand-edit to the + generated file (or a spec change without regen) fails it. + +**Infrastructure:** `check:types-sync` script (IaC for the local/prepublish gate). +**Documentation:** corrected header comment; deferral docstrings; README note that the +generated file is a reference. **Dependencies:** **Phase 1 must land first** (regen +reflects the corrected spec). +**Definition of done:** +- [ ] No `openapi/memory_v2.json` reference remains in `src/`/roadmap (`grep` clean; the ADR/design docs may still quote it as the defect they fixed). +- [ ] `gen:types` runs clean; the regenerated reference reflects the reconciled 1.0 spec — i.e. it includes step 2A's annotated (retained) `UpdateRequest`/PATCH op per ADR-001 A4 (annotate-not-delete), NOT hand-stripped (consistent with the Phase-1 DoD above). +- [ ] `check:types-sync` wired into `prepublishOnly` and passing. +- [ ] `resolveSuperseded` tests pass; deferrals documented; gate green. + +### Release + +Bump `package.json` `0.2.1 → 0.3.0` (minor; additive). Call out the new capabilities +(both-envelope errors, rate-limit surface, `x-api-key` opt-in, filter DSL, recall +`include`, `searchAll`, superseded resolver) in the PR/release notes per `RELEASING.md`. +*Not a code card per se — fold into the closeout/PR.* + +## Migration & Rollback + +**Migration:** pure addition. No method signature changes, no default behaviour change, +no removed exports. Consumers upgrade `0.2.x → 0.3.0` with zero code changes and opt +into new surface at will. + +**Rollback:** clean `git revert` per phase. Phases 2-5 are code-isolated; Phase 1 is +spec/doc-only. The only intra-repo ordering constraint is Phase 1 → Phase 5 regen. + +## Risks + +| Risk | Impact | Likelihood | Mitigation | +|------|--------|------------|------------| +| The live API actually emits only one error shape; the other branch is dead | Low (harmless dead branch) | Med | Branch is unit-tested both ways; ADR-001 Validation schedules a probe to prune later | +| `parseErrorBody` precedence regresses the current `{error:{…}}` path | High | Low | Legacy-first precedence + a regression-lock unit test on the exact current shape | +| Regenerating `src/generated/types.ts` produces churn unrelated to the spec edits | Med (noisy diff) | Med | Regenerate only in Phase 5 after spec edits; commit the regen in isolation; `check:types-sync` makes drift visible | +| Filter DSL diverges from the server's actual operator handling (can't probe) | Med | Low | DSL emits exactly the spec's documented operator JSON; `Filter` escape hatch remains for anything unmodeled | +| `x-api-key` mode untested against the live API (no creds) | Low (opt-in, off by default) | Med | Default unchanged; mode is opt-in; unit test asserts header shape, not server acceptance | +| Phase 3 and Phase 2 both edit the `request()` header block → merge churn | Low | Med | Sequence Phase 3 after Phase 2 | + +## Roadmap Connection + +Serves `m2/s5` end to end. Mapping: Phase 1 → `spec-contract-alignment` +(A2a/A4/A5) + the new metadata item; Phase 2 → `error-envelope-mismatch` + +`rate-limit-headers`; Phase 3 → `auth-header-form`; Phase 1+won't-do → `update-endpoint`; +Phase 4 → `typed-filter-dsl` + `search-include-mechanism` + `search-auto-pager`; +Phase 5 → `wire-generated-types` + `model-expanded-and-templates`. After execution, +each feature is set `done` or `won't-do` (PATCH; `expand`/server-`PromptTemplate`). + +## Open Questions + +1. **422 synthetic code name** — `'validation_error'` is proposed; confirm it doesn't + collide with a real server code. (Low stakes; switchable in one place.) +2. **`resolveSuperseded` shape** — ship `resolveAllSuperseded(result) → Map` as the primary (the common "what did this ingest supersede" case) with + `(result, oldId)` as sugar, or just the single-id form? (Sprint-architect's call.) +3. **`gen:types` determinism** — pin/record the `openapi-typescript` version so a dep + bump doesn't make `check:types-sync` fail spuriously. (Note in the C1 card.) + +--- + +## Revision History + +| Date | Author | Notes | +|------|--------|-------| +| 2026-06-12 | gitban M2 | Initial design — bridges ADR-001 + ADR-002 + additive search features into 5 phases (v0.3.0). | +| 2026-06-12 | gitban M2 | Design-review fixes: B1 single-field `f.field` + throwing `f.all` (no silent range clobber); B2 `detail:string` branch (FastAPI default); S1 dropped racy client rate-limit snapshot (per-response return + error field only); S2 scoped recall `include` to `full_content`; M1 non-mutating `searchAll` clarified. | diff --git a/docs/designs/m3-live-verified-contract-corrections.md b/docs/designs/m3-live-verified-contract-corrections.md new file mode 100644 index 0000000..07a1acd --- /dev/null +++ b/docs/designs/m3-live-verified-contract-corrections.md @@ -0,0 +1,617 @@ +# Design Doc: M3 — Live-verified contract corrections (verification & corrections) + +> **ADR**: [ADR-003](../adr/ADR-003-live-verified-supersession-of-adr-001-dispositions.md) (live-verified supersession of ADR-001's dispositions) | **Related**: [ADR-001](../adr/ADR-001-sdk-spec-reconciliation-policy.md), [ADR-002](../adr/ADR-002-type-surface-source-of-truth.md) | **Date**: 2026-06-15 | **Author**: gitban M3 | **Roadmap**: `m3/s6` + +## Overview + +This document is the implementation bridge for ADR-003 — applying the live-probed +contract facts (captured in gitban card **2syddu** against +`https://api.production.xtrace.ai` on 2026-06-15) to the SDK and `spec/memory.json`. +ADR-001 reconciled four divergences *blind* (no live credentials); the probe has +now run, fired ADR-001's own revisit trigger, and overturned three of those +dispositions. This design encodes the corrections. + +Four work areas, in decreasing evidentiary weight: + +- **A3 — a shipped bug.** `parseRateLimit` reads `RateLimit-*`; the live server + emits `x-ratelimit-*`, so `RateLimitSnapshot` has *never* populated in + production. Fix the parser to read `x-ratelimit-*` (keep `RateLimit-*` as a + fallback); redefine the `reset` field's semantics from delta-seconds to + epoch-seconds (doc-only — the field never populated, so no consumer depends on + the old reading); correct the spec's header section. +- **A4 — a premise reversal, now fully captured.** `PATCH /v1/memories/{id}` is + *live* (not 405 as ADR-001 inferred). Its contract is group-membership editing: + request `{add_group_ids?, remove_group_ids?}` (≥1 required; empty → `422 + empty_patch`), response `200` + the full updated `Memory`. Add + `memories.patch()`; correct the spec (un-annotate PATCH, drop the false + 405/removed claim, document the membership contract). The A4b "gated on a future + probe" condition in ADR-003 is **cleared** — card 2syddu's "A4b HAPPY-PATH + CAPTURED" section recorded the full success contract. +- **A1 — a spec + docs correction (code unchanged).** Every observed non-2xx + (401/404/422, n=1 each) used the legacy `{error:{type,code,message,request_id, + details?}}` shape; `parseErrorBody`'s legacy-first branch already handles it (and + already lifts `error.details`). Correct the spec's "Error envelope" section from + `{detail:{…}}` to `{error:{…}}`; document that live 422 field errors are carried + under `error.details.errors[]` as `{field,message,type}`. Add a regression + fixture proving the live 422 body parses correctly. The `{detail}`/`detail[]` + branches are *retained* (n=1 observations + the 429 envelope was never captured). +- **A2 — a docs note (no code, no spec change).** Both auth schemes share one + `(org_id, key_hash)` rate bucket; the apparent "10× quota" was an n=1 cold-start + artifact, retracted by a controlled re-probe. `Bearer` stays the default. + Document the shared bucket in the README / `authMode` JSDoc. + +The work is **entirely additive plus one bug fix plus spec/doc corrections** — no +shipping default changes (the auth-default flip ADR-003 considered was withdrawn). +Under `RELEASING.md`'s pre-1.0 policy this is a **minor** bump: **v0.3.0 → v0.4.0**. + +## Requirements + +The implementation is complete when: + +1. **`RateLimitSnapshot` populates against the live header family.** + `parseRateLimit` reads `x-ratelimit-limit/remaining/reset`, with `RateLimit-*` + retained as a fallback; a 200 carrying `x-ratelimit-*` yields a defined + snapshot. The bug (snapshot always `undefined` in production) is fixed. +2. **`reset` semantics are documented as epoch-seconds.** `RateLimitSnapshot.reset` + JSDoc states it is an absolute epoch-seconds timestamp (was delta-seconds), with + the doc-only redefinition rationale recorded (field never populated). +3. **`memories.patch()` edits group membership.** `memories.patch(id, { + add_group_ids?, remove_group_ids? }): Promise` exists, validates ≥1 + field client-side (throws before the call on an empty patch, mirroring the + server's `empty_patch`), PATCHes `/v1/memories/{id}`, and returns the full + updated `Memory`. +4. **The spec matches the live wire.** `spec/memory.json` documents the error + envelope as `{error:{…}}` (with 422 fields under `error.details.errors[]`), the + rate-limit headers as `x-ratelimit-*` (with `-reset` = epoch seconds), and the + live PATCH group-membership contract (the false 405/removed claim and the + self-contradiction removed). +5. **The auth shared-bucket fact is documented.** The README / `authMode` JSDoc + records that `Bearer` and `x-api-key` share one `(org_id, key_hash)` bucket and + that scheme choice carries no quota advantage. `Bearer` remains the default. +6. **`delete()`'s docstring no longer asserts a falsehood.** Its "the memory API + has no update endpoint" claim is corrected (PATCH exists for membership editing; + *content* corrections still flow through ingest/supersede). +7. **The reproducibility guard still passes.** After the spec edits, + `npm run check:types-sync` is green (`gen:types` regenerates + `src/generated/types.ts` from the corrected spec, committed in isolation). +8. **Tolerance is retained, not pruned.** The `{detail}`/`detail[]` branches and + the `RateLimit-*` fallback stay (n=1 observations + uncaptured 429), with + regression coverage for each. +9. **All existing tests pass unchanged**, and every new work item ships with + tests written first (TDD, mocked fetch only) and a `typecheck → test → + check:types-sync → build` that is green. + +## Current State + +- **`src/http.ts`** — `request()` returns `{ body, status, requestId, rateLimit }`. + `parseRateLimit` (L156-174) reads `RateLimit-Limit/Remaining/Reset` only — the + `x-` prefix is literal, so against the live `x-ratelimit-*` headers it **never + matches** and `rateLimit` is always `undefined` in production. It already drops + absent/empty/whitespace headers (`raw.trim() === ""` guard, L162) and applies + `Number.isFinite` (L164). `authMode` defaults to `'bearer'` (L192) — unchanged by + this work. +- **`src/errors.ts`** — `RateLimitSnapshot` (L8-15); `reset` JSDoc says + "seconds until the window resets" (delta semantics, L13). `parseErrorBody` + (L94-140) is legacy-first: branch 1 reads `{error:{…}}` and lifts + `error.details` (L108); branches 2-4 handle `detail` array/object/string. The + live 401/404/422 bodies all land in branch 1. +- **`src/memories.ts`** — `Memories` has `get()` (L219), `delete()` (L289), `search()`, + `recall()`, `list()`, the superseded resolvers. There is **no `patch()`**. + `delete()`'s docstring (L284-288) asserts *"the memory API has no update + endpoint"* — false per the live probe. +- **`src/types.ts`** — `Memory` (L91), `ApiErrorBody` (L383, the legacy `{error:{…}}` + shape). There is **no PATCH request type**. The header (L1-5) already references + `spec/memory.json` (ADR-002 fix landed in M2). +- **`src/groups.ts`** — uses `PATCH /v1/groups/{id}` already (L48-55); a useful + precedent for shaping `memories.patch()`. +- **`spec/memory.json`** — "Error envelope" section documents `{detail:{code, + message}}` and a FastAPI `detail:[{loc,msg,type}]` 422 (both **wrong** vs live); + "Response headers" documents `RateLimit-*` (**wrong** — live is `x-ratelimit-*`); + the PATCH op (L596-660) is annotated `deprecated: true` with a *"[REMOVED + server-side — returns 405]"* summary (L601) and a long "REMOVED SERVER-SIDE" + description (L602), referencing `UpdateRequest` (`{text,metadata}`, schema L1523). + Spec L113 simultaneously treats PATCH as a *live* API-key-only op — the + self-contradiction ADR-003 calls out. `ErrorEnvelope`/`ErrorDetail` schemas + (L815-834) model the wrong `{detail}` shape. The auth section already documents + `Bearer`/`Token`/`x-api-key` and `(org_id, key_hash)` bucketing (no spec change + needed for A2). +- **`package.json`** — `"version": "0.3.0"`; `gen:types` (L49), + `check:types-sync` (L50), and `prepublishOnly` (L52, which already chains + `typecheck → test → check:types-sync → build`). +- **Tests** — vitest, mocked fetch (`src/errors.test.ts`, `src/http.test.ts`). + `errors.test.ts` L170-227 already exercises `RateLimit-*` parsing, the + non-numeric drop, and the empty/whitespace drop — these become the **fallback** + regression suite. No CI; the only gate is the local pre-commit + `typecheck → test → build` (= `prepublishOnly`), run when code is staged. + +## Target State + +``` + MemoryClient(options) authMode default 'bearer' (UNCHANGED) + │ + ▼ + ┌───────────────────────────────────────┐ + │ HttpClient │ + request() returns │ parseRateLimit(headers): │ + {body,status,requestId, │ x-ratelimit-limit/remaining/reset │ ◀ A3 fix + rateLimit?} │ └ fallback: RateLimit-* (retained) │ + │ toError → parseErrorBody({error:{…}})│ (A1: unchanged) + └──────┬──────────────────┬─────────────┘ + │ │ + ┌──────────────▼──┐ ┌────────▼──────────────────┐ + │ errors.ts │ │ memories.ts │ + │ RateLimitSnapshot│ │ get / delete / search … │ + │ .reset = EPOCH │ ◀A3 │ patch(id,{add,remove}) ◀ │ A4b + │ parseErrorBody │ │ → Promise │ + │ (legacy-first) │ │ ≥1-field client guard │ + └──────────────────┘ └────────┬──────────────────┘ + │ PATCH /v1/memories/{id} + │ {add_group_ids?,remove_group_ids?} + ▼ 200 + full Memory + (empty body → 422 empty_patch) + +spec/memory.json ──(A1 error-envelope · A3 headers · A4a PATCH — ASSERTIVE edits)──▶ + gen:types ──▶ src/generated/types.ts ──▶ check:types-sync ✔ +README / authMode JSDoc ──(A2: shared (org_id,key_hash) bucket, no scheme advantage)──▶ +``` + +After all phases: `RateLimitSnapshot` actually populates in production; the SDK can +edit group membership via `memories.patch()`; the spec describes the error +envelope, rate-limit headers, and PATCH contract the live server presents; the +auth shared-bucket fact is documented; and the false "no update endpoint" docstring +is corrected. Every change is additive, a bug fix, or a spec/doc correction. + +## Design + +### Architecture + +This is a **medium** change confined to existing modules — no new module is +introduced. Four logical units map onto the existing surface: + +- **A3** lives entirely in `src/http.ts` (`parseRateLimit`) and `src/errors.ts` + (the `RateLimitSnapshot.reset` JSDoc). Pure header-parsing + doc change. +- **A4b** adds one method to `Memories` (`src/memories.ts`) and one request type + to `src/types.ts`. It reuses the existing `Memory` return type (the live response + is a full `Memory`, identical to `GET`) and the existing `request()` plumbing. +- **A1 / A4a** are `spec/memory.json` edits (assertive — we observed the shapes), + followed by a `gen:types` regen + `check:types-sync` verification, plus the + `delete()` docstring fix and a `parseErrorBody` live-422 regression fixture. +- **A2** is a README + `authMode` JSDoc note. No code, no spec. + +No data flow changes for existing methods. `parseRateLimit` is called on every +response already; only *which* header names it reads changes. `memories.patch()` +is a new leaf that uses the same `http.request()` path as every other method. + +### Key Design Decisions + +**KD-1 — `parseRateLimit`: x-prefixed primary, `RateLimit-*` fallback, per-field.** +The live server emits `x-ratelimit-limit/remaining/reset`. The fix reads the +x-prefixed name first and falls back to the un-prefixed name **per field**, so a +deployment that mixes families (or a future revision that reverts) still parses: + +```ts +const num = (name: string): number | undefined => { /* existing trim + isFinite guard */ }; +const pick = (xName: string, plainName: string): number | undefined => + num(xName) ?? num(plainName); +const limit = pick("x-ratelimit-limit", "RateLimit-Limit"); +const remaining = pick("x-ratelimit-remaining", "RateLimit-Remaining"); +const reset = pick("x-ratelimit-reset", "RateLimit-Reset"); +``` + +*Why per-field fallback, not "all-x-or-all-plain":* `Headers.get()` is +case-insensitive but the `x-` prefix is a literal part of the name, so the two +families are genuinely distinct lookups. A per-field `??` is the smallest change +that prefers the proven-live family while keeping ADR-003's retained tolerance +(ADR-003 A3: "keep `RateLimit-*` as a fallback"). The existing +empty/whitespace/`Number.isFinite` guards (the `num` helper) are preserved +verbatim — they are not coupled to the header name and their regression tests +(`errors.test.ts` L208-227) must keep passing unchanged. *Why x-prefixed wins on a +tie:* it is the measured-live family; `RateLimit-*` is the unobserved-but-tolerated +fallback, so the proven shape takes precedence. + +**KD-2 — `reset` is redefined as epoch-seconds (doc-only).** The live `-reset` is +an absolute epoch timestamp (e.g. `1781491860`), not delta-seconds. The +`RateLimitSnapshot.reset` JSDoc is rewritten to say so. *Why this is safe and +doc-only:* the field has **never populated in production** (the parser never +matched), so no consumer can depend on the old delta-seconds reading — this is a +deliberate redefinition of a value that was always `undefined`, not a silent +behavioural break. The SDK does **not** convert the value (it surfaces the raw +parsed number); any consumer-facing duration math is the caller's, and the JSDoc +warns explicitly not to treat it as a delta. ADR-003 A3 mandates this redefinition. + +**KD-3 — `memories.patch()` lives on `Memories`, validates ≥1 field client-side, +reuses `Memory` as the return.** The method sits alongside `get`/`delete` on the +`Memories` class (it has `http` and the `/v1/memories/{id}` path family). It mirrors +the server's `empty_patch` contract by throwing **before** the call when neither +`add_group_ids` nor `remove_group_ids` carries an entry: + +```ts +async patch( + id: string, + changes: MemoryPatchRequest, + context: RequestContext = {}, +): Promise { + const add = changes.add_group_ids ?? []; + const remove = changes.remove_group_ids ?? []; + if (add.length === 0 && remove.length === 0) { + throw new Error( + "memories.patch(): provide at least one of add_group_ids or remove_group_ids " + + "(the server rejects an empty patch with 422 empty_patch)", + ); + } + const body: MemoryPatchRequest = {}; + if (add.length > 0) body.add_group_ids = add; + if (remove.length > 0) body.remove_group_ids = remove; + const { body: res } = await this.http.request( + "PATCH", + `/v1/memories/${encodeURIComponent(id)}`, + { body, signal: context.signal, requestId: context.requestId }, + ); + return res; +} +``` + +*Why a client-side guard instead of letting the server 422:* the server's +`empty_patch` is a real round-trip a caller can avoid; failing fast with an +actionable message is cheaper and clearer, and it mirrors the existing +`recall()` "loud throw on an empty pool" convention (`memories.ts` L420-444). The +guard treats an empty *array* the same as an absent field (both are empty +patches), and only forwards the non-empty arrays on the wire so the request stays +minimal. *Why reuse `Memory` as the return:* the live happy-path capture (card +2syddu) confirmed the response is `200` + the **full** `Memory` object, identical +in shape to `GET /v1/memories/{id}`, with `group_ids` reflecting the edit — so the +existing `Memory` discriminated union is the exact return type; no new response +type is warranted. *Why a new `MemoryPatchRequest` type and not reusing the dead +`UpdateRequest`:* the old `{text,metadata}` `UpdateRequest` is genuinely gone +(content corrections flow through ingest/supersede); the live contract is a +disjoint `{add_group_ids?, remove_group_ids?}`. A fresh hand-authored type on the +ADR-002 canonical surface keeps the dead and live contracts from being conflated. + +**KD-4 — `parseErrorBody` is unchanged; A1 is spec + docs + one regression test.** +The live 401/404/422 bodies are the legacy `{error:{…}}` shape, which branch 1 +already handles — including lifting `error.details` (so the 422's +`error.details.errors[]` is preserved verbatim under `MemoryError.details`). No +code change. *Why retain the `{detail}`/`detail[]` branches rather than prune:* +ADR-003 (S1) keeps them — the observations are n=1 per class and the **429 envelope +was never captured**, so pruning the only tolerant fallback on three single +observations is a larger bet than keeping a few fixture-tested lines. We add one +new fixture asserting the *exact live 422 body* parses correctly (proving branch 1 +lifts `error.details.errors[]`), and we keep the existing `{detail}`/`detail[]` +tests as the retained-tolerance suite. + +**KD-5 — spec edits are assertive (A1/A3/A4a), not annotative.** ADR-003 reverses +ADR-001's additive-only spec posture *for these sections specifically*, because we +are no longer asserting an un-probed claim — we observed the shapes. So the spec's +"Error envelope", "Response headers", and PATCH sections are **rewritten to the +measured wire**, not merely annotated. The dead `UpdateRequest` schema and the +PATCH `{text,metadata}` history are removed from the live contract description (a +brief historical note may remain), and the PATCH op is re-specced to the live +group-membership contract. *Why assertive is correct here:* it is precisely the +condition ADR-001 required before asserting (measured, not guessed), and leaving +the false 405/removed claim would steer consumers away from a working endpoint +(ADR-003 Rationale #2). The regen must stay clean: `gen:types` must still parse the +edited spec, and `check:types-sync` must pass after the regen is committed. + +**KD-6 — A2 is a documentation note only.** The spec already documents all three +auth schemes and `(org_id, key_hash)` bucketing, so A2 touches neither code nor +spec. A README / `authMode` JSDoc note records that both schemes hit the same +shared bucket and scheme choice carries no quota advantage. *Why doc-only and why +`Bearer` stays default:* the controlled re-probe (n=8, order reversed) showed the +apparent 10× gap was a first-call/cold-start artifact tracking call *position*, not +scheme, and `remaining` decremented continuously across both schemes — confirming +the shared bucket. With no quota to gain, flipping the default would be a breaking +change for zero benefit (ADR-003 A2; the withdrawn Alternative 3). + +### Interface Design (new/changed public surface) + +| Surface | Change | Breaking? | +|---|---|---| +| `Memories.patch(id, changes, ctx?)` → `Promise` | new method (A4b) | No (additive) | +| `MemoryPatchRequest` (exported type) | new — `{ add_group_ids?: string[]; remove_group_ids?: string[] }` | No (additive) | +| `RateLimitSnapshot.reset` JSDoc | redefined delta-seconds → epoch-seconds | No (doc-only; field never populated) | +| `parseRateLimit` header names | `RateLimit-*` → `x-ratelimit-*` (fallback retained) | No (bug fix; surface unchanged) | +| `Memories.delete()` docstring | corrected (PATCH exists for membership) | No (doc-only) | +| `spec/memory.json` error/headers/PATCH sections | corrected to live wire | No (spec/doc) | +| README / `authMode` JSDoc | shared-bucket note | No (doc-only) | + +New `MemoryPatchRequest` is re-exported from `src/index.ts` alongside the other +request types. + +```ts +// src/types.ts — hand-authored canonical surface (ADR-002) +/** + * Body for `PATCH /v1/memories/{id}` — group-membership editing. At least one + * of `add_group_ids` / `remove_group_ids` must carry an entry (an empty patch + * is rejected client-side, mirroring the server's `422 empty_patch`). This is + * the *only* mutate-in-place op on a memory; content corrections flow through + * ingest/supersede, not here. The old `{text,metadata}` UpdateRequest is gone. + */ +export interface MemoryPatchRequest { + /** Group ids to add to the memory's membership. */ + add_group_ids?: string[]; + /** Group ids to remove from the memory's membership. */ + remove_group_ids?: string[]; +} +``` + +## Implementation Phases + +The phases are ordered so that the spec edits (Phase 3) land *before* the type +regen they feed, mirroring the M2 design doc's Phase-1-before-regen constraint. +Phases 1 and 2 are pure code and independent; Phase 4 is docs-only. + +### Phase 1 — A3: fix `parseRateLimit` + redefine `reset` (the shipped bug) + +**Goal:** `RateLimitSnapshot` populates against the live `x-ratelimit-*` headers, +with `RateLimit-*` retained as a fallback, and `reset` documented as epoch-seconds. + +**Deliverables:** +- `src/http.ts` `parseRateLimit`: per-field `x-ratelimit-*` primary, `RateLimit-*` + fallback (KD-1); existing trim/`Number.isFinite` guards preserved verbatim. +- `src/errors.ts` `RateLimitSnapshot.reset` JSDoc → epoch-seconds, with the + doc-only-redefinition rationale (KD-2). +- New + adjusted unit tests in `src/errors.test.ts` (or `src/http.test.ts`). + +**Test strategy (TDD, mocked fetch) — write these first:** +- **Given** a 200 with headers `x-ratelimit-limit:30, x-ratelimit-remaining:23, + x-ratelimit-reset:1781491860` **When** `request()` returns **Then** + `rateLimit` is `{ limit:30, remaining:23, reset:1781491860 }` (the live capture; + `reset` surfaced as the raw epoch number, not converted). *(new — the A3 fix)* +- **Given** a 200 with only `RateLimit-Limit:100, RateLimit-Remaining:42, + RateLimit-Reset:30` (no x-prefixed) **When** `request()` returns **Then** + `rateLimit` is `{ limit:100, remaining:42, reset:30 }`. *(retained fallback — + this is the existing `errors.test.ts` L170-178 test, kept green.)* +- **Given** a 200 with **both** families present **When** `request()` returns + **Then** the `x-ratelimit-*` values win (x-prefixed precedence). *(new)* +- **Given** a 200 with `x-ratelimit-remaining:""` and `x-ratelimit-reset:" "` + **When** parsed **Then** those fields are dropped (not coerced to 0). *(the + existing empty/whitespace drop test, L219-227, re-pointed at x-prefixed names — + proves the guard survived the rename.)* +- **Given** `x-ratelimit-limit:"not-a-number"` **When** parsed **Then** that field + is dropped (`Number.isFinite` guard). *(L208-216, re-pointed.)* +- **Given** no rate-limit headers of either family **When** `request()` returns + **Then** `rateLimit` is `undefined`, nothing throws. *(L202-206, kept.)* + +**Infrastructure:** none. **Documentation:** `RateLimitSnapshot.reset` JSDoc (DaC, +part of this phase's DoD). +**Dependencies:** none. +**Definition of done:** +- [ ] `parseRateLimit` reads `x-ratelimit-*` first, `RateLimit-*` as fallback. +- [ ] Live-capture fixture (`x-ratelimit-*`, epoch `reset`) parses to a defined snapshot. +- [ ] `RateLimit-*`-only fallback test still passes (un-prefixed family still parsed). +- [ ] Both-families test confirms x-prefixed precedence. +- [ ] Empty/whitespace and non-numeric drop tests pass against x-prefixed names. +- [ ] `reset` JSDoc states epoch-seconds with the redefinition rationale. +- [ ] `typecheck → test → build` green. + +### Phase 2 — A4b: `memories.patch()` + `MemoryPatchRequest` + +**Goal:** the SDK can edit group membership via `PATCH /v1/memories/{id}`, returning +the full updated `Memory`, with an empty patch rejected client-side. + +**Deliverables:** +- `src/types.ts`: `MemoryPatchRequest` interface (KD-3 / Interface Design). +- `src/memories.ts`: `Memories.patch(id, changes, ctx?)` (KD-3). +- `src/memories.ts`: corrected `delete()` docstring (Requirement 6) — remove the + "the memory API has no update endpoint" assertion; note PATCH exists for + membership editing while *content* corrections still flow through + ingest/supersede. +- `src/index.ts`: export `MemoryPatchRequest`. +- Unit tests in `src/memories.test.ts` (new file or existing memories test suite), + mocked `HttpClient`. + +**Test strategy (TDD, mocked fetch) — write these first, citing card 2syddu's +captured contract:** +- **Given** `patch("mem_1", { add_group_ids: ["grp_x"] })` **When** called against + a mocked `request()` that returns `200` + a `Memory` with + `group_ids:["grp_x"]` **Then** the call issues `PATCH /v1/memories/mem_1` with + body `{ add_group_ids:["grp_x"] }` (no `remove_group_ids` key) and resolves to + that `Memory`. +- **Given** `patch("mem_1", { remove_group_ids: ["grp_x"] })` against a mock + returning a `Memory` with `group_ids:[]` **Then** body is + `{ remove_group_ids:["grp_x"] }` and the resolved memory's `group_ids` is `[]`. +- **Given** `patch("mem_1", { add_group_ids:["a"], remove_group_ids:["b"] })` + **Then** both keys are sent on the wire. +- **Given** `patch("mem_1", {})` (or `{ add_group_ids: [], remove_group_ids: [] }`) + **When** called **Then** it **throws** before any `request()` call (empty-patch + client guard), with a message naming `add_group_ids`/`remove_group_ids` and the + server's `empty_patch` (mirrors the live `422 empty_patch`). Assert the mock + `request()` was **not** invoked. +- **Given** the live `422 empty_patch` body + `{"error":{"type":"invalid_request_error","code":"empty_patch",...}}` returned by + a mocked `request()` **When** `patch()` *did* send a body the server rejects (a + defensive path) **Then** the raised `Unprocessable` carries `code:"empty_patch"`. + *(Documents the server-side contract even though the client guard normally + prevents reaching it.)* +- **Given** `context.requestId` / `context.signal` **Then** they thread into + `request()` (parity with `get`/`delete`). + +**Infrastructure:** none. **Documentation:** `MemoryPatchRequest` JSDoc, `patch()` +JSDoc (membership-editing usage + the empty-patch contract), corrected `delete()` +docstring. +**Dependencies:** none (independent of Phase 1; uses the existing `request()` path). +**Definition of done:** +- [ ] `patch()` sends only the non-empty arrays and returns the full `Memory`. +- [ ] Empty patch throws client-side; `request()` is not called. +- [ ] `MemoryPatchRequest` exported from `src/index.ts`. +- [ ] `delete()` docstring no longer claims "no update endpoint." +- [ ] `typecheck → test → build` green. + +### Phase 3 — A1/A4a spec corrections + regen + the live-422 regression fixture + +**Goal:** `spec/memory.json` matches the live wire on the error envelope, rate-limit +headers, and PATCH; the generated reference is regenerated and the guard passes; the +live 422 body is locked as a `parseErrorBody` regression fixture. + +**Deliverables (spec — assertive edits, KD-5):** +- **Error envelope section** → `{error:{type,code,message,request_id,details?}}`; + document that 422 field errors are carried under `error.details.errors[]` as + `{field,message,type}` (not FastAPI `{loc,msg,type}`). Update the + `ErrorEnvelope`/`ErrorDetail` component schemas (spec L815-834) and the per-status + `detail.code = …` response descriptions to the `{error:{…}}` shape / `error.code`. +- **Response headers section** → `x-ratelimit-limit/remaining/reset`; document + `-reset` = absolute epoch seconds; update the per-status "`Retry-After` and + `RateLimit-*`" mentions to `x-ratelimit-*`. +- **PATCH `/v1/memories/{memory_id}` op (A4a, L596-660):** drop `deprecated: true` + and the "[REMOVED server-side — returns 405]" summary/description; document the + live **group-membership** contract — request `{add_group_ids?, remove_group_ids?}` + (≥1 required), `200` + `Memory` response, `422 empty_patch` on an empty body; + replace the `UpdateRequest` `$ref` with the group-membership request schema; + resolve the self-contradiction (the "removed" note vs spec L113's live API-key-only + treatment). Remove/retire the dead `UpdateRequest` schema (L1523) from the live + contract (a one-line historical note that `{text,metadata}` was removed is fine). + +**Deliverables (code — the A1 regression fixture):** +- `src/errors.test.ts`: a new fixture asserting the **exact live 422 body** + `{"error":{"type":"invalid_request_error","code":"invalid_request","message": + "Request failed validation","request_id":"…","details":{"errors":[{"field": + "query","message":"Field required","type":"missing"}]}}}` parses (via the live + `request()` path) to a `MemoryError` with `code:"invalid_request"` and + `details.errors[0] === {field:"query",message:"Field required",type:"missing"}` + (proving branch 1 lifts `error.details` verbatim). Plus retained tests confirming + the live 401 (`code:"http_401"`) and 404 (`code:"memory_not_found"`) bodies parse. +- Keep the existing `{detail}`/`detail[]` branch tests as the retained-tolerance + suite (ADR-003 S1 — not pruned). + +**Deliverables (regen):** +- `npm run gen:types` → regenerate `src/generated/types.ts` from the corrected spec; + commit the regen **in isolation** (one commit) so the diff is reviewable. +- `npm run check:types-sync` green. + +**Test strategy (TDD, mocked fetch):** +- **Given** the live 422 `{error:{…,details:{errors:[…]}}}` body **When** + `parseErrorBody` runs (via `request()`) **Then** `code` is `"invalid_request"` + and `details.errors[0]` is the captured field error verbatim. *(A1 regression + lock — the headline new test.)* +- **Given** the live 401 / 404 bodies **Then** `code` is `"http_401"` / + `"memory_not_found"` and the message survives. +- **spec validation:** `npx openapi-typescript spec/memory.json` parses clean + (proven by `gen:types`), and a human diff review confirms the three sections. +- **guard:** `check:types-sync` passes on the committed regen; a spec edit without + regen fails it (the guard's purpose). + +**Infrastructure:** none (the `check:types-sync` script already exists). +**Documentation:** the spec *is* the doc for A1/A4a. +**Dependencies:** the spec edits must land before the regen (intra-phase order). +This phase is independent of Phases 1 and 2 (different files), though it should be +sequenced last so the regen captures a settled spec. +**Definition of done:** +- [ ] Error envelope section + `ErrorEnvelope`/`ErrorDetail` schemas describe + `{error:{…}}`; 422 `error.details.errors[]` documented. +- [ ] Response-headers section + per-status mentions use `x-ratelimit-*`; `-reset` + documented as epoch seconds. +- [ ] PATCH op un-annotated; group-membership contract (request/200 Memory/422 + empty_patch) documented; false 405/removed claim and self-contradiction gone; + dead `UpdateRequest` retired from the live contract. +- [ ] `npx openapi-typescript spec/memory.json` parses without error. +- [ ] `src/generated/types.ts` regenerated and committed in isolation; + `check:types-sync` green. +- [ ] Live-422 (`error.details.errors[]`), 401, 404 regression fixtures pass; + `{detail}`/`detail[]` tolerance tests retained and green. + +### Phase 4 — A2 docs note + README rate-limit/PATCH docs + +**Goal:** the shared-bucket fact and the new capabilities are documented for +consumers. + +**Deliverables:** +- `src/http.ts` `AuthMode` JSDoc (L4-10): add a note that both schemes share one + `(org_id, key_hash)` rate bucket and scheme choice carries no quota advantage — + `Bearer` stays the default for that reason (no quota incentive to flip). +- README: an auth note (shared bucket / no scheme advantage), a rate-limit note + (`x-ratelimit-*`, `reset` is epoch-seconds — don't treat it as a duration), and a + short `memories.patch()` usage example (group-membership editing). +- `CHANGELOG` entry for `0.4.0` (planned): A3 bug fix, `memories.patch()` + + `MemoryPatchRequest`, spec corrections (error envelope, rate-limit headers, + PATCH), `reset` epoch redefinition, A2 shared-bucket doc. + +**Test strategy:** docs are prose, not code — validation is a human review that the +note matches ADR-003 A2 and the captured re-probe (n=8). No new tests. +**Infrastructure:** none. **Documentation:** this phase *is* the DaC deliverable. +**Dependencies:** Phases 1-3 (the docs describe their behaviour). +**Definition of done:** +- [ ] `authMode` JSDoc records the shared bucket / no scheme advantage. +- [ ] README documents the shared bucket, `x-ratelimit-*`/epoch `reset`, and + `memories.patch()`. +- [ ] `CHANGELOG` `0.4.0` entry drafted. + +### Release + +Bump `package.json` `0.3.0 → 0.4.0` (minor; additive + bug fix per `RELEASING.md`). +Call out in the PR/release notes: the A3 rate-limit bug fix (snapshot now +populates; `reset` is epoch-seconds), the new `memories.patch()` group-membership +method, and the spec corrections (error envelope `{error:{…}}`, rate-limit headers +`x-ratelimit-*`, live PATCH contract). **No breaking change** — the auth default is +unchanged. *Not a code card per se — fold into the closeout/PR.* + +## Migration & Rollback + +**Migration:** additive plus one bug fix. No method signature changes, no removed +exports, no default behaviour change. Consumers upgrade `0.3.x → 0.4.0` with zero +code changes. The one behaviour *change* is the A3 fix: `RateLimitSnapshot` now +populates where it was always `undefined` — strictly more information, never a +regression (a consumer that ignored the always-`undefined` snapshot is unaffected; +one that read it now gets real data). The `reset` redefinition is doc-only because +the field never populated. + +**Rollback:** clean `git revert` per phase. Phases 1, 2, 4 are code/doc-isolated; +Phase 3's spec edit + regen are one logical unit (revert the spec and the +regenerated `src/generated/types.ts` together). The only intra-repo ordering +constraint is Phase 3's spec-before-regen. + +## Risks + +| Risk | Impact | Likelihood | Mitigation | +|------|--------|------------|------------| +| A future deployment emits `RateLimit-*` (un-prefixed) again | Low (snapshot still parses) | Low | `RateLimit-*` retained as a per-field fallback; both-family test locks precedence | +| Pruning the `{detail}` branches would break against the uncaptured 429 | Med | Low | ADR-003 S1 mandates retaining them; tolerance tests kept, not pruned | +| Spec regen produces churn unrelated to the three section edits | Med (noisy diff) | Med | Regen only after the spec edits settle; commit the regen in isolation; `check:types-sync` makes drift visible | +| `gen:types` chokes on the re-specced PATCH op (malformed schema) | Med | Low | `npx openapi-typescript` run as part of Phase 3 DoD before commit; the new request schema mirrors `GroupCreateRequest`'s shape | +| `memories.patch()` return shape differs from the captured `Memory` on some path | Low | Low | Card 2syddu captured the live `200 + full Memory` happy-path (org mutated + cleaned up); return type reuses the existing `Memory` union | +| Client-side empty-patch guard diverges from server semantics | Low | Low | Guard mirrors the captured `422 empty_patch` ("≥1 of add/remove"); a defensive test asserts the server body still maps to `Unprocessable` | +| The `reset` epoch value is mistaken for a delta by a consumer | Low | Med | JSDoc + README explicitly state epoch-seconds and warn against duration math; the SDK surfaces the raw value, does no conversion | + +## Roadmap Connection + +Serves roadmap node `m3/s6` (verification & corrections) end to end. Mapping: +Phase 1 → the A3 rate-limit-header bug fix; Phase 2 → the A4b `memories.patch()` +capability; Phase 3 → the A1 error-envelope + A4a PATCH spec corrections (+ regen +guard); Phase 4 → the A2 shared-bucket documentation. After execution, set the +relevant `m3/s6` features `done`. This design doc should be linked at the feature +level under `m3/s6` via `upsert_roadmap` `docs_ref` (ADR-003 is the parent node's +`docs_ref`). + +## Open Questions + +1. **Whether to retire `UpdateRequest` from the spec entirely or keep a one-line + historical stub.** ADR-003 A4a says drop the false annotation and document the + live contract; the dead `{text,metadata}` schema can be deleted or kept as a + one-line "removed" note. Low stakes — sprint-architect's call. (Deleting it is + cleaner; keeping a note aids upstream diff legibility.) +2. **`check:types-sync` determinism across `openapi-typescript` versions.** A dep + bump could make the regen diff spuriously (carried over from the M2 design's + Open Question 3). Pin/record the generator version if the guard flaps. +3. **Pruning the `{detail}`/`detail[]` dead branches (deferred, maintainer call).** + ADR-003 retains them pending a fuller capture (including a real 429). Out of + scope here; a follow-up once the 429 envelope is captured. + +## Out of Scope / Follow-ups + +- **Group-create `prompt` divergence (flag only).** Card 2syddu's A4b capture noted + live `POST /v1/groups` **requires** a `prompt` field (422 without it). `src/groups.ts` + `create()` sends `GroupCreateRequest` (`{name, prompt}`), so the SDK already sends + it — but this should be verified and, if the spec/types ever make `prompt` + optional, tightened. Flagged for a follow-up card, not part of this sprint. +- **Pruning the dead `{detail}`/`detail[]` `parseErrorBody` branches** — deferred + (maintainer call) pending a fuller capture, per ADR-003 S1. +- **Capturing a real 429 envelope.** The read-only probe never forced a 429; the + envelope is extrapolated, not measured. A future probe with a deliberately + exhausted bucket should capture it; until then the tolerant fallbacks stay. + +--- + +## Revision History + +| Date | Author | Notes | +|------|--------|-------| +| 2026-06-15 | gitban M3 | Initial design — implements ADR-003 (A1/A2/A3/A4) from card 2syddu's live captures into 4 phases (v0.4.0). A4b ungated: card 2syddu's "A4b HAPPY-PATH CAPTURED" recorded the full `200 + Memory` contract, clearing ADR-003's probe gate. | diff --git a/spec/memory.json b/spec/memory.json index a2ef332..e8fc56f 100644 --- a/spec/memory.json +++ b/spec/memory.json @@ -2,7 +2,7 @@ "openapi": "3.1.0", "info": { "title": "VecDB Memory API", - "description": "Unified memory API at `/v1/memories/*`. Mode-free reads (list, search,\nget-by-id), async-by-default ingest (`POST /v1/memories` \u2192 202 + job_id,\nor `?wait=true` for inline 200 within 30s), and polling at\n`GET /v1/memories/jobs/{job_id}`.\n\n## Authentication\n\nSend **both**:\n\n- `Authorization: Token ` *or* `x-api-key: `\n- `X-Org-Id: `\n\nTenancy is keyed by `(org_id, key_hash)`. Every row is scoped to the\ncalling org; cross-org reads are not possible even with a guessed id.\n\n## Ingest contract (`POST /v1/memories`)\n\n```\n{\n \"messages\": [...], // REQUIRED \u2014 chat-style turns\n \"user_id\": \"alice\", // REQUIRED \u2014 per-user scope\n \"conv_id\": \"conv-1\", // REQUIRED \u2014 conversation anchor\n \"agent_id\": \"...\", // optional\n \"app_id\": \"...\", // optional\n \"metadata\": {...}, // optional, free-form customer keys\n \"extract_artifacts\": false // opt-in to artifact-extraction stage\n}\n```\n\n- **Required entities:** `user_id` and `conv_id`. `user_id` keys the\n per-user namespace (facts accumulate across that user's\n conversations); `conv_id` anchors each extracted memory to a session\n for replay, export, and bulk retract.\n- Facts are always extracted from messages. Episodes are always\n extracted (`conv_id` is guaranteed). Artifacts are extracted only\n when `extract_artifacts: true` is set.\n- The server picks live vs batch extraction by message count; no\n client-facing pipeline hint exists.\n- Returns `202 Accepted` + `IngestJobResponse` with `status: \"pending\"`\n by default. With `?wait=true`, the server holds the connection up to\n ~30s and returns `200 OK` + terminal `IngestJobResponse` if\n extraction completes within the window; otherwise falls back to 202.\n\n## Memory object\n\n```\n{\n \"id\": \"\",\n \"object\": \"memory\",\n \"type\": \"fact\" | \"artifact\" | \"episode\",\n \"text\": \"short readable preview\",\n \"user_id\": \"...\", \"agent_id\": \"...\", \"conv_id\": \"...\", \"app_id\": \"...\",\n \"metadata\": {...},\n \"categories\": [...],\n \"score\": 0.83, // present on search responses only\n \"created_at\": \"2026-05-15T18:42:11Z\",\n \"updated_at\": \"2026-05-15T18:42:11Z\",\n \"details\": {...} // type-specific; see FactDetails /\n // ArtifactDetails / EpisodeDetails schemas\n}\n```\n\n`type` is the discriminator. `details` carries the per-type extension\nfields; the three variants are in the components section.\n\n## Pagination\n\nCursor-based on list and search. Cursors are opaque, tenant-scoped\nstrings. `has_more` plus `next_cursor` drive the next page. Default\nordering is `created_at_desc`; pass `order=created_at_asc` for the\nreverse. `limit` is 1\u2013100 (default 50 on list, 20 on search).\n\n## Filter DSL\n\n`GET /v1/memories` accepts flat equality on the four entity fields\n(`user_id`, `agent_id`, `conv_id`, `app_id`) plus `type`.\n\n`POST /v1/memories/search` accepts the full DSL in the `filters`\nobject:\n\n| Operator | Meaning |\n| ---------- | ------------------------------------------------ |\n| bare value | exact match (e.g. `{\"user_id\": \"alice\"}`) |\n| `null` | field is unset (absent or null) |\n| `$eq` | explicit equality (same as bare value) |\n| `$ne` | not equal AND field is set (SQL semantics) |\n| `$in` | one-of (`{\"agent_id\": {\"$in\": [\"bot-a\",\"bot-b\"]}}`) |\n| `$nin` | none-of (MongoDB semantics \u2014 null/absent passes) |\n| `$exists` | `true` = field set, `false` = field unset |\n| `$gt` / `$gte` / `$lt` / `$lte` | range comparators |\n| `$between` | `[lo, hi]` inclusive range |\n| `AND` | `{\"AND\": [, , ...]}` \u2014 intersection |\n| `OR` | `{\"OR\": [, ...]}` \u2014 union |\n| `NOT` | `{\"NOT\": }` \u2014 negation |\n\nTop-level keys are implicit-AND'd. Unknown operators are silently\nignored. Filters apply to any indexed payload field, including\ncustomer metadata keys (those land as their own indexed payload keys).\n\n## Error envelope\n\nAll non-2xx responses use a single body shape:\n\n```\n{\n \"detail\": {\n \"code\": \"\",\n \"message\": \"\"\n }\n}\n```\n\nSwitch on `detail.code` rather than parsing the message. Each endpoint\ndeclares the codes it can return.\n\nPydantic field-validation failures (422) use the FastAPI default\nshape \u2014 `detail` is an array of `{loc, msg, type}` entries listing\neach invalid field.\n\n## Response headers\n\n- `X-Request-Id` \u2014 echo of the inbound request id, or a server-\n generated UUID. Use this when filing support requests; operator\n logs are keyed on it.\n- `RateLimit-Limit`, `RateLimit-Remaining`, `RateLimit-Reset` \u2014\n current bucket state for this `(org_id, key_hash)` pair.\n- `Retry-After` \u2014 present on 429 responses, seconds until the next\n permitted request.\n", + "description": "Unified memory API at `/v1/memories/*`. Mode-free reads (list, search,\nget-by-id), async-by-default ingest (`POST /v1/memories` \u2192 202 + job_id,\nor `?wait=true` for inline 200 within 30s), and polling at\n`GET /v1/memories/jobs/{job_id}`.\n\n## Authentication\n\nSend **both**:\n\n- `Authorization: Bearer ` *or* `Authorization: Token ` *or* `x-api-key: `\n- `X-Org-Id: `\n\nThe `@xtraceai/memory` SDK sends `Authorization: Bearer ` (its\nshipping default through v0.2.1; accepted by the live API). The `Token`\nand `x-api-key` forms remain valid alternatives. The `Bearer` form was\nadded to this spec additively per ADR-001 (A2a); nothing was removed.\n\nTenancy is keyed by `(org_id, key_hash)`. Every row is scoped to the\ncalling org; cross-org reads are not possible even with a guessed id.\n\n## Ingest contract (`POST /v1/memories`)\n\n```\n{\n \"messages\": [...], // REQUIRED \u2014 chat-style turns\n \"user_id\": \"alice\", // REQUIRED \u2014 per-user scope\n \"conv_id\": \"conv-1\", // REQUIRED \u2014 conversation anchor\n \"agent_id\": \"...\", // optional\n \"app_id\": \"...\", // optional\n \"metadata\": {...}, // DROPPED server-side (see note below)\n \"extract_artifacts\": false // opt-in to artifact-extraction stage\n}\n```\n\n> **`metadata` dropped server-side (ADR-001 A5, commit `a7da7a9`).** The\n> deployed API no longer accepts a `metadata` field on ingest, and the\n> `@xtraceai/memory` SDK dropped it from `IngestRequest`. The key is shown\n> above for diff legibility only; sending it has no effect. This annotation\n> is additive — the field reference is retained, not deleted.\n\n- **Required entities:** `user_id` and `conv_id`. `user_id` keys the\n per-user namespace (facts accumulate across that user's\n conversations); `conv_id` anchors each extracted memory to a session\n for replay, export, and bulk retract.\n- Facts are always extracted from messages. Episodes are always\n extracted (`conv_id` is guaranteed). Artifacts are extracted only\n when `extract_artifacts: true` is set.\n- The server picks live vs batch extraction by message count; no\n client-facing pipeline hint exists.\n- Returns `202 Accepted` + `IngestJobResponse` with `status: \"pending\"`\n by default. With `?wait=true`, the server holds the connection up to\n ~30s and returns `200 OK` + terminal `IngestJobResponse` if\n extraction completes within the window; otherwise falls back to 202.\n\n## Memory object\n\n```\n{\n \"id\": \"\",\n \"object\": \"memory\",\n \"type\": \"fact\" | \"artifact\" | \"episode\",\n \"text\": \"short readable preview\",\n \"user_id\": \"...\", \"agent_id\": \"...\", \"conv_id\": \"...\", \"app_id\": \"...\",\n \"metadata\": {...}, // DROPPED server-side (see note below)\n \"categories\": [...],\n \"score\": 0.83, // present on search responses only\n \"created_at\": \"2026-05-15T18:42:11Z\",\n \"updated_at\": \"2026-05-15T18:42:11Z\",\n \"details\": {...} // type-specific; see FactDetails /\n // ArtifactDetails / EpisodeDetails schemas\n}\n```\n\n> **`metadata` dropped server-side (ADR-001 A5, commit `a7da7a9`).** The\n> deployed API no longer returns a `metadata` field on the Memory object,\n> and the `@xtraceai/memory` SDK dropped it from its `Memory` type. The key\n> is shown above for diff legibility only. This annotation is additive — the\n> field reference is retained, not deleted.\n\n`type` is the discriminator. `details` carries the per-type extension\nfields; the three variants are in the components section.\n\n## Pagination\n\nCursor-based on list and search. Cursors are opaque, tenant-scoped\nstrings. `has_more` plus `next_cursor` drive the next page. Default\nordering is `created_at_desc`; pass `order=created_at_asc` for the\nreverse. `limit` is 1\u2013100 (default 50 on list, 20 on search).\n\n## Filter DSL\n\n`GET /v1/memories` accepts flat equality on the four entity fields\n(`user_id`, `agent_id`, `conv_id`, `app_id`) plus `type`.\n\n`POST /v1/memories/search` accepts the full DSL in the `filters`\nobject:\n\n| Operator | Meaning |\n| ---------- | ------------------------------------------------ |\n| bare value | exact match (e.g. `{\"user_id\": \"alice\"}`) |\n| `null` | field is unset (absent or null) |\n| `$eq` | explicit equality (same as bare value) |\n| `$ne` | not equal AND field is set (SQL semantics) |\n| `$in` | one-of (`{\"agent_id\": {\"$in\": [\"bot-a\",\"bot-b\"]}}`) |\n| `$nin` | none-of (MongoDB semantics \u2014 null/absent passes) |\n| `$exists` | `true` = field set, `false` = field unset |\n| `$gt` / `$gte` / `$lt` / `$lte` | range comparators |\n| `$between` | `[lo, hi]` inclusive range |\n| `AND` | `{\"AND\": [, , ...]}` \u2014 intersection |\n| `OR` | `{\"OR\": [, ...]}` \u2014 union |\n| `NOT` | `{\"NOT\": }` \u2014 negation |\n\nTop-level keys are implicit-AND'd. Unknown operators are silently\nignored. Filters apply to any indexed payload field, including\ncustomer metadata keys (those land as their own indexed payload keys).\n\n> **Note (ADR-001 A5, commit `a7da7a9`):** `metadata` was dropped\n> server-side and removed from the SDK, so filtering on *customer metadata\n> keys* no longer has a `metadata` field to target. The entity fields\n> (`user_id`, `agent_id`, `conv_id`, `app_id`), `type`, and other indexed\n> payload keys are unaffected. Whether/how metadata-key filtering returns is\n> owned by the typed-filter-DSL (B1) design doc. This annotation is\n> additive — the operator table above is unchanged.\n\n## Error envelope\n\nAll non-2xx responses use a single body shape (verified against the\nlive deployment, gitban card 2syddu / ADR-003 A1):\n\n```\n{\n \"error\": {\n \"type\": \"\",\n \"code\": \"\",\n \"message\": \"\",\n \"request_id\": \"\",\n \"details\": {...} // optional; present on 422 (see below)\n }\n}\n```\n\nSwitch on `error.code` rather than parsing the message. Each endpoint\ndeclares the codes it can return.\n\nField-validation failures (422) carry the per-field errors under\n`error.details.errors`, an array of `{field, message, type}` entries\n(e.g. `{\"field\": \"query\", \"message\": \"Field required\", \"type\": \"missing\"}`)\n\u2014 **not** the FastAPI-default `detail: [{loc, msg, type}]` shape. The\n`@xtraceai/memory` SDK lifts `error.details` onto `MemoryError.details`.\n\n## Response headers\n\n- `X-Request-Id` \u2014 echo of the inbound request id, or a server-\n generated UUID. Use this when filing support requests; operator\n logs are keyed on it.\n- `x-ratelimit-limit`, `x-ratelimit-remaining`, `x-ratelimit-reset` \u2014\n current bucket state for this `(org_id, key_hash)` pair (verified live,\n gitban card 2syddu / ADR-003 A3). `x-ratelimit-reset` is an **absolute\n epoch-seconds timestamp** (e.g. `1781491860`), not a delta; derive the\n seconds-remaining as `reset - now_epoch_seconds`. The bucket is shared\n across auth schemes (keyed on `(org_id, key_hash)`). The un-prefixed\n RFC-draft `RateLimit-*` names are not emitted by the live deployment;\n the SDK reads them only as a defensive fallback.\n- `Retry-After` \u2014 present on 429 responses, seconds until the next\n permitted request.\n", "version": "1.0.0" }, "paths": { @@ -60,7 +60,7 @@ } }, "400": { - "description": "`detail.code = \"invalid_messages\"` \u2014 empty `messages` array.", + "description": "`error.code = \"invalid_messages\"` \u2014 empty `messages` array.", "content": { "application/json": { "schema": { @@ -80,7 +80,7 @@ } }, "401": { - "description": "Authentication failed. Missing / invalid API key or X-Org-Id. Body carries `detail.code = \"unauthorized\"`.", + "description": "Authentication failed. Missing / invalid API key or X-Org-Id. Body carries `error.code = \"unauthorized\"`.", "content": { "application/json": { "schema": { @@ -90,7 +90,7 @@ } }, "429": { - "description": "Rate-limit or daily-cap exceeded. `Retry-After` and `RateLimit-*` response headers indicate when to retry.", + "description": "Rate-limit or daily-cap exceeded. `Retry-After` and `x-ratelimit-*` response headers indicate when to retry.", "content": { "application/json": { "schema": { @@ -100,7 +100,7 @@ } }, "500": { - "description": "Server-side failure. Body carries `detail.code = \"server_error\"` with a stable error code; details are in the operator log, indexed by the `X-Request-Id` echoed in the response header.", + "description": "Server-side failure. Body carries `error.code = \"server_error\"` with a stable error code; details are in the operator log, indexed by the `X-Request-Id` echoed in the response header.", "content": { "application/json": { "schema": { @@ -110,7 +110,7 @@ } }, "403": { - "description": "Authenticated, but the caller is not permitted on this endpoint. Currently fires when a service-token (first-party portal) caller hits an ingest / PATCH / DELETE \u2014 those are reserved for API-key callers. Body carries `detail.code = \"forbidden\"`.", + "description": "Authenticated, but the caller is not permitted on this endpoint. Currently fires when a service-token (first-party portal) caller hits an ingest / PATCH / DELETE \u2014 those are reserved for API-key callers. Body carries `error.code = \"forbidden\"`.", "content": { "application/json": { "schema": { @@ -313,7 +313,7 @@ } }, "401": { - "description": "Authentication failed. Missing / invalid API key or X-Org-Id. Body carries `detail.code = \"unauthorized\"`.", + "description": "Authentication failed. Missing / invalid API key or X-Org-Id. Body carries `error.code = \"unauthorized\"`.", "content": { "application/json": { "schema": { @@ -323,7 +323,7 @@ } }, "429": { - "description": "Rate-limit or daily-cap exceeded. `Retry-After` and `RateLimit-*` response headers indicate when to retry.", + "description": "Rate-limit or daily-cap exceeded. `Retry-After` and `x-ratelimit-*` response headers indicate when to retry.", "content": { "application/json": { "schema": { @@ -333,7 +333,7 @@ } }, "500": { - "description": "Server-side failure. Body carries `detail.code = \"server_error\"` with a stable error code; details are in the operator log, indexed by the `X-Request-Id` echoed in the response header.", + "description": "Server-side failure. Body carries `error.code = \"server_error\"` with a stable error code; details are in the operator log, indexed by the `X-Request-Id` echoed in the response header.", "content": { "application/json": { "schema": { @@ -376,7 +376,7 @@ } }, "404": { - "description": "`detail.code = \"job_not_found\"` \u2014 unknown job id or tenant mismatch.", + "description": "`error.code = \"job_not_found\"` \u2014 unknown job id or tenant mismatch.", "content": { "application/json": { "schema": { @@ -396,7 +396,7 @@ } }, "401": { - "description": "Authentication failed. Missing / invalid API key or X-Org-Id. Body carries `detail.code = \"unauthorized\"`.", + "description": "Authentication failed. Missing / invalid API key or X-Org-Id. Body carries `error.code = \"unauthorized\"`.", "content": { "application/json": { "schema": { @@ -406,7 +406,7 @@ } }, "429": { - "description": "Rate-limit or daily-cap exceeded. `Retry-After` and `RateLimit-*` response headers indicate when to retry.", + "description": "Rate-limit or daily-cap exceeded. `Retry-After` and `x-ratelimit-*` response headers indicate when to retry.", "content": { "application/json": { "schema": { @@ -416,7 +416,7 @@ } }, "500": { - "description": "Server-side failure. Body carries `detail.code = \"server_error\"` with a stable error code; details are in the operator log, indexed by the `X-Request-Id` echoed in the response header.", + "description": "Server-side failure. Body carries `error.code = \"server_error\"` with a stable error code; details are in the operator log, indexed by the `X-Request-Id` echoed in the response header.", "content": { "application/json": { "schema": { @@ -459,7 +459,7 @@ } }, "400": { - "description": "`detail.code = \"missing_user_id\"` or `\"missing_conv_id\"` \u2014 `include=context_prompt` was requested but the corresponding entity is absent from `filters` (the retrieval-agent pipeline needs both to construct the right cache namespace).", + "description": "`error.code = \"missing_user_id\"` or `\"missing_conv_id\"` \u2014 `include=context_prompt` was requested but the corresponding entity is absent from `filters` (the retrieval-agent pipeline needs both to construct the right cache namespace).", "content": { "application/json": { "schema": { @@ -479,7 +479,7 @@ } }, "401": { - "description": "Authentication failed. Missing / invalid API key or X-Org-Id. Body carries `detail.code = \"unauthorized\"`.", + "description": "Authentication failed. Missing / invalid API key or X-Org-Id. Body carries `error.code = \"unauthorized\"`.", "content": { "application/json": { "schema": { @@ -489,7 +489,7 @@ } }, "429": { - "description": "Rate-limit or daily-cap exceeded. `Retry-After` and `RateLimit-*` response headers indicate when to retry.", + "description": "Rate-limit or daily-cap exceeded. `Retry-After` and `x-ratelimit-*` response headers indicate when to retry.", "content": { "application/json": { "schema": { @@ -499,7 +499,7 @@ } }, "500": { - "description": "Server-side failure. Body carries `detail.code = \"server_error\"` with a stable error code; details are in the operator log, indexed by the `X-Request-Id` echoed in the response header.", + "description": "Server-side failure. Body carries `error.code = \"server_error\"` with a stable error code; details are in the operator log, indexed by the `X-Request-Id` echoed in the response header.", "content": { "application/json": { "schema": { @@ -542,7 +542,7 @@ } }, "404": { - "description": "`detail.code = \"memory_not_found\"` \u2014 no row with that id under this org.", + "description": "`error.code = \"memory_not_found\"` \u2014 no row with that id under this org.", "content": { "application/json": { "schema": { @@ -562,7 +562,7 @@ } }, "401": { - "description": "Authentication failed. Missing / invalid API key or X-Org-Id. Body carries `detail.code = \"unauthorized\"`.", + "description": "Authentication failed. Missing / invalid API key or X-Org-Id. Body carries `error.code = \"unauthorized\"`.", "content": { "application/json": { "schema": { @@ -572,7 +572,7 @@ } }, "429": { - "description": "Rate-limit or daily-cap exceeded. `Retry-After` and `RateLimit-*` response headers indicate when to retry.", + "description": "Rate-limit or daily-cap exceeded. `Retry-After` and `x-ratelimit-*` response headers indicate when to retry.", "content": { "application/json": { "schema": { @@ -582,7 +582,7 @@ } }, "500": { - "description": "Server-side failure. Body carries `detail.code = \"server_error\"` with a stable error code; details are in the operator log, indexed by the `X-Request-Id` echoed in the response header.", + "description": "Server-side failure. Body carries `error.code = \"server_error\"` with a stable error code; details are in the operator log, indexed by the `X-Request-Id` echoed in the response header.", "content": { "application/json": { "schema": { @@ -597,8 +597,8 @@ "tags": [ "memories" ], - "summary": "Update a memory's text and/or metadata", - "description": "Update text and/or metadata.\n\n- ``text`` change \u2192 supersede (create a new ACTIVE fact pointing\n back at the old one via ``supersedes``); fact-only operation.\n- ``metadata`` change \u2192 merge onto the existing payload in place\n (works for any type).\n- Both \u2192 text supersede; the new revision carries the merged\n metadata. The old revision keeps its original metadata.\n\nTrying to set an entity id / ``type`` / ``created_at`` via\n``metadata`` surfaces ``422 immutable_field``.", + "summary": "Edit a memory's group memberships", + "description": "Add and/or remove group memberships on an existing memory. **Live\nendpoint** \u2014 verified against the deployed API (gitban card 2syddu /\nADR-003 A4).\n\nThe earlier `[REMOVED \u2014 returns 405]` annotation was **wrong**: the\nlive probe confirmed `PATCH /v1/memories/{memory_id}` exists and its\ncurrent contract is group-membership editing, not the old\n`{text, metadata}` `UpdateRequest`. (That old content-edit contract is\ngenuinely gone; corrections to a memory's *content* still flow through\ningest / supersede via `POST /v1/memories`.)\n\nContract:\n\n- Request body is `{add_group_ids?, remove_group_ids?}`; at least one\n must carry an entry. An empty body (or both arrays empty) returns\n `422` with `error.code = \"empty_patch\"`.\n- On success returns `200 OK` + the full updated `Memory` (identical\n shape to `GET /v1/memories/{memory_id}`), with `group_ids`\n reflecting the edit.\n- API-key callers only; a service-token (first-party portal) caller\n gets `403 forbidden` (same as ingest / DELETE).\n\nThe `@xtraceai/memory` SDK exposes this as\n`memories.patch(id, {add_group_ids?, remove_group_ids?}): Promise`,\nwhich also guards the empty-patch case client-side.", "operationId": "patch_memory_v1_memories__memory_id__patch", "parameters": [ { @@ -616,14 +616,14 @@ "content": { "application/json": { "schema": { - "$ref": "#/components/schemas/UpdateRequest" + "$ref": "#/components/schemas/MemoryPatchRequest" } } } }, "responses": { "200": { - "description": "Successful Response", + "description": "Successful Response \u2014 the full updated `Memory`, `group_ids` reflecting the edit.", "content": { "application/json": { "schema": { @@ -632,18 +632,8 @@ } } }, - "400": { - "description": "`detail.code = \"invalid_request\"` \u2014 empty PATCH body (neither `text` nor `metadata`).", - "content": { - "application/json": { - "schema": { - "$ref": "#/components/schemas/ErrorEnvelope" - } - } - } - }, "404": { - "description": "`detail.code = \"memory_not_found\"`.", + "description": "`error.code = \"memory_not_found\"` \u2014 no row with that id under this org.", "content": { "application/json": { "schema": { @@ -653,7 +643,7 @@ } }, "422": { - "description": "Validation error. Possible codes: `immutable_field` (entity ids / `type` / `created_at` in `metadata`), `reserved_field` (internal storage key in `metadata`), `empty_text_field` (empty / whitespace-only `text`), or `invalid_request` (`text` patch on a non-fact row).", + "description": "`error.code = \"empty_patch\"` \u2014 the body carried neither `add_group_ids` nor `remove_group_ids` (or both were empty). Provide at least one group id to add or remove.", "content": { "application/json": { "schema": { @@ -663,7 +653,7 @@ } }, "401": { - "description": "Authentication failed. Missing / invalid API key or X-Org-Id. Body carries `detail.code = \"unauthorized\"`.", + "description": "Authentication failed. Missing / invalid API key or X-Org-Id. Body carries `error.code = \"unauthorized\"`.", "content": { "application/json": { "schema": { @@ -673,7 +663,7 @@ } }, "429": { - "description": "Rate-limit or daily-cap exceeded. `Retry-After` and `RateLimit-*` response headers indicate when to retry.", + "description": "Rate-limit or daily-cap exceeded. `Retry-After` and `x-ratelimit-*` response headers indicate when to retry.", "content": { "application/json": { "schema": { @@ -683,7 +673,7 @@ } }, "500": { - "description": "Server-side failure. Body carries `detail.code = \"server_error\"` with a stable error code; details are in the operator log, indexed by the `X-Request-Id` echoed in the response header.", + "description": "Server-side failure. Body carries `error.code = \"server_error\"` with a stable error code; details are in the operator log, indexed by the `X-Request-Id` echoed in the response header.", "content": { "application/json": { "schema": { @@ -693,7 +683,7 @@ } }, "403": { - "description": "Authenticated, but the caller is not permitted on this endpoint. Currently fires when a service-token (first-party portal) caller hits an ingest / PATCH / DELETE \u2014 those are reserved for API-key callers. Body carries `detail.code = \"forbidden\"`.", + "description": "Authenticated, but the caller is not permitted on this endpoint. Currently fires when a service-token (first-party portal) caller hits an ingest / PATCH / DELETE \u2014 those are reserved for API-key callers. Body carries `error.code = \"forbidden\"`.", "content": { "application/json": { "schema": { @@ -727,7 +717,7 @@ "description": "Deleted. Subsequent DELETEs return 404 (idempotent contract)." }, "404": { - "description": "`detail.code = \"memory_not_found\"` \u2014 unknown id, or a fact that was already soft-deleted (`tag2` is anything other than `active`).", + "description": "`error.code = \"memory_not_found\"` \u2014 unknown id, or a fact that was already soft-deleted (`tag2` is anything other than `active`).", "content": { "application/json": { "schema": { @@ -737,7 +727,7 @@ } }, "500": { - "description": "`detail.code = \"delete_failed\"` \u2014 storage error on hard delete (artifacts / episodes).", + "description": "`error.code = \"delete_failed\"` \u2014 storage error on hard delete (artifacts / episodes).", "content": { "application/json": { "schema": { @@ -757,7 +747,7 @@ } }, "401": { - "description": "Authentication failed. Missing / invalid API key or X-Org-Id. Body carries `detail.code = \"unauthorized\"`.", + "description": "Authentication failed. Missing / invalid API key or X-Org-Id. Body carries `error.code = \"unauthorized\"`.", "content": { "application/json": { "schema": { @@ -767,7 +757,7 @@ } }, "429": { - "description": "Rate-limit or daily-cap exceeded. `Retry-After` and `RateLimit-*` response headers indicate when to retry.", + "description": "Rate-limit or daily-cap exceeded. `Retry-After` and `x-ratelimit-*` response headers indicate when to retry.", "content": { "application/json": { "schema": { @@ -777,7 +767,7 @@ } }, "403": { - "description": "Authenticated, but the caller is not permitted on this endpoint. Currently fires when a service-token (first-party portal) caller hits an ingest / PATCH / DELETE \u2014 those are reserved for API-key callers. Body carries `detail.code = \"forbidden\"`.", + "description": "Authenticated, but the caller is not permitted on this endpoint. Currently fires when a service-token (first-party portal) caller hits an ingest / PATCH / DELETE \u2014 those are reserved for API-key callers. Body carries `error.code = \"forbidden\"`.", "content": { "application/json": { "schema": { @@ -792,12 +782,72 @@ }, "components": { "schemas": { - "ErrorDetail": { + "ErrorFieldError": { + "properties": { + "field": { + "type": "string", + "title": "Field", + "description": "Name of the request field that failed validation.", + "examples": [ + "query" + ] + }, + "message": { + "type": "string", + "title": "Message", + "description": "Human-readable description of the validation failure.", + "examples": [ + "Field required" + ] + }, + "type": { + "type": "string", + "title": "Type", + "description": "Stable validation-failure type identifier.", + "examples": [ + "missing" + ] + } + }, + "type": "object", + "required": [ + "field", + "message", + "type" + ], + "title": "ErrorFieldError", + "description": "One per-field validation error, as carried in `error.details.errors`\non a 422 response. The live deployment uses `{field, message, type}`\n(NOT the FastAPI-default `{loc, msg, type}`). Verified against the live\nAPI (gitban card 2syddu / ADR-003 A1)." + }, + "ErrorDetails": { "properties": { + "errors": { + "type": "array", + "items": { + "$ref": "#/components/schemas/ErrorFieldError" + }, + "title": "Errors", + "description": "Per-field validation errors. Present on 422 responses." + } + }, + "type": "object", + "additionalProperties": true, + "title": "ErrorDetails", + "description": "Optional structured detail attached to an error. On 422 responses\nit carries an `errors` array of per-field `{field, message, type}`\nentries. Other endpoints may attach additional keys." + }, + "ErrorBody": { + "properties": { + "type": { + "type": "string", + "title": "Type", + "description": "Error class \u2014 the broad category of failure.", + "examples": [ + "invalid_request_error" + ] + }, "code": { "type": "string", "title": "Code", - "description": "Stable error identifier. Switch on this rather than parsing the message. Common values: `invalid_request`, `invalid_messages`, `memory_not_found`, `job_not_found`, `immutable_field`, `reserved_field`, `empty_text_field`, `missing_user_id`, `missing_conv_id`, `unsupported_include_option`, `cursor_mismatch`, `unauthorized`, `forbidden`, `rate_limited`, `delete_failed`, `search_failed`, `ingest_failed`, `server_error`.", + "description": "Stable error identifier. Switch on this rather than parsing the message. Common values: `invalid_request`, `invalid_messages`, `empty_patch`, `memory_not_found`, `job_not_found`, `immutable_field`, `reserved_field`, `empty_text_field`, `missing_user_id`, `missing_conv_id`, `unsupported_include_option`, `cursor_mismatch`, `unauthorized`, `forbidden`, `rate_limited`, `delete_failed`, `search_failed`, `ingest_failed`, `server_error`.", "examples": [ "memory_not_found" ] @@ -809,28 +859,46 @@ "examples": [ "Memory 0fa1c0e6-... not found" ] + }, + "request_id": { + "type": "string", + "title": "Request Id", + "description": "Server-generated request id, echoed in the `X-Request-Id` response header. Use it when filing support requests." + }, + "details": { + "anyOf": [ + { + "$ref": "#/components/schemas/ErrorDetails" + }, + { + "type": "null" + } + ], + "title": "Details", + "description": "Optional structured detail; on 422 carries `errors` (per-field validation errors)." } }, "type": "object", "required": [ + "type", "code", "message" ], - "title": "ErrorDetail", - "description": "Inner ``detail`` block on every non-2xx response raised by a\nmemory-API route handler. ``code`` is the stable identifier\nclients should switch on; ``message`` is a sanitized human-\nreadable summary." + "title": "ErrorBody", + "description": "Inner `error` object on every non-2xx response raised by a memory-API\nroute handler. `code` is the stable identifier clients should switch\non; `message` is a sanitized human-readable summary; `request_id`\nmirrors the `X-Request-Id` header; `details` is optional structured\ndata (per-field validation errors on 422). Verified against the live\nAPI (gitban card 2syddu / ADR-003 A1)." }, "ErrorEnvelope": { "properties": { - "detail": { - "$ref": "#/components/schemas/ErrorDetail" + "error": { + "$ref": "#/components/schemas/ErrorBody" } }, "type": "object", "required": [ - "detail" + "error" ], "title": "ErrorEnvelope", - "description": "Standard error body for non-2xx responses raised by the memory\nAPI. Pydantic field-validation failures (422) use the FastAPI-\ndefault ``HTTPValidationError`` shape instead, where ``detail`` is\nan array of per-field error entries \u2014 switch on the response\nstatus code to pick the right shape." + "description": "Standard error body for all non-2xx responses raised by the memory\nAPI: a single top-level `error` object. The previously-documented\n`{detail: {...}}` / FastAPI `detail: [{loc, msg, type}]` shapes are NOT\nproduced by the live deployment (gitban card 2syddu / ADR-003 A1); the\n`@xtraceai/memory` SDK still tolerates them defensively. Switch on\n`error.code`; per-field 422 errors live under `error.details.errors`." }, "HTTPValidationError": { "properties": { @@ -1441,6 +1509,43 @@ "title": "SearchListEnvelope", "description": "Search response \u2014 list envelope + optional ``extras`` block." }, + "MemoryPatchRequest": { + "properties": { + "add_group_ids": { + "anyOf": [ + { + "type": "array", + "items": { + "type": "string" + } + }, + { + "type": "null" + } + ], + "title": "Add Group Ids", + "description": "Group ids to add this memory to. Omit or send `[]` to add nothing." + }, + "remove_group_ids": { + "anyOf": [ + { + "type": "array", + "items": { + "type": "string" + } + }, + { + "type": "null" + } + ], + "title": "Remove Group Ids", + "description": "Group ids to remove this memory from. Omit or send `[]` to remove nothing." + } + }, + "type": "object", + "title": "MemoryPatchRequest", + "description": "Body for `PATCH /v1/memories/{memory_id}` \u2014 the live group-membership\nedit contract (gitban card 2syddu / ADR-003 A4). At least one of\n`add_group_ids` / `remove_group_ids` must carry an entry; an empty\npatch returns `422 empty_patch`. This is the **only** mutate-in-place\noperation; the old `{text, metadata}` `UpdateRequest` is gone (content\ncorrections flow through ingest / supersede). The `@xtraceai/memory`\nSDK surfaces this as `MemoryPatchRequest`." + }, "SearchRequest": { "properties": { "query": { @@ -1519,38 +1624,6 @@ "title": "SearchRequest", "description": "POST /v1/memories/search \u2014 vector + filter search over the\nunified memory pool. No ``mode`` flag; opt into composed output\nvia ``include``.\n\n``filters`` is the full DSL (bare values, ``$in``/``$ne``/``$gte``/\n``$lte``, ``AND``/``OR``/``NOT``). Empty/omitted filters mean\n\"all memories in this org\"." }, - "UpdateRequest": { - "properties": { - "text": { - "anyOf": [ - { - "type": "string" - }, - { - "type": "null" - } - ], - "title": "Text", - "description": "New fact text. When provided, supersedes the existing fact: a new ACTIVE row is created pointing back at the old one via `supersedes`, the old row's `tag2` flips to superseded. Fact-only \u2014 sending `text` on an artifact or episode row returns 422. Empty / whitespace-only values return 422 `empty_text_field`." - }, - "metadata": { - "anyOf": [ - { - "additionalProperties": true, - "type": "object" - }, - { - "type": "null" - } - ], - "title": "Metadata", - "description": "Customer metadata to merge onto the row. Keys present here replace same-named keys on the existing row; keys absent here are left untouched (no key deletion via PATCH). Setting an immutable field (entity ids, `type`, `created_at`) returns 422 `immutable_field`; setting an internal storage key (`tag1`-`tag5`, `kb_type`, etc.) returns 422 `reserved_field`." - } - }, - "type": "object", - "title": "UpdateRequest", - "description": "PATCH /v1/memories/{id} \u2014 update text and/or metadata.\n\nMetadata is merged onto the existing payload, not replaced.\nImmutable fields (``type``, entity ids, ``created_at``) cannot be\nchanged via this endpoint; attempting to set them surfaces\n``422 immutable_field``." - }, "ValidationError": { "properties": { "loc": { @@ -1829,6 +1902,12 @@ "bearerFormat": "Token", "description": "Long-lived API key sent as `Authorization: Token `." }, + "BearerAuth": { + "type": "http", + "scheme": "bearer", + "bearerFormat": "API key", + "description": "Long-lived API key sent as `Authorization: Bearer `. This is the form the `@xtraceai/memory` SDK has shipped since its initial commit (through v0.2.1) and that the live API accepts; documented here additively per ADR-001 (A2a) alongside the `Token`/`x-api-key` schemes, which remain valid. Added because the spec previously documented only `Token`/`x-api-key`; nothing was removed." + }, "OrgId": { "type": "apiKey", "in": "header", @@ -1845,6 +1924,10 @@ { "BearerToken": [], "OrgId": [] + }, + { + "BearerAuth": [], + "OrgId": [] } ] } diff --git a/src/client.ts b/src/client.ts index d712c28..941dda6 100644 --- a/src/client.ts +++ b/src/client.ts @@ -1,4 +1,5 @@ import { defaultHttpConfig, HttpClient } from "./http.js"; +import type { AuthMode } from "./http.js"; import { Groups } from "./groups.js"; import { Memories } from "./memories.js"; @@ -15,6 +16,12 @@ export interface MemoryClientOptions { maxRetries?: number; /** Provide a custom request-id generator. Default: `req_`. */ requestIdFactory?: () => string; + /** + * How to present the API key on the wire. `'bearer'` (default) sends + * `Authorization: Bearer `; `'x-api-key'` sends `x-api-key: ` + * and no `Authorization` header. `X-Org-Id` is sent in both modes. + */ + authMode?: AuthMode; } export class MemoryClient { @@ -33,6 +40,7 @@ export class MemoryClient { fetch: options.fetch, maxRetries: options.maxRetries, defaultRequestId: options.requestIdFactory, + authMode: options.authMode, }), ); this.memories = new Memories(http); diff --git a/src/errors.test.ts b/src/errors.test.ts new file mode 100644 index 0000000..f97d84d --- /dev/null +++ b/src/errors.test.ts @@ -0,0 +1,385 @@ +import { describe, it, expect } from "vitest"; +import { HttpClient } from "./http.js"; +import { + MemoryError, + Unauthorized, + Unprocessable, + MemoryNotFound, + RateLimited, + parseErrorBody, +} from "./errors.js"; + +/** + * Build an HttpClient whose `fetch` returns a single crafted Response. This + * drives the real `request()` -> `toError` -> `parseErrorBody` path so the + * tests exercise the whole transport stack, not just the pure helper. + * + * `maxRetries: 0` keeps error/429 responses from being retried, so each test + * sees exactly one fetch and throws on the first non-ok response. + */ +function clientReturning(opts: { + status: number; + body?: unknown; + headers?: Record; +}): HttpClient { + const fetchImpl = (async () => { + const text = opts.body === undefined ? "" : JSON.stringify(opts.body); + return new Response(text, { + status: opts.status, + headers: { "content-type": "application/json", ...opts.headers }, + }); + }) as unknown as typeof globalThis.fetch; + + return new HttpClient({ + apiKey: "k", + orgId: "o", + baseUrl: "https://api.test.local", + fetch: fetchImpl, + maxRetries: 0, + sleep: async () => {}, + }); +} + +/** Force the error from a request, asserting it is a MemoryError subclass. */ +async function errorFrom(client: HttpClient): Promise { + try { + await client.request("GET", "/v1/thing"); + } catch (err) { + expect(err).toBeInstanceOf(MemoryError); + return err as MemoryError; + } + throw new Error("expected request to throw"); +} + +describe("parseErrorBody (pure precedence helper)", () => { + it("legacy {error:{...}} → fields lifted verbatim", () => { + const parsed = parseErrorBody({ + error: { type: "rate_limit", code: "too_many", message: "slow down", details: { a: 1 } }, + }); + expect(parsed).toEqual({ + type: "rate_limit", + code: "too_many", + message: "slow down", + details: { a: 1 }, + }); + }); + + it("spec {detail:{code,message}} → code/message extracted", () => { + const parsed = parseErrorBody({ detail: { code: "bad_input", message: "nope" } }); + expect(parsed).toEqual({ code: "bad_input", message: "nope" }); + }); + + it("422 {detail:[...]} → validation_error code with raw array under details", () => { + const arr = [{ loc: ["body", "name"], msg: "field required", type: "value_error.missing" }]; + const parsed = parseErrorBody({ detail: arr }); + expect(parsed?.code).toBe("validation_error"); + expect(parsed?.message).toMatch(/1 field error/i); + expect(parsed?.details).toEqual({ validation_errors: arr }); + }); + + it("FastAPI string {detail:'...'} → message is that string, code left to default", () => { + const parsed = parseErrorBody({ detail: "Not authenticated" }); + expect(parsed).toEqual({ message: "Not authenticated" }); + expect(parsed?.code).toBeUndefined(); + }); + + it("array check precedes object check (an array is typeof 'object')", () => { + // If object-before-array, the array would be treated as detail.code/message + // (both undefined) and lose the validation_error synthesis. + const parsed = parseErrorBody({ detail: [{ loc: ["x"], msg: "m", type: "t" }] }); + expect(parsed?.code).toBe("validation_error"); + }); + + it("unrecognized shape → null (so errorForStatus applies its defaults)", () => { + expect(parseErrorBody({})).toBeNull(); + expect(parseErrorBody(null)).toBeNull(); + expect(parseErrorBody("plain string")).toBeNull(); + expect(parseErrorBody({ detail: 42 })).toBeNull(); + }); + + it("array `error` is NOT a legacy envelope — falls through to a sibling detail", () => { + // An array is `typeof 'object'`; the legacy branch must reject it (isRecord) + // so a populated `detail` envelope is still parsed instead of returning + // an all-undefined legacy match. + const parsed = parseErrorBody({ error: [], detail: { code: "rate_limited", message: "slow down" } }); + expect(parsed).toEqual({ code: "rate_limited", message: "slow down" }); + }); + + it("array `error` with no detail → null (not an all-undefined legacy match)", () => { + expect(parseErrorBody({ error: [1, 2] })).toBeNull(); + }); +}); + +describe("error envelope through request() -> toError (capstone)", () => { + it("legacy {error:{...}} parses exactly as today (regression lock)", async () => { + const err = await errorFrom( + clientReturning({ + status: 401, + body: { + error: { + type: "auth_error", + code: "invalid_key", + message: "bad key", + details: { hint: "rotate" }, + }, + }, + }), + ); + expect(err).toBeInstanceOf(Unauthorized); + expect(err.status).toBe(401); + expect(err.errorType).toBe("auth_error"); + expect(err.code).toBe("invalid_key"); + expect(err.message).toBe("bad key"); + expect(err.details).toEqual({ hint: "rotate" }); + }); + + it("spec {detail:{code,message}} yields that code/message", async () => { + const err = await errorFrom( + clientReturning({ status: 400, body: { detail: { code: "bad_request", message: "missing q" } } }), + ); + expect(err.code).toBe("bad_request"); + expect(err.message).toBe("missing q"); + }); + + it("422 {detail:[...]} → Unprocessable with validation_error + details.validation_errors", async () => { + const arr = [ + { loc: ["body", "query"], msg: "field required", type: "value_error.missing" }, + { loc: ["body", "limit"], msg: "must be > 0", type: "value_error" }, + ]; + const err = await errorFrom(clientReturning({ status: 422, body: { detail: arr } })); + expect(err).toBeInstanceOf(Unprocessable); + expect(err.code).toBe("validation_error"); + expect(err.message).toMatch(/2 field error/i); + expect(err.details).toEqual({ validation_errors: arr }); + }); + + it("FastAPI {detail:'string'} → message is that string", async () => { + const err = await errorFrom(clientReturning({ status: 403, body: { detail: "Not authenticated" } })); + expect(err.message).toBe("Not authenticated"); + // code falls back to the status default since the wire carried none + expect(err.code).toBe("unknown_error"); + }); + + it("unrecognized body → generic fallback unchanged (unknown_error)", async () => { + const err = await errorFrom(clientReturning({ status: 500, body: { unexpected: true } })); + expect(err.code).toBe("unknown_error"); + expect(err.message).toMatch(/status 500/); + }); +}); + +describe("live wire bodies (gitban card 2syddu / ADR-003 A1 regression lock)", () => { + // The deployed Memory API uses the legacy `{error:{...}}` envelope for every + // non-2xx response — NOT the previously-spec'd `{detail:{...}}` shape. These + // fixtures are the exact bodies captured against the live API in card 2syddu; + // they lock the production `request() -> toError -> parseErrorBody` path so a + // future refactor of parseErrorBody can't silently stop lifting them. + + it("live 422 → Unprocessable with code:invalid_request and per-field error.details lifted", async () => { + // Verbatim live 422 body (card 2syddu A1). Field errors live under + // error.details.errors[] as {field,message,type} (NOT FastAPI {loc,msg,type}). + const err = await errorFrom( + clientReturning({ + status: 422, + body: { + error: { + type: "invalid_request_error", + code: "invalid_request", + message: "Request failed validation", + request_id: "req_live_422", + details: { errors: [{ field: "query", message: "Field required", type: "missing" }] }, + }, + }, + }), + ); + expect(err).toBeInstanceOf(Unprocessable); + expect(err.status).toBe(422); + expect(err.errorType).toBe("invalid_request_error"); + expect(err.code).toBe("invalid_request"); + expect(err.message).toBe("Request failed validation"); + // The whole `error.details` object is lifted onto MemoryError.details, so the + // captured per-field error survives verbatim (this is what a consumer reads). + expect(err.details).toEqual({ + errors: [{ field: "query", message: "Field required", type: "missing" }], + }); + expect((err.details?.errors as unknown[])[0]).toEqual({ + field: "query", + message: "Field required", + type: "missing", + }); + }); + + it("live 401 → Unauthorized with code:http_401, message survives", async () => { + const err = await errorFrom( + clientReturning({ + status: 401, + body: { + error: { + type: "authentication_error", + code: "http_401", + message: "Invalid API key for this organization", + request_id: "req_live_401", + }, + }, + }), + ); + expect(err).toBeInstanceOf(Unauthorized); + expect(err.status).toBe(401); + expect(err.code).toBe("http_401"); + expect(err.errorType).toBe("authentication_error"); + expect(err.message).toBe("Invalid API key for this organization"); + }); + + it("live 404 → MemoryNotFound with code:memory_not_found, message survives", async () => { + const err = await errorFrom( + clientReturning({ + status: 404, + body: { + error: { + type: "not_found_error", + code: "memory_not_found", + message: "Memory abc not found", + request_id: "req_live_404", + }, + }, + }), + ); + expect(err).toBeInstanceOf(MemoryNotFound); + expect(err.status).toBe(404); + expect(err.code).toBe("memory_not_found"); + expect(err.errorType).toBe("not_found_error"); + expect(err.message).toBe("Memory abc not found"); + }); +}); + +describe("rate-limit snapshot (KD-2)", () => { + it("x-ratelimit-* live capture → parsed with raw epoch reset (A3 fix)", async () => { + // card 2syddu A3: live server emits x-ratelimit-* with reset as an + // absolute epoch-seconds timestamp (1781491860), not a delta. + const client = clientReturning({ + status: 200, + body: { ok: true }, + headers: { + "x-ratelimit-limit": "30", + "x-ratelimit-remaining": "23", + "x-ratelimit-reset": "1781491860", + }, + }); + const res = await client.request("GET", "/v1/thing"); + expect(res.rateLimit).toEqual({ limit: 30, remaining: 23, reset: 1781491860 }); + }); + + it("both families present → x-ratelimit-* wins per field (tie-break)", async () => { + const client = clientReturning({ + status: 200, + body: { ok: true }, + headers: { + "x-ratelimit-limit": "30", + "x-ratelimit-remaining": "23", + "x-ratelimit-reset": "1781491860", + "RateLimit-Limit": "100", + "RateLimit-Remaining": "42", + "RateLimit-Reset": "30", + }, + }); + const res = await client.request("GET", "/v1/thing"); + expect(res.rateLimit).toEqual({ limit: 30, remaining: 23, reset: 1781491860 }); + }); + + it("non-numeric x-ratelimit-* values are dropped (num guard preserved)", async () => { + const client = clientReturning({ + status: 200, + body: { ok: true }, + headers: { "x-ratelimit-limit": "not-a-number", "x-ratelimit-remaining": "9" }, + }); + const res = await client.request("GET", "/v1/thing"); + expect(res.rateLimit).toEqual({ remaining: 9 }); + }); + + it("empty / whitespace x-ratelimit-* values are dropped, not coerced to 0", async () => { + const client = clientReturning({ + status: 200, + body: { ok: true }, + headers: { + "x-ratelimit-remaining": "", + "x-ratelimit-reset": " ", + "x-ratelimit-limit": "50", + }, + }); + const res = await client.request("GET", "/v1/thing"); + expect(res.rateLimit).toEqual({ limit: 50 }); + }); + + it("legitimate 0-valued x-ratelimit-* is preserved, not dropped (?? not ||)", async () => { + // remaining:0 is an exhausted bucket — the most consumer-visible signal. + // The num() trim-guard drops "" but must keep "0"; pick() uses ?? so a real + // 0 wins over an absent RateLimit-* fallback (|| would wrongly discard it). + const client = clientReturning({ + status: 200, + body: { ok: true }, + headers: { "x-ratelimit-limit": "0", "x-ratelimit-remaining": "0", "x-ratelimit-reset": "0" }, + }); + const res = await client.request("GET", "/v1/thing"); + expect(res.rateLimit).toEqual({ limit: 0, remaining: 0, reset: 0 }); + }); + + it("RateLimit-* present → parsed onto the request() return (fallback retained)", async () => { + const client = clientReturning({ + status: 200, + body: { ok: true }, + headers: { "RateLimit-Limit": "100", "RateLimit-Remaining": "42", "RateLimit-Reset": "30" }, + }); + const res = await client.request("GET", "/v1/thing"); + expect(res.rateLimit).toEqual({ limit: 100, remaining: 42, reset: 30 }); + }); + + it("RateLimit-* present → parsed onto a thrown RateLimited", async () => { + const client = clientReturning({ + status: 429, + body: { error: { type: "rate_limit", code: "too_many", message: "slow down" } }, + headers: { "RateLimit-Limit": "100", "RateLimit-Remaining": "0", "RateLimit-Reset": "5", "retry-after": "5" }, + }); + const err = await errorFrom(client); + expect(err).toBeInstanceOf(RateLimited); + expect(err.rateLimit).toEqual({ limit: 100, remaining: 0, reset: 5 }); + expect((err as RateLimited).retryAfter).toBe(5); + }); + + it("RateLimit-* present → parsed onto any thrown MemoryError (not just 429)", async () => { + const client = clientReturning({ + status: 503, + body: { error: { type: "server_error", code: "unavailable", message: "down" } }, + headers: { "RateLimit-Remaining": "7" }, + }); + const err = await errorFrom(client); + expect(err.rateLimit).toEqual({ remaining: 7 }); + }); + + it("RateLimit-* absent → rateLimit is undefined on the return, nothing throws", async () => { + const client = clientReturning({ status: 200, body: { ok: true } }); + const res = await client.request("GET", "/v1/thing"); + expect(res.rateLimit).toBeUndefined(); + }); + + it("non-numeric RateLimit-* values are dropped (Number.isFinite guard)", async () => { + const client = clientReturning({ + status: 200, + body: { ok: true }, + headers: { "RateLimit-Limit": "not-a-number", "RateLimit-Remaining": "9" }, + }); + const res = await client.request("GET", "/v1/thing"); + // limit dropped (non-finite), remaining kept; an all-empty snapshot would be undefined + expect(res.rateLimit).toEqual({ remaining: 9 }); + }); + + it("empty / whitespace RateLimit-* values are dropped, not coerced to 0", async () => { + const client = clientReturning({ + status: 200, + body: { ok: true }, + headers: { "RateLimit-Remaining": "", "RateLimit-Reset": " ", "RateLimit-Limit": "50" }, + }); + const res = await client.request("GET", "/v1/thing"); + // An empty/whitespace header must NOT become 0 (a falsely-exhausted bucket); + // only the real numeric limit survives. + expect(res.rateLimit).toEqual({ limit: 50 }); + }); +}); diff --git a/src/errors.ts b/src/errors.ts index 34b9fba..be799ae 100644 --- a/src/errors.ts +++ b/src/errors.ts @@ -1,9 +1,44 @@ +/** + * A point-in-time read of the server's rate-limit bucket, parsed from the + * `x-ratelimit-limit` / `x-ratelimit-remaining` / `x-ratelimit-reset` response + * headers (the family the live deployment emits), with the un-prefixed + * `RateLimit-*` family read as a per-field fallback. Every field is optional: a + * header that is absent or non-numeric yields `undefined` for that field + * (never a throw). Use it to back off proactively before you hit a `429`. + */ +export interface RateLimitSnapshot { + /** `x-ratelimit-limit` (or `RateLimit-Limit`) — the request ceiling for the current window. */ + limit?: number; + /** `x-ratelimit-remaining` (or `RateLimit-Remaining`) — requests still allowed in this window. */ + remaining?: number; + /** + * `x-ratelimit-reset` (or `RateLimit-Reset`) — the absolute **epoch-seconds** + * timestamp at which the window resets, surfaced as the raw header value with + * no conversion. This is a point in time, **not** a countdown/duration: to get + * the seconds remaining, compute `reset - Math.floor(Date.now() / 1000)`. + * + * Redefinition note: earlier docs described this as "seconds until the window + * resets" (a delta). That was incorrect — the live server (gitban card 2syddu, + * ADR-003 A3) emits an epoch timestamp, and because the snapshot never + * populated in production before the A3 parser fix (it read the wrong header + * family), no consumer could have depended on the old delta reading. This is a + * documentation correction, not a behavioural change. + */ + reset?: number; +} + export class MemoryError extends Error { readonly status: number; readonly errorType: string; readonly code: string; readonly requestId: string | undefined; readonly details: Record | undefined; + /** + * The server's rate-limit bucket state at failure time, if the response + * carried `x-ratelimit-*` (or fallback `RateLimit-*`) headers. `undefined` + * when none were present. + */ + readonly rateLimit: RateLimitSnapshot | undefined; constructor(args: { status: number; @@ -12,6 +47,7 @@ export class MemoryError extends Error { message: string; requestId?: string; details?: Record; + rateLimit?: RateLimitSnapshot; }) { super(args.message); this.name = this.constructor.name; @@ -20,6 +56,7 @@ export class MemoryError extends Error { this.code = args.code; this.requestId = args.requestId; this.details = args.details; + this.rateLimit = args.rateLimit; } } @@ -40,11 +77,96 @@ export class RateLimited extends MemoryError { } } +/** Normalized error body shape produced by {@link parseErrorBody}. */ +export interface ParsedError { + type?: string; + code?: string; + message?: string; + details?: Record; +} + +/** + * Normalize any server error body to `{ type?, code?, message?, details? }`, + * handling every envelope the Memory API can emit. Precedence is LIVE-first: the + * `{error:{…}}` shape is what the deployed API actually returns (ADR-003 live + * probe / gitban card 2syddu), so it keeps top precedence. The `{detail:…}` + * branches below are retained as defensive tolerance (the FORMER spec shape + + * FastAPI defaults) — do NOT remove them or demote `{error}`. Each field is + * validated to its declared type — a conformant `code` is a string, `details` an + * object — so verbatim and validated agree: + * + * 1. `{ error: {…} }` — the live/canonical `ApiErrorBody` object (spec-documented + * since ADR-003). Not an array; an array `error` falls through so a sibling + * `detail` envelope is still read. + * 2. `{ detail: [...] }` — FastAPI 422 validation array. The array carries no + * top-level code, so synthesize `code: 'validation_error'`, build a human + * `message`, and preserve the raw array under `details.validation_errors`. + * (Checked *before* the object branch — a JS array is also `typeof 'object'`.) + * 3. `{ detail: {…} }` — the FORMER spec envelope (superseded by ADR-003), + * retained defensively; `{ code, message }`. + * 4. `{ detail: "..." }` — FastAPI's default `HTTPException(detail=...)` shape; + * `{ message }` only, leaving `code` to the status default. + * 5. anything else → `null`, so {@link errorForStatus} applies its existing + * `unknown_error` / generic-message defaults unchanged. + */ +export function parseErrorBody(parsed: unknown): ParsedError | null { + if (parsed === null || typeof parsed !== "object") return null; + const obj = parsed as Record; + + // 1. Legacy `{ error: {…} }` — top precedence. Must be a plain object: an + // array `error` is not a legacy envelope, so fall through to let a sibling + // `detail` envelope (if any) be parsed rather than returning all-undefined. + const legacy = obj.error; + if (isRecord(legacy)) { + const e = legacy; + return { + type: typeof e.type === "string" ? e.type : undefined, + code: typeof e.code === "string" ? e.code : undefined, + message: typeof e.message === "string" ? e.message : undefined, + details: isRecord(e.details) ? e.details : undefined, + }; + } + + const detail = obj.detail; + + // 2. 422 validation array — must precede the object branch. + if (Array.isArray(detail)) { + const n = detail.length; + return { + code: "validation_error", + message: `Validation failed: ${n} field error${n === 1 ? "" : "s"}`, + details: { validation_errors: detail }, + }; + } + + // 3. Spec `{ detail: { code, message } }`. + if (detail !== null && typeof detail === "object") { + const d = detail as Record; + return { + code: typeof d.code === "string" ? d.code : undefined, + message: typeof d.message === "string" ? d.message : undefined, + }; + } + + // 4. FastAPI default `{ detail: "..." }`. + if (typeof detail === "string") { + return { message: detail }; + } + + // 5. Unrecognized. + return null; +} + +function isRecord(v: unknown): v is Record { + return v !== null && typeof v === "object" && !Array.isArray(v); +} + export function errorForStatus( status: number, - body: { type?: string; code?: string; message?: string; details?: Record } | null, + body: ParsedError | null, requestId: string | undefined, retryAfter?: number, + rateLimit?: RateLimitSnapshot, ): MemoryError { const args = { status, @@ -53,6 +175,7 @@ export function errorForStatus( message: body?.message ?? `Memory API request failed with status ${status}`, requestId, details: body?.details, + rateLimit, }; if (status === 400) return new BadRequest(args); if (status === 401) return new Unauthorized(args); diff --git a/src/filter.test.ts b/src/filter.test.ts new file mode 100644 index 0000000..caad1e9 --- /dev/null +++ b/src/filter.test.ts @@ -0,0 +1,228 @@ +import { describe, it, expect } from "vitest"; +import { f } from "./filter.js"; +import type { Clause, FieldOps } from "./filter.js"; +import { Memories } from "./memories.js"; +import type { HttpClient } from "./http.js"; +import type { Filter, Memory, SearchRequest } from "./types.js"; + +describe("filter DSL — single-operator shorthands emit the documented wire JSON", () => { + it("eq → { field: { $eq: v } }", () => { + expect(f.eq("agent_id", "bot")).toEqual({ agent_id: { $eq: "bot" } }); + }); + + it("ne → { field: { $ne: v } }", () => { + expect(f.ne("status", "archived")).toEqual({ status: { $ne: "archived" } }); + }); + + it("in → { field: { $in: [...] } }", () => { + expect(f.in("agent_id", ["bot-a", "bot-b"])).toEqual({ agent_id: { $in: ["bot-a", "bot-b"] } }); + }); + + it("nin → { field: { $nin: [...] } }", () => { + expect(f.nin("plan", ["free", "trial"])).toEqual({ plan: { $nin: ["free", "trial"] } }); + }); + + it("exists defaults to true, and takes an explicit false", () => { + expect(f.exists("conv_id")).toEqual({ conv_id: { $exists: true } }); + expect(f.exists("conv_id", false)).toEqual({ conv_id: { $exists: false } }); + }); + + it("between → { field: { $between: [lo, hi] } }", () => { + expect(f.between("score", 0.5, 0.9)).toEqual({ score: { $between: [0.5, 0.9] } }); + }); + + it("isNull → { field: null } (null = unset, per spec)", () => { + expect(f.isNull("agent_id")).toEqual({ agent_id: null }); + }); + + it("each range comparator is expressible via f.field", () => { + expect(f.field("score", { $gt: 1 })).toEqual({ score: { $gt: 1 } }); + expect(f.field("score", { $gte: 1 })).toEqual({ score: { $gte: 1 } }); + expect(f.field("score", { $lt: 5 })).toEqual({ score: { $lt: 5 } }); + expect(f.field("score", { $lte: 5 })).toEqual({ score: { $lte: 5 } }); + }); +}); + +describe("filter DSL — f.field is the only multi-operator-per-field path", () => { + it("keeps BOTH operators for a two-sided range (no silent clobber)", () => { + // The B1 fix: a range must not collapse to one operator. + expect(f.field("price", { $gt: 10, $lt: 100 })).toEqual({ price: { $gt: 10, $lt: 100 } }); + }); + + it("copies the ops object so a later caller mutation can't corrupt the clause", () => { + const ops: FieldOps = { $gte: 0.5, $lt: 0.9 }; + const clause = f.field("score", ops); + ops.$lt = 999; // mutate the caller's input after building + expect(clause).toEqual({ score: { $gte: 0.5, $lt: 0.9 } }); + }); + + it("deep-copies array-valued operators so caller array mutation can't corrupt the clause", () => { + // A shallow {...ops} would leave $between/$in/$nin aliased to the caller's + // arrays — mutating them afterwards would corrupt the already-built clause. + const between: [number, number] = [10, 100]; + const inVals = ["a", "b"]; + const ops: FieldOps = { $between: between, $in: inVals }; + const clause = f.field("price", ops); + between[1] = 999; // mutate the caller's tuple + inVals.push("c"); // mutate the caller's array + expect(clause).toEqual({ price: { $between: [10, 100], $in: ["a", "b"] } }); + }); +}); + +describe("filter DSL — combinators emit AND/OR/NOT wire keys", () => { + it("and → { AND: [...] }", () => { + expect(f.and(f.eq("a", 1), f.eq("b", 2))).toEqual({ + AND: [{ a: { $eq: 1 } }, { b: { $eq: 2 } }], + }); + }); + + it("or → { OR: [...] }", () => { + expect(f.or(f.eq("a", 1), f.eq("b", 2))).toEqual({ + OR: [{ a: { $eq: 1 } }, { b: { $eq: 2 } }], + }); + }); + + it("not → { NOT: }", () => { + expect(f.not(f.eq("a", 1))).toEqual({ NOT: { a: { $eq: 1 } } }); + }); + + it("f.and lets you AND two operators on the SAME field explicitly", () => { + expect(f.and(f.field("price", { $gt: 10 }), f.field("price", { $lt: 100 }))).toEqual({ + AND: [{ price: { $gt: 10 } }, { price: { $lt: 100 } }], + }); + }); +}); + +describe("filter DSL — f.all implicit-ANDs distinct fields and throws on collision", () => { + it("merges distinct-field clauses into one implicit-AND object", () => { + expect(f.all(f.eq("agent_id", "bot"), f.in("plan", ["a", "b"]))).toEqual({ + agent_id: { $eq: "bot" }, + plan: { $in: ["a", "b"] }, + }); + }); + + it("throws on a duplicate field, naming the field and the remedy", () => { + expect(() => f.all(f.eq("x", 1), f.eq("x", 2))).toThrow(/duplicate field "x"/); + // the message points the caller at f.field / f.and + expect(() => f.all(f.eq("x", 1), f.eq("x", 2))).toThrow(/f\.field|f\.and/); + }); + + it("does not mutate its input clauses", () => { + const a = f.eq("agent_id", "bot"); + const b = f.in("plan", ["a", "b"]); + f.all(a, b); + expect(a).toEqual({ agent_id: { $eq: "bot" } }); + expect(b).toEqual({ plan: { $in: ["a", "b"] } }); + }); +}); + +describe("filter DSL — key-agnostic (A5 reconciliation)", () => { + it("filters an arbitrary metadata/payload key identically to an entity axis", () => { + // Entity axis and a customer metadata key build the same wire shape — the + // builder has no fixed field enum, so dropping the typed `metadata` field + // does not block metadata-key filtering. + const entity = f.eq("agent_id", "bot"); + const meta = f.eq("tier", "gold"); + expect(entity).toEqual({ agent_id: { $eq: "bot" } }); + expect(meta).toEqual({ tier: { $eq: "gold" } }); + // mixed in one f.all, distinct keys coexist + expect(f.all(entity, meta)).toEqual({ + agent_id: { $eq: "bot" }, + tier: { $eq: "gold" }, + }); + }); +}); + +describe("filter DSL — output assignable to SearchRequest.filters", () => { + it("a built Clause / Filter is assignable to the escape-hatch Filter type", () => { + const clause: Clause = f.field("score", { $gte: 0.5 }); + const filter: Filter = clause; // Clause assignable to Record + const req: SearchRequest = { query: "q", filters: filter }; + expect(req.filters).toEqual({ score: { $gte: 0.5 } }); + }); + + it("the raw Record escape hatch is still accepted directly", () => { + const raw: Filter = { agent_id: { $eq: "bot" } }; + const req: SearchRequest = { query: "q", filters: raw }; + expect(req.filters).toEqual({ agent_id: { $eq: "bot" } }); + }); +}); + +/** Minimal fact-shaped Memory; `over` patches any field. */ +function mem(id: string, over: Partial = {}): Memory { + return { + id, + object: "memory", + type: "fact", + text: id, + user_id: null, + agent_id: null, + conv_id: null, + app_id: null, + group_ids: [], + categories: [], + score: 1, + created_at: "2026-01-01T00:00:00Z", + updated_at: "2026-01-01T00:00:00Z", + details: { + fact_type: null, + status: null, + supersedes: null, + source_role: null, + episode_id: null, + artifact_id: null, + artifact_ids: [], + source_event_ids: [], + }, + ...over, + } as Memory; +} + +function fakeHttp(): { http: HttpClient; calls: SearchRequest[] } { + const calls: SearchRequest[] = []; + const http = { + request: async (_method: string, path: string, options: { body?: SearchRequest } = {}) => { + if (path === "/v1/groups") { + return { body: { object: "list", data: [] }, status: 200, requestId: "req_test" }; + } + calls.push(options.body as SearchRequest); + return { + body: { object: "list", data: [mem("A")], has_more: false, next_cursor: null }, + status: 200, + requestId: "req_test", + }; + }, + } as unknown as HttpClient; + return { http, calls }; +} + +describe("filter DSL — capstone: round-trips through search() (mocked)", () => { + it("builds a mixed eq/range/in filter, keeps both range ops, and search() forwards it verbatim", async () => { + const filter = f.all( + f.eq("agent_id", "bot"), + f.field("score", { $gte: 0.5, $lt: 0.9 }), + f.in("plan", ["a", "b"]), + ); + + // 1. exact documented wire shape, with BOTH range operators present + expect(filter).toEqual({ + agent_id: { $eq: "bot" }, + score: { $gte: 0.5, $lt: 0.9 }, + plan: { $in: ["a", "b"] }, + }); + + // 2. a same-field f.all is a loud throw, not a silent drop + expect(() => f.all(f.eq("x", 1), f.eq("x", 2))).toThrow(); + + // 3. assignable to SearchRequest.filters and forwarded unchanged by search() + const { http, calls } = fakeHttp(); + const env = await new Memories(http).search({ query: "q", filters: filter }); + expect(calls).toHaveLength(1); + expect(calls[0]!.filters).toEqual({ + agent_id: { $eq: "bot" }, + score: { $gte: 0.5, $lt: 0.9 }, + plan: { $in: ["a", "b"] }, + }); + expect(env.data.map((m) => m.id)).toEqual(["A"]); + }); +}); diff --git a/src/filter.ts b/src/filter.ts new file mode 100644 index 0000000..61192e7 --- /dev/null +++ b/src/filter.ts @@ -0,0 +1,139 @@ +// Typed filter DSL builder (design B1 / KD-4). +// +// Emits the wire JSON the server's filter DSL expects for +// `SearchRequest.filters` (see spec/memory.json "Filter DSL"): +// - per-field operators: $eq $ne $in $nin $exists $gt $gte $lt $lte $between +// - a bare `null` value means "field is unset" +// - boolean composition via the AND / OR / NOT keys +// - top-level keys are implicit-AND'd +// +// The builder is **key-agnostic** by construction: it builds conditions over +// arbitrary field-name strings, not a fixed field enum. That is the deliberate +// resolution of the `metadata` tension (ADR-001 A5) — the SDK dropped the typed +// `metadata` field, but the server still filters on *any* indexed payload key, +// including customer metadata keys. A key-agnostic builder therefore filters +// entity axes (`user_id`, `agent_id`, …) and arbitrary metadata/payload keys +// identically, without reintroducing a typed `metadata` field. +// +// `Filter = Record` (src/types.ts) stays as the raw escape +// hatch; every value this module emits is assignable to it. + +import type { Filter } from "./types.js"; + +/** A value usable with the equality/range operators. */ +export type Comparable = string | number | boolean; + +/** + * The operators available on a single field. Use {@link f.field} to put more + * than one operator on the same field — e.g. a two-sided range is + * `f.field('price', { $gt: 10, $lt: 100 })`, which keeps **both** operators. + */ +export interface FieldOps { + $eq?: Comparable; + $ne?: Comparable; + $in?: Comparable[]; + $nin?: Comparable[]; + $exists?: boolean; + $gt?: Comparable; + $gte?: Comparable; + $lt?: Comparable; + $lte?: Comparable; + /** Inclusive `[lo, hi]` range. */ + $between?: [Comparable, Comparable]; +} + +/** + * One filter clause: a per-field condition (`{ field: ops }` or `{ field: null }` + * for "unset"), or a boolean composition of clauses. Assignable to + * {@link Filter} (`Record`), so any clause can be passed + * directly as `SearchRequest.filters`. + */ +export type Clause = + | Record + | { AND: Clause[] } + | { OR: Clause[] } + | { NOT: Clause }; + +/** + * Typed, discoverable builder for the filter DSL. Build a clause and pass it as + * `SearchRequest.filters`; the emitted object is the exact wire shape. + * + * The design forces every multi-operator-per-field condition through a single + * {@link f.field} call (and makes {@link f.all} throw on a same-field collision) + * so a silently-wrong range — two operators collapsing to one — is + * unrepresentable. + * + * @example + * // (agent_id == "bot") AND (0.5 <= score < 0.9) AND (plan in ["a","b"]) + * search({ query, filters: f.all( + * f.eq("agent_id", "bot"), + * f.field("score", { $gte: 0.5, $lt: 0.9 }), + * f.in("plan", ["a", "b"]), + * )}); + */ +export const f = { + /** + * Single-field condition carrying one or more operators. This is the ONLY way + * to put multiple operators on one field, so a range is + * `f.field('price', { $gt: 10, $lt: 100 })` — both operators kept. The ops + * object AND its array-valued operators (`$in`/`$nin`/`$between`) are copied, + * so mutating the caller's input afterwards can't corrupt the clause. + */ + field: (name: string, ops: FieldOps): Clause => { + const copy: FieldOps = { ...ops }; + // Deep-copy the array-typed operators — a shallow `{ ...ops }` would leave + // these aliased to the caller's arrays, so a later `ops.$between[1] = …` + // would mutate the already-built clause. + if (Array.isArray(copy.$in)) copy.$in = [...copy.$in]; + if (Array.isArray(copy.$nin)) copy.$nin = [...copy.$nin]; + if (Array.isArray(copy.$between)) copy.$between = [copy.$between[0], copy.$between[1]]; + return { [name]: copy }; + }, + + /** `field == v` (explicit equality; same as a bare value server-side). */ + eq: (field: string, v: Comparable): Clause => ({ [field]: { $eq: v } }), + /** `field != v` AND field is set (SQL semantics). */ + ne: (field: string, v: Comparable): Clause => ({ [field]: { $ne: v } }), + /** `field` is one-of `v`. */ + in: (field: string, v: Comparable[]): Clause => ({ [field]: { $in: [...v] } }), + /** `field` is none-of `v` (MongoDB semantics — null/absent passes). */ + nin: (field: string, v: Comparable[]): Clause => ({ [field]: { $nin: [...v] } }), + /** `$exists`: `true` (default) = field set, `false` = field unset. */ + exists: (field: string, v = true): Clause => ({ [field]: { $exists: v } }), + /** Inclusive `[lo, hi]` range. */ + between: (field: string, lo: Comparable, hi: Comparable): Clause => ({ + [field]: { $between: [lo, hi] }, + }), + /** `field` is unset — emits `{ field: null }` (null = unset, per spec). */ + isNull: (field: string): Clause => ({ [field]: null }), + + /** Boolean AND of clauses → `{ AND: [...] }`. */ + and: (...c: Clause[]): Clause => ({ AND: c }), + /** Boolean OR of clauses → `{ OR: [...] }`. */ + or: (...c: Clause[]): Clause => ({ OR: c }), + /** Boolean NOT of a clause → `{ NOT: }`. */ + not: (c: Clause): Clause => ({ NOT: c }), + + /** + * Implicit-AND of clauses on DISTINCT fields, merged into one object (the + * server implicit-ANDs top-level keys). Throws on a same-field collision + * rather than silently dropping operators — combine a field's operators in a + * single {@link f.field} call, or use {@link f.and} to AND two conditions on + * the same field explicitly. Inputs are not mutated. + */ + all: (...c: Clause[]): Filter => { + const out: Record = {}; + for (const clause of c) { + for (const [k, v] of Object.entries(clause)) { + if (k in out) { + throw new Error( + `filter: duplicate field "${k}" in f.all(); combine its operators in a ` + + `single f.field("${k}", {...}), or use f.and(...) to AND them explicitly`, + ); + } + out[k] = v; + } + } + return out; + }, +}; diff --git a/src/generated/types.ts b/src/generated/types.ts index 1dab175..5c47452 100644 --- a/src/generated/types.ts +++ b/src/generated/types.ts @@ -139,18 +139,32 @@ export interface paths { options?: never; head?: never; /** - * Update a memory's text and/or metadata - * @description Update text and/or metadata. + * Edit a memory's group memberships + * @description Add and/or remove group memberships on an existing memory. **Live + * endpoint** — verified against the deployed API (gitban card 2syddu / + * ADR-003 A4). * - * - ``text`` change → supersede (create a new ACTIVE fact pointing - * back at the old one via ``supersedes``); fact-only operation. - * - ``metadata`` change → merge onto the existing payload in place - * (works for any type). - * - Both → text supersede; the new revision carries the merged - * metadata. The old revision keeps its original metadata. + * The earlier `[REMOVED — returns 405]` annotation was **wrong**: the + * live probe confirmed `PATCH /v1/memories/{memory_id}` exists and its + * current contract is group-membership editing, not the old + * `{text, metadata}` `UpdateRequest`. (That old content-edit contract is + * genuinely gone; corrections to a memory's *content* still flow through + * ingest / supersede via `POST /v1/memories`.) * - * Trying to set an entity id / ``type`` / ``created_at`` via - * ``metadata`` surfaces ``422 immutable_field``. + * Contract: + * + * - Request body is `{add_group_ids?, remove_group_ids?}`; at least one + * must carry an entry. An empty body (or both arrays empty) returns + * `422` with `error.code = "empty_patch"`. + * - On success returns `200 OK` + the full updated `Memory` (identical + * shape to `GET /v1/memories/{memory_id}`), with `group_ids` + * reflecting the edit. + * - API-key callers only; a service-token (first-party portal) caller + * gets `403 forbidden` (same as ingest / DELETE). + * + * The `@xtraceai/memory` SDK exposes this as + * `memories.patch(id, {add_group_ids?, remove_group_ids?}): Promise`, + * which also guards the empty-patch case client-side. */ patch: operations["patch_memory_v1_memories__memory_id__patch"]; trace?: never; @@ -160,16 +174,66 @@ export type webhooks = Record; export interface components { schemas: { /** - * ErrorDetail - * @description Inner ``detail`` block on every non-2xx response raised by a - * memory-API route handler. ``code`` is the stable identifier - * clients should switch on; ``message`` is a sanitized human- - * readable summary. + * ErrorFieldError + * @description One per-field validation error, as carried in `error.details.errors` + * on a 422 response. The live deployment uses `{field, message, type}` + * (NOT the FastAPI-default `{loc, msg, type}`). Verified against the live + * API (gitban card 2syddu / ADR-003 A1). + */ + ErrorFieldError: { + /** + * Field + * @description Name of the request field that failed validation. + * @example query + */ + field: string; + /** + * Message + * @description Human-readable description of the validation failure. + * @example Field required + */ + message: string; + /** + * Type + * @description Stable validation-failure type identifier. + * @example missing + */ + type: string; + }; + /** + * ErrorDetails + * @description Optional structured detail attached to an error. On 422 responses + * it carries an `errors` array of per-field `{field, message, type}` + * entries. Other endpoints may attach additional keys. */ - ErrorDetail: { + ErrorDetails: { + /** + * Errors + * @description Per-field validation errors. Present on 422 responses. + */ + errors?: components["schemas"]["ErrorFieldError"][]; + } & { + [key: string]: unknown; + }; + /** + * ErrorBody + * @description Inner `error` object on every non-2xx response raised by a memory-API + * route handler. `code` is the stable identifier clients should switch + * on; `message` is a sanitized human-readable summary; `request_id` + * mirrors the `X-Request-Id` header; `details` is optional structured + * data (per-field validation errors on 422). Verified against the live + * API (gitban card 2syddu / ADR-003 A1). + */ + ErrorBody: { + /** + * Type + * @description Error class — the broad category of failure. + * @example invalid_request_error + */ + type: string; /** * Code - * @description Stable error identifier. Switch on this rather than parsing the message. Common values: `invalid_request`, `invalid_messages`, `memory_not_found`, `job_not_found`, `immutable_field`, `reserved_field`, `empty_text_field`, `missing_user_id`, `missing_conv_id`, `unsupported_include_option`, `cursor_mismatch`, `unauthorized`, `forbidden`, `rate_limited`, `delete_failed`, `search_failed`, `ingest_failed`, `server_error`. + * @description Stable error identifier. Switch on this rather than parsing the message. Common values: `invalid_request`, `invalid_messages`, `empty_patch`, `memory_not_found`, `job_not_found`, `immutable_field`, `reserved_field`, `empty_text_field`, `missing_user_id`, `missing_conv_id`, `unsupported_include_option`, `cursor_mismatch`, `unauthorized`, `forbidden`, `rate_limited`, `delete_failed`, `search_failed`, `ingest_failed`, `server_error`. * @example memory_not_found */ code: string; @@ -179,17 +243,28 @@ export interface components { * @example Memory 0fa1c0e6-... not found */ message: string; + /** + * Request Id + * @description Server-generated request id, echoed in the `X-Request-Id` response header. Use it when filing support requests. + */ + request_id?: string; + /** + * Details + * @description Optional structured detail; on 422 carries `errors` (per-field validation errors). + */ + details?: components["schemas"]["ErrorDetails"] | null; }; /** * ErrorEnvelope - * @description Standard error body for non-2xx responses raised by the memory - * API. Pydantic field-validation failures (422) use the FastAPI- - * default ``HTTPValidationError`` shape instead, where ``detail`` is - * an array of per-field error entries — switch on the response - * status code to pick the right shape. + * @description Standard error body for all non-2xx responses raised by the memory + * API: a single top-level `error` object. The previously-documented + * `{detail: {...}}` / FastAPI `detail: [{loc, msg, type}]` shapes are NOT + * produced by the live deployment (gitban card 2syddu / ADR-003 A1); the + * `@xtraceai/memory` SDK still tolerates them defensively. Switch on + * `error.code`; per-field 422 errors live under `error.details.errors`. */ ErrorEnvelope: { - detail: components["schemas"]["ErrorDetail"]; + error: components["schemas"]["ErrorBody"]; }; /** HTTPValidationError */ HTTPValidationError: { @@ -605,6 +680,28 @@ export interface components { /** @description Populated only when `include` opts in. `null` on a default search response. */ extras?: components["schemas"]["SearchExtras"] | null; }; + /** + * MemoryPatchRequest + * @description Body for `PATCH /v1/memories/{memory_id}` — the live group-membership + * edit contract (gitban card 2syddu / ADR-003 A4). At least one of + * `add_group_ids` / `remove_group_ids` must carry an entry; an empty + * patch returns `422 empty_patch`. This is the **only** mutate-in-place + * operation; the old `{text, metadata}` `UpdateRequest` is gone (content + * corrections flow through ingest / supersede). The `@xtraceai/memory` + * SDK surfaces this as `MemoryPatchRequest`. + */ + MemoryPatchRequest: { + /** + * Add Group Ids + * @description Group ids to add this memory to. Omit or send `[]` to add nothing. + */ + add_group_ids?: string[] | null; + /** + * Remove Group Ids + * @description Group ids to remove this memory from. Omit or send `[]` to remove nothing. + */ + remove_group_ids?: string[] | null; + }; /** * SearchRequest * @description POST /v1/memories/search — vector + filter search over the @@ -665,29 +762,6 @@ export interface components { */ include?: ("full_content" | "context_prompt")[]; }; - /** - * UpdateRequest - * @description PATCH /v1/memories/{id} — update text and/or metadata. - * - * Metadata is merged onto the existing payload, not replaced. - * Immutable fields (``type``, entity ids, ``created_at``) cannot be - * changed via this endpoint; attempting to set them surfaces - * ``422 immutable_field``. - */ - UpdateRequest: { - /** - * Text - * @description New fact text. When provided, supersedes the existing fact: a new ACTIVE row is created pointing back at the old one via `supersedes`, the old row's `tag2` flips to superseded. Fact-only — sending `text` on an artifact or episode row returns 422. Empty / whitespace-only values return 422 `empty_text_field`. - */ - text?: string | null; - /** - * Metadata - * @description Customer metadata to merge onto the row. Keys present here replace same-named keys on the existing row; keys absent here are left untouched (no key deletion via PATCH). Setting an immutable field (entity ids, `type`, `created_at`) returns 422 `immutable_field`; setting an internal storage key (`tag1`-`tag5`, `kb_type`, etc.) returns 422 `reserved_field`. - */ - metadata?: { - [key: string]: unknown; - } | null; - }; /** ValidationError */ ValidationError: { /** Location */ @@ -856,7 +930,7 @@ export interface operations { "application/json": components["schemas"]["ListEnvelope"]; }; }; - /** @description Authentication failed. Missing / invalid API key or X-Org-Id. Body carries `detail.code = "unauthorized"`. */ + /** @description Authentication failed. Missing / invalid API key or X-Org-Id. Body carries `error.code = "unauthorized"`. */ 401: { headers: { [name: string]: unknown; @@ -874,7 +948,7 @@ export interface operations { "application/json": components["schemas"]["ErrorEnvelope"]; }; }; - /** @description Rate-limit or daily-cap exceeded. `Retry-After` and `RateLimit-*` response headers indicate when to retry. */ + /** @description Rate-limit or daily-cap exceeded. `Retry-After` and `x-ratelimit-*` response headers indicate when to retry. */ 429: { headers: { [name: string]: unknown; @@ -883,7 +957,7 @@ export interface operations { "application/json": components["schemas"]["ErrorEnvelope"]; }; }; - /** @description Server-side failure. Body carries `detail.code = "server_error"` with a stable error code; details are in the operator log, indexed by the `X-Request-Id` echoed in the response header. */ + /** @description Server-side failure. Body carries `error.code = "server_error"` with a stable error code; details are in the operator log, indexed by the `X-Request-Id` echoed in the response header. */ 500: { headers: { [name: string]: unknown; @@ -928,7 +1002,7 @@ export interface operations { "application/json": components["schemas"]["IngestJobResponse"]; }; }; - /** @description `detail.code = "invalid_messages"` — empty `messages` array. */ + /** @description `error.code = "invalid_messages"` — empty `messages` array. */ 400: { headers: { [name: string]: unknown; @@ -937,7 +1011,7 @@ export interface operations { "application/json": components["schemas"]["ErrorEnvelope"]; }; }; - /** @description Authentication failed. Missing / invalid API key or X-Org-Id. Body carries `detail.code = "unauthorized"`. */ + /** @description Authentication failed. Missing / invalid API key or X-Org-Id. Body carries `error.code = "unauthorized"`. */ 401: { headers: { [name: string]: unknown; @@ -946,7 +1020,7 @@ export interface operations { "application/json": components["schemas"]["ErrorEnvelope"]; }; }; - /** @description Authenticated, but the caller is not permitted on this endpoint. Currently fires when a service-token (first-party portal) caller hits an ingest / PATCH / DELETE — those are reserved for API-key callers. Body carries `detail.code = "forbidden"`. */ + /** @description Authenticated, but the caller is not permitted on this endpoint. Currently fires when a service-token (first-party portal) caller hits an ingest / PATCH / DELETE — those are reserved for API-key callers. Body carries `error.code = "forbidden"`. */ 403: { headers: { [name: string]: unknown; @@ -964,7 +1038,7 @@ export interface operations { "application/json": components["schemas"]["HTTPValidationError"]; }; }; - /** @description Rate-limit or daily-cap exceeded. `Retry-After` and `RateLimit-*` response headers indicate when to retry. */ + /** @description Rate-limit or daily-cap exceeded. `Retry-After` and `x-ratelimit-*` response headers indicate when to retry. */ 429: { headers: { [name: string]: unknown; @@ -973,7 +1047,7 @@ export interface operations { "application/json": components["schemas"]["ErrorEnvelope"]; }; }; - /** @description Server-side failure. Body carries `detail.code = "server_error"` with a stable error code; details are in the operator log, indexed by the `X-Request-Id` echoed in the response header. */ + /** @description Server-side failure. Body carries `error.code = "server_error"` with a stable error code; details are in the operator log, indexed by the `X-Request-Id` echoed in the response header. */ 500: { headers: { [name: string]: unknown; @@ -1004,7 +1078,7 @@ export interface operations { "application/json": components["schemas"]["IngestJobResponse"]; }; }; - /** @description Authentication failed. Missing / invalid API key or X-Org-Id. Body carries `detail.code = "unauthorized"`. */ + /** @description Authentication failed. Missing / invalid API key or X-Org-Id. Body carries `error.code = "unauthorized"`. */ 401: { headers: { [name: string]: unknown; @@ -1013,7 +1087,7 @@ export interface operations { "application/json": components["schemas"]["ErrorEnvelope"]; }; }; - /** @description `detail.code = "job_not_found"` — unknown job id or tenant mismatch. */ + /** @description `error.code = "job_not_found"` — unknown job id or tenant mismatch. */ 404: { headers: { [name: string]: unknown; @@ -1031,7 +1105,7 @@ export interface operations { "application/json": components["schemas"]["HTTPValidationError"]; }; }; - /** @description Rate-limit or daily-cap exceeded. `Retry-After` and `RateLimit-*` response headers indicate when to retry. */ + /** @description Rate-limit or daily-cap exceeded. `Retry-After` and `x-ratelimit-*` response headers indicate when to retry. */ 429: { headers: { [name: string]: unknown; @@ -1040,7 +1114,7 @@ export interface operations { "application/json": components["schemas"]["ErrorEnvelope"]; }; }; - /** @description Server-side failure. Body carries `detail.code = "server_error"` with a stable error code; details are in the operator log, indexed by the `X-Request-Id` echoed in the response header. */ + /** @description Server-side failure. Body carries `error.code = "server_error"` with a stable error code; details are in the operator log, indexed by the `X-Request-Id` echoed in the response header. */ 500: { headers: { [name: string]: unknown; @@ -1073,7 +1147,7 @@ export interface operations { "application/json": components["schemas"]["SearchListEnvelope"]; }; }; - /** @description `detail.code = "missing_user_id"` or `"missing_conv_id"` — `include=context_prompt` was requested but the corresponding entity is absent from `filters` (the retrieval-agent pipeline needs both to construct the right cache namespace). */ + /** @description `error.code = "missing_user_id"` or `"missing_conv_id"` — `include=context_prompt` was requested but the corresponding entity is absent from `filters` (the retrieval-agent pipeline needs both to construct the right cache namespace). */ 400: { headers: { [name: string]: unknown; @@ -1082,7 +1156,7 @@ export interface operations { "application/json": components["schemas"]["ErrorEnvelope"]; }; }; - /** @description Authentication failed. Missing / invalid API key or X-Org-Id. Body carries `detail.code = "unauthorized"`. */ + /** @description Authentication failed. Missing / invalid API key or X-Org-Id. Body carries `error.code = "unauthorized"`. */ 401: { headers: { [name: string]: unknown; @@ -1100,7 +1174,7 @@ export interface operations { "application/json": components["schemas"]["ErrorEnvelope"]; }; }; - /** @description Rate-limit or daily-cap exceeded. `Retry-After` and `RateLimit-*` response headers indicate when to retry. */ + /** @description Rate-limit or daily-cap exceeded. `Retry-After` and `x-ratelimit-*` response headers indicate when to retry. */ 429: { headers: { [name: string]: unknown; @@ -1109,7 +1183,7 @@ export interface operations { "application/json": components["schemas"]["ErrorEnvelope"]; }; }; - /** @description Server-side failure. Body carries `detail.code = "server_error"` with a stable error code; details are in the operator log, indexed by the `X-Request-Id` echoed in the response header. */ + /** @description Server-side failure. Body carries `error.code = "server_error"` with a stable error code; details are in the operator log, indexed by the `X-Request-Id` echoed in the response header. */ 500: { headers: { [name: string]: unknown; @@ -1140,7 +1214,7 @@ export interface operations { "application/json": components["schemas"]["Memory"]; }; }; - /** @description Authentication failed. Missing / invalid API key or X-Org-Id. Body carries `detail.code = "unauthorized"`. */ + /** @description Authentication failed. Missing / invalid API key or X-Org-Id. Body carries `error.code = "unauthorized"`. */ 401: { headers: { [name: string]: unknown; @@ -1149,7 +1223,7 @@ export interface operations { "application/json": components["schemas"]["ErrorEnvelope"]; }; }; - /** @description `detail.code = "memory_not_found"` — no row with that id under this org. */ + /** @description `error.code = "memory_not_found"` — no row with that id under this org. */ 404: { headers: { [name: string]: unknown; @@ -1167,7 +1241,7 @@ export interface operations { "application/json": components["schemas"]["HTTPValidationError"]; }; }; - /** @description Rate-limit or daily-cap exceeded. `Retry-After` and `RateLimit-*` response headers indicate when to retry. */ + /** @description Rate-limit or daily-cap exceeded. `Retry-After` and `x-ratelimit-*` response headers indicate when to retry. */ 429: { headers: { [name: string]: unknown; @@ -1176,7 +1250,7 @@ export interface operations { "application/json": components["schemas"]["ErrorEnvelope"]; }; }; - /** @description Server-side failure. Body carries `detail.code = "server_error"` with a stable error code; details are in the operator log, indexed by the `X-Request-Id` echoed in the response header. */ + /** @description Server-side failure. Body carries `error.code = "server_error"` with a stable error code; details are in the operator log, indexed by the `X-Request-Id` echoed in the response header. */ 500: { headers: { [name: string]: unknown; @@ -1205,7 +1279,7 @@ export interface operations { }; content?: never; }; - /** @description Authentication failed. Missing / invalid API key or X-Org-Id. Body carries `detail.code = "unauthorized"`. */ + /** @description Authentication failed. Missing / invalid API key or X-Org-Id. Body carries `error.code = "unauthorized"`. */ 401: { headers: { [name: string]: unknown; @@ -1214,7 +1288,7 @@ export interface operations { "application/json": components["schemas"]["ErrorEnvelope"]; }; }; - /** @description Authenticated, but the caller is not permitted on this endpoint. Currently fires when a service-token (first-party portal) caller hits an ingest / PATCH / DELETE — those are reserved for API-key callers. Body carries `detail.code = "forbidden"`. */ + /** @description Authenticated, but the caller is not permitted on this endpoint. Currently fires when a service-token (first-party portal) caller hits an ingest / PATCH / DELETE — those are reserved for API-key callers. Body carries `error.code = "forbidden"`. */ 403: { headers: { [name: string]: unknown; @@ -1223,7 +1297,7 @@ export interface operations { "application/json": components["schemas"]["ErrorEnvelope"]; }; }; - /** @description `detail.code = "memory_not_found"` — unknown id, or a fact that was already soft-deleted (`tag2` is anything other than `active`). */ + /** @description `error.code = "memory_not_found"` — unknown id, or a fact that was already soft-deleted (`tag2` is anything other than `active`). */ 404: { headers: { [name: string]: unknown; @@ -1241,7 +1315,7 @@ export interface operations { "application/json": components["schemas"]["HTTPValidationError"]; }; }; - /** @description Rate-limit or daily-cap exceeded. `Retry-After` and `RateLimit-*` response headers indicate when to retry. */ + /** @description Rate-limit or daily-cap exceeded. `Retry-After` and `x-ratelimit-*` response headers indicate when to retry. */ 429: { headers: { [name: string]: unknown; @@ -1250,7 +1324,7 @@ export interface operations { "application/json": components["schemas"]["ErrorEnvelope"]; }; }; - /** @description `detail.code = "delete_failed"` — storage error on hard delete (artifacts / episodes). */ + /** @description `error.code = "delete_failed"` — storage error on hard delete (artifacts / episodes). */ 500: { headers: { [name: string]: unknown; @@ -1272,11 +1346,11 @@ export interface operations { }; requestBody: { content: { - "application/json": components["schemas"]["UpdateRequest"]; + "application/json": components["schemas"]["MemoryPatchRequest"]; }; }; responses: { - /** @description Successful Response */ + /** @description Successful Response — the full updated `Memory`, `group_ids` reflecting the edit. */ 200: { headers: { [name: string]: unknown; @@ -1285,16 +1359,7 @@ export interface operations { "application/json": components["schemas"]["Memory"]; }; }; - /** @description `detail.code = "invalid_request"` — empty PATCH body (neither `text` nor `metadata`). */ - 400: { - headers: { - [name: string]: unknown; - }; - content: { - "application/json": components["schemas"]["ErrorEnvelope"]; - }; - }; - /** @description Authentication failed. Missing / invalid API key or X-Org-Id. Body carries `detail.code = "unauthorized"`. */ + /** @description Authentication failed. Missing / invalid API key or X-Org-Id. Body carries `error.code = "unauthorized"`. */ 401: { headers: { [name: string]: unknown; @@ -1303,7 +1368,7 @@ export interface operations { "application/json": components["schemas"]["ErrorEnvelope"]; }; }; - /** @description Authenticated, but the caller is not permitted on this endpoint. Currently fires when a service-token (first-party portal) caller hits an ingest / PATCH / DELETE — those are reserved for API-key callers. Body carries `detail.code = "forbidden"`. */ + /** @description Authenticated, but the caller is not permitted on this endpoint. Currently fires when a service-token (first-party portal) caller hits an ingest / PATCH / DELETE — those are reserved for API-key callers. Body carries `error.code = "forbidden"`. */ 403: { headers: { [name: string]: unknown; @@ -1312,7 +1377,7 @@ export interface operations { "application/json": components["schemas"]["ErrorEnvelope"]; }; }; - /** @description `detail.code = "memory_not_found"`. */ + /** @description `error.code = "memory_not_found"` — no row with that id under this org. */ 404: { headers: { [name: string]: unknown; @@ -1321,7 +1386,7 @@ export interface operations { "application/json": components["schemas"]["ErrorEnvelope"]; }; }; - /** @description Validation error. Possible codes: `immutable_field` (entity ids / `type` / `created_at` in `metadata`), `reserved_field` (internal storage key in `metadata`), `empty_text_field` (empty / whitespace-only `text`), or `invalid_request` (`text` patch on a non-fact row). */ + /** @description `error.code = "empty_patch"` — the body carried neither `add_group_ids` nor `remove_group_ids` (or both were empty). Provide at least one group id to add or remove. */ 422: { headers: { [name: string]: unknown; @@ -1330,7 +1395,7 @@ export interface operations { "application/json": components["schemas"]["ErrorEnvelope"]; }; }; - /** @description Rate-limit or daily-cap exceeded. `Retry-After` and `RateLimit-*` response headers indicate when to retry. */ + /** @description Rate-limit or daily-cap exceeded. `Retry-After` and `x-ratelimit-*` response headers indicate when to retry. */ 429: { headers: { [name: string]: unknown; @@ -1339,7 +1404,7 @@ export interface operations { "application/json": components["schemas"]["ErrorEnvelope"]; }; }; - /** @description Server-side failure. Body carries `detail.code = "server_error"` with a stable error code; details are in the operator log, indexed by the `X-Request-Id` echoed in the response header. */ + /** @description Server-side failure. Body carries `error.code = "server_error"` with a stable error code; details are in the operator log, indexed by the `X-Request-Id` echoed in the response header. */ 500: { headers: { [name: string]: unknown; diff --git a/src/http.test.ts b/src/http.test.ts new file mode 100644 index 0000000..7722d32 --- /dev/null +++ b/src/http.test.ts @@ -0,0 +1,85 @@ +import { describe, it, expect } from "vitest"; +import { HttpClient, defaultHttpConfig } from "./http.js"; +import { MemoryClient } from "./client.js"; + +/** + * Capture the outgoing request headers from a single `request()` call. The + * fetch impl records the `headers` object it was handed and returns a trivial + * 200 so `request()` resolves without retrying. + */ +function captureHeaders(): { + fetch: typeof globalThis.fetch; + last: () => Record; +} { + let captured: Record = {}; + const fetchImpl = (async (_url: string, init: RequestInit) => { + captured = (init.headers ?? {}) as Record; + return new Response("{}", { status: 200, headers: { "content-type": "application/json" } }); + }) as unknown as typeof globalThis.fetch; + return { fetch: fetchImpl, last: () => captured }; +} + +describe("HttpClient auth header form", () => { + it("defaults to Authorization: Bearer and sends no x-api-key", async () => { + const cap = captureHeaders(); + const http = new HttpClient( + defaultHttpConfig({ apiKey: "xtk_123", orgId: "org_9", fetch: cap.fetch }), + ); + await http.request("GET", "/v1/thing"); + const h = cap.last(); + expect(h["Authorization"]).toBe("Bearer xtk_123"); + expect(h["x-api-key"]).toBeUndefined(); + expect(h["X-Org-Id"]).toBe("org_9"); + }); + + it("authMode 'bearer' is byte-identical to the default", async () => { + const cap = captureHeaders(); + const http = new HttpClient( + defaultHttpConfig({ apiKey: "xtk_123", orgId: "org_9", fetch: cap.fetch, authMode: "bearer" }), + ); + await http.request("GET", "/v1/thing"); + const h = cap.last(); + expect(h["Authorization"]).toBe("Bearer xtk_123"); + expect(h["x-api-key"]).toBeUndefined(); + expect(h["X-Org-Id"]).toBe("org_9"); + }); + + it("authMode 'x-api-key' sends x-api-key and no Authorization", async () => { + const cap = captureHeaders(); + const http = new HttpClient( + defaultHttpConfig({ apiKey: "xtk_123", orgId: "org_9", fetch: cap.fetch, authMode: "x-api-key" }), + ); + await http.request("GET", "/v1/thing"); + const h = cap.last(); + expect(h["x-api-key"]).toBe("xtk_123"); + expect(h["Authorization"]).toBeUndefined(); + expect(h["X-Org-Id"]).toBe("org_9"); + }); +}); + +describe("MemoryClient auth header form (end-to-end plumbing)", () => { + it("default client sends Authorization: Bearer and X-Org-Id, no x-api-key", async () => { + const cap = captureHeaders(); + const client = new MemoryClient({ apiKey: "xtk_abc", orgId: "org_1", fetch: cap.fetch }); + await client.groups.list(); + const h = cap.last(); + expect(h["Authorization"]).toBe("Bearer xtk_abc"); + expect(h["x-api-key"]).toBeUndefined(); + expect(h["X-Org-Id"]).toBe("org_1"); + }); + + it("authMode 'x-api-key' client sends x-api-key and X-Org-Id, no Authorization", async () => { + const cap = captureHeaders(); + const client = new MemoryClient({ + apiKey: "xtk_abc", + orgId: "org_1", + fetch: cap.fetch, + authMode: "x-api-key", + }); + await client.groups.list(); + const h = cap.last(); + expect(h["x-api-key"]).toBe("xtk_abc"); + expect(h["Authorization"]).toBeUndefined(); + expect(h["X-Org-Id"]).toBe("org_1"); + }); +}); diff --git a/src/http.ts b/src/http.ts index 72f7269..673f5fb 100644 --- a/src/http.ts +++ b/src/http.ts @@ -1,5 +1,21 @@ -import { errorForStatus, MemoryError, RateLimited, ServerError } from "./errors.js"; -import type { ApiErrorBody } from "./types.js"; +import { errorForStatus, MemoryError, parseErrorBody, RateLimited, ServerError } from "./errors.js"; +import type { RateLimitSnapshot } from "./errors.js"; + +/** + * How the API key is presented on the wire. `'bearer'` (default) sends + * `Authorization: Bearer `; `'x-api-key'` sends `x-api-key: ` + * and no `Authorization` header. An enum (not a boolean) so a future `Token` + * form can be added without renaming. + * + * The scheme is purely a wire-format choice — it carries **no rate-limit + * advantage**. Both schemes are rate-limited against the same + * `(org_id, key_hash)` bucket, so switching from `Bearer` to `x-api-key` does + * not buy extra quota. (An early n=1 probe appeared to show a 10× quota + * difference; a controlled n=8 re-probe showed that was a first-call cold-start + * artifact tracking call position, not the auth scheme — see card 2syddu and + * ADR-003 A2 / KD-6.) `Bearer` therefore stays the default. + */ +export type AuthMode = "bearer" | "x-api-key"; export interface HttpClientConfig { apiKey: string; @@ -8,6 +24,8 @@ export interface HttpClientConfig { fetch: typeof globalThis.fetch; defaultRequestId?: () => string; maxRetries: number; + /** How to present the API key. Defaults to `'bearer'`. */ + authMode?: AuthMode; /** Hook for tests; pass a no-op to disable real timers. */ sleep: (ms: number) => Promise; } @@ -30,7 +48,7 @@ function makeRequestId(): string { export class HttpClient { constructor(private readonly config: HttpClientConfig) {} - async request(method: string, path: string, options: RequestOptions = {}): Promise<{ body: T; status: number; requestId: string | undefined }> { + async request(method: string, path: string, options: RequestOptions = {}): Promise<{ body: T; status: number; requestId: string | undefined; rateLimit: RateLimitSnapshot | undefined }> { const url = new URL(path, this.config.baseUrl); if (options.query) { for (const [k, v] of Object.entries(options.query)) { @@ -41,11 +59,15 @@ export class HttpClient { const requestId = options.requestId ?? this.config.defaultRequestId?.() ?? makeRequestId(); const headers: Record = { - Authorization: `Bearer ${this.config.apiKey}`, "X-Org-Id": this.config.orgId, "X-Request-Id": requestId, Accept: "application/json", }; + if (this.config.authMode === "x-api-key") { + headers["x-api-key"] = this.config.apiKey; + } else { + headers["Authorization"] = `Bearer ${this.config.apiKey}`; + } let body: string | undefined; if (options.body !== undefined) { @@ -75,19 +97,20 @@ export class HttpClient { } const respRequestId = response.headers.get("x-request-id") ?? requestId; + const rateLimit = parseRateLimit(response.headers); if (response.status === 204) { - return { body: undefined as T, status: response.status, requestId: respRequestId }; + return { body: undefined as T, status: response.status, requestId: respRequestId, rateLimit }; } const text = await response.text(); const parsed: unknown = text ? safeJson(text) : null; if (response.ok) { - return { body: parsed as T, status: response.status, requestId: respRequestId }; + return { body: parsed as T, status: response.status, requestId: respRequestId, rateLimit }; } - const err = toError(response, parsed, respRequestId); + const err = toError(response, parsed, respRequestId, rateLimit); const retryable = err instanceof RateLimited || (err instanceof ServerError && idempotent); if (retryable && attempt < this.config.maxRetries && !options.signal?.aborted) { attempt++; @@ -114,18 +137,65 @@ function safeJson(text: string): unknown { } } -function toError(response: Response, parsed: unknown, requestId: string | undefined): MemoryError { - const body = (parsed as ApiErrorBody | null)?.error; +function toError( + response: Response, + parsed: unknown, + requestId: string | undefined, + rateLimit: RateLimitSnapshot | undefined, +): MemoryError { + const body = parseErrorBody(parsed); const retryAfterHeader = response.headers.get("retry-after"); const retryAfter = retryAfterHeader ? Number(retryAfterHeader) : undefined; return errorForStatus( response.status, - body ?? null, + body, requestId, Number.isFinite(retryAfter) ? retryAfter : undefined, + rateLimit, ); } +/** + * Parse the rate-limit response headers into a {@link RateLimitSnapshot}. + * + * The live deployment emits the `x-ratelimit-*` family (`x-ratelimit-limit` / + * `-remaining` / `-reset`), so each field reads that name first and falls back + * per-field to the un-prefixed `RateLimit-*` name. The fallback is read with the + * same `??` selection so the proven-live family wins on a tie; `RateLimit-*` is + * retained as the unobserved-but-tolerated fallback (ADR-003 A3). The two + * families are distinct `Headers.get()` lookups — the `x-` prefix is a literal + * part of the name, not a case variant — so the selection is per field rather + * than all-x-or-all-plain. + * + * `reset` is surfaced as the raw header value: against the live server it is an + * absolute epoch-seconds timestamp (see {@link RateLimitSnapshot.reset}), and + * no conversion is applied here. Each field is included only when its header is + * present and numerically finite; if no field survives, returns `undefined` + * (an absent bucket, never an empty object). Never throws. + */ +function parseRateLimit(headers: Headers): RateLimitSnapshot | undefined { + const num = (name: string): number | undefined => { + const raw = headers.get(name); + // Drop absent, empty, and whitespace-only headers: `Number("")` and + // `Number(" ")` both coerce to a finite 0, which would otherwise be + // reported as `remaining: 0` (a falsely-exhausted bucket). + if (raw === null || raw.trim() === "") return undefined; + const n = Number(raw); + return Number.isFinite(n) ? n : undefined; + }; + // x-ratelimit-* (proven live) first, RateLimit-* as a per-field fallback. + const pick = (xName: string, plainName: string): number | undefined => + num(xName) ?? num(plainName); + const snapshot: RateLimitSnapshot = {}; + const limit = pick("x-ratelimit-limit", "RateLimit-Limit"); + const remaining = pick("x-ratelimit-remaining", "RateLimit-Remaining"); + const reset = pick("x-ratelimit-reset", "RateLimit-Reset"); + if (limit !== undefined) snapshot.limit = limit; + if (remaining !== undefined) snapshot.remaining = remaining; + if (reset !== undefined) snapshot.reset = reset; + return Object.keys(snapshot).length > 0 ? snapshot : undefined; +} + export function defaultHttpConfig(input: { apiKey: string; orgId: string; @@ -133,6 +203,7 @@ export function defaultHttpConfig(input: { fetch?: typeof globalThis.fetch; maxRetries?: number; defaultRequestId?: () => string; + authMode?: AuthMode; }): HttpClientConfig { return { apiKey: input.apiKey, @@ -141,6 +212,7 @@ export function defaultHttpConfig(input: { fetch: input.fetch ?? globalThis.fetch.bind(globalThis), maxRetries: input.maxRetries ?? 2, defaultRequestId: input.defaultRequestId, + authMode: input.authMode ?? "bearer", sleep: defaultSleep, }; } diff --git a/src/index.ts b/src/index.ts index 984eb5b..2e91f32 100644 --- a/src/index.ts +++ b/src/index.ts @@ -1,5 +1,6 @@ export { MemoryClient } from "./client.js"; export type { MemoryClientOptions } from "./client.js"; +export type { AuthMode } from "./http.js"; export { Memories, renderMemoriesPrompt, DEFAULT_PROMPT_TEMPLATE } from "./memories.js"; export type { IngestOptions, RequestContext } from "./memories.js"; @@ -9,6 +10,9 @@ export { Groups } from "./groups.js"; export { Jobs } from "./jobs.js"; export type { PollOptions } from "./jobs.js"; +export { f } from "./filter.js"; +export type { Clause, Comparable, FieldOps } from "./filter.js"; + export { MemoryError, BadRequest, @@ -21,6 +25,8 @@ export { ServerError, } from "./errors.js"; +export type { RateLimitSnapshot } from "./errors.js"; + export type { ApiErrorBody, ArtifactDetails, @@ -42,6 +48,7 @@ export type { ListEnvelope, ListQuery, Memory, + MemoryPatchRequest, MemoryRef, MemoryStatus, MemoryType, diff --git a/src/memories-patch.test.ts b/src/memories-patch.test.ts new file mode 100644 index 0000000..abd7892 --- /dev/null +++ b/src/memories-patch.test.ts @@ -0,0 +1,274 @@ +import { describe, it, expect, vi } from "vitest"; +import { Memories } from "./memories.js"; +import { HttpClient } from "./http.js"; +import type { Memory } from "./types.js"; +import { Unprocessable } from "./errors.js"; + +/** + * Build a real {@link HttpClient} whose `fetch` returns a single crafted + * `Response`, so a call drives the *production* error path + * (`http.toError → parseErrorBody → errorForStatus → Unprocessable`) rather + * than a hand-built error. Mirrors the `clientReturning` helper in + * `errors.test.ts`. `maxRetries: 0` so a 422 is not retried and the first + * non-ok response throws immediately. + */ +function clientReturning(opts: { + status: number; + body?: unknown; + headers?: Record; +}): HttpClient { + const fetchImpl = (async () => { + const text = opts.body === undefined ? "" : JSON.stringify(opts.body); + return new Response(text, { + status: opts.status, + headers: { "content-type": "application/json", ...opts.headers }, + }); + }) as unknown as typeof globalThis.fetch; + + return new HttpClient({ + apiKey: "k", + orgId: "o", + baseUrl: "https://api.test.local", + fetch: fetchImpl, + maxRetries: 0, + sleep: async () => {}, + }); +} + +/** Minimal fact-shaped Memory for fixtures; override `group_ids` per case. */ +function mem(id: string, group_ids: string[] = []): Memory { + return { + id, + object: "memory", + type: "fact", + text: `text-${id}`, + user_id: null, + agent_id: null, + conv_id: null, + app_id: null, + group_ids, + categories: [], + score: null, + created_at: "2026-01-01T00:00:00Z", + updated_at: "2026-01-01T00:00:00Z", + details: { + fact_type: null, + status: null, + supersedes: null, + source_role: null, + episode_id: null, + artifact_id: null, + artifact_ids: [], + source_event_ids: [], + }, + } as Memory; +} + +/** + * Fake HttpClient capturing every (method, path, body, signal, requestId) and + * returning a caller-supplied `Memory` as the response body. Mirrors the live + * `PATCH /v1/memories/{id}` happy path captured in gitban card 2syddu + * ("A4b HAPPY-PATH CAPTURED": 200 + full Memory, group_ids reflecting the edit). + */ +function fakeHttp(responder: () => Memory): { + http: HttpClient; + request: ReturnType; + calls: Array<{ + method: string; + path: string; + body: unknown; + signal: unknown; + requestId: unknown; + }>; +} { + const calls: Array<{ + method: string; + path: string; + body: unknown; + signal: unknown; + requestId: unknown; + }> = []; + const request = vi.fn( + async ( + method: string, + path: string, + options: { body?: unknown; signal?: unknown; requestId?: unknown } = {}, + ) => { + calls.push({ + method, + path, + body: options.body, + signal: options.signal, + requestId: options.requestId, + }); + return { body: responder(), status: 200, requestId: "req_test", rateLimit: undefined }; + }, + ); + const http = { request } as unknown as HttpClient; + return { http, request, calls }; +} + +describe("memories.patch — group-membership editing (A4b, card 2syddu)", () => { + it("add: PATCHes /v1/memories/{id} with only add_group_ids, resolves to the updated Memory", async () => { + const { http, calls } = fakeHttp(() => mem("mem_1", ["grp_x"])); + const memories = new Memories(http); + + const out = await memories.patch("mem_1", { add_group_ids: ["grp_x"] }); + + expect(calls).toHaveLength(1); + expect(calls[0]).toMatchObject({ + method: "PATCH", + path: "/v1/memories/mem_1", + body: { add_group_ids: ["grp_x"] }, + }); + // no remove_group_ids key on the wire when its array is empty/absent + expect(calls[0]!.body).not.toHaveProperty("remove_group_ids"); + expect(out.group_ids).toEqual(["grp_x"]); + }); + + it("remove: sends only remove_group_ids, resolves to a Memory with the membership gone", async () => { + const { http, calls } = fakeHttp(() => mem("mem_1", [])); + const memories = new Memories(http); + + const out = await memories.patch("mem_1", { remove_group_ids: ["grp_x"] }); + + expect(calls[0]).toMatchObject({ + method: "PATCH", + path: "/v1/memories/mem_1", + body: { remove_group_ids: ["grp_x"] }, + }); + expect(calls[0]!.body).not.toHaveProperty("add_group_ids"); + expect(out.group_ids).toEqual([]); + }); + + it("both: sends both arrays on the wire", async () => { + const { http, calls } = fakeHttp(() => mem("mem_1", ["a"])); + const memories = new Memories(http); + + await memories.patch("mem_1", { add_group_ids: ["a"], remove_group_ids: ["b"] }); + + expect(calls[0]!.body).toEqual({ add_group_ids: ["a"], remove_group_ids: ["b"] }); + }); + + it("url-encodes the id in the path", async () => { + const { http, calls } = fakeHttp(() => mem("weird id", ["g"])); + const memories = new Memories(http); + + await memories.patch("weird id", { add_group_ids: ["g"] }); + + expect(calls[0]!.path).toBe("/v1/memories/weird%20id"); + }); + + it("empty patch ({}) throws synchronously BEFORE any request() call", async () => { + const { http, request } = fakeHttp(() => mem("mem_1")); + const memories = new Memories(http); + + await expect(memories.patch("mem_1", {})).rejects.toThrow( + /add_group_ids.*remove_group_ids.*empty_patch|empty_patch/i, + ); + const msg = await memories.patch("mem_1", {}).catch((e: Error) => e.message); + expect(msg).toContain("add_group_ids"); + expect(msg).toContain("remove_group_ids"); + expect(msg).toContain("empty_patch"); + expect(request).not.toHaveBeenCalled(); + }); + + it("both arrays empty is treated as an empty patch and throws before request()", async () => { + const { http, request } = fakeHttp(() => mem("mem_1")); + const memories = new Memories(http); + + await expect( + memories.patch("mem_1", { add_group_ids: [], remove_group_ids: [] }), + ).rejects.toThrow(/empty_patch/i); + expect(request).not.toHaveBeenCalled(); + }); + + it("threads context.requestId and context.signal into request()", async () => { + const { http, calls } = fakeHttp(() => mem("mem_1", ["grp_x"])); + const memories = new Memories(http); + const controller = new AbortController(); + + await memories.patch( + "mem_1", + { add_group_ids: ["grp_x"] }, + { requestId: "req_custom", signal: controller.signal }, + ); + + expect(calls[0]!.requestId).toBe("req_custom"); + expect(calls[0]!.signal).toBe(controller.signal); + }); + + it("defensive: a server 422 empty_patch surfaces as Unprocessable carrying code 'empty_patch'", async () => { + // The client guard normally prevents this, but document the server contract + // captured in card 2syddu: an empty patch the server sees → 422 empty_patch. + const request = vi.fn(async () => { + throw new Unprocessable({ + message: "Provide add_group_ids and/or remove_group_ids", + status: 422, + errorType: "invalid_request_error", + code: "empty_patch", + }); + }); + const http = { request } as unknown as HttpClient; + const memories = new Memories(http); + + // Bypass the client guard with a populated array so the (mocked) server path runs. + await expect(memories.patch("mem_1", { add_group_ids: ["grp_x"] })).rejects.toMatchObject({ + code: "empty_patch", + }); + }); + + // The defensive test above documents the contract by hand-building an + // `Unprocessable`. The two below instead drive a *real* 422 wire body through + // the production error path (`http.toError → parseErrorBody → errorForStatus`) + // via a real HttpClient, proving the SDK's own parsing extracts + // `code: "empty_patch"` from the live envelope — one assertion per wire shape + // the parser supports (legacy `{error:{…}}` and spec `{detail:{…}}`). + it("drives the live empty_patch 422 envelope through the production parser — legacy {error:{…}} shape", async () => { + // The exact shape captured in card 2syddu's "A4b" section (legacy-first). + const http = clientReturning({ + status: 422, + body: { + error: { + type: "invalid_request_error", + code: "empty_patch", + message: "Provide add_group_ids and/or remove_group_ids", + }, + }, + }); + const memories = new Memories(http); + + // Populated array bypasses the client guard so the wire/parse path runs. + const err = await memories + .patch("mem_1", { add_group_ids: ["grp_x"] }) + .then(() => { + throw new Error("expected patch to reject"); + }) + .catch((e: unknown) => e); + + expect(err).toBeInstanceOf(Unprocessable); + expect(err).toMatchObject({ + status: 422, + code: "empty_patch", + errorType: "invalid_request_error", + }); + }); + + it("drives the live empty_patch 422 envelope through the production parser — spec {detail:{…}} shape", async () => { + const http = clientReturning({ + status: 422, + body: { detail: { code: "empty_patch", message: "Provide add_group_ids and/or remove_group_ids" } }, + }); + const memories = new Memories(http); + + const err = await memories + .patch("mem_1", { remove_group_ids: ["grp_x"] }) + .then(() => { + throw new Error("expected patch to reject"); + }) + .catch((e: unknown) => e); + + expect(err).toBeInstanceOf(Unprocessable); + expect(err).toMatchObject({ status: 422, code: "empty_patch" }); + }); +}); diff --git a/src/memories.ts b/src/memories.ts index e6ea2ba..ac88493 100644 --- a/src/memories.ts +++ b/src/memories.ts @@ -2,11 +2,13 @@ import type { HttpClient } from "./http.js"; import { Jobs } from "./jobs.js"; import type { IngestJob, + IngestJobResult, IngestRequest, GroupListEnvelope, ListEnvelope, ListQuery, Memory, + MemoryPatchRequest, PromptTemplate, RecallParams, RecallResult, @@ -223,11 +225,68 @@ export class Memories { return body; } + /** + * Resolve a superseded fact to its replacement. + * + * When an ingest supersedes an existing fact, the server reports the old → + * new id mapping in {@link IngestJobResult.memories_superseded_by} (and only + * there — there is no global "what replaced id X?" endpoint). This helper + * follows that map for you: pass the `IngestJobResult` and the old id, and it + * returns the replacement `Memory` (fetched via {@link get}), or `null` when + * `oldId` was not superseded in this ingest. + * + * Takes the result explicitly rather than pretending old ids are globally + * queryable — the superseded map lives only on the job result. See + * ADR-002 (C2) for the rationale. + * + * ```ts + * const done = await client.memories.jobs.pollUntilDone(job.id); + * const replacement = await client.memories.resolveSuperseded(done.result!, oldId); + * // replacement is the new Memory, or null if oldId wasn't superseded + * ``` + */ + async resolveSuperseded( + result: IngestJobResult, + oldId: string, + context: RequestContext = {}, + ): Promise { + const newId = result.memories_superseded_by?.[oldId]; + return newId ? this.get(newId, context) : null; + } + + /** + * Batch twin of {@link resolveSuperseded}: resolve **every** fact this ingest + * superseded to its replacement in one call. Returns a `Map` keyed by the old + * id whose value is the replacement `Memory`. An ingest that superseded + * nothing yields an empty map. + * + * The per-id `get()` fetches run in parallel. Like {@link resolveSuperseded}, + * it reads the mapping from {@link IngestJobResult.memories_superseded_by}. + * + * ```ts + * const replacements = await client.memories.resolveAllSuperseded(done.result!); + * for (const [oldId, replacement] of replacements) { + * // ... + * } + * ``` + */ + async resolveAllSuperseded( + result: IngestJobResult, + context: RequestContext = {}, + ): Promise> { + const entries = Object.entries(result.memories_superseded_by ?? {}); + const resolved = await Promise.all( + entries.map(async ([oldId, newId]) => [oldId, await this.get(newId, context)] as const), + ); + return new Map(resolved); + } + /** * Hard-delete a memory by id. The point is removed outright (no tombstone): * afterwards `get` returns 404, it's gone from list/search, and a second - * delete returns 404 (idempotent by absence). Corrections flow through - * ingest, not a PATCH — the memory API has no update endpoint. + * delete returns 404 (idempotent by absence). PATCH exists only for editing a + * memory's group membership (see {@link patch}); content corrections flow + * through ingest/supersede, not an in-place text update. */ async delete(id: string, context: RequestContext = {}): Promise { await this.http.request("DELETE", `/v1/memories/${encodeURIComponent(id)}`, { @@ -236,6 +295,54 @@ export class Memories { }); } + /** + * Edit a memory's group membership — the **only** in-place mutate op on a + * memory. Add and/or remove group tags via + * `{ add_group_ids?, remove_group_ids? }`; the server returns the full updated + * {@link Memory} with its new `group_ids`. This is the live + * `PATCH /v1/memories/{id}` contract verified against the deployed API + * (gitban card 2syddu); the old `{ text, metadata }` update the spec once + * documented is gone — content corrections flow through ingest/supersede. + * + * At least one array must carry an entry. An empty patch (`{}`, or both arrays + * empty) throws **before** any request — mirroring the server's + * `422 empty_patch` and saving a doomed round-trip (same loud-throw style as + * {@link recall}). An empty array is treated like an absent field and is not + * sent on the wire, so only the populated arrays appear in the request body. + * + * ```ts + * // add to a group: + * const m = await client.memories.patch("mem_1", { add_group_ids: ["grp_x"] }); + * m.group_ids; // ["grp_x"] + * // remove from a group: + * await client.memories.patch("mem_1", { remove_group_ids: ["grp_x"] }); + * ``` + */ + async patch( + id: string, + changes: MemoryPatchRequest, + context: RequestContext = {}, + ): Promise { + const add = changes.add_group_ids ?? []; + const remove = changes.remove_group_ids ?? []; + if (add.length === 0 && remove.length === 0) { + throw new Error( + "memories.patch(): provide at least one of add_group_ids or remove_group_ids " + + "(the server rejects an empty patch with 422 empty_patch)", + ); + } + const body: MemoryPatchRequest = {}; + if (add.length > 0) body.add_group_ids = add; + if (remove.length > 0) body.remove_group_ids = remove; + + const { body: res } = await this.http.request( + "PATCH", + `/v1/memories/${encodeURIComponent(id)}`, + { body, signal: context.signal, requestId: context.requestId }, + ); + return res; + } + /** Vector + filter search. */ async search(body: SearchRequest, context: RequestContext = {}): Promise { const { body: response } = await this.http.request("POST", "/v1/memories/search", { @@ -246,6 +353,35 @@ export class Memories { return response; } + /** + * Iterate every memory matching a search query, auto-paginating until the + * server says `has_more: false`. The async-generator twin of {@link list}, + * for {@link search} instead of the list endpoint: + * + * ```ts + * for await (const m of client.memories.searchAll({ query: "q", user_id: "alice" })) { + * // ... + * } + * ``` + * + * Each page is a fresh `{ ...body, cursor }` spread, so the caller's `body` + * object is never mutated — the same request can be reused across calls. + * + * For a stable full sweep, prefer `mode: "retrieve"` (the raw vector-ranked + * set): the default `"compose"` re-runs the per-page LLM selection pass, so a + * cursor threaded across compose pages can re-rank between pages and re-incurs + * the compose cost on every fetch. + */ + async *searchAll(body: SearchRequest, context: RequestContext = {}): AsyncGenerator { + let cursor = body.cursor; + while (true) { + const env = await this.search({ ...body, cursor }, context); + for (const memory of env.data) yield memory; + if (!env.has_more || !env.next_cursor) return; + cursor = env.next_cursor; + } + } + /** * Sugar over {@link search} that forces `mode: "compose"` — the response's * `context` carries xmem's LLM-assembled, ready-to-inject prompt (and `data` @@ -281,6 +417,11 @@ export class Memories { * Pass `pools` — the scopes to union. Axes within a pool AND; pools OR. At * least one pool (each with ≥1 axis) is required. * + * Pass `include: ["full_content"]` to fetch heavy artifact bodies on the + * returned rows in the same round-trip — it's forwarded to every per-pool + * search, so `ArtifactDetails.full_content` is populated on the merged set + * without follow-up `get()` calls. + * * @example * // personal + a group ("my stuff OR the trip's stuff"): * const { prompt } = await client.memories.recall({ @@ -313,7 +454,7 @@ export class Memories { ) => string; } & RequestContext = {}, ): Promise { - const { query } = params; + const { query, include } = params; const mode = params.mode ?? "compose"; const limit = params.limit ?? 10; @@ -364,9 +505,15 @@ export class Memories { const poolLabel = (p: ScopePool): string => p.group_ids && p.group_ids.length > 0 ? "shared" : p.user_id ? "personal" : "scope"; + // `include` (full_content only — KD-5) rides on EVERY per-pool search so the + // merged rows are uniformly enriched. Spread it only when it carries fields: + // omitting the option — or passing an empty `include: []` (truthy, but a + // no-op) — must leave the wire request byte-identical to today's. `...pool` + // carries no `include` axis, so there's no collision. + const includeFields = include && include.length > 0 ? { include } : {}; const jobs = pools.map((pool) => ({ pool, - promise: this.search({ query, mode, limit, ...pool }, ctx), + promise: this.search({ query, mode, limit, ...includeFields, ...pool }, ctx), })); // Resolve group id → name from the registry when any pool targets a group, diff --git a/src/recall.test.ts b/src/recall.test.ts index ca38ddf..21e0231 100644 --- a/src/recall.test.ts +++ b/src/recall.test.ts @@ -507,3 +507,94 @@ describe("Memories.recall — pools (general union)", () => { expect(res.prompt).not.toContain("you:"); // no group sections here }); }); + +describe("Memories.recall — include[] threading (B2 / KD-5)", () => { + it("forwards include:['full_content'] to EVERY per-pool search body", async () => { + const { http, calls } = fakeHttp({ onSearch: () => [] }); + await new Memories(http).recall({ + query: "q", + pools: [{ user_id: "alice" }, { group_ids: ["grp_x"] }, { app_id: "kb" }], + include: ["full_content"], + }); + // capstone: every pool's search body must carry the include verbatim + expect(calls).toHaveLength(3); + expect(calls.every((c) => Array.isArray(c.include) && c.include[0] === "full_content")).toBe( + true, + ); + for (const c of calls) expect(c.include).toEqual(["full_content"]); + }); + + it("omitting include leaves no `include` key on any pool body (unchanged behaviour)", async () => { + const { http, calls } = fakeHttp({ onSearch: () => [] }); + await new Memories(http).recall({ + query: "q", + pools: [{ user_id: "alice" }, { group_ids: ["grp_x"] }], + }); + expect(calls).toHaveLength(2); + // body must not even carry the key — not `include: undefined` + for (const c of calls) { + expect("include" in c).toBe(false); + expect(c.include).toBeUndefined(); + } + }); + + it("the per-pool scope axes still win over any stray include collision", async () => { + // include rides alongside the spread scope; `...pool` carries no include, so + // the pool axes and the include coexist on each body without clobbering. + const { http, calls } = fakeHttp({ onSearch: () => [] }); + await new Memories(http).recall({ + query: "q", + pools: [{ user_id: "alice", agent_id: "bot" }], + include: ["full_content"], + }); + expect(calls).toHaveLength(1); + expect(calls[0]!.user_id).toBe("alice"); + expect(calls[0]!.agent_id).toBe("bot"); + expect(calls[0]!.include).toEqual(["full_content"]); + }); + + it("rejects context_prompt on RecallParams.include at compile time (KD-5)", async () => { + const { http, calls } = fakeHttp({ onSearch: () => [] }); + await new Memories(http).recall({ + query: "q", + pools: [{ user_id: "alice" }], + // @ts-expect-error — recall scopes include to "full_content" only; recall + // discards per-pool envelopes, so context_prompt has no output channel. + include: ["context_prompt"], + }); + // it still runs (the value is forwarded verbatim) — the contract is the + // compile-time rejection above, enforced by @ts-expect-error under typecheck. + expect(calls).toHaveLength(1); + }); + + it("full_content is reachable on a returned artifact row (typed accessor)", async () => { + const { http } = fakeHttp({ + onSearch: () => [ + mem("ART", "5-day plan body", 0.9, { + type: "artifact", + details: { + title: "Itinerary", + rationale: null, + version: null, + root_id: null, + source_fact_ids: [], + episode_ids: [], + full_content: "the entire artifact body text", + }, + }), + ], + }); + const res = await new Memories(http).recall({ + query: "q", + pools: [{ user_id: "alice" }], + include: ["full_content"], + }); + const row = res.memories.find((m) => m.id === "ART")!; + // compile-time + runtime: full_content lives on the artifact's details + if (row.type === "artifact") { + expect(row.details.full_content).toBe("the entire artifact body text"); + } else { + throw new Error("expected an artifact row"); + } + }); +}); diff --git a/src/search-all.test.ts b/src/search-all.test.ts new file mode 100644 index 0000000..53adab4 --- /dev/null +++ b/src/search-all.test.ts @@ -0,0 +1,127 @@ +import { describe, it, expect } from "vitest"; +import { Memories } from "./memories.js"; +import type { HttpClient } from "./http.js"; +import type { Memory, SearchListEnvelope, SearchRequest } from "./types.js"; + +/** Minimal fact-shaped Memory for fixtures. */ +function mem(id: string): Memory { + return { + id, + object: "memory", + type: "fact", + text: `text-${id}`, + user_id: null, + agent_id: null, + conv_id: null, + app_id: null, + group_ids: [], + categories: [], + score: null, + created_at: "2026-01-01T00:00:00Z", + updated_at: "2026-01-01T00:00:00Z", + details: { + fact_type: null, + status: null, + supersedes: null, + source_role: null, + episode_id: null, + artifact_id: null, + artifact_ids: [], + source_event_ids: [], + }, + } as Memory; +} + +/** + * Fake HttpClient that replays a scripted sequence of `SearchListEnvelope`s for + * successive `POST /v1/memories/search` calls. Records each request body so a + * test can assert how the cursor was threaded. + */ +function scriptedHttp(pages: SearchListEnvelope[]): { + http: HttpClient; + calls: SearchRequest[]; +} { + const calls: SearchRequest[] = []; + let i = 0; + const http = { + request: async (_method: string, _path: string, options: { body?: SearchRequest } = {}) => { + calls.push(options.body as SearchRequest); + const body = pages[i++]; + if (!body) throw new Error("scriptedHttp: more search calls than scripted pages"); + return { body, status: 200, requestId: "req_test" }; + }, + } as unknown as HttpClient; + return { http, calls }; +} + +function page(ids: string[], has_more: boolean, next_cursor: string | null): SearchListEnvelope { + return { object: "list", data: ids.map(mem), has_more, next_cursor }; +} + +describe("Memories.searchAll", () => { + it("yields every row across multiple pages in order, then completes", async () => { + const { http, calls } = scriptedHttp([ + page(["A", "B"], true, "cur1"), + page(["C", "D"], true, "cur2"), + page(["E"], false, null), + ]); + const body: SearchRequest = { query: "q", user_id: "alice" }; + + const seen: string[] = []; + for await (const m of new Memories(http).searchAll(body)) seen.push(m.id); + + expect(seen).toEqual(["A", "B", "C", "D", "E"]); + // three pages requested; cursor threaded from each page's next_cursor + expect(calls).toHaveLength(3); + expect(calls[0]!.cursor).toBeUndefined(); + expect(calls[1]!.cursor).toBe("cur1"); + expect(calls[2]!.cursor).toBe("cur2"); + }); + + it("stops after a single page when has_more is false", async () => { + const { http, calls } = scriptedHttp([page(["A"], false, null)]); + + const seen: string[] = []; + for await (const m of new Memories(http).searchAll({ query: "q" })) seen.push(m.id); + + expect(seen).toEqual(["A"]); + expect(calls).toHaveLength(1); + }); + + it("stops when next_cursor is null even if has_more is true (defensive)", async () => { + const { http, calls } = scriptedHttp([page(["A"], true, null)]); + + const seen: string[] = []; + for await (const m of new Memories(http).searchAll({ query: "q" })) seen.push(m.id); + + expect(seen).toEqual(["A"]); + expect(calls).toHaveLength(1); + }); + + it("does not mutate the caller's body object", async () => { + const { http } = scriptedHttp([ + page(["A"], true, "cur1"), + page(["B"], false, null), + ]); + const body: SearchRequest = { query: "q", user_id: "alice" }; + const snapshot = structuredClone(body); + + // drain the generator + for await (const _m of new Memories(http).searchAll(body)) { + void _m; + } + + expect(body).toEqual(snapshot); + expect("cursor" in body).toBe(false); + }); + + it("honors an explicit starting cursor on the first request", async () => { + const { http, calls } = scriptedHttp([page(["A"], false, null)]); + + const seen: string[] = []; + for await (const m of new Memories(http).searchAll({ query: "q", cursor: "start" })) seen.push(m.id); + + expect(seen).toEqual(["A"]); + expect(calls[0]!.cursor).toBe("start"); + }); +}); diff --git a/src/superseded.test.ts b/src/superseded.test.ts new file mode 100644 index 0000000..ac16bbd --- /dev/null +++ b/src/superseded.test.ts @@ -0,0 +1,137 @@ +import { describe, it, expect } from "vitest"; +import { Memories } from "./memories.js"; +import type { HttpClient } from "./http.js"; +import type { IngestJobResult, Memory } from "./types.js"; + +/** Minimal fact-shaped Memory for fixtures. */ +function mem(id: string): Memory { + return { + id, + object: "memory", + type: "fact", + text: `text-${id}`, + user_id: null, + agent_id: null, + conv_id: null, + app_id: null, + group_ids: [], + categories: [], + score: null, + created_at: "2026-01-01T00:00:00Z", + updated_at: "2026-01-01T00:00:00Z", + details: { + fact_type: null, + status: null, + supersedes: null, + source_role: null, + episode_id: null, + artifact_id: null, + artifact_ids: [], + source_event_ids: [], + }, + } as Memory; +} + +/** Build an IngestJobResult carrying only the superseded map (rest is filler). */ +function result(superseded: Record): IngestJobResult { + return { + memories_created: [], + memories_updated: [], + memories_superseded_by: superseded, + stage_timings: {}, + ignored_group_ids: [], + }; +} + +/** + * Fake HttpClient that serves `GET /v1/memories/{id}` from a fixture map and + * records every requested path so a test can assert which ids were fetched. + * A 404 (unknown id) throws, mirroring the real client. + */ +function fakeHttp(byId: Record): { + http: HttpClient; + paths: string[]; +} { + const paths: string[] = []; + const http = { + request: async (_method: string, path: string) => { + paths.push(path); + // path is /v1/memories/{encodedId} + const id = decodeURIComponent(path.slice("/v1/memories/".length)); + const found = byId[id]; + if (!found) throw new Error(`404: ${id}`); + return { body: found, status: 200, requestId: "req_test" }; + }, + } as unknown as HttpClient; + return { http, paths }; +} + +describe("Memories.resolveSuperseded", () => { + it("returns the replacement Memory when oldId is in the superseded map", async () => { + const { http, paths } = fakeHttp({ new1: mem("new1") }); + const memories = new Memories(http); + + const replacement = await memories.resolveSuperseded(result({ old1: "new1" }), "old1"); + + expect(replacement).not.toBeNull(); + expect(replacement!.id).toBe("new1"); + // followed the map: old1 → new1, fetched via get(new1) + expect(paths).toEqual(["/v1/memories/new1"]); + }); + + it("returns null when oldId is not superseded (no fetch)", async () => { + const { http, paths } = fakeHttp({ new1: mem("new1") }); + const memories = new Memories(http); + + const replacement = await memories.resolveSuperseded(result({ old1: "new1" }), "unknown"); + + expect(replacement).toBeNull(); + // a non-superseded id never hits the network + expect(paths).toEqual([]); + }); + + it("returns null when the superseded map is empty", async () => { + const { http, paths } = fakeHttp({}); + const memories = new Memories(http); + + const replacement = await memories.resolveSuperseded(result({}), "old1"); + + expect(replacement).toBeNull(); + expect(paths).toEqual([]); + }); + + it("url-encodes the replacement id in the get() path", async () => { + const { http, paths } = fakeHttp({ "new id": mem("new id") }); + const memories = new Memories(http); + + const replacement = await memories.resolveSuperseded(result({ old1: "new id" }), "old1"); + + expect(replacement!.id).toBe("new id"); + expect(paths).toEqual(["/v1/memories/new%20id"]); + }); +}); + +describe("Memories.resolveAllSuperseded", () => { + it("resolves every superseded old id to its replacement Memory", async () => { + const { http, paths } = fakeHttp({ newA: mem("newA"), newB: mem("newB") }); + const memories = new Memories(http); + + const map = await memories.resolveAllSuperseded(result({ oldA: "newA", oldB: "newB" })); + + expect(map.size).toBe(2); + expect(map.get("oldA")!.id).toBe("newA"); + expect(map.get("oldB")!.id).toBe("newB"); + // one get() per superseded entry + expect(paths.sort()).toEqual(["/v1/memories/newA", "/v1/memories/newB"]); + }); + + it("returns an empty map when nothing was superseded", async () => { + const { http, paths } = fakeHttp({}); + const memories = new Memories(http); + + const map = await memories.resolveAllSuperseded(result({})); + + expect(map.size).toBe(0); + expect(paths).toEqual([]); + }); +}); diff --git a/src/types.ts b/src/types.ts index 169102f..af75aaf 100644 --- a/src/types.ts +++ b/src/types.ts @@ -1,5 +1,8 @@ -// Hand-written types matching openapi/memory_v2.json. -// Regenerate the canonical version with `npm run gen:types` once the spec is final. +// Hand-authored domain types — the canonical public surface (ADR-002). +// The spec-derived reference lives at src/generated/types.ts; regenerate it +// with `npm run gen:types` (source: spec/memory.json) and keep it in sync via +// `npm run check:types-sync`. The generated file is a reference, not the +// public surface — these hand-authored types are. export type MemoryType = "fact" | "artifact" | "episode"; @@ -71,6 +74,14 @@ interface MemoryBase { created_at: string; updated_at: string; details: D; + /** + * **Deferred for 1.0** — typed but inert. No request-side `expand` parameter + * exists on search/list/recall yet, so the server has no trigger to populate + * this and it stays `undefined` in practice. The field is kept as a + * forward-looking contract; it will be wired when the API documents an + * `expand` request shape. See ADR-002 (C2: `Memory.expanded` / `expand` → + * defer, no documented request trigger). + */ expanded?: Record; } @@ -199,9 +210,17 @@ export interface SearchRequest { cursor?: string | null; include?: Array<"context_prompt" | "full_content">; /** - * @deprecated Legacy pre-#68 wire shape. The server still lifts - * `user_id` / `agent_id` / `app_id` out of here, but new code should use - * the top-level fields above. + * Filter DSL over indexed payload keys — operators (`$eq`/`$in`/`$gt`/…), + * `null` for "unset", and `AND`/`OR`/`NOT` composition; top-level keys are + * implicit-AND'd. Build it with the typed `f` builder (`src/filter.ts`) or + * pass a raw {@link Filter} object. This operator-DSL use is **supported and + * not deprecated**. + * + * @deprecated *only* for lifting the scope axes + * (`user_id` / `agent_id` / `app_id`) out of here — that legacy pre-#68 form + * still works (the server lifts them) but new code should set those as the + * top-level fields above. Filtering on non-scope payload keys via the + * operator DSL is the supported, non-deprecated use of this field. */ filters?: Filter; } @@ -249,6 +268,22 @@ export interface RecallParams { mode?: SearchMode; /** Cap on the merged, deduped result. Default 10. */ limit?: number; + /** + * Heavy fields to fetch on the returned rows. Forwarded to **every** per-pool + * search, so a single recall enriches the merged set without follow-up `get()` + * calls — e.g. `include: ["full_content"]` makes `ArtifactDetails.full_content` + * available on artifact rows. + * + * Scoped to `"full_content"` only (NOT the `"context_prompt"` that + * {@link SearchRequest} accepts): recall discards each pool's + * {@link SearchListEnvelope} — it dedupes/re-ranks the rows and renders **one** + * client-side prompt, returning `{ memories, prompt, scopes }` with no per-pool + * `extras` channel — so a requested `context_prompt` would only make the server + * assemble prompts recall then throws away. Use {@link Memories.search} / + * {@link Memories.retrieve} (which return the envelope intact) when you need the + * per-scope `context_prompt`. + */ + include?: Array<"full_content">; } /** Per-pool diagnostic returned by {@link Memories.recall}. */ @@ -279,6 +314,15 @@ export interface RecallResult { * Note: a template controls *formatting of the fields each row carries*. It can't * reproduce xmem features that need extra data (authorship sections, full * artifact bodies, char-budgeting) without also widening the search payload. + * + * **Server-supplied templates are deferred for 1.0.** The type is fully usable + * today as a client-side override (`recall(..., { template })`), but the + * "future API endpoint returns xmem's preferred template" path above is + * **not yet implemented** — no server endpoint returns a `PromptTemplate`, so + * the SDK ships the {@link DEFAULT_PROMPT_TEMPLATE} and does not fetch one. The + * type stays as a forward contract; the fetch/cache path is wired when the + * server exposes it. See ADR-002 (C2: server-supplied `PromptTemplate` → defer, + * no server endpoint; client default suffices). */ export interface PromptTemplate { /** Leading line of the block. */ @@ -331,6 +375,25 @@ export interface GroupUpdateRequest { status?: GroupStatus; } +/** + * Body for `memories.patch(id, …)` — the **only** in-place mutate operation on a + * memory. The live `PATCH /v1/memories/{id}` contract is group-membership + * editing (verified against the deployed API; see gitban card 2syddu): you add + * and/or remove a memory's group tags and get the full updated {@link Memory} + * back. The old `{ text, metadata }` "UpdateRequest" the spec once documented is + * **gone** — content corrections flow through ingest/supersede, not PATCH. + * + * At least one of the two arrays must carry an entry; an empty patch is rejected + * client-side (and would be a `422 empty_patch` server-side). An empty array is + * treated the same as an absent field and is not sent on the wire. + */ +export interface MemoryPatchRequest { + /** Group ids to add to the memory's membership. */ + add_group_ids?: string[]; + /** Group ids to remove from the memory's membership. */ + remove_group_ids?: string[]; +} + export interface GroupListEnvelope { object: "list"; data: Group[];