feat(memory): the memory_relations table behind memory_link (step 1 of the graph)

[Eldad-Caura]

What, and what this deliberately is not

memory_link promises a typed relation between two memory ids, and nothing in the schema could hold one — relations is entity↔entity, memory_entity_links is memory↔entity, and memories.supersedes_id is memory→memory but single-valued and written only by the contradiction detector. That is why the tool has been accepted-but-unadvertised since memclawd #130.

This adds the store and nothing else. Nothing consumes it yet: no storage endpoint, no core-api route, no cloud.Client method, no dispatcher wiring, and memory_link stays unadvertised. memclawd's TestAdvertisedToolsAreAllServable enforces that pairing, so the cloud side must exist before the tool can be re-advertised — this is the prerequisite, landed on its own rather than as part of a cross-repo change.

Design, decided rather than inferred

The six relation types are contracts/mcp-tools.md §8 verbatim; Eldad confirmed the set rather than letting it be read off a tool description.

supersedes is not stored here. It reuses memories.supersedes_id so the detector and the API write one field instead of two stores that can disagree. A CHECK constraint makes that structural — a supersedes row cannot be inserted at all. The consequence is a real asymmetry the tool will have to document: supersedes is 1:1 and a second one overwrites, while a second elaborates is an additional row.

One directed row per link, never two. Three of the five stored types are semantically symmetric, and writing both directions creates pairs that must stay in sync plus half-edges when one is deleted. Symmetry is a read concern, and all per-type knowledge lives in one constant (SYMMETRIC_RELATION_TYPES) instead of spreading into the schema.

Soft delete leaves the rows. memories is soft-deleted routinely and that is reversible, so cascading would make un-delete lossy. The read path filters deleted_at IS NULL — free, since it already joins memories for endpoint content — and ON DELETE CASCADE covers the terminal case when the purge sweep hard-deletes the row. No new retention machinery.

Verified rather than asserted

Model and migration produce byte-identical schemas. Applied 037 to a scratch database and diffed \d memory_relations against the create_all result: identical across columns, defaults, all four indexes, both CHECKs, and both CASCADE FKs. Tests build the schema with create_all while prod runs alembic, so a divergence there stays invisible until production.

Seven tests exercise the guarantees against a real database — a comment claiming a constraint exists is worth nothing until something has watched it refuse a write:

• supersedes rejected by CHECK
• self-link rejected by CHECK
• duplicate (tenant, from, type, to) rejected by the natural key
• hard-deleting an endpoint cascades the link away
• soft-deleting an endpoint does not

One test pins a gap on purpose: the reversed pair of a symmetric type is accepted by the natural key. That is exactly why the write path will have to check it — the schema does not make symmetry idempotent, and this documents that where someone would otherwise assume it does. If a later change makes the schema enforce it, that test should fail and be replaced rather than deleted.

Notes on the migration

Indexes are plain, not CONCURRENTLY: the table is created in the same migration, so it is empty and unlocked. test_no_plain_create_index_on_large_tables scopes its requirement to large pre-existing tables — the case that crashed six storage-writer boots on 2026-06-16.

Both endpoint columns get their own index because the natural key leads with tenant_id and therefore serves neither as a prefix. test_every_fk_referencing_column_is_index_leading fails at PR time otherwise, and an unindexed FK referencing column is what cost 15.3 s of a 15.5 s bulk delete before migration 035.

test_single_head expectation moved 036 → 037.

Measurement

	result
tests/	4382 → 4389 (+7, exactly the new tests)
core-storage-api/tests/	118 on a fresh database
mypy	clean on both src/ trees
ruff check / format	clean at CI's scopes

One local-only wrinkle worth recording: running core-storage-api/tests/ against a database where the root tests/ tree had already run create_all gives 106 DuplicateTableErrors, because create_all made memory_relations while alembic_version still said 036. It is not reachable in CI — that job gets its own fresh Postgres service, and CI runs alembic upgrade head before pytest tests/, so alembic is always ahead. Flagging it so nobody debugs it as a code defect.

Remaining chain for memory_link

1. storage-api endpoint (upsert + list), with the reversed-pair check for symmetric types
2. core-api route
3. supersedes path — PATCH /memories/{id}/status already has the full authz stack and already reaches a storage layer supporting supersedes_id / unset_supersedes / expected_supersedes_id; it simply forwards none of them. Use the CAS gate so an API write cannot clobber the detector's pointer.
4. memclawd: cloud.Client method, CloudLinker capability, newCloudDispatcher wiring, a real input schema, then re-advertise in tools/list
5. a BROKER_OPERATIONS row here

[github-actions]

🤖 Review by Claude Code

Summary

This PR adds a new memory_relations table (model + migration) backing a memory_link tool, with well-documented design rationale for routing supersedes elsewhere, storing symmetric relations as single directed rows, and leaving soft-deleted endpoints untouched. The schema and migration are consistent with each other and reasonably indexed for the FK columns. Two non-blocking issues stood out: the lack of a DB-level constraint enumerating valid relation_type values, and a redundant single-column index that duplicates the leading column of the unique constraint's index.

Medium/Low Issues

No constraint restricting relation_type to the documented vocabulary

Severity: Medium
File: common/models/memory_relation.py:95-129, core-storage-api/src/core_storage_api/database/migrations/versions/037_memory_relations.py:38-70
Problem: The only relation_type-related CHECK excludes the literal 'supersedes' string, but nothing in the schema constrains relation_type to the five stored values in STORED_RELATION_TYPES (elaborates, contradicts, depends_on, alternative_to, related_to), so an application bug or direct insert can persist an arbitrary/typo'd relation type, undermining the module's stated goal of making these guarantees "structural rather than a convention."

🤖 Claude Code Prompt

In common/models/memory_relation.py, add a CHECK constraint (and mirror it in
core-storage-api/src/core_storage_api/database/migrations/versions/037_memory_relations.py)
that restricts `relation_type` to the values in STORED_RELATION_TYPES
(elaborates, contradicts, depends_on, alternative_to, related_to) in addition to the
existing `relation_type <> 'supersedes'` check — e.g.
CheckConstraint("relation_type IN ('elaborates','contradicts','depends_on',
'alternative_to','related_to')", name="ck_memory_relations_valid_type").
This closes the gap where an arbitrary string can currently be persisted as
relation_type, which contradicts the module's stated design goal that these
invariants be enforced at the schema level rather than relying on service-layer
validation.

---

Redundant standalone index on tenant_id

Severity: Low
File: common/models/memory_relation.py:117-127, core-storage-api/src/core_storage_api/database/migrations/versions/037_memory_relations.py:75-78
Problem: ix_memory_relations_tenant indexes tenant_id alone, but the unique constraint uq_memory_relations_natural_key already leads with tenant_id, so its backing btree index already serves tenant-only lookups as a prefix — the extra index adds write overhead and storage with no query benefit.

🤖 Claude Code Prompt

In common/models/memory_relation.py (the __table_args__ tuple) and
core-storage-api/src/core_storage_api/database/migrations/versions/037_memory_relations.py
(op.create_index("ix_memory_relations_tenant", ...)), remove the standalone
ix_memory_relations_tenant index. The unique constraint
uq_memory_relations_natural_key already begins with tenant_id, so its underlying
btree index can already serve tenant-only lookups via leftmost-prefix matching,
making the separate single-column index redundant write/storage overhead. Update
the accompanying comment that justifies the index so it no longer claims a
standalone tenant index is needed.

---

Reviewed by claude-sonnet-5 · cost $0.20135725