From 6e9628b07330cea41f5aa83e52b486b1b47c993d Mon Sep 17 00:00:00 2001 From: Josh Knapp Date: Tue, 11 Aug 2026 15:10:36 -0700 Subject: [PATCH] docs: record the memory API decisions, including the one declined MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Keeps the reasoning behind PR #19 next to the code, since none of it is recoverable from the diff: why memory_get stopped returning the embedding and tsvector, why memory_patch refuses ambiguous matches rather than picking one, and why memory_append was dropped as redundant with patch. Also records P3 (mechanising file->memory mirroring) as DECLINED with its reasoning and, more usefully, the condition that would reopen it — the mirror going stale again now that patching is cheap. The evidence we had pointed at edit cost, which P2 fixed; if drift recurs the cause was attention instead, and the answer is probably to remove the duplication rather than build a drift detector for it. Notes two traps for anyone extending this: content_tsv is a generated column so full-text search cannot rot after a patch (only the embedding needs recomputing), and the obvious "does search find the patched text" acceptance check therefore passes on an implementation that skips re-embedding entirely. The brief previously lived outside the repo. Moved rather than copied — two hand-maintained copies is the exact drift problem described in the document. Co-Authored-By: Claude Opus 5 (1M context) --- README.md | 14 ++ docs/memory-api-improvements.md | 262 ++++++++++++++++++++++++++++++++ 2 files changed, 276 insertions(+) create mode 100644 docs/memory-api-improvements.md diff --git a/README.md b/README.md index e67ffc1..dac2f06 100644 --- a/README.md +++ b/README.md @@ -655,6 +655,20 @@ the `lint` script is a cheap way to keep it that way. --- +## Design notes + +`docs/` holds decision records for changes whose reasoning isn't recoverable +from the diff — what was built, what was deliberately rejected, and what would +reopen a closed question. + +- [`docs/memory-api-improvements.md`](docs/memory-api-improvements.md) — why + `memory_get` stopped returning the embedding and tsvector, why `memory_patch` + refuses ambiguous matches instead of guessing, why `memory_append` was + dropped, and why file-mirroring was left as a convention rather than + mechanised. + +--- + ## Troubleshooting - **`401 claim invalid: aud`** from `/api/mcp` — your MCP client isn't diff --git a/docs/memory-api-improvements.md b/docs/memory-api-improvements.md new file mode 100644 index 0000000..0bf3db6 --- /dev/null +++ b/docs/memory-api-improvements.md @@ -0,0 +1,262 @@ +# Decision record: memory read payloads and patch-style updates + +**Status:** P1 shipped · P2 shipped · P3 declined +**Originated:** 2026-08-11, from live use of the deployed server +**Revised:** 2026-08-11, against source +**Closed:** 2026-08-11 — implemented in `feat/memory-patch-and-lean-get` (PR #19) + +This started as a proposal written from black-box observation of the deployed +server. It is kept as a decision record because the reasoning behind what was +built — and behind what was deliberately *not* built — is not recoverable from +the diff. + +--- + +## 0. Orientation + +Tools are defined internally with dots (`memory.get`) and surfaced to clients +with underscores (`memory_get`). Don't let it confuse a grep. + +### Data model + +- Memories have `id` (uuid), `content`, `tags[]`, `scope` (`project` | `user`), + `projectId`, `visibility`, `version`, `lastEditedBy`, `createdAt`, + `updatedAt`, `deletedAt`, plus `userId`, `embedding`, and `contentTsv` + (`apps/web/lib/db/schema.ts`). +- `scope: project` requires `project`; `scope: user` requires it omitted. +- `version` is an optimistic-locking token, bumped on every successful update. + A stale `version` returns a concurrent-edit error. +- Content limit is 64,000 chars (`packages/schemas/src/index.ts`). +- Shared projects allow anyone with rw access to edit any memory — hence the + locking. + +--- + +## 1. P1 — `memory_get` returned the embedding and the tsvector ✅ SHIPPED + +**Problem.** `memory_get` used a bare `select()` and returned the raw DB row, +including the `embedding` vector and the `contentTsv` lexeme index. Both are +Postgres retrieval internals with zero value to a model consumer. +`memory_list` and `memory_search` already projected an explicit 9-field shape — +`memory_get` was the only outlier. + +**Measured against the live server** on a ~13k-char memory: + +| | chars | share | +|---|---|---| +| `content` | 27,662 | 44.1% | +| `contentTsv` | 29,912 | 47.7% | +| `embedding` | 4,688 | 7.5% | +| other 12 fields | 433 | 0.7% | +| **total** | **62,695** | | + +Two distinct failure shapes, which the original draft had flattened together: + +- `embedding` is a **fixed** ~4,690-char tax on every read — 384 dims + regardless of content length. Nearly invisible on large memories (7.5%), + dominant on small ones (~58% of an ~8k response). Most memories are small. +- `contentTsv` scales **super-linearly** with content (frequent lexemes + accumulate long position lists, ~21 chars/entry) and was the largest single + component of the large payload — larger than the content itself. + +**This was a correctness problem, not an efficiency nit.** Fetching that memory +exceeded the MCP tool-output cap and spilled to a file, even though `content` +alone is comfortably under the limit. `memory_get` was unusable on exactly the +large living documents P2 exists to serve. At the 64,000-char content ceiling a +response would land near 140,000 characters, under half of it content. + +**Shipped:** `memory_get` returns the same 9 fields as `memory_list` / +`memory_search`. `userId` is still selected for the authorization check and +stripped before responding. + +**Rejected: an `include: ("embedding" | "tsv")[]` opt-in.** The original draft +proposed gating the fields behind a flag in case some caller needed them. No +caller can: no MCP client can consume a 384-float vector or a lexeme index, and +the Web UI never goes through the MCP tools (it reads via `lib/memories.ts` and +writes via `lib/memory-actions.ts`). The parameter would have been dead on +arrival. + +--- + +## 2. P2 — patch-style updates ✅ SHIPPED + +**Problem.** `memory_update` accepted only full replacement. Adding four lines +to a 13,000-char living document meant reproducing the entire document. + +**The evidence this was a real blocker.** The WHP roadmap mirror was three +weeks and two shipped releases out of date. The agent that noticed **declined +to fix it**, on the grounds that hand-reproducing 13k characters of shared team +history to add one entry risked silently dropping some of it — a worse outcome +than leaving it stale. + +That is the failure mode this was designed against: **when the only safe way to +make a small edit is expensive, the edit doesn't happen.** + +**Shipped:** `memory_patch(id, old_string, new_string, version?)` + +| condition | behaviour | +|---|---| +| `old_string` absent | error — never a silent no-op | +| `old_string` matches >1 | error naming the count — ambiguity never resolves arbitrarily | +| matches exactly once | replace, bump `version`, re-embed | +| stale `version` | concurrent-edit error | + +Both failure modes refuse rather than clobber. That is the property that makes +the operation safe to hand to an agent editing a shared document it cannot +afford to corrupt. Semantics live in `lib/memory-patch.ts` as a pure function, +free of DB and auth, so both surfaces share them. + +**Locking came nearly free.** `memory.update` already computed +`expectedVersion = version ?? existing.version`, falling back to the version +read in the same handler. The CAS therefore already guarded the server-side +read-modify-write; patch copies that shape and is race-safe even when the +caller omits `version`. No explicit transaction was required. + +**Rejected: `memory_append` with a `section` parameter.** Proposed as sugar for +heading-structured logs. Patch already covers that case exactly — +`memory_patch(id, "## RECENTLY SHIPPED", "## RECENTLY SHIPPED\n- entry")` — and +does so *with* the uniqueness guarantee: if the heading appears twice you get an +error instead of an arbitrary insert. Implementing `section` would have meant +defining heading-match semantics, insert position within a section, and +duplicate-heading behaviour, for something patch handles for free. + +--- + +## 3. P3 — deriving mirrors rather than relying on convention ❌ DECLINED + +**The problem as stated.** Some memories mirror local files. The sync is +enforced only by a note in the file's own header: *"MIRRORED to shared-memory +MCP … when you update this file, also `memory_update` that record."* That +depends on whoever edits the file noticing the note and performing a second +write. It went three weeks without one. + +Proposed shapes were: a server-side `memory_sync_from_file`, a staleness signal +via `sourcePath` + content hash, or leaving it manual but cheap via P2. + +**Declined, 2026-08-11.** Three reasons, in order of weight: + +1. **The cause we have evidence for is now fixed.** The evidence was specific: + an agent *noticed* the drift and *declined* to fix it because the edit was + expensive and risky. That is a cost failure, not an attention failure. P2 + makes that edit a single call. We have direct evidence for the cost cause + and none yet for any other. + +2. **It would likely be a mechanism for N=1.** One mirror is known to exist. + A `sourcePath` column, hash computation, and staleness plumbing is real + schema-and-sync work; building it to police a single document is + disproportionate. + +3. **If drift recurs, the better fix probably isn't sync machinery.** P3 + assumes the mirror should exist and be kept honest. That assumption deserves + scrutiny first: the memory is a condensed prose rendition of a file that + lives in a container, existing separately only because the memory is + cross-machine and the file is not. Two hand-maintained sources of truth plus + a drift detector is strictly more machinery than one source of truth. The + cheaper answer would be to remove the duplication — make the memory + canonical and drop the file, or generate one from the other. + +**What would reopen this:** the roadmap going stale *again* now that patching +is cheap. That is the clean experiment and it costs nothing to run. If it +drifts again, the cause was attention rather than cost, and option 2 above — a +staleness signal that makes drift *visible* rather than trying to fix it +automatically — becomes worth its weight. + +Note for whoever picks this up: the mirror is **not** a byte copy of its local +file. It is a condensed prose rendition with different headings and no +wiki-links. A naive file-sync would destroy its established form. Any solution +has to preserve that distinction or deliberately abandon it. + +--- + +## 4. Structural work this depended on ✅ SHIPPED + +Neither of these was in the original proposal; both were found once the source +was available. + +### 4.1 The write path was duplicated + +`updateMemoryAction` (Web UI) and the `memory.update` MCP handler each +reimplemented authorize → mutate → re-embed → CAS → audit. Neither delegated to +a shared helper, and they had **drifted**: `memory.delete` over MCP skipped the +project ACL whenever the caller authored the row, so a memory written while a +share was `rw` stayed deletable by its author after an owner downgraded that +share to `ro`. `memory.update` and the entire Web UI always checked. + +Both surfaces now route through `lib/memory-mutations.ts`. Authoring a row +grants no standing write privilege on any path — the project ACL is the +authority, not the byline. This was a behaviour change, shipped deliberately, +and is covered by a test that fails against the old code. + +The one genuine difference between the surfaces is injected as a +`ProjectResolver`: MCP refuses an unknown project key (`call project.identify +first`) so an agent cannot spawn near-miss projects off a typo, while the Web +UI creates one, because a person typing a name into a form means to. + +### 4.2 There was no test infrastructure + +No vitest, no jest, no test files, no `test` script. Added vitest, with +integration tests running against a real Postgres rather than a mocked DB. +Setup is documented in the README. + +--- + +## 5. Invariants — preserve these + +- **Optimistic locking.** `version` must keep working, and every new mutating + primitive must accept it. Shared projects have concurrent editors. +- **Scope/project rules.** `scope: project` ⇒ `project` required and must + already exist on the MCP path; `scope: user` ⇒ `project` omitted. +- **Re-embedding on content change — and note what this does NOT cover.** + `content_tsv` is `GENERATED ALWAYS AS (to_tsvector('english', + coalesce(content, ''))) STORED` (`apps/web/drizzle/0000_init.sql`), so + Postgres maintains it and **full-text search cannot rot**. Only `embedding` + requires an explicit recompute. Any new mutation path must re-embed, or + *semantic* retrieval degrades silently while FTS keeps working — which is + exactly what makes the failure hard to notice. +- **Stable `id` across edits.** Never implement an edit as delete + recreate. +- **64,000-char content limit** enforced after a patch is applied, not just on + the incoming fragment. +- **Audit trail.** Partial edits record match offset and length delta, not just + the changed field names. + +--- + +## 6. Acceptance checks + +**P1** — verified in tests; the payload figures need a deploy to confirm. +- ✅ Response contains neither `embedding` nor `contentTsv`. +- ✅ Field set matches `memory_list` / `memory_search` exactly (9, down from 14). +- ⏳ Large specimen drops 62,695 → ~28,100 chars (−55%); small specimen ~−75%. +- ⏳ `memory_get` on `aaea192c-edce-4372-b011-5113a02dea16` returns inline + instead of spilling to a file. **This is the check that matters** — it is the + difference between the tool working and not working on large memories. + +**P2** — all verified against a real Postgres. +- ✅ Unique `old_string` → applied; `version` incremented by exactly 1. +- ✅ Absent `old_string` → error; content byte-identical afterward. +- ✅ `old_string` occurring twice → error naming the count; content unchanged. +- ✅ Stale `version` → concurrent-edit error; content unchanged. +- ✅ Content exceeding 64,000 chars post-patch → rejected. +- ✅ Re-embedding — see the caveat below. + +**A trap in the re-embedding check.** The original draft proposed "after a +patch, `memory_search` finds text introduced by that patch." *That test does not +work.* Search fuses three rankers via RRF, and because `content_tsv` is a +generated column the FTS ranker finds the literal inserted text **even if the +patch skipped re-embedding entirely** — it would pass on a broken +implementation. The shipped test asserts the **stored vector changed**, using a +deterministic per-text embedder stub. A separate test pins that `content_tsv` +updates itself, documenting why the naive check is misleading. + +--- + +## 7. Test specimen + +Memory `aaea192c-edce-4372-b011-5113a02dea16` (project +`cloud-hosting-platform/whp`, tags `roadmap` / `progress-tracker` / `planning`) +is a good real-world subject: ~13k chars, heading-structured, and its local +counterpart is +`/home/claude/.claude/projects/-workspace/memory/project_roadmap.md`. + +**It is live team data.** Check its `updatedAt` and `version` before using it as +a fixture, and prefer a scratch memory for destructive tests.