diff --git a/docs/cli.md b/docs/cli.md index 72be8b9e5..2448b4364 100644 --- a/docs/cli.md +++ b/docs/cli.md @@ -435,6 +435,29 @@ kept on every rewrite instead of being dropped. `migrate` to remote carries `att `content_type`; it does not carry `supersedes` (a local id means nothing to the hosted store) or the local `id` (the hosted store mints its own). +**Versions and `--expected-version` (migration 00120).** Every record has a `version`: `1` when it +is created, one higher on each content change (value, content type, merged attributes, tags, or a +new `supersedes` link), unchanged by an identical rewrite, a TTL change, provenance, archive or +restore. A write to an archived key starts again at `1`. The local store keeps it in the +frontmatter (appended to `FIELDS`) and applies the same rule as the hosted trigger; a file written +before the field existed reads as version `1`. `write` reports the new version (`version` in +`--json`), and `show` and `list --json` include it. + +- `--expected-version ` — write only if the memory is still at version `n`; `0` means it must + not exist yet (an archived or expired record counts as absent, as it does for reads; an expired + record rewritten this way keeps counting from its old version, and with no `--ttl-days` / + `--clear-ttl` and no configured default its past expiry is cleared so it is live again). On a + mismatch nothing is written and the + command exits `1` naming the stored version: + `version conflict — global::k is at version 4, not 3. Nothing was written.` Works against both + stores; remotely the check is atomic in the `memory_write` RPC (`409 version_conflict`). + +`show` also prints `superseded by , …` when other live records name this one in their +`supersedes` (newest first; `superseded_by` in `--json`). Locally that is computed across both store +tiers on `show`/`read`; `list` and `search` do not compute it offline, because it would cost a walk +of the whole store on the hook hot path. A local relocation (`migrate --to project`) keeps each +record's `version`; `migrate` to remote does not send it — the hosted store counts its own. + ### `archive`/`delete`/`restore` (aliases: `rm` → `delete`) (the removal lifecycle, the counterpart to `write` — `archive diff --git a/docs/key-files.md b/docs/key-files.md index 617397baf..d2676526b 100644 --- a/docs/key-files.md +++ b/docs/key-files.md @@ -51,7 +51,9 @@ else. Grouped loosely by subsystem in migration/PR order. | `supabase/migrations/00009_memory_write_created_at.sql` | Drops + recreates the `memory_write` RPC with an optional `p_created_at timestamptz default null`; INSERT uses `coalesce(p_created_at, now())` for both `created_at` and `updated_at` (conflict-update never touches `created_at`). Lets `memory.write` backdate a migrated memory. No table change — `created_at` already existed | | `packages/mcp-core/src/limits/ttl.ts` | Pure `parseTtlDays` — validates the optional `memory.write` `ttl_days` override (1–365 integer, rejects 0/fractional/non-finite). Mirrored self-contained in `supabase/functions/mcp/ttl.ts` (edge) and, for the days half only, in `packages/cli/src/store/ttl.mjs` (zero-dep CLI — which adds the offline-only `expiresAtFrom`/`isExpired`/`isLive`/`resolveExpiresAt`; `parseTtlSeconds`/`parseTtlMinutes`/`parseTtl` have no CLI equivalent) — the `limits.ts` pattern | | `packages/mcp-core/src/provenance/origin.ts` | Pure `parseOrigin` (+ per-field `parseOriginRepo`/`Branch`/`Commit`/`Pr`) — validates the four optional `memory.write` provenance params (`origin_repo`/`origin_branch`/`origin_commit`/`origin_pr`), the record of WHERE a lesson was recorded from as opposed to `scope`, which says where it applies. `origin_branch` is deliberately NOT lowercased (unlike a `branch::` scope) so the dashboard's `/tree/` link resolves for a mixed-case branch. Mirrored self-contained into `supabase/functions/_shared/provenance/origin.ts` (`_shared/` because BOTH the MCP tools and the REST `POST /memories` handler validate through it) and guarded by `edge-parity.spec.ts` — the `created-at.ts` pattern | -| `packages/mcp-core/src/provenance/record-extensions.ts` | Pure `parseRecordFields` — validates the three optional record extensions on `memory.write` (`attributes`, `content_type`, `supersedes`, migration 00119), plus `splitCitedIds` (a `cited` entry may be a memory id), `translateRecordFieldDbError` (the RPC's and the table constraints' rejections → the same user error) and `compactRecordFields` (a read leaves out extensions that hold their defaults). Mirrored into `supabase/functions/_shared/provenance/record-extensions.ts` and used by MCP `toolWrite`/`toolRead`, REST `POST /memories`, `_shared/telemetry/citations.ts` and mcp-core `write.ts` — the `origin.ts` pattern | +| `packages/mcp-core/src/provenance/record-extensions.ts` | Pure `parseRecordFields` — validates the three optional record extensions on `memory.write` (`attributes`, `content_type`, `supersedes`, migration 00119), plus `splitCitedIds` (a `cited` entry may be a memory id), `translateRecordFieldDbError` (the RPC's and the table constraints' rejections → the same user error) and `compactRecordFields` (a read leaves out extensions that hold their defaults). Mirrored into `supabase/functions/_shared/provenance/record-extensions.ts` and used by MCP `toolWrite`/`toolRead`, REST `POST /memories`, `_shared/telemetry/citations.ts` and mcp-core `write.ts` — the `origin.ts` pattern. Also (migration 00120) `parseExpectedVersion`, `VersionConflictError` (the RPC's `LK003` → code `version_conflict`, REST 409 / MCP `isError`), and the pure `groupSupersededBy` / `withSupersededBy` behind the computed `superseded_by` read field | +| `supabase/functions/_shared/memory/superseded-by.ts` | `fetchSupersededBy` — the ONE query (per page, served by `memories_supersedes_idx`) that computes `superseded_by` for MCP `memory.read`/`list`/`search` and the REST read handlers. The caller passes its own tenant filter (`applyTenantScope` / `applyRestTenantScope` / RLS), so a successor the caller cannot read is never named | +| `supabase/migrations/00120_memory_version.sql` | `memories.version` + the `memories_bump_version` trigger (content changes only), `memory_write`'s `p_expected_version` compare-and-set (`LK003`), and `version` on `lorekit_memory_list` | | `supabase/migrations/00048_memory_origin.sql` | Adds the four nullable `memories.origin_*` columns (+ CHECK backstops) and drops/recreates `memory_write` with trailing defaulted `p_origin_repo`/`p_origin_branch`/`p_origin_commit`/`p_origin_pr`. **Last KNOWN origin wins**: every branch's `do update set` uses `coalesce(excluded.origin_x, memories.origin_x)`, so a write with no git context never erases what an earlier write recorded. No index — no read path filters on origin yet. Proven by `migrations.test.sql` §62–§65 | | `packages/web/src/lib/origin.ts` | Pure `originLinks(origin, scope?)` + the four URL builders (`originPullRequestUrl`/`originBranchUrl`/`originCommitUrl`/`originRepoUrl`) and `shortSha` — the provenance counterpart to `scope.ts`'s `scopeRepoUrl`. Total functions: a partial or malformed origin yields fewer links, never a throw or a broken href. **Complements the scope rather than repeating it**: given the memory's `scope` it drops any origin the scope-derived Repo row already links — the repo row when `scopeRepoRef(scope).repo` is the same repo (so it survives only for a `global`/`project` lesson or a genuinely cross-repo origin), and the branch row when a `branch::` scope already names that branch of that repo. Pull request and commit are never suppressed — no scope can express them. Storage stays complete (the DB always records the full origin); the de-duplication is a RENDER concern, so a memory that is later re-scoped still shows the right rows. Rendered by `components/memory/MemoryOrigin.tsx` inside `LessonDetailSheet`'s Metadata `
` | | `packages/cli/src/shared/origin.mjs` | `deriveOrigin({cwd,env,run})` / `prNumberFromEnv` / `mergeOrigin` — the CLI's provenance derivation, the counterpart to `scope.mjs`'s `deriveScope`. Git runner and env are injected so it is testable with no repo and no CI runner. Prefers `GITHUB_HEAD_REF` over `git rev-parse` (a GitHub Actions `pull_request` checkout is a detached merge commit); PR precedence is `LOREKIT_PR` → `GITHUB_REF`'s `refs/pull//merge` → `GITHUB_PR_NUMBER`. `mergeOrigin` OMITS an unknown field rather than sending null, which is what makes the RPC's coalesce rule non-destructive. Consumed by `write.mjs` (with `--no-origin` / `--origin-*` overrides) and by `mcp-server.mjs`'s `memory.write` dispatch (best-effort, caller values win) | diff --git a/docs/mcp-tools.md b/docs/mcp-tools.md index fd3054e2e..344436651 100644 --- a/docs/mcp-tools.md +++ b/docs/mcp-tools.md @@ -100,7 +100,7 @@ Store or update a lesson. Requires a token with write permission (`lk_rw_*` or ` | `kind` | | Bucket kind: `lesson`, `bus`, or `signal`. Omit to have it inferred from a `loop::-lessons` tag. See **Taxonomy** below. | | `host` | | Owning skill/agent (e.g. `reviewer`, `aw`). Omit to have it inferred from a `loop::-lessons` tag. | | `org` | | Org slug to write under (org-owned write). Omit for a personal memory. You must be a write-capable member (`member`/`admin`/`owner`, not `viewer`) of the org — verified server-side; supplying an org you're not authorized for is rejected. | -| `ttl_days` | | Integer 1–365. The memory auto-expires after this many days. Mutually exclusive with `ttl_minutes` and `ttl_seconds`; supply at most one. On an update, refreshes the expiry; omitting all three leaves the existing expiry unchanged. | +| `ttl_days` | | Integer 1–365. The memory auto-expires after this many days. Mutually exclusive with `ttl_minutes` and `ttl_seconds`; supply at most one. On an update, refreshes the expiry; omitting all three leaves the existing expiry unchanged (except on an `expected_version` write that revives an expired memory, which clears the past expiry). | | `ttl_minutes` | | Integer 1–525600 (365 days in minutes). The memory auto-expires after this many minutes. Mutually exclusive with `ttl_days` and `ttl_seconds`. | | `ttl_seconds` | | Integer 1–31536000 (365 days in seconds). The memory auto-expires after this many seconds. Useful for short-lived session memories (e.g. 30 s, 5 min). Mutually exclusive with `ttl_days` and `ttl_minutes`. | | `origin_repo` | | Provenance: `owner/name` of the repository the lesson was recorded from. Lowercased. | @@ -111,6 +111,7 @@ Store or update a lesson. Requires a token with write permission (`lk_rw_*` or ` | `content_type` | | `text/markdown` (default) or `application/json`. With `application/json` the value must parse. Omit on an update to keep the stored type. | | `supersedes` | | The `id` of the memory this one replaces. Must name a memory this token can read **and** that has the same owner as the one being written: your own personal memory for a personal write, a memory in the same org for an org write. Anything else, including an id that does not exist, fails with the same error, so the field cannot be used to probe for ids. Kept on update unless re-supplied. | | `cited` | | Lessons that shaped this run, as `scope::key` strings or memory ids. Unresolvable entries are dropped silently. | +| `expected_version` | | Integer ≥ 0. Write only if the memory at `scope`+`key` is still at this `version`; `0` means it must not exist yet. A mismatch writes nothing and fails with `version_conflict`, naming the stored version. Omit for an unconditional write. See **Versions** below. | **Provenance (`origin_*`).** `scope` says where a lesson **applies**; the four `origin_*` fields say where it was **recorded from**, and the dashboard turns @@ -166,6 +167,37 @@ All three are validated by the shared parser in inputs with the same message. `PATCH /memories/:id` does not accept them; set them with a write. +**Versions (`version` / `expected_version` / `superseded_by`).** Every memory +carries a `version` (migration 00120): `1` when it is created, and one higher +each time its **content** changes — `value`, `content_type`, `attributes`, +`tags`, or a `supersedes` link being set or changed. Nothing else bumps it: not +a TTL change, a counter (`seen_count`, reads), an embedding, archive/restore, a +move between owners, a link cleared because its target was deleted, or a write +that changes nothing. A database trigger maintains it, so no caller can set it. +A write to an archived key starts a new record at version `1`. + +`expected_version` turns a write into a compare-and-set. Read the memory, keep +its `version`, and pass it back on the write: if anyone changed the content in +between, the write is refused instead of silently overwriting their change. +`expected_version: 0` creates the memory only if it does not exist yet — an +archived or expired memory counts as absent, exactly as reads hide it (an expired +one is rewritten in place and keeps counting from its old version; with no +`ttl_days` / `ttl_minutes` / `ttl_seconds` / `clear_ttl` on that write its past expiry is cleared, so the +record is live again). The +check is atomic in the `memory_write` RPC. A refusal is an `isError` tool +result (REST: `409`) with code `version_conflict`: + +```text +version_conflict: expected_version 3 does not match the stored version 4. Re-read the memory, then retry with its current version. +``` + +`superseded_by` is the reverse of `supersedes`: on `memory.read`, +`memory.list` and `memory.search`, a memory that other live records name in +their `supersedes` carries their ids, newest first. It is computed on read, +never stored, with one query per page; it only names records the caller can +read (same tenant filter as the read itself) that are neither archived nor +expired, and it is omitted when empty. + **Taxonomy (`kind` / `host`).** A self-improvement loop's bucket carries two facts: WHAT KIND of memory it is and WHICH HOST owns it. `kind` is a closed vocabulary — `lesson` (procedural, read every run), `bus` (a transient outcome @@ -180,7 +212,7 @@ an update the last KNOWN value wins, exactly like `origin_*`. **Scope→org binding.** If you omit `org` but the scope is **bound to an org** (an admin set that up — see [org-sharing.md](./org-sharing.md#scope--org-binding-auto-routing)), the write auto-routes to that org **when you're a write-capable member**. A binding can also be a **wildcard prefix** (`repo::owner/*`), and when several bindings match, resolution is **most-specific-wins** (an exact scope beats any wildcard; a longer wildcard prefix beats a shorter one). If you're *not* a member of the winning org, it's saved to your personal lore instead (never rejected) and the response carries a `notice` explaining that — unless your token is limited to selected orgs (`orgs_only`), which may not write personal memories: that write is refused with an in-band `isError` result (`key_personal_denied`), so pass `org`. An explicit `org` always overrides binding resolution entirely. -**Returns:** `{ "id": "", "created_at": "" }` — the `id` is the memory's stable identity, kept across updates — plus an optional `"expires_at": ""` when any `ttl_*` field was supplied, and an optional `"notice": ""` when a write fell back to personal because the scope is bound to an org you can't write to. +**Returns:** `{ "id": "", "created_at": "", "version": }` — the `id` is the memory's stable identity, kept across updates; `version` is its version after this write — plus an optional `"expires_at": ""` when any `ttl_*` field was supplied, and an optional `"notice": ""` when a write fell back to personal because the scope is bound to an org you can't write to. **Default TTL — there isn't one, server-side.** Omitting every `ttl_*` field means the memory never expires, and that is unchanged. Two things nonetheless @@ -218,9 +250,11 @@ scope you can see (see [Unscoped reads](#unscoped-reads)). } ``` -**Returns:** `{ "id": "", "value": "", "updated_at": "", "scope": "" }`, or +**Returns:** `{ "id": "", "value": "", "updated_at": "", "scope": "", "version": }`, or `null` if not found. `id` is the memory's stable identity (what `supersedes` and `cited` accept). -`attributes`, `content_type` and `supersedes` are included only when they hold a non-default value. `scope` names the scope that actually answered — the same value you passed when +`attributes`, `content_type` and `supersedes` are included only when they hold a non-default value. +`superseded_by` (the ids of newer records that replace this one) is included only when non-empty — +see **Versions** under `memory.write`. `scope` names the scope that actually answered — the same value you passed when you passed one, and the resolved winner when you did not. An ambiguous unscoped read additionally carries `other_scopes: ["", …]`, every OTHER scope the key also exists in. @@ -304,7 +338,7 @@ reference past the 32nd, so neither appears in `entries` or in `missing`: the li report, never a malformed-input or truncation one. To tell "absent" from "never looked up", compare `entries` + `missing` against what you sent. -**Returns:** `{ "entries": [{ "scope", "key", "value", "updated_at" }], "missing": ["scope::key", …] }` +**Returns:** `{ "entries": [{ "scope", "key", "value", "updated_at", "version" }], "missing": ["scope::key", …] }` (each entry also carries `superseded_by` when non-empty) — `missing` names every well-formed reference within the first 32 that matched no lesson, in no particular order. @@ -342,7 +376,7 @@ a list returns every match rather than one winner. | `host` | | Filter to the owning skill/agent, e.g. `reviewer`, `aw`, `ci-auto-fix` | | `view` | `full` | `full` returns each entry's complete `value`. `summary` omits `value` and returns `value_bytes` + a 200-character `preview` instead | -**Returns:** `{ "entries": [{ "scope", "key", "value", "tags", "updated_at" }], "hasMore": boolean, "nextCursor": string | null }` — with `view: "summary"` each entry is `{ "scope", "key", "tags", "updated_at", "value_bytes", "preview" }` instead, and `value` is omitted entirely. `scope` is present on every entry, scoped call or not, so one response shape covers both. +**Returns:** `{ "entries": [{ "scope", "key", "value", "tags", "updated_at", "version" }], "hasMore": boolean, "nextCursor": string | null }` — each entry also carries `superseded_by` when non-empty; with `view: "summary"` each entry is `{ "scope", "key", "tags", "updated_at", "version", "value_bytes", "preview" }` instead, and `value` is omitted entirely. `scope` is present on every entry, scoped call or not, so one response shape covers both. - `recency` (default): pass `nextCursor` back as `cursor` to read the next page. - `rank`: a single bounded top-N page — `hasMore` is always `false` and `nextCursor` always `null`. @@ -459,7 +493,7 @@ Full-text search across all lessons. Supports owner-level scope wildcards. | `tags` | `[]` | AND-filter on tags | | `limit` | `20` | Max results (cap: 100) | -**Returns:** `{ "entries": [{ "key", "value", "scope", "tags", "rank" }] }` +**Returns:** `{ "entries": [{ "key", "value", "scope", "tags", "version", "rank" }] }` (each entry also carries `superseded_by` when non-empty) --- diff --git a/packages/cli/bin/lorekit.mjs b/packages/cli/bin/lorekit.mjs index f79a66228..48c39723f 100755 --- a/packages/cli/bin/lorekit.mjs +++ b/packages/cli/bin/lorekit.mjs @@ -449,6 +449,7 @@ ${c.bold('Options')} --attributes Open metadata, e.g. '{"ci.run_url":"https://…"}' (merged; null removes a key) --content-type text/markdown (default) or application/json (value must parse) --supersedes The id of the memory this one replaces + --expected-version Write only if the memory is still at version n (0 = must not exist yet) --origin-repo Override the derived provenance repository --origin-branch Override the derived provenance branch --origin-commit Override the derived provenance commit SHA @@ -1180,7 +1181,7 @@ const KNOWN_FLAGS = [ 'from', 'to', 'apply', 'yes', 'hooks', 'no-hooks', 'mcp-json', 'completions', 'complete', 'force', 'deep', 'adapter', 'event', 'json', 'scope', 'key', 'threshold', 'help', 'version', 'telemetry', 'value', 'tags', 'source-agent', 'trigger', 'kind', 'host', 'ttl-days', 'clear-ttl', 'org', 'cited', - 'attributes', 'content-type', 'supersedes', + 'attributes', 'content-type', 'supersedes', 'expected-version', 'remote', 'local', // `view` is accepted-and-IGNORED, not documented: the Explorer dropped the // scope/time tab so the flag is a no-op, but `link` is a HUMAN_COMMAND that diff --git a/packages/cli/src/commands/show.mjs b/packages/cli/src/commands/show.mjs index 39d3e1223..1500109c4 100644 --- a/packages/cli/src/commands/show.mjs +++ b/packages/cli/src/commands/show.mjs @@ -425,7 +425,13 @@ function renderRecordSection(title, section, subtitle) { const e = section.record; if (e.updated) log(` ${c.dim('updated')} ${shortDate(e.updated)}`); + if (e.version != null) log(` ${c.dim('version')} ${e.version}`); if (e.tags && e.tags.length) log(` ${c.dim('tags')} ${e.tags.join(', ')}`); + // A newer record names this one in its `supersedes`: say so, since a reader + // acting on a replaced lesson is the case the link exists to prevent. + if (e.superseded_by && e.superseded_by.length) { + log(` ${c.dim('superseded by')} ${e.superseded_by.join(', ')}`); + } log(` ${c.dim('value')}`); // The full, untruncated body — indented, line by line, so multi-line lessons // read as written. diff --git a/packages/cli/src/commands/write.mjs b/packages/cli/src/commands/write.mjs index 3fbdc8bfb..2fc690012 100644 --- a/packages/cli/src/commands/write.mjs +++ b/packages/cli/src/commands/write.mjs @@ -37,6 +37,10 @@ // --content-type text/markdown (default) or application/json (the // value must then parse as JSON) // --supersedes The id of the memory this one replaces +// --expected-version Optimistic concurrency: write only if the memory is +// still at version n (0 = it must not exist yet). A +// mismatch writes nothing, names the stored version, +// and exits non-zero // // `--cited` credits the lore that shaped the work being written up. It is the // only signal that distinguishes a lesson that was read from one that was used: @@ -276,6 +280,19 @@ export async function write(args) { const contentType = args['content-type']; const supersedes = args.supersedes; + // `--expected-version` (00120). Validated here, at the flag seam, like + // `--origin-pr`: a bare flag or a non-integer is a usage error, never dropped + // — a dropped check would turn a guarded write into an unconditional one. + let expectedVersion; + if (args['expected-version'] !== undefined) { + const raw = args['expected-version']; + expectedVersion = typeof raw === 'string' && /^\d+$/.test(raw.trim()) ? Number(raw.trim()) : NaN; + if (!Number.isSafeInteger(expectedVersion) || expectedVersion > 2147483647) { + err(`${c.red('Error:')} --expected-version must be a non-negative integer (got ${raw === true ? 'no value' : raw})`); + return 1; + } + } + // ── Provenance ──────────────────────────────────────────────────────────── // Derived from git + CI unless --no-origin; explicit --origin-* flags win. // A field that is neither supplied nor derivable is omitted, never sent as @@ -381,6 +398,7 @@ export async function write(args) { ...(attributes ? { attributes } : {}), ...(contentType ? { content_type: contentType } : {}), ...(supersedes ? { supersedes } : {}), + ...(expectedVersion !== undefined ? { expected_version: expectedVersion } : {}), ...origin, }; @@ -393,7 +411,17 @@ export async function write(args) { } if (!result || result.ok === false) { - const detail = result && result.error ? result.error : 'unknown error'; + // The local store reports a string; the remote one a `{ message, code }` + // object, which interpolated bare would print as `[object Object]`. + const error = result?.error; + const detail = typeof error === 'string' ? error : error?.message || 'unknown error'; + const code = result?.code ?? (typeof error === 'object' ? error?.code : undefined); + if (code === 'version_conflict') { + const current = result?.current_version ?? error?.current_version; + err(`${c.red('Error:')} version conflict — ${scope}::${key} is at version ${current ?? 'unknown'}, not ${expectedVersion}. Nothing was written.`); + err(`Re-read it (${c.cyan(`lorekit show ${scope}::${key}`)}), then retry with --expected-version ${current ?? ''}.`); + return { exitCode: 1, 'lorekit.cli.write.store': storeName, 'lorekit.cli.write.version_conflict': true }; + } err(`${c.red('Error writing to')} ${storeName} store: ${detail}`); return 1; } @@ -401,6 +429,9 @@ export async function write(args) { // `inserted` is additive from the remote (memory_write RPC 00011). Local store // returns { ok, entry } with no explicit field; treat absence as unknown. const inserted = result.inserted ?? null; + // The record's version after this write (00120): the remote reports it on the + // result, the local store on the entry it wrote. Null when unknown. + const version = result.version ?? result.entry?.version ?? null; if (args.json) { log(JSON.stringify({ @@ -408,6 +439,7 @@ export async function write(args) { key, store: storeName, inserted, + version, value: String(value), tags, source_agent: sourceAgent || null, @@ -424,6 +456,7 @@ export async function write(args) { log(` ${c.dim('scope')} ${scope}`); log(` ${c.dim('key')} ${key}`); if (tags.length) log(` ${c.dim('tags')} ${tags.join(', ')}`); + if (version != null) log(` ${c.dim('version')} ${version}`); // Name the source. A TTL the caller typed needs no explanation; one that came // from a config file two directories up does, or the first surprise is a // memory that quietly vanished. diff --git a/packages/cli/src/shared/completions.mjs b/packages/cli/src/shared/completions.mjs index 01cff36ba..9b1fbc26f 100644 --- a/packages/cli/src/shared/completions.mjs +++ b/packages/cli/src/shared/completions.mjs @@ -67,6 +67,7 @@ const FLAG = { attributes: { desc: 'Open metadata as a JSON object', arg: 'json' }, 'content-type': { desc: 'What the value is', arg: 'type', values: ['text/markdown', 'application/json'] }, supersedes: { desc: 'Id of the memory this one replaces', arg: 'id' }, + 'expected-version': { desc: 'Write only if the memory is at this version (0 = new)', arg: 'n' }, 'origin-repo': { desc: 'Provenance repository', arg: 'owner/name' }, 'origin-repo-mode': { desc: 'How --origin-repo matches', arg: 'mode', values: ['in', 'nin'] }, 'origin-branch': { desc: 'Provenance branch', arg: 'branch' }, @@ -165,7 +166,7 @@ const COMMANDS = [ flags: ['yes', 'endpoint', 'token'] }, { name: 'write', summary: 'Create or update a memory', positional: 'address', flags: ['dir', 'scope', 'key', 'value', 'tags', 'source-agent', 'trigger', 'ttl-days', 'clear-ttl', - 'org', 'cited', 'attributes', 'content-type', 'supersedes', 'origin-repo', 'origin-branch', 'origin-commit', 'origin-pr', 'no-origin', 'remote', 'local', + 'org', 'cited', 'attributes', 'content-type', 'supersedes', 'expected-version', 'origin-repo', 'origin-branch', 'origin-commit', 'origin-pr', 'no-origin', 'remote', 'local', 'json', 'endpoint', 'token', 'store'] }, { name: 'archive', summary: 'Hide a memory without losing it', positional: 'address', flags: ['scope', 'key', 'remote', 'local', 'json'] }, diff --git a/packages/cli/src/shared/lessons-view.mjs b/packages/cli/src/shared/lessons-view.mjs index 42597dcd0..5588d68a3 100644 --- a/packages/cli/src/shared/lessons-view.mjs +++ b/packages/cli/src/shared/lessons-view.mjs @@ -94,6 +94,13 @@ export function normalizeEntry(e = {}) { origin_repo: e.origin_repo ?? null, origin_branch: e.origin_branch ?? null, origin_pr: e.origin_pr ?? null, + // The record's version (00120). Null when the store did not report one — + // a remote deployed before the column, never a local read, which resolves + // an absent version to 1. + version: Number.isInteger(e.version) ? e.version : null, + // The ids of the records that supersede this one, newest first. Computed on + // read and carried only when non-empty, as both stores report it. + ...(Array.isArray(e.superseded_by) && e.superseded_by.length ? { superseded_by: e.superseded_by } : {}), }; } diff --git a/packages/cli/src/shared/mcp.mjs b/packages/cli/src/shared/mcp.mjs index 9d024d025..a68204579 100644 --- a/packages/cli/src/shared/mcp.mjs +++ b/packages/cli/src/shared/mcp.mjs @@ -467,7 +467,15 @@ export async function restFetch(baseUrl, token, path, { method = 'GET', body, ti // but this is read on every failure so a future 503 needs no change // here. Additive: existing callers ignore the extra key. retryAfter: retryAfterFrom(data, res.headers), - error: data?.error ? { message: data.error, code: data.code } : { code: res.status, message: text.slice(0, 200) || res.statusText }, + // `current_version` rides along on a `version_conflict` 409 (00120) so a + // caller can say what the stored version is without a second read. + error: data?.error + ? { + message: data.error, + code: data.code, + ...(Number.isInteger(data.current_version) ? { current_version: data.current_version } : {}), + } + : { code: res.status, message: text.slice(0, 200) || res.statusText }, }; } return { ok: true, httpStatus: res.status, data, userId }; diff --git a/packages/cli/src/store/entry-fields.mjs b/packages/cli/src/store/entry-fields.mjs index e469ddd89..f6b6b2b25 100644 --- a/packages/cli/src/store/entry-fields.mjs +++ b/packages/cli/src/store/entry-fields.mjs @@ -77,3 +77,18 @@ export function withReadFields(row) { updatedAt: updatedAtOf(row), }; } + +/** + * The record's version (migration 00120): an integer that starts at 1 and + * goes up by one on every content change. + * + * A row that does not carry a usable version is reported as `1`, not `0` or + * null: a local file written before the column existed is still a record that + * has been written once, which is exactly what version 1 means on the server + * (the column's backfill default). Total, like everything here. + */ +export function versionOf(row) { + const raw = row?.version; + const n = typeof raw === 'string' && raw.trim() !== '' ? Number(raw) : raw; + return Number.isInteger(n) && n >= 1 ? n : 1; +} diff --git a/packages/cli/src/store/format.mjs b/packages/cli/src/store/format.mjs index 2e3e62b17..24225dc07 100644 --- a/packages/cli/src/store/format.mjs +++ b/packages/cli/src/store/format.mjs @@ -52,6 +52,11 @@ export const FIELDS = [ 'attributes', 'content_type', 'supersedes', + // Version (migration 00120): starts at 1 and goes up by one on every content + // change — value, content_type, attributes, tags, or a new supersedes link — + // mirroring the hosted `memories_bump_version` trigger. Appended like the + // columns above: a file written before it existed reads as version 1. + 'version', ]; // The content types a record may declare; the first is what null means. The diff --git a/packages/cli/src/store/local.mjs b/packages/cli/src/store/local.mjs index 45db4a9c4..6814d31dd 100644 --- a/packages/cli/src/store/local.mjs +++ b/packages/cli/src/store/local.mjs @@ -12,8 +12,8 @@ import { serializeEntry, parseEntry, slugify, scopeToDir, extraFields, mergeAttributes, recordFieldsIssue, ATTRIBUTES_MAX_KEYS, } from './format.mjs'; import { normalizeCreatedAt } from './created-at.mjs'; -import { isLive, resolveExpiresAt } from './ttl.mjs'; -import { seenCountOf, withReadFields, updatedAtOf } from './entry-fields.mjs'; +import { isLive, parseTtlDays, resolveExpiresAt } from './ttl.mjs'; +import { seenCountOf, withReadFields, updatedAtOf, versionOf } from './entry-fields.mjs'; import { pickScopeWinner } from '../shared/scope-precedence.mjs'; import { matchesTags } from '../shared/lessons-pure.mjs'; @@ -98,7 +98,7 @@ class LocalStore { if (limit) rows = rows.slice(0, limit); // The same additive projection the remote store applies, so a caller that // ranks entries never has to ask which store produced them. - return { ok: true, entries: rows.map(withReadFields) }; + return { ok: true, entries: rows.map(projectEntry) }; } // read({ scope, key }) → { ok, entry } — null when absent, archived, or expired. @@ -108,20 +108,27 @@ class LocalStore { // the SAME rule the hosted MCP and REST surfaces apply, so `lorekit show // ` offline and `memory.read { key }` online cannot resolve the same key // to different lessons. + // + // A found entry carries `superseded_by` (00120) when any live record in this + // store names it in `supersedes` — see `attachSupersededBy`. async read({ scope, key } = {}) { - if (!scope) return { ok: true, entry: this._readUnscoped(key) }; + const entry = this._read({ scope, key }); + return { ok: true, entry: entry ? attachSupersededBy([entry], [this])[0] : null }; + } + + // `read` without the `superseded_by` lookup, so `TwoTierStore` can resolve + // the entry per tier and look successors up once, across both tiers. + _read({ scope, key } = {}) { + if (!scope) return this._readUnscoped(key); const found = this._findByKey(scope, key); - return { - ok: true, - entry: found && isLive(found.entry) ? withReadFields(found.entry) : null, - }; + return found && isLive(found.entry) ? projectEntry(found.entry) : null; } // The unscoped half of `read`, split out so `TwoTierStore` can reuse the // candidate gathering without re-deciding the precedence rule. _readUnscoped(key) { const winner = pickScopeWinner(this._findAllByKey(key).map(scopeCandidate)); - return winner ? withReadFields(winner.entry) : null; + return winner ? projectEntry(winner.entry) : null; } // readMany(refs) → { ok, entries, missing } — batch lookup for an array of @@ -137,12 +144,12 @@ class LocalStore { for (const { scope, key } of refs) { const found = this._findByKey(scope, key); if (found && isLive(found.entry)) { - entries.push(withReadFields(found.entry)); + entries.push(projectEntry(found.entry)); } else { missing.push(`${scope}::${key}`); } } - return { ok: true, entries, missing }; + return { ok: true, entries: attachSupersededBy(entries, [this]), missing }; } // write(...) → { ok, entry } — upsert by scope+key. Preserves `created` and @@ -158,7 +165,7 @@ class LocalStore { async write({ scope, key, value, tags, source_agent, trigger, created_at, ttl_days, clear_ttl, origin_repo, origin_branch, origin_commit, origin_pr, kind, host, - attributes, content_type, supersedes, + attributes, content_type, supersedes, expected_version, _resolved, } = {}) { const now = new Date().toISOString(); const existing = this._findByKey(scope, key); @@ -193,11 +200,47 @@ class LocalStore { : null) ?? (supersedesId && supersedesId === live?.id ? 'a memory cannot supersede itself' : null); if (issue) return { ok: false, error: issue }; + // The record a READ resolves for this scope+key — what the caller last saw. + // Standalone, that is this store's own copy when it is live. Under a + // TwoTierStore the caller passes `_resolved` (internal): the two-tier + // read's answer, which can be the OTHER tier's copy when this tier holds + // none (a home record seen through an active project tier). + const visible = _resolved !== undefined + ? _resolved + : (live && isLive(live) ? live : null); + // Optimistic concurrency (00120), the local twin of the hosted check in + // `memory_write`: the write lands only when the record is still at the + // version the caller last read, and `0` means "must not exist yet". The + // check is against the record a read resolves, so it uses the same + // liveness reads do: an archived OR expired record is not read, so like + // the server it counts as absent (version 0). + // Omitted → no check, exactly as before. + if (expected_version != null) { + if (!Number.isInteger(expected_version) || expected_version < 0) { + return { ok: false, error: 'expected_version must be a non-negative integer' }; + } + const current = visible ? versionOf(visible) : 0; + if (expected_version !== current) { + return { + ok: false, + code: 'version_conflict', + current_version: current, + error: `version_conflict: expected_version ${expected_version} does not match the stored version ${current}` + + `${current === 0 ? ' (the memory does not exist)' : ''}. Re-read the memory, then retry with its current version.`, + }; + } + } let override, expires_at; try { override = normalizeCreatedAt(created_at); + // A write that passed expected_version against an EXPIRED copy revives + // it in place; naming no TTL would keep the past expires_at, leaving the + // record hidden and purgeable. Clear it, as the hosted memory_write does + // (00120) — what a fresh record with no TTL gets. + const revivesExpired = expected_version != null && live != null && !isLive(live) + && parseTtlDays(ttl_days) == null; expires_at = resolveExpiresAt({ - clearTtl: clear_ttl, ttlDays: ttl_days, now, current: existing?.entry.expires_at, + clearTtl: clear_ttl || revivesExpired, ttlDays: ttl_days, now, current: existing?.entry.expires_at, }); } catch (e) { return { ok: false, error: e.message }; @@ -205,12 +248,15 @@ class LocalStore { const dir = this._dir(scope); fs.mkdirSync(dir, { recursive: true }); const created = existing ? existing.entry.created || now : override || now; + const nextTags = Array.isArray(tags) ? tags : []; + const nextValue = value == null ? '' : String(value); + const nextSupersedes = supersedesId ?? live?.supersedes ?? null; const entry = { // Fields this version does not know survive the rewrite (lossless). ...extraFields(existing?.entry), scope, key, - tags: Array.isArray(tags) ? tags : [], + tags: nextTags, source_agent: source_agent || null, trigger: trigger || null, // Provenance keeps the last KNOWN value per field, mirroring the hosted @@ -250,8 +296,33 @@ class LocalStore { id: live?.id || randomUUID(), attributes: mergedAttributes, content_type: contentType, - supersedes: supersedesId ?? live?.supersedes ?? null, - value: value == null ? '' : String(value), + supersedes: nextSupersedes, + // Version (00120), bumped under the hosted trigger's rule: only a change + // a reader can see counts. A first write — or a revive of an archived + // key, which the server inserts as a fresh row — starts at 1. + // + // An EXPIRED copy here still counts on (it is the same record, as the + // server updates the expired row in place and its trigger continues + // the count) even though the expected_version check above saw it as 0. + // + // With no copy in this tier, the count continues from the record the + // two-tier read resolved: this write's copy will SHADOW that one (the + // project tier shadows home), so a reader that saw version 2 next sees + // 2 (identical content) or 3, never a reset to 1. + // + // With BOTH — an expired copy here and a live one resolved from the + // other tier — it continues from the HIGHER of the two, so the version a + // reader sees never goes backwards (an expired project copy at 2 under a + // live home copy at 5 writes 6, not 3). Content is compared against what + // the reader saw. + version: (live || visible) + ? Math.max(live ? versionOf(live) : 0, visible ? versionOf(visible) : 0) + + (contentChanged(visible ?? live, { + value: nextValue, content_type: contentType, attributes: mergedAttributes, + tags: nextTags, supersedes: supersedesId, + }) ? 1 : 0) + : 1, + value: nextValue, }; const file = existing ? existing.file : this._freshPath(dir, key); fs.writeFileSync(file, serializeEntry(entry)); @@ -295,6 +366,9 @@ class LocalStore { attributes: entry.attributes ?? {}, content_type: entry.content_type ?? null, supersedes: entry.supersedes ?? null, + // Verbatim: a relocated record keeps its version. One that never carried + // one lands as null and reads back as 1. + version: entry.version ?? null, value: entry.value == null ? '' : String(entry.value), }; const file = existing ? existing.file : this._freshPath(dir, entry.key); @@ -447,6 +521,68 @@ function scopeCandidate(entry) { return { scope: entry.scope, updated_at: updatedAtOf(entry), entry }; } +// The read projection every local read applies: the shared `withReadFields`, +// plus the record's `version` resolved through `versionOf`, so a file written +// before the column existed reads as version 1 rather than as no version. +function projectEntry(entry) { + return withReadFields({ ...entry, version: versionOf(entry) }); +} + +// Did a write change what a reader reads? The local twin of the hosted +// `memories_bump_version` trigger (00120): value, content_type (null and +// text/markdown are the same type), attributes, tags, or a NEW non-null +// supersedes link. TTL, provenance, taxonomy and the sighting count do not. +// `next.supersedes` is the link the write named, null when it named none. +function contentChanged(prev, next) { + if (String(prev.value ?? '') !== next.value) return true; + if ((prev.content_type || 'text/markdown') !== (next.content_type || 'text/markdown')) return true; + if (canonicalJson(prev.attributes ?? {}) !== canonicalJson(next.attributes ?? {})) return true; + if (JSON.stringify(Array.isArray(prev.tags) ? prev.tags : []) !== JSON.stringify(next.tags)) return true; + return next.supersedes != null && next.supersedes !== (prev.supersedes ?? null); +} + +// JSON with object keys sorted, so two attribute maps that differ only in key +// order compare equal — jsonb equality, which the server's trigger relies on. +function canonicalJson(v) { + if (Array.isArray(v)) return `[${v.map(canonicalJson).join(',')}]`; + if (v && typeof v === 'object') { + return `{${Object.keys(v).sort().map((k) => `${JSON.stringify(k)}:${canonicalJson(v[k])}`).join(',')}}`; + } + return JSON.stringify(v); +} + +/** + * Attach `superseded_by` (00120) to each entry: the ids of the LIVE records, + * across `stores`, whose `supersedes` names the entry's id — newest first. + * Never stored, computed on read like the hosted field, and omitted when empty + * (as the server omits it). One walk per store for the whole batch, never one + * per entry. Only `read`/`readMany` call this: `list`/`search` would pay a + * whole-tree walk on the hook hot path for a field nobody there reads. + */ +function attachSupersededBy(entries, stores) { + const ids = new Set(entries.map((e) => e && e.id).filter((id) => typeof id === 'string' && id)); + if (ids.size === 0) return entries; + const now = new Date(); + const seen = new Set(); + const successors = []; + for (const store of stores) { + for (const { entry } of store._walkEntries()) { + if (!entry || !entry.id || seen.has(entry.id)) continue; + if (!ids.has(entry.supersedes) || !isLive(entry, now)) continue; + seen.add(entry.id); + successors.push(entry); + } + } + if (successors.length === 0) return entries; + successors.sort((a, b) => String(b.updated || '').localeCompare(String(a.updated || ''))); + const byTarget = new Map(); + for (const s of successors) { + if (!byTarget.has(s.supersedes)) byTarget.set(s.supersedes, []); + byTarget.get(s.supersedes).push(s.id); + } + return entries.map((e) => (e && byTarget.has(e.id) ? { ...e, superseded_by: byTarget.get(e.id) } : e)); +} + // A scope string is global when its type segment is `global`. function isGlobalScope(scope) { return String(scope).split('::')[0] === 'global'; @@ -533,13 +669,25 @@ class TwoTierStore { } } const winner = pickScopeWinner([...byScope.values()].map(scopeCandidate)); - return { ok: true, entry: winner ? withReadFields(winner.entry) : null }; + const entry = winner ? projectEntry(winner.entry) : null; + return { ok: true, entry: entry ? attachSupersededBy([entry], this._tiers())[0] : null }; } + const entry = this._read({ scope, key }); + return { ok: true, entry: entry ? attachSupersededBy([entry], this._tiers())[0] : null }; + } + + // The tiers a read can see, project first. + _tiers() { + return this.projectActive() ? [this.project, this.home] : [this.home]; + } + + // A scoped read without the successor lookup: project shadows home. + _read({ scope, key } = {}) { if (this.projectActive()) { - const r = await this.project.read({ scope, key }); - if (r.entry) return r; + const entry = this.project._read({ scope, key }); + if (entry) return entry; } - return this.home.read({ scope, key }); + return this.home._read({ scope, key }); } // readMany(refs) → { ok, entries, missing } — per-ref `read()`, so the same @@ -549,15 +697,21 @@ class TwoTierStore { const entries = []; const missing = []; for (const { scope, key } of refs) { - const r = await this.read({ scope, key }); - if (r.entry) entries.push(r.entry); + const entry = scope ? this._read({ scope, key }) : (await this.read({ scope, key })).entry; + if (entry) entries.push(entry); else missing.push(`${scope}::${key}`); } - return { ok: true, entries, missing }; + return { ok: true, entries: attachSupersededBy(entries, this._tiers()), missing }; } + // The expected_version check (00120) is against the record a two-tier READ + // resolves — project shadows home — not only the target tier's copy: with a + // project tier active and the key held only in home, a reader saw the home + // version, and a write naming it must land (in the project tier, where it + // then shadows home and continues that version). See LocalStore.write. async write(args = {}) { - return this.tierFor(args.scope).write(args); + const resolved = args.key == null ? null : this._read({ scope: args.scope, key: args.key }); + return this.tierFor(args.scope).write({ ...args, _resolved: resolved }); } async putEntry(entry = {}) { diff --git a/packages/cli/src/store/remote.mjs b/packages/cli/src/store/remote.mjs index ce2172146..9d9dfe1e5 100644 --- a/packages/cli/src/store/remote.mjs +++ b/packages/cli/src/store/remote.mjs @@ -370,7 +370,7 @@ class RemoteStore { const { scope, key, value, tags, source_agent, trigger, kind, host, org, ttl_days, clear_ttl, created_at, cited, origin_repo, origin_branch, origin_commit, origin_pr, - attributes, content_type, supersedes, + attributes, content_type, supersedes, expected_version, } = args; const body = { scope, key, value }; if (tags !== undefined) body.tags = tags; @@ -399,6 +399,10 @@ class RemoteStore { if (attributes !== undefined) body.attributes = attributes; if (content_type !== undefined) body.content_type = content_type; if (supersedes !== undefined) body.supersedes = supersedes; + // Optimistic concurrency (00120): sent only when given, so an omitted + // version keeps the unconditional write. A mismatch is a 409 whose error + // carries `code: 'version_conflict'` and the stored version. + if (expected_version !== undefined) body.expected_version = expected_version; const res = await this._rest('/memories', { method: 'POST', body }); // `httpStatus` and `retryAfter` are passed through so a caller can tell the // two 429s apart and honour the server's own backoff. They are DIFFERENT @@ -421,6 +425,9 @@ class RemoteStore { // a failure with every field null and no reason. Passed through the way // `listScopes` already does. unusable: res.unusable ?? false, + // The record's version after the write (00120), or null when the write + // failed or the server predates the column. Additive. + version: Number.isInteger(res.data?.version) ? res.data.version : null, }; } diff --git a/packages/cli/src/surfaces.generated.mjs b/packages/cli/src/surfaces.generated.mjs index 2a7be904a..48a01cf21 100644 --- a/packages/cli/src/surfaces.generated.mjs +++ b/packages/cli/src/surfaces.generated.mjs @@ -128,7 +128,7 @@ export const MCP_TOOL_DEFS = [ "type": "integer", "minimum": 1, "maximum": 365, - "description": "Number of days until the memory auto-expires. Omit for a permanent memory. On an update, supplying ttl_days refreshes the expiry; omitting it leaves the existing expiry unchanged." + "description": "Number of days until the memory auto-expires. Omit for a permanent memory. On an update, supplying ttl_days refreshes the expiry; omitting it leaves the existing expiry unchanged — except on an `expected_version` write that revives an expired memory, where omitting it (and `clear_ttl`) clears the past expiry so the memory is live again." }, "clear_ttl": { "type": "boolean", @@ -173,6 +173,11 @@ export const MCP_TOOL_DEFS = [ "supersedes": { "type": "string", "description": "The id of the memory this one replaces (from `memory.read`). Must name a memory this token can read that has the same owner as the one being written (personal to personal, or the same org); anything else is rejected as not found. Kept on update unless re-supplied." + }, + "expected_version": { + "type": "integer", + "minimum": 0, + "description": "Optimistic concurrency: the `version` you last read. If the stored version differs, the write is rejected with `version_conflict` (naming the current version) and nothing changes. `0` means the memory must not exist yet. An archived or expired memory counts as absent (version `0`), exactly as reads hide it; an expired one is rewritten in place, and with no `ttl_days` / `clear_ttl` on that write its past expiry is cleared. Omit for an unconditional write." } } }, diff --git a/packages/cli/test/list.test.mjs b/packages/cli/test/list.test.mjs index 42bce38fc..ea16cc0ce 100644 --- a/packages/cli/test/list.test.mjs +++ b/packages/cli/test/list.test.mjs @@ -64,6 +64,7 @@ test('normalizeEntry maps updated_at → updated and coerces value/tags', () => origin_repo: null, origin_branch: null, origin_pr: null, + version: null, }); // A row without taxonomy columns normalizes them to null. assert.deepEqual( diff --git a/packages/cli/test/record-extensions.test.mjs b/packages/cli/test/record-extensions.test.mjs index f83772a94..f3bb185ca 100644 --- a/packages/cli/test/record-extensions.test.mjs +++ b/packages/cli/test/record-extensions.test.mjs @@ -11,7 +11,7 @@ import fs from 'node:fs'; import os from 'node:os'; import path from 'node:path'; import { fileURLToPath } from 'node:url'; -import { createLocalStore } from '../src/store/local.mjs'; +import { createLocalStore, createTwoTierStore } from '../src/store/local.mjs'; import { parseEntry } from '../src/store/format.mjs'; const BIN = fileURLToPath(new URL('../bin/lorekit.mjs', import.meta.url)); @@ -212,3 +212,251 @@ test('revive: a revived key is a new record that may supersede its archived pred assert.notEqual(revived.entry.id, first.entry.id); assert.equal(revived.entry.supersedes, first.entry.id); }); + +// ── Version + optimistic concurrency + superseded_by (00120's offline twin) ── + +function cliEnv(home) { + const env = { ...process.env, NO_COLOR: '1', HOME: home, USERPROFILE: home, LOREKIT_HOME: home, LOREKIT_TELEMETRY: '0' }; + delete env.LOREKIT_TOKEN; + delete env.LOREKIT_MCP_URL; + delete env.LOREKIT_ENDPOINT; + return env; +} + +test('version: starts at 1 and bumps only on a content change', async () => { + const store = createLocalStore(tmp('lk-ver-bump-')); + const w = (args) => store.write({ scope: 'global', key: 'k', ...args }); + assert.equal((await w({ value: 'one' })).entry.version, 1); + // An identical rewrite is not a change. + assert.equal((await w({ value: 'one' })).entry.version, 1); + assert.equal((await w({ value: 'two' })).entry.version, 2); + assert.equal((await w({ value: 'two', tags: ['a'] })).entry.version, 3); + assert.equal((await w({ value: 'two', tags: ['a'], attributes: { 'x.y': 1 } })).entry.version, 4); + // The same attribute again merges to the same map: no change. + assert.equal((await w({ value: 'two', tags: ['a'], attributes: { 'x.y': 1 } })).entry.version, 4); + // text/markdown is what null means, so naming it is not a change. + assert.equal((await w({ value: 'two', tags: ['a'], content_type: 'text/markdown' })).entry.version, 4); + const target = '6f1c2a5e-3b4d-4e8f-9a0b-1c2d3e4f5a6b'; + assert.equal((await w({ value: 'two', tags: ['a'], supersedes: target })).entry.version, 5); + // Re-naming the same link, TTL, provenance and taxonomy are not content. + assert.equal((await w({ value: 'two', tags: ['a'], supersedes: target })).entry.version, 5); + assert.equal((await w({ value: 'two', tags: ['a'], ttl_days: 7 })).entry.version, 5); + assert.equal((await w({ value: 'two', tags: ['a'], origin_repo: 'o/r', kind: 'lesson' })).entry.version, 5); + // Archive/restore does not bump; a write to an archived key starts at 1. + await store.archive({ scope: 'global', key: 'k' }); + await store.restore({ scope: 'global', key: 'k' }); + assert.equal((await store.read({ scope: 'global', key: 'k' })).entry.version, 5); + await store.archive({ scope: 'global', key: 'k' }); + assert.equal((await w({ value: 'reborn' })).entry.version, 1); +}); + +test('version: a file written before the column existed reads, and rewrites, from 1', async () => { + const dir = tmp('lk-ver-legacy-'); + const store = createLocalStore(dir); + await store.write({ scope: 'global', key: 'k', value: 'v' }); + const { file } = readOnly(dir); + fs.writeFileSync(file, fs.readFileSync(file, 'utf8').replace(/^version: .*\n/m, '')); + assert.equal((await store.read({ scope: 'global', key: 'k' })).entry.version, 1); + assert.equal((await store.list({ scope: 'global' })).entries[0].version, 1); + assert.equal((await store.write({ scope: 'global', key: 'k', value: 'v2' })).entry.version, 2); +}); + +test('expected_version: a match writes, a mismatch refuses and writes nothing', async () => { + const dir = tmp('lk-ver-cas-'); + const store = createLocalStore(dir); + // 0 = must not exist yet. + assert.equal((await store.write({ scope: 'global', key: 'k', value: 'a', expected_version: 0 })).ok, true); + const again = await store.write({ scope: 'global', key: 'k', value: 'b', expected_version: 0 }); + assert.equal(again.ok, false); + assert.equal(again.code, 'version_conflict'); + assert.equal(again.current_version, 1); + assert.match(again.error, /^version_conflict: expected_version 0 does not match the stored version 1/); + assert.equal(readOnly(dir).entry.value, 'a'); + + assert.equal((await store.write({ scope: 'global', key: 'k', value: 'b', expected_version: 1 })).entry.version, 2); + const stale = await store.write({ scope: 'global', key: 'k', value: 'c', expected_version: 1 }); + assert.equal(stale.code, 'version_conflict'); + assert.equal(stale.current_version, 2); + assert.equal(readOnly(dir).entry.value, 'b'); + + // A key that does not exist is at version 0. + const missing = await store.write({ scope: 'global', key: 'nope', value: 'x', expected_version: 3 }); + assert.equal(missing.current_version, 0); + assert.match(missing.error, /the memory does not exist/); + // An archived record is not live, so it is absent too. + await store.archive({ scope: 'global', key: 'k' }); + assert.equal((await store.write({ scope: 'global', key: 'k', value: 'new', expected_version: 0 })).entry.version, 1); + // Malformed values are refused. + for (const bad of [-1, 1.5, '2']) { + const r = await store.write({ scope: 'global', key: 'z', value: 'x', expected_version: bad }); + assert.equal(r.ok, false); + assert.match(r.error, /expected_version must be a non-negative integer/); + } +}); + +test('expected_version: an expired record counts as absent, as it does for reads', async () => { + const dir = tmp('lk-ver-exp-'); + const store = createLocalStore(dir); + await store.write({ scope: 'global', key: 'k', value: 'one' }); + await store.write({ scope: 'global', key: 'k', value: 'two' }); + // Expire it on disk: every read now hides it. + const { file } = readOnly(dir); + fs.writeFileSync(file, fs.readFileSync(file, 'utf8') + .replace(/^expires_at: .*\n/m, '') + .replace(/^version: /m, `expires_at: ${new Date(Date.now() - 60_000).toISOString()}\nversion: `)); + assert.equal((await store.read({ scope: 'global', key: 'k' })).entry, null); + // The version it last had is refused — a reader sees nothing, version 0 … + const stale = await store.write({ scope: 'global', key: 'k', value: 'x', expected_version: 2 }); + assert.equal(stale.code, 'version_conflict'); + assert.equal(stale.current_version, 0); + // … and 0 lands. The same record is rewritten in place and, as on the + // server, its version continues counting rather than restarting at 1. + const ok = await store.write({ scope: 'global', key: 'k', value: 'three', expected_version: 0, clear_ttl: true }); + assert.equal(ok.ok, true, ok.error); + assert.equal(ok.entry.version, 3); + assert.equal(readOnly(dir).entry.value, 'three'); + // Live again, so 0 now conflicts at its version. + assert.equal((await store.write({ scope: 'global', key: 'k', value: 'y', expected_version: 0 })).current_version, 3); +}); + +test('expected_version: reviving an expired record with no TTL named makes it live again', async () => { + const dir = tmp('lk-ver-exp-ttl-'); + const store = createLocalStore(dir); + const expire = () => { + const { file } = readOnly(dir); + fs.writeFileSync(file, fs.readFileSync(file, 'utf8') + .replace(/^expires_at: .*\n/m, '') + .replace(/^version: /m, `expires_at: ${new Date(Date.now() - 60_000).toISOString()}\nversion: `)); + }; + await store.write({ scope: 'global', key: 'k', value: 'one' }); + expire(); + // No ttl_days, no clear_ttl: keeping the past expires_at would leave the + // write hidden (and purgeable). It is cleared, as a fresh record gets. + const ok = await store.write({ scope: 'global', key: 'k', value: 'two', expected_version: 0 }); + assert.equal(ok.ok, true, ok.error); + assert.equal(ok.entry.expires_at, null); + assert.equal((await store.read({ scope: 'global', key: 'k' })).entry.value, 'two'); + // A TTL named on the revive is honoured. + expire(); + const ttl = await store.write({ scope: 'global', key: 'k', value: 'three', expected_version: 0, ttl_days: 7 }); + assert.ok(Date.parse(ttl.entry.expires_at) > Date.now()); + // Scope of the fix: a write without expected_version keeps the stored TTL. + expire(); + const plain = await store.write({ scope: 'global', key: 'k', value: 'four' }); + assert.ok(Date.parse(plain.entry.expires_at) <= Date.now()); +}); + +test('expected_version: two tiers check against the record a read resolves, and continue its version', async () => { + const home = tmp('lk-ver-2t-home-'); + const project = path.join(tmp('lk-ver-2t-proj-'), '.lorekit'); + const scope = 'repo::o/r'; + // The key lives only in the home tier, at version 2 (written before the + // project tier existed). + const homeOnly = createTwoTierStore({ home, project }); + await homeOnly.write({ scope, key: 'k', value: 'one' }); + assert.equal((await homeOnly.write({ scope, key: 'k', value: 'two' })).entry.version, 2); + // Opt the project tier in: writes now target it, reads fall through to home. + fs.mkdirSync(project, { recursive: true }); + const store = createTwoTierStore({ home, project }); + assert.equal((await store.read({ scope, key: 'k' })).entry.version, 2); + // 0 is refused — a reader sees the home record — and reports its version. + const absent = await store.write({ scope, key: 'k', value: 'x', expected_version: 0 }); + assert.equal(absent.code, 'version_conflict'); + assert.equal(absent.current_version, 2); + // The version the reader saw is accepted, and the project copy that now + // shadows home continues it rather than restarting at 1. + const ok = await store.write({ scope, key: 'k', value: 'three', expected_version: 2 }); + assert.equal(ok.ok, true, ok.error); + assert.equal(ok.entry.version, 3); + assert.equal(readOnly(project).entry.value, 'three'); + assert.equal(readOnly(home).entry.version, 2); + const read = (await store.read({ scope, key: 'k' })).entry; + assert.equal(read.value, 'three'); + assert.equal(read.version, 3); + // From here the project copy is the one checked. + assert.equal((await store.write({ scope, key: 'k', value: 'y', expected_version: 2 })).current_version, 3); + // Identical content copied up does not bump: a fresh home-only key at v1, + // re-written unchanged through the project tier, stays at 1. + await homeOnly.home.write({ scope, key: 'same', value: 'v', tags: ['t'] }); + assert.equal((await store.write({ scope, key: 'same', value: 'v', tags: ['t'], expected_version: 1 })).entry.version, 1); +}); + +test('expected_version: an expired project copy under a live home copy never moves the version backwards', async () => { + const home = tmp('lk-ver-2t-max-home-'); + const project = path.join(tmp('lk-ver-2t-max-proj-'), '.lorekit'); + const scope = 'repo::o/r'; + fs.mkdirSync(project, { recursive: true }); + const store = createTwoTierStore({ home, project }); + // Project copy at v2, then expired on disk. + await store.write({ scope, key: 'k', value: 'p1' }); + await store.write({ scope, key: 'k', value: 'p2' }); + const { file } = readOnly(project); + fs.writeFileSync(file, fs.readFileSync(file, 'utf8') + .replace(/^expires_at: .*\n/m, '') + .replace(/^version: /m, `expires_at: ${new Date(Date.now() - 60_000).toISOString()}\nversion: `)); + // Home copy live at v5. + for (const v of ['h1', 'h2', 'h3', 'h4', 'h5']) await store.home.write({ scope, key: 'k', value: v }); + const seen = (await store.read({ scope, key: 'k' })).entry; + assert.equal(seen.version, 5); + assert.equal(seen.value, 'h5'); + const ok = await store.write({ scope, key: 'k', value: 'next', expected_version: 5 }); + assert.equal(ok.ok, true, ok.error); + assert.ok(ok.entry.version >= 6, `version went backwards: ${ok.entry.version}`); + assert.equal(ok.entry.version, 6); + const read = (await store.read({ scope, key: 'k' })).entry; + assert.equal(read.value, 'next'); + assert.equal(read.version, 6); + // Identical content to what the reader saw does not bump, and still never drops below it. + const { entry: same } = await store.write({ scope, key: 'k', value: 'next', expected_version: 6 }); + assert.equal(same.version, 6); +}); + +test('superseded_by: read and readMany name the live records that supersede this one', async () => { + const store = createLocalStore(tmp('lk-ver-sby-')); + const old = (await store.write({ scope: 'global', key: 'old', value: 'v' })).entry; + // No successor: the field is omitted. + assert.equal('superseded_by' in (await store.read({ scope: 'global', key: 'old' })).entry, false); + const a = (await store.write({ scope: 'repo::o/r', key: 'new-a', value: 'v', supersedes: old.id })).entry; + await new Promise((r) => setTimeout(r, 5)); + const b = (await store.write({ scope: 'global', key: 'new-b', value: 'v', supersedes: old.id })).entry; + const read = (await store.read({ scope: 'global', key: 'old' })).entry; + assert.deepEqual(read.superseded_by, [b.id, a.id], 'newest first'); + assert.deepEqual((await store.read({ key: 'old' })).entry.superseded_by, [b.id, a.id]); + const many = await store.readMany([{ scope: 'global', key: 'old' }, { scope: 'global', key: 'new-b' }]); + assert.deepEqual(many.entries[0].superseded_by, [b.id, a.id]); + assert.equal('superseded_by' in many.entries[1], false); + // An archived successor no longer counts. + await store.archive({ scope: 'repo::o/r', key: 'new-a' }); + assert.deepEqual((await store.read({ scope: 'global', key: 'old' })).entry.superseded_by, [b.id]); +}); + +test('write flags: --expected-version guards the write and a conflict exits non-zero', () => { + const root = tmp('lk-ver-cli-root-'); + const home = tmp('lk-ver-cli-home-'); + const env = cliEnv(home); + const run = (...extra) => spawnSync(process.execPath, [ + BIN, 'write', 'global::cas', ...extra, '--local', '--no-origin', '--dir', root, + ], { encoding: 'utf8', env }); + + const first = run('one', '--expected-version', '0', '--json'); + assert.equal(first.status, 0, first.stderr); + assert.equal(JSON.parse(first.stdout).version, 1); + + const conflict = run('two', '--expected-version', '0'); + assert.equal(conflict.status, 1); + assert.match(conflict.stderr, /version conflict — global::cas is at version 1, not 0\. Nothing was written\./); + + const ok = run('two', '--expected-version', '1', '--json'); + assert.equal(ok.status, 0, ok.stderr); + assert.equal(JSON.parse(ok.stdout).version, 2); + + for (const bad of [['--expected-version'], ['--expected-version', '1.5'], ['--expected-version', 'x']]) { + const res = run('three', ...bad); + assert.equal(res.status, 1, `${bad.join(' ')} must fail`); + assert.match(res.stderr, /--expected-version must be a non-negative integer/); + } + + const show = spawnSync(process.execPath, [BIN, 'show', 'global::cas', '--local', '--json', '--dir', root], { encoding: 'utf8', env }); + assert.equal(show.status, 0, show.stderr); + assert.equal(JSON.parse(show.stdout).offline.record.version, 2); +}); diff --git a/packages/cli/test/store.test.mjs b/packages/cli/test/store.test.mjs index 3902b8987..683106e40 100644 --- a/packages/cli/test/store.test.mjs +++ b/packages/cli/test/store.test.mjs @@ -37,6 +37,7 @@ test('serializeEntry / parseEntry round-trip', () => { attributes: null, content_type: null, supersedes: null, + version: null, value: 'First line of the lesson.\nSecond line with details.', }; const parsed = parseEntry(serializeEntry(entry)); @@ -298,6 +299,7 @@ test('putEntry upserts verbatim by scope+key, preserving created/updated/archive attributes: { 'ci.run_url': 'https://ci.example/1' }, content_type: null, supersedes: null, + version: 3, value: 'preserved body', }; await store.putEntry(entry); @@ -448,6 +450,7 @@ test('origin fields round-trip through the on-disk format', () => { attributes: null, content_type: null, supersedes: null, + version: null, value: 'A lesson learned in a pull request.', }; assert.deepEqual(parseEntry(serializeEntry(entry)), entry); diff --git a/packages/mcp-core/src/db/database.types.ts b/packages/mcp-core/src/db/database.types.ts index ba0e58d67..47c929b46 100644 --- a/packages/mcp-core/src/db/database.types.ts +++ b/packages/mcp-core/src/db/database.types.ts @@ -185,6 +185,7 @@ export type Database = { updated_by: string | null user_id: string | null value: string + version: number } Insert: { archived_at?: string | null @@ -210,6 +211,7 @@ export type Database = { updated_by?: string | null user_id?: string | null value: string + version?: number } Update: { archived_at?: string | null @@ -235,6 +237,7 @@ export type Database = { updated_by?: string | null user_id?: string | null value?: string + version?: number } Relationships: [ { @@ -1027,6 +1030,7 @@ export type Database = { p_clear_ttl?: boolean p_content_type?: string p_created_at?: string + p_expected_version?: number p_key: string p_key_org_access?: string p_key_org_ids?: string[] @@ -1070,6 +1074,7 @@ export type Database = { trigger: string | null updated_at: string value: string + version: number }[] } purge_archived_memories: { diff --git a/packages/mcp-core/src/index.ts b/packages/mcp-core/src/index.ts index dbeb65839..5aaa1cd57 100644 --- a/packages/mcp-core/src/index.ts +++ b/packages/mcp-core/src/index.ts @@ -65,6 +65,11 @@ export { splitCitedIds, isMemoryId, RecordFieldError, + VersionConflictError, + parseExpectedVersion, + groupSupersededBy, + withSupersededBy, + SUPERSEDED_BY_COLUMNS, translateRecordFieldDbError, compactRecordFields, RECORD_FIELD_COLUMNS, diff --git a/packages/mcp-core/src/provenance/record-extensions.spec.ts b/packages/mcp-core/src/provenance/record-extensions.spec.ts index 51a88442d..085ab660e 100644 --- a/packages/mcp-core/src/provenance/record-extensions.spec.ts +++ b/packages/mcp-core/src/provenance/record-extensions.spec.ts @@ -10,6 +10,11 @@ import { splitCitedIds, translateRecordFieldDbError, compactRecordFields, + parseExpectedVersion, + VersionConflictError, + groupSupersededBy, + withSupersededBy, + RECORD_FIELD_COLUMNS, } from './record-extensions.js'; const ID = '6f1c2a5e-3b4d-4e8f-9a0b-1c2d3e4f5a6b'; @@ -77,7 +82,7 @@ describe('parseSupersedes / isMemoryId', () => { describe('parseRecordFields', () => { it('returns all nulls when nothing is supplied, so the RPC keeps what is stored', () => { - expect(parseRecordFields({}, 'body')).toEqual({ attributes: null, contentType: null, supersedes: null }); + expect(parseRecordFields({}, 'body')).toEqual({ attributes: null, contentType: null, supersedes: null, expectedVersion: null }); }); it('requires a JSON body to parse when the content type says JSON', () => { @@ -134,3 +139,84 @@ describe('compactRecordFields', () => { expect(compactRecordFields(row)).toEqual(row); }); }); + +describe('parseExpectedVersion', () => { + it('returns null when omitted, so a write is unchecked as before', () => { + expect(parseExpectedVersion(undefined)).toBeNull(); + expect(parseExpectedVersion(null)).toBeNull(); + expect(parseRecordFields({}, 'v').expectedVersion).toBeNull(); + }); + + it('accepts 0 ("must not exist") and any positive integer', () => { + expect(parseExpectedVersion(0)).toBe(0); + expect(parseExpectedVersion(7)).toBe(7); + expect(parseRecordFields({ expected_version: 3 }, 'v').expectedVersion).toBe(3); + }); + + it.each([ + ['a negative number', -1], + ['a fraction', 1.5], + ['a numeric string', '2'], + ['a boolean', true], + ['NaN', Number.NaN], + ['past int4', 2147483648], + ])('rejects %s', (_label, input) => { + expect(() => parseExpectedVersion(input)).toThrow(RecordFieldError); + }); +}); + +describe('version conflicts', () => { + it('translates memory_write LK003 into a VersionConflictError carrying both versions', () => { + const err = translateRecordFieldDbError({ + code: 'LK003', + message: 'version_conflict: expected_version=2 current_version=5', + }); + expect(err).toBeInstanceOf(VersionConflictError); + const conflict = err as VersionConflictError; + expect(conflict.code).toBe('version_conflict'); + expect(conflict.expectedVersion).toBe(2); + expect(conflict.currentVersion).toBe(5); + expect(conflict.message).toMatch(/^version_conflict: /); + expect(conflict.message).toContain('stored version 5'); + }); + + it('says so when the record does not exist', () => { + const err = translateRecordFieldDbError({ message: 'version_conflict: expected_version=3 current_version=0' }); + expect((err as Error).message).toContain('does not exist'); + }); + + it('is not a RecordFieldError: the input was well-formed', () => { + expect(new VersionConflictError(1, 2)).not.toBeInstanceOf(RecordFieldError); + }); + + it('reads version alongside the record extensions', () => { + expect(RECORD_FIELD_COLUMNS.split(',')).toContain('version'); + expect(compactRecordFields({ id: ID, version: 1, attributes: {}, content_type: 'text/markdown', supersedes: null })) + .toEqual({ id: ID, version: 1 }); + }); +}); + +describe('superseded_by', () => { + const A = '00000000-0000-4000-8000-00000000000a'; + const B = '00000000-0000-4000-8000-00000000000b'; + const C = '00000000-0000-4000-8000-00000000000c'; + + it('groups successors by the id they supersede, keeping their order', () => { + const links = groupSupersededBy([ + { id: B, supersedes: A }, + { id: C, supersedes: A }, + { id: C, supersedes: A }, + { id: A, supersedes: null }, + { supersedes: A }, + ]); + expect(links.get(A)).toEqual([B, C]); + expect(links.size).toBe(1); + }); + + it('attaches superseded_by only when something supersedes the record', () => { + const links = groupSupersededBy([{ id: B, supersedes: A }]); + expect(withSupersededBy({ id: A, value: 'v' }, links)).toEqual({ id: A, value: 'v', superseded_by: [B] }); + expect(withSupersededBy({ id: B, value: 'v' }, links)).toEqual({ id: B, value: 'v' }); + expect(withSupersededBy({ value: 'v' }, links)).toEqual({ value: 'v' }); + }); +}); diff --git a/packages/mcp-core/src/provenance/record-extensions.ts b/packages/mcp-core/src/provenance/record-extensions.ts index 565f5441b..5dc7075ee 100644 --- a/packages/mcp-core/src/provenance/record-extensions.ts +++ b/packages/mcp-core/src/provenance/record-extensions.ts @@ -11,6 +11,13 @@ // cited (ids) a citation that names a record by its stable id instead of // by its `scope::key` address, which can change // +// Two more (00120) make a correction visible and safe: +// version every record's content version, starting at 1; read-only +// expected_version on a write, the version the caller last read. A stale one +// is refused with `version_conflict`; 0 means "must not exist" +// superseded_by computed on read (never stored): the ids of the records +// whose `supersedes` names this one +// // `parseRecordFields` is the single, pure validation gate for all of them. It // is mirrored self-contained into the Deno edge tree // (supabase/functions/_shared/provenance/record-extensions.ts) because BOTH the @@ -53,6 +60,7 @@ export interface RecordFields { attributes: AttributesInput | null; contentType: ContentType | null; supersedes: string | null; + expectedVersion: number | null; } /** The raw, unvalidated write-tool parameters. */ @@ -60,6 +68,30 @@ export interface RecordFieldsInput { attributes?: unknown; content_type?: unknown; supersedes?: unknown; + expected_version?: unknown; +} + +/** + * A write refused because the record moved on since the caller read it + * (00120). A distinct class, not a {@link RecordFieldError}: the request was + * well-formed, and the fix is to re-read and retry, not to change the input. + * REST answers it with 409 and MCP with an `isError` result; both carry the + * stored version so the caller can retry without another read. + */ +export class VersionConflictError extends Error { + readonly code = 'version_conflict'; + readonly expectedVersion: number; + readonly currentVersion: number; + constructor(expectedVersion: number, currentVersion: number) { + super( + `version_conflict: expected_version ${expectedVersion} does not match the stored version ${currentVersion}` + + (currentVersion === 0 ? ' (the memory does not exist)' : '') + + '. Re-read the memory, then retry with its current version.', + ); + this.name = 'VersionConflictError'; + this.expectedVersion = expectedVersion; + this.currentVersion = currentVersion; + } } /** True when `input` is a record id (a lowercase or uppercase uuid). */ @@ -123,6 +155,18 @@ export function parseSupersedes(input: unknown): string | null { return (input as string).trim().toLowerCase(); } +/** + * Validate `expected_version`: a non-negative integer, where 0 means "the + * record must not exist yet". Omitted is `null` — no check, today's behaviour. + */ +export function parseExpectedVersion(input: unknown): number | null { + if (input === undefined || input === null) return null; + if (typeof input !== 'number' || !Number.isInteger(input) || input < 0 || input > 2147483647) { + throw new RecordFieldError(`expected_version must be a non-negative integer: ${String(input)}`); + } + return input; +} + /** * Validate and normalise the record-extension write parameters. * @@ -146,6 +190,7 @@ export function parseRecordFields(input: RecordFieldsInput, value: string): Reco attributes: parseAttributes(input.attributes), contentType, supersedes: parseSupersedes(input.supersedes), + expectedVersion: parseExpectedVersion(input.expected_version), }; } @@ -181,17 +226,30 @@ const DB_FIELD_ERRORS: ReadonlyArray = [ ['memories_attributes_max_keys', `attributes must have <= ${ATTRIBUTES_MAX_KEYS} keys`], ]; -/** Map a database error about a record field to a {@link RecordFieldError}; pass anything else through. */ +/** `memory_write`'s LK003 message (00120): `version_conflict: expected_version=N current_version=M`. */ +const VERSION_CONFLICT_RE = /version_conflict: expected_version=(\d+) current_version=(\d+)/; + +/** + * Map a database error about a record field to a {@link RecordFieldError}, and + * `memory_write`'s version refusal to a {@link VersionConflictError}; pass + * anything else through. + */ export function translateRecordFieldDbError(err: unknown): unknown { const message = (err as { message?: string } | null | undefined)?.message ?? ''; + const conflict = VERSION_CONFLICT_RE.exec(message); + if (conflict) return new VersionConflictError(Number(conflict[1]), Number(conflict[2])); for (const [marker, text] of DB_FIELD_ERRORS) { if (message.includes(marker)) return new RecordFieldError(text); } return err; } -/** The columns a read selects to report the record extensions. */ -export const RECORD_FIELD_COLUMNS = 'attributes,content_type,supersedes'; +/** + * The columns a read selects to report the record extensions. `version` rides + * along and is always returned: unlike the three extensions it has no default + * that says nothing (00120). + */ +export const RECORD_FIELD_COLUMNS = 'attributes,content_type,supersedes,version'; type WithRecordFields = { attributes?: unknown; content_type?: unknown; supersedes?: unknown }; @@ -213,3 +271,39 @@ export function compactRecordFields( ...(supersedes ? { supersedes } : {}), }; } + +/** The columns a `superseded_by` lookup selects: the successor and the id it names. */ +export const SUPERSEDED_BY_COLUMNS = 'id,supersedes'; + +/** + * Group successor rows (`{ id, supersedes }`, newest first) by the id they + * supersede: `superseded_by` for every record on a page, from ONE query over + * the page's ids. Total: a row without both ids is skipped, and a successor + * listed twice is kept once. + */ +export function groupSupersededBy( + rows: ReadonlyArray<{ id?: unknown; supersedes?: unknown }>, +): Map { + const out = new Map(); + for (const row of rows) { + if (typeof row?.id !== 'string' || typeof row.supersedes !== 'string') continue; + const list = out.get(row.supersedes) ?? []; + if (!list.includes(row.id)) list.push(row.id); + out.set(row.supersedes, list); + } + return out; +} + +/** + * Attach `superseded_by` to a read result — only when something supersedes it, + * the same "say nothing when there is nothing to say" rule + * {@link compactRecordFields} applies, so an uncorrected record reads exactly as + * before. + */ +export function withSupersededBy( + row: T, + links: ReadonlyMap, +): T & { superseded_by?: string[] } { + const ids = typeof row.id === 'string' ? links.get(row.id) : undefined; + return ids && ids.length > 0 ? { ...row, superseded_by: ids } : row; +} diff --git a/packages/mcp-core/src/tools/write.spec.ts b/packages/mcp-core/src/tools/write.spec.ts index 33b9fc949..bfdec73e7 100644 --- a/packages/mcp-core/src/tools/write.spec.ts +++ b/packages/mcp-core/src/tools/write.spec.ts @@ -1,6 +1,7 @@ import { describe, it, expect, vi } from 'vitest'; import { write } from './write.js'; import { LimitError, MEMORY_CAP_SQLSTATE } from '../limits/limits.js'; +import { RecordFieldError, VersionConflictError } from '../provenance/record-extensions.js'; import type { SupabaseClient } from '@supabase/supabase-js'; vi.mock('../telemetry.js', () => ({ @@ -13,10 +14,10 @@ vi.mock('../telemetry.js', () => ({ // ── helpers ────────────────────────────────────────────────────────────────── -const fakeResult = { id: 'uuid-1', created_at: '2026-01-01T00:00:00Z' }; +const fakeResult = { id: 'uuid-1', created_at: '2026-01-01T00:00:00Z', version: 1 }; function makeDb( - data: null | { id: string; created_at: string; inserted?: boolean }, + data: null | { id: string; created_at: string; inserted?: boolean; version?: number }, error: null | { message: string; code?: string } = null, ) { return { @@ -391,4 +392,34 @@ describe('write — origin', () => { ).rejects.toThrow(/origin_repo/); expect(db.rpc).not.toHaveBeenCalled(); }); + + // ── expected_version (00120) ─────────────────────────────────────────────── + + it('omits p_expected_version when expected_version is not given, so the write is unchecked', async () => { + const db = makeDb(fakeResult); + await write(db, { scope: 'global', key: 'k', value: 'v' }); + const args = (db.rpc as ReturnType).mock.calls[0][1] as Record; + expect(args).not.toHaveProperty('p_expected_version'); + }); + + it('forwards expected_version (0 included) to the RPC', async () => { + for (const n of [0, 4]) { + const db = makeDb(fakeResult); + await write(db, { scope: 'global', key: 'k', value: 'v', expected_version: n }); + expect(db.rpc).toHaveBeenCalledWith('memory_write', expect.objectContaining({ p_expected_version: n })); + } + }); + + it('rejects a malformed expected_version before touching the DB', async () => { + const db = makeDb(fakeResult); + await expect(write(db, { scope: 'global', key: 'k', value: 'v', expected_version: -1 })).rejects.toThrow(RecordFieldError); + expect(db.rpc).not.toHaveBeenCalled(); + }); + + it('turns the RPC version refusal into a VersionConflictError naming the stored version', async () => { + const db = makeDb(null, { code: 'LK003', message: 'version_conflict: expected_version=1 current_version=3' }); + const err = await write(db, { scope: 'global', key: 'k', value: 'v', expected_version: 1 }).catch((e) => e); + expect(err).toBeInstanceOf(VersionConflictError); + expect((err as VersionConflictError).currentVersion).toBe(3); + }); }); diff --git a/packages/mcp-core/src/tools/write.ts b/packages/mcp-core/src/tools/write.ts index b23621a63..d2bf61022 100644 --- a/packages/mcp-core/src/tools/write.ts +++ b/packages/mcp-core/src/tools/write.ts @@ -61,6 +61,8 @@ export const WriteInputSchema = z.object({ attributes: z.record(z.unknown()).optional(), content_type: z.string().optional(), supersedes: z.string().optional(), + // Optimistic concurrency (00120): the version last read; 0 = must not exist. + expected_version: z.number().optional(), }); export type WriteInput = z.infer; @@ -69,7 +71,7 @@ export async function write( db: SupabaseClient, raw: unknown, userId: string | null = null, -): Promise<{ id: string; created_at: string; expires_at?: string | null }> { +): Promise<{ id: string; created_at: string; version: number; expires_at?: string | null }> { const input = WriteInputSchema.parse(raw); const createdAt = parseCreatedAt(input.created_at); // parseTtl validates individual bounds and mutual exclusivity. Zod has already @@ -147,6 +149,8 @@ export async function write( p_attributes: record.attributes, p_content_type: record.contentType, p_supersedes: record.supersedes, + // Omitted = unchecked, exactly as before 00120. + ...(record.expectedVersion !== null ? { p_expected_version: record.expectedVersion } : {}), }) .single(); @@ -157,6 +161,7 @@ export async function write( created_at: string; inserted?: boolean; expires_at?: string | null; + version: number; }; await recordAudit( db, @@ -171,9 +176,10 @@ export async function write( ); span.setStatus({ code: SpanStatusCode.UNSET }); - const result: { id: string; created_at: string; expires_at?: string | null } = { + const result: { id: string; created_at: string; version: number; expires_at?: string | null } = { id: row.id, created_at: row.created_at, + version: row.version, }; // Only include expires_at in the response when it was explicitly requested // (i.e. a TTL was supplied) — omitting it keeps the response stable for diff --git a/packages/schemas/src/domain/memory.ts b/packages/schemas/src/domain/memory.ts index fbd43d761..454107af7 100644 --- a/packages/schemas/src/domain/memory.ts +++ b/packages/schemas/src/domain/memory.ts @@ -77,6 +77,12 @@ export const MemoryWriteSchema = z.object({ attributes: z.record(z.union([z.string(), z.number(), z.boolean(), z.null()])).optional(), content_type: z.string().optional(), supersedes: z.string().optional(), + // Optimistic concurrency (00120): the `version` the caller last read. A write + // whose expected_version is not the stored version is refused with + // `version_conflict` and changes nothing; 0 means "the key must not exist + // yet". Omitted = unchecked, exactly as before. The shared + // `parseExpectedVersion` owns the rule; this only admits the wire type. + expected_version: z.number().optional(), }); export type MemoryWrite = z.infer; @@ -928,6 +934,10 @@ export type ReadActivityResponse = z.infer; * omitted for the origin fields' reason: each has write-path validation (a * merge, a JSON body check, a visibility check on the target) that lives in * `memory_write`, and a column patch would bypass it. Set them with a write. + * + * `expected_version` (00120) STAYS: the handler turns it into a predicate on + * the update itself (`version = n`), so the check and the write are one + * statement. It is not a column and is stripped from the patch. */ export const UpdateMemoryBodySchema = MemoryWriteSchema .omit({ @@ -977,6 +987,13 @@ export const MemoryEntrySchema = z.object({ attributes: z.record(z.union([z.string(), z.number(), z.boolean()])).optional(), content_type: z.string().optional(), supersedes: z.string().uuid().nullable().optional(), + // The record's content version (00120): 1 on create, +1 on every change to + // value, content_type, attributes, tags or supersedes. What `expected_version` + // compares against. Optional for a pre-00120 backend, like the fields above. + version: z.number().int().min(1).optional(), + // Computed on read, never stored (00120): the ids of the live records the + // caller can see whose `supersedes` names this one. Omitted when none do. + superseded_by: z.array(z.string().uuid()).optional(), // Recurrence — how many times this lesson has been written (00059). The // column is NOT NULL DEFAULT 1, so a live row always has >= 1; optional here // for the same reason kind/host are, so a client reading a response from a @@ -1046,7 +1063,7 @@ export type MemoryEntry = z.infer; export const MEMORY_SELECT = 'id,scope,key,value,tags,source_agent,trigger,created_at,updated_at,expires_at,archived_at,' + 'origin_repo,origin_branch,origin_commit,origin_pr,kind,host,seen_count,' - + 'attributes,content_type,supersedes,' + + 'attributes,content_type,supersedes,version,' + 'read_count,opened_count,last_read_at,last_opened_at,cited_count,last_cited_at,' + 'org_id,created_by,updated_by,orgs(id,name,slug)'; diff --git a/packages/schemas/src/llms/template.md b/packages/schemas/src/llms/template.md index 23151a35a..f4141a18f 100644 --- a/packages/schemas/src/llms/template.md +++ b/packages/schemas/src/llms/template.md @@ -425,7 +425,23 @@ Two shapes, decided by whether the tool ran. That is the MCP spec's shape, so the model can see the failure and self-correct rather than having the client library swallow it. You get it for anything you can fix: the memory cap, a malformed scope, an invalid `ttl_*` or `created_at`, an -unresolvable org slug, an insufficient org role. +unresolvable org slug, an insufficient org role, and a stale `expected_version`. + +**A version conflict** is how a conditional write fails. Every memory carries a +`version` (1 on create, +1 each time its value, tags, `attributes`, +`content_type` or `supersedes` changes; a TTL change or an identical rewrite +leaves it alone). `memory.write` returns it, and `memory.read` / `.list` / +`.search` return it on every entry. Send it back as `expected_version` and the +write applies only if nobody changed the memory in between (`0` = it must not +exist yet). Otherwise nothing is written and the text starts with +`version_conflict:` and names the stored version — re-read, reconcile, and retry +with that number. Over REST the same refusal is a `409` with +`code: "version_conflict"` and `current_version` in the body. + +**A corrected memory says so.** A read returns `superseded_by` — the ids of the +live memories you can see whose `supersedes` names this one — when there is at +least one. Superseded memories still appear in lists and searches; they are +marked, not hidden. **Writing a client? Check `result.isError`.** The HTTP status is `200` and there is no `error` member, so a client that only tests for `response.error` reads diff --git a/packages/schemas/src/openapi/spec.ts b/packages/schemas/src/openapi/spec.ts index 31a591dfb..5e626e28b 100644 --- a/packages/schemas/src/openapi/spec.ts +++ b/packages/schemas/src/openapi/spec.ts @@ -231,7 +231,8 @@ export function generateSpec(baseUrl = 'https://pqokxlhvnosogizsjztg.supabase.co registry.registerPath({ method: 'post', path: '/memories', summary: 'Create or update a memory', tags: ['Memories'], security, request: { body: { content: { 'application/json': { schema: CreateMemoryBodySchema } } } }, - responses: { 201: memoryResponse('Created'), 400: errorResponse, 401: errorResponse, 429: errorResponse }, + description: 'With `expected_version`, the write is conditional: a `409` with `code: "version_conflict"` and the stored `current_version` means nothing was written (`0` = the memory must not exist yet).', + responses: { 201: memoryResponse('Created'), 400: errorResponse, 401: errorResponse, 409: errorResponse, 429: errorResponse }, }); registry.registerPath({ method: 'post', path: '/memories/search', summary: 'Search memories with OR+AND filtering', tags: ['Memories'], @@ -585,12 +586,13 @@ export function generateSpec(baseUrl = 'https://pqokxlhvnosogizsjztg.supabase.co }); registry.registerPath({ method: 'patch', path: '/memories/{id}', summary: 'Update a memory', tags: ['Memories'], + description: 'With `expected_version`, the update applies only while the memory is at that version; otherwise a `409` with `code: "version_conflict"` and the stored `current_version`, and nothing changes. An archived or expired memory is `404`, exactly as on `GET /memories/{id}` — and is `404`, not `409`, with an `expected_version` too.', security, request: { params: MemoryIdParamsSchema, body: { content: { 'application/json': { schema: UpdateMemoryBodySchema } } }, }, - responses: { 200: memoryResponse('Updated memory'), 400: errorResponse, 404: errorResponse, 401: errorResponse }, + responses: { 200: memoryResponse('Updated memory'), 400: errorResponse, 404: errorResponse, 409: errorResponse, 401: errorResponse }, }); registry.registerPath({ method: 'delete', path: '/memories/{id}', diff --git a/packages/schemas/src/shared/tool-catalog.ts b/packages/schemas/src/shared/tool-catalog.ts index b0d2afb66..9eeb1ff20 100644 --- a/packages/schemas/src/shared/tool-catalog.ts +++ b/packages/schemas/src/shared/tool-catalog.ts @@ -321,7 +321,7 @@ export const MCP_TOOLS = [ minimum: 1, maximum: 365, description: - 'Number of days until the memory auto-expires. Omit for a permanent memory. On an update, supplying ttl_days refreshes the expiry; omitting it leaves the existing expiry unchanged.', + 'Number of days until the memory auto-expires. Omit for a permanent memory. On an update, supplying ttl_days refreshes the expiry; omitting it leaves the existing expiry unchanged — except on an `expected_version` write that revives an expired memory, where omitting it (and `clear_ttl`) clears the past expiry so the memory is live again.', }, clear_ttl: { type: 'boolean', @@ -370,13 +370,20 @@ export const MCP_TOOLS = [ description: 'The id of the memory this one replaces (from `memory.read`). Must name a memory this token can read that has the same owner as the one being written (personal to personal, or the same org); anything else is rejected as not found. Kept on update unless re-supplied.', }, + expected_version: { + type: 'integer', + minimum: 0, + description: + 'Optimistic concurrency: the `version` you last read. If the stored version differs, the write is rejected with `version_conflict` (naming the current version) and nothing changes. `0` means the memory must not exist yet. An archived or expired memory counts as absent (version `0`), exactly as reads hide it; an expired one is rewritten in place, and with no `ttl_days` / `clear_ttl` on that write its past expiry is cleared. Omit for an unconditional write.', + }, }, }, returns: - '`{ "id": "", "created_at": "" }` — the `id` is stable across updates and is what `supersedes` and `cited` accept — plus optional `"expires_at"` and `"notice"` (when a write fell back to personal because the scope is bound to an org the caller cannot write to).', + '`{ "id": "", "created_at": "", "version": }` — the `id` is stable across updates and is what `supersedes` and `cited` accept; `version` is the content version after this write, the value to pass as `expected_version` next time — plus optional `"expires_at"` and `"notice"` (when a write fell back to personal because the scope is bound to an org the caller cannot write to).', notes: [ '**Provenance (`origin_*`):** `scope` says where a lesson *applies*; the four `origin_*` fields say where it was *recorded from*. Each is independently optional and the last KNOWN value wins — on an update, a field you omit keeps whatever a previous write recorded rather than being erased. A malformed value is rejected, never silently dropped.', '**Record extensions (`attributes`, `content_type`, `supersedes`):** additive, and a write that omits them behaves exactly as before. `attributes` holds metadata no column anticipates — a CI run URL, a PR number, an owning team — under namespaced keys; it is merged on update, so name only the keys you change. `content_type: "application/json"` marks a value a program should parse (a CI result, a structured finding), and the server rejects one that does not parse. `supersedes` links a corrected lesson to the one it replaces, by the old one\u2019s `id`, so the history survives a re-key; the target must have the same owner as the memory being written (personal to personal, or the same org), and any other id, including one you cannot read, is rejected exactly like one that does not exist.', + '**Versions (`version`, `expected_version`):** every memory starts at version 1 and gains 1 each time its content changes — `value`, `content_type`, `attributes`, `tags`, or `supersedes` being set. A TTL change, a re-write of identical content, archive/restore and read counters do not move it, so a reader can tell a corrected memory from a re-sighted one. Pass `expected_version` to make a write conditional: a mismatch fails with a `version_conflict` error stating the current version, and nothing is written. Re-read, reconcile, and retry with that version.', '**Citations (`cited`):** the one signal read counts cannot produce. A lesson injected at session start is already in your context and is applied without ever being fetched, so LoreKit can measure whether lore was *looked up* but not whether it *worked* — `cited` is where you say so. Each entry is a `scope::key` you were shown; they raise the cited lesson\u2019s `cited_count` and are grouped by the run\u2019s correlation id. Recorded once per run, so a retried write does not inflate anything, and every failure mode is silent: an unknown reference, a self-citation, and anything past 32 entries are dropped and the write still succeeds.', '**Scope→org binding:** An org admin can bind a scope (e.g. a repo) — or a WILDCARD prefix such as `repo::owner/*` or `branch::owner/repo::*`, matching every scope under it — to an org. A write under a bound scope with no explicit `org` auto-routes to that org for write-capable members, resolved MOST-SPECIFIC-WINS when several bindings match (an exact scope beats any wildcard; a longer wildcard prefix beats a shorter one). A non-member\u2019s write falls back to personal (never rejected) with a `notice` \u2014 except from a token limited to selected orgs (`orgs_only`), which may not write personal memories: that write is refused, so pass `org`.', ], @@ -408,7 +415,7 @@ export const MCP_TOOLS = [ refs: { type: 'array', items: { type: 'string' }, description: 'Batch mode: one or more `scope::key` references, fetched in a single call. Cannot be combined with `scope`/`key`. Each entry is parsed by the same reference grammar `memory.write`\'s `cited` field uses (`scope::key`, verbatim scope — never lowercased). Silently truncated past 32 entries.' }, }, }, - returns: '`{ "id": "", "value": "", "updated_at": "", "scope": "" }` or `null` if not found. `id` is the record\u2019s stable identity — the value `supersedes` and `cited` accept. `attributes`, `content_type` and `supersedes` appear only when they hold something other than their default (an empty map, `text/markdown`, no link). `scope` names the scope that ANSWERED — always present, so an unscoped read can tell a `global` hit from a `repo::…` one. When the key also existed in other scopes, an additional `other_scopes` array names them in precedence order; it is omitted entirely when the key was unambiguous. With `refs`, instead returns `{ "entries": [{ "id", "scope", "key", "value", "updated_at" }], "missing": ["scope::key", …] }` — `missing` names every well-formed reference within the first 32 that matched no lesson.', + returns: '`{ "id": "", "value": "", "updated_at": "", "scope": "", "version": }` or `null` if not found. `id` is the record\u2019s stable identity — the value `supersedes` and `cited` accept. `version` is its content version (see `memory.write`\u2019s `expected_version`). `attributes`, `content_type` and `supersedes` appear only when they hold something other than their default (an empty map, `text/markdown`, no link). `superseded_by` lists the ids of the live memories you can read whose `supersedes` names this one \u2014 present only when there is at least one, and a sign this memory has been corrected. `scope` names the scope that ANSWERED — always present, so an unscoped read can tell a `global` hit from a `repo::…` one. When the key also existed in other scopes, an additional `other_scopes` array names them in precedence order; it is omitted entirely when the key was unambiguous. With `refs`, instead returns `{ "entries": [{ "id", "scope", "key", "value", "updated_at", "version" }], "missing": ["scope::key", …] }` (each entry carrying `superseded_by` the same way) — `missing` names every well-formed reference within the first 32 that matched no lesson.', notes: [ '**Scope is optional.** `memory.read { key }` with no scope resolves that key across every scope you can see and returns the most specific match — `project` beats `branch` beats `repo` beats `global`, with ties broken by most-recently-updated then scope ascending. This is the shape to use when a key reached you through a session-start injection and you do not know which scope it came from. Pass `scope` when you DO know it: it is one indexed row instead of a fan-out, and it removes the ambiguity entirely. Either way the response names the scope that answered.', '**Exactly one of two argument shapes is required:** `key` (optionally with `scope`) for a single read, **or** `refs` alone for a batch read. They cannot be combined — a call carrying both is rejected, and so is one carrying neither. No argument is required on its own, which is why none is marked required above: the table cannot express "one of these two groups", so this rule is the whole of it. The schema deliberately does NOT advertise the alternatives as a top-level `oneOf` either — Amazon Bedrock rejects a tool whose `input_schema` carries one, and it fails the entire request rather than that one tool. Both shapes are validated by the server on every call regardless.', @@ -439,7 +446,7 @@ export const MCP_TOOLS = [ view: { type: 'string', enum: ['full', 'summary'], default: 'full', description: 'full (default) returns each entry\'s complete `value`. summary omits `value` and returns `value_bytes` + a 200-character `preview` instead — the cheap discovery read for deciding WHICH lessons to then fetch with `memory.read`.' }, }, }, - returns: '`{ "entries": [{ "scope", "key", "value", "tags", "updated_at" }], "hasMore": boolean, "nextCursor": string | null }` — every entry names its own `scope`, so an unscoped listing stays readable; newest-first (recency mode) or ranked by salience+recency then MMR-diversified (rank mode). Because rank mode diversifies, entries are NOT strictly score-descending — a more diverse lower-scored lesson can precede a higher-scored near-duplicate. Pass `nextCursor` back as `cursor` to paginate — recency mode only. Rank mode is a single bounded top-N page: `hasMore` is always false and `nextCursor` always null. With `view: "summary"` each entry is `{ "scope", "key", "tags", "updated_at", "value_bytes", "preview" }` — `value` is omitted entirely.', + returns: '`{ "entries": [{ "scope", "key", "value", "tags", "updated_at", "version" }], "hasMore": boolean, "nextCursor": string | null }` — plus `superseded_by` on an entry that a newer memory supersedes (superseded entries are still listed, only marked) — every entry names its own `scope`, so an unscoped listing stays readable; newest-first (recency mode) or ranked by salience+recency then MMR-diversified (rank mode). Because rank mode diversifies, entries are NOT strictly score-descending — a more diverse lower-scored lesson can precede a higher-scored near-duplicate. Pass `nextCursor` back as `cursor` to paginate — recency mode only. Rank mode is a single bounded top-N page: `hasMore` is always false and `nextCursor` always null. With `view: "summary"` each entry is `{ "scope", "key", "tags", "updated_at", "value_bytes", "preview" }` — `value` is omitted entirely.', }, { name: 'memory.delete', @@ -490,7 +497,7 @@ export const MCP_TOOLS = [ cursor: { type: 'string', description: 'Opaque cursor from a previous response\'s `nextCursor`. Omit to start from the first page.' }, }, }, - returns: '`{ "entries": [{ "key", "value", "scope", "tags", "rank" }], "hasMore": boolean, "nextCursor": string | null }`. Pass `nextCursor` back as `cursor` to retrieve the next page.', + returns: '`{ "entries": [{ "key", "value", "scope", "tags", "rank", "version" }], "hasMore": boolean, "nextCursor": string | null }` — plus `superseded_by` on an entry that a newer memory supersedes (superseded entries are still returned, only marked). Pass `nextCursor` back as `cursor` to retrieve the next page.', }, { name: 'memory.archive', diff --git a/packages/web/public/llms.txt b/packages/web/public/llms.txt index 88bbb8fd9..7595afd09 100644 --- a/packages/web/public/llms.txt +++ b/packages/web/public/llms.txt @@ -244,7 +244,7 @@ Store or update a lesson. | `host` | | string | The owning skill or agent (e.g. `reviewer`, `aw`, `ci-auto-fix`). Omit to have it inferred from a `loop::-lessons` tag. | | `created_at` | | string | Optional ISO 8601 creation date. Use when migrating a pre-existing memory so it is dated by its original time instead of now. Rejected if invalid or in the future. Applies only when the memory is first created. _(date-time)_ | | `org` | | string | Org slug to write under (org-owned write). Omit for a personal memory. You must be a write-capable member (member/admin/owner, not viewer) of the org, verified server-side — supplying an org slug you are not authorized for is rejected. | -| `ttl_days` | | integer | Number of days until the memory auto-expires. Omit for a permanent memory. On an update, supplying ttl_days refreshes the expiry; omitting it leaves the existing expiry unchanged. _(1–365)_ | +| `ttl_days` | | integer | Number of days until the memory auto-expires. Omit for a permanent memory. On an update, supplying ttl_days refreshes the expiry; omitting it leaves the existing expiry unchanged — except on an `expected_version` write that revives an expired memory, where omitting it (and `clear_ttl`) clears the past expiry so the memory is live again. _(1–365)_ | | `clear_ttl` | | boolean | When true, removes the existing expiry and makes the memory permanent again. Takes precedence over ttl_days when both are supplied. | | `origin_repo` | | string | Provenance: the owner/name of the repository this memory was recorded from. Distinct from `scope`, which says where the lesson APPLIES. | | `origin_branch` | | string | Provenance: the git branch this memory was recorded from. Stored verbatim (case-sensitive) so its GitHub link resolves. | @@ -254,17 +254,20 @@ Store or update a lesson. | `attributes` | | object | Open metadata as namespaced keys with scalar values, e.g. `{"ci.run_url": "https://…", "vcs.pr": 482}`. Keys are lowercase and dot-separated; at most 32 keys, each key at most 128 characters and each string value at most 1024. On an update the map is MERGED into what is stored, and a key set to null is removed. | | `content_type` | | string | What `value` is. Defaults to `text/markdown`. With `application/json` the value must parse as JSON. Omit on an update to keep the stored type. | | `supersedes` | | string | The id of the memory this one replaces (from `memory.read`). Must name a memory this token can read that has the same owner as the one being written (personal to personal, or the same org); anything else is rejected as not found. Kept on update unless re-supplied. | +| `expected_version` | | integer | Optimistic concurrency: the `version` you last read. If the stored version differs, the write is rejected with `version_conflict` (naming the current version) and nothing changes. `0` means the memory must not exist yet. An archived or expired memory counts as absent (version `0`), exactly as reads hide it; an expired one is rewritten in place, and with no `ttl_days` / `clear_ttl` on that write its past expiry is cleared. Omit for an unconditional write. _(min 0)_ | Requires **write** permission. MCP annotations: write, destructive, not idempotent, closed-world. -Returns: `{ "id": "", "created_at": "" }` — the `id` is stable across updates and is what `supersedes` and `cited` accept — plus optional `"expires_at"` and `"notice"` (when a write fell back to personal because the scope is bound to an org the caller cannot write to). +Returns: `{ "id": "", "created_at": "", "version": }` — the `id` is stable across updates and is what `supersedes` and `cited` accept; `version` is the content version after this write, the value to pass as `expected_version` next time — plus optional `"expires_at"` and `"notice"` (when a write fell back to personal because the scope is bound to an org the caller cannot write to). **Provenance (`origin_*`):** `scope` says where a lesson *applies*; the four `origin_*` fields say where it was *recorded from*. Each is independently optional and the last KNOWN value wins — on an update, a field you omit keeps whatever a previous write recorded rather than being erased. A malformed value is rejected, never silently dropped. **Record extensions (`attributes`, `content_type`, `supersedes`):** additive, and a write that omits them behaves exactly as before. `attributes` holds metadata no column anticipates — a CI run URL, a PR number, an owning team — under namespaced keys; it is merged on update, so name only the keys you change. `content_type: "application/json"` marks a value a program should parse (a CI result, a structured finding), and the server rejects one that does not parse. `supersedes` links a corrected lesson to the one it replaces, by the old one’s `id`, so the history survives a re-key; the target must have the same owner as the memory being written (personal to personal, or the same org), and any other id, including one you cannot read, is rejected exactly like one that does not exist. +**Versions (`version`, `expected_version`):** every memory starts at version 1 and gains 1 each time its content changes — `value`, `content_type`, `attributes`, `tags`, or `supersedes` being set. A TTL change, a re-write of identical content, archive/restore and read counters do not move it, so a reader can tell a corrected memory from a re-sighted one. Pass `expected_version` to make a write conditional: a mismatch fails with a `version_conflict` error stating the current version, and nothing is written. Re-read, reconcile, and retry with that version. + **Citations (`cited`):** the one signal read counts cannot produce. A lesson injected at session start is already in your context and is applied without ever being fetched, so LoreKit can measure whether lore was *looked up* but not whether it *worked* — `cited` is where you say so. Each entry is a `scope::key` you were shown; they raise the cited lesson’s `cited_count` and are grouped by the run’s correlation id. Recorded once per run, so a retried write does not inflate anything, and every failure mode is silent: an unknown reference, a self-citation, and anything past 32 entries are dropped and the write still succeeds. **Scope→org binding:** An org admin can bind a scope (e.g. a repo) — or a WILDCARD prefix such as `repo::owner/*` or `branch::owner/repo::*`, matching every scope under it — to an org. A write under a bound scope with no explicit `org` auto-routes to that org for write-capable members, resolved MOST-SPECIFIC-WINS when several bindings match (an exact scope beats any wildcard; a longer wildcard prefix beats a shorter one). A non-member’s write falls back to personal (never rejected) with a `notice` — except from a token limited to selected orgs (`orgs_only`), which may not write personal memories: that write is refused, so pass `org`. @@ -285,7 +288,7 @@ Requires **read** permission. MCP annotations: read-only, closed-world. -Returns: `{ "id": "", "value": "", "updated_at": "", "scope": "" }` or `null` if not found. `id` is the record’s stable identity — the value `supersedes` and `cited` accept. `attributes`, `content_type` and `supersedes` appear only when they hold something other than their default (an empty map, `text/markdown`, no link). `scope` names the scope that ANSWERED — always present, so an unscoped read can tell a `global` hit from a `repo::…` one. When the key also existed in other scopes, an additional `other_scopes` array names them in precedence order; it is omitted entirely when the key was unambiguous. With `refs`, instead returns `{ "entries": [{ "id", "scope", "key", "value", "updated_at" }], "missing": ["scope::key", …] }` — `missing` names every well-formed reference within the first 32 that matched no lesson. +Returns: `{ "id": "", "value": "", "updated_at": "", "scope": "", "version": }` or `null` if not found. `id` is the record’s stable identity — the value `supersedes` and `cited` accept. `version` is its content version (see `memory.write`’s `expected_version`). `attributes`, `content_type` and `supersedes` appear only when they hold something other than their default (an empty map, `text/markdown`, no link). `superseded_by` lists the ids of the live memories you can read whose `supersedes` names this one — present only when there is at least one, and a sign this memory has been corrected. `scope` names the scope that ANSWERED — always present, so an unscoped read can tell a `global` hit from a `repo::…` one. When the key also existed in other scopes, an additional `other_scopes` array names them in precedence order; it is omitted entirely when the key was unambiguous. With `refs`, instead returns `{ "entries": [{ "id", "scope", "key", "value", "updated_at", "version" }], "missing": ["scope::key", …] }` (each entry carrying `superseded_by` the same way) — `missing` names every well-formed reference within the first 32 that matched no lesson. **Scope is optional.** `memory.read { key }` with no scope resolves that key across every scope you can see and returns the most specific match — `project` beats `branch` beats `repo` beats `global`, with ties broken by most-recently-updated then scope ascending. This is the shape to use when a key reached you through a session-start injection and you do not know which scope it came from. Pass `scope` when you DO know it: it is one indexed row instead of a fan-out, and it removes the ambiguity entirely. Either way the response names the scope that answered. @@ -314,7 +317,7 @@ Requires **read** permission. MCP annotations: read-only, closed-world. -Returns: `{ "entries": [{ "scope", "key", "value", "tags", "updated_at" }], "hasMore": boolean, "nextCursor": string | null }` — every entry names its own `scope`, so an unscoped listing stays readable; newest-first (recency mode) or ranked by salience+recency then MMR-diversified (rank mode). Because rank mode diversifies, entries are NOT strictly score-descending — a more diverse lower-scored lesson can precede a higher-scored near-duplicate. Pass `nextCursor` back as `cursor` to paginate — recency mode only. Rank mode is a single bounded top-N page: `hasMore` is always false and `nextCursor` always null. With `view: "summary"` each entry is `{ "scope", "key", "tags", "updated_at", "value_bytes", "preview" }` — `value` is omitted entirely. +Returns: `{ "entries": [{ "scope", "key", "value", "tags", "updated_at", "version" }], "hasMore": boolean, "nextCursor": string | null }` — plus `superseded_by` on an entry that a newer memory supersedes (superseded entries are still listed, only marked) — every entry names its own `scope`, so an unscoped listing stays readable; newest-first (recency mode) or ranked by salience+recency then MMR-diversified (rank mode). Because rank mode diversifies, entries are NOT strictly score-descending — a more diverse lower-scored lesson can precede a higher-scored near-duplicate. Pass `nextCursor` back as `cursor` to paginate — recency mode only. Rank mode is a single bounded top-N page: `hasMore` is always false and `nextCursor` always null. With `view: "summary"` each entry is `{ "scope", "key", "tags", "updated_at", "value_bytes", "preview" }` — `value` is omitted entirely. --- @@ -353,7 +356,7 @@ Requires **read** permission. MCP annotations: read-only, closed-world. -Returns: `{ "entries": [{ "key", "value", "scope", "tags", "rank" }], "hasMore": boolean, "nextCursor": string | null }`. Pass `nextCursor` back as `cursor` to retrieve the next page. +Returns: `{ "entries": [{ "key", "value", "scope", "tags", "rank", "version" }], "hasMore": boolean, "nextCursor": string | null }` — plus `superseded_by` on an entry that a newer memory supersedes (superseded entries are still returned, only marked). Pass `nextCursor` back as `cursor` to retrieve the next page. --- @@ -973,7 +976,23 @@ Two shapes, decided by whether the tool ran. That is the MCP spec's shape, so the model can see the failure and self-correct rather than having the client library swallow it. You get it for anything you can fix: the memory cap, a malformed scope, an invalid `ttl_*` or `created_at`, an -unresolvable org slug, an insufficient org role. +unresolvable org slug, an insufficient org role, and a stale `expected_version`. + +**A version conflict** is how a conditional write fails. Every memory carries a +`version` (1 on create, +1 each time its value, tags, `attributes`, +`content_type` or `supersedes` changes; a TTL change or an identical rewrite +leaves it alone). `memory.write` returns it, and `memory.read` / `.list` / +`.search` return it on every entry. Send it back as `expected_version` and the +write applies only if nobody changed the memory in between (`0` = it must not +exist yet). Otherwise nothing is written and the text starts with +`version_conflict:` and names the stored version — re-read, reconcile, and retry +with that number. Over REST the same refusal is a `409` with +`code: "version_conflict"` and `current_version` in the body. + +**A corrected memory says so.** A read returns `superseded_by` — the ids of the +live memories you can see whose `supersedes` names this one — when there is at +least one. Superseded memories still appear in lists and searches; they are +marked, not hidden. **Writing a client? Check `result.isError`.** The HTTP status is `200` and there is no `error` member, so a client that only tests for `response.error` reads diff --git a/packages/web/src/content/docs/use-cases.mdx b/packages/web/src/content/docs/use-cases.mdx index 153d22649..0d21ed5c8 100644 --- a/packages/web/src/content/docs/use-cases.mdx +++ b/packages/web/src/content/docs/use-cases.mdx @@ -117,12 +117,13 @@ jq -nc --argjson flaky "$FLAKY" \ || echo "LoreKit write failed (exit $?) — not failing the build." ``` -Five things worth copying from this snippet: +Six things worth copying from this snippet: - **Keep the TTL short — about a week — and always pass it.** `expires_at` is recomputed as `now() + ttl_days` on *every* write, so a record the job rewrites each run never expires while the job keeps running. The TTL measures how long the job has been *silent*, which is exactly when its contents stopped being true: 7 days for anything running at least daily, 14 for nightly, 30 for weekly. It also caps the damage if someone keys on the run id by mistake — the store drains instead of climbing to the memory cap. Reserve `clear_ttl` for a record whose absence is worse than its staleness. Never omit both, or the write silently inherits whatever default the repo configured for lessons. - **A write-only `lk_wo_…` token** in `LOREKIT_TOKEN` is enough for a job that only records. It cannot read anything back, so a leaked CI token cannot exfiltrate the team's lore. Use `lk_ro_…` for a read-only job. - **`--content-type application/json` says the value is for a program.** The server rejects a value that does not parse, so a reader that trusts the type never meets a half-written record. `--attributes` carries the run URL as metadata (`ci.run_url`), where `memory.read` returns it without anyone parsing the value. - **`v` is a required version stamp.** A reader that sees an unrecognised version falls back to its first-run path instead of mis-parsing. +- **Jobs that can overlap should pass `--expected-version`.** `show --json` returns the record's `version` (`.remote.record.version`). Write back with `--expected-version `, or `0` when `show` found nothing, and a concurrent run's write is refused with exit 1 (`version_conflict`) instead of overwriting the other run's result. Re-read, merge, retry. This `version` is LoreKit's count of content changes to the record, separate from the `v` schema stamp inside the value. - **Never on the critical path.** A LoreKit outage falls back to the first-run path and logs it; it never turns a green build red. Never build the payload from an environment dump or a raw error body — a CI environment is full of tokens. Assemble it from an explicit list of fields. The value cap is 64 KiB, so reports, logs, and traces belong in artifacts, not here. diff --git a/supabase/byod/bootstrap.sql b/supabase/byod/bootstrap.sql index fe76d3476..480db4238 100644 --- a/supabase/byod/bootstrap.sql +++ b/supabase/byod/bootstrap.sql @@ -2,7 +2,7 @@ -- -- ⚠ GENERATED FILE — DO NOT EDIT BY HAND. -- Generated by `node supabase/byod/generate.mjs` from the --- 114 migrations in supabase/migrations/ (newest: 00119_memory_record_extensions.sql). +-- 116 migrations in supabase/migrations/ (newest: 00121_validate_memory_version_check.sql). -- CI fails when this file and the migrations disagree; edit a migration (or add -- one) and regenerate instead. -- @@ -25557,6 +25557,866 @@ insert into supabase_migrations.schema_migrations (version, name) values ('00119', 'memory_record_extensions') on conflict (version) do nothing; +-- ───────────────────────────────────────────────────────────────────────────── +-- 00120_memory_version.sql +-- ───────────────────────────────────────────────────────────────────────────── + +-- 00120 — a version number on every memory, and optimistic concurrency on write. +-- +-- THE GAP. A memory gets corrected (a January runbook fixed in March) and a +-- reader cannot tell: a write to the same scope+key overwrites in place and only +-- `updated_at` moves — and `updated_at` also moves for edits that change nothing +-- a reader would read (a TTL, a re-sighting). Two agents correcting the same +-- lesson also race: the second write silently replaces the first. +-- +-- version integer, starts at 1, +1 on every CONTENT change. +-- Content is what a reader reads: `value`, `content_type`, +-- `attributes`, `tags`, and a `supersedes` link being set +-- or changed. A TTL change, a read/open/cite counter, an +-- embedding, archive/restore, an owner move, a re-write of +-- identical content, and a link CLEARED because its target +-- was hard-deleted (00119) are not edits and do not bump it. +-- p_expected_version on memory_write: the version the caller last read. A +-- mismatch is refused with LK003 `version_conflict` and +-- nothing is written; 0 means "the key must not exist". +-- +-- WHY A TRIGGER. Every write path obeys the rule — memory_write's upsert, a +-- REST PATCH (a plain column update), a direct PostgREST update, a groom — so it +-- lives on the table, not in one RPC. The same trigger is why a caller can never +-- set `version` itself: an INSERT always starts at 1 and an UPDATE always gets +-- the derived value, whatever the statement said. +-- +-- TRIGGER ORDER. Postgres fires same-timing row triggers in NAME order, so +-- `memories_bump_version` runs before `memories_updated_at` (00103). That order +-- is what lets the updated_at trigger compare whole rows unchanged: by the time +-- it runs, `version` differs from the old row only when content did. +-- +-- `memory_write` changes signature (a new parameter and a new returned column), +-- so 00119's overload is DROPPED and recreated — the body is 00119's verbatim +-- except the lines marked 00120. `lorekit_memory_list` (00115) returns `version` +-- too, which changes its return type, so it is likewise dropped and recreated +-- verbatim except the marked lines; its arguments, grants and comment are 00115's. +-- +-- The column is a constant default on a NOT NULL column: since Postgres 11 that +-- is a catalog-only change, so existing rows read back as 1 without a rewrite +-- or a backfill UPDATE (REVIEW.md invariants 1 and 2). The brief ACCESS +-- EXCLUSIVE lock it still takes runs under a lock_timeout. + +-- 1. Column ─────────────────────────────────────────────────────────────────── + +set local lock_timeout = '5s'; + +alter table memories + add column if not exists version integer not null default 1; + +-- The CHECK is added NOT VALID — a catalog-only change, no scan of existing +-- rows under the ACCESS EXCLUSIVE lock. New and updated rows are checked from +-- the moment it is added. It is VALIDATED in 00121, not here: a migration file +-- runs in ONE transaction, so a `validate constraint` in this file would scan +-- while this transaction still holds the ACCESS EXCLUSIVE lock `add column` +-- took (a lock is held to the end of its transaction; validate's own weaker +-- SHARE UPDATE EXCLUSIVE does not release it). Validation cannot fail: every +-- existing row reads `version` = 1 from the default. +alter table memories drop constraint if exists memories_version_positive; +alter table memories add constraint memories_version_positive + check (version >= 1) not valid; + +reset lock_timeout; + +comment on column memories.version is + 'Starts at 1 and increases by 1 on every content change (value, content_type, attributes, tags, or a supersedes link set or changed). Maintained by the memories_bump_version trigger; a caller cannot set it.'; + +-- 2. The bump trigger ───────────────────────────────────────────────────────── + +create or replace function lorekit_memories_bump_version() +returns trigger +language plpgsql +set search_path = public +as $$ +begin + if tg_op = 'INSERT' then + new.version := 1; + return new; + end if; + + if new.value is distinct from old.value + or new.content_type is distinct from old.content_type + or new.attributes is distinct from old.attributes + or new.tags is distinct from old.tags + -- Setting or changing a link is an edit; clearing one is not (00119: a + -- link is cleared by `on delete set null` when its target is hard-deleted). + or (new.supersedes is not null and new.supersedes is distinct from old.supersedes) + then + new.version := old.version + 1; + else + new.version := old.version; + end if; + return new; +end; +$$; + +comment on function lorekit_memories_bump_version() is + 'BEFORE INSERT OR UPDATE on memories: an insert starts at version 1; an update gets old.version + 1 when value, content_type, attributes, tags or a non-null supersedes changed, and old.version otherwise. Overrides any version the statement set.'; + +revoke execute on function lorekit_memories_bump_version() from public, anon; + +drop trigger if exists memories_bump_version on memories; +create trigger memories_bump_version + before insert or update on memories + for each row execute function lorekit_memories_bump_version(); + +-- 3. memory_write ───────────────────────────────────────────────────────────── + +drop function if exists memory_write( + uuid, text, text, text, text[], text, text, timestamptz, text, + integer, boolean, text, text, text, integer, text, text, + jsonb, text, uuid, text[], text, uuid[] +); + +create or replace function memory_write( + p_user_id uuid, + p_scope text, + p_key text, + p_value text, + p_tags text[] default '{}', + p_source_agent text default null, + p_trigger text default null, + p_created_at timestamptz default null, + p_org_slug text default null, + p_ttl_seconds integer default null, + p_clear_ttl boolean default false, + p_origin_repo text default null, + p_origin_branch text default null, + p_origin_commit text default null, + p_origin_pr integer default null, + p_kind text default null, + p_host text default null, + p_attributes jsonb default null, + p_content_type text default null, + p_supersedes uuid default null, + p_key_scopes text[] default '{}', + p_key_org_access text default 'all', + p_key_org_ids uuid[] default '{}', + p_expected_version integer default null -- 00120 +) +returns table ( + id uuid, + created_at timestamptz, + inserted boolean, + org_routed boolean, + binding_org_slug text, + expires_at timestamptz, + scope text, + key text, + value text, + tags text[], + source_agent text, + trigger text, + updated_at timestamptz, + archived_at timestamptz, + origin_repo text, + origin_branch text, + origin_commit text, + origin_pr integer, + kind text, + host text, + seen_count integer, + attributes jsonb, + content_type text, + supersedes uuid, + version integer -- 00120 +) +language plpgsql +security definer +set search_path = public +as $$ +-- See 00075's comment for why this directive is required (the OUT columns +-- `scope`/`key` collide with the bare column names in the `on conflict` +-- arbiter lists below). +#variable_conflict use_column +declare + v_org_id uuid; + v_binding_org uuid; + v_binding_slug text; + v_expires_at timestamptz; + v_ttl_action text := 'keep'; + v_current_version integer; -- 00120 + v_was_expired boolean; -- 00120 +begin + -- The actor is the verified caller, never a request field (the rule 00046 + -- applied to archive/restore/purge). This function is SECURITY DEFINER and + -- granted to anon/authenticated, so without this a PostgREST caller names + -- any p_user_id. A caller-supplied id is honoured only on a verified + -- service-role connection (the edge/MCP api_key path resolves the token + -- owner and passes it) or a direct connection with no JWT role at all; an + -- anon or authenticated caller acts as `auth.uid()`, and no identity is refused. + if auth.role() is distinct from 'service_role' and auth.role() is not null then + if auth.uid() is null then + raise exception using errcode = 'LK002', message = 'not_authenticated'; + end if; + p_user_id := auth.uid(); + end if; + + if not lorekit_api_token_scope_allowed(p_key_scopes, p_scope) then + raise exception using errcode = 'LK002', + message = format('key_scope_denied: scope=%s', p_scope); + end if; + + if p_clear_ttl then + v_ttl_action := 'clear'; + elsif p_ttl_seconds is not null then + if p_ttl_seconds < 1 or p_ttl_seconds > 31536000 then + raise exception using errcode = 'P0001', + message = format('ttl_seconds must be between 1 and 31536000, got %s', p_ttl_seconds); + end if; + v_expires_at := now() + (p_ttl_seconds * interval '1 second'); + v_ttl_action := 'set'; + end if; + + if p_org_slug is not null then + select o.id into v_org_id from orgs o where o.slug = p_org_slug and o.deleted_at is null; + if v_org_id is null then + raise exception using errcode = 'P0001', message = format('unknown_org: %s', p_org_slug); + end if; + if not lorekit_org_can(p_user_id, v_org_id, 'write') then + raise exception using errcode = 'LK002', message = format('org_permission_denied: org=%s', p_org_slug); + end if; + if not lorekit_api_token_org_allowed(p_key_org_access, p_key_org_ids, v_org_id) then + raise exception using errcode = 'LK002', + message = format('key_org_denied: org=%s', p_org_slug); + end if; + else + -- Wildcard-aware, most-specific-wins resolution. A binding matches + -- p_scope either by exact string equality, or — when it ends in '*' — as + -- an owner-wildcard prefix (the same LIKE-escaping `lorekit_api_token_ + -- scope_allowed` (00068) uses, since `%` and `\` cannot occur in a + -- pattern that already passed the grammar gate above). Among matches: + -- exact (is-wildcard = false) always sorts before any wildcard; among + -- wildcards, the LONGER literal prefix (length desc) sorts first as the + -- more specific match; the trailing `scope asc` is a determinism + -- safeguard for a tie that provably cannot occur (see the header + -- comment), not a reachable precedence branch. + select b.org_id, o.slug into v_binding_org, v_binding_slug + from org_scope_bindings b + join orgs o on o.id = b.org_id + where o.deleted_at is null + and ( + b.scope = p_scope + or ( + right(b.scope, 1) = '*' + and p_scope like replace(left(b.scope, -1), '_', '\_') || '%' + ) + ) + order by (right(b.scope, 1) = '*') asc, length(b.scope) desc, b.scope asc + limit 1; + + if v_binding_org is not null + and p_user_id is not null + and lorekit_org_can(p_user_id, v_binding_org, 'write') + and lorekit_api_token_org_allowed(p_key_org_access, p_key_org_ids, v_binding_org) then + v_org_id := v_binding_org; + end if; + end if; + + -- 00116: a PERSONAL write is gated by the key's tenancy too. Every branch + -- above either picked an org (and checked it) or left v_org_id NULL, which + -- means "write a personal row" — the one destination no tenancy used to gate, + -- because every tenancy could reach personal rows. `orgs_only` cannot, so + -- refuse instead of silently falling back to personal: a token that was + -- scoped to publish org lore must never grow its owner a personal row. + if v_org_id is null + and not lorekit_api_token_org_allowed(p_key_org_access, p_key_org_ids, null) then + raise exception using errcode = 'LK002', + message = format('key_personal_denied: this token may not write personal memories (scope=%s)', p_scope); + end if; + + -- 00119: a `supersedes` link may only name a memory with the SAME OWNER + -- as the row being written — the caller's own personal memory for a personal + -- write, a memory of the same org for an org write — inside this key's scope + -- allowlist. Same-owner rather than "any memory the caller can read" so an + -- org row never exposes the id of its writer's personal memory to every + -- other member. An unreachable id fails EXACTLY like a nonexistent one, so + -- the link cannot be used to probe for other tenants' memory ids. The + -- service-role tier (p_user_id null) may link to any existing memory. + if p_supersedes is not null and not exists ( + select 1 + from memories m + where m.id = p_supersedes + and ( + p_user_id is null + or ( + lorekit_api_token_scope_allowed(p_key_scopes, m.scope) + and case + when v_org_id is not null then m.org_id = v_org_id + else m.org_id is null and m.user_id = p_user_id + end + ) + ) + ) then + raise exception using errcode = 'P0001', + message = format('supersedes_not_found: %s', p_supersedes); + end if; + + -- 00120: optimistic concurrency. `p_expected_version` is the version the + -- caller last read; 0 means "this key must not exist yet". The live row this + -- write would upsert (the same owner + scope + key the conflict target below + -- names) is locked FOR UPDATE, so nothing can move its version between this + -- check and the upsert. A key with no live row is version 0. The `where` on + -- every `do update` below re-states the check, which is what closes the one + -- race the lock cannot: two writers that both expected 0 and both found no + -- row. The second one's insert lands on the first one's row, the `where` is + -- false, nothing is written, and it gets the same conflict. + -- + -- EXPIRED counts as absent. Every read hides a row whose `expires_at` has + -- passed, so a caller that read the key saw nothing: its version is 0 here + -- and in the `where` guards, the same liveness the reads use. The expired + -- row is still the conflict target (the unique indexes are partial on + -- `archived_at` only), so the write UPDATES it in place, and its version + -- CONTINUES counting from the expired row's (+1 on a content change) rather + -- than restarting at 1: the row keeps its id, created_at and seen_count, so + -- it is the same record coming back, and restarting would need the bump + -- trigger to second-guess liveness on every update. A caller therefore + -- expects 0 and may read back a version above 1. + -- + -- A write that names no TTL keeps the row's `expires_at`, which here is in + -- the PAST: the revived record would stay hidden and the nightly purge would + -- delete it. So when the gate passed on an expired row and the write names no + -- TTL, the TTL is cleared — what a brand-new insert without a TTL gets (null). + -- Scoped to writes that pass p_expected_version; a plain write to an expired + -- key keeps its past `expires_at` as before. + if p_expected_version is not null then + if p_expected_version < 0 then + raise exception using errcode = '22023', + message = format('expected_version must be >= 0, got %s', p_expected_version); + end if; + select case when m.expires_at is not null and m.expires_at <= now() then 0 + else m.version end, + m.expires_at is not null and m.expires_at <= now() + into v_current_version, v_was_expired + from memories m + where m.scope = p_scope + and m.key = p_key + and m.archived_at is null + and case + when v_org_id is not null then m.org_id = v_org_id + when p_user_id is null then m.org_id is null and m.user_id is null + else m.org_id is null and m.user_id = p_user_id + end + for update; + v_current_version := coalesce(v_current_version, 0); + if v_current_version <> p_expected_version then + raise exception using errcode = 'LK003', + message = format('version_conflict: expected_version=%s current_version=%s', + p_expected_version, v_current_version); + end if; + if coalesce(v_was_expired, false) and v_ttl_action = 'keep' then + v_ttl_action := 'clear'; + end if; + end if; + + if v_org_id is not null then + return query + insert into memories ( + user_id, org_id, scope, key, value, tags, source_agent, trigger, + created_at, updated_at, created_by, updated_by, expires_at, + origin_repo, origin_branch, origin_commit, origin_pr, kind, host, seen_count, + attributes, content_type, supersedes + ) + values ( + null, v_org_id, p_scope, p_key, p_value, p_tags, p_source_agent, p_trigger, + coalesce(p_created_at, now()), coalesce(p_created_at, now()), + p_user_id, p_user_id, v_expires_at, + p_origin_repo, p_origin_branch, p_origin_commit, p_origin_pr, p_kind, p_host, 1, + jsonb_strip_nulls(coalesce(p_attributes, '{}'::jsonb)), + coalesce(p_content_type, 'text/markdown'), p_supersedes + ) + on conflict (org_id, scope, key) where org_id is not null and archived_at is null + do update set + value = excluded.value, + tags = excluded.tags, + source_agent = excluded.source_agent, + trigger = excluded.trigger, + updated_at = now(), + updated_by = p_user_id, + seen_count = memories.seen_count + 1, + origin_repo = coalesce(excluded.origin_repo, memories.origin_repo), + origin_branch = coalesce(excluded.origin_branch, memories.origin_branch), + origin_commit = coalesce(excluded.origin_commit, memories.origin_commit), + origin_pr = coalesce(excluded.origin_pr, memories.origin_pr), + kind = coalesce(excluded.kind, memories.kind), + host = coalesce(excluded.host, memories.host), + attributes = jsonb_strip_nulls(memories.attributes || coalesce(p_attributes, '{}'::jsonb)), + content_type = coalesce(p_content_type, memories.content_type), + supersedes = coalesce(p_supersedes, memories.supersedes), + expires_at = case v_ttl_action + when 'clear' then null + when 'set' then v_expires_at + else memories.expires_at + end + where p_expected_version is null -- 00120: the same expired-is-0 rule as the gate + or p_expected_version = case when memories.expires_at is not null and memories.expires_at <= now() + then 0 else memories.version end + returning + memories.id, memories.created_at, (xmax::text = '0') as inserted, + true as org_routed, v_binding_slug as binding_org_slug, memories.expires_at, + memories.scope, memories.key, memories.value, memories.tags, + memories.source_agent, memories.trigger, memories.updated_at, memories.archived_at, + memories.origin_repo, memories.origin_branch, memories.origin_commit, memories.origin_pr, + memories.kind, memories.host, memories.seen_count, + memories.attributes, memories.content_type, memories.supersedes, + memories.version; -- 00120 + + elsif p_user_id is null then + return query + insert into memories ( + user_id, scope, key, value, tags, source_agent, trigger, + created_at, updated_at, created_by, updated_by, expires_at, + origin_repo, origin_branch, origin_commit, origin_pr, kind, host, seen_count, + attributes, content_type, supersedes + ) + values ( + null, p_scope, p_key, p_value, p_tags, p_source_agent, p_trigger, + coalesce(p_created_at, now()), coalesce(p_created_at, now()), + null, null, v_expires_at, + p_origin_repo, p_origin_branch, p_origin_commit, p_origin_pr, p_kind, p_host, 1, + jsonb_strip_nulls(coalesce(p_attributes, '{}'::jsonb)), + coalesce(p_content_type, 'text/markdown'), p_supersedes + ) + on conflict (scope, key) where org_id is null and user_id is null and archived_at is null + do update set + value = excluded.value, + tags = excluded.tags, + source_agent = excluded.source_agent, + trigger = excluded.trigger, + updated_at = now(), + seen_count = memories.seen_count + 1, + origin_repo = coalesce(excluded.origin_repo, memories.origin_repo), + origin_branch = coalesce(excluded.origin_branch, memories.origin_branch), + origin_commit = coalesce(excluded.origin_commit, memories.origin_commit), + origin_pr = coalesce(excluded.origin_pr, memories.origin_pr), + kind = coalesce(excluded.kind, memories.kind), + host = coalesce(excluded.host, memories.host), + attributes = jsonb_strip_nulls(memories.attributes || coalesce(p_attributes, '{}'::jsonb)), + content_type = coalesce(p_content_type, memories.content_type), + supersedes = coalesce(p_supersedes, memories.supersedes), + expires_at = case v_ttl_action + when 'clear' then null + when 'set' then v_expires_at + else memories.expires_at + end + where p_expected_version is null -- 00120: the same expired-is-0 rule as the gate + or p_expected_version = case when memories.expires_at is not null and memories.expires_at <= now() + then 0 else memories.version end + returning + memories.id, memories.created_at, (xmax::text = '0') as inserted, + false as org_routed, v_binding_slug as binding_org_slug, memories.expires_at, + memories.scope, memories.key, memories.value, memories.tags, + memories.source_agent, memories.trigger, memories.updated_at, memories.archived_at, + memories.origin_repo, memories.origin_branch, memories.origin_commit, memories.origin_pr, + memories.kind, memories.host, memories.seen_count, + memories.attributes, memories.content_type, memories.supersedes, + memories.version; -- 00120 + + else + return query + insert into memories ( + user_id, scope, key, value, tags, source_agent, trigger, + created_at, updated_at, created_by, updated_by, expires_at, + origin_repo, origin_branch, origin_commit, origin_pr, kind, host, seen_count, + attributes, content_type, supersedes + ) + values ( + p_user_id, p_scope, p_key, p_value, p_tags, p_source_agent, p_trigger, + coalesce(p_created_at, now()), coalesce(p_created_at, now()), + p_user_id, p_user_id, v_expires_at, + p_origin_repo, p_origin_branch, p_origin_commit, p_origin_pr, p_kind, p_host, 1, + jsonb_strip_nulls(coalesce(p_attributes, '{}'::jsonb)), + coalesce(p_content_type, 'text/markdown'), p_supersedes + ) + on conflict (user_id, scope, key) where org_id is null and user_id is not null and archived_at is null + do update set + value = excluded.value, + tags = excluded.tags, + source_agent = excluded.source_agent, + trigger = excluded.trigger, + updated_at = now(), + updated_by = p_user_id, + seen_count = memories.seen_count + 1, + origin_repo = coalesce(excluded.origin_repo, memories.origin_repo), + origin_branch = coalesce(excluded.origin_branch, memories.origin_branch), + origin_commit = coalesce(excluded.origin_commit, memories.origin_commit), + origin_pr = coalesce(excluded.origin_pr, memories.origin_pr), + kind = coalesce(excluded.kind, memories.kind), + host = coalesce(excluded.host, memories.host), + attributes = jsonb_strip_nulls(memories.attributes || coalesce(p_attributes, '{}'::jsonb)), + content_type = coalesce(p_content_type, memories.content_type), + supersedes = coalesce(p_supersedes, memories.supersedes), + expires_at = case v_ttl_action + when 'clear' then null + when 'set' then v_expires_at + else memories.expires_at + end + where p_expected_version is null -- 00120: the same expired-is-0 rule as the gate + or p_expected_version = case when memories.expires_at is not null and memories.expires_at <= now() + then 0 else memories.version end + returning + memories.id, memories.created_at, (xmax::text = '0') as inserted, + false as org_routed, v_binding_slug as binding_org_slug, memories.expires_at, + memories.scope, memories.key, memories.value, memories.tags, + memories.source_agent, memories.trigger, memories.updated_at, memories.archived_at, + memories.origin_repo, memories.origin_branch, memories.origin_commit, memories.origin_pr, + memories.kind, memories.host, memories.seen_count, + memories.attributes, memories.content_type, memories.supersedes, + memories.version; -- 00120 + end if; + + -- 00120: the `do update ... where` above declined the write (a concurrent + -- writer moved the version after the gate, or created the row a 0 expected + -- to be absent). Nothing was written; report the version that won. + if p_expected_version is not null and not found then + select case when m.expires_at is not null and m.expires_at <= now() then 0 + else m.version end + into v_current_version + from memories m + where m.scope = p_scope + and m.key = p_key + and m.archived_at is null + and case + when v_org_id is not null then m.org_id = v_org_id + when p_user_id is null then m.org_id is null and m.user_id is null + else m.org_id is null and m.user_id = p_user_id + end; + raise exception using errcode = 'LK003', + message = format('version_conflict: expected_version=%s current_version=%s', + p_expected_version, coalesce(v_current_version, 0)); + end if; +end; +$$; + + +revoke execute on function memory_write( + uuid, text, text, text, text[], text, text, timestamptz, text, + integer, boolean, text, text, text, integer, text, text, + jsonb, text, uuid, text[], text, uuid[], integer +) from public; +-- anon keeps 00119's grant: the body refuses it with LK002 not_authenticated. +grant execute on function memory_write( + uuid, text, text, text, text[], text, text, timestamptz, text, + integer, boolean, text, text, text, integer, text, text, + jsonb, text, uuid, text[], text, uuid[], integer +) to anon, authenticated, service_role; + +-- 4. lorekit_memory_list ───────────────────────────────────────────────────── + +drop function if exists lorekit_memory_list( + uuid, boolean, text, text, text, text, timestamptz, timestamptz, timestamptz, + timestamptz, text[], text, text[], text, text[], text, text[], text, text[], + text, text[], text, text[], text, text[], text, text[], text, text, + timestamptz, uuid, integer, text[], text, uuid[], integer, integer, integer, + integer, integer, + integer, integer, integer, integer, integer, + text[]); + +create or replace function lorekit_memory_list( + p_user_id uuid, + p_archived boolean default false, + p_scope text default null, + p_key text default null, + p_key_prefix text default null, + p_q text default null, + p_created_since timestamptz default null, + p_created_until timestamptz default null, + p_expires_after timestamptz default null, + p_expires_on_or_before timestamptz default null, + p_tags text[] default null, + p_tags_mode text default 'any', + p_source_agent text[] default null, + p_source_agent_mode text default 'in', + p_trigger text[] default null, + p_trigger_mode text default 'in', + p_kind text[] default null, + p_kind_mode text default 'in', + p_host text[] default null, + p_host_mode text default 'in', + p_origin_repo text[] default null, + p_origin_repo_mode text default 'in', + p_origin_branch text[] default null, + p_origin_branch_mode text default 'in', + p_origin_pr text[] default null, + p_origin_pr_mode text default 'in', + p_owner text[] default null, + p_owner_mode text default 'in', + p_sort text default 'updated_at', + p_cursor_ts timestamptz default null, + p_cursor_id uuid default null, + p_limit integer default 51, + p_key_scopes text[] default '{}', + p_key_org_access text default 'all', + p_key_org_ids uuid[] default '{}', + p_min_age_days integer default null, + p_unseen_days integer default null, + p_max_seen_count integer default null, + p_max_read_count integer default null, + p_max_opened_count integer default null, + -- 00113: the Explorer's reverse directions, appended and null-defaulted. + p_opened_within_days integer default null, + p_min_seen_count integer default null, + p_min_read_count integer default null, + p_min_opened_count integer default null, + -- 00113: the keyset cursor for `p_sort => 'opened_count'`. An integer sort + -- needs an integer cursor; `p_cursor_ts` keeps serving the timestamp sorts — + -- since 00115 including the nullable `last_opened_at`. + p_cursor_count integer default null, + -- 00114: labels the row must carry NONE of — an always-applied conjunct, + -- independent of p_tags/p_tags_mode, so "tagged a AND NOT tagged b" is + -- expressible. Appended, null-defaulted: null = not filtered. + p_exclude_tags text[] default null +) +returns table ( + id uuid, + scope text, + key text, + value text, + tags text[], + source_agent text, + trigger text, + created_at timestamptz, + updated_at timestamptz, + expires_at timestamptz, + archived_at timestamptz, + origin_repo text, + origin_branch text, + origin_commit text, + origin_pr integer, + kind text, + host text, + seen_count integer, + read_count integer, + opened_count integer, + last_read_at timestamptz, + last_opened_at timestamptz, + org_id uuid, + created_by uuid, + updated_by uuid, + org_name text, + org_slug text, + total_count integer, + version integer -- 00120 +) +language plpgsql +stable +security definer +set search_path = public +as $$ +#variable_conflict use_column +declare + v_actor uuid := case + when auth.role() = 'service_role' then coalesce(p_user_id, auth.uid()) + else auth.uid() + end; + v_origin_pr integer[] := ( + select array_agg(x::integer) + from unnest(coalesce(p_origin_pr, '{}'::text[])) as x + where x ~ '^0*[0-9]{1,9}$' + ); + -- 00113: `opened_count` joins the two date sorts; 00115: `last_opened_at`. + -- Anything else still falls back to `updated_at`, so an unknown value cannot + -- reach the format() below. + v_sort text := case + when p_sort in ('created_at', 'opened_count', 'last_opened_at') then p_sort else 'updated_at' + end; + -- Which bound parameter carries the keyset value: the integer cursor ($44) + -- for the count sort, the timestamp cursor ($29) for the others. A fixed + -- placeholder chosen here, never caller text, so splicing it is safe. + v_cursor text := case when v_sort = 'opened_count' then '$44' else '$29' end; + -- The keyset predicate and the ORDER BY, per sort. Built from fixed text and + -- `v_sort` (whitelisted above, quoted with %I) only. + -- + -- 00115: `last_opened_at` is NULLABLE, so it cannot use the shared shape — + -- NULLs would sort first under `desc`, and `col < cursor` is never true for + -- a NULL row, so the never-opened tail would be unreachable past page one. + -- See the header for the three cursor states. + v_keyset text := case + when v_sort = 'last_opened_at' then + $k$( + $30 is null + or case + when $29 is null then m.last_opened_at is null and m.id < $30 + else m.last_opened_at < $29 + or (m.last_opened_at = $29 and m.id < $30) + or m.last_opened_at is null + end + )$k$ + else format( + '(%2$s is null or m.%1$I < %2$s or (m.%1$I = %2$s and m.id < $30))', + v_sort, v_cursor) + end; + v_order text := case + when v_sort = 'last_opened_at' then 'm.last_opened_at desc nulls last, m.id desc' + else format('m.%I desc, m.id desc', v_sort) + end; + -- Resolved once per call so lorekit_match_retention stays immutable (00108). + v_created_cutoff timestamptz := case + when p_min_age_days is null then null else now() - (p_min_age_days * interval '1 day') + end; + v_unseen_cutoff timestamptz := case + when p_unseen_days is null then null else now() - (p_unseen_days * interval '1 day') + end; + -- 00113: the "opened within N days" lower bound, resolved once per call for + -- the same reason as the two cutoffs above. + v_opened_since timestamptz := case + when p_opened_within_days is null then null else now() - (p_opened_within_days * interval '1 day') + end; +begin + return query execute format($q$ + select + m.id, m.scope, m.key, m.value, m.tags, m.source_agent, m.trigger, + m.created_at, m.updated_at, m.expires_at, m.archived_at, + m.origin_repo, m.origin_branch, m.origin_commit, m.origin_pr, + m.kind, m.host, m.seen_count, + m.read_count, m.opened_count, m.last_read_at, m.last_opened_at, + m.org_id, m.created_by, m.updated_by, + o.name as org_name, o.slug as org_slug, + (count(*) over ())::integer as total_count, + m.version -- 00120 + from memories m + left join orgs o on o.id = m.org_id + where ( + ($1 is null and auth.role() = 'service_role') + or m.user_id = $1 + or m.org_id in (select lorekit_member_org_ids($1)) + ) + and lorekit_api_token_scope_allowed($32, m.scope) + and lorekit_api_token_org_allowed($33, $34, m.org_id) + and ( + case + when $2 then m.archived_at is not null + else m.archived_at is null + and (m.expires_at is null or m.expires_at > now()) + end + ) + and ($3 is null or m.scope = $3) + and ($4 is null or m.key = $4) + and ($5 is null or m.key ilike $5 || '%%') + and ($6 is null or m.key ilike '%%' || $6 || '%%' or m.value ilike '%%' || $6 || '%%') + and ($7 is null or m.created_at >= $7) + and ($8 is null or m.created_at < $8) + and ($9 is null or m.expires_at > $9) + and ($10 is null or m.expires_at <= $10) + and lorekit_match_tags(m.tags, $11, $12) + and lorekit_match_tags(m.tags, $45, 'none') + and lorekit_match_text(m.source_agent, $13, $14) + and lorekit_match_text(m.trigger, $15, $16) + and lorekit_match_text(m.kind, $17, $18) + and lorekit_match_text(m.host, $19, $20) + and lorekit_match_text(m.origin_repo, $21, $22) + and lorekit_match_text(m.origin_branch, $23, $24) + and lorekit_match_int (m.origin_pr, $25, $26) + and ($27 is null or case coalesce($28, 'in') + when 'nin' then ( + (case when m.org_id is null then 'personal' else o.slug end) is not null + and (case when m.org_id is null then 'personal' else o.slug end) <> all($27) + ) + else ( + ('personal' = any($27) and m.org_id is null) + or (m.org_id is not null and o.slug = any($27)) + ) + end) + and lorekit_match_retention( + m.created_at, m.last_opened_at, m.seen_count, m.read_count, m.opened_count, + $35, $36, $37, $38, $39, + $40, $41, $42, $43 + ) + and %1$s + order by %2$s + limit $31 + $q$, v_keyset, v_order) + using + v_actor, coalesce(p_archived, false), p_scope, p_key, p_key_prefix, p_q, + p_created_since, p_created_until, p_expires_after, p_expires_on_or_before, + p_tags, p_tags_mode, + p_source_agent, p_source_agent_mode, + p_trigger, p_trigger_mode, + p_kind, p_kind_mode, + p_host, p_host_mode, + p_origin_repo, p_origin_repo_mode, + p_origin_branch, p_origin_branch_mode, + v_origin_pr, p_origin_pr_mode, + p_owner, p_owner_mode, + p_cursor_ts, p_cursor_id, + greatest(coalesce(p_limit, 51), 1), + coalesce(p_key_scopes, '{}'::text[]), + coalesce(p_key_org_access, 'all'), + coalesce(p_key_org_ids, '{}'::uuid[]), + v_created_cutoff, v_unseen_cutoff, p_max_seen_count, p_max_read_count, + p_max_opened_count, + v_opened_since, p_min_seen_count, p_min_read_count, p_min_opened_count, + p_cursor_count, + p_exclude_tags; +end; +$$; + +revoke execute on function lorekit_memory_list( + uuid, boolean, text, text, text, text, timestamptz, timestamptz, timestamptz, + timestamptz, text[], text, text[], text, text[], text, text[], text, text[], + text, text[], text, text[], text, text[], text, text[], text, text, + timestamptz, uuid, integer, text[], text, uuid[], integer, integer, integer, + integer, integer, + integer, integer, integer, integer, integer, + text[]) from public, anon; +grant execute on function lorekit_memory_list( + uuid, boolean, text, text, text, text, timestamptz, timestamptz, timestamptz, + timestamptz, text[], text, text[], text, text[], text, text[], text, text[], + text, text[], text, text[], text, text[], text, text[], text, text, + timestamptz, uuid, integer, text[], text, uuid[], integer, integer, integer, + integer, integer, + integer, integer, integer, integer, integer, + text[]) to authenticated, service_role; + +comment on function lorekit_memory_list( + uuid, boolean, text, text, text, text, timestamptz, timestamptz, timestamptz, + timestamptz, text[], text, text[], text, text[], text, text[], text, text[], + text, text[], text, text[], text, text[], text, text[], text, text, + timestamptz, uuid, integer, text[], text, uuid[], integer, integer, integer, + integer, integer, + integer, integer, integer, integer, integer, + text[]) is + 'The keyset-paginated memory list behind GET /memories and POST /memories/list. + Sorts by updated_at (default), created_at, opened_count (00113) or + last_opened_at (00115) — each descending with id as the tie-break; + last_opened_at is NULLS LAST, so never-opened lessons trail. p_cursor_ts + carries the timestamp sorts'' keyset value (NULL under last_opened_at with a + set p_cursor_id = inside the never-opened tail) and p_cursor_count the count + sort''s. Applies the five retention thresholds and the Explorer''s reverse + directions through the shared lorekit_match_retention, so its total_count + agrees with _facets, _activity and _pivot under the same parameters. + p_exclude_tags (00114): the row carries none of these labels, ANDed with + whatever p_tags/p_tags_mode ask.'; +; +insert into supabase_migrations.schema_migrations (version, name) +values ('00120', 'memory_version') +on conflict (version) do nothing; + +-- ───────────────────────────────────────────────────────────────────────────── +-- 00121_validate_memory_version_check.sql +-- ───────────────────────────────────────────────────────────────────────────── + +-- 00121 — validate memories_version_positive (added NOT VALID by 00120). +-- +-- A separate file because each migration runs in ONE transaction: validating in +-- 00120 would scan every row while that transaction still held the ACCESS +-- EXCLUSIVE lock its `add column` took, blocking reads and writes for the scan. +-- In its own transaction, VALIDATE CONSTRAINT takes only SHARE UPDATE EXCLUSIVE, +-- so reads and writes proceed while it scans. It cannot fail: every pre-00120 +-- row reads `version` = 1 from the column default, and every row since has been +-- checked on write. + +alter table memories validate constraint memories_version_positive; +; +insert into supabase_migrations.schema_migrations (version, name) +values ('00121', 'validate_memory_version_check') +on conflict (version) do nothing; + -- ───────────────────────────────────────────────────────────────────────────── -- storage-overlay.sql — BYOD only: drop foreign keys into auth.users -- ───────────────────────────────────────────────────────────────────────────── diff --git a/supabase/functions/_shared/api/respond.ts b/supabase/functions/_shared/api/respond.ts index 1fa4e9c1e..ad73436dc 100644 --- a/supabase/functions/_shared/api/respond.ts +++ b/supabase/functions/_shared/api/respond.ts @@ -31,6 +31,25 @@ export function tooManyRequests(retryAfterSeconds: number, cors: Record = {}, +): Response { + return new Response( + JSON.stringify({ + error: err.message, + code: 'version_conflict', + expected_version: err.expectedVersion, + current_version: err.currentVersion, + }), + { status: 409, headers: h(cors) }, + ); +} export function methodNotAllowed(cors: Record = {}): Response { return new Response(JSON.stringify({ error: 'Method not allowed', code: 'method_not_allowed' }), { status: 405, headers: h(cors) }); } diff --git a/supabase/functions/_shared/api/tenant.ts b/supabase/functions/_shared/api/tenant.ts index 70725de62..70045bdb3 100644 --- a/supabase/functions/_shared/api/tenant.ts +++ b/supabase/functions/_shared/api/tenant.ts @@ -27,6 +27,7 @@ import { restrictsTenancy, type KeyRestriction, } from '../auth/tenant-scope.ts'; +import { fetchSupersededBy } from '../memory/superseded-by.ts'; /** * Resolve the org IDs the user is a member of via the single-source RPC. @@ -241,3 +242,25 @@ export async function hasOrgCapability( span.setAttributes({ 'lorekit.org.capability': capability, 'lorekit.org.capability_granted': granted }).end(); return granted; } + +/** + * `superseded_by` (00120) for a REST read page: one query over the page's ids, + * under the SAME visibility the read itself used — `applyRestTenantScope` for an + * api_key caller, RLS for a JWT caller (the client is already user-scoped), and + * nothing for the service tier. Pass `orgIds` when the handler already resolved + * them; otherwise they are resolved here, and only when the page has rows. + */ +export async function restSupersededBy( + db: DbClient, + auth: AuthContext, + span: Span, + ids: ReadonlyArray, + orgIds?: string[], +): Promise> { + if (!ids.some((id) => typeof id === 'string' && id.length > 0)) return new Map(); + const tracedDb = createTracedClient(db, span); + if (auth.type !== 'api_key' || !auth.userId) return fetchSupersededBy(tracedDb, ids, (q) => q); + const userId = auth.userId; + const visible = orgIds ?? await getMemberOrgIds(db, userId, span); + return fetchSupersededBy(tracedDb, ids, (q) => applyRestTenantScope(q, userId, visible, keyRestriction(auth))); +} diff --git a/supabase/functions/_shared/db/database.types.ts b/supabase/functions/_shared/db/database.types.ts index 411328dcf..2c80ca87f 100644 --- a/supabase/functions/_shared/db/database.types.ts +++ b/supabase/functions/_shared/db/database.types.ts @@ -203,6 +203,7 @@ export type Database = { updated_by: string | null user_id: string | null value: string + version: number } Insert: { archived_at?: string | null @@ -228,6 +229,7 @@ export type Database = { updated_by?: string | null user_id?: string | null value: string + version?: number } Update: { archived_at?: string | null @@ -253,6 +255,7 @@ export type Database = { updated_by?: string | null user_id?: string | null value?: string + version?: number } Relationships: [ { @@ -1303,6 +1306,7 @@ export type Database = { p_clear_ttl?: boolean p_content_type?: string p_created_at?: string + p_expected_version?: number p_key: string p_key_org_access?: string p_key_org_ids?: string[] @@ -1346,6 +1350,7 @@ export type Database = { trigger: string | null updated_at: string value: string + version: number }[] } purge_archived_memories: { diff --git a/supabase/functions/_shared/memory/live.ts b/supabase/functions/_shared/memory/live.ts new file mode 100644 index 000000000..5c85808f2 --- /dev/null +++ b/supabase/functions/_shared/memory/live.ts @@ -0,0 +1,8 @@ +/** + * The PostgREST `.or()` filter that keeps only UNEXPIRED rows: no expiry, or + * one still in the future. An expired memory is absent to every by-id route — + * `GET /:id` 404s on it, and `memory_write` (00120) counts it as version 0 — + * so `PATCH /:id` uses the same predicate, and the two cannot drift apart. + * Pair it with `.is('archived_at', null)` for "live". + */ +export const NOT_EXPIRED = 'expires_at.is.null,expires_at.gt.now()'; diff --git a/supabase/functions/_shared/memory/superseded-by.ts b/supabase/functions/_shared/memory/superseded-by.ts new file mode 100644 index 000000000..29da2d1c5 --- /dev/null +++ b/supabase/functions/_shared/memory/superseded-by.ts @@ -0,0 +1,60 @@ +// `superseded_by` for a page of read results (00120) — shared by the MCP tools +// (`mcp/tools.ts`) and the REST read handlers (`memories/handlers/*`). +// +// `superseded_by` is never stored: it is the ids of the records whose +// `supersedes` names this one. It is computed with ONE query per page — an +// `in` over the page's ids, served by `memories_supersedes_idx` (00119) — never +// one query per row. +// +// VISIBILITY. The caller passes `scope`, which applies the SAME tenant filter +// its own read applied (MCP: `applyTenantScope`; REST api_key: +// `applyRestTenantScope`; REST JWT: nothing, because RLS already does it). So a +// successor the caller cannot read is never named, and the lookup cannot be +// used to probe for ids in another tenant. Only live successors count: an +// archived or expired record is not a correction anyone can read. +// +// The grouping and the attach rule are pure and live in the mirrored +// `provenance/record-extensions.ts`; this file is only the query. + +import { groupSupersededBy, SUPERSEDED_BY_COLUMNS } from '../provenance/record-extensions.ts'; +import type { createTracedClient, TracedQuery } from '../telemetry/otel.ts'; + +/** + * A ceiling on successor rows per page, so a pathological record that hundreds + * of others claim to supersede cannot turn one read into an unbounded fetch. + * Newest successors are kept. + */ +export const SUPERSEDED_BY_MAX_ROWS = 1000; + +/** One successor row: its id and the id it supersedes. */ +export interface SuccessorRow { + id: string; + supersedes: string | null; +} + +/** + * Fetch `superseded_by` for every id on a page, as a map from a record id to + * its successors' ids (newest first). Ids with no visible successor are + * absent. Resolves to an empty map for an empty page without a query. + * + * Throws on a query error, like the read it accompanies: a page that cannot + * say whether its records were corrected must not silently claim they were not. + */ +export async function fetchSupersededBy( + db: ReturnType, + ids: ReadonlyArray, + scope: (query: TracedQuery) => TracedQuery, +): Promise> { + const unique = [...new Set(ids.filter((id): id is string => typeof id === 'string' && id.length > 0))]; + if (unique.length === 0) return new Map(); + const base = db.from<'memories', SuccessorRow>('memories').select(SUPERSEDED_BY_COLUMNS) + .in('supersedes', unique) + .is('archived_at', null) + .or('expires_at.is.null,expires_at.gt.now()'); + const { data, error } = await scope(base) + .order('updated_at', { ascending: false }) + .order('id', { ascending: false }) + .limit(SUPERSEDED_BY_MAX_ROWS); + if (error) throw new Error(error.message); + return groupSupersededBy(Array.isArray(data) ? data : []); +} diff --git a/supabase/functions/_shared/provenance/record-extensions.ts b/supabase/functions/_shared/provenance/record-extensions.ts index 9df84dd77..5241a82eb 100644 --- a/supabase/functions/_shared/provenance/record-extensions.ts +++ b/supabase/functions/_shared/provenance/record-extensions.ts @@ -17,6 +17,13 @@ // cited (ids) a citation that names a record by its stable id instead of // by its `scope::key` address, which can change // +// Two more (00120) make a correction visible and safe: +// version every record's content version, starting at 1; read-only +// expected_version on a write, the version the caller last read. A stale one +// is refused with `version_conflict`; 0 means "must not exist" +// superseded_by computed on read (never stored): the ids of the records +// whose `supersedes` names this one +// // `parseRecordFields` is the single, pure validation gate for all of them. It // is mirrored self-contained into the Deno edge tree // (supabase/functions/_shared/provenance/record-extensions.ts) because BOTH the @@ -59,6 +66,7 @@ export interface RecordFields { attributes: AttributesInput | null; contentType: ContentType | null; supersedes: string | null; + expectedVersion: number | null; } /** The raw, unvalidated write-tool parameters. */ @@ -66,6 +74,30 @@ export interface RecordFieldsInput { attributes?: unknown; content_type?: unknown; supersedes?: unknown; + expected_version?: unknown; +} + +/** + * A write refused because the record moved on since the caller read it + * (00120). A distinct class, not a {@link RecordFieldError}: the request was + * well-formed, and the fix is to re-read and retry, not to change the input. + * REST answers it with 409 and MCP with an `isError` result; both carry the + * stored version so the caller can retry without another read. + */ +export class VersionConflictError extends Error { + readonly code = 'version_conflict'; + readonly expectedVersion: number; + readonly currentVersion: number; + constructor(expectedVersion: number, currentVersion: number) { + super( + `version_conflict: expected_version ${expectedVersion} does not match the stored version ${currentVersion}` + + (currentVersion === 0 ? ' (the memory does not exist)' : '') + + '. Re-read the memory, then retry with its current version.', + ); + this.name = 'VersionConflictError'; + this.expectedVersion = expectedVersion; + this.currentVersion = currentVersion; + } } /** True when `input` is a record id (a lowercase or uppercase uuid). */ @@ -129,6 +161,18 @@ export function parseSupersedes(input: unknown): string | null { return (input as string).trim().toLowerCase(); } +/** + * Validate `expected_version`: a non-negative integer, where 0 means "the + * record must not exist yet". Omitted is `null` — no check, today's behaviour. + */ +export function parseExpectedVersion(input: unknown): number | null { + if (input === undefined || input === null) return null; + if (typeof input !== 'number' || !Number.isInteger(input) || input < 0 || input > 2147483647) { + throw new RecordFieldError(`expected_version must be a non-negative integer: ${String(input)}`); + } + return input; +} + /** * Validate and normalise the record-extension write parameters. * @@ -152,6 +196,7 @@ export function parseRecordFields(input: RecordFieldsInput, value: string): Reco attributes: parseAttributes(input.attributes), contentType, supersedes: parseSupersedes(input.supersedes), + expectedVersion: parseExpectedVersion(input.expected_version), }; } @@ -187,17 +232,30 @@ const DB_FIELD_ERRORS: ReadonlyArray = [ ['memories_attributes_max_keys', `attributes must have <= ${ATTRIBUTES_MAX_KEYS} keys`], ]; -/** Map a database error about a record field to a {@link RecordFieldError}; pass anything else through. */ +/** `memory_write`'s LK003 message (00120): `version_conflict: expected_version=N current_version=M`. */ +const VERSION_CONFLICT_RE = /version_conflict: expected_version=(\d+) current_version=(\d+)/; + +/** + * Map a database error about a record field to a {@link RecordFieldError}, and + * `memory_write`'s version refusal to a {@link VersionConflictError}; pass + * anything else through. + */ export function translateRecordFieldDbError(err: unknown): unknown { const message = (err as { message?: string } | null | undefined)?.message ?? ''; + const conflict = VERSION_CONFLICT_RE.exec(message); + if (conflict) return new VersionConflictError(Number(conflict[1]), Number(conflict[2])); for (const [marker, text] of DB_FIELD_ERRORS) { if (message.includes(marker)) return new RecordFieldError(text); } return err; } -/** The columns a read selects to report the record extensions. */ -export const RECORD_FIELD_COLUMNS = 'attributes,content_type,supersedes'; +/** + * The columns a read selects to report the record extensions. `version` rides + * along and is always returned: unlike the three extensions it has no default + * that says nothing (00120). + */ +export const RECORD_FIELD_COLUMNS = 'attributes,content_type,supersedes,version'; type WithRecordFields = { attributes?: unknown; content_type?: unknown; supersedes?: unknown }; @@ -219,3 +277,39 @@ export function compactRecordFields( ...(supersedes ? { supersedes } : {}), }; } + +/** The columns a `superseded_by` lookup selects: the successor and the id it names. */ +export const SUPERSEDED_BY_COLUMNS = 'id,supersedes'; + +/** + * Group successor rows (`{ id, supersedes }`, newest first) by the id they + * supersede: `superseded_by` for every record on a page, from ONE query over + * the page's ids. Total: a row without both ids is skipped, and a successor + * listed twice is kept once. + */ +export function groupSupersededBy( + rows: ReadonlyArray<{ id?: unknown; supersedes?: unknown }>, +): Map { + const out = new Map(); + for (const row of rows) { + if (typeof row?.id !== 'string' || typeof row.supersedes !== 'string') continue; + const list = out.get(row.supersedes) ?? []; + if (!list.includes(row.id)) list.push(row.id); + out.set(row.supersedes, list); + } + return out; +} + +/** + * Attach `superseded_by` to a read result — only when something supersedes it, + * the same "say nothing when there is nothing to say" rule + * {@link compactRecordFields} applies, so an uncorrected record reads exactly as + * before. + */ +export function withSupersededBy( + row: T, + links: ReadonlyMap, +): T & { superseded_by?: string[] } { + const ids = typeof row.id === 'string' ? links.get(row.id) : undefined; + return ids && ids.length > 0 ? { ...row, superseded_by: ids } : row; +} diff --git a/supabase/functions/_shared/schemas/memory.ts b/supabase/functions/_shared/schemas/memory.ts index 4f9180bc6..eaa601a85 100644 --- a/supabase/functions/_shared/schemas/memory.ts +++ b/supabase/functions/_shared/schemas/memory.ts @@ -82,6 +82,12 @@ export const MemoryWriteSchema = z.object({ attributes: z.record(z.union([z.string(), z.number(), z.boolean(), z.null()])).optional(), content_type: z.string().optional(), supersedes: z.string().optional(), + // Optimistic concurrency (00120): the `version` the caller last read. A write + // whose expected_version is not the stored version is refused with + // `version_conflict` and changes nothing; 0 means "the key must not exist + // yet". Omitted = unchecked, exactly as before. The shared + // `parseExpectedVersion` owns the rule; this only admits the wire type. + expected_version: z.number().optional(), }); export type MemoryWrite = z.infer; @@ -933,6 +939,10 @@ export type ReadActivityResponse = z.infer; * omitted for the origin fields' reason: each has write-path validation (a * merge, a JSON body check, a visibility check on the target) that lives in * `memory_write`, and a column patch would bypass it. Set them with a write. + * + * `expected_version` (00120) STAYS: the handler turns it into a predicate on + * the update itself (`version = n`), so the check and the write are one + * statement. It is not a column and is stripped from the patch. */ export const UpdateMemoryBodySchema = MemoryWriteSchema .omit({ @@ -982,6 +992,13 @@ export const MemoryEntrySchema = z.object({ attributes: z.record(z.union([z.string(), z.number(), z.boolean()])).optional(), content_type: z.string().optional(), supersedes: z.string().uuid().nullable().optional(), + // The record's content version (00120): 1 on create, +1 on every change to + // value, content_type, attributes, tags or supersedes. What `expected_version` + // compares against. Optional for a pre-00120 backend, like the fields above. + version: z.number().int().min(1).optional(), + // Computed on read, never stored (00120): the ids of the live records the + // caller can see whose `supersedes` names this one. Omitted when none do. + superseded_by: z.array(z.string().uuid()).optional(), // Recurrence — how many times this lesson has been written (00059). The // column is NOT NULL DEFAULT 1, so a live row always has >= 1; optional here // for the same reason kind/host are, so a client reading a response from a @@ -1051,7 +1068,7 @@ export type MemoryEntry = z.infer; export const MEMORY_SELECT = 'id,scope,key,value,tags,source_agent,trigger,created_at,updated_at,expires_at,archived_at,' + 'origin_repo,origin_branch,origin_commit,origin_pr,kind,host,seen_count,' - + 'attributes,content_type,supersedes,' + + 'attributes,content_type,supersedes,version,' + 'read_count,opened_count,last_read_at,last_opened_at,cited_count,last_cited_at,' + 'org_id,created_by,updated_by,orgs(id,name,slug)'; diff --git a/supabase/functions/_shared/schemas/openapi/spec.ts b/supabase/functions/_shared/schemas/openapi/spec.ts index 148fceaec..199fb5c39 100644 --- a/supabase/functions/_shared/schemas/openapi/spec.ts +++ b/supabase/functions/_shared/schemas/openapi/spec.ts @@ -236,7 +236,8 @@ export function generateSpec(baseUrl = 'https://pqokxlhvnosogizsjztg.supabase.co registry.registerPath({ method: 'post', path: '/memories', summary: 'Create or update a memory', tags: ['Memories'], security, request: { body: { content: { 'application/json': { schema: CreateMemoryBodySchema } } } }, - responses: { 201: memoryResponse('Created'), 400: errorResponse, 401: errorResponse, 429: errorResponse }, + description: 'With `expected_version`, the write is conditional: a `409` with `code: "version_conflict"` and the stored `current_version` means nothing was written (`0` = the memory must not exist yet).', + responses: { 201: memoryResponse('Created'), 400: errorResponse, 401: errorResponse, 409: errorResponse, 429: errorResponse }, }); registry.registerPath({ method: 'post', path: '/memories/search', summary: 'Search memories with OR+AND filtering', tags: ['Memories'], @@ -590,12 +591,13 @@ export function generateSpec(baseUrl = 'https://pqokxlhvnosogizsjztg.supabase.co }); registry.registerPath({ method: 'patch', path: '/memories/{id}', summary: 'Update a memory', tags: ['Memories'], + description: 'With `expected_version`, the update applies only while the memory is at that version; otherwise a `409` with `code: "version_conflict"` and the stored `current_version`, and nothing changes. An archived or expired memory is `404`, exactly as on `GET /memories/{id}` — and is `404`, not `409`, with an `expected_version` too.', security, request: { params: MemoryIdParamsSchema, body: { content: { 'application/json': { schema: UpdateMemoryBodySchema } } }, }, - responses: { 200: memoryResponse('Updated memory'), 400: errorResponse, 404: errorResponse, 401: errorResponse }, + responses: { 200: memoryResponse('Updated memory'), 400: errorResponse, 404: errorResponse, 409: errorResponse, 401: errorResponse }, }); registry.registerPath({ method: 'delete', path: '/memories/{id}', diff --git a/supabase/functions/_shared/schemas/tool-catalog.ts b/supabase/functions/_shared/schemas/tool-catalog.ts index 62297b2cd..f96747b5e 100644 --- a/supabase/functions/_shared/schemas/tool-catalog.ts +++ b/supabase/functions/_shared/schemas/tool-catalog.ts @@ -326,7 +326,7 @@ export const MCP_TOOLS = [ minimum: 1, maximum: 365, description: - 'Number of days until the memory auto-expires. Omit for a permanent memory. On an update, supplying ttl_days refreshes the expiry; omitting it leaves the existing expiry unchanged.', + 'Number of days until the memory auto-expires. Omit for a permanent memory. On an update, supplying ttl_days refreshes the expiry; omitting it leaves the existing expiry unchanged — except on an `expected_version` write that revives an expired memory, where omitting it (and `clear_ttl`) clears the past expiry so the memory is live again.', }, clear_ttl: { type: 'boolean', @@ -375,13 +375,20 @@ export const MCP_TOOLS = [ description: 'The id of the memory this one replaces (from `memory.read`). Must name a memory this token can read that has the same owner as the one being written (personal to personal, or the same org); anything else is rejected as not found. Kept on update unless re-supplied.', }, + expected_version: { + type: 'integer', + minimum: 0, + description: + 'Optimistic concurrency: the `version` you last read. If the stored version differs, the write is rejected with `version_conflict` (naming the current version) and nothing changes. `0` means the memory must not exist yet. An archived or expired memory counts as absent (version `0`), exactly as reads hide it; an expired one is rewritten in place, and with no `ttl_days` / `clear_ttl` on that write its past expiry is cleared. Omit for an unconditional write.', + }, }, }, returns: - '`{ "id": "", "created_at": "" }` — the `id` is stable across updates and is what `supersedes` and `cited` accept — plus optional `"expires_at"` and `"notice"` (when a write fell back to personal because the scope is bound to an org the caller cannot write to).', + '`{ "id": "", "created_at": "", "version": }` — the `id` is stable across updates and is what `supersedes` and `cited` accept; `version` is the content version after this write, the value to pass as `expected_version` next time — plus optional `"expires_at"` and `"notice"` (when a write fell back to personal because the scope is bound to an org the caller cannot write to).', notes: [ '**Provenance (`origin_*`):** `scope` says where a lesson *applies*; the four `origin_*` fields say where it was *recorded from*. Each is independently optional and the last KNOWN value wins — on an update, a field you omit keeps whatever a previous write recorded rather than being erased. A malformed value is rejected, never silently dropped.', '**Record extensions (`attributes`, `content_type`, `supersedes`):** additive, and a write that omits them behaves exactly as before. `attributes` holds metadata no column anticipates — a CI run URL, a PR number, an owning team — under namespaced keys; it is merged on update, so name only the keys you change. `content_type: "application/json"` marks a value a program should parse (a CI result, a structured finding), and the server rejects one that does not parse. `supersedes` links a corrected lesson to the one it replaces, by the old one\u2019s `id`, so the history survives a re-key; the target must have the same owner as the memory being written (personal to personal, or the same org), and any other id, including one you cannot read, is rejected exactly like one that does not exist.', + '**Versions (`version`, `expected_version`):** every memory starts at version 1 and gains 1 each time its content changes — `value`, `content_type`, `attributes`, `tags`, or `supersedes` being set. A TTL change, a re-write of identical content, archive/restore and read counters do not move it, so a reader can tell a corrected memory from a re-sighted one. Pass `expected_version` to make a write conditional: a mismatch fails with a `version_conflict` error stating the current version, and nothing is written. Re-read, reconcile, and retry with that version.', '**Citations (`cited`):** the one signal read counts cannot produce. A lesson injected at session start is already in your context and is applied without ever being fetched, so LoreKit can measure whether lore was *looked up* but not whether it *worked* — `cited` is where you say so. Each entry is a `scope::key` you were shown; they raise the cited lesson\u2019s `cited_count` and are grouped by the run\u2019s correlation id. Recorded once per run, so a retried write does not inflate anything, and every failure mode is silent: an unknown reference, a self-citation, and anything past 32 entries are dropped and the write still succeeds.', '**Scope→org binding:** An org admin can bind a scope (e.g. a repo) — or a WILDCARD prefix such as `repo::owner/*` or `branch::owner/repo::*`, matching every scope under it — to an org. A write under a bound scope with no explicit `org` auto-routes to that org for write-capable members, resolved MOST-SPECIFIC-WINS when several bindings match (an exact scope beats any wildcard; a longer wildcard prefix beats a shorter one). A non-member\u2019s write falls back to personal (never rejected) with a `notice` \u2014 except from a token limited to selected orgs (`orgs_only`), which may not write personal memories: that write is refused, so pass `org`.', ], @@ -413,7 +420,7 @@ export const MCP_TOOLS = [ refs: { type: 'array', items: { type: 'string' }, description: 'Batch mode: one or more `scope::key` references, fetched in a single call. Cannot be combined with `scope`/`key`. Each entry is parsed by the same reference grammar `memory.write`\'s `cited` field uses (`scope::key`, verbatim scope — never lowercased). Silently truncated past 32 entries.' }, }, }, - returns: '`{ "id": "", "value": "", "updated_at": "", "scope": "" }` or `null` if not found. `id` is the record\u2019s stable identity — the value `supersedes` and `cited` accept. `attributes`, `content_type` and `supersedes` appear only when they hold something other than their default (an empty map, `text/markdown`, no link). `scope` names the scope that ANSWERED — always present, so an unscoped read can tell a `global` hit from a `repo::…` one. When the key also existed in other scopes, an additional `other_scopes` array names them in precedence order; it is omitted entirely when the key was unambiguous. With `refs`, instead returns `{ "entries": [{ "id", "scope", "key", "value", "updated_at" }], "missing": ["scope::key", …] }` — `missing` names every well-formed reference within the first 32 that matched no lesson.', + returns: '`{ "id": "", "value": "", "updated_at": "", "scope": "", "version": }` or `null` if not found. `id` is the record\u2019s stable identity — the value `supersedes` and `cited` accept. `version` is its content version (see `memory.write`\u2019s `expected_version`). `attributes`, `content_type` and `supersedes` appear only when they hold something other than their default (an empty map, `text/markdown`, no link). `superseded_by` lists the ids of the live memories you can read whose `supersedes` names this one \u2014 present only when there is at least one, and a sign this memory has been corrected. `scope` names the scope that ANSWERED — always present, so an unscoped read can tell a `global` hit from a `repo::…` one. When the key also existed in other scopes, an additional `other_scopes` array names them in precedence order; it is omitted entirely when the key was unambiguous. With `refs`, instead returns `{ "entries": [{ "id", "scope", "key", "value", "updated_at", "version" }], "missing": ["scope::key", …] }` (each entry carrying `superseded_by` the same way) — `missing` names every well-formed reference within the first 32 that matched no lesson.', notes: [ '**Scope is optional.** `memory.read { key }` with no scope resolves that key across every scope you can see and returns the most specific match — `project` beats `branch` beats `repo` beats `global`, with ties broken by most-recently-updated then scope ascending. This is the shape to use when a key reached you through a session-start injection and you do not know which scope it came from. Pass `scope` when you DO know it: it is one indexed row instead of a fan-out, and it removes the ambiguity entirely. Either way the response names the scope that answered.', '**Exactly one of two argument shapes is required:** `key` (optionally with `scope`) for a single read, **or** `refs` alone for a batch read. They cannot be combined — a call carrying both is rejected, and so is one carrying neither. No argument is required on its own, which is why none is marked required above: the table cannot express "one of these two groups", so this rule is the whole of it. The schema deliberately does NOT advertise the alternatives as a top-level `oneOf` either — Amazon Bedrock rejects a tool whose `input_schema` carries one, and it fails the entire request rather than that one tool. Both shapes are validated by the server on every call regardless.', @@ -444,7 +451,7 @@ export const MCP_TOOLS = [ view: { type: 'string', enum: ['full', 'summary'], default: 'full', description: 'full (default) returns each entry\'s complete `value`. summary omits `value` and returns `value_bytes` + a 200-character `preview` instead — the cheap discovery read for deciding WHICH lessons to then fetch with `memory.read`.' }, }, }, - returns: '`{ "entries": [{ "scope", "key", "value", "tags", "updated_at" }], "hasMore": boolean, "nextCursor": string | null }` — every entry names its own `scope`, so an unscoped listing stays readable; newest-first (recency mode) or ranked by salience+recency then MMR-diversified (rank mode). Because rank mode diversifies, entries are NOT strictly score-descending — a more diverse lower-scored lesson can precede a higher-scored near-duplicate. Pass `nextCursor` back as `cursor` to paginate — recency mode only. Rank mode is a single bounded top-N page: `hasMore` is always false and `nextCursor` always null. With `view: "summary"` each entry is `{ "scope", "key", "tags", "updated_at", "value_bytes", "preview" }` — `value` is omitted entirely.', + returns: '`{ "entries": [{ "scope", "key", "value", "tags", "updated_at", "version" }], "hasMore": boolean, "nextCursor": string | null }` — plus `superseded_by` on an entry that a newer memory supersedes (superseded entries are still listed, only marked) — every entry names its own `scope`, so an unscoped listing stays readable; newest-first (recency mode) or ranked by salience+recency then MMR-diversified (rank mode). Because rank mode diversifies, entries are NOT strictly score-descending — a more diverse lower-scored lesson can precede a higher-scored near-duplicate. Pass `nextCursor` back as `cursor` to paginate — recency mode only. Rank mode is a single bounded top-N page: `hasMore` is always false and `nextCursor` always null. With `view: "summary"` each entry is `{ "scope", "key", "tags", "updated_at", "value_bytes", "preview" }` — `value` is omitted entirely.', }, { name: 'memory.delete', @@ -495,7 +502,7 @@ export const MCP_TOOLS = [ cursor: { type: 'string', description: 'Opaque cursor from a previous response\'s `nextCursor`. Omit to start from the first page.' }, }, }, - returns: '`{ "entries": [{ "key", "value", "scope", "tags", "rank" }], "hasMore": boolean, "nextCursor": string | null }`. Pass `nextCursor` back as `cursor` to retrieve the next page.', + returns: '`{ "entries": [{ "key", "value", "scope", "tags", "rank", "version" }], "hasMore": boolean, "nextCursor": string | null }` — plus `superseded_by` on an entry that a newer memory supersedes (superseded entries are still returned, only marked). Pass `nextCursor` back as `cursor` to retrieve the next page.', }, { name: 'memory.archive', diff --git a/supabase/functions/mcp/mcp-handler.ts b/supabase/functions/mcp/mcp-handler.ts index f8cbcf361..236a3d656 100644 --- a/supabase/functions/mcp/mcp-handler.ts +++ b/supabase/functions/mcp/mcp-handler.ts @@ -12,7 +12,7 @@ import { OrgPermissionError, UnknownOrgError } from './org-permissions.ts'; import { TtlError } from './ttl.ts'; import { CreatedAtError } from '../_shared/limits/created-at.ts'; import { OriginError } from '../_shared/provenance/origin.ts'; -import { RecordFieldError } from '../_shared/provenance/record-extensions.ts'; +import { RecordFieldError, VersionConflictError } from '../_shared/provenance/record-extensions.ts'; import { type Params } from './tools.ts'; // The dispatch maps are GENERATED from packages/schemas/src/shared/tool-catalog.ts — // see tool-dispatch.generated.ts. They were hand-written here, which is why a @@ -519,6 +519,9 @@ export async function handleMcp(req: Request, auth: AuthContext, span: Span, ada || err instanceof CreatedAtError || err instanceof OriginError || err instanceof RecordFieldError + // A stale `expected_version` (00120): the caller re-reads and retries, + // so the model must SEE it — an in-band isError naming the stored version. + || err instanceof VersionConflictError || err instanceof LimitError; if (isClientError || isKeyScopeDenied) { toolSpan.clientError(msg).end(); diff --git a/supabase/functions/mcp/tools.read-refs.test.ts b/supabase/functions/mcp/tools.read-refs.test.ts index 2334add1d..9818c7026 100644 --- a/supabase/functions/mcp/tools.read-refs.test.ts +++ b/supabase/functions/mcp/tools.read-refs.test.ts @@ -59,7 +59,13 @@ type Row = { id: string; scope: string; key: string; value: string; updated_at: * feeds the singular path, as a LIST so the unscoped multi-scope case can be * exercised. */ -function fakeDb(opts: { rowsByScope?: Map; singleRows?: Row[] } = {}) { +function fakeDb(opts: { + rowsByScope?: Map; + singleRows?: Row[]; + // The `superseded_by` lookup (00120): `.in('supersedes', ids)`, answered from + // this list filtered to the ids asked about. + successors?: { id: string; supersedes: string }[]; +} = {}) { const order: string[] = []; const queries: { table: string; @@ -102,6 +108,11 @@ function fakeDb(opts: { rowsByScope?: Map; singleRows?: Row[] } = return Promise.resolve() .then(() => new Promise((res) => queueMicrotask(() => res(undefined)))) .then(() => { + const supersedesIn = rec.filters.find(([c]) => c === 'supersedes'); + if (supersedesIn) { + const ids = supersedesIn[1] as string[]; + return { data: (opts.successors ?? []).filter((r) => ids.includes(r.supersedes)), error: null }; + } if (!rec.batch) { // The singular path: `scope` may be absent, so the canned rows // are filtered by it only when the query actually carried one. @@ -295,17 +306,56 @@ Deno.test('AC-6: one query per distinct scope, all started before the first reso span, ); - // 3 distinct scopes, 4 keys total -> exactly 3 query builds, never 4 or 1. - assertEquals(queries.length, 3); - const byScope = new Map(queries.map((q) => [q.filters.find(([c]) => c === 'scope')?.[1], q])); + // 3 distinct scopes, 4 keys total -> exactly 3 read queries, never 4 or 1 — + // plus ONE `superseded_by` lookup for the whole batch (00120), never one per row. + const reads = queries.filter((q) => q.filters.some(([c]) => c === 'scope')); + assertEquals(reads.length, 3); + const lookups = queries.filter((q) => q.filters.some(([c]) => c === 'supersedes')); + assertEquals(lookups.length, 1); + assertEquals(new Set(lookups[0].filters.find(([c]) => c === 'supersedes')?.[1] as string[]), new Set(['1', '2', '3', '4'])); + const byScope = new Map(reads.map((q) => [q.filters.find(([c]) => c === 'scope')?.[1], q])); assertEquals((byScope.get('repo::o/r') as typeof queries[number]).filters.find(([c]) => c === 'key')?.[1], ['b', 'c']); // All 3 must have STARTED (order recorded at .then()-invocation time) before // any of them resolves — the fake's queueMicrotask hop guarantees this by // construction (see the module docblock), so this assertion is a genuine - // check on the recorded order length, not a tautology. - assertEquals(order.length, 3); - assertEquals(new Set(order), new Set(['global', 'repo::o/r', 'project::demo'])); + // check on the recorded order length, not a tautology. The trailing '' + // is the `superseded_by` lookup, which runs after the reads it is about. + assertEquals(order.length, 4); + assertEquals(new Set(order.slice(0, 3)), new Set(['global', 'repo::o/r', 'project::demo'])); +}); + +Deno.test('00120: superseded_by is attached where a successor exists, and omitted otherwise', async () => { + const { span } = newRootSpan(); + const rowsByScope = new Map([ + ['global', [ + { id: '1', scope: 'global', key: 'a', value: 'v', updated_at: 't' }, + { id: '2', scope: 'global', key: 'b', value: 'v', updated_at: 't' }, + ]], + ]); + const { db } = fakeDb({ rowsByScope, successors: [{ id: '9', supersedes: '1' }] }); + + const result = await toolRead(db, { refs: ['global::a', 'global::b'] }, null, span) as { + entries: { id: string; superseded_by?: string[] }[]; + }; + const byId = new Map(result.entries.map((e) => [e.id, e])); + assertEquals(byId.get('1')?.superseded_by, ['9']); + assertEquals('superseded_by' in (byId.get('2') ?? {}), false); +}); + +Deno.test('00120: a singular read carries superseded_by, and the lookup carries the tenant filter', async () => { + const { span } = newRootSpan(); + const { db, queries } = fakeDb({ + singleRows: [{ id: '1', scope: 'global', key: 'a', value: 'v', updated_at: 't' }], + successors: [{ id: '9', supersedes: '1' }], + }); + + const result = await toolRead(db, { key: 'a', scope: 'global' }, 'user-1', span) as { superseded_by?: string[] }; + assertEquals(result.superseded_by, ['9']); + const lookup = queries.find((q) => q.filters.some(([c]) => c === 'supersedes')); + // `applyTenantScope` for a user in no org is `.eq('user_id', userId)`. + assertEquals(lookup?.filters.some(([c, v]) => c === 'user_id' && v === 'user-1'), true); + assertEquals(lookup?.filters.some(([c, v]) => c === 'archived_at' && v === null), true); }); Deno.test("AC-7: N found rows record one targeted read for all N ids", async () => { diff --git a/supabase/functions/mcp/tools.ts b/supabase/functions/mcp/tools.ts index d344fb964..1efe13a15 100644 --- a/supabase/functions/mcp/tools.ts +++ b/supabase/functions/mcp/tools.ts @@ -36,8 +36,10 @@ import { parseRecordFields, translateRecordFieldDbError, compactRecordFields, + withSupersededBy, RECORD_FIELD_COLUMNS, } from '../_shared/provenance/record-extensions.ts'; +import { fetchSupersededBy } from '../_shared/memory/superseded-by.ts'; import { recordAuditDeferred } from '../_shared/audit/audit.ts'; import { applyTenantScope, reachesPersonal, type KeyRestriction } from '../_shared/auth/tenant-scope.ts'; import { decodeCursor, buildPage } from './cursor.ts'; @@ -73,6 +75,7 @@ interface ListRow { value: string; tags: string[]; updated_at: string | null; + version?: number; } /** @@ -166,7 +169,7 @@ export async function toolWrite( // argument rather than a parameter threaded through all twelve. correlationId?: string | null, ) { - const { scope: rawScope, key, value, tags = [], source_agent, trigger, created_at, org, ttl_days, ttl_minutes, ttl_seconds, clear_ttl = false, origin_repo, origin_branch, origin_commit, origin_pr, kind, host, cited, attributes, content_type, supersedes } = params; + const { scope: rawScope, key, value, tags = [], source_agent, trigger, created_at, org, ttl_days, ttl_minutes, ttl_seconds, clear_ttl = false, origin_repo, origin_branch, origin_commit, origin_pr, kind, host, cited, attributes, content_type, supersedes, expected_version } = params; if (!rawScope || !key || !value) throw new UserInputError('scope, key, and value are required'); if (value.length > MAX_VALUE_BYTES) throw new UserInputError(`value exceeds ${MAX_VALUE_BYTES} bytes`); const scope = validateScope(rawScope); @@ -179,8 +182,9 @@ export async function toolWrite( // per field, so omitting one never erases what an earlier write recorded. const origin = parseOrigin({ origin_repo, origin_branch, origin_commit, origin_pr }); // Record extensions (00119): open attributes, the value's content type, and - // the id this record supersedes. Null means "keep what is stored". - const record = parseRecordFields({ attributes, content_type, supersedes }, value); + // the id this record supersedes. Null means "keep what is stored". And the + // optimistic-concurrency check (00120): null means "unchecked". + const record = parseRecordFields({ attributes, content_type, supersedes, expected_version }, value); // Taxonomy: explicit kind/host win; otherwise recover them from the loop tag. // The shared resolver is the one place the closed-vocabulary check and the // host-length clamp live, so storage and usage tracking classify identically. @@ -206,6 +210,7 @@ export async function toolWrite( ...(record.attributes ? { 'lorekit.attributes.count': Object.keys(record.attributes).length } : {}), ...(record.contentType ? { 'lorekit.content_type': record.contentType } : {}), ...(record.supersedes ? { 'lorekit.supersedes': true } : {}), + ...(record.expectedVersion !== null ? { 'lorekit.expected_version': record.expectedVersion } : {}), }); // 00003 replaced the plain unique constraint with PARTIAL indexes @@ -235,6 +240,9 @@ export async function toolWrite( p_attributes: record.attributes, p_content_type: record.contentType, p_supersedes: record.supersedes, + // Sent only when given, so an unchecked write is the exact call it was + // before 00120. The RPC holds the check and the write in one statement. + ...(record.expectedVersion !== null ? { p_expected_version: record.expectedVersion } : {}), // The calling key's restriction, BOTH axes (00068/00069). The RPC is the // LAST gate on the write path — the edge runs on the service-role client, // so the dispatcher's check above it is advisory — and it is also the only @@ -255,6 +263,7 @@ export async function toolWrite( inserted?: boolean; org_routed?: boolean; binding_org_slug?: string | null; + version?: number; }; // Emit the write outcome as a telemetry attribute (insert vs update) so @@ -293,10 +302,13 @@ export async function toolWrite( return { id: row.id, created_at: row.created_at, + version: row.version, notice: `Saved to your personal lore. The scope "${scope}" is shared with the "${row.binding_org_slug}" organization, but you're not a write-member — ask an admin to add you to share it with the team.`, }; } - const result: Record = { id: row.id, created_at: row.created_at }; + // `version` (00120) is what the caller passes back as `expected_version` on + // its next write, so it is returned on every write, not only when it moved. + const result: Record = { id: row.id, created_at: row.created_at, version: row.version }; if (ttlSeconds !== null || clear_ttl) result.expires_at = (row as { id: string; created_at: string; expires_at?: string | null }).expires_at ?? null; return result; } @@ -365,7 +377,8 @@ export async function toolRead( // A named scope still narrows in SQL — same single-row read as before, no // extra rows fetched to be discarded client-side. if (scope) query = query.eq('scope', scope); - if (userId) query = applyTenantScope(query, userId, await memberOrgIds(db, userId), keyScoping); + const orgIds = userId ? await memberOrgIds(db, userId) : []; + if (userId) query = applyTenantScope(query, userId, orgIds, keyScoping); // The cap is a safety valve on a pathological key, not a correctness // boundary: `scope`+`key` is unique, so the scoped path can only ever match // one row, and an unscoped one matches once per scope holding that key. @@ -385,7 +398,7 @@ export async function toolRead( .order('updated_at', { ascending: false }) .limit(UNSCOPED_READ_CANDIDATE_LIMIT); if (error) throw new Error(error.message); - const rows = (data ?? []) as { id: string; scope: string; value: string; updated_at: string; attributes?: unknown; content_type?: unknown; supersedes?: unknown }[]; + const rows = (data ?? []) as { id: string; scope: string; value: string; updated_at: string; attributes?: unknown; content_type?: unknown; supersedes?: unknown; version?: number }[]; // Stamped BEFORE the miss return, not after: a miss is exactly the population // this measure exists to size — how often an unscoped read fans out and still // finds nothing. Stamping it after the return also made this surface disagree @@ -407,7 +420,11 @@ export async function toolRead( // discarded were never shown to the caller, and counting them would inflate // the `opened_count / read_count` ratio `/insights` is built on. recordMemoryReads(db, [winner.id], 'targeted', 'mcp'); - const rest = compactRecordFields(winner); + // `superseded_by` (00120): the records that say they replace this one, read + // under the SAME tenant filter as the read above. + const links = await fetchSupersededBy(tracedDb, [winner.id], (q) => + userId ? applyTenantScope(q, userId, orgIds, keyScoping) : q); + const rest = withSupersededBy(compactRecordFields(winner), links); // `other_scopes` appears ONLY when the key was genuinely ambiguous — an // always-present empty array would be noise on every single-lesson read. const others = shadowedScopes(rows, winner); @@ -472,7 +489,7 @@ async function toolReadRefs( if (userId) query = applyTenantScope(query, userId, orgIds, keyScoping); const { data, error } = await query; if (error) throw new Error(error.message); - return (data ?? []) as { id: string; scope: string; key: string; value: string; updated_at: string; attributes?: unknown; content_type?: unknown; supersedes?: unknown }[]; + return (data ?? []) as { id: string; scope: string; key: string; value: string; updated_at: string; attributes?: unknown; content_type?: unknown; supersedes?: unknown; version?: number }[]; }; const rows = ( @@ -486,7 +503,10 @@ async function toolReadRefs( ).flat(); if (rows.length > 0) recordMemoryReads(db, rows.map((r) => r.id), 'targeted', 'mcp'); - const entries = rows.map(compactRecordFields); + // One `superseded_by` query for the whole batch, never one per ref. + const links = await fetchSupersededBy(tracedDb, rows.map((r) => r.id), (q) => + userId ? applyTenantScope(q, userId, orgIds, keyScoping) : q); + const entries = rows.map((r) => withSupersededBy(compactRecordFields(r), links)); const missing = missingRefs(parsed, rows); // `result.count` is what every other tool reports, so a batch read that omits // it is absent from any panel built over that attribute rather than reading @@ -567,6 +587,12 @@ export async function toolList( span.setAttributes({ ...(scope ? { 'lorekit.scope': scope } : {}), 'lorekit.list.unscoped': scope === null }); const tracedDb = createTracedClient(db, span); + const orgIds = userId ? await memberOrgIds(db, userId) : []; + // `superseded_by` (00120) for a page: ONE query over the page's ids, under + // the same tenant filter as the page itself. List filtering is unchanged — + // a superseded record still appears, it is only marked. + const supersededBy = (ids: (string | undefined)[]) => + fetchSupersededBy(tracedDb, ids, (q) => (userId ? applyTenantScope(q, userId, orgIds, keyScoping) : q)); if (ranked) { // ── Ranked path (D1/D2/D4/D8) ───────────────────────────────────────── @@ -581,14 +607,14 @@ export async function toolList( // lesson came from where — and a field that appears only on the unscoped // shape would be one more conditional response for every consumer to // dispatch on, which `memory.read`'s `refs` already cost us once. - .select('id,scope,key,value,tags,updated_at,seen_count,origin_pr') + .select('id,scope,key,value,tags,updated_at,seen_count,origin_pr,version') .is('archived_at', null) .or('expires_at.is.null,expires_at.gt.now()') .order('updated_at', { ascending: false }) .order('id', { ascending: false }) .limit(CANDIDATE_LIMIT); if (scope) rankQuery = rankQuery.eq('scope', scope); - if (userId) rankQuery = applyTenantScope(rankQuery, userId, await memberOrgIds(db, userId), keyScoping); + if (userId) rankQuery = applyTenantScope(rankQuery, userId, orgIds, keyScoping); if (tags.length) rankQuery = rankQuery.overlaps('tags', pgArrayLiteral(tags)); // Taxonomy filters are applied BEFORE ranking, not after: the candidate // window is bounded at CANDIDATE_LIMIT, so filtering afterwards would rank @@ -598,7 +624,7 @@ export async function toolList( const { data: rankData, error: rankError } = await rankQuery; if (rankError) throw new Error(rankError.message); - const candidates: (RankableLesson & { id: string; key: string; value: string; tags: string[]; updated_at: string | null })[] = (rankData ?? []).map( + const candidates: (RankableLesson & { id: string; key: string; value: string; tags: string[]; updated_at: string | null; version?: number })[] = (rankData ?? []).map( // deno-lint-ignore no-explicit-any (r: any) => ({ id: r.id, @@ -606,6 +632,7 @@ export async function toolList( value: r.value, tags: r.tags, updated_at: r.updated_at, + version: r.version, seen_count: r.seen_count, outcome: outcomeFromTags(r.tags, r.origin_pr), }), @@ -619,16 +646,21 @@ export async function toolList( // output; the tool-catalog `order` description (tool-catalog.ts) says so too. const page = selectDiverse(rankedLessons, pageLimit); + const links = await supersededBy(page.map(({ entry }) => entry.id)); const entries = page.map(({ entry }) => - projectListEntry( - { - id: entry.id, - key: entry.key, - value: entry.value, - tags: entry.tags, - updated_at: entry.updated_at, - }, - summarize, + withSupersededBy( + projectListEntry( + { + id: entry.id, + key: entry.key, + value: entry.value, + tags: entry.tags, + updated_at: entry.updated_at, + version: entry.version, + }, + summarize, + ), + links, ), ); @@ -659,14 +691,14 @@ export async function toolList( let query = tracedDb .from('memories') // `scope` selected unconditionally — see the ranked path's note above. - .select('id,scope,key,value,tags,updated_at') + .select('id,scope,key,value,tags,updated_at,version') .is('archived_at', null) .or('expires_at.is.null,expires_at.gt.now()') .order('updated_at', { ascending: false }) .order('id', { ascending: false }) .limit(pageLimit + 1); if (scope) query = query.eq('scope', scope); - if (userId) query = applyTenantScope(query, userId, await memberOrgIds(db, userId), keyScoping); + if (userId) query = applyTenantScope(query, userId, orgIds, keyScoping); if (tags.length) query = query.overlaps('tags', pgArrayLiteral(tags)); if (kind) query = query.eq('kind', kind); if (host) query = query.eq('host', host); @@ -688,9 +720,10 @@ export async function toolList( // legacy shape that predates it — the query above always selects `id`, so // filtering `undefined` here is a type narrowing, not an expected drop. recordMemoryReads(db, (page.entries as ListRow[]).map((row) => row.id).filter((id): id is string => id !== undefined), 'bulk'); + const links = await supersededBy((page.entries as ListRow[]).map((row) => row.id)); return { ...page, - entries: (page.entries as ListRow[]).map((row) => projectListEntry(row, summarize)), + entries: (page.entries as ListRow[]).map((row) => withSupersededBy(projectListEntry(row, summarize), links)), }; } @@ -796,7 +829,7 @@ export async function toolSearch( const tracedDb = createTracedClient(db, span); let query = tracedDb .from('memories') - .select('id,key,value,scope,tags,updated_at') + .select('id,key,value,scope,tags,updated_at,version') .textSearch('fts', q, { type: 'websearch', config: 'english' }) .is('archived_at', null) .or('expires_at.is.null,expires_at.gt.now()') @@ -806,7 +839,8 @@ export async function toolSearch( // Tenant .or() and the scope-glob .or() below are applied as two separate // .or() calls, which PostgREST ANDs together — never merged into one // filter (see tenant-scope.ts and the Edge Cases note in plan.md). - if (userId) query = applyTenantScope(query, userId, await memberOrgIds(db, userId), keyScoping); + const orgIds = userId ? await memberOrgIds(db, userId) : []; + if (userId) query = applyTenantScope(query, userId, orgIds, keyScoping); if (tags.length) query = query.overlaps('tags', pgArrayLiteral(tags)); if (scopes?.length) { const exactScopes: string[] = []; @@ -844,7 +878,10 @@ export async function toolSearch( span.setAttributes({ 'lorekit.result.count': page.entries.length }); // memory.search is a BULK read for the per-memory counter (migration 00077). recordMemoryReads(db, page.entries.map((e) => e.id), 'bulk'); - return page; + // `superseded_by` (00120): one query over the page, same tenant filter. + const links = await fetchSupersededBy(tracedDb, page.entries.map((e) => e.id), (q) => + userId ? applyTenantScope(q, userId, orgIds, keyScoping) : q); + return { ...page, entries: page.entries.map((e) => withSupersededBy(e, links)) }; } /** Soft-archive a memory by setting archived_at. */ diff --git a/supabase/functions/memories/handlers/create.ts b/supabase/functions/memories/handlers/create.ts index b4bfbd1b4..dde50477b 100644 --- a/supabase/functions/memories/handlers/create.ts +++ b/supabase/functions/memories/handlers/create.ts @@ -15,7 +15,9 @@ import { parseRecordFields, RecordFieldError, translateRecordFieldDbError, + VersionConflictError, } from '../../_shared/provenance/record-extensions.ts'; +import { versionConflict } from '../../_shared/api/respond.ts'; import { resolveKindHost } from '../../_shared/schemas/tags.ts'; import { recordAuditDeferred } from '../../_shared/audit/audit.ts'; import { recordCitations } from '../../_shared/telemetry/citations.ts'; @@ -147,6 +149,10 @@ export async function handleCreate( p_attributes: record.attributes, p_content_type: record.contentType, p_supersedes: record.supersedes, + // Optimistic concurrency (00120). Sent only when given, so an unchecked + // write is the exact call it was before; the RPC checks and writes in one + // statement. + ...(record.expectedVersion !== null ? { p_expected_version: record.expectedVersion } : {}), // The calling key's restriction, BOTH axes (00068/00069). The RPC is the // last gate the edge cannot bypass — it runs on the service-role client, so // the `firstDeniedScope` refusal above is advisory — and the only place that @@ -162,6 +168,7 @@ export async function handleCreate( if (error) { const fieldError = translateRecordFieldDbError(error); if (fieldError instanceof RecordFieldError) return badRequest(fieldError.message, undefined, cors); + if (fieldError instanceof VersionConflictError) return versionConflict(fieldError, cors); const mapped = translateDbError(error); if (mapped) return mapped.toResponse(cors); span.error(`DB: ${error.message}`); @@ -185,7 +192,7 @@ export async function handleCreate( origin_commit: string | null; origin_pr: number | null; kind: string | null; host: string | null; seen_count: number | null; attributes: Record | null; content_type: string | null; - supersedes: string | null; + supersedes: string | null; version: number | null; }; const row = data as MemoryWriteRow | null; if (!row?.id) { @@ -200,7 +207,7 @@ export async function handleCreate( origin_repo: row.origin_repo, origin_branch: row.origin_branch, origin_commit: row.origin_commit, origin_pr: row.origin_pr, kind: row.kind, host: row.host, seen_count: row.seen_count ?? undefined, attributes: row.attributes ?? undefined, content_type: row.content_type ?? undefined, - supersedes: row.supersedes, + supersedes: row.supersedes, version: row.version ?? undefined, }; // Queue the embedding. Returns synchronously and is a no-op unless embedding diff --git a/supabase/functions/memories/handlers/get.ts b/supabase/functions/memories/handlers/get.ts index 062dca1e5..c14f97d37 100644 --- a/supabase/functions/memories/handlers/get.ts +++ b/supabase/functions/memories/handlers/get.ts @@ -5,12 +5,14 @@ import { createTracedClient } from '../../_shared/telemetry/otel.ts'; import type { TracedQuery, Span } from '../../_shared/telemetry/otel.ts'; import type { DbClient } from '../../_shared/api/auth.ts'; import type { Tables } from '../../_shared/db/database.types.ts'; -import { getMemberOrgIds, applyRestTenantScope } from '../../_shared/api/tenant.ts'; +import { getMemberOrgIds, applyRestTenantScope, restSupersededBy } from '../../_shared/api/tenant.ts'; +import { withSupersededBy } from '../../_shared/provenance/record-extensions.ts'; import { keyRestriction } from '../../_shared/api/auth.ts'; import { MEMORY_SELECT, shapeMemoryRow } from '../../_shared/schemas/memory.ts'; import { recordMemoryReads } from '../../_shared/telemetry/memory-reads.ts'; import { CLIENT_HEADER } from '../../_shared/api/router.ts'; import { parseUsageClient } from '../../_shared/telemetry/usage-stats.ts'; +import { NOT_EXPIRED } from '../../_shared/memory/live.ts'; type MemoryRow = Tables<'memories'>; @@ -29,12 +31,12 @@ export async function handleGet( .select(MEMORY_SELECT) .eq('id', v.data) .is('archived_at', null) - .or('expires_at.is.null,expires_at.gt.now()'); + .or(NOT_EXPIRED); // api_key auth uses service-role client (bypasses RLS) — apply tenant filter. // JWT auth uses RLS-scoped client — RLS handles visibility automatically. - if (auth.type === 'api_key' && auth.userId) { - const orgIds = await getMemberOrgIds(db, auth.userId, span); + const orgIds = auth.type === 'api_key' && auth.userId ? await getMemberOrgIds(db, auth.userId, span) : undefined; + if (auth.type === 'api_key' && auth.userId && orgIds) { q = applyRestTenantScope(q, auth.userId, orgIds, keyRestriction(auth)); } @@ -42,7 +44,9 @@ export async function handleGet( if (error) { span.error(`DB: ${error.message}`); throw error; } if (!data) return notFound('Memory', cors); // One record read — surfaced for the router's usage event (RESULT_COUNT_HEADER). - const res = ok(shapeMemoryRow(data as Record), cors); + // `superseded_by` (00120), under the same visibility as this read. + const links = await restSupersededBy(db, auth, span, [(data as { id: string }).id], orgIds); + const res = ok(withSupersededBy(shapeMemoryRow(data as Record), links), cors); res.headers.set('X-LoreKit-Result-Count', '1'); // GET /:id is the REST equivalent of memory.read — one exact scope+key — // so it is a TARGETED read for the per-memory counter (migration 00077). diff --git a/supabase/functions/memories/handlers/list.ts b/supabase/functions/memories/handlers/list.ts index 7aa7feedf..ec882a086 100644 --- a/supabase/functions/memories/handlers/list.ts +++ b/supabase/functions/memories/handlers/list.ts @@ -1,6 +1,7 @@ import type { AuthContext } from '../../_shared/api/auth.ts'; import { keyRestriction } from '../../_shared/api/auth.ts'; -import { firstDeniedScope } from '../../_shared/api/tenant.ts'; +import { firstDeniedScope, restSupersededBy } from '../../_shared/api/tenant.ts'; +import { withSupersededBy } from '../../_shared/provenance/record-extensions.ts'; import { badRequest, forbidden, ok } from '../../_shared/api/respond.ts'; import { validateOptionalBody, validateQuery } from '../../_shared/api/validate.ts'; import { parseScopeFilter } from '../../_shared/scope/scope.ts'; @@ -295,7 +296,11 @@ async function respondWithPage( }); // Let the router record the RECORD count (not just the call) — see // RESULT_COUNT_HEADER in _shared/api/router.ts. - const res = ok({ ...page, entries: page.entries.map(shapeRpcRow), total }, cors); + // `superseded_by` (00120): one query over the page, under the same + // visibility. The list's own filters and `total` are unchanged — a superseded + // record still appears, it is only marked. + const links = await restSupersededBy(db, auth, span, page.entries.map((e) => e.id)); + const res = ok({ ...page, entries: page.entries.map((e) => withSupersededBy(shapeRpcRow(e), links)), total }, cors); res.headers.set('X-LoreKit-Result-Count', String(page.entries.length)); // memory.list is a BULK read (every row a listing call returned) for the // per-memory counter (migration 00077) — one statement for the whole page — diff --git a/supabase/functions/memories/handlers/read.ts b/supabase/functions/memories/handlers/read.ts index 2449eeee8..95a7a4a0e 100644 --- a/supabase/functions/memories/handlers/read.ts +++ b/supabase/functions/memories/handlers/read.ts @@ -5,7 +5,8 @@ import { createTracedClient } from '../../_shared/telemetry/otel.ts'; import type { TracedQuery, Span } from '../../_shared/telemetry/otel.ts'; import type { DbClient } from '../../_shared/api/auth.ts'; import type { Tables } from '../../_shared/db/database.types.ts'; -import { getMemberOrgIds, applyRestTenantScope } from '../../_shared/api/tenant.ts'; +import { getMemberOrgIds, applyRestTenantScope, restSupersededBy } from '../../_shared/api/tenant.ts'; +import { withSupersededBy } from '../../_shared/provenance/record-extensions.ts'; import { keyRestriction } from '../../_shared/api/auth.ts'; import { MEMORY_SELECT, shapeMemoryRow, ReadMemoriesBodySchema } from '../../_shared/schemas/memory.ts'; import { parseMemoryRefs } from '../../_shared/scope/scope.ts'; @@ -102,9 +103,11 @@ export async function handleRead( 'lorekit.result.count': rows.length, 'lorekit.refs.missing': missing.length, }); + // `superseded_by` (00120): ONE query for the whole batch, same visibility. + const links = await restSupersededBy(db, auth, span, rows.map((r) => r.id), orgIds); const res = ok( { - entries: rows.map((r) => shapeMemoryRow(r as Record)), + entries: rows.map((r) => withSupersededBy(shapeMemoryRow(r as Record), links)), missing, }, cors, diff --git a/supabase/functions/memories/handlers/search.ts b/supabase/functions/memories/handlers/search.ts index 43d44f9f0..486591c47 100644 --- a/supabase/functions/memories/handlers/search.ts +++ b/supabase/functions/memories/handlers/search.ts @@ -8,7 +8,8 @@ import { SearchMemoriesBodySchema, MEMORY_SELECT, shapeMemoryRow } from '../../_ import { normalizeTagList, pgArrayLiteral } from '../../_shared/schemas/tags.ts'; import type { DbClient } from '../../_shared/api/auth.ts'; import type { Tables } from '../../_shared/db/database.types.ts'; -import { getMemberOrgIds, applyRestTenantScope, firstDeniedScope } from '../../_shared/api/tenant.ts'; +import { getMemberOrgIds, applyRestTenantScope, firstDeniedScope, restSupersededBy } from '../../_shared/api/tenant.ts'; +import { withSupersededBy } from '../../_shared/provenance/record-extensions.ts'; import { keyRestriction } from '../../_shared/api/auth.ts'; import { applyFilter } from '../../_shared/api/filter.ts'; import { recordMemoryReads } from '../../_shared/telemetry/memory-reads.ts'; @@ -39,8 +40,8 @@ export async function handleSearch( // api_key auth uses service-role client (bypasses RLS) — apply tenant filter. // JWT auth uses RLS-scoped client — RLS handles visibility automatically. - if (auth.type === 'api_key' && auth.userId) { - const orgIds = await getMemberOrgIds(db, auth.userId, span); + const orgIds = auth.type === 'api_key' && auth.userId ? await getMemberOrgIds(db, auth.userId, span) : undefined; + if (auth.type === 'api_key' && auth.userId && orgIds) { q = applyRestTenantScope(q, auth.userId, orgIds, keyRestriction(auth)); } if (body.q) q = q.textSearch('fts', body.q, { type: 'websearch', config: 'english' }); @@ -72,7 +73,10 @@ export async function handleSearch( const page = buildPage(data ?? [], body.limit); span.setAttributes({ 'lorekit.result_count': page.entries.length }); // Record count for the router's usage event — see RESULT_COUNT_HEADER. - const res = ok({ ...page, entries: page.entries.map(shapeMemoryRow) }, cors); + // `superseded_by` (00120): one query over the page, same visibility. Search + // filtering is unchanged — a superseded record still appears, marked. + const links = await restSupersededBy(db, auth, span, page.entries.map((e) => e.id), orgIds); + const res = ok({ ...page, entries: page.entries.map((e) => withSupersededBy(shapeMemoryRow(e), links)) }, cors); res.headers.set('X-LoreKit-Result-Count', String(page.entries.length)); // memory.search is a BULK read for the per-memory counter (migration 00077). recordMemoryReads(db, page.entries.map((e) => e.id), 'bulk'); diff --git a/supabase/functions/memories/handlers/update.expired.test.ts b/supabase/functions/memories/handlers/update.expired.test.ts new file mode 100644 index 000000000..6169a6d97 --- /dev/null +++ b/supabase/functions/memories/handlers/update.expired.test.ts @@ -0,0 +1,101 @@ +/** + * `PATCH /memories/:id` treats an EXPIRED row as absent, exactly as + * `GET /memories/:id` does (`get.ts`) and as `memory_write` counts it for + * `expected_version` (00120): a 404, never an edit and never a 409 naming the + * dead row's version. + * + * `supabase start` is impossible in this sandbox, so the handler runs against a + * chainable fake `db` holding one row, which evaluates the filters the handler + * applies (`eq`, `is`, and the `NOT_EXPIRED` `.or()`), so a missing predicate + * shows up as the wrong status rather than as an assertion on call shape. + * + * Run with: deno test --no-check --node-modules-dir=none supabase/functions/memories/handlers/update.expired.test.ts + */ +import { assertEquals } from 'https://deno.land/std@0.224.0/assert/mod.ts'; +import { ExportBatch, Span } from '../../_shared/telemetry/otel.ts'; +import { NOT_EXPIRED } from '../../_shared/memory/live.ts'; +import { handleUpdate } from './update.ts'; + +const ID = '11111111-1111-4111-8111-111111111111'; +const USER = '22222222-2222-4222-8222-222222222222'; +const PAST = '2020-01-01T00:00:00.000Z'; +const FUTURE = '2999-01-01T00:00:00.000Z'; + +type Row = Record & { id: string; version: number; expires_at: string | null }; + +function newSpan(): Span { + return new Span('lorekit.test', { traceId: 'a'.repeat(32), spanId: 'b'.repeat(16), sampled: true }, new ExportBatch(), 2); +} + +function fakeDb(row: Row) { + const writes: Record[] = []; + function chain(table: string) { + const preds: ((r: Row) => boolean)[] = []; + let patch: Record | null = null; + // deno-lint-ignore no-explicit-any + const builder: any = { + update(p: Record) { patch = p; return builder; }, + insert(_r: unknown) { return builder; }, + select(..._a: unknown[]) { return builder; }, + eq(col: string, val: unknown) { preds.push((r) => r[col] === val); return builder; }, + is(col: string, val: unknown) { preds.push((r) => (r[col] ?? null) === val); return builder; }, + or(expr: string) { + if (expr === NOT_EXPIRED) preds.push((r) => r.expires_at === null || Date.parse(r.expires_at) > Date.now()); + return builder; + }, + maybeSingle() { return builder; }, + then(resolve: (v: unknown) => void, reject?: (e: unknown) => void) { + if (table !== 'memories') return Promise.resolve({ data: null, error: null }).then(resolve, reject); + const hit = preds.every((p) => p(row)); + if (hit && patch) { writes.push(patch); Object.assign(row, patch, { version: row.version + 1 }); } + return Promise.resolve({ data: hit ? { ...row } : null, error: null }).then(resolve, reject); + }, + }; + return builder; + } + // deno-lint-ignore no-explicit-any + const db: any = { from: (t: string) => chain(t), rpc: () => Promise.resolve({ data: null, error: null }) }; + return { db, writes }; +} + +function row(expires_at: string | null): Row { + return { + id: ID, user_id: USER, scope: 'global', key: 'k', value: 'old', tags: [], version: 4, + archived_at: null, expires_at, org_id: null, + }; +} + +async function patch(r: Row, body: Record) { + const { db, writes } = fakeDb(r); + const req = new Request(`http://local/memories/${ID}`, { + method: 'PATCH', headers: { 'content-type': 'application/json' }, body: JSON.stringify(body), + }); + const res = await handleUpdate( + req, { type: 'api_key', userId: USER, permissions: ['read', 'write'] }, db, newSpan(), { id: ID }, {}, + ); + return { status: res.status, writes }; +} + +Deno.test('PATCH on an expired memory is 404 and writes nothing, like GET /:id', async () => { + const { status, writes } = await patch(row(PAST), { value: 'new' }); + assertEquals(status, 404); + assertEquals(writes.length, 0); +}); + +Deno.test('PATCH with expected_version on an expired memory is 404, not a 409 naming its version', async () => { + const { status, writes } = await patch(row(PAST), { value: 'new', expected_version: 3 }); + assertEquals(status, 404); + assertEquals(writes.length, 0); +}); + +Deno.test('PATCH with a stale expected_version on a live memory is still 409', async () => { + const { status, writes } = await patch(row(FUTURE), { value: 'new', expected_version: 3 }); + assertEquals(status, 409); + assertEquals(writes.length, 0); +}); + +Deno.test('PATCH with the current expected_version on a live memory succeeds', async () => { + const { status, writes } = await patch(row(null), { value: 'new', expected_version: 4 }); + assertEquals(status, 200); + assertEquals(writes.length, 1); +}); diff --git a/supabase/functions/memories/handlers/update.ts b/supabase/functions/memories/handlers/update.ts index ff5b97b7a..fd824f5f5 100644 --- a/supabase/functions/memories/handlers/update.ts +++ b/supabase/functions/memories/handlers/update.ts @@ -2,7 +2,7 @@ import { applyKeyScopeFilter } from '../../_shared/api/tenant.ts'; import type { AuthContext } from '../../_shared/api/auth.ts'; import { auditUserId } from '../../_shared/api/auth.ts'; import { recordAudit } from '../../_shared/audit/audit.ts'; -import { ok, notFound, badRequest, dryRun } from '../../_shared/api/respond.ts'; +import { ok, notFound, badRequest, dryRun, versionConflict } from '../../_shared/api/respond.ts'; import { DRY_RUN_HEADER, isDryRunHeader } from '../../_shared/limits/dry-run.ts'; import { validateUuid, validateBody } from '../../_shared/api/validate.ts'; import { createTracedClient } from '../../_shared/telemetry/otel.ts'; @@ -10,7 +10,13 @@ import type { TracedQuery, Span } from '../../_shared/telemetry/otel.ts'; import { UpdateMemoryBodySchema, MEMORY_SELECT, shapeMemoryRow } from '../../_shared/schemas/memory.ts'; import type { DbClient } from '../../_shared/api/auth.ts'; import type { Tables } from '../../_shared/db/database.types.ts'; -import { RecordFieldError, translateRecordFieldDbError } from '../../_shared/provenance/record-extensions.ts'; +import { NOT_EXPIRED } from '../../_shared/memory/live.ts'; +import { + parseExpectedVersion, + RecordFieldError, + translateRecordFieldDbError, + VersionConflictError, +} from '../../_shared/provenance/record-extensions.ts'; type MemoryRow = Tables<'memories'>; @@ -33,9 +39,19 @@ export async function handleUpdate( // set (mirroring memory_write's p_clear_ttl precedence, 00031), and the // deadline is computed from the request clock rather than the database's, // which is accurate to within clock skew and keeps this a plain column patch. - const { ttl_days: ttlDays, clear_ttl: clearTtl, ...columns } = bodyV.data as Record & { - ttl_days?: number; clear_ttl?: boolean; + const { ttl_days: ttlDays, clear_ttl: clearTtl, expected_version: rawExpected, ...columns } = bodyV.data as Record & { + ttl_days?: number; clear_ttl?: boolean; expected_version?: unknown; }; + // `expected_version` (00120) is not a column either: it becomes a predicate + // on the UPDATE itself, so the check and the write are one statement. + let expectedVersion: number | null; + try { + expectedVersion = parseExpectedVersion(rawExpected); + } catch (err) { + if (err instanceof RecordFieldError) return badRequest(err.message, undefined, cors); + throw err; + } + if (expectedVersion !== null) span.setAttributes({ 'lorekit.expected_version': expectedVersion }); const patch: Record = {}; for (const [k, v] of Object.entries(columns)) { if (v !== undefined) patch[k] = v; } @@ -51,13 +67,21 @@ export async function handleUpdate( } const tracedDb = createTracedClient(db, span); - let q: TracedQuery = tracedDb.from('memories').update(patch).eq('id', idV.data).is('archived_at', null); - // api_key auth uses service-role client — restrict to caller's own rows. - // JWT auth uses RLS-scoped client — RLS handles access control (org-owned rows included). - if (auth.type === 'api_key' && auth.userId) q = q.eq('user_id', auth.userId); - // The allowlist half. `user_id` alone let a scoped key patch a memory outside - // its allowlist by id. - q = applyKeyScopeFilter(q, auth); + // The row this caller may patch: live (unarchived AND unexpired — the same + // predicate as GET /:id, so PATCH cannot edit a row GET 404s on, and an + // expired row's version never reaches the 409 path: it is absent, as in + // memory_write), its own (api_key — the service-role client bypasses RLS) or + // RLS-visible (JWT, org-owned rows included), and inside the key's allowlist + // (`user_id` alone let a scoped key patch a memory outside its allowlist by + // id). One definition, used by the update and by the version-conflict lookup + // below, so the two cannot disagree on "which row". + const target = >(query: Q): Q => { + let q = query.eq('id', idV.data).is('archived_at', null).or(NOT_EXPIRED) as Q; + if (auth.type === 'api_key' && auth.userId) q = q.eq('user_id', auth.userId) as Q; + return applyKeyScopeFilter(q, auth); + }; + let q: TracedQuery = target(tracedDb.from('memories').update(patch)); + if (expectedVersion !== null) q = q.eq('version', expectedVersion); // Dry-run: everything above validated + authorized; stop before any write. if (isDryRunHeader(req.headers.get(DRY_RUN_HEADER))) return dryRun(cors); @@ -77,7 +101,22 @@ export async function handleUpdate( // Audit only when the update actually matched a row — the REST analogue of // the MCP tools' `if (archived)` / `if (deleted)` guards. A 404 changed // nothing and must not leave an audit trail claiming otherwise. - if (!data) return notFound('Memory', cors); + if (!data) { + // With an expected_version, no row means either "no such memory" or "it + // moved on". Tell them apart under the SAME visibility as the update, so a + // 409 never reveals a row the caller could not have patched. + if (expectedVersion !== null) { + const { data: current, error: currentError } = await target( + tracedDb.from('memories').select('version'), + ).maybeSingle(); + if (currentError) { span.error(`DB: ${currentError.message}`); throw currentError; } + const currentVersion = (current as { version?: number } | null)?.version; + if (typeof currentVersion === 'number') { + return versionConflict(new VersionConflictError(expectedVersion, currentVersion), cors); + } + } + return notFound('Memory', cors); + } const updated = data as unknown as { id: string; scope: string; key: string }; await recordAudit( diff --git a/supabase/migrations/00120_memory_version.sql b/supabase/migrations/00120_memory_version.sql new file mode 100644 index 000000000..036297e0f --- /dev/null +++ b/supabase/migrations/00120_memory_version.sql @@ -0,0 +1,831 @@ +-- 00120 — a version number on every memory, and optimistic concurrency on write. +-- +-- THE GAP. A memory gets corrected (a January runbook fixed in March) and a +-- reader cannot tell: a write to the same scope+key overwrites in place and only +-- `updated_at` moves — and `updated_at` also moves for edits that change nothing +-- a reader would read (a TTL, a re-sighting). Two agents correcting the same +-- lesson also race: the second write silently replaces the first. +-- +-- version integer, starts at 1, +1 on every CONTENT change. +-- Content is what a reader reads: `value`, `content_type`, +-- `attributes`, `tags`, and a `supersedes` link being set +-- or changed. A TTL change, a read/open/cite counter, an +-- embedding, archive/restore, an owner move, a re-write of +-- identical content, and a link CLEARED because its target +-- was hard-deleted (00119) are not edits and do not bump it. +-- p_expected_version on memory_write: the version the caller last read. A +-- mismatch is refused with LK003 `version_conflict` and +-- nothing is written; 0 means "the key must not exist". +-- +-- WHY A TRIGGER. Every write path obeys the rule — memory_write's upsert, a +-- REST PATCH (a plain column update), a direct PostgREST update, a groom — so it +-- lives on the table, not in one RPC. The same trigger is why a caller can never +-- set `version` itself: an INSERT always starts at 1 and an UPDATE always gets +-- the derived value, whatever the statement said. +-- +-- TRIGGER ORDER. Postgres fires same-timing row triggers in NAME order, so +-- `memories_bump_version` runs before `memories_updated_at` (00103). That order +-- is what lets the updated_at trigger compare whole rows unchanged: by the time +-- it runs, `version` differs from the old row only when content did. +-- +-- `memory_write` changes signature (a new parameter and a new returned column), +-- so 00119's overload is DROPPED and recreated — the body is 00119's verbatim +-- except the lines marked 00120. `lorekit_memory_list` (00115) returns `version` +-- too, which changes its return type, so it is likewise dropped and recreated +-- verbatim except the marked lines; its arguments, grants and comment are 00115's. +-- +-- The column is a constant default on a NOT NULL column: since Postgres 11 that +-- is a catalog-only change, so existing rows read back as 1 without a rewrite +-- or a backfill UPDATE (REVIEW.md invariants 1 and 2). The brief ACCESS +-- EXCLUSIVE lock it still takes runs under a lock_timeout. + +-- 1. Column ─────────────────────────────────────────────────────────────────── + +set local lock_timeout = '5s'; + +alter table memories + add column if not exists version integer not null default 1; + +-- The CHECK is added NOT VALID — a catalog-only change, no scan of existing +-- rows under the ACCESS EXCLUSIVE lock. New and updated rows are checked from +-- the moment it is added. It is VALIDATED in 00121, not here: a migration file +-- runs in ONE transaction, so a `validate constraint` in this file would scan +-- while this transaction still holds the ACCESS EXCLUSIVE lock `add column` +-- took (a lock is held to the end of its transaction; validate's own weaker +-- SHARE UPDATE EXCLUSIVE does not release it). Validation cannot fail: every +-- existing row reads `version` = 1 from the default. +alter table memories drop constraint if exists memories_version_positive; +alter table memories add constraint memories_version_positive + check (version >= 1) not valid; + +reset lock_timeout; + +comment on column memories.version is + 'Starts at 1 and increases by 1 on every content change (value, content_type, attributes, tags, or a supersedes link set or changed). Maintained by the memories_bump_version trigger; a caller cannot set it.'; + +-- 2. The bump trigger ───────────────────────────────────────────────────────── + +create or replace function lorekit_memories_bump_version() +returns trigger +language plpgsql +set search_path = public +as $$ +begin + if tg_op = 'INSERT' then + new.version := 1; + return new; + end if; + + if new.value is distinct from old.value + or new.content_type is distinct from old.content_type + or new.attributes is distinct from old.attributes + or new.tags is distinct from old.tags + -- Setting or changing a link is an edit; clearing one is not (00119: a + -- link is cleared by `on delete set null` when its target is hard-deleted). + or (new.supersedes is not null and new.supersedes is distinct from old.supersedes) + then + new.version := old.version + 1; + else + new.version := old.version; + end if; + return new; +end; +$$; + +comment on function lorekit_memories_bump_version() is + 'BEFORE INSERT OR UPDATE on memories: an insert starts at version 1; an update gets old.version + 1 when value, content_type, attributes, tags or a non-null supersedes changed, and old.version otherwise. Overrides any version the statement set.'; + +revoke execute on function lorekit_memories_bump_version() from public, anon; + +drop trigger if exists memories_bump_version on memories; +create trigger memories_bump_version + before insert or update on memories + for each row execute function lorekit_memories_bump_version(); + +-- 3. memory_write ───────────────────────────────────────────────────────────── + +drop function if exists memory_write( + uuid, text, text, text, text[], text, text, timestamptz, text, + integer, boolean, text, text, text, integer, text, text, + jsonb, text, uuid, text[], text, uuid[] +); + +create or replace function memory_write( + p_user_id uuid, + p_scope text, + p_key text, + p_value text, + p_tags text[] default '{}', + p_source_agent text default null, + p_trigger text default null, + p_created_at timestamptz default null, + p_org_slug text default null, + p_ttl_seconds integer default null, + p_clear_ttl boolean default false, + p_origin_repo text default null, + p_origin_branch text default null, + p_origin_commit text default null, + p_origin_pr integer default null, + p_kind text default null, + p_host text default null, + p_attributes jsonb default null, + p_content_type text default null, + p_supersedes uuid default null, + p_key_scopes text[] default '{}', + p_key_org_access text default 'all', + p_key_org_ids uuid[] default '{}', + p_expected_version integer default null -- 00120 +) +returns table ( + id uuid, + created_at timestamptz, + inserted boolean, + org_routed boolean, + binding_org_slug text, + expires_at timestamptz, + scope text, + key text, + value text, + tags text[], + source_agent text, + trigger text, + updated_at timestamptz, + archived_at timestamptz, + origin_repo text, + origin_branch text, + origin_commit text, + origin_pr integer, + kind text, + host text, + seen_count integer, + attributes jsonb, + content_type text, + supersedes uuid, + version integer -- 00120 +) +language plpgsql +security definer +set search_path = public +as $$ +-- See 00075's comment for why this directive is required (the OUT columns +-- `scope`/`key` collide with the bare column names in the `on conflict` +-- arbiter lists below). +#variable_conflict use_column +declare + v_org_id uuid; + v_binding_org uuid; + v_binding_slug text; + v_expires_at timestamptz; + v_ttl_action text := 'keep'; + v_current_version integer; -- 00120 + v_was_expired boolean; -- 00120 +begin + -- The actor is the verified caller, never a request field (the rule 00046 + -- applied to archive/restore/purge). This function is SECURITY DEFINER and + -- granted to anon/authenticated, so without this a PostgREST caller names + -- any p_user_id. A caller-supplied id is honoured only on a verified + -- service-role connection (the edge/MCP api_key path resolves the token + -- owner and passes it) or a direct connection with no JWT role at all; an + -- anon or authenticated caller acts as `auth.uid()`, and no identity is refused. + if auth.role() is distinct from 'service_role' and auth.role() is not null then + if auth.uid() is null then + raise exception using errcode = 'LK002', message = 'not_authenticated'; + end if; + p_user_id := auth.uid(); + end if; + + if not lorekit_api_token_scope_allowed(p_key_scopes, p_scope) then + raise exception using errcode = 'LK002', + message = format('key_scope_denied: scope=%s', p_scope); + end if; + + if p_clear_ttl then + v_ttl_action := 'clear'; + elsif p_ttl_seconds is not null then + if p_ttl_seconds < 1 or p_ttl_seconds > 31536000 then + raise exception using errcode = 'P0001', + message = format('ttl_seconds must be between 1 and 31536000, got %s', p_ttl_seconds); + end if; + v_expires_at := now() + (p_ttl_seconds * interval '1 second'); + v_ttl_action := 'set'; + end if; + + if p_org_slug is not null then + select o.id into v_org_id from orgs o where o.slug = p_org_slug and o.deleted_at is null; + if v_org_id is null then + raise exception using errcode = 'P0001', message = format('unknown_org: %s', p_org_slug); + end if; + if not lorekit_org_can(p_user_id, v_org_id, 'write') then + raise exception using errcode = 'LK002', message = format('org_permission_denied: org=%s', p_org_slug); + end if; + if not lorekit_api_token_org_allowed(p_key_org_access, p_key_org_ids, v_org_id) then + raise exception using errcode = 'LK002', + message = format('key_org_denied: org=%s', p_org_slug); + end if; + else + -- Wildcard-aware, most-specific-wins resolution. A binding matches + -- p_scope either by exact string equality, or — when it ends in '*' — as + -- an owner-wildcard prefix (the same LIKE-escaping `lorekit_api_token_ + -- scope_allowed` (00068) uses, since `%` and `\` cannot occur in a + -- pattern that already passed the grammar gate above). Among matches: + -- exact (is-wildcard = false) always sorts before any wildcard; among + -- wildcards, the LONGER literal prefix (length desc) sorts first as the + -- more specific match; the trailing `scope asc` is a determinism + -- safeguard for a tie that provably cannot occur (see the header + -- comment), not a reachable precedence branch. + select b.org_id, o.slug into v_binding_org, v_binding_slug + from org_scope_bindings b + join orgs o on o.id = b.org_id + where o.deleted_at is null + and ( + b.scope = p_scope + or ( + right(b.scope, 1) = '*' + and p_scope like replace(left(b.scope, -1), '_', '\_') || '%' + ) + ) + order by (right(b.scope, 1) = '*') asc, length(b.scope) desc, b.scope asc + limit 1; + + if v_binding_org is not null + and p_user_id is not null + and lorekit_org_can(p_user_id, v_binding_org, 'write') + and lorekit_api_token_org_allowed(p_key_org_access, p_key_org_ids, v_binding_org) then + v_org_id := v_binding_org; + end if; + end if; + + -- 00116: a PERSONAL write is gated by the key's tenancy too. Every branch + -- above either picked an org (and checked it) or left v_org_id NULL, which + -- means "write a personal row" — the one destination no tenancy used to gate, + -- because every tenancy could reach personal rows. `orgs_only` cannot, so + -- refuse instead of silently falling back to personal: a token that was + -- scoped to publish org lore must never grow its owner a personal row. + if v_org_id is null + and not lorekit_api_token_org_allowed(p_key_org_access, p_key_org_ids, null) then + raise exception using errcode = 'LK002', + message = format('key_personal_denied: this token may not write personal memories (scope=%s)', p_scope); + end if; + + -- 00119: a `supersedes` link may only name a memory with the SAME OWNER + -- as the row being written — the caller's own personal memory for a personal + -- write, a memory of the same org for an org write — inside this key's scope + -- allowlist. Same-owner rather than "any memory the caller can read" so an + -- org row never exposes the id of its writer's personal memory to every + -- other member. An unreachable id fails EXACTLY like a nonexistent one, so + -- the link cannot be used to probe for other tenants' memory ids. The + -- service-role tier (p_user_id null) may link to any existing memory. + if p_supersedes is not null and not exists ( + select 1 + from memories m + where m.id = p_supersedes + and ( + p_user_id is null + or ( + lorekit_api_token_scope_allowed(p_key_scopes, m.scope) + and case + when v_org_id is not null then m.org_id = v_org_id + else m.org_id is null and m.user_id = p_user_id + end + ) + ) + ) then + raise exception using errcode = 'P0001', + message = format('supersedes_not_found: %s', p_supersedes); + end if; + + -- 00120: optimistic concurrency. `p_expected_version` is the version the + -- caller last read; 0 means "this key must not exist yet". The live row this + -- write would upsert (the same owner + scope + key the conflict target below + -- names) is locked FOR UPDATE, so nothing can move its version between this + -- check and the upsert. A key with no live row is version 0. The `where` on + -- every `do update` below re-states the check, which is what closes the one + -- race the lock cannot: two writers that both expected 0 and both found no + -- row. The second one's insert lands on the first one's row, the `where` is + -- false, nothing is written, and it gets the same conflict. + -- + -- EXPIRED counts as absent. Every read hides a row whose `expires_at` has + -- passed, so a caller that read the key saw nothing: its version is 0 here + -- and in the `where` guards, the same liveness the reads use. The expired + -- row is still the conflict target (the unique indexes are partial on + -- `archived_at` only), so the write UPDATES it in place, and its version + -- CONTINUES counting from the expired row's (+1 on a content change) rather + -- than restarting at 1: the row keeps its id, created_at and seen_count, so + -- it is the same record coming back, and restarting would need the bump + -- trigger to second-guess liveness on every update. A caller therefore + -- expects 0 and may read back a version above 1. + -- + -- A write that names no TTL keeps the row's `expires_at`, which here is in + -- the PAST: the revived record would stay hidden and the nightly purge would + -- delete it. So when the gate passed on an expired row and the write names no + -- TTL, the TTL is cleared — what a brand-new insert without a TTL gets (null). + -- Scoped to writes that pass p_expected_version; a plain write to an expired + -- key keeps its past `expires_at` as before. + if p_expected_version is not null then + if p_expected_version < 0 then + raise exception using errcode = '22023', + message = format('expected_version must be >= 0, got %s', p_expected_version); + end if; + select case when m.expires_at is not null and m.expires_at <= now() then 0 + else m.version end, + m.expires_at is not null and m.expires_at <= now() + into v_current_version, v_was_expired + from memories m + where m.scope = p_scope + and m.key = p_key + and m.archived_at is null + and case + when v_org_id is not null then m.org_id = v_org_id + when p_user_id is null then m.org_id is null and m.user_id is null + else m.org_id is null and m.user_id = p_user_id + end + for update; + v_current_version := coalesce(v_current_version, 0); + if v_current_version <> p_expected_version then + raise exception using errcode = 'LK003', + message = format('version_conflict: expected_version=%s current_version=%s', + p_expected_version, v_current_version); + end if; + if coalesce(v_was_expired, false) and v_ttl_action = 'keep' then + v_ttl_action := 'clear'; + end if; + end if; + + if v_org_id is not null then + return query + insert into memories ( + user_id, org_id, scope, key, value, tags, source_agent, trigger, + created_at, updated_at, created_by, updated_by, expires_at, + origin_repo, origin_branch, origin_commit, origin_pr, kind, host, seen_count, + attributes, content_type, supersedes + ) + values ( + null, v_org_id, p_scope, p_key, p_value, p_tags, p_source_agent, p_trigger, + coalesce(p_created_at, now()), coalesce(p_created_at, now()), + p_user_id, p_user_id, v_expires_at, + p_origin_repo, p_origin_branch, p_origin_commit, p_origin_pr, p_kind, p_host, 1, + jsonb_strip_nulls(coalesce(p_attributes, '{}'::jsonb)), + coalesce(p_content_type, 'text/markdown'), p_supersedes + ) + on conflict (org_id, scope, key) where org_id is not null and archived_at is null + do update set + value = excluded.value, + tags = excluded.tags, + source_agent = excluded.source_agent, + trigger = excluded.trigger, + updated_at = now(), + updated_by = p_user_id, + seen_count = memories.seen_count + 1, + origin_repo = coalesce(excluded.origin_repo, memories.origin_repo), + origin_branch = coalesce(excluded.origin_branch, memories.origin_branch), + origin_commit = coalesce(excluded.origin_commit, memories.origin_commit), + origin_pr = coalesce(excluded.origin_pr, memories.origin_pr), + kind = coalesce(excluded.kind, memories.kind), + host = coalesce(excluded.host, memories.host), + attributes = jsonb_strip_nulls(memories.attributes || coalesce(p_attributes, '{}'::jsonb)), + content_type = coalesce(p_content_type, memories.content_type), + supersedes = coalesce(p_supersedes, memories.supersedes), + expires_at = case v_ttl_action + when 'clear' then null + when 'set' then v_expires_at + else memories.expires_at + end + where p_expected_version is null -- 00120: the same expired-is-0 rule as the gate + or p_expected_version = case when memories.expires_at is not null and memories.expires_at <= now() + then 0 else memories.version end + returning + memories.id, memories.created_at, (xmax::text = '0') as inserted, + true as org_routed, v_binding_slug as binding_org_slug, memories.expires_at, + memories.scope, memories.key, memories.value, memories.tags, + memories.source_agent, memories.trigger, memories.updated_at, memories.archived_at, + memories.origin_repo, memories.origin_branch, memories.origin_commit, memories.origin_pr, + memories.kind, memories.host, memories.seen_count, + memories.attributes, memories.content_type, memories.supersedes, + memories.version; -- 00120 + + elsif p_user_id is null then + return query + insert into memories ( + user_id, scope, key, value, tags, source_agent, trigger, + created_at, updated_at, created_by, updated_by, expires_at, + origin_repo, origin_branch, origin_commit, origin_pr, kind, host, seen_count, + attributes, content_type, supersedes + ) + values ( + null, p_scope, p_key, p_value, p_tags, p_source_agent, p_trigger, + coalesce(p_created_at, now()), coalesce(p_created_at, now()), + null, null, v_expires_at, + p_origin_repo, p_origin_branch, p_origin_commit, p_origin_pr, p_kind, p_host, 1, + jsonb_strip_nulls(coalesce(p_attributes, '{}'::jsonb)), + coalesce(p_content_type, 'text/markdown'), p_supersedes + ) + on conflict (scope, key) where org_id is null and user_id is null and archived_at is null + do update set + value = excluded.value, + tags = excluded.tags, + source_agent = excluded.source_agent, + trigger = excluded.trigger, + updated_at = now(), + seen_count = memories.seen_count + 1, + origin_repo = coalesce(excluded.origin_repo, memories.origin_repo), + origin_branch = coalesce(excluded.origin_branch, memories.origin_branch), + origin_commit = coalesce(excluded.origin_commit, memories.origin_commit), + origin_pr = coalesce(excluded.origin_pr, memories.origin_pr), + kind = coalesce(excluded.kind, memories.kind), + host = coalesce(excluded.host, memories.host), + attributes = jsonb_strip_nulls(memories.attributes || coalesce(p_attributes, '{}'::jsonb)), + content_type = coalesce(p_content_type, memories.content_type), + supersedes = coalesce(p_supersedes, memories.supersedes), + expires_at = case v_ttl_action + when 'clear' then null + when 'set' then v_expires_at + else memories.expires_at + end + where p_expected_version is null -- 00120: the same expired-is-0 rule as the gate + or p_expected_version = case when memories.expires_at is not null and memories.expires_at <= now() + then 0 else memories.version end + returning + memories.id, memories.created_at, (xmax::text = '0') as inserted, + false as org_routed, v_binding_slug as binding_org_slug, memories.expires_at, + memories.scope, memories.key, memories.value, memories.tags, + memories.source_agent, memories.trigger, memories.updated_at, memories.archived_at, + memories.origin_repo, memories.origin_branch, memories.origin_commit, memories.origin_pr, + memories.kind, memories.host, memories.seen_count, + memories.attributes, memories.content_type, memories.supersedes, + memories.version; -- 00120 + + else + return query + insert into memories ( + user_id, scope, key, value, tags, source_agent, trigger, + created_at, updated_at, created_by, updated_by, expires_at, + origin_repo, origin_branch, origin_commit, origin_pr, kind, host, seen_count, + attributes, content_type, supersedes + ) + values ( + p_user_id, p_scope, p_key, p_value, p_tags, p_source_agent, p_trigger, + coalesce(p_created_at, now()), coalesce(p_created_at, now()), + p_user_id, p_user_id, v_expires_at, + p_origin_repo, p_origin_branch, p_origin_commit, p_origin_pr, p_kind, p_host, 1, + jsonb_strip_nulls(coalesce(p_attributes, '{}'::jsonb)), + coalesce(p_content_type, 'text/markdown'), p_supersedes + ) + on conflict (user_id, scope, key) where org_id is null and user_id is not null and archived_at is null + do update set + value = excluded.value, + tags = excluded.tags, + source_agent = excluded.source_agent, + trigger = excluded.trigger, + updated_at = now(), + updated_by = p_user_id, + seen_count = memories.seen_count + 1, + origin_repo = coalesce(excluded.origin_repo, memories.origin_repo), + origin_branch = coalesce(excluded.origin_branch, memories.origin_branch), + origin_commit = coalesce(excluded.origin_commit, memories.origin_commit), + origin_pr = coalesce(excluded.origin_pr, memories.origin_pr), + kind = coalesce(excluded.kind, memories.kind), + host = coalesce(excluded.host, memories.host), + attributes = jsonb_strip_nulls(memories.attributes || coalesce(p_attributes, '{}'::jsonb)), + content_type = coalesce(p_content_type, memories.content_type), + supersedes = coalesce(p_supersedes, memories.supersedes), + expires_at = case v_ttl_action + when 'clear' then null + when 'set' then v_expires_at + else memories.expires_at + end + where p_expected_version is null -- 00120: the same expired-is-0 rule as the gate + or p_expected_version = case when memories.expires_at is not null and memories.expires_at <= now() + then 0 else memories.version end + returning + memories.id, memories.created_at, (xmax::text = '0') as inserted, + false as org_routed, v_binding_slug as binding_org_slug, memories.expires_at, + memories.scope, memories.key, memories.value, memories.tags, + memories.source_agent, memories.trigger, memories.updated_at, memories.archived_at, + memories.origin_repo, memories.origin_branch, memories.origin_commit, memories.origin_pr, + memories.kind, memories.host, memories.seen_count, + memories.attributes, memories.content_type, memories.supersedes, + memories.version; -- 00120 + end if; + + -- 00120: the `do update ... where` above declined the write (a concurrent + -- writer moved the version after the gate, or created the row a 0 expected + -- to be absent). Nothing was written; report the version that won. + if p_expected_version is not null and not found then + select case when m.expires_at is not null and m.expires_at <= now() then 0 + else m.version end + into v_current_version + from memories m + where m.scope = p_scope + and m.key = p_key + and m.archived_at is null + and case + when v_org_id is not null then m.org_id = v_org_id + when p_user_id is null then m.org_id is null and m.user_id is null + else m.org_id is null and m.user_id = p_user_id + end; + raise exception using errcode = 'LK003', + message = format('version_conflict: expected_version=%s current_version=%s', + p_expected_version, coalesce(v_current_version, 0)); + end if; +end; +$$; + + +revoke execute on function memory_write( + uuid, text, text, text, text[], text, text, timestamptz, text, + integer, boolean, text, text, text, integer, text, text, + jsonb, text, uuid, text[], text, uuid[], integer +) from public; +-- anon keeps 00119's grant: the body refuses it with LK002 not_authenticated. +grant execute on function memory_write( + uuid, text, text, text, text[], text, text, timestamptz, text, + integer, boolean, text, text, text, integer, text, text, + jsonb, text, uuid, text[], text, uuid[], integer +) to anon, authenticated, service_role; + +-- 4. lorekit_memory_list ───────────────────────────────────────────────────── + +drop function if exists lorekit_memory_list( + uuid, boolean, text, text, text, text, timestamptz, timestamptz, timestamptz, + timestamptz, text[], text, text[], text, text[], text, text[], text, text[], + text, text[], text, text[], text, text[], text, text[], text, text, + timestamptz, uuid, integer, text[], text, uuid[], integer, integer, integer, + integer, integer, + integer, integer, integer, integer, integer, + text[]); + +create or replace function lorekit_memory_list( + p_user_id uuid, + p_archived boolean default false, + p_scope text default null, + p_key text default null, + p_key_prefix text default null, + p_q text default null, + p_created_since timestamptz default null, + p_created_until timestamptz default null, + p_expires_after timestamptz default null, + p_expires_on_or_before timestamptz default null, + p_tags text[] default null, + p_tags_mode text default 'any', + p_source_agent text[] default null, + p_source_agent_mode text default 'in', + p_trigger text[] default null, + p_trigger_mode text default 'in', + p_kind text[] default null, + p_kind_mode text default 'in', + p_host text[] default null, + p_host_mode text default 'in', + p_origin_repo text[] default null, + p_origin_repo_mode text default 'in', + p_origin_branch text[] default null, + p_origin_branch_mode text default 'in', + p_origin_pr text[] default null, + p_origin_pr_mode text default 'in', + p_owner text[] default null, + p_owner_mode text default 'in', + p_sort text default 'updated_at', + p_cursor_ts timestamptz default null, + p_cursor_id uuid default null, + p_limit integer default 51, + p_key_scopes text[] default '{}', + p_key_org_access text default 'all', + p_key_org_ids uuid[] default '{}', + p_min_age_days integer default null, + p_unseen_days integer default null, + p_max_seen_count integer default null, + p_max_read_count integer default null, + p_max_opened_count integer default null, + -- 00113: the Explorer's reverse directions, appended and null-defaulted. + p_opened_within_days integer default null, + p_min_seen_count integer default null, + p_min_read_count integer default null, + p_min_opened_count integer default null, + -- 00113: the keyset cursor for `p_sort => 'opened_count'`. An integer sort + -- needs an integer cursor; `p_cursor_ts` keeps serving the timestamp sorts — + -- since 00115 including the nullable `last_opened_at`. + p_cursor_count integer default null, + -- 00114: labels the row must carry NONE of — an always-applied conjunct, + -- independent of p_tags/p_tags_mode, so "tagged a AND NOT tagged b" is + -- expressible. Appended, null-defaulted: null = not filtered. + p_exclude_tags text[] default null +) +returns table ( + id uuid, + scope text, + key text, + value text, + tags text[], + source_agent text, + trigger text, + created_at timestamptz, + updated_at timestamptz, + expires_at timestamptz, + archived_at timestamptz, + origin_repo text, + origin_branch text, + origin_commit text, + origin_pr integer, + kind text, + host text, + seen_count integer, + read_count integer, + opened_count integer, + last_read_at timestamptz, + last_opened_at timestamptz, + org_id uuid, + created_by uuid, + updated_by uuid, + org_name text, + org_slug text, + total_count integer, + version integer -- 00120 +) +language plpgsql +stable +security definer +set search_path = public +as $$ +#variable_conflict use_column +declare + v_actor uuid := case + when auth.role() = 'service_role' then coalesce(p_user_id, auth.uid()) + else auth.uid() + end; + v_origin_pr integer[] := ( + select array_agg(x::integer) + from unnest(coalesce(p_origin_pr, '{}'::text[])) as x + where x ~ '^0*[0-9]{1,9}$' + ); + -- 00113: `opened_count` joins the two date sorts; 00115: `last_opened_at`. + -- Anything else still falls back to `updated_at`, so an unknown value cannot + -- reach the format() below. + v_sort text := case + when p_sort in ('created_at', 'opened_count', 'last_opened_at') then p_sort else 'updated_at' + end; + -- Which bound parameter carries the keyset value: the integer cursor ($44) + -- for the count sort, the timestamp cursor ($29) for the others. A fixed + -- placeholder chosen here, never caller text, so splicing it is safe. + v_cursor text := case when v_sort = 'opened_count' then '$44' else '$29' end; + -- The keyset predicate and the ORDER BY, per sort. Built from fixed text and + -- `v_sort` (whitelisted above, quoted with %I) only. + -- + -- 00115: `last_opened_at` is NULLABLE, so it cannot use the shared shape — + -- NULLs would sort first under `desc`, and `col < cursor` is never true for + -- a NULL row, so the never-opened tail would be unreachable past page one. + -- See the header for the three cursor states. + v_keyset text := case + when v_sort = 'last_opened_at' then + $k$( + $30 is null + or case + when $29 is null then m.last_opened_at is null and m.id < $30 + else m.last_opened_at < $29 + or (m.last_opened_at = $29 and m.id < $30) + or m.last_opened_at is null + end + )$k$ + else format( + '(%2$s is null or m.%1$I < %2$s or (m.%1$I = %2$s and m.id < $30))', + v_sort, v_cursor) + end; + v_order text := case + when v_sort = 'last_opened_at' then 'm.last_opened_at desc nulls last, m.id desc' + else format('m.%I desc, m.id desc', v_sort) + end; + -- Resolved once per call so lorekit_match_retention stays immutable (00108). + v_created_cutoff timestamptz := case + when p_min_age_days is null then null else now() - (p_min_age_days * interval '1 day') + end; + v_unseen_cutoff timestamptz := case + when p_unseen_days is null then null else now() - (p_unseen_days * interval '1 day') + end; + -- 00113: the "opened within N days" lower bound, resolved once per call for + -- the same reason as the two cutoffs above. + v_opened_since timestamptz := case + when p_opened_within_days is null then null else now() - (p_opened_within_days * interval '1 day') + end; +begin + return query execute format($q$ + select + m.id, m.scope, m.key, m.value, m.tags, m.source_agent, m.trigger, + m.created_at, m.updated_at, m.expires_at, m.archived_at, + m.origin_repo, m.origin_branch, m.origin_commit, m.origin_pr, + m.kind, m.host, m.seen_count, + m.read_count, m.opened_count, m.last_read_at, m.last_opened_at, + m.org_id, m.created_by, m.updated_by, + o.name as org_name, o.slug as org_slug, + (count(*) over ())::integer as total_count, + m.version -- 00120 + from memories m + left join orgs o on o.id = m.org_id + where ( + ($1 is null and auth.role() = 'service_role') + or m.user_id = $1 + or m.org_id in (select lorekit_member_org_ids($1)) + ) + and lorekit_api_token_scope_allowed($32, m.scope) + and lorekit_api_token_org_allowed($33, $34, m.org_id) + and ( + case + when $2 then m.archived_at is not null + else m.archived_at is null + and (m.expires_at is null or m.expires_at > now()) + end + ) + and ($3 is null or m.scope = $3) + and ($4 is null or m.key = $4) + and ($5 is null or m.key ilike $5 || '%%') + and ($6 is null or m.key ilike '%%' || $6 || '%%' or m.value ilike '%%' || $6 || '%%') + and ($7 is null or m.created_at >= $7) + and ($8 is null or m.created_at < $8) + and ($9 is null or m.expires_at > $9) + and ($10 is null or m.expires_at <= $10) + and lorekit_match_tags(m.tags, $11, $12) + and lorekit_match_tags(m.tags, $45, 'none') + and lorekit_match_text(m.source_agent, $13, $14) + and lorekit_match_text(m.trigger, $15, $16) + and lorekit_match_text(m.kind, $17, $18) + and lorekit_match_text(m.host, $19, $20) + and lorekit_match_text(m.origin_repo, $21, $22) + and lorekit_match_text(m.origin_branch, $23, $24) + and lorekit_match_int (m.origin_pr, $25, $26) + and ($27 is null or case coalesce($28, 'in') + when 'nin' then ( + (case when m.org_id is null then 'personal' else o.slug end) is not null + and (case when m.org_id is null then 'personal' else o.slug end) <> all($27) + ) + else ( + ('personal' = any($27) and m.org_id is null) + or (m.org_id is not null and o.slug = any($27)) + ) + end) + and lorekit_match_retention( + m.created_at, m.last_opened_at, m.seen_count, m.read_count, m.opened_count, + $35, $36, $37, $38, $39, + $40, $41, $42, $43 + ) + and %1$s + order by %2$s + limit $31 + $q$, v_keyset, v_order) + using + v_actor, coalesce(p_archived, false), p_scope, p_key, p_key_prefix, p_q, + p_created_since, p_created_until, p_expires_after, p_expires_on_or_before, + p_tags, p_tags_mode, + p_source_agent, p_source_agent_mode, + p_trigger, p_trigger_mode, + p_kind, p_kind_mode, + p_host, p_host_mode, + p_origin_repo, p_origin_repo_mode, + p_origin_branch, p_origin_branch_mode, + v_origin_pr, p_origin_pr_mode, + p_owner, p_owner_mode, + p_cursor_ts, p_cursor_id, + greatest(coalesce(p_limit, 51), 1), + coalesce(p_key_scopes, '{}'::text[]), + coalesce(p_key_org_access, 'all'), + coalesce(p_key_org_ids, '{}'::uuid[]), + v_created_cutoff, v_unseen_cutoff, p_max_seen_count, p_max_read_count, + p_max_opened_count, + v_opened_since, p_min_seen_count, p_min_read_count, p_min_opened_count, + p_cursor_count, + p_exclude_tags; +end; +$$; + +revoke execute on function lorekit_memory_list( + uuid, boolean, text, text, text, text, timestamptz, timestamptz, timestamptz, + timestamptz, text[], text, text[], text, text[], text, text[], text, text[], + text, text[], text, text[], text, text[], text, text[], text, text, + timestamptz, uuid, integer, text[], text, uuid[], integer, integer, integer, + integer, integer, + integer, integer, integer, integer, integer, + text[]) from public, anon; +grant execute on function lorekit_memory_list( + uuid, boolean, text, text, text, text, timestamptz, timestamptz, timestamptz, + timestamptz, text[], text, text[], text, text[], text, text[], text, text[], + text, text[], text, text[], text, text[], text, text[], text, text, + timestamptz, uuid, integer, text[], text, uuid[], integer, integer, integer, + integer, integer, + integer, integer, integer, integer, integer, + text[]) to authenticated, service_role; + +comment on function lorekit_memory_list( + uuid, boolean, text, text, text, text, timestamptz, timestamptz, timestamptz, + timestamptz, text[], text, text[], text, text[], text, text[], text, text[], + text, text[], text, text[], text, text[], text, text[], text, text, + timestamptz, uuid, integer, text[], text, uuid[], integer, integer, integer, + integer, integer, + integer, integer, integer, integer, integer, + text[]) is + 'The keyset-paginated memory list behind GET /memories and POST /memories/list. + Sorts by updated_at (default), created_at, opened_count (00113) or + last_opened_at (00115) — each descending with id as the tie-break; + last_opened_at is NULLS LAST, so never-opened lessons trail. p_cursor_ts + carries the timestamp sorts'' keyset value (NULL under last_opened_at with a + set p_cursor_id = inside the never-opened tail) and p_cursor_count the count + sort''s. Applies the five retention thresholds and the Explorer''s reverse + directions through the shared lorekit_match_retention, so its total_count + agrees with _facets, _activity and _pivot under the same parameters. + p_exclude_tags (00114): the row carries none of these labels, ANDed with + whatever p_tags/p_tags_mode ask.'; diff --git a/supabase/migrations/00121_validate_memory_version_check.sql b/supabase/migrations/00121_validate_memory_version_check.sql new file mode 100644 index 000000000..7d7921df8 --- /dev/null +++ b/supabase/migrations/00121_validate_memory_version_check.sql @@ -0,0 +1,11 @@ +-- 00121 — validate memories_version_positive (added NOT VALID by 00120). +-- +-- A separate file because each migration runs in ONE transaction: validating in +-- 00120 would scan every row while that transaction still held the ACCESS +-- EXCLUSIVE lock its `add column` took, blocking reads and writes for the scan. +-- In its own transaction, VALIDATE CONSTRAINT takes only SHARE UPDATE EXCLUSIVE, +-- so reads and writes proceed while it scans. It cannot fail: every pre-00120 +-- row reads `version` = 1 from the column default, and every row since has been +-- checked on write. + +alter table memories validate constraint memories_version_positive; diff --git a/supabase/tests/migrations.test.sql b/supabase/tests/migrations.test.sql index b8a5f1bd4..3f361d345 100644 --- a/supabase/tests/migrations.test.sql +++ b/supabase/tests/migrations.test.sql @@ -58,6 +58,9 @@ values -- §119's own account, plus the foreign tenant its probe assertion needs. ('00000000-0000-0000-0000-000000000000', '00000000-0000-0000-0000-0000000000ac', 'authenticated', 'authenticated', 'lk-mig-ac@test.local', now(), now()), ('00000000-0000-0000-0000-000000000000', '00000000-0000-0000-0000-0000000000ad', 'authenticated', 'authenticated', 'lk-mig-ad@test.local', now(), now()), + -- §120's own account, plus the foreign tenant its visibility assertion needs. + ('00000000-0000-0000-0000-000000000000', '00000000-0000-0000-0000-0000000000ae', 'authenticated', 'authenticated', 'lk-mig-ae@test.local', now(), now()), + ('00000000-0000-0000-0000-000000000000', '00000000-0000-0000-0000-0000000000af', 'authenticated', 'authenticated', 'lk-mig-af@test.local', now(), now()), ('00000000-0000-0000-0000-000000000000', '00000000-0000-0000-0000-0000000000b2', 'authenticated', 'authenticated', 'lk-mig-b@test.local', now(), now()), ('00000000-0000-0000-0000-000000000000', '00000000-0000-0000-0000-0000000000c3', 'authenticated', 'authenticated', 'lk-mig-c@test.local', now(), now()), ('00000000-0000-0000-0000-000000000000', '00000000-0000-0000-0000-0000000000d4', 'authenticated', 'authenticated', 'lk-mig-d@test.local', now(), now()), @@ -3523,8 +3526,9 @@ do $$ declare -- 00069 appended the key restriction (p_key_scopes, p_key_org_access, -- p_key_org_ids → 20 args); 00119 inserted p_attributes, p_content_type and - -- p_supersedes after p_host (23 args). - v_sig text := 'memory_write(uuid, text, text, text, text[], text, text, timestamp with time zone, text, integer, boolean, text, text, text, integer, text, text, jsonb, text, uuid, text[], text, uuid[])'; + -- p_supersedes after p_host (23 args); 00120 appended p_expected_version + -- (24 args). + v_sig text := 'memory_write(uuid, text, text, text, text[], text, text, timestamp with time zone, text, integer, boolean, text, text, text, integer, text, text, jsonb, text, uuid, text[], text, uuid[], integer)'; begin assert has_function_privilege('anon', v_sig, 'EXECUTE'), 'origin: anon must have EXECUTE on the widened memory_write'; @@ -12031,6 +12035,273 @@ end; $$; +-- ═══════════════════════════════════════════════════════════════════════════ +-- §120 — 00120: `version` on every memory, and `p_expected_version` +-- +-- version starts at 1 (existing rows included), moves +1 on a CONTENT change +-- only, cannot be set by a caller, and memory_write refuses a stale +-- p_expected_version with LK003 version_conflict without writing anything. +-- `superseded_by` is computed by the edge from `supersedes`; the visibility +-- half of it is the RLS read asserted at the end. +-- ═══════════════════════════════════════════════════════════════════════════ +do $$ +declare + v_me constant uuid := '00000000-0000-0000-0000-0000000000ae'; + v_other constant uuid := '00000000-0000-0000-0000-0000000000af'; + v_id uuid; + v_old uuid; + v_v integer; + v_val text; + v_err text; + v_keys text[]; +begin + -- ── backfill: the column is NOT NULL DEFAULT 1, so pre-00120 rows read 1 ─ + assert (select is_nullable = 'NO' and column_default = '1' + from information_schema.columns + where table_name = 'memories' and column_name = 'version'), + '120 AC-1a: memories.version must be NOT NULL DEFAULT 1'; + -- §1's rows were inserted before any section edited them; a row whose + -- content no section changed reads exactly 1, and no row reads below it. + assert (select version from memories where key = 'rls-a') = 1, + '120 AC-1b: an untouched row must read version 1'; + assert not exists (select 1 from memories where version < 1), + '120 AC-1b: no row may read below version 1'; + -- added NOT VALID by 00120, validated by 00121: it must end VALID + assert (select convalidated from pg_constraint + where conname = 'memories_version_positive' and conrelid = 'memories'::regclass), + '120 AC-1r: memories_version_positive must be validated (00121) after its NOT VALID add (00120)'; + + -- ── a write starts at 1 and returns it ─────────────────────────────────── + select id, version into v_id, v_v + from memory_write(p_user_id => v_me, p_scope => 'global', p_key => '120-a', p_value => 'one', + p_tags => array['t1']); + assert v_v = 1, format('120 AC-1c: a new memory must start at version 1, got %s', v_v); + + -- ── an identical rewrite, a TTL change and a re-sighting do not bump ──── + select version into v_v + from memory_write(p_user_id => v_me, p_scope => 'global', p_key => '120-a', p_value => 'one', + p_tags => array['t1'], p_ttl_seconds => 3600); + assert v_v = 1, format('120 AC-1d: identical content with a new TTL must stay at 1, got %s', v_v); + assert (select seen_count from memories where id = v_id) = 2, + '120 AC-1d: the rewrite must still count as a sighting'; + + -- ── a value change bumps ───────────────────────────────────────────────── + select version into v_v + from memory_write(p_user_id => v_me, p_scope => 'global', p_key => '120-a', p_value => 'two', + p_tags => array['t1']); + assert v_v = 2, format('120 AC-1e: a value change must bump to 2, got %s', v_v); + + -- each content field bumps on its own + select version into v_v from memory_write(p_user_id => v_me, p_scope => 'global', p_key => '120-a', + p_value => 'two', p_tags => array['t1', 't2']); + assert v_v = 3, format('120 AC-1f: a tags change must bump, got %s', v_v); + select version into v_v from memory_write(p_user_id => v_me, p_scope => 'global', p_key => '120-a', + p_value => 'two', p_tags => array['t1', 't2'], p_attributes => '{"ci.run": 1}'::jsonb); + assert v_v = 4, format('120 AC-1g: an attributes change must bump, got %s', v_v); + select version into v_v from memory_write(p_user_id => v_me, p_scope => 'global', p_key => '120-a', + p_value => 'two', p_tags => array['t1', 't2'], p_attributes => '{"ci.run": 1}'::jsonb); + assert v_v = 4, format('120 AC-1h: re-sending the same attributes must not bump, got %s', v_v); + select version into v_v from memory_write(p_user_id => v_me, p_scope => 'global', p_key => '120-a', + p_value => '{"a": 1}', p_tags => array['t1', 't2'], p_content_type => 'application/json'); + assert v_v = 5, format('120 AC-1i: a value + content_type change must bump once, got %s', v_v); + + -- ── counters, embeddings, archive and restore do not bump ─────────────── + update memories set read_count = read_count + 1, last_read_at = now(), + opened_count = opened_count + 1, cited_count = cited_count + 1 + where id = v_id; + update memories set expires_at = now() + interval '1 day' where id = v_id; + update memories set archived_at = now() where id = v_id; + update memories set archived_at = null where id = v_id; + assert (select version from memories where id = v_id) = 5, + format('120 AC-1j: counters, TTL, archive and restore must not bump, got %s', + (select version from memories where id = v_id)); + + -- ── a caller cannot set version ───────────────────────────────────────── + update memories set version = 99 where id = v_id; + assert (select version from memories where id = v_id) = 5, + '120 AC-1k: a direct UPDATE of version alone must be overridden'; + update memories set version = 99, value = '{"a": 2}' where id = v_id; + assert (select version from memories where id = v_id) = 6, + '120 AC-1l: a direct UPDATE that changes content gets old + 1, never the stated version'; + insert into memories (user_id, scope, key, value, version) + values (v_me, 'global', '120-direct', 'v', 42) returning version into v_v; + assert v_v = 1, format('120 AC-1m: a direct INSERT must start at 1, got %s', v_v); + begin + update memories set version = 0 where id = v_id; + v_err := null; + exception when others then v_err := sqlerrm; + end; + assert (select version from memories where id = v_id) = 6 and v_err is null, + '120 AC-1n: a direct UPDATE to 0 is overridden, not an error'; + + -- ── supersedes: setting bumps; the target's deletion clears without a bump + select id into v_old + from memory_write(p_user_id => v_me, p_scope => 'global', p_key => '120-old', p_value => 'v'); + select version into v_v from memory_write(p_user_id => v_me, p_scope => 'global', p_key => '120-a', + p_value => '{"a": 2}', p_tags => array['t1', 't2'], p_supersedes => v_old); + assert v_v = 7, format('120 AC-1o: setting supersedes must bump, got %s', v_v); + delete from memories where id = v_old; + assert (select supersedes is null and version = 7 from memories where id = v_id), + '120 AC-1p: a link cleared by its target''s deletion must not bump'; + + -- ── p_expected_version ────────────────────────────────────────────────── + -- match → written, bumped + select version, value into v_v, v_val from memory_write(p_user_id => v_me, p_scope => 'global', + p_key => '120-a', p_value => '{"a": 3}', p_tags => array['t1', 't2'], p_expected_version => 7); + assert v_v = 8 and v_val = '{"a": 3}', format('120 AC-2a: a matching expected_version must write, got %s', v_v); + + -- mismatch → LK003 version_conflict naming the stored version, nothing written + begin + perform memory_write(p_user_id => v_me, p_scope => 'global', p_key => '120-a', + p_value => '{"a": 4}', p_expected_version => 7); + v_err := null; + exception when others then v_err := sqlstate || ' ' || sqlerrm; + end; + assert v_err = 'LK003 version_conflict: expected_version=7 current_version=8', + format('120 AC-2b: a stale expected_version must raise version_conflict, got %L', v_err); + assert (select value = '{"a": 3}' and version = 8 and seen_count = 9 from memories where id = v_id), + '120 AC-2c: a refused write must change nothing (value, version, seen_count)'; + + -- 0 on an existing key → conflict + begin + perform memory_write(p_user_id => v_me, p_scope => 'global', p_key => '120-a', + p_value => 'x', p_expected_version => 0); + v_err := null; + exception when others then v_err := sqlstate || ' ' || sqlerrm; + end; + assert v_err = 'LK003 version_conflict: expected_version=0 current_version=8', + format('120 AC-2d: expected 0 on an existing key must conflict, got %L', v_err); + + -- 0 on an absent key → created + select version into v_v from memory_write(p_user_id => v_me, p_scope => 'global', + p_key => '120-new', p_value => 'v', p_expected_version => 0); + assert v_v = 1, format('120 AC-2e: expected 0 on an absent key must create it, got %s', v_v); + + -- N > 0 on an absent key → conflict at 0, no row + begin + perform memory_write(p_user_id => v_me, p_scope => 'global', p_key => '120-absent', + p_value => 'v', p_expected_version => 3); + v_err := null; + exception when others then v_err := sqlstate || ' ' || sqlerrm; + end; + assert v_err = 'LK003 version_conflict: expected_version=3 current_version=0', + format('120 AC-2f: expected 3 on an absent key must conflict at 0, got %L', v_err); + assert not exists (select 1 from memories where key = '120-absent'), + '120 AC-2g: a refused create must leave no row'; + + -- an archived row is not the live record: expected 0 revives the key as new + update memories set archived_at = now() where key = '120-new' and user_id = v_me; + select version into v_v from memory_write(p_user_id => v_me, p_scope => 'global', + p_key => '120-new', p_value => 'again', p_expected_version => 0); + assert v_v = 1, format('120 AC-2h: an archived key counts as absent, got %s', v_v); + + -- the gate is per owner: another user's row of the same key is not "the" row + perform memory_write(p_user_id => v_other, p_scope => 'global', p_key => '120-shared', p_value => 'theirs'); + select version into v_v from memory_write(p_user_id => v_me, p_scope => 'global', + p_key => '120-shared', p_value => 'mine', p_expected_version => 0); + assert v_v = 1, format('120 AC-2i: another owner''s row must not count against expected 0, got %s', v_v); + + begin + perform memory_write(p_user_id => v_me, p_scope => 'global', p_key => '120-neg', + p_value => 'v', p_expected_version => -1); + v_err := null; + exception when others then v_err := sqlstate; + end; + assert v_err = '22023', format('120 AC-2j: a negative expected_version must be rejected, got %L', v_err); + + -- an EXPIRED row is not read, so it counts as absent (version 0) — the same + -- liveness the reads use. The write updates that row in place and its + -- version continues counting (not a restart at 1). + perform memory_write(p_user_id => v_me, p_scope => 'global', p_key => '120-exp', p_value => 'one'); + perform memory_write(p_user_id => v_me, p_scope => 'global', p_key => '120-exp', p_value => 'two'); + update memories set expires_at = now() - interval '1 minute' where key = '120-exp' and user_id = v_me; + begin + perform memory_write(p_user_id => v_me, p_scope => 'global', p_key => '120-exp', + p_value => 'x', p_expected_version => 2); + v_err := null; + exception when others then v_err := sqlstate || ' ' || sqlerrm; + end; + assert v_err = 'LK003 version_conflict: expected_version=2 current_version=0', + format('120 AC-2k: an expired row must read as version 0 to the gate, got %L', v_err); + select version, value into v_v, v_val from memory_write(p_user_id => v_me, p_scope => 'global', + p_key => '120-exp', p_value => 'three', p_expected_version => 0, p_clear_ttl => true); + assert v_v = 3 and v_val = 'three', + format('120 AC-2l: expected 0 on an expired key must write and continue the version, got %s %L', v_v, v_val); + assert (select count(*) = 1 from memories where key = '120-exp' and user_id = v_me), + '120 AC-2m: the expired row must be updated in place, not duplicated'; + -- revived (TTL cleared), it is live again: 0 now conflicts at its version + begin + perform memory_write(p_user_id => v_me, p_scope => 'global', p_key => '120-exp', + p_value => 'y', p_expected_version => 0); + v_err := null; + exception when others then v_err := sqlstate || ' ' || sqlerrm; + end; + assert v_err = 'LK003 version_conflict: expected_version=0 current_version=3', + format('120 AC-2n: a live row must conflict against expected 0, got %L', v_err); + + -- an expired row revived by an expected_version write that names NO TTL + -- must come back live: keeping its past expires_at would leave it hidden + -- and purgeable. Its TTL is cleared, as a fresh insert without one gets. + perform memory_write(p_user_id => v_me, p_scope => 'global', p_key => '120-exp2', p_value => 'one'); + update memories set expires_at = now() - interval '1 minute' where key = '120-exp2' and user_id = v_me; + select version, value into v_v, v_val from memory_write(p_user_id => v_me, p_scope => 'global', + p_key => '120-exp2', p_value => 'two', p_expected_version => 0); + assert v_v = 2 and v_val = 'two', + format('120 AC-2o: expected 0 on an expired key without a TTL must write, got %s %L', v_v, v_val); + assert (select expires_at is null from memories where key = '120-exp2' and user_id = v_me), + '120 AC-2o: a revived expired row with no TTL named must have its expires_at cleared'; + -- an explicit TTL on the revive is honoured, not cleared + update memories set expires_at = now() - interval '1 minute' where key = '120-exp2' and user_id = v_me; + perform memory_write(p_user_id => v_me, p_scope => 'global', p_key => '120-exp2', + p_value => 'three', p_expected_version => 0, p_ttl_seconds => 3600); + assert (select expires_at = now() + interval '1 hour' from memories where key = '120-exp2' and user_id = v_me), + '120 AC-2p: a revive that names a TTL must keep that TTL'; + -- scope of the fix: a write with no expected_version keeps the past TTL as before + update memories set expires_at = now() - interval '1 minute' where key = '120-exp2' and user_id = v_me; + perform memory_write(p_user_id => v_me, p_scope => 'global', p_key => '120-exp2', p_value => 'four'); + assert (select expires_at < now() from memories where key = '120-exp2' and user_id = v_me), + '120 AC-2q: a plain write without expected_version must keep expires_at unchanged'; + + -- ── lorekit_memory_list returns version ───────────────────────────────── + set local role service_role; + perform set_config('request.jwt.claims', '{"role":"service_role"}', true); + select l.version into v_v from lorekit_memory_list(p_user_id => v_me, p_key => '120-a') l; + reset role; + perform set_config('request.jwt.claims', '', true); + assert v_v = 8, format('120 AC-1q: lorekit_memory_list must return version, got %s', v_v); + + -- ── superseded_by visibility: successors are read under the caller's RLS ─ + -- The edge computes `superseded_by` as "live rows whose supersedes is this + -- id", read under the same tenant filter as the read itself. A successor in + -- another tenant (only the service tier can create one), an archived one and + -- an expired one must all stay invisible. + insert into memories (user_id, scope, key, value) values (v_me, 'global', '120-target', 'v') + returning id into v_old; + insert into memories (user_id, scope, key, value, supersedes) values + (v_me, 'global', '120-succ-mine', 'v', v_old), + (v_other, 'global', '120-succ-foreign', 'v', v_old); + insert into memories (user_id, scope, key, value, supersedes, archived_at) values + (v_me, 'global', '120-succ-archived', 'v', v_old, now()); + insert into memories (user_id, scope, key, value, supersedes, expires_at) values + (v_me, 'global', '120-succ-expired', 'v', v_old, now() - interval '1 minute'); + set local role authenticated; + perform set_config('request.jwt.claims', format('{"sub":"%s","role":"authenticated"}', v_me), true); + select array_agg(m.key order by m.key) into v_keys + from memories m + where m.supersedes = v_old + and m.archived_at is null + and (m.expires_at is null or m.expires_at > now()); + reset role; + perform set_config('request.jwt.claims', '', true); + assert v_keys = array['120-succ-mine'], + format('120 AC-3a: only the caller''s own live successor may be visible, got %s', v_keys); + -- and the foreign successor really exists, so the assertion above can fail + assert exists (select 1 from memories where key = '120-succ-foreign' and supersedes = v_old), + '120 AC-3b: the foreign successor must exist for AC-3a to mean anything'; +end; +$$; + + rollback; \echo 'migrations.test.sql: all assertions passed'