feat: knowledge entry versioning — DB-enforced snapshot+bump, If-Match concurrency, versions/diff

[andrei-hasna]

What this is

Entry versioning for knowledge: a version column, an append-only revisions table, a Postgres trigger that bumps and snapshots in one write, optimistic concurrency on the patch path, and the versions / diff read verbs.

Implements todos K2 b7c7b224, K3 c7cc5238, K4 ab938375, K5 be94d5b2. Design: designs/r4-knowledge-versioning-and-s3-design.md §3.2–3.4, §3.8. Doctrine: knowledge k_ms4x92lg_evtnx9.

Revision note. This description was corrected after an adversarial review found two P1 bypasses. See the review-round comment on this PR for exactly which claims changed and why. The original text overstated three things; they are marked [corrected] below.

⚠️ This targets main. It has NOT been reconciled with the deployed artifact.

The deployed API reports 1.0.0-rc.2 with 10 paths (including /v1/sources); this repo's main is 0.2.91 with 7. Locating the deployed source is task e0759534 and is not part of this change. Merging this is not a production rollout.

The migration is additive — new nullable column, new table, triggers — so it can be applied wherever the deployed source turns out to live. Migrations are strictly appended: verified, 65 pre-existing checksums unchanged, 10 appended, 0 drifted, with a positive control confirming the checker flags a real one-character edit and ignores trailing whitespace.

[corrected] The earlier claim that "one migration is correct against both schemas" was an inference stated as a measurement. What is measured: the trigger reads tenancy via to_jsonb(OLD)->>'tenant_id', which yields NULL where the column is absent and the real value where it is present — verified by a test that ALTERs the column in. What is not known: the actual deployed schema. The reviewer constructed one plausible shape — a tenant-scoped composite primary key (tenant_id, id) — under which this migration fails outright with no unique constraint matching given keys. Until e0759534 closes, treat applicability to production as unverified.

The measured problem

An entry was created over the hosted API and edited twice; both prior bodies are gone. knowledge_items had no version column, no versions table existed, and PATCH /v1/notes/{id} accepted no If-Match — last-writer-wins with no conflict detection, on a fleet where many agents write to one store.

Positive control that the absence was real: the same DDL file already defines source_revisions with UNIQUE(source_id, revision) and a hash column.

Why a trigger and not application code

open-mementos implements the same feature in TypeScript. The bump lives in the merge branch of createMemory (memories.ts:~400); the snapshot lives in updateMemory (:823). mementos save takes the first. A memory sitting at version 4 returns "No previous versions" — zero retained bodies.

The failure was not a caller forgetting a helper. It was a second write path inside the owning package forgetting. A BEFORE UPDATE trigger is the only place below the serve handler, the upsert/import path, ingest rules, sync replay, backfills, and psql — including the paths nobody has written yet.

Stated trade-off: the bump is invisible in the TypeScript, and the row the database returns differs from the row the caller sent. That is why the suite writes via raw SQL, bypassing every application path, and asserts the snapshot appeared anyway.

(Not independently verified by me: the open-mementos root-cause narrative is outside this repo. It is load-bearing for this argument and is reported as read, not as re-measured.)

Scope

In: schema (knowledge_items.version, knowledge_item_versions with UNIQUE(item_id, version), non-null content_hash, body_uri for offloaded bodies) · the trigger · optional If-Match / expected_version with 409 {error, expected, current} · GET /v1/notes/{id}/versions[/{version}] · knowledge versions / knowledge diff.

Out, each with its own task: S3 artifact storage (7b80e498) — the server has no artifact code path at all; governed-file binding (b74bc29e); v1 backfill for the 729 existing entries (open decision 5).

Hardening added after adversarial review

• ENABLE ALWAYS on the entry trigger. A normal trigger does not fire under session_replication_role = replica — what logical-replication apply workers, pg_restore --disable-triggers, and AWS DMS set. Measured before the fix: the update landed, the prior body was destroyed, no version row appeared, and the counter stayed at 1, so version actively lied. The design pre-committed to this test ("any success is a P0 and Phase 1 does not ship") and it had not been run.
  • Accepted limit, stated rather than implied: the table owner can still ALTER TABLE … DISABLE TRIGGER. Nothing a trigger can do defends against its own owner. The DML-only service role is refused both this and the replication role.
• knowledge_item_versions is append-only by enforcement. A plain application role could previously UPDATE a retained snapshot and rewrite history in place. DELETE remains permitted because the table cascades from knowledge_items and blocking it would make knowledge delete fail outright — so history for a deleted entry goes with the entry until the S3 journal lands. The word "immutable" has been dropped; this is append-only for live entries.

Deliberate deviations from the design

Design	Shipped	Reason
diff -v N	diff --rev N	-v is already the global alias for --version.
valid_from/valid_to as TIMESTAMPTZ	TEXT, ISO-8601	Every timestamp in this DDL is TEXT. A cast inside a BEFORE UPDATE trigger that throws on one badly-shaped legacy string would abort a legitimate write.
(not in design)	trigger stamps updated_at only when the caller did not set it, in toISOString() shape	[corrected — this was undisclosed in the original description.] NOW()::text renders 2026-07-28 21:29:56.01+00; space (0x20) sorts below T (0x54), so a TEXT column carrying both formats orders every trigger-written row before every application-written one regardless of actual time. It also discarded a caller-supplied updated_at, a regression against import/sync replay carrying source timestamps.
restore --id <id> --version N	not added	knowledge restore already means unarchive. Overloading it would be ambiguous and destructive; it needs its own name and its own decision.

Residuals — known and deliberately not fixed in this PR

• reason is not settable over HTTP. Column, trigger read, and GUC plumbing exist and are tested; no surface sets it.
• No automatic retry on 409. The CLI sends the version it read, so no agent types one, and a conflict exits non-zero naming both versions. Re-applying without comparing which fields moved is how you overwrite a colleague while believing you handled it.
• The concurrency guard is now sent from update/archive/restore/untag/upsert, but the CLI-side interleaving that would prove it end-to-end is not staged. The server-side 409 path is covered.
• NoteRepo.create() now costs a transaction per insert (~23ms against loopback, 30 creates in 689ms) because attribution is transaction-local. Undisclosed cost on the ingest/import/sync hot path; no before/after benchmark.
• tests/serve.test.ts's in-memory shim cannot observe versioning at all — it matches SQL strings and has no trigger. It needed a transaction passthrough to keep passing; a comment at the top now says what it does not prove. Migrating it to PGlite is a reasonable follow-up.

Evidence

Exit codes measured unpiped (cmd > file; rc=$?), never through | head.

TDD, both rounds:

• Round 1 behavioural red — application surface in, migration withheld, real in-process Postgres with main's actual DDL: 3 pass / 24 fail, rc=1 on column version does not exist. Green after the migration: 27/0, rc=0.
• Round 2 — four new assertions written first against the shipped migration: 28 pass / 4 fail, rc=1. Green after the fixes: 32/0, rc=0.

Planted-defect controls (each reverted afterwards; unmodified copy re-runs clean at rc=0):

Planted defect	Result
drop ENABLE ALWAYS	rc=1, 1 test fails
revert updated_at to NOW()::text	rc=1, 2 tests fail
remove the append-only guard	rc=1, 1 test fails
hardcode tenant_id to NULL	rc=1, 1 test fails

That last one is the defect the reviewer proved the previous suite could not catch.

Gates: bunx tsc --noEmit rc=0 · bun run verify:generated rc=0 ("6 generated bundles rebuild byte-identically") · validate-public-package rc=0 · migration checksum check rc=0 with positive control.

Secrets: scanned before every commit; positive control with a planted AKIA… returned 1 match / rc=0 (fired), real staged diff 0 matches / rc=1 (clean). Control removed from index and worktree before committing.

Full suite: main is already red, and this branch is less red

[corrected] The original description gave fixed pass/fail counts. They are load-dependent on this shared machine and should never have been stated as fixed. Measured under load average 38.76 on 20 cores with 233 concurrent bun processes, this branch showed 28 failures; the identical tree under load ~12–25 showed 4. The counts move; the structure does not.

Same-machine runs under comparable load:

	result	wall
pristine main (eed77b2)	304 pass / 2 skip / 6 fail, rc=1	298.9s
this branch	364 pass / 2 skip / 4 fail, rc=1	323.9s

Every failure in both is the same class: tests/cli.test.ts subprocess-spawn timeouts (Received: null = killed spawn), plus app wiki standard. Re-run with a generous timeout, the three non-cli.test.ts suites that failed under peak load pass 39/39, rc=0 — confirming timeouts rather than defects.

Not claimed: that CI will be green. main is red before this change and after it, in the same file.

[andrei-hasna]

Review round 1 — remediation, and exactly what changed in the description

An adversarial reviewer ran this PR against real PostgreSQL 16.14, not the PGlite the suite uses. Verdict was APPROVE-WITH-FIXES. Two P1s were real; both are now fixed in 100b39d, with the rebuilt bundles in efcd657.

I am recording the description edits here rather than silently rewriting history: this PR is unmerged, so the body is still a working document, but three of its claims were wrong and the record should say which.

Fixed — P1

P1-1 · The trigger was bypassable. A trigger created normally does not fire under session_replication_role = replica — what logical-replication apply workers, pg_restore --disable-triggers, and AWS DMS set. Reviewer measured against the then-current migration: the update landed, the prior body was destroyed, no version row appeared, and the counter stayed at 1, so version actively lied about the row. The design pre-committed to exactly this test — "any success is a P0 and Phase 1 does not ship" — and I had not run it. Fixed with ALTER TABLE knowledge_items ENABLE ALWAYS TRIGGER. The owner-level DISABLE TRIGGER path is undefendable and is now documented as an accepted limit instead of an unstated failed criterion.

P1-2 · The trigger wrote a second, incompatible timestamp format. NOW()::text into a TEXT column the application fills with toISOString(). Space (0x20) sorts below T (0x54), so every trigger-written timestamp sorted before every application-written one regardless of actual time, and valid_from/valid_to stopped being comparable. It also discarded a caller-supplied updated_at — a regression against import and sync replay, which carry source timestamps. Both fixed. My test could not have caught it: it compared against '2000-01-01', true under either format — a non-discriminating input.

Fixed — cheap correctness

• knowledge_item_versions is append-only by enforcement. A plain app role could previously UPDATE a snapshot and rewrite history in place. DELETE stays permitted (cascade from knowledge_items; blocking it would break knowledge delete), so history dies with a deleted entry until the S3 journal lands. "Immutable" removed from the description.
• The to_jsonb(OLD)->>'tenant_id' branch now has a test that ALTERs the column in. This was the one planted defect the previous suite could not catch — the repo's schema cannot otherwise supply a distinguishing input.
• The concurrency guard is sent from archive/restore/untag/upsert, not just update (design §3.4: "the CLI always sends it").
• versions pages. The server caps at 200, so history past that was counted in total and unreachable.
• tests/search-pg-parity.test.ts now uses the shared PGlite fixture. Its private copy had no transaction() and would have thrown the moment it touched the write path — the exact drift the fixture's own docstring claimed to have eliminated. Two false statements in that docstring are corrected to what was measured.

Each fix was verified by planting the defect back: dropping ENABLE ALWAYS, reverting updated_at, removing the append-only guard, and hardcoding tenant_id to NULL each turn the suite red (rc=1). Unmodified copy re-runs clean.

Description corrections

1. "One migration is correct against both schemas" was an inference stated as a measurement. What is measured is the to_jsonb behaviour. The reviewer constructed a plausible deployed shape — composite PK (tenant_id, id) — under which this migration fails outright. Since e0759534 is open, applicability to production is unverified. Softened.
2. Fixed pass/fail counts were load-dependent and should not have been stated as fixed. The same tree gave 28 failures at load average 38.76 (20 cores, 233 concurrent bun processes) and 4 at load ~12–25. Now stated as a structural claim with the mechanism named. The reviewer's own counts differed from mine again, which is the point.
3. The updated_at reformat was undisclosed. Now in the deviations table.

Not fixed — residuals, in the description

reason unsettable over HTTP · no auto-retry on 409 · no staged CLI-side interleaving test for the guard · undisclosed per-insert transaction cost (~23ms) on the ingest/import hot path · tests/serve.test.ts's SQL-string fake still cannot observe versioning.

Per the remediation-round termination rule: two rounds, then stop. Anything above that cannot corrupt history or lose a version is recorded rather than fixed in-train.