Skip to content

fix(store): enforce same-KB provenance references - #832

Merged
WaylandYang merged 3 commits into
deeplethe:devfrom
ansonnmm:upstream-pr-provenance-schema-invariant
Sep 21, 2026
Merged

WaylandYang merged 3 commits into
deeplethe:devfrom
ansonnmm:upstream-pr-provenance-schema-invariant

Conversation

@ansonnmm

@ansonnmm ansonnmm commented Sep 20, 2026

Copy link
Copy Markdown
Contributor

PROBLEM

Provenance-bearing rows can reference rows that live in a different knowledge base. A column foreign key proves the target exists — not that it is the same KB's. As a result an export can mint a local IRI that names another KB's row, or silently drop a vocabulary reference that resolves to nothing.

WHY THIS IS A GENERIC UTOPIA BUG OR CONTRACT GAP

The schema enforces per-column FK existence but never same-KB ownership on provenance edges. Any write path that reaches the tables directly — including restores from older backups or SQL that bypasses the store layer — can persist cross-KB references that no later check catches.

FIX

A new migration installs the invariant at three layers:

  • a precondition scan that refuses to migrate a ledger already holding cross-KB rows (atomic; the error names the offending edge),
  • composite (kb_id, ref) foreign keys on the 26 edges whose row carries its own kb_idDEFERRABLE INITIALLY DEFERRED on the four same-table self-references so COPY batches and multi-row inserts are judged at commit — and row triggers on the 13 edges whose KB authority is a parent row with no kb_id column to key on,
  • kb-ownership immutability on every owned table.

Design record: docs/decisions/0048; mechanism question open as #842.

REGRESSION EVIDENCE

Three new database test files cover: clean/dirty-ledger migration (atomic refusal, nothing installed), forward references via multi-row INSERT, COPY, UPDATE and deferred commit, from_statement edges, relation self-links, and installation under normal/empty/hostile search_path. A catalog-derived completeness guard in the same file enumerates every column-level reference inside the ledger surface from pg_catalog and fails on any edge that resolves to no declared composite-FK, owner-derived trigger, or explicit exclusion. 14/14 pass on PostgreSQL 16; the full utopia-store suite is green. Write-path cost measured on bench_100k (UTOPIA_BENCH_DOCS=1000, UTOPIA_BENCH_HUB_FACTS=1000, alternating runs, same host / PG16 / toolchain): populate median 9.35s patched vs 9.77s base — within noise. The trigger-covered edges that path never touches measured separately: ~+6–7µs per row per edge (4000-row bulk inserts — fact_evidence 63.7ms vs 37.1ms, typed_fact_sources 58.1ms vs 34.3ms).

COMPATIBILITY RISK

The migration fails closed on ledgers that already contain cross-KB rows — such installs must repair the data before upgrading, and the error tells them where. The §0 scan doubles as the pre-upgrade audit to run against real deployments (recorded in 0048). Clean ledgers pay ~6–7µs/row on the 13 trigger-covered edges and no measurable cost on the populate path.

@ansonnmm

Copy link
Copy Markdown
Contributor Author

Closing pending author's final review of the PR set — will reopen once the series is finalized.

@ansonnmm ansonnmm closed this Sep 20, 2026
@ansonnmm ansonnmm changed the title Provenance never points across a knowledge base fix(store): enforce same-KB provenance references Sep 20, 2026
@ansonnmm ansonnmm reopened this Sep 20, 2026
@WaylandYang

Copy link
Copy Markdown
Contributor

I've approved the CI run — fork PRs need that here.

The analysis behind this is strong, and three parts in particular are the work of someone who has been bitten before: enumerating every provenance edge with the row whose kb_id is authoritative; pinning search_path in every function body and qualifying the DDL as public.* because pg_restore empties the session search_path; and using DEFERRABLE INITIALLY DEFERRED constraint triggers for same-table self-references, since a row-level BEFORE trigger cannot see a sibling inserted later in the same transaction. The migration_0070_runs_under_any_search_path test earns its place.

Four things before this can land.

1. DCO is failing. The commit carries no Signed-off-by. Please amend with -s and force-push.

2. This needs an issue and a decision record first. The convention in this repo is that a change touching the data model opens an issue and waits, and that design reasoning lives in docs/decisions/ as a numbered record. This installs a permanent schema-wide invariant across every owned table and makes states that were previously writable impossible — that is a design decision, not only a bug fix. The record is also the right home for the reasoning that is currently in the migration's header comment, which is good enough to deserve somewhere more durable than one SQL file.

3. The write-path cost is asserted rather than measured. 38 triggers and 23 functions sit on provenance writes, and extraction writes facts, evidence and chunks continuously. "Clean ledgers see only trigger overhead" is the claim that needs a number here — a before/after on the extraction path, or on the existing 100k bench. This repo settles this kind of question by measuring, and a permanent write-path tax is exactly the kind that should not go in on an estimate.

4. Please record why triggers rather than composite foreign keys. FOREIGN KEY (kb_id, target_id) REFERENCES target (kb_id, id) enforces the same invariant declaratively, with no PL/pgSQL on the write path. I can see it is not uniformly available — attribute_rule_conditions has no kb_id column of its own, as your header notes, and several edges take their owner's kb_id rather than their own — so composite keys would mean adding columns in some places. That may well be the right trade, but the answer belongs in the record rather than being inferred by the next reader.

One operational note for whoever merges this: §0 fails the migration closed on a ledger that already holds cross-KB rows. That is the correct behaviour, but it means the precondition scan should be run against real bases before this lands, so an upgrade does not stop halfway on a base nobody had checked.

@ansonnmm

Copy link
Copy Markdown
Contributor Author

All four points addressed in the pushed update (71373e7 + de4524f, rebased onto current dev):

  1. DCO — both commits now carry Signed-off-by, matching the author identity.
  2. Issue + record — design question opened as Same-KB provenance references: choosing the enforcement mechanism #842 (it asks, rather than presumes, whether maintainers want the hybrid split); reasoning recorded in docs/decisions/0048-provenance-references-stay-inside-the-knowledge-base.md.
  3. Measured write-path costbench_100k populate, alternating runs on the same host/PG16/toolchain: median 9.35s patched vs 9.77s base (docs=1000, hub_facts=1000; ranges overlap, no measurable overhead). The edges that path doesn't touch measured separately: ~+6–7µs/row on triggered-edge bulk inserts. Details in the record.
  4. Triggers vs composite FKs — settled per-edge in the record and implemented: the 26 edges whose row carries its own kb_id are now composite (kb_id, ref) foreign keys (also stronger — declarative checks still fire under session_replication_role = replica, where user triggers go silent); the 13 whose authority is a parent row's kb_id keep triggers, since a composite key cannot name the parent without a denormalized column that would itself need a trigger. Same-table self-references are DEFERRABLE INITIALLY DEFERRED FKs, which also replaced the deferred constraint triggers.

Your operational note is recorded in the record as well: the §0 scan is the pre-upgrade audit to run against real deployments so the migration does not stop halfway on an unchecked base.

@WaylandYang

Copy link
Copy Markdown
Contributor

All four points answered, and the third and fourth answered better than I asked.

Converting 26 of the 39 edges to FOREIGN KEY (kb_id, ref) REFERENCES t (kb_id, id) takes the triggers from 38 to 22 and the functions from 23 to 10. The argument in 0048 for why the remaining 13 cannot follow — the link row has no kb_id of its own, and adding one would be a denormalization whose equality is itself a second invariant needing a trigger, so you would pay for a column and still pay for the check — is the right answer and is now written down where the next reader will find it.

The point I did not have and that decides it: user triggers go silent under session_replication_role = replica and pg_restore --disable-triggers, while declarative constraints are internal and do not. That makes the hybrid better than uniform triggers on correctness rather than on taste, and it is worth the sentence it gets.

The measured cost section is what I was asking for: no measurable difference on the populate path (medians 9.77s against 9.35s with overlapping ranges) and roughly +6–7µs per row per triggered edge on a focused bulk insert. "A permanent write tax should carry a number, not an adjective" — agreed, and now it does.

I have re-approved the CI run, which fork PRs need after each push.

I am not merging this one myself. It changes the data model permanently and 0048 is explicitly "pending review" with an open question for maintainers in #842 — hybrid split against uniform triggers. That is the maintainer's call, so it is with them now, along with your operational point that the §0 scan should be run read-only against real deployments before rollout.

@WaylandYang

Copy link
Copy Markdown
Contributor

CI came back: migrations and web pass, backend fails on one assertion, and it is a one-line fix.

test tests::schema_version_policy_compares_against_current ... FAILED
crates/utopia-cli/src/main.rs:954: CURRENT_SCHEMA_VERSION must match the count of
applied migrations; bump it when you add a migration

crates/utopia-cli/src/main.rs:83 has const CURRENT_SCHEMA_VERSION: u32 = 69;, and dev currently holds 69 migration files. Adding 0070_provenance_never_points_across_a_knowledge_base.sql makes it 70, so the constant needs to go to 70 in the same commit. It is what the backup manifest writes and what restore compares against, so a backup taken after this migration has to declare the new version.

Nothing else failed. Once that is in, please push and I will re-approve the run — fork PRs need approval after every push.

The hold I described above still stands and is unrelated to this: the data-model decision and #842 are for the maintainer, not for CI.

@ansonnmm
ansonnmm force-pushed the upstream-pr-provenance-schema-invariant branch from de4524f to 31b0a7f Compare September 20, 2026 16:58
@ansonnmm

Copy link
Copy Markdown
Contributor Author

Fixed the remaining backend assertion by bumping CURRENT_SCHEMA_VERSION from 69 to 70 with migration 0070 and folded the change into the migration commit. utopia-cli tests and cargo fmt --check pass. Pushed for CI re-approval.

@WaylandYang

Copy link
Copy Markdown
Contributor

I ran §0 read-only across the local databases, since "does this actually happen" was the one thing the record could not answer. It happens.

Method

The §0 UNION ALL was split into its 39 individual edge checks and each run as a COUNT, so a database on an older schema still reports the edges whose tables it has instead of erroring out wholesale. 22 databases, nothing written.

Result

19 databases clean on every edge they could run. Three — utopia, utopia_nod, utopia_or — each held exactly one bad row, on the same edge:

relation.inverse:  leaks (kb …c0a16a)  --inverse_of-->  employs (kb …76a245)

All three carry the identical row down to the KB ids, so they are copies of one source: one distinct occurrence, not three. It is a genuine cross-KB reference rather than a dangling id — I checked those separately, and the target row exists, in another base.

The detail that makes it diagnostic: the owning base has its own employs (…9c6ccb). The link points past it at another base's employs — which is the signature of an IRI-to-id resolution that was not scoped by kb_id.

Its failure mode is live, not hypothetical

#824 landed earlier today and emits owl:inverseOf only when vocab.relation(id) resolves inside the exporting base. For this row it does not resolve, so the declared inverse is silently dropped from the export — no error, no warning. That is exactly the "a slice of semantics dropped without an error" case in your record, observed on real data rather than argued.

What limits it

Every current write path already scopes the target to the same base:

So the row is a fossil of something since removed or of a write that bypassed the store, rather than a leak the API can reproduce today.

Repaired

Set to NULL on all three, and re-scanned: 22 of 22 now clean, including utopia_open_e2e on the full 39/39.

NULL rather than repointing at the in-base employs, because leaks inverseOf employs is not true, and with #824 merged, repointing would publish a false owl:inverseOf instead of dropping a broken one.

This also clears an upgrade blocker: §0 would have aborted migration 0070 on all three of those databases.

What it changes in my reading

It supports the invariant and it supports your mechanism split — this landed on a self-referencing edge that the declarative composite key covers, and a composite key would have refused it at write time.

It does not, on its own, carry the 13 trigger-covered edges: one fossil in years of development data, on a path the application layer already closes, is a thin basis for a permanent check on every provenance write. My position from the earlier review is unchanged and now better evidenced — take the 26 declarative edges, and gate the triggers on a completeness test that enumerates reference columns from the catalog and asserts each is covered, so the scheme cannot silently stop covering edges added later.

The scan is worth keeping as an operational tool regardless of how that is decided.

南慶麟 added 2 commits September 21, 2026 11:27
Install a schema-level invariant: every reference an export can resolve
must join rows that live in the same knowledge base. A column foreign
key proves the target exists, not that it is the same KB's — the
exporter would otherwise mint local IRIs naming foreign rows, or
silently drop vocabulary references that resolve to nothing.

Three layers: a precondition scan that refuses the migration on a
dirty ledger, per-edge same-KB enforcement (composite (kb_id, ref)
foreign keys where the row carries its own kb_id, row triggers where
the kb authority is a parent row, deferred keys on same-table
self-references so COPY and multi-row inserts are judged at commit),
and kb-ownership immutability on every owned table.

Signed-off-by: 南慶麟 <isolated@test.local>
0048 decides the mechanism per edge: composite (kb_id, ref) foreign keys
where the row carries its own kb_id, row triggers where the kb authority
is a parent row, deferred keys on same-table self-references, kb
immutability throughout. Carries the measured write-path cost and the
operational precondition-scan requirement for real deployments.

Signed-off-by: 南慶麟 <isolated@test.local>
@ansonnmm
ansonnmm force-pushed the upstream-pr-provenance-schema-invariant branch from 31b0a7f to 4097d0a Compare September 21, 2026 03:28
@ansonnmm

Copy link
Copy Markdown
Contributor Author

Added the catalog completeness guard you suggested. every_reference_edge_on_the_ledger_is_classified derives the column-level same-KB reference surface from pg_catalog (conkey/confkey paired by ordinality) and requires each discovered edge to resolve to exactly one class: the composite-FK proof (both kb_id→kb_id and ref→id inside one constraint, deferred for same-table self-references), a registered owner-derived trigger (verified installed and watching that column via tgattr), or an explicit one-line exclusion. Unknown edges and stale registry entries both fail, so a future reference column cannot slip past silently.

Two scratch-schema probes confirm the sensitivity: a new single-column FK on a kb-owned row lands in UNPROTECTED_DIRECT, and a new reference column on an owner-derived row whose existing trigger does not inspect it lands in UNKNOWN — "the table has a trigger" does not imply "the column is covered". Existing coverage resolves exactly: 26 declarative, 13 trigger, 12 explicit non-scope (10 owner edges plus documents.source_id and entities.merged_into, both never export-resolved).

Folded into the two existing commits; fork CI will need re-approval.

@WaylandYang
WaylandYang merged commit ea0557b into deeplethe:dev Sep 21, 2026
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants