From 77e3fabc6e2eb5e6f9781c2e69260ec5ec00a1ee Mon Sep 17 00:00:00 2001 From: abrichr Date: Sun, 25 Oct 2026 10:19:23 -0400 Subject: [PATCH 01/19] frame: key audit entries by a record field, and declare side records Co-Authored-By: Claude Opus 5.5 --- .../openadapt_flow/runtime/effects/frame.py | 103 ++++++++++++++++- packages/flow/tests/test_frame_links.py | 104 ++++++++++++++++++ 2 files changed, 205 insertions(+), 2 deletions(-) create mode 100644 packages/flow/tests/test_frame_links.py diff --git a/packages/flow/openadapt_flow/runtime/effects/frame.py b/packages/flow/openadapt_flow/runtime/effects/frame.py index 15f3fd3dbf..d36c0b11f2 100644 --- a/packages/flow/openadapt_flow/runtime/effects/frame.py +++ b/packages/flow/openadapt_flow/runtime/effects/frame.py @@ -97,6 +97,11 @@ class FrameAttribution(BaseModel): #: ``audit_read`` only: the read-only read of the log, as an ``effects:`` #: verifier section. Built by the caller (``verify-run``). read: Optional[dict[str, Any]] = None + #: ``audit_read`` only: the record field the audit entry's + #: ``record_field`` refers to, when it isn't the frame's identity field. + #: OpenEMR's log names a patient (``pid``) while ``lists`` or ``pnotes`` + #: rows are told apart by their own ``id``. + record_key: Optional[str] = None #: ``record_field`` only: a version, revision or ETag on each record. version_field: Optional[str] = None #: ``record_field`` only: the record fields this app sets on the server @@ -145,6 +150,27 @@ def _shape(self) -> "FrameAttribution": return self +class FrameSideRecord(BaseModel): + """Rows the save adds beside each new declared record (a side record). + + OpenEMR's New Patient path writes one ``history_data`` row for the new + chart, linked by ``pid``. Declared here, such rows are part of the save, + not collateral, and a missing or extra one refutes the frame. + """ + + model_config = ConfigDict(extra="forbid") + + #: The surface the side rows are read from. + surface: str + #: The side row's field that names its record (``pid``). + link_field: str + #: The surface of the declared record, and the field the link names. + of_surface: str + of_field: str + #: How many side rows each new declared record gets. + count: int = Field(default=1, ge=1) + + class FrameSpec(BaseModel): """``frame:`` in ``verify.yaml``: the slice that must otherwise not move.""" @@ -159,6 +185,8 @@ class FrameSpec(BaseModel): #: the fields the effects name. target_fields: Optional[list[str]] = None attribution: Optional[FrameAttribution] = None + #: Rows the save adds beside each new declared record (``FrameSideRecord``). + side_records: list[FrameSideRecord] = Field(default_factory=list) @model_validator(mode="after") def _identity_for_audit(self) -> "FrameSpec": @@ -383,6 +411,19 @@ def key_of(record: Mapping[str, Any]) -> Any: seen = {stable_id(e) for e in audit_before} window_entries = [e for e in audit_after if stable_id(e) not in seen] + def ref_of( + key: Any, + before: Optional[Mapping[str, Any]], + after: Optional[Mapping[str, Any]], + ) -> Any: + """What an audit entry names for this record (``record_key``).""" + + if attribution is None or not attribution.record_key: + return key + record = after if after is not None else before + value = None if record is None else record.get(attribution.record_key) + return None if value in (None, "") else str(value) + def other_on_target( key: Any, before: Optional[Mapping[str, Any]], @@ -392,6 +433,7 @@ def other_on_target( if attribution is None: return False + key = ref_of(key, before, after) if attribution.kind == "audit_read": return window_entries is not None and any( str(get_path(e, attribution.record_field or "")) == str(key) @@ -414,7 +456,8 @@ def attribute( if attribution is None: return "unattributed" if attribution.kind == "audit_read": - if window_entries is None: + key = ref_of(key, before, after) + if window_entries is None or key is None: return "unattributed" actors = { str(get_path(e, attribution.actor_field)) @@ -461,6 +504,33 @@ def note(change: FrameChange) -> None: else: collateral.append(change) + def selectors_for(name: str) -> list[Any]: + mine = [ + effect + for index, effect in enumerate(effects) + if routes is None or name in routes.get(str(index), (name,)) + ] + return [s for effect in mine for s in declared_selectors(effect)] + + # Side records: the links each new declared record gives its side rows. + side_links: list[set[str]] = [] + side_found: list[dict[str, int]] = [] + for side in spec.side_records: + links: set[str] = set() + before_state, after_records = slices.get(side.of_surface, (None, None)) + if before_state is not None and after_records is not None: + sels = selectors_for(side.of_surface) + old = {key_of(r) for r in before_state.records} + for record in after_records: + if key_of(record) in old: + continue + if any(record_matches(dict(record), s) for s in sels): + value = record.get(side.of_field) + if value not in (None, ""): + links.add(str(value)) + side_links.append(links) + side_found.append({link: 0 for link in links}) + for name, (before_state, after_records) in slices.items(): if not before_state.reachable: return _not_checked( @@ -508,7 +578,7 @@ def declared(record: Mapping[str, Any]) -> bool: after is not None and declared(after) ) if is_declared: - declared_keys.add(str(key)) + declared_keys.add(str(ref_of(key, before, after))) if other_on_target(key, before, after): # The saved value may not stand: never excused. collateral.append( @@ -543,6 +613,21 @@ def declared(record: Mapping[str, Any]) -> bool: ) continue if before is None and after is not None: + side_index = next( + ( + i + for i, side in enumerate(spec.side_records) + if side.surface == name + and str(after.get(side.link_field)) in side_links[i] + ), + None, + ) + if side_index is not None: + # A side record of a new declared record: part of the save. + link = str(after.get(spec.side_records[side_index].link_field)) + side_found[side_index][link] += 1 + declared_keys.add(str(ref_of(key, None, after))) + continue note( FrameChange( name, @@ -569,6 +654,20 @@ def declared(record: Mapping[str, Any]) -> bool: ) ) + for i, side in enumerate(spec.side_records): + for link, found in side_found[i].items(): + if found != side.count: + # Missing (or doubled) side record: the chart is not whole. + collateral.append( + FrameChange( + side.surface, + "removed" if found < side.count else "added", + "unattributed", + [f"{found} of {side.count} side record(s)"], + key=link, + ) + ) + if mode == "audit_read": if window_entries is None: # The log could not be read. Every change stays unattributed, and diff --git a/packages/flow/tests/test_frame_links.py b/packages/flow/tests/test_frame_links.py new file mode 100644 index 0000000000..be00c6bbef --- /dev/null +++ b/packages/flow/tests/test_frame_links.py @@ -0,0 +1,104 @@ +"""Frame links: audit entries keyed by a record field, and side records. + +OpenEMR's audit log names a patient (``pid``) while its ``pnotes`` and +``lists`` rows are told apart by their own ``id``, and its New Patient path +adds one ``history_data`` row beside each new chart. Records are made up. +""" + +from __future__ import annotations + +from openadapt_flow.runtime.effects import Effect, EffectKind +from openadapt_flow.runtime.effects.effect import EffectState +from openadapt_flow.runtime.effects.frame import ( + FrameAttribution, + FrameSideRecord, + FrameSpec, + judge_frame, +) + + +def _state(*records: dict) -> EffectState: + return EffectState( + substrate="t", reachable=True, records=[dict(r) for r in records] + ) + + +ATTRIBUTION = FrameAttribution( + kind="audit_read", + actor_field="user", + record_field="patient_id", + record_key="pid", + **{"self": ["bot"], "read": {"kind": "sql", "sql_query": "SELECT 1"}}, +) +NOTE = {"id": 10, "pid": 1, "body": "follow-up"} + + +def _notes_change(spec: FrameSpec, author: str) -> str: + verdict = judge_frame( + spec, + [Effect(kind=EffectKind.RECORD_WRITTEN, match={"lname": "Newchart"})], + {"": (_state(NOTE), [{**NOTE, "body": "changed"}])}, + audit_before=[], + audit_after=[{"id": 1, "user": author, "patient_id": "1"}], + ) + return verdict.status + + +def test_an_audit_entry_names_a_row_through_its_record_key() -> None: + spec = FrameSpec(identity_field="id", attribution=ATTRIBUTION) + # Another user's edit to patient 1's note: concurrent, not collateral. + assert _notes_change(spec, "clinic2") == "held" + # The bot's own: collateral. + assert _notes_change(spec, "bot") == "refuted" + + +def test_without_a_record_key_the_entry_can_t_name_the_row() -> None: + plain = ATTRIBUTION.model_copy(update={"record_key": None}) + spec = FrameSpec(identity_field="id", attribution=plain) + assert _notes_change(spec, "clinic2") == "refuted" # unattributed + + +PATIENT = Effect( + kind=EffectKind.RECORD_WRITTEN, + match={"lname": "Newchart"}, + count_new_only=True, + probe="surface:patient", +) +SIDE = FrameSideRecord( + surface="history", link_field="pid", of_surface="patient", of_field="pid" +) + + +def _with_history(*history_after: dict) -> str: + verdict = judge_frame( + FrameSpec(identity_field="id", side_records=[SIDE]), + [PATIENT], + { + "patient": ( + _state({"id": 1, "pid": 1, "lname": "Old"}), + [ + {"id": 1, "pid": 1, "lname": "Old"}, + {"id": 2, "pid": 2, "lname": "Newchart"}, + ], + ), + "history": ( + _state({"id": 5, "pid": 1}), + [{"id": 5, "pid": 1}, *history_after], + ), + }, + routes={"0": ["patient"]}, + ) + return verdict.status + + +def test_a_side_record_of_the_new_chart_is_part_of_the_save() -> None: + assert _with_history({"id": 6, "pid": 2}) == "held" + + +def test_a_missing_or_doubled_side_record_refutes() -> None: + assert _with_history() == "refuted" + assert _with_history({"id": 6, "pid": 2}, {"id": 7, "pid": 2}) == "refuted" + + +def test_a_row_for_another_chart_is_still_collateral() -> None: + assert _with_history({"id": 6, "pid": 2}, {"id": 7, "pid": 1}) == "refuted" From 96db2b269e24b6ad611fdf8798a015b6d7b372c8 Mon Sep 17 00:00:00 2001 From: abrichr Date: Sun, 25 Oct 2026 11:02:23 -0400 Subject: [PATCH 02/19] frame: diff the whole database, with a reviewed allow-list frame.database snapshots every table and column before and after the save (three statements, whatever the table count) and judges each table as a slice. A row added, removed or changed is collateral unless a declared effect, a side record or an attributed concurrent writer explains it. A new declared or side row that sets a column nothing declares is collateral too. Tables over max_rows are fingerprinted. allow_tables, reviewed per app, replaces the read-list; an unreadable snapshot leaves the frame not checked. Co-Authored-By: Claude Opus 5.5 --- .../runtime/effects/db_snapshot.py | 276 ++++++++++++++++++ .../openadapt_flow/runtime/effects/frame.py | 136 ++++++++- packages/flow/openadapt_flow/verify_run.py | 136 +++++++++ packages/flow/tests/test_frame_database.py | 215 ++++++++++++++ 4 files changed, 752 insertions(+), 11 deletions(-) create mode 100644 packages/flow/openadapt_flow/runtime/effects/db_snapshot.py create mode 100644 packages/flow/tests/test_frame_database.py diff --git a/packages/flow/openadapt_flow/runtime/effects/db_snapshot.py b/packages/flow/openadapt_flow/runtime/effects/db_snapshot.py new file mode 100644 index 0000000000..8ac792cacb --- /dev/null +++ b/packages/flow/openadapt_flow/runtime/effects/db_snapshot.py @@ -0,0 +1,276 @@ +"""Snapshot a whole database for the frame: every table, every column. + +A frame that compares only the surfaces a contract reads can't see a write +to anything else: a column the effects don't name, a field on a side row, a +table nobody thought to read (#1580's held-out misses). ``frame.database`` +turns the frame into a diff of the whole database instead. This module takes +the snapshot; ``frame.judge_frame`` compares two of them like any other +slice. + +The snapshot costs three statements however many tables there are, so a +database reached through a slow channel (``docker exec``) is still read in a +few seconds: + +1. the schema: every table's columns, primary key and column defaults; +2. every table's row count (one ``UNION ALL``); +3. every row of every table at or under ``max_rows``, each row one JSON + object (one ``UNION ALL``). Binary values are hex. + +A table over ``max_rows`` (a translation table, a code set) is fingerprinted +instead (``CHECKSUM TABLE`` on MySQL and MariaDB; a hash of its rows on +SQLite): a change shows that it moved, without which rows. Tables in +``allow_tables`` are not read at all; that list is the reviewed statement of +which tables the app writes on its own. + +Read-only: SQLite files open with ``mode=ro``; a driver connection should use +a SELECT-only account. +""" + +from __future__ import annotations + +import hashlib +import json +from collections.abc import Callable, Mapping, Sequence +from dataclasses import dataclass, field +from typing import Any, Literal, Optional + +Dialect = Literal["sqlite", "mysql"] + +#: Values a column holds when nothing set it. +_EMPTY = {"", "0", "0.0", "0.00", "0000-00-00", "0000-00-00 00:00:00", "NULL"} + + +@dataclass +class Table: + columns: list[str] + primary_key: list[str] + #: Column -> its declared default, as text (None when it has none). + defaults: dict[str, Optional[str]] + count: int = 0 + #: The rows, or None for a table over ``max_rows`` (``digest`` then). + rows: Optional[list[dict[str, Any]]] = None + digest: Optional[str] = None + + +@dataclass +class Snapshot: + tables: dict[str, Table] = field(default_factory=dict) + + def to_json(self) -> dict[str, Any]: + return { + name: { + "columns": t.columns, + "primary_key": t.primary_key, + "defaults": t.defaults, + "count": t.count, + "rows": t.rows, + "digest": t.digest, + } + for name, t in self.tables.items() + } + + @classmethod + def from_json(cls, data: Mapping[str, Any]) -> "Snapshot": + return cls( + tables={ + name: Table( + columns=list(t["columns"]), + primary_key=list(t["primary_key"]), + defaults=dict(t["defaults"]), + count=int(t["count"]), + rows=None if t["rows"] is None else list(t["rows"]), + digest=t.get("digest"), + ) + for name, t in data.items() + } + ) + + +#: A column default meaning any value: the row's own key, which every new +#: row sets. +ANY_VALUE = "\x00any" + + +def is_default(value: Any, default: Optional[str]) -> bool: + """Does a new row's ``value`` look like nothing set it?""" + + if default == ANY_VALUE or value is None: + return True + text = str(value).strip() + if text in _EMPTY: + return True + if default is None: + return False + return text == default.strip().strip("'\"") + + +def _quote(dialect: Dialect, name: str) -> str: + if dialect == "mysql": + return "`" + name.replace("`", "``") + "`" + return '"' + name.replace('"', '""') + '"' + + +def _literal(name: str) -> str: + return "'" + name.replace("'", "''") + "'" + + +def _rows(cursor: Any) -> list[tuple[Any, ...]]: + return [tuple(r) for r in cursor.fetchall()] + + +def _query(connect: Callable[[], Any], sql: str) -> list[tuple[Any, ...]]: + con = connect() + try: + cur = con.cursor() + cur.execute(sql) + return _rows(cur) + finally: + con.close() + + +def _schema( + connect: Callable[[], Any], dialect: Dialect +) -> dict[str, tuple[list[str], list[str], dict[str, Optional[str]], set[str]]]: + """Table -> (columns, primary key, defaults, binary columns).""" + + out: dict[str, tuple[list[str], list[str], dict[str, Optional[str]], set[str]]] + out = {} + if dialect == "mysql": + rows = _query( + connect, + "SELECT c.TABLE_NAME, c.COLUMN_NAME, c.COLUMN_KEY, c.COLUMN_DEFAULT, " + "c.DATA_TYPE FROM information_schema.COLUMNS c JOIN " + "information_schema.TABLES t ON t.TABLE_SCHEMA = c.TABLE_SCHEMA AND " + "t.TABLE_NAME = c.TABLE_NAME WHERE c.TABLE_SCHEMA = DATABASE() AND " + "t.TABLE_TYPE = 'BASE TABLE' ORDER BY c.TABLE_NAME, c.ORDINAL_POSITION", + ) + for table, column, key, default, data_type in rows: + cols, pk, defaults, binary = out.setdefault(str(table), ([], [], {}, set())) + cols.append(str(column)) + if str(key or "") == "PRI": + pk.append(str(column)) + defaults[str(column)] = None if default is None else str(default) + if "binary" in str(data_type).lower() or "blob" in str(data_type).lower(): + binary.add(str(column)) + return out + tables = [ + str(r[0]) + for r in _query( + connect, + "SELECT name FROM sqlite_master WHERE type = 'table' " + "AND name NOT LIKE 'sqlite_%' ORDER BY name", + ) + ] + for table in tables: + info = _query(connect, f"PRAGMA table_info({_quote(dialect, table)})") + cols = [str(r[1]) for r in info] + pk = [str(r[1]) for r in sorted((r for r in info if r[5]), key=lambda r: r[5])] + defaults = {str(r[1]): None if r[4] is None else str(r[4]) for r in info} + out[table] = (cols, pk, defaults, set()) + return out + + +def _cell(dialect: Dialect, column: str, binary: bool) -> str: + q = _quote(dialect, column) + if dialect == "mysql": + return f"HEX({q})" if binary else q + return f"CASE WHEN typeof({q}) = 'blob' THEN hex({q}) ELSE {q} END" + + +def take_snapshot( + connect: Callable[[], Any], + dialect: Dialect, + *, + skip: Sequence[str] = (), + max_rows: int = 20000, +) -> Snapshot: + """Read every table except ``skip``. Raises when the database can't be read.""" + + schema = _schema(connect, dialect) + names = sorted(t for t in schema if t not in set(skip)) + snap = Snapshot() + if not names: + return snap + counts = { + str(t): int(n) + for t, n in _query( + connect, + " UNION ALL ".join( + f"SELECT {_literal(t)}, COUNT(*) FROM {_quote(dialect, t)}" + for t in names + ), + ) + } + small = [t for t in names if 0 < counts.get(t, 0) <= max_rows] + large = [t for t in names if counts.get(t, 0) > max_rows] + for t in names: + cols, pk, defaults, _binary = schema[t] + snap.tables[t] = Table( + columns=cols, + primary_key=pk, + defaults=defaults, + count=counts.get(t, 0), + rows=None if t in large else [], + ) + if small: + obj = "JSON_OBJECT" if dialect == "mysql" else "json_object" + selects = [] + for t in small: + cols, _pk, _d, binary = schema[t] + pairs = ", ".join( + f"{_literal(c)}, {_cell(dialect, c, c in binary)}" for c in cols + ) + selects.append( + f"SELECT {_literal(t)} AS t, {obj}({pairs}) AS r " + f"FROM {_quote(dialect, t)}" + ) + for t, row in _query(connect, " UNION ALL ".join(selects)): + rows = snap.tables[str(t)].rows + assert rows is not None + rows.append(json.loads(row) if isinstance(row, str) else dict(row)) + if large: + if dialect == "mysql": + for t, checksum in _query( + connect, + "CHECKSUM TABLE " + ", ".join(_quote(dialect, t) for t in large), + ): + name = str(t).split(".")[-1] + if name in snap.tables: + snap.tables[name].digest = str(checksum) + else: + for t in large: + h = hashlib.sha256() + for row in _query(connect, f"SELECT * FROM {_quote(dialect, t)}"): + h.update(repr(row).encode()) + snap.tables[t].digest = h.hexdigest() + return snap + + +def row_key(table: Table, row: Mapping[str, Any]) -> Optional[str]: + """A row's identity: its primary key, or None when the table has none.""" + + if not table.primary_key: + return None + return "|".join(str(row.get(c)) for c in table.primary_key) + + +def keyed_rows(table: Table) -> list[dict[str, Any]]: + """The rows, each carrying ``_key``: the primary key, or for a table with + none, the row's own content numbered by occurrence (so a doubled + identical row still counts twice).""" + + out: list[dict[str, Any]] = [] + seen: dict[str, int] = {} + for row in table.rows or []: + key = row_key(table, row) + if key is None: + body = json.dumps(row, sort_keys=True, default=str) + digest = hashlib.sha256(body.encode()).hexdigest()[:16] + seen[digest] = seen.get(digest, 0) + 1 + key = f"{digest}#{seen[digest]}" + out.append({**row, KEY: key}) + return out + + +#: The synthetic identity field of a database slice's records. +KEY = "__row_key__" diff --git a/packages/flow/openadapt_flow/runtime/effects/frame.py b/packages/flow/openadapt_flow/runtime/effects/frame.py index d36c0b11f2..7e7c621df0 100644 --- a/packages/flow/openadapt_flow/runtime/effects/frame.py +++ b/packages/flow/openadapt_flow/runtime/effects/frame.py @@ -171,6 +171,49 @@ class FrameSideRecord(BaseModel): count: int = Field(default=1, ge=1) +class FrameDatabase(BaseModel): + """``frame.database``: diff the whole database, not only what is read. + + Every table and column is snapshotted before and after the save + (``db_snapshot.py``), and each table is judged as one more slice: a row + added, removed or changed is collateral unless a declared effect, a + declared side record or an attributed concurrent writer explains it. A + new declared or side row that sets a column the effects don't name, and + that ``new_row_fields`` doesn't list, is collateral too. + + ``allow_tables`` replaces a read-list with a reviewed allow-list: the + tables the app writes on its own for any signed-in user (logs, sessions, + id registries). Everything else is compared. + """ + + model_config = ConfigDict(extra="forbid") + + dialect: Literal["sqlite", "mysql"] + #: The read-only connection: ``sqlite_database``, or ``sql_driver`` with + #: ``sql_connect_args`` and ``sql_password_env``, as in a ``kind: sql`` + #: verifier (no query: every table is read). + read: dict[str, Any] + #: Tables not compared: the app's own bookkeeping. Reviewed per app. + allow_tables: list[str] = Field(default_factory=list) + #: Table -> the surface whose records it holds (``patient_data`` -> + #: ``patient``): an effect routed to the surface declares rows here too. + views: dict[str, str] = Field(default_factory=dict) + #: Table -> columns a new declared or side row may set beyond the fields + #: the effects name (ids, creation dates, columns the app's form fills + #: on its own). Any other column must hold its default. + new_row_fields: dict[str, list[str]] = Field(default_factory=dict) + #: Tables over this many rows are fingerprinted, not compared row by row. + max_rows: int = Field(default=20000, ge=1) + + @model_validator(mode="after") + def _connection(self) -> "FrameDatabase": + if not (self.read.get("sqlite_database") or self.read.get("sql_driver")): + raise ValueError("frame.database.read needs sqlite_database or sql_driver") + if self.read.get("sql_query"): + raise ValueError("frame.database.read reads every table; drop sql_query") + return self + + class FrameSpec(BaseModel): """``frame:`` in ``verify.yaml``: the slice that must otherwise not move.""" @@ -187,6 +230,8 @@ class FrameSpec(BaseModel): attribution: Optional[FrameAttribution] = None #: Rows the save adds beside each new declared record (``FrameSideRecord``). side_records: list[FrameSideRecord] = Field(default_factory=list) + #: Diff the whole database (``FrameDatabase``). + database: Optional[FrameDatabase] = None @model_validator(mode="after") def _identity_for_audit(self) -> "FrameSpec": @@ -352,6 +397,9 @@ def judge_frame( audit_before: Optional[list[dict[str, Any]]] = None, audit_after: Optional[list[dict[str, Any]]] = None, run_actors: Sequence[str] = (), + identities: Optional[Mapping[str, str]] = None, + new_row_defaults: Optional[Mapping[str, Mapping[str, Optional[str]]]] = None, + unread_changes: Sequence[str] = (), ) -> FrameVerdict: """Judge the frame of ``effects`` over ``slices``. @@ -368,6 +416,13 @@ def judge_frame( audit_after: ``audit_read`` entries read after the save. run_actors: More ids of this run (the executor's), never "another actor". + identities: Slice -> its identity field, where it isn't + ``spec.identity_field`` (a database table's row key). + new_row_defaults: Slice -> column defaults, for slices whose new + declared and side rows must set nothing undeclared (database + tables, ``spec.database``). + unread_changes: Tables that moved but were fingerprinted, not read + row by row (or whose schema changed). Each is collateral. """ attribution = spec.attribution @@ -383,7 +438,9 @@ def judge_frame( ) self_ids = resolved | {str(a) for a in run_actors if str(a)} - ignore = set(spec.ignore_fields) + from openadapt_flow.runtime.effects.db_snapshot import KEY, is_default + + ignore = set(spec.ignore_fields) | {KEY} if attribution is not None and attribution.kind == "record_field": # Server-set bookkeeping moves on every write; it says who, not what. ignore |= set(attribution.server_set) @@ -399,12 +456,35 @@ def judge_frame( attribution = None mode = "attribution_refused" - def key_of(record: Mapping[str, Any]) -> Any: - if spec.identity_field: - value = record.get(spec.identity_field) + def identity_of(name: str) -> Optional[str]: + if identities is not None and name in identities: + return identities[name] + return spec.identity_field + + def key_of(record: Mapping[str, Any], name: str = "") -> Any: + identity = identity_of(name) + if identity: + value = record.get(identity) return None if value is None else str(value) return stable_id(_without(record, ignore)) + row_fields = spec.database.new_row_fields if spec.database is not None else {} + + def undeclared_columns( + name: str, record: Mapping[str, Any], also: Sequence[str] = () + ) -> list[str]: + """Columns a new row sets that nothing declares (database slices).""" + + defaults = (new_row_defaults or {}).get(name) + if defaults is None: + return [] + allowed = written | set(row_fields.get(name, ())) | ignore | set(also) + return sorted( + c + for c, v in record.items() + if c not in allowed and not is_default(v, defaults.get(c)) + ) + window_entries: Optional[list[dict[str, Any]]] = None if mode == "audit_read": if audit_before is not None and audit_after is not None: @@ -520,9 +600,9 @@ def selectors_for(name: str) -> list[Any]: before_state, after_records = slices.get(side.of_surface, (None, None)) if before_state is not None and after_records is not None: sels = selectors_for(side.of_surface) - old = {key_of(r) for r in before_state.records} + old = {key_of(r, side.of_surface) for r in before_state.records} for record in after_records: - if key_of(record) in old: + if key_of(record, side.of_surface) in old: continue if any(record_matches(dict(record), s) for s in sels): value = record.get(side.of_field) @@ -555,16 +635,16 @@ def declared(record: Mapping[str, Any]) -> bool: ("post-save", after_records, after_map), ): for record in records: - key = key_of(record) + key = key_of(record, name) if key is None: return _not_checked( - f"a {label} record carries no {spec.identity_field!r}; " + f"a {label} record carries no {identity_of(name)!r}; " "the slice can't be compared record by record", mode, ) - if key in into and spec.identity_field: + if key in into and identity_of(name): return _not_checked( - f"two {label} records share one {spec.identity_field!r}; " + f"two {label} records share one {identity_of(name)!r}; " "the slice can't be compared record by record", mode, ) @@ -593,6 +673,20 @@ def declared(record: Mapping[str, Any]) -> bool: ) ) continue + if before is None and after is not None: + extra = undeclared_columns(name, after) + if extra: + # The new record sets a column nothing declares. + collateral.append( + FrameChange( + name, + "added", + attribute(key, None, after), + extra, + on_target=True, + key=key, + ) + ) if before is None or after is None: continue # a new or removed target: the delta check's case stray = [ @@ -624,9 +718,22 @@ def declared(record: Mapping[str, Any]) -> bool: ) if side_index is not None: # A side record of a new declared record: part of the save. - link = str(after.get(spec.side_records[side_index].link_field)) + side = spec.side_records[side_index] + link = str(after.get(side.link_field)) side_found[side_index][link] += 1 declared_keys.add(str(ref_of(key, None, after))) + extra = undeclared_columns(name, after, (side.link_field,)) + if extra: + collateral.append( + FrameChange( + name, + "added", + attribute(key, None, after), + extra, + on_target=True, + key=key, + ) + ) continue note( FrameChange( @@ -654,6 +761,13 @@ def declared(record: Mapping[str, Any]) -> bool: ) ) + for table in unread_changes: + # Fingerprinted, not read row by row: it moved, and nothing can say + # which rows or who moved them. + collateral.append( + FrameChange(table, "changed", "unattributed", ["rows not compared"]) + ) + for i, side in enumerate(spec.side_records): for link, found in side_found[i].items(): if found != side.count: diff --git a/packages/flow/openadapt_flow/verify_run.py b/packages/flow/openadapt_flow/verify_run.py index 1d9d4100e7..97b0bf8e63 100644 --- a/packages/flow/openadapt_flow/verify_run.py +++ b/packages/flow/openadapt_flow/verify_run.py @@ -145,6 +145,8 @@ #: Where the baseline keeps the audit log read for frame attribution. Not a #: surface name: those are plain identifiers. FRAME_AUDIT_DETAIL = "openadapt:frame_audit" +#: Where the baseline keeps the whole-database snapshot (``frame.database``). +FRAME_DB_DETAIL = "openadapt:frame_database" #: ``checked`` (``docs/VERIFICATION_COVERAGE.md``): which parts of the claim #: behind ``verified`` were checked, and how each came out. @@ -337,6 +339,14 @@ def parse_verify_config(data: Any, *, source: str = "verify config") -> VerifyCo "frame.attribution.read must be one read-only system-of-record " "read (rest, sql, fhir, graphql, ...)" ) + database = cfg.frame.database if cfg.frame is not None else None + if database is not None: + surfaces = set(cfg.verifier.surfaces) or {""} + unknown = sorted(set(database.views.values()) - surfaces) + if unknown: + raise VerifyRunError( + "frame.database.views names no such surface: " + ", ".join(unknown) + ) for effect in cfg.effects: if effect.needs_operator_confirmation: raise VerifyRunError( @@ -400,6 +410,101 @@ def _audit_verifier(cfg: VerifyConfig, params: Mapping[str, object]) -> Any: raise VerifyRunError(f"cannot build the audit read: {exc}") from exc +def _db_connect(read: Mapping[str, Any]) -> Any: + """A connect function for ``frame.database.read`` (read-only).""" + + if read.get("sqlite_database"): + import sqlite3 + + uri = Path(str(read["sqlite_database"])).resolve().as_uri() + "?mode=ro" + return lambda: sqlite3.connect(uri, uri=True) + import importlib + + from openadapt_flow.deployment import _require_env + + module = importlib.import_module(str(read["sql_driver"])) + args = dict(read.get("sql_connect_args") or {}) + if read.get("sql_password_env"): + args["password"] = _require_env( + str(read["sql_password_env"]), "frame.database.read.sql_password_env" + ) + return lambda: module.connect(**args) + + +def _read_database(cfg: VerifyConfig) -> Optional[dict[str, Any]]: + """The whole-database snapshot, or None when it can't be read.""" + + from openadapt_flow.runtime.effects.db_snapshot import take_snapshot + + database = cfg.frame.database if cfg.frame is not None else None + if database is None: + return None + try: + snap = take_snapshot( + _db_connect(database.read), + database.dialect, + skip=database.allow_tables, + max_rows=database.max_rows, + ) + except Exception: # noqa: BLE001 - unreadable -> the frame is not checked + return None + return snap.to_json() + + +DatabaseSlices = tuple[ + dict[str, tuple[EffectState, Optional[list[dict[str, Any]]]]], + dict[str, dict[str, Optional[str]]], + list[str], +] + + +def _database_slices( + cfg: VerifyConfig, before: EffectState +) -> Optional[DatabaseSlices]: + """Slices for every table, their column defaults, and tables moved unread. + + None when there is no ``frame.database``. A baseline without a readable + snapshot, or an unreadable post-save read, gives one unreachable slice, so + the frame is not checked. + """ + + from openadapt_flow.runtime.effects.db_snapshot import ( + ANY_VALUE, + Snapshot, + keyed_rows, + ) + + if cfg.frame is None or cfg.frame.database is None: + return None + stored = ( + before.detail.get(FRAME_DB_DETAIL) if isinstance(before.detail, dict) else None + ) + after_json = _read_database(cfg) + if not isinstance(stored, dict) or after_json is None: + unreachable = EffectState(substrate="database", reachable=False, records=[]) + return {"database": (unreachable, None)}, {}, [] + old, new = Snapshot.from_json(stored), Snapshot.from_json(after_json) + slices: dict[str, tuple[EffectState, Optional[list[dict[str, Any]]]]] = {} + defaults: dict[str, dict[str, Optional[str]]] = {} + unread: list[str] = [] + for name in sorted(set(old.tables) | set(new.tables)): + a, b = old.tables.get(name), new.tables.get(name) + if a is None or b is None or a.columns != b.columns: + unread.append(name) # created, dropped or altered in the window + continue + if a.rows is None or b.rows is None: + if (a.count, a.digest) != (b.count, b.digest): + unread.append(name) + continue + slices[name] = ( + EffectState(substrate="database", reachable=True, records=keyed_rows(a)), + keyed_rows(b), + ) + # The primary key is the row's identity: every new row sets it. + defaults[name] = {**b.defaults, **dict.fromkeys(b.primary_key, ANY_VALUE)} + return slices, defaults, unread + + def _read_audit(verifier: Any) -> Optional[list[dict[str, Any]]]: if verifier is None: return None @@ -469,6 +574,28 @@ def read_of(name: str) -> str: ) else: slices[""] = (before, _post_records(after)) + identities: Optional[dict[str, str]] = None + defaults: Optional[dict[str, dict[str, Optional[str]]]] = None + unread: list[str] = [] + database = _database_slices(cfg, before) + if database is not None: + from openadapt_flow.runtime.effects.db_snapshot import KEY + + db_slices, defaults, unread = database + clash = sorted(set(db_slices) & set(slices)) + if clash: + return FrameVerdict( + status="not_checked", + reason="a table has the same name as a surface: " + ", ".join(clash), + ) + if routes is None: + routes = {str(i): [""] for i in range(len(bound_effects))} + views = spec.database.views if spec.database is not None else {} + routes = {k: list(v) for k, v in routes.items()} + for own in routes.values(): + own.extend(t for t, surface in views.items() if surface in own) + slices.update(db_slices) + identities = {name: KEY for name in db_slices} audit_before: Optional[list[dict[str, Any]]] = None audit_after: Optional[list[dict[str, Any]]] = None if audit_verifier is not None: @@ -489,6 +616,9 @@ def read_of(name: str) -> str: audit_before=audit_before, audit_after=audit_after, run_actors=run_actors, + identities=identities, + new_row_defaults=defaults, + unread_changes=unread, ) @@ -701,6 +831,12 @@ def capture_baseline( } } ) + if cfg.frame is not None and cfg.frame.database is not None: + snapshot = _read_database(cfg) + if snapshot is not None: + state = state.model_copy( + update={"detail": {**state.detail, FRAME_DB_DETAIL: snapshot}} + ) payload = { "schema_version": BASELINE_SCHEMA, "execution_id": execution_id, diff --git a/packages/flow/tests/test_frame_database.py b/packages/flow/tests/test_frame_database.py new file mode 100644 index 0000000000..0f4528c668 --- /dev/null +++ b/packages/flow/tests/test_frame_database.py @@ -0,0 +1,215 @@ +"""The whole-database frame: every table and column, not a read-list. + +A frame over the surfaces a contract reads passes any write outside them: a +column the effects don't name on the new record, a field on its side row, a +row in a table nobody reads. ``frame.database`` diffs the whole database +instead, with a reviewed allow-list for the app's own bookkeeping. Records +are made up; the database is SQLite, judged through ``verify_run``. +""" + +from __future__ import annotations + +import sqlite3 +from pathlib import Path +from typing import Any, Callable + +import pytest + +from openadapt_flow.runtime.effects.db_snapshot import is_default, take_snapshot +from openadapt_flow.verify_run import ( + capture_baseline, + parse_verify_config, + verify_run, +) + +SCHEMA = """ +CREATE TABLE patient_data (id INTEGER PRIMARY KEY, pid INTEGER, fname TEXT, + lname TEXT, email TEXT, phone_cell TEXT DEFAULT '', regdate TEXT); +CREATE TABLE history_data (id INTEGER PRIMARY KEY, pid INTEGER, date TEXT, + tobacco TEXT DEFAULT ''); +CREATE TABLE calendar (id INTEGER PRIMARY KEY, pid INTEGER, title TEXT); +CREATE TABLE codes (code TEXT); +CREATE TABLE log (id INTEGER PRIMARY KEY, user TEXT, patient_id INTEGER); +INSERT INTO patient_data VALUES (1, 1, 'Wendell', 'Prior', 'w@example.test', '', '2026-01-01'); +INSERT INTO history_data VALUES (1, 1, '2026-01-01', ''); +INSERT INTO codes VALUES ('A'), ('B'), ('C'); +""" +NEW = {"fname": "Ada", "lname": "Quill", "email": "ada@example.test"} + + +def _db(tmp_path: Path) -> Path: + path = tmp_path / "app.sqlite" + con = sqlite3.connect(path) + con.executescript(SCHEMA) + con.commit() + con.close() + return path + + +def _doc(db: Path, **database: Any) -> dict[str, Any]: + return { + "schema_version": "openadapt.verify/v1", + "verifier": { + "kind": "sql", + "sqlite_database": str(db), + "sql_query": "SELECT * FROM patient_data", + }, + "record": { + "name": "patient", + "match": ["email", "lname"], + "fields": {"fname": "fname", "lname": "lname", "email": "email"}, + }, + "frame": { + "identity_field": "id", + "side_records": [ + { + "surface": "history_data", + "link_field": "pid", + "of_surface": "patient_data", + "of_field": "pid", + } + ], + "database": { + "dialect": "sqlite", + "read": {"sqlite_database": str(db)}, + "allow_tables": ["log"], + "views": {"patient_data": ""}, + "new_row_fields": { + "patient_data": ["pid", "regdate"], + "history_data": ["date"], + }, + **database, + }, + }, + } + + +def _save(con: sqlite3.Connection) -> None: + """The app's own save: the chart, its history row, a log line.""" + + con.execute( + "INSERT INTO patient_data (id, pid, fname, lname, email, regdate) " + "VALUES (2, 2, 'Ada', 'Quill', 'ada@example.test', '2026-10-25')" + ) + con.execute("INSERT INTO history_data (pid, date) VALUES (2, '2026-10-25')") + con.execute("INSERT INTO log (user, patient_id) VALUES ('bot', 2)") + + +def _outcome( + tmp_path: Path, + after: Callable[[sqlite3.Connection], None], + **database: Any, +) -> dict[str, Any]: + db = _db(tmp_path) + cfg = parse_verify_config(_doc(db, **database)) + capture_baseline(cfg, NEW, tmp_path / "baseline.json") + con = sqlite3.connect(db) + _save(con) + after(con) + con.commit() + con.close() + result = verify_run( + cfg, + params=NEW, + run_record={"executor_kind": "agent", "executor_id": "bot"}, + authorization_ref="test", + baseline_path=tmp_path / "baseline.json", + ) + return result + + +def _status(result: dict[str, Any]) -> str: + return result["receipt"]["checked"]["frame"] + + +def test_a_clean_save_holds(tmp_path: Path) -> None: + result = _outcome(tmp_path, lambda con: None) + assert _status(result) == "held" + assert result["receipt"]["outcome"] == "verified" + + +@pytest.mark.parametrize( + "stray", + [ + # A column the effects don't name, set on the new record. + "UPDATE patient_data SET phone_cell = '617-555-0142' WHERE pid = 2", + # A field on the new record's side row. + "UPDATE history_data SET tobacco = 'Never' WHERE pid = 2", + # A row in a table no surface reads. + "INSERT INTO calendar (pid, title) VALUES (2, 'New Patient')", + # A second side row. + "INSERT INTO history_data (pid, date) VALUES (2, '2026-10-25')", + # A table fingerprinted rather than read row by row. + "INSERT INTO codes VALUES ('D')", + ], +) +def test_any_write_outside_the_declared_delta_refutes( + tmp_path: Path, stray: str +) -> None: + result = _outcome(tmp_path, lambda con: con.execute(stray), max_rows=2) + assert _status(result) == "refuted", stray + assert result["receipt"]["outcome"] != "verified" + + +def test_the_allow_list_is_the_only_exemption(tmp_path: Path) -> None: + # Bookkeeping the app writes on its own: allowed by name. + more_log = "INSERT INTO log (user, patient_id) VALUES ('bot', 2)" + assert _status(_outcome(tmp_path, lambda con: con.execute(more_log))) == "held" + + +def test_an_unreadable_database_is_not_checked(tmp_path: Path) -> None: + db = _db(tmp_path) + doc = _doc(db) + doc["frame"]["database"]["read"] = {"sqlite_database": str(tmp_path / "gone")} + cfg = parse_verify_config(doc) + capture_baseline(cfg, NEW, tmp_path / "baseline.json") + result = verify_run( + cfg, + params=NEW, + run_record={"executor_kind": "agent", "executor_id": "bot"}, + authorization_ref="test", + baseline_path=tmp_path / "baseline.json", + ) + assert _status(result) == "not_checked" + assert result["receipt"]["outcome"] != "verified" + + +def test_the_snapshot_reads_every_table_in_three_statements(tmp_path: Path) -> None: + db = _db(tmp_path) + statements: list[str] = [] + + class Spy: + def __init__(self) -> None: + self.con = sqlite3.connect(db) + + def cursor(self) -> Any: + outer = self + + class Cur: + def __init__(self) -> None: + self.cur = outer.con.cursor() + + def execute(self, sql: str) -> None: + statements.append(sql) + self.cur.execute(sql) + + def fetchall(self) -> list: + return self.cur.fetchall() + + return Cur() + + def close(self) -> None: + self.con.close() + + snap = take_snapshot(Spy, "sqlite", skip=["log"]) + assert set(snap.tables) == {"patient_data", "history_data", "calendar", "codes"} + assert snap.tables["calendar"].rows == [] + reads = [s for s in statements if not s.startswith(("PRAGMA", "SELECT name"))] + assert len(reads) == 2 # counts, then every row + + +def test_default_values_are_unset() -> None: + assert is_default("", None) + assert is_default("0000-00-00", None) + assert is_default("NO", "'NO'") + assert not is_default("617-555-0142", "''") From 3ad205f397e704716341770a1c7a997ad261c811 Mon Sep 17 00:00:00 2001 From: abrichr Date: Sun, 25 Oct 2026 11:08:48 -0400 Subject: [PATCH 03/19] frame: read rows wider than one JSON call, keeping null columns SQLite caps a function at 127 arguments and OpenEMR's chart has more columns, so the snapshot failed and the frame was not checked. Each row is now read as JSON objects of at most 50 columns, merged in Python (a JSON merge patch would drop null values). Co-Authored-By: Claude Opus 5.5 --- .../runtime/effects/db_snapshot.py | 43 +++++++++++++++---- packages/flow/tests/test_frame_database.py | 15 +++++++ 2 files changed, 49 insertions(+), 9 deletions(-) diff --git a/packages/flow/openadapt_flow/runtime/effects/db_snapshot.py b/packages/flow/openadapt_flow/runtime/effects/db_snapshot.py index 8ac792cacb..232be3b7a8 100644 --- a/packages/flow/openadapt_flow/runtime/effects/db_snapshot.py +++ b/packages/flow/openadapt_flow/runtime/effects/db_snapshot.py @@ -177,6 +177,29 @@ def _cell(dialect: Dialect, column: str, binary: bool) -> str: return f"CASE WHEN typeof({q}) = 'blob' THEN hex({q}) ELSE {q} END" +#: Columns per JSON object call: SQLite caps a function at 127 arguments. +_CHUNK = 50 + + +def _row_json(dialect: Dialect, cols: Sequence[str], binary: set[str]) -> list[str]: + """One row as JSON objects of at most ``_CHUNK`` columns each. + + Merged in Python, not in SQL: a JSON merge patch drops null values. + """ + + obj = "JSON_OBJECT" if dialect == "mysql" else "json_object" + return [ + obj + + "(" + + ", ".join( + f"{_literal(c)}, {_cell(dialect, c, c in binary)}" + for c in cols[i : i + _CHUNK] + ) + + ")" + for i in range(0, len(cols), _CHUNK) + ] + + def take_snapshot( connect: Callable[[], Any], dialect: Dialect, @@ -213,21 +236,23 @@ def take_snapshot( rows=None if t in large else [], ) if small: - obj = "JSON_OBJECT" if dialect == "mysql" else "json_object" + parts = {t: _row_json(dialect, schema[t][0], schema[t][3]) for t in small} + width = max(len(p) for p in parts.values()) selects = [] for t in small: - cols, _pk, _d, binary = schema[t] - pairs = ", ".join( - f"{_literal(c)}, {_cell(dialect, c, c in binary)}" for c in cols - ) + padded = parts[t] + ["NULL"] * (width - len(parts[t])) + cells = ", ".join(f"{c} AS r{i}" for i, c in enumerate(padded)) selects.append( - f"SELECT {_literal(t)} AS t, {obj}({pairs}) AS r " - f"FROM {_quote(dialect, t)}" + f"SELECT {_literal(t)} AS t, {cells} FROM {_quote(dialect, t)}" ) - for t, row in _query(connect, " UNION ALL ".join(selects)): + for t, *chunks in _query(connect, " UNION ALL ".join(selects)): rows = snap.tables[str(t)].rows assert rows is not None - rows.append(json.loads(row) if isinstance(row, str) else dict(row)) + row: dict[str, Any] = {} + for chunk in chunks: + if chunk is not None: + row.update(json.loads(chunk) if isinstance(chunk, str) else chunk) + rows.append(row) if large: if dialect == "mysql": for t, checksum in _query( diff --git a/packages/flow/tests/test_frame_database.py b/packages/flow/tests/test_frame_database.py index 0f4528c668..0488391971 100644 --- a/packages/flow/tests/test_frame_database.py +++ b/packages/flow/tests/test_frame_database.py @@ -213,3 +213,18 @@ def test_default_values_are_unset() -> None: assert is_default("0000-00-00", None) assert is_default("NO", "'NO'") assert not is_default("617-555-0142", "''") + + +def test_a_table_wider_than_one_json_call_is_read(tmp_path: Path) -> None: + # SQLite caps a function at 127 arguments; OpenEMR's chart has more + # columns than that. + cols = ", ".join(f"c{i} TEXT" for i in range(150)) + path = tmp_path / "wide.sqlite" + con = sqlite3.connect(path) + con.execute(f"CREATE TABLE wide (id INTEGER PRIMARY KEY, {cols})") + con.execute("INSERT INTO wide (id, c149) VALUES (1, 'last')") + con.commit() + con.close() + snap = take_snapshot(lambda: sqlite3.connect(path), "sqlite") + (row,) = snap.tables["wide"].rows or [] + assert row["c149"] == "last" and len(row) == 151 From 430ea8e1c544045690d7102865ec8753892e311f Mon Sep 17 00:00:00 2001 From: abrichr Date: Sun, 25 Oct 2026 11:13:04 -0400 Subject: [PATCH 04/19] frame: a default the database computes at insert is a default Co-Authored-By: Claude Opus 5.5 --- .../flow/openadapt_flow/runtime/effects/db_snapshot.py | 9 ++++++++- packages/flow/tests/test_frame_database.py | 3 +++ 2 files changed, 11 insertions(+), 1 deletion(-) diff --git a/packages/flow/openadapt_flow/runtime/effects/db_snapshot.py b/packages/flow/openadapt_flow/runtime/effects/db_snapshot.py index 232be3b7a8..df12ff857b 100644 --- a/packages/flow/openadapt_flow/runtime/effects/db_snapshot.py +++ b/packages/flow/openadapt_flow/runtime/effects/db_snapshot.py @@ -91,10 +91,17 @@ def from_json(cls, data: Mapping[str, Any]) -> "Snapshot": ANY_VALUE = "\x00any" +def _computed(default: Optional[str]) -> bool: + """A default the database computes at insert (the current time).""" + + text = (default or "").strip().lower().strip("()") + return text.startswith(("current_timestamp", "now", "current_date", "localtime")) + + def is_default(value: Any, default: Optional[str]) -> bool: """Does a new row's ``value`` look like nothing set it?""" - if default == ANY_VALUE or value is None: + if default == ANY_VALUE or value is None or _computed(default): return True text = str(value).strip() if text in _EMPTY: diff --git a/packages/flow/tests/test_frame_database.py b/packages/flow/tests/test_frame_database.py index 0488391971..c5645a18cc 100644 --- a/packages/flow/tests/test_frame_database.py +++ b/packages/flow/tests/test_frame_database.py @@ -213,6 +213,9 @@ def test_default_values_are_unset() -> None: assert is_default("0000-00-00", None) assert is_default("NO", "'NO'") assert not is_default("617-555-0142", "''") + # Set by the database at insert: any value is its default. + assert is_default("2026-10-25 11:00:00", "CURRENT_TIMESTAMP") + assert is_default("2026-10-25 11:00:00", "current_timestamp()") def test_a_table_wider_than_one_json_call_is_read(tmp_path: Path) -> None: From 6e0742801ab6545b19d160916c00d0d8adefdfda Mon Sep 17 00:00:00 2001 From: abrichr Date: Sun, 25 Oct 2026 11:18:05 -0400 Subject: [PATCH 05/19] frame: audit attribution can name a table's record through another field Co-Authored-By: Claude Opus 5.5 --- .../openadapt_flow/runtime/effects/frame.py | 40 +++++++++++++------ packages/flow/tests/test_frame_links.py | 18 +++++++++ 2 files changed, 45 insertions(+), 13 deletions(-) diff --git a/packages/flow/openadapt_flow/runtime/effects/frame.py b/packages/flow/openadapt_flow/runtime/effects/frame.py index 7e7c621df0..8c0711fdf6 100644 --- a/packages/flow/openadapt_flow/runtime/effects/frame.py +++ b/packages/flow/openadapt_flow/runtime/effects/frame.py @@ -102,6 +102,10 @@ class FrameAttribution(BaseModel): #: OpenEMR's log names a patient (``pid``) while ``lists`` or ``pnotes`` #: rows are told apart by their own ``id``. record_key: Optional[str] = None + #: ``audit_read`` only: per surface or table, the field that names the + #: record when it isn't ``record_key`` (OpenEMR's ``contact`` row points + #: at its chart through ``foreign_id``). + record_keys: dict[str, str] = Field(default_factory=dict) #: ``record_field`` only: a version, revision or ETag on each record. version_field: Optional[str] = None #: ``record_field`` only: the record fields this app sets on the server @@ -132,6 +136,8 @@ def _shape(self) -> "FrameAttribution": raise ValueError("version_field applies to record_field attribution") if self.server_set: raise ValueError("server_set applies to record_field attribution") + elif self.record_keys: + raise ValueError("record_keys applies to audit_read attribution") else: if self.read is not None or self.record_field is not None: raise ValueError( @@ -492,19 +498,24 @@ def undeclared_columns( window_entries = [e for e in audit_after if stable_id(e) not in seen] def ref_of( + name: str, key: Any, before: Optional[Mapping[str, Any]], after: Optional[Mapping[str, Any]], ) -> Any: """What an audit entry names for this record (``record_key``).""" - if attribution is None or not attribution.record_key: + if attribution is None: + return key + field = attribution.record_keys.get(name, attribution.record_key) + if not field: return key record = after if after is not None else before - value = None if record is None else record.get(attribution.record_key) + value = None if record is None else record.get(field) return None if value in (None, "") else str(value) def other_on_target( + name: str, key: Any, before: Optional[Mapping[str, Any]], after: Optional[Mapping[str, Any]], @@ -513,7 +524,7 @@ def other_on_target( if attribution is None: return False - key = ref_of(key, before, after) + key = ref_of(name, key, before, after) if attribution.kind == "audit_read": return window_entries is not None and any( str(get_path(e, attribution.record_field or "")) == str(key) @@ -529,6 +540,7 @@ def other_on_target( return actor not in (None, "") and str(actor) not in self_ids def attribute( + name: str, key: Any, before: Optional[Mapping[str, Any]], after: Optional[Mapping[str, Any]], @@ -536,7 +548,7 @@ def attribute( if attribution is None: return "unattributed" if attribution.kind == "audit_read": - key = ref_of(key, before, after) + key = ref_of(name, key, before, after) if window_entries is None or key is None: return "unattributed" actors = { @@ -658,8 +670,8 @@ def declared(record: Mapping[str, Any]) -> bool: after is not None and declared(after) ) if is_declared: - declared_keys.add(str(ref_of(key, before, after))) - if other_on_target(key, before, after): + declared_keys.add(str(ref_of(name, key, before, after))) + if other_on_target(name, key, before, after): # The saved value may not stand: never excused. collateral.append( FrameChange( @@ -681,7 +693,7 @@ def declared(record: Mapping[str, Any]) -> bool: FrameChange( name, "added", - attribute(key, None, after), + attribute(name, key, None, after), extra, on_target=True, key=key, @@ -699,7 +711,7 @@ def declared(record: Mapping[str, Any]) -> bool: FrameChange( name, "changed", - attribute(key, before, after), + attribute(name, key, before, after), stray, on_target=True, key=key, @@ -721,14 +733,14 @@ def declared(record: Mapping[str, Any]) -> bool: side = spec.side_records[side_index] link = str(after.get(side.link_field)) side_found[side_index][link] += 1 - declared_keys.add(str(ref_of(key, None, after))) + declared_keys.add(str(ref_of(name, key, None, after))) extra = undeclared_columns(name, after, (side.link_field,)) if extra: collateral.append( FrameChange( name, "added", - attribute(key, None, after), + attribute(name, key, None, after), extra, on_target=True, key=key, @@ -739,14 +751,16 @@ def declared(record: Mapping[str, Any]) -> bool: FrameChange( name, "added", - attribute(key, None, after), + attribute(name, key, None, after), sorted(_without(after, ignore)), key=key, ) ) elif after is None and before is not None: note( - FrameChange(name, "removed", attribute(key, before, None), key=key) + FrameChange( + name, "removed", attribute(name, key, before, None), key=key + ) ) elif before is not None and after is not None: fields = _changed_fields(before, after, ignore) @@ -755,7 +769,7 @@ def declared(record: Mapping[str, Any]) -> bool: FrameChange( name, "changed", - attribute(key, before, after), + attribute(name, key, before, after), fields, key=key, ) diff --git a/packages/flow/tests/test_frame_links.py b/packages/flow/tests/test_frame_links.py index be00c6bbef..729b9f7cdb 100644 --- a/packages/flow/tests/test_frame_links.py +++ b/packages/flow/tests/test_frame_links.py @@ -102,3 +102,21 @@ def test_a_missing_or_doubled_side_record_refutes() -> None: def test_a_row_for_another_chart_is_still_collateral() -> None: assert _with_history({"id": 6, "pid": 2}, {"id": 7, "pid": 1}) == "refuted" + + +def test_a_table_can_name_its_record_through_another_field() -> None: + # OpenEMR's contact row points at its chart through foreign_id. + keyed = ATTRIBUTION.model_copy(update={"record_keys": {"contact": "foreign_id"}}) + contact = {"id": 3, "foreign_id": 1} + + def status(spec: FrameSpec) -> str: + return judge_frame( + spec, + [Effect(kind=EffectKind.RECORD_WRITTEN, match={"lname": "Newchart"})], + {"contact": (_state(), [contact])}, + audit_before=[], + audit_after=[{"id": 1, "user": "clinic2", "patient_id": "1"}], + ).status + + assert status(FrameSpec(identity_field="id", attribution=keyed)) == "held" + assert status(FrameSpec(identity_field="id", attribution=ATTRIBUTION)) == "refuted" From bac815b7849240a672641f7e9d142aaea018de44 Mon Sep 17 00:00:00 2001 From: abrichr Date: Wed, 28 Oct 2026 15:08:36 -0400 Subject: [PATCH 06/19] frame: whole-database diff without values, in one consistent read From the adversarial review of #1582: - PHI: the baseline keeps keyed MACs (BLAKE2b with the run salt) per cell and per row key, and keyed ids of audit entries; plain values are read only for rows the save added, in memory. - Consistency: one connection, one read-only transaction (START TRANSACTION WITH CONSISTENT SNAPSHOT, READ ONLY; BEGIN on SQLite), with the audit log read inside it when it shares the connection. - Large tables: salted SHA-256 per bucket of 1024 keys up to the baseline's largest key, added rows read in full, so a clean save on a table over max_rows holds. A table keeps its baseline representation after the save. - Silent passes closed: a value is a default only if it equals the declared one; a clock default must fall between the snapshots' database clocks; ignore_fields apply to surfaces and database.ignore_columns to tables; sqlite_sequence and AUTO_INCREMENT catch a write then delete; table definitions and triggers, views, routines and events are digested; only a single generated key may hold any value; with frame.database, only a write event on the same table attributes a change (event_field, write_events, insert_events, table_field). - Allow-listed tables are append-only unless skipped with a reason; every visible schema is read; settle_s delays the read for late writes; the receipt counts tables compared, append-only and skipped and binds the allow-list digest. - Another actor's later write to the bot's new row, in a column the save doesn't declare, is concurrent when the bot only inserted it; a bot write to that column still fails. A surface that is a view of a snapshotted table is judged through the table. Co-Authored-By: Claude Opus 5.5 --- .../runtime/effects/db_snapshot.py | 855 ++++++++++++++---- .../openadapt_flow/runtime/effects/frame.py | 396 ++++++-- packages/flow/openadapt_flow/verify_run.py | 269 ++++-- packages/flow/tests/test_frame_database.py | 422 +++++++-- 4 files changed, 1495 insertions(+), 447 deletions(-) diff --git a/packages/flow/openadapt_flow/runtime/effects/db_snapshot.py b/packages/flow/openadapt_flow/runtime/effects/db_snapshot.py index df12ff857b..4f1bfd776a 100644 --- a/packages/flow/openadapt_flow/runtime/effects/db_snapshot.py +++ b/packages/flow/openadapt_flow/runtime/effects/db_snapshot.py @@ -2,28 +2,38 @@ A frame that compares only the surfaces a contract reads can't see a write to anything else: a column the effects don't name, a field on a side row, a -table nobody thought to read (#1580's held-out misses). ``frame.database`` -turns the frame into a diff of the whole database instead. This module takes -the snapshot; ``frame.judge_frame`` compares two of them like any other -slice. - -The snapshot costs three statements however many tables there are, so a -database reached through a slow channel (``docker exec``) is still read in a -few seconds: - -1. the schema: every table's columns, primary key and column defaults; -2. every table's row count (one ``UNION ALL``); -3. every row of every table at or under ``max_rows``, each row one JSON - object (one ``UNION ALL``). Binary values are hex. - -A table over ``max_rows`` (a translation table, a code set) is fingerprinted -instead (``CHECKSUM TABLE`` on MySQL and MariaDB; a hash of its rows on -SQLite): a change shows that it moved, without which rows. Tables in -``allow_tables`` are not read at all; that list is the reviewed statement of -which tables the app writes on its own. +table nobody thought to read. ``frame.database`` turns the frame into a diff +of the whole database. This module takes the snapshot; ``frame.judge_frame`` +compares two of them like any other slice. + +What a snapshot holds, and why: + +- **No values on disk.** Each cell is a keyed hash (keyed BLAKE2b with the + run's salt), so a baseline is not a copy of the records. Plain values are + read only for rows the save added, and held in memory to judge them. +- **One consistent read.** Everything is read on one connection inside one + read-only transaction (``START TRANSACTION WITH CONSISTENT SNAPSHOT, READ + ONLY`` on MySQL and MariaDB; ``BEGIN`` on SQLite), so a row committed + mid-snapshot is never half-seen. The audit log is read in the same + transaction (``extra_queries``). +- **Large tables by bucket.** A table over ``max_rows`` with a single + integer key is digested in buckets of 1024 keys (salted SHA-256 of its rows' + hashes). Rows the save adds sit above the baseline's largest key and are + read in full; a changed bucket below it means an existing row moved. + A large table without an integer key gets one digest. +- **Schema and code.** Each table's definition (column types, defaults, + nullability, extras) and the database's triggers, views, routines and + events are digested; a change is reported, never ignored. +- **Counters.** ``sqlite_sequence`` and ``AUTO_INCREMENT`` show a row that + was written and deleted inside the window. +- **The clock.** ``CURRENT_TIMESTAMP`` at the snapshot, so a column the + database fills with the time can be held to the window. +- **Allow-listed tables** are append-only by default: their existing rows + must not change (the audit log is the attribution channel). ``skip`` mode + reads nothing and needs a stated reason. Read-only: SQLite files open with ``mode=ro``; a driver connection should use -a SELECT-only account. +a SELECT-only account with SELECT on every schema the app can write. """ from __future__ import annotations @@ -32,149 +42,391 @@ import json from collections.abc import Callable, Mapping, Sequence from dataclasses import dataclass, field +from decimal import Decimal, InvalidOperation from typing import Any, Literal, Optional Dialect = Literal["sqlite", "mysql"] +TableMode = Literal["compare", "append_only", "skip"] -#: Values a column holds when nothing set it. -_EMPTY = {"", "0", "0.0", "0.00", "0000-00-00", "0000-00-00 00:00:00", "NULL"} +#: Keys per bucket in a large table's digest. +BUCKET = 1024 +#: Columns per JSON object call: SQLite caps a function at 127 arguments. +_CHUNK = 50 +#: The synthetic identity field of a database slice's records. +KEY = "__row_key__" +#: A column default meaning any value: the table's own generated key. +ANY_VALUE = "\x00any" +#: A column default the database computes as the current time at insert. +CLOCK_DEFAULT = "\x00clock" +_SYSTEM_SCHEMAS = ("information_schema", "mysql", "performance_schema", "sys") + + +class SnapshotError(RuntimeError): + """The database can't be snapshotted as the frame needs.""" + + +# -- hashing ------------------------------------------------------------------ + + +def canonical(value: Any) -> str: + """One text form per value, so equal values hash equally.""" + + if value is None: + return "\x00null" + if isinstance(value, bool): + return "true" if value else "false" + if isinstance(value, (int, float, Decimal)): + number = Decimal(str(value)) + return format(number.normalize(), "f") if number == number else str(value) + if isinstance(value, (dict, list)): + return json.dumps(value, sort_keys=True, default=str) + return str(value) + + +#: Hex characters per cell MAC (32 bits). A cell's MAC is only compared +#: with the same cell's MAC, and is keyed, so it can't be forged; the row +#: key MAC is 128 bits so distinct rows never share one. +CELL_HEX = 8 + + +def cell_hash(key: bytes, value: Any) -> str: + """A cell's keyed MAC (BLAKE2b, 32 bits).""" + + return hashlib.blake2b( + canonical(value).encode(), digest_size=CELL_HEX // 2, key=key[:64] + ).hexdigest() + + +def row_key_hash(key: bytes, text: str) -> str: + """A row key's keyed MAC (BLAKE2b, 128 bits).""" + + return hashlib.blake2b(text.encode(), digest_size=16, key=key[:64]).hexdigest() + + +def unpack(columns: Sequence[str], packed: str) -> dict[str, str]: + """A stored row's cell MACs by column (stored as one string per row).""" + + return {c: packed[i * CELL_HEX : (i + 1) * CELL_HEX] for i, c in enumerate(columns)} + + +# -- shapes ------------------------------------------------------------------- @dataclass class Table: + mode: TableMode + #: Digest of the table's definition: column names, types, defaults, + #: nullability and extras. + schema: str columns: list[str] primary_key: list[str] - #: Column -> its declared default, as text (None when it has none). + #: The single integer key the database generates, when there is one. + generated_key: Optional[str] + #: Column -> declared default as text (None: no default). Schema, not data. defaults: dict[str, Optional[str]] count: int = 0 - #: The rows, or None for a table over ``max_rows`` (``digest`` then). - rows: Optional[list[dict[str, Any]]] = None + #: ``sqlite_sequence`` or ``AUTO_INCREMENT``, when the table has one. + counter: Optional[int] = None + #: The largest generated key (None for an empty table or no such key). + max_key: Optional[int] = None + #: Row key MAC -> the row's cell MACs in column order, concatenated + #: (:func:`unpack`), for a table read row by row. + rows: Optional[dict[str, str]] = None + #: Bucket -> digest of the rows at or below ``max_key`` (large and + #: append-only tables with a generated key). + buckets: Optional[dict[str, str]] = None + #: One digest of the whole table (large tables without one). digest: Optional[str] = None + #: In memory only, never saved: plain values of rows read in full, by + #: row key hash (a small table's rows; a large table's added rows). + values: dict[str, dict[str, Any]] = field(default_factory=dict, repr=False) + #: The engine keeps no snapshot isolation for it (MyISAM, MEMORY). + non_transactional: bool = False @dataclass class Snapshot: + dialect: str = "sqlite" + clock: str = "" + #: Digest of triggers, views, routines and events. + objects: str = "" tables: dict[str, Table] = field(default_factory=dict) + #: In memory only: rows of ``extra_queries``, read in the same transaction. + extra: dict[str, list[dict[str, Any]]] = field(default_factory=dict, repr=False) def to_json(self) -> dict[str, Any]: + """The persisted form: hashes and schema only, never values.""" + return { - name: { - "columns": t.columns, - "primary_key": t.primary_key, - "defaults": t.defaults, - "count": t.count, - "rows": t.rows, - "digest": t.digest, - } - for name, t in self.tables.items() + "dialect": self.dialect, + "clock": self.clock, + "objects": self.objects, + "tables": { + name: { + "mode": t.mode, + "schema": t.schema, + "columns": t.columns, + "primary_key": t.primary_key, + "generated_key": t.generated_key, + "defaults": t.defaults, + "count": t.count, + "counter": t.counter, + "max_key": t.max_key, + "rows": t.rows, + "buckets": t.buckets, + "digest": t.digest, + "non_transactional": t.non_transactional, + } + for name, t in self.tables.items() + }, } @classmethod def from_json(cls, data: Mapping[str, Any]) -> "Snapshot": return cls( + dialect=str(data.get("dialect", "sqlite")), + clock=str(data.get("clock", "")), + objects=str(data.get("objects", "")), tables={ name: Table( + mode=t["mode"], + schema=t["schema"], columns=list(t["columns"]), primary_key=list(t["primary_key"]), + generated_key=t.get("generated_key"), defaults=dict(t["defaults"]), count=int(t["count"]), - rows=None if t["rows"] is None else list(t["rows"]), + counter=t.get("counter"), + max_key=t.get("max_key"), + rows=t.get("rows"), + buckets=t.get("buckets"), digest=t.get("digest"), + non_transactional=bool(t.get("non_transactional")), ) - for name, t in data.items() - } + for name, t in data["tables"].items() + }, ) -#: A column default meaning any value: the row's own key, which every new -#: row sets. -ANY_VALUE = "\x00any" +# -- defaults ----------------------------------------------------------------- -def _computed(default: Optional[str]) -> bool: - """A default the database computes at insert (the current time).""" +def _number(text: str) -> Optional[Decimal]: + try: + return Decimal(text) + except (InvalidOperation, ValueError): + return None - text = (default or "").strip().lower().strip("()") - return text.startswith(("current_timestamp", "now", "current_date", "localtime")) +def _clock_text(value: Any) -> str: + return str(value).replace("T", " ")[:19] -def is_default(value: Any, default: Optional[str]) -> bool: - """Does a new row's ``value`` look like nothing set it?""" - if default == ANY_VALUE or value is None or _computed(default): - return True - text = str(value).strip() - if text in _EMPTY: +def is_default( + value: Any, default: Optional[str], clock: tuple[str, str] = ("", "") +) -> bool: + """Does a new row's ``value`` equal what the column holds when nothing + sets it? + + Only the declared default counts: NULL when the column has none, the + stored text otherwise (numbers compared as numbers). A default the + database computes as the time counts only when the value falls between + the two snapshots' clocks. The table's own generated key is any value. + """ + + if default == ANY_VALUE: return True + if default == CLOCK_DEFAULT: + low, high = clock + return ( + bool(low and high) + and value is not None + and (low <= _clock_text(value) <= high) + ) if default is None: + return value is None + if value is None: return False - return text == default.strip().strip("'\"") + a, b = canonical(value), default + na, nb = _number(a), _number(b) + if na is not None and nb is not None: + return na == nb + return a == b + + +def _default_text(raw: Any, dialect: Dialect) -> Optional[str]: + """A declared default as the value it stores, or ``CLOCK_DEFAULT``.""" + + if raw is None: + return None + text = str(raw).strip() + lowered = text.lower().strip("()") + if lowered.startswith( + ("current_timestamp", "now", "current_date", "localtime", "datetime('now'") + ): + return CLOCK_DEFAULT + if dialect == "mysql" and text.upper() == "NULL": + return None # MariaDB shows a NULL default as the text NULL + if len(text) >= 2 and text[0] == text[-1] and text[0] in "'\"": + return text[1:-1].replace("''", "'") + return text + + +# -- reading ------------------------------------------------------------------ def _quote(dialect: Dialect, name: str) -> str: if dialect == "mysql": - return "`" + name.replace("`", "``") + "`" + return ".".join("`" + p.replace("`", "``") + "`" for p in name.split(".", 1)) return '"' + name.replace('"', '""') + '"' -def _literal(name: str) -> str: - return "'" + name.replace("'", "''") + "'" +def _literal(text: str) -> str: + return "'" + text.replace("'", "''") + "'" -def _rows(cursor: Any) -> list[tuple[Any, ...]]: - return [tuple(r) for r in cursor.fetchall()] +class _Session: + """One connection, one read-only transaction.""" + def __init__(self, connect: Callable[[], Any], dialect: Dialect) -> None: + self.dialect = dialect + self.con = connect() + self.cur = self.con.cursor() + if dialect == "mysql": + for statement in ( + "SET SESSION information_schema_stats_expiry = 0", # MySQL 8 + "SET SESSION group_concat_max_len = 1073741824", + ): + try: + self.cur.execute(statement) + except Exception: # noqa: BLE001 - MariaDB lacks the first + pass + self.cur.execute("SET SESSION TRANSACTION ISOLATION LEVEL REPEATABLE READ") + self.cur.execute("START TRANSACTION WITH CONSISTENT SNAPSHOT, READ ONLY") + else: + self.cur.execute("BEGIN") -def _query(connect: Callable[[], Any], sql: str) -> list[tuple[Any, ...]]: - con = connect() - try: - cur = con.cursor() - cur.execute(sql) - return _rows(cur) - finally: - con.close() + def rows(self, sql: str) -> list[tuple[Any, ...]]: + self.cur.execute(sql) + return [tuple(r) for r in self.cur.fetchall()] + def dicts(self, sql: str) -> list[dict[str, Any]]: + self.cur.execute(sql) + names = [d[0] for d in self.cur.description or []] + return [dict(zip(names, r)) for r in self.cur.fetchall()] -def _schema( - connect: Callable[[], Any], dialect: Dialect -) -> dict[str, tuple[list[str], list[str], dict[str, Optional[str]], set[str]]]: - """Table -> (columns, primary key, defaults, binary columns).""" + def close(self) -> None: + try: + self.cur.execute("COMMIT" if self.dialect == "mysql" else "ROLLBACK") + finally: + self.con.close() - out: dict[str, tuple[list[str], list[str], dict[str, Optional[str]], set[str]]] - out = {} - if dialect == "mysql": - rows = _query( - connect, - "SELECT c.TABLE_NAME, c.COLUMN_NAME, c.COLUMN_KEY, c.COLUMN_DEFAULT, " - "c.DATA_TYPE FROM information_schema.COLUMNS c JOIN " - "information_schema.TABLES t ON t.TABLE_SCHEMA = c.TABLE_SCHEMA AND " - "t.TABLE_NAME = c.TABLE_NAME WHERE c.TABLE_SCHEMA = DATABASE() AND " - "t.TABLE_TYPE = 'BASE TABLE' ORDER BY c.TABLE_NAME, c.ORDINAL_POSITION", - ) - for table, column, key, default, data_type in rows: - cols, pk, defaults, binary = out.setdefault(str(table), ([], [], {}, set())) - cols.append(str(column)) - if str(key or "") == "PRI": - pk.append(str(column)) - defaults[str(column)] = None if default is None else str(default) - if "binary" in str(data_type).lower() or "blob" in str(data_type).lower(): - binary.add(str(column)) - return out - tables = [ - str(r[0]) - for r in _query( - connect, - "SELECT name FROM sqlite_master WHERE type = 'table' " - "AND name NOT LIKE 'sqlite_%' ORDER BY name", - ) - ] - for table in tables: - info = _query(connect, f"PRAGMA table_info({_quote(dialect, table)})") + +@dataclass +class _Def: + columns: list[str] + primary_key: list[str] + generated_key: Optional[str] + defaults: dict[str, Optional[str]] + binary: set[str] + schema: str + non_transactional: bool = False + + +def _digest(parts: Sequence[Any]) -> str: + return hashlib.sha256( + json.dumps(list(parts), sort_keys=True, default=str).encode() + ).hexdigest() + + +def _definitions(s: _Session) -> tuple[dict[str, _Def], str]: + """Every table's definition, and the digest of the database's code.""" + + out: dict[str, _Def] = {} + if s.dialect == "mysql": + default = s.rows("SELECT DATABASE()")[0][0] + excluded = ", ".join(_literal(x) for x in _SYSTEM_SCHEMAS) + engines = { + (str(a), str(b)): str(c or "") + for a, b, c in s.rows( + "SELECT TABLE_SCHEMA, TABLE_NAME, ENGINE FROM information_schema.TABLES " + f"WHERE TABLE_TYPE = 'BASE TABLE' AND TABLE_SCHEMA NOT IN ({excluded})" + ) + } + for row in s.rows( + "SELECT TABLE_SCHEMA, TABLE_NAME, COLUMN_NAME, COLUMN_TYPE, COLUMN_DEFAULT, " + "IS_NULLABLE, EXTRA, COLUMN_KEY, DATA_TYPE FROM information_schema.COLUMNS " + f"WHERE TABLE_SCHEMA NOT IN ({excluded}) " + "ORDER BY TABLE_SCHEMA, TABLE_NAME, ORDINAL_POSITION" + ): + schema, table, column, ctype, cdefault, nullable, extra, key, dtype = row + if (str(schema), str(table)) not in engines: + continue # a view + name = str(table) if schema == default else f"{schema}.{table}" + d = out.get(name) + if d is None: + engine = engines[(str(schema), str(table))].lower() + d = out[name] = _Def( + [], [], None, {}, set(), "", engine not in ("innodb", "") + ) + d.columns.append(str(column)) + d.defaults[str(column)] = _default_text(cdefault, "mysql") + if str(key) == "PRI": + d.primary_key.append(str(column)) + if "auto_increment" in str(extra).lower(): + d.generated_key = str(column) + if "binary" in str(dtype).lower() or "blob" in str(dtype).lower(): + d.binary.add(str(column)) + d.schema = _digest( + [d.schema, column, ctype, cdefault, nullable, extra, key] + ) + code = [ + s.rows( + "SELECT TRIGGER_SCHEMA, TRIGGER_NAME, EVENT_MANIPULATION, " + "EVENT_OBJECT_TABLE, ACTION_TIMING, ACTION_STATEMENT " + "FROM information_schema.TRIGGERS ORDER BY 1, 2" + ), + s.rows( + "SELECT TABLE_SCHEMA, TABLE_NAME, VIEW_DEFINITION " + f"FROM information_schema.VIEWS WHERE TABLE_SCHEMA NOT IN ({excluded}) " + "ORDER BY 1, 2" + ), + s.rows( + "SELECT ROUTINE_SCHEMA, ROUTINE_NAME, ROUTINE_TYPE, ROUTINE_DEFINITION " + f"FROM information_schema.ROUTINES WHERE ROUTINE_SCHEMA NOT IN ({excluded}) " + "ORDER BY 1, 2" + ), + s.rows( + "SELECT EVENT_SCHEMA, EVENT_NAME, STATUS, EVENT_DEFINITION " + "FROM information_schema.EVENTS ORDER BY 1, 2" + ), + ] + for d in out.values(): + if len(d.primary_key) != 1 or d.generated_key != d.primary_key[0]: + d.generated_key = None + return out, _digest(code) + master = s.rows("SELECT type, name, tbl_name, sql FROM sqlite_master ORDER BY 1, 2") + for kind, name, _tbl, sql in master: + if kind != "table" or str(name).startswith("sqlite_"): + continue + info = s.rows(f"PRAGMA table_info({_quote('sqlite', str(name))})") cols = [str(r[1]) for r in info] pk = [str(r[1]) for r in sorted((r for r in info if r[5]), key=lambda r: r[5])] - defaults = {str(r[1]): None if r[4] is None else str(r[4]) for r in info} - out[table] = (cols, pk, defaults, set()) - return out + generated = None + if len(pk) == 1: + ctype = next(str(r[2]) for r in info if str(r[1]) == pk[0]) + if ctype.strip().upper() == "INTEGER": + generated = pk[0] # a rowid alias: the database assigns it + out[str(name)] = _Def( + cols, + pk, + generated, + {str(r[1]): _default_text(r[4], "sqlite") for r in info}, + set(), + _digest([sql]), + ) + code = [r for r in master if r[0] in ("trigger", "view")] + return out, _digest(code) def _cell(dialect: Dialect, column: str, binary: bool) -> str: @@ -184,125 +436,358 @@ def _cell(dialect: Dialect, column: str, binary: bool) -> str: return f"CASE WHEN typeof({q}) = 'blob' THEN hex({q}) ELSE {q} END" -#: Columns per JSON object call: SQLite caps a function at 127 arguments. -_CHUNK = 50 - - -def _row_json(dialect: Dialect, cols: Sequence[str], binary: set[str]) -> list[str]: - """One row as JSON objects of at most ``_CHUNK`` columns each. - - Merged in Python, not in SQL: a JSON merge patch drops null values. - """ - +def _row_json(dialect: Dialect, d: _Def) -> list[str]: obj = "JSON_OBJECT" if dialect == "mysql" else "json_object" return [ obj + "(" + ", ".join( - f"{_literal(c)}, {_cell(dialect, c, c in binary)}" - for c in cols[i : i + _CHUNK] + f"{_literal(c)}, {_cell(dialect, c, c in d.binary)}" + for c in d.columns[i : i + _CHUNK] ) + ")" - for i in range(0, len(cols), _CHUNK) + for i in range(0, len(d.columns), _CHUNK) ] +def _read_rows( + s: _Session, name: str, d: _Def, where: str = "" +) -> list[dict[str, Any]]: + parts = _row_json(s.dialect, d) + cells = ", ".join(f"{c} AS r{i}" for i, c in enumerate(parts)) + out = [] + for chunks in s.rows(f"SELECT {cells} FROM {_quote(s.dialect, name)} {where}"): + row: dict[str, Any] = {} + for chunk in chunks: + if chunk is not None: + row.update(json.loads(chunk) if isinstance(chunk, str) else chunk) + out.append(row) + return out + + +def _row_key(d: _Def, row: Mapping[str, Any], seen: dict[str, int]) -> str: + if d.primary_key: + return "|".join(canonical(row.get(c)) for c in d.primary_key) + body = json.dumps(row, sort_keys=True, default=str) + digest = hashlib.sha256(body.encode()).hexdigest()[:16] + seen[digest] = seen.get(digest, 0) + 1 + return f"{digest}#{seen[digest]}" + + +def _hash_rows( + key: bytes, d: _Def, rows: Sequence[Mapping[str, Any]] +) -> tuple[dict[str, str], dict[str, dict[str, Any]]]: + hashed: dict[str, str] = {} + plain: dict[str, dict[str, Any]] = {} + seen: dict[str, int] = {} + for row in rows: + k = row_key_hash(key, _row_key(d, row, seen)) + hashed[k] = "".join(cell_hash(key, row.get(c)) for c in d.columns) + plain[k] = dict(row) + return hashed, plain + + +def _bucket_digests( + s: _Session, name: str, d: _Def, key: bytes, upto: int +) -> dict[str, str]: + """Salted digest per bucket of ``BUCKET`` keys, over keys <= ``upto``.""" + + k = _quote(s.dialect, d.generated_key or "") + if s.dialect == "mysql": + row = "CONCAT(" + ", ".join(_row_json("mysql", d)) + ")" + salt = _literal(key.hex()) + return { + str(b): str(h) + for b, h in s.rows( + f"SELECT {k} DIV {BUCKET} AS b, SHA2(CONCAT({salt}, GROUP_CONCAT(" + f"SHA2({row}, 256) ORDER BY {k} SEPARATOR '')), 256) " + f"FROM {_quote('mysql', name)} WHERE {k} <= {int(upto)} GROUP BY b" + ) + } + out: dict[str, hashlib._Hash] = {} + for row in _read_rows(s, name, d, f"WHERE {k} <= {int(upto)} ORDER BY {k}"): + bucket = str(int(row[d.generated_key or ""]) // BUCKET) + h = out.setdefault(bucket, hashlib.sha256(key)) + h.update(json.dumps(row, sort_keys=True, default=str).encode()) + return {b: h.hexdigest() for b, h in out.items()} + + +def _table_digest(s: _Session, name: str, d: _Def, key: bytes) -> str: + h = hashlib.sha256(key) + rows = sorted( + json.dumps(r, sort_keys=True, default=str) for r in _read_rows(s, name, d) + ) + for r in rows: + h.update(r.encode()) + return h.hexdigest() + + def take_snapshot( connect: Callable[[], Any], dialect: Dialect, *, - skip: Sequence[str] = (), + key: bytes, + modes: Mapping[str, TableMode] = {}, max_rows: int = 20000, + before: Optional[Snapshot] = None, + extra_queries: Mapping[str, str] = {}, ) -> Snapshot: - """Read every table except ``skip``. Raises when the database can't be read.""" + """Read every table in one read-only transaction. + + Args: + connect: Opens one connection (DB-API 2.0: ``cursor()``, ``close()``). + key: The run's salt, which keys every hash. + modes: Table -> ``append_only`` or ``skip`` (allow-listed tables); + every other table is compared. + max_rows: Over this many rows, a table is digested by bucket. + before: The baseline, when this is the read after the save: a large + or append-only table's existing rows are digested up to the + baseline's largest key, and rows above it are read in full. + extra_queries: Name -> a SELECT to run in the same transaction (the + audit log); rows land in :attr:`Snapshot.extra`, in memory only. + + Raises: + SnapshotError: the database can't be read as the frame needs it. + """ + + try: + s = _Session(connect, dialect) + except Exception as exc: # noqa: BLE001 + raise SnapshotError(f"could not open a read transaction: {exc}") from exc + try: + return _snapshot(s, key, modes, max_rows, before, extra_queries) + except SnapshotError: + raise + except Exception as exc: # noqa: BLE001 + raise SnapshotError(f"the database could not be read: {exc}") from exc + finally: + s.close() + - schema = _schema(connect, dialect) - names = sorted(t for t in schema if t not in set(skip)) - snap = Snapshot() +def _snapshot( + s: _Session, + key: bytes, + modes: Mapping[str, TableMode], + max_rows: int, + before: Optional[Snapshot], + extra_queries: Mapping[str, str], +) -> Snapshot: + dialect = s.dialect + snap = Snapshot( + dialect=dialect, clock=_clock_text(s.rows("SELECT CURRENT_TIMESTAMP")[0][0]) + ) + defs, snap.objects = _definitions(s) + for name, sql in extra_queries.items(): + snap.extra[name] = s.dicts(sql) + unknown = sorted(set(modes) - set(defs)) + if unknown: + raise SnapshotError( + "allow-listed tables not in the database: " + ", ".join(unknown) + ) + names = sorted(t for t in defs if modes.get(t) != "skip") + for t in sorted(t for t in defs if modes.get(t) == "skip"): + d = defs[t] + snap.tables[t] = Table("skip", d.schema, d.columns, d.primary_key, None, {}) if not names: return snap counts = { str(t): int(n) - for t, n in _query( - connect, + for t, n in s.rows( " UNION ALL ".join( f"SELECT {_literal(t)}, COUNT(*) FROM {_quote(dialect, t)}" for t in names - ), + ) ) } - small = [t for t in names if 0 < counts.get(t, 0) <= max_rows] - large = [t for t in names if counts.get(t, 0) > max_rows] + keyed = [t for t in names if defs[t].generated_key] + maxes = ( + { + str(t): (None if m is None else int(m)) + for t, m in s.rows( + " UNION ALL ".join( + f"SELECT {_literal(t)}, MAX({_quote(dialect, defs[t].generated_key or '')}) " + f"FROM {_quote(dialect, t)}" + for t in keyed + ) + ) + } + if keyed + else {} + ) + counters: dict[str, int] = {} + if dialect == "sqlite": + if any( + n == "sqlite_sequence" + for (n,) in s.rows("SELECT name FROM sqlite_master WHERE type = 'table'") + ): + counters = { + str(n): int(v) + for n, v in s.rows("SELECT name, seq FROM sqlite_sequence") + } + else: + default = s.rows("SELECT DATABASE()")[0][0] + for schema, table, value in s.rows( + "SELECT TABLE_SCHEMA, TABLE_NAME, AUTO_INCREMENT FROM information_schema.TABLES " + "WHERE AUTO_INCREMENT IS NOT NULL" + ): + name = str(table) if schema == default else f"{schema}.{table}" + counters[name] = int(value) for t in names: - cols, pk, defaults, _binary = schema[t] - snap.tables[t] = Table( - columns=cols, - primary_key=pk, - defaults=defaults, - count=counts.get(t, 0), - rows=None if t in large else [], + d = defs[t] + mode: TableMode = "append_only" if modes.get(t) == "append_only" else "compare" + old = before.tables.get(t) if before is not None else None + table = Table( + mode, + d.schema, + d.columns, + d.primary_key, + d.generated_key, + dict(d.defaults), + counts.get(t, 0), + counters.get(t), + maxes.get(t), + non_transactional=d.non_transactional, ) - if small: - parts = {t: _row_json(dialect, schema[t][0], schema[t][3]) for t in small} - width = max(len(p) for p in parts.values()) - selects = [] - for t in small: - padded = parts[t] + ["NULL"] * (width - len(parts[t])) - cells = ", ".join(f"{c} AS r{i}" for i, c in enumerate(padded)) - selects.append( - f"SELECT {_literal(t)} AS t, {cells} FROM {_quote(dialect, t)}" + if d.generated_key: + # Only the table's own generated key may hold any value. + table.defaults[d.generated_key] = ANY_VALUE + if mode == "append_only" and not d.generated_key: + raise SnapshotError( + f"append-only table {t!r} has no generated key; mark it skip with " + "a reason, or compare it" ) - for t, *chunks in _query(connect, " UNION ALL ".join(selects)): - rows = snap.tables[str(t)].rows - assert rows is not None - row: dict[str, Any] = {} - for chunk in chunks: - if chunk is not None: - row.update(json.loads(chunk) if isinstance(chunk, str) else chunk) - rows.append(row) - if large: - if dialect == "mysql": - for t, checksum in _query( - connect, - "CHECKSUM TABLE " + ", ".join(_quote(dialect, t) for t in large), - ): - name = str(t).split(".")[-1] - if name in snap.tables: - snap.tables[name].digest = str(checksum) + # Read after the save the way the baseline was read, so a table that + # crossed max_rows in the window is still compared like for like. + if old is not None and old.mode == mode and old.schema == d.schema: + by_bucket = old.buckets is not None + whole = old.digest is not None + else: + by_bucket = bool(d.generated_key) and ( + mode == "append_only" or table.count > max_rows + ) + whole = not by_bucket and table.count > max_rows + if by_bucket: + upto = (old.max_key if old is not None else table.max_key) or 0 + table.buckets = _bucket_digests(s, t, d, key, upto) + if before is not None and mode == "compare": + k = _quote(dialect, d.generated_key or "") + added = _read_rows(s, t, d, f"WHERE {k} > {int(upto)}") + _hashed, table.values = _hash_rows(key, d, added) + elif whole: + table.digest = _table_digest(s, t, d, key) else: - for t in large: - h = hashlib.sha256() - for row in _query(connect, f"SELECT * FROM {_quote(dialect, t)}"): - h.update(repr(row).encode()) - snap.tables[t].digest = h.hexdigest() + rows = _read_rows(s, t, d) if table.count else [] + table.rows, table.values = _hash_rows(key, d, rows) + snap.tables[t] = table return snap -def row_key(table: Table, row: Mapping[str, Any]) -> Optional[str]: - """A row's identity: its primary key, or None when the table has none.""" +# -- comparing ---------------------------------------------------------------- - if not table.primary_key: - return None - return "|".join(str(row.get(c)) for c in table.primary_key) +@dataclass +class TableDiff: + """How one table moved between two snapshots. Counts and column names + only, never values.""" + + added: int = 0 + removed: int = 0 + changed: int = 0 + changed_columns: list[str] = field(default_factory=list) + #: Moved, but not row by row: why (a schema change, a changed bucket of a + #: large table, an append-only table whose existing rows moved, a + #: counter that moved past the rows added). + unread: list[str] = field(default_factory=list) + + +def unread_reasons( + old: Optional[Table], new: Optional[Table], dialect: str = "sqlite" +) -> list[str]: + """Why a table moved in a way no row comparison explains; [] if none.""" + + if old is None or new is None: + return ["created or dropped"] + if old.mode != new.mode: + return ["compared differently"] + if old.mode == "skip": + return [] + if old.schema != new.schema or old.columns != new.columns: + return ["definition changed"] + reasons = [] + if old.buckets is not None or new.buckets is not None: + if old.buckets != new.buckets: + reasons.append("existing rows changed (by bucket)") + elif old.digest is not None or new.digest is not None: + if old.digest is None or new.digest is None: + raise SnapshotError("a large table has no digest") + if old.digest != new.digest: + reasons.append("rows changed (whole-table digest)") + if new.counter is not None: + # sqlite_sequence holds the last key used; AUTO_INCREMENT the next. + step = 1 if dialect == "mysql" else 0 + expected = max( + v + for v in ( + old.counter or 0, + 0 if new.max_key is None else new.max_key + step, + ) + ) + if new.counter > expected: + reasons.append("a row was written and removed (counter moved)") + return reasons -def keyed_rows(table: Table) -> list[dict[str, Any]]: - """The rows, each carrying ``_key``: the primary key, or for a table with - none, the row's own content numbered by occurrence (so a doubled - identical row still counts twice).""" - out: list[dict[str, Any]] = [] - seen: dict[str, int] = {} - for row in table.rows or []: - key = row_key(table, row) - if key is None: - body = json.dumps(row, sort_keys=True, default=str) - digest = hashlib.sha256(body.encode()).hexdigest()[:16] - seen[digest] = seen.get(digest, 0) + 1 - key = f"{digest}#{seen[digest]}" - out.append({**row, KEY: key}) +def diff( + before: Snapshot, + after: Snapshot, + *, + key: bytes, + allow_tables: Sequence[str] = (), +) -> dict[str, TableDiff]: + """Every table that moved between ``before`` and ``after``, except + ``allow_tables`` and skipped ones. Empty when the database is back where + it was. Database code (triggers, views, routines, events) that changed + shows under ``"(database code)"``.""" + + out: dict[str, TableDiff] = {} + if before.objects != after.objects: + out["(database code)"] = TableDiff(unread=["changed"]) + skip = set(allow_tables) + for name in sorted(set(before.tables) | set(after.tables)): + if name in skip: + continue + a, b = before.tables.get(name), after.tables.get(name) + reasons = unread_reasons(a, b, after.dialect) + moved = TableDiff(unread=reasons) + if ( + a is not None + and b is not None + and a.mode == "compare" + and not ( + reasons and reasons[0] in ("definition changed", "created or dropped") + ) + ): + old = {k: unpack(a.columns, v) for k, v in (a.rows or {}).items()} + new = ( + {k: unpack(b.columns, v) for k, v in b.rows.items()} + if b.rows is not None + else { + k: {c: cell_hash(key, v.get(c)) for c in b.columns} + for k, v in b.values.items() + } + ) + if a.rows is not None: + moved.added = len(new.keys() - old.keys()) + moved.removed = len(old.keys() - new.keys()) + else: + moved.added = len(new) + columns: set[str] = set() + for k in old.keys() & new.keys(): + fields = [c for c in b.columns if old[k].get(c) != new[k].get(c)] + if fields: + moved.changed += 1 + columns.update(fields) + moved.changed_columns = sorted(columns) + if moved.added or moved.removed or moved.changed or moved.unread: + out[name] = moved return out - - -#: The synthetic identity field of a database slice's records. -KEY = "__row_key__" diff --git a/packages/flow/openadapt_flow/runtime/effects/frame.py b/packages/flow/openadapt_flow/runtime/effects/frame.py index 8c0711fdf6..0d205ac5f7 100644 --- a/packages/flow/openadapt_flow/runtime/effects/frame.py +++ b/packages/flow/openadapt_flow/runtime/effects/frame.py @@ -46,7 +46,7 @@ import hashlib import json -from collections.abc import Mapping, Sequence +from collections.abc import Callable, Mapping, Sequence from dataclasses import dataclass, field from typing import Any, Literal, Optional @@ -106,6 +106,20 @@ class FrameAttribution(BaseModel): #: record when it isn't ``record_key`` (OpenEMR's ``contact`` row points #: at its chart through ``foreign_id``). record_keys: dict[str, str] = Field(default_factory=dict) + #: ``audit_read`` only: the entry field naming what the entry did, and + #: the values that are writes. Set, only write entries attribute a change: + #: an entry for viewing a record never excuses a write to it. Required + #: with ``frame.database``. + event_field: Optional[str] = None + write_events: list[str] = Field(default_factory=list) + #: The write events that create a record (a subset of ``write_events``). + #: Another actor's later write to a record the bot created can be told + #: apart from the bot's own insert only through these. + insert_events: list[str] = Field(default_factory=list) + #: ``audit_read`` only: the entry field naming the table written. Set, + #: an entry attributes a change only to a row of that table. Required + #: with ``frame.database``. + table_field: Optional[str] = None #: ``record_field`` only: a version, revision or ETag on each record. version_field: Optional[str] = None #: ``record_field`` only: the record fields this app sets on the server @@ -136,8 +150,14 @@ def _shape(self) -> "FrameAttribution": raise ValueError("version_field applies to record_field attribution") if self.server_set: raise ValueError("server_set applies to record_field attribution") - elif self.record_keys: - raise ValueError("record_keys applies to audit_read attribution") + if bool(self.event_field) != bool(self.write_events): + raise ValueError("event_field and write_events go together") + if not set(self.insert_events) <= set(self.write_events): + raise ValueError("insert_events must be among write_events") + elif self.record_keys or self.write_events or self.table_field: + raise ValueError( + "record_keys, write_events and table_field apply to audit_read" + ) else: if self.read is not None or self.record_field is not None: raise ValueError( @@ -177,6 +197,25 @@ class FrameSideRecord(BaseModel): count: int = Field(default=1, ge=1) +class AllowedTable(BaseModel): + """One allow-listed table: the app writes it on its own.""" + + model_config = ConfigDict(extra="forbid") + + table: str + #: ``append_only`` (the default): rows may be added, existing rows must + #: not change, and the table needs a generated key. ``skip``: not read at + #: all, which needs a ``reason``. + mode: Literal["append_only", "skip"] = "append_only" + reason: Optional[str] = None + + @model_validator(mode="after") + def _skip_needs_reason(self) -> "AllowedTable": + if self.mode == "skip" and not (self.reason or "").strip(): + raise ValueError(f"skipping table {self.table!r} needs a reason") + return self + + class FrameDatabase(BaseModel): """``frame.database``: diff the whole database, not only what is read. @@ -189,7 +228,9 @@ class FrameDatabase(BaseModel): ``allow_tables`` replaces a read-list with a reviewed allow-list: the tables the app writes on its own for any signed-in user (logs, sessions, - id registries). Everything else is compared. + id registries). They are append-only unless an entry says ``skip`` with a + reason. Everything else is compared. The receipt counts tables compared, + append-only and skipped, and binds the list's digest. """ model_config = ConfigDict(extra="forbid") @@ -199,8 +240,9 @@ class FrameDatabase(BaseModel): #: ``sql_connect_args`` and ``sql_password_env``, as in a ``kind: sql`` #: verifier (no query: every table is read). read: dict[str, Any] - #: Tables not compared: the app's own bookkeeping. Reviewed per app. - allow_tables: list[str] = Field(default_factory=list) + #: The app's own bookkeeping, reviewed per app: a table name + #: (append-only) or ``{table, mode, reason}``. + allow_tables: list[AllowedTable] = Field(default_factory=list) #: Table -> the surface whose records it holds (``patient_data`` -> #: ``patient``): an effect routed to the surface declares rows here too. views: dict[str, str] = Field(default_factory=dict) @@ -208,8 +250,33 @@ class FrameDatabase(BaseModel): #: the effects name (ids, creation dates, columns the app's form fills #: on its own). Any other column must hold its default. new_row_fields: dict[str, list[str]] = Field(default_factory=dict) - #: Tables over this many rows are fingerprinted, not compared row by row. + #: Table -> columns that change on their own on every row (a last-seen + #: time). ``frame.ignore_fields`` applies to surfaces only. + ignore_columns: dict[str, list[str]] = Field(default_factory=dict) + #: Over this many rows a table is digested by key bucket: its existing + #: rows by digest, the rows the save adds in full. max_rows: int = Field(default=20000, ge=1) + #: Seconds to wait after the save before the read, so writes the app + #: makes a moment later (background jobs) fall inside the window. + settle_s: float = Field(default=0.0, ge=0) + + @model_validator(mode="before") + @classmethod + def _names(cls, data: Any) -> Any: + if isinstance(data, dict) and isinstance(data.get("allow_tables"), list): + data = dict(data) + data["allow_tables"] = [ + {"table": t} if isinstance(t, str) else t for t in data["allow_tables"] + ] + return data + + def modes(self) -> dict[str, str]: + return {t.table: t.mode for t in self.allow_tables} + + def allow_list_sha256(self) -> str: + payload = [t.model_dump(mode="json") for t in self.allow_tables] + canonical = json.dumps(payload, sort_keys=True, separators=(",", ":")) + return "sha256:" + hashlib.sha256(canonical.encode("utf-8")).hexdigest() @model_validator(mode="after") def _connection(self) -> "FrameDatabase": @@ -239,6 +306,22 @@ class FrameSpec(BaseModel): #: Diff the whole database (``FrameDatabase``). database: Optional[FrameDatabase] = None + @model_validator(mode="after") + def _database_attribution(self) -> "FrameSpec": + a = self.attribution + if ( + self.database is not None + and a is not None + and a.kind == "audit_read" + and not (a.write_events and a.table_field) + ): + raise ValueError( + "with frame.database, audit attribution needs event_field, " + "write_events and table_field: only a write to the same table " + "attributes a change" + ) + return self + @model_validator(mode="after") def _identity_for_audit(self) -> "FrameSpec": if ( @@ -288,6 +371,9 @@ class FrameVerdict: records_compared: int = 0 collateral: list[FrameChange] = field(default_factory=list) concurrent: list[FrameChange] = field(default_factory=list) + #: ``frame.database`` only: tables compared, append-only and skipped, + #: and the allow-list's digest. + database: Optional[dict[str, Any]] = None @property def held(self) -> bool: @@ -311,6 +397,7 @@ def evidence(self, spec: FrameSpec) -> dict[str, Any]: "target_changed_by_other": sum( 1 for c in self.collateral if c.other_on_target ), + **({"database": self.database} if self.database is not None else {}), } @@ -405,7 +492,10 @@ def judge_frame( run_actors: Sequence[str] = (), identities: Optional[Mapping[str, str]] = None, new_row_defaults: Optional[Mapping[str, Mapping[str, Optional[str]]]] = None, - unread_changes: Sequence[str] = (), + unread_changes: Sequence[tuple[str, str]] = (), + hashers: Optional[Mapping[str, Callable[[Any], str]]] = None, + clock: tuple[str, str] = ("", ""), + database_evidence: Optional[dict[str, Any]] = None, ) -> FrameVerdict: """Judge the frame of ``effects`` over ``slices``. @@ -426,11 +516,19 @@ def judge_frame( ``spec.identity_field`` (a database table's row key). new_row_defaults: Slice -> column defaults, for slices whose new declared and side rows must set nothing undeclared (database - tables, ``spec.database``). - unread_changes: Tables that moved but were fingerprinted, not read - row by row (or whose schema changed). Each is collateral. + tables). ``db_snapshot.ANY_VALUE`` marks the generated key and + ``CLOCK_DEFAULT`` a column the database fills with the time. + unread_changes: (table, why) for tables that moved in a way no row + comparison explains. Each is collateral. + hashers: Slice -> the keyed hash its baseline records hold instead of + values (database tables). A post-save record is plain; it is + hashed to compare with the baseline. + clock: The database clock at the baseline and after the save. + database_evidence: Counts for the receipt (tables compared, ...). """ + from openadapt_flow.runtime.effects.db_snapshot import KEY, is_default + attribution = spec.attribution mode: str = attribution.kind if attribution is not None else "none" self_ids: set[str] = set() @@ -444,12 +542,12 @@ def judge_frame( ) self_ids = resolved | {str(a) for a in run_actors if str(a)} - from openadapt_flow.runtime.effects.db_snapshot import KEY, is_default - - ignore = set(spec.ignore_fields) | {KEY} + hashers = hashers or {} + database = spec.database + surface_ignore = set(spec.ignore_fields) | {KEY} if attribution is not None and attribution.kind == "record_field": # Server-set bookkeeping moves on every write; it says who, not what. - ignore |= set(attribution.server_set) + surface_ignore |= set(attribution.server_set) written = declared_fields(effects) may_change = set(spec.target_fields) if spec.target_fields is not None else written if attribution is not None and attribution.kind == "record_field": @@ -462,6 +560,13 @@ def judge_frame( attribution = None mode = "attribution_refused" + def ignore_for(name: str) -> set[str]: + if name in hashers: + # Database tables: only the columns declared for that table. + cols = database.ignore_columns.get(name, []) if database else [] + return set(cols) | {KEY} + return surface_ignore + def identity_of(name: str) -> Optional[str]: if identities is not None and name in identities: return identities[name] @@ -472,9 +577,25 @@ def key_of(record: Mapping[str, Any], name: str = "") -> Any: if identity: value = record.get(identity) return None if value is None else str(value) - return stable_id(_without(record, ignore)) + return stable_id(_without(record, ignore_for(name))) + + def changed( + name: str, before: Mapping[str, Any], after: Mapping[str, Any] + ) -> list[str]: + """Fields that differ. A database baseline holds hashes, so the + post-save value is hashed to compare.""" + + ignore = ignore_for(name) + hasher = hashers.get(name) + if hasher is None: + return _changed_fields(before, after, ignore) + return sorted( + k + for k in set(before) | set(after) + if k not in ignore and before.get(k) != hasher(after.get(k)) + ) - row_fields = spec.database.new_row_fields if spec.database is not None else {} + row_fields = database.new_row_fields if database is not None else {} def undeclared_columns( name: str, record: Mapping[str, Any], also: Sequence[str] = () @@ -484,35 +605,80 @@ def undeclared_columns( defaults = (new_row_defaults or {}).get(name) if defaults is None: return [] - allowed = written | set(row_fields.get(name, ())) | ignore | set(also) + allowed = written | set(row_fields.get(name, ())) | ignore_for(name) | set(also) return sorted( c for c, v in record.items() - if c not in allowed and not is_default(v, defaults.get(c)) + if c not in allowed and not is_default(v, defaults.get(c), clock) ) + # The audit window, kept to write entries when the frame says which they + # are: an entry for viewing a record never excuses a write to it. window_entries: Optional[list[dict[str, Any]]] = None if mode == "audit_read": if audit_before is not None and audit_after is not None: seen = {stable_id(e) for e in audit_before} window_entries = [e for e in audit_after if stable_id(e) not in seen] + if attribution is not None and attribution.write_events: + writes = set(attribution.write_events) + window_entries = [ + e + for e in window_entries + if str(get_path(e, attribution.event_field or "")) in writes + ] + + surface_tables = {v: k for k, v in (database.views if database else {}).items()} + + def table_of(name: str) -> Optional[str]: + return name if name in hashers else surface_tables.get(name) + + def entries_for(name: str, ref: Any, hashed: bool) -> list[dict[str, Any]]: + """Window entries that name this record (and its table, when the + attribution says which table an entry wrote).""" + + if window_entries is None or ref is None or attribution is None: + return [] + hasher = hashers.get(name) if hashed else None + table = table_of(name) + out = [] + for e in window_entries: + entry_ref = get_path(e, attribution.record_field or "") + if entry_ref in (None, ""): + continue + if (hasher(entry_ref) if hasher else str(entry_ref)) != str(ref): + continue + if attribution.table_field: + if table is None or str(get_path(e, attribution.table_field)) != table: + continue + if get_path(e, attribution.actor_field) in (None, ""): + continue + out.append(e) + return out def ref_of( name: str, key: Any, before: Optional[Mapping[str, Any]], after: Optional[Mapping[str, Any]], - ) -> Any: - """What an audit entry names for this record (``record_key``).""" + ) -> tuple[Any, bool]: + """What an audit entry names for this record, and whether it is a + baseline hash (the record is gone from the post-save read).""" if attribution is None: - return key + return key, False field = attribution.record_keys.get(name, attribution.record_key) if not field: - return key + return key, name in hashers and after is None record = after if after is not None else before value = None if record is None else record.get(field) - return None if value in (None, "") else str(value) + hashed = name in hashers and after is None + return (None if value in (None, "") else str(value)), hashed + + def actors(name: str, ref: Any, hashed: bool) -> set[str]: + return { + str(get_path(e, attribution.actor_field)) # type: ignore[union-attr] + for e in entries_for(name, ref, hashed) + } def other_on_target( name: str, @@ -524,21 +690,32 @@ def other_on_target( if attribution is None: return False - key = ref_of(name, key, before, after) if attribution.kind == "audit_read": - return window_entries is not None and any( - str(get_path(e, attribution.record_field or "")) == str(key) - and get_path(e, attribution.actor_field) not in (None, "") - and str(get_path(e, attribution.actor_field)) not in self_ids - for e in window_entries - ) + ref, hashed = ref_of(name, key, before, after) + return bool(actors(name, ref, hashed) - self_ids) if after is None: return False - if before is not None and not _changed_fields(before, after, ignore): + if before is not None and not changed(name, before, after): return False # untouched in the window actor = get_path(after, attribution.actor_field) return actor not in (None, "") and str(actor) not in self_ids + def bot_only_inserted(name: str, ref: Any) -> bool: + """The bot's write entries for this record are inserts only, so a + later write to it by another actor is theirs, not the bot's.""" + + if attribution is None or not attribution.insert_events: + return False + inserts = set(attribution.insert_events) + mine = [ + e + for e in entries_for(name, ref, False) + if str(get_path(e, attribution.actor_field)) in self_ids + ] + return all( + str(get_path(e, attribution.event_field or "")) in inserts for e in mine + ) + def attribute( name: str, key: Any, @@ -548,18 +725,13 @@ def attribute( if attribution is None: return "unattributed" if attribution.kind == "audit_read": - key = ref_of(name, key, before, after) - if window_entries is None or key is None: + ref, hashed = ref_of(name, key, before, after) + if window_entries is None or ref is None: return "unattributed" - actors = { - str(get_path(e, attribution.actor_field)) - for e in window_entries - if str(get_path(e, attribution.record_field or "")) == str(key) - and get_path(e, attribution.actor_field) not in (None, "") - } - if actors & self_ids: + who = actors(name, ref, hashed) + if who & self_ids: return "self" - return "other" if actors else "unattributed" + return "other" if who else "unattributed" # record_field: the record says who last changed it. if after is None: return "unattributed" # a removed record can't say who removed it @@ -604,6 +776,25 @@ def selectors_for(name: str) -> list[Any]: ] return [s for effect in mine for s in declared_selectors(effect)] + def new_row( + name: str, key: Any, after: Mapping[str, Any], also: Sequence[str] = () + ) -> None: + """A new declared or side row may set only declared columns. Columns + another actor set later, by a write to this table, while the bot only + inserted the row, are that actor's (concurrent).""" + + extra = undeclared_columns(name, after, also) + if not extra: + return + ref, _hashed = ref_of(name, key, None, after) + others = actors(name, ref, False) - self_ids + change = FrameChange(name, "added", "self", extra, on_target=True, key=key) + if others and bot_only_inserted(name, ref): + change.attribution = "other" + concurrent.append(change) + else: + collateral.append(change) + # Side records: the links each new declared record gives its side rows. side_links: list[set[str]] = [] side_found: list[dict[str, int]] = [] @@ -630,14 +821,19 @@ def selectors_for(name: str) -> list[Any]: ) if after_records is None: return _not_checked("the slice could not be read after the save", mode) - mine = [ - effect - for index, effect in enumerate(effects) - if routes is None or name in routes.get(str(index), (name,)) - ] - selectors = [s for effect in mine for s in declared_selectors(effect)] - - def declared(record: Mapping[str, Any]) -> bool: + if surface_tables.get(name) in hashers: + # A surface whose table the frame snapshots: the table, with its + # columns, defaults and write attribution, judges these records. + continue + selectors = selectors_for(name) + hasher = hashers.get(name) + + def declared(record: Mapping[str, Any], hashed: bool = False) -> bool: + if hashed and hasher is not None: + return any( + all(record.get(k) == hasher(v) for k, v in s.items()) + for s in selectors + ) return any(record_matches(dict(record), s) for s in selectors) before_map: dict[Any, dict[str, Any]] = {} @@ -666,46 +862,62 @@ def declared(record: Mapping[str, Any]) -> bool: for key in set(before_map) | set(after_map): before = before_map.get(key) after = after_map.get(key) - is_declared = (before is not None and declared(before)) or ( - after is not None and declared(after) - ) + is_declared = ( + before is not None and declared(before, hashed=hasher is not None) + ) or (after is not None and declared(after)) if is_declared: - declared_keys.add(str(ref_of(name, key, before, after))) + declared_keys.add(str(ref_of(name, key, before, after)[0])) + if before is None and after is not None: + # A new declared record. In a database table, with write + # and insert events to tell actors apart, anything it sets + # beyond the declared fields must be its default or + # another actor's later write. Elsewhere nothing says + # which fields another actor touched: their entry blocks. + separable = ( + name in hashers + and attribution is not None + and bool(attribution.insert_events) + ) + if not separable and other_on_target(name, key, None, after): + collateral.append( + FrameChange( + name, + "changed", + "other", + [], + on_target=True, + other_on_target=True, + key=key, + ) + ) + continue + new_row(name, key, after) + continue + if after is None: + continue # a removed target: the delta check's case + fields = changed(name, before, after) # type: ignore[arg-type] + stray = [f for f in fields if f not in may_change] if other_on_target(name, key, before, after): - # The saved value may not stand: never excused. + if fields and not (set(fields) & may_change): + # Another actor wrote only fields this save doesn't + # declare: theirs. A declared field moved by another + # actor never passes: the saved value may not stand. + concurrent.append( + FrameChange(name, "changed", "other", fields, key=key) + ) + continue collateral.append( FrameChange( name, "changed", "other", - _changed_fields(before or {}, after or {}, ignore), + fields, on_target=True, other_on_target=True, key=key, ) ) continue - if before is None and after is not None: - extra = undeclared_columns(name, after) - if extra: - # The new record sets a column nothing declares. - collateral.append( - FrameChange( - name, - "added", - attribute(name, key, None, after), - extra, - on_target=True, - key=key, - ) - ) - if before is None or after is None: - continue # a new or removed target: the delta check's case - stray = [ - f - for f in _changed_fields(before, after, ignore) - if f not in may_change - ] if stray: note( FrameChange( @@ -733,26 +945,15 @@ def declared(record: Mapping[str, Any]) -> bool: side = spec.side_records[side_index] link = str(after.get(side.link_field)) side_found[side_index][link] += 1 - declared_keys.add(str(ref_of(name, key, None, after))) - extra = undeclared_columns(name, after, (side.link_field,)) - if extra: - collateral.append( - FrameChange( - name, - "added", - attribute(name, key, None, after), - extra, - on_target=True, - key=key, - ) - ) + declared_keys.add(str(ref_of(name, key, None, after)[0])) + new_row(name, key, after, (side.link_field,)) continue note( FrameChange( name, "added", attribute(name, key, None, after), - sorted(_without(after, ignore)), + sorted(_without(after, ignore_for(name))), key=key, ) ) @@ -763,7 +964,7 @@ def declared(record: Mapping[str, Any]) -> bool: ) ) elif before is not None and after is not None: - fields = _changed_fields(before, after, ignore) + fields = changed(name, before, after) if fields: note( FrameChange( @@ -775,12 +976,10 @@ def declared(record: Mapping[str, Any]) -> bool: ) ) - for table in unread_changes: - # Fingerprinted, not read row by row: it moved, and nothing can say - # which rows or who moved them. - collateral.append( - FrameChange(table, "changed", "unattributed", ["rows not compared"]) - ) + for table, why in unread_changes: + # Moved in a way no row comparison explains: nothing can say which + # rows, or who moved them. + collateral.append(FrameChange(table, "changed", "unattributed", [why])) for i, side in enumerate(spec.side_records): for link, found in side_found[i].items(): @@ -822,6 +1021,7 @@ def declared(record: Mapping[str, Any]) -> bool: records_compared=compared, collateral=collateral, concurrent=concurrent, + database=database_evidence, ) verdict.reason = _reason(verdict) return verdict diff --git a/packages/flow/openadapt_flow/verify_run.py b/packages/flow/openadapt_flow/verify_run.py index 97b0bf8e63..146dc3df3c 100644 --- a/packages/flow/openadapt_flow/verify_run.py +++ b/packages/flow/openadapt_flow/verify_run.py @@ -51,6 +51,8 @@ import os import re import secrets +import time +from dataclasses import dataclass from datetime import datetime, timezone from pathlib import Path from typing import Any, Literal, Mapping, Optional, Sequence, cast @@ -431,7 +433,36 @@ def _db_connect(read: Mapping[str, Any]) -> Any: return lambda: module.connect(**args) -def _read_database(cfg: VerifyConfig) -> Optional[dict[str, Any]]: +def _same_connection( + audit: Optional[Mapping[str, Any]], read: Mapping[str, Any] +) -> bool: + """Does the audit read use the database read's own connection?""" + + if not audit or audit.get("kind") != "sql": + return False + keys = ("sqlite_database", "sql_driver", "sql_connect_args") + return all(audit.get(k) == read.get(k) for k in keys) + + +def _audit_sql(cfg: VerifyConfig) -> dict[str, str]: + """The audit query, when it can run inside the database snapshot.""" + + frame = cfg.frame + if frame is None or frame.database is None or frame.attribution is None: + return {} + audit = frame.attribution.read + if ( + _same_connection(audit, frame.database.read) + and audit + and audit.get("sql_query") + ): + return {"audit": str(audit["sql_query"])} + return {} + + +def _read_database( + cfg: VerifyConfig, salt: bytes, before: Optional[Any] = None +) -> Optional[Any]: """The whole-database snapshot, or None when it can't be read.""" from openadapt_flow.runtime.effects.db_snapshot import take_snapshot @@ -440,69 +471,140 @@ def _read_database(cfg: VerifyConfig) -> Optional[dict[str, Any]]: if database is None: return None try: - snap = take_snapshot( + return take_snapshot( _db_connect(database.read), database.dialect, - skip=database.allow_tables, + key=salt, + modes=database.modes(), # type: ignore[arg-type] max_rows=database.max_rows, + before=before, + extra_queries=_audit_sql(cfg), ) except Exception: # noqa: BLE001 - unreadable -> the frame is not checked return None - return snap.to_json() -DatabaseSlices = tuple[ - dict[str, tuple[EffectState, Optional[list[dict[str, Any]]]]], - dict[str, dict[str, Optional[str]]], - list[str], -] +def _audit_ids_each(entries: Sequence[Mapping[str, Any]], salt: bytes) -> list[str]: + from openadapt_flow.runtime.effects.effect import stable_id + + return [ + hmac.new(salt, repr(stable_id(dict(e))).encode(), hashlib.sha256).hexdigest() + for e in entries + ] + + +def _audit_ids(entries: Sequence[Mapping[str, Any]], salt: bytes) -> list[str]: + """Keyed hashes of audit entries: the baseline keeps which entries + existed, never what they say.""" + + from openadapt_flow.runtime.effects.effect import stable_id + + return sorted( + hmac.new(salt, repr(stable_id(dict(e))).encode(), hashlib.sha256).hexdigest() + for e in entries + ) -def _database_slices( - cfg: VerifyConfig, before: EffectState -) -> Optional[DatabaseSlices]: - """Slices for every table, their column defaults, and tables moved unread. +@dataclass +class _DatabaseFrame: + slices: dict[str, tuple[EffectState, Optional[list[dict[str, Any]]]]] + defaults: dict[str, dict[str, Optional[str]]] + unread: list[tuple[str, str]] + hashers: dict[str, Any] + clock: tuple[str, str] + evidence: dict[str, Any] + audit_after: Optional[list[dict[str, Any]]] + + +def _database_frame( + cfg: VerifyConfig, before: EffectState, salt: bytes +) -> Optional[_DatabaseFrame]: + """Every table as a slice, from the baseline snapshot and one read now. None when there is no ``frame.database``. A baseline without a readable - snapshot, or an unreadable post-save read, gives one unreachable slice, so - the frame is not checked. + snapshot, or an unreadable read now, gives one unreachable slice, so the + frame is not checked. """ from openadapt_flow.runtime.effects.db_snapshot import ( - ANY_VALUE, + KEY, Snapshot, - keyed_rows, + SnapshotError, + cell_hash, + unpack, + unread_reasons, ) - if cfg.frame is None or cfg.frame.database is None: + database = cfg.frame.database if cfg.frame is not None else None + if database is None: return None + unreachable = _DatabaseFrame( + {"database": (EffectState(substrate="database", reachable=False), None)}, + {}, + [], + {}, + ("", ""), + {}, + None, + ) stored = ( before.detail.get(FRAME_DB_DETAIL) if isinstance(before.detail, dict) else None ) - after_json = _read_database(cfg) - if not isinstance(stored, dict) or after_json is None: - unreachable = EffectState(substrate="database", reachable=False, records=[]) - return {"database": (unreachable, None)}, {}, [] - old, new = Snapshot.from_json(stored), Snapshot.from_json(after_json) - slices: dict[str, tuple[EffectState, Optional[list[dict[str, Any]]]]] = {} - defaults: dict[str, dict[str, Optional[str]]] = {} - unread: list[str] = [] + if not isinstance(stored, dict): + return unreachable + old = Snapshot.from_json(stored) + if database.settle_s: + time.sleep(database.settle_s) + new = _read_database(cfg, salt, before=old) + if new is None: + return unreachable + out = _DatabaseFrame({}, {}, [], {}, (old.clock, new.clock), {}, None) + if "audit" in new.extra: + out.audit_after = list(new.extra["audit"]) + if old.objects != new.objects: + out.unread.append( + ("(database code)", "triggers, views, routines or events changed") + ) + hasher = lambda value: cell_hash(salt, value) # noqa: E731 + counts = {"compare": 0, "append_only": 0, "skip": 0} for name in sorted(set(old.tables) | set(new.tables)): a, b = old.tables.get(name), new.tables.get(name) - if a is None or b is None or a.columns != b.columns: - unread.append(name) # created, dropped or altered in the window - continue - if a.rows is None or b.rows is None: - if (a.count, a.digest) != (b.count, b.digest): - unread.append(name) + try: + reasons = unread_reasons(a, b, new.dialect) + except SnapshotError as exc: + reasons = [str(exc)] + out.unread.extend((name, why) for why in reasons) + if b is not None: + counts[b.mode] = counts.get(b.mode, 0) + 1 + if ( + a is None + or b is None + or a.mode != "compare" + or (reasons and reasons[0] in ("definition changed", "created or dropped")) + ): continue - slices[name] = ( - EffectState(substrate="database", reachable=True, records=keyed_rows(a)), - keyed_rows(b), + if b.rows is None and b.buckets is None: + continue # a whole-table digest: moved or not, nothing row by row + before_rows = [ + {KEY: k, **unpack(a.columns, cells)} for k, cells in (a.rows or {}).items() + ] + after_rows = [{KEY: k, **row} for k, row in b.values.items()] + out.slices[name] = ( + EffectState(substrate="database", reachable=True, records=before_rows), + after_rows, ) - # The primary key is the row's identity: every new row sets it. - defaults[name] = {**b.defaults, **dict.fromkeys(b.primary_key, ANY_VALUE)} - return slices, defaults, unread + out.defaults[name] = dict(b.defaults) + out.hashers[name] = hasher + out.evidence = { + "tables_compared": counts.get("compare", 0), + "tables_append_only": counts.get("append_only", 0), + "tables_skipped": counts.get("skip", 0), + "tables_without_snapshot_isolation": sum( + 1 for t in new.tables.values() if t.non_transactional + ), + "allow_list_sha256": database.allow_list_sha256(), + } + return out def _read_audit(verifier: Any) -> Optional[list[dict[str, Any]]]: @@ -523,6 +625,7 @@ def judge_run_frame( *, audit_verifier: Any = None, run_actors: Sequence[str] = (), + salt: bytes = b"", ) -> FrameVerdict: """Read the slice after the save and judge the frame against ``before``.""" @@ -575,14 +678,11 @@ def read_of(name: str) -> str: else: slices[""] = (before, _post_records(after)) identities: Optional[dict[str, str]] = None - defaults: Optional[dict[str, dict[str, Optional[str]]]] = None - unread: list[str] = [] - database = _database_slices(cfg, before) - if database is not None: + db = _database_frame(cfg, before, salt) + if db is not None: from openadapt_flow.runtime.effects.db_snapshot import KEY - db_slices, defaults, unread = database - clash = sorted(set(db_slices) & set(slices)) + clash = sorted(set(db.slices) & set(slices)) if clash: return FrameVerdict( status="not_checked", @@ -594,19 +694,34 @@ def read_of(name: str) -> str: routes = {k: list(v) for k, v in routes.items()} for own in routes.values(): own.extend(t for t, surface in views.items() if surface in own) - slices.update(db_slices) - identities = {name: KEY for name in db_slices} + slices.update(db.slices) + identities = {name: KEY for name in db.slices} audit_before: Optional[list[dict[str, Any]]] = None audit_after: Optional[list[dict[str, Any]]] = None - if audit_verifier is not None: - stored = ( - before.detail.get(FRAME_AUDIT_DETAIL) - if isinstance(before.detail, dict) - else None + stored = ( + before.detail.get(FRAME_AUDIT_DETAIL) + if isinstance(before.detail, dict) + else None + ) + if isinstance(stored, dict) and stored.get("reachable"): + entries_now = ( + db.audit_after + if db is not None and db.audit_after is not None + else _read_audit(audit_verifier) ) - if isinstance(stored, dict) and stored.get("reachable"): - audit_before = list(stored.get("records", [])) - audit_after = _read_audit(audit_verifier) + if entries_now is not None: + if "ids" in stored: + # The window: entries now that the baseline didn't hold. + seen = set(stored["ids"]) + audit_before = [] + audit_after = [ + e + for e, h in zip(entries_now, _audit_ids_each(entries_now, salt)) + if h not in seen + ] + else: + audit_before = list(stored.get("records", [])) + audit_after = entries_now return judge_frame( spec, bound_effects, @@ -617,8 +732,11 @@ def read_of(name: str) -> str: audit_after=audit_after, run_actors=run_actors, identities=identities, - new_row_defaults=defaults, - unread_changes=unread, + new_row_defaults=None if db is None else db.defaults, + unread_changes=() if db is None else db.unread, + hashers=None if db is None else db.hashers, + clock=("", "") if db is None else db.clock, + database_evidence=None if db is None else db.evidence, ) @@ -814,36 +932,36 @@ def capture_baseline( identity = verifier_identity(verifier) execution_id = _new_execution_id() _tag_reads(verifier, execution_id) + salt = secrets.token_bytes(32) state = verifier.capture_pre_state() if not isinstance(state, EffectState): raise VerifyRunError("verifier returned no readable baseline") - audit_verifier = _audit_verifier(cfg, params) - if audit_verifier is not None: - entries = _read_audit(audit_verifier) - state = state.model_copy( - update={ - "detail": { - **state.detail, - FRAME_AUDIT_DETAIL: { - "reachable": entries is not None, - "records": entries or [], - }, - } - } - ) + detail: dict[str, Any] = {} + entries: Optional[list[dict[str, Any]]] = None + snapshot = None if cfg.frame is not None and cfg.frame.database is not None: - snapshot = _read_database(cfg) + snapshot = _read_database(cfg, salt) if snapshot is not None: - state = state.model_copy( - update={"detail": {**state.detail, FRAME_DB_DETAIL: snapshot}} - ) + detail[FRAME_DB_DETAIL] = snapshot.to_json() + entries = snapshot.extra.get("audit") + audit_verifier = _audit_verifier(cfg, params) + if audit_verifier is not None: + if entries is None: + entries = _read_audit(audit_verifier) + # Which entries existed, as keyed hashes: never what they say. + detail[FRAME_AUDIT_DETAIL] = { + "reachable": entries is not None, + "ids": _audit_ids(entries or [], salt), + } + if detail: + state = state.model_copy(update={"detail": {**state.detail, **detail}}) payload = { "schema_version": BASELINE_SCHEMA, "execution_id": execution_id, "baseline_read_at": _now(), "verifier_identity": identity, "state": state.model_dump(mode="json"), - "salt": secrets.token_bytes(32).hex(), + "salt": salt.hex(), } _write_private(Path(out), payload) return payload @@ -1195,6 +1313,7 @@ def verify_run( audit_verifier=_audit_verifier(cfg, params), # The executor's own id is this run, never "another user". run_actors=[str(run_record.get("executor_id") or "")], + salt=salt, ) else: frame = FrameVerdict( diff --git a/packages/flow/tests/test_frame_database.py b/packages/flow/tests/test_frame_database.py index c5645a18cc..25c1f40bbe 100644 --- a/packages/flow/tests/test_frame_database.py +++ b/packages/flow/tests/test_frame_database.py @@ -1,21 +1,26 @@ """The whole-database frame: every table and column, not a read-list. -A frame over the surfaces a contract reads passes any write outside them: a -column the effects don't name on the new record, a field on its side row, a -row in a table nobody reads. ``frame.database`` diffs the whole database -instead, with a reviewed allow-list for the app's own bookkeeping. Records -are made up; the database is SQLite, judged through ``verify_run``. +A frame over the surfaces a contract reads passes any write outside them. +``frame.database`` diffs the whole database instead, with a reviewed +allow-list for the app's own bookkeeping. Each test below is one way a write +could slip past (most from the adversarial review of #1582), judged through +``verify_run`` on SQLite. Records are made up. """ from __future__ import annotations +import json import sqlite3 from pathlib import Path -from typing import Any, Callable +from typing import Any, Callable, Optional import pytest -from openadapt_flow.runtime.effects.db_snapshot import is_default, take_snapshot +from openadapt_flow.runtime.effects.db_snapshot import ( + CLOCK_DEFAULT, + is_default, + take_snapshot, +) from openadapt_flow.verify_run import ( capture_baseline, parse_verify_config, @@ -23,21 +28,39 @@ ) SCHEMA = """ +PRAGMA journal_mode = WAL; CREATE TABLE patient_data (id INTEGER PRIMARY KEY, pid INTEGER, fname TEXT, - lname TEXT, email TEXT, phone_cell TEXT DEFAULT '', regdate TEXT); + lname TEXT, email TEXT, phone_cell TEXT DEFAULT '', regdate TEXT, + active INTEGER DEFAULT 1, created TEXT DEFAULT CURRENT_TIMESTAMP); CREATE TABLE history_data (id INTEGER PRIMARY KEY, pid INTEGER, date TEXT, tobacco TEXT DEFAULT ''); -CREATE TABLE calendar (id INTEGER PRIMARY KEY, pid INTEGER, title TEXT); -CREATE TABLE codes (code TEXT); -CREATE TABLE log (id INTEGER PRIMARY KEY, user TEXT, patient_id INTEGER); -INSERT INTO patient_data VALUES (1, 1, 'Wendell', 'Prior', 'w@example.test', '', '2026-01-01'); +CREATE TABLE calendar (id INTEGER PRIMARY KEY AUTOINCREMENT, pid INTEGER, title TEXT); +CREATE TABLE access (pid INTEGER, user TEXT, role TEXT DEFAULT '', + PRIMARY KEY (pid, user)); +CREATE TABLE codes (id INTEGER PRIMARY KEY, code TEXT); +CREATE TABLE log (id INTEGER PRIMARY KEY, user TEXT, patient_id INTEGER, + event TEXT, tbl TEXT); +INSERT INTO patient_data (id, pid, fname, lname, email, regdate) + VALUES (1, 1, 'Wendell', 'Prior', 'w@example.test', '2026-01-01'); INSERT INTO history_data VALUES (1, 1, '2026-01-01', ''); -INSERT INTO codes VALUES ('A'), ('B'), ('C'); +INSERT INTO codes (code) VALUES ('A'), ('B'), ('C'); """ NEW = {"fname": "Ada", "lname": "Quill", "email": "ada@example.test"} +ATTRIBUTION = { + "kind": "audit_read", + "actor_field": "user", + "self": ["bot"], + "record_field": "patient_id", + "record_key": "pid", + "event_field": "event", + "write_events": ["insert", "update", "delete"], + "insert_events": ["insert"], + "table_field": "tbl", +} def _db(tmp_path: Path) -> Path: + tmp_path.mkdir(parents=True, exist_ok=True) path = tmp_path / "app.sqlite" con = sqlite3.connect(path) con.executescript(SCHEMA) @@ -46,7 +69,38 @@ def _db(tmp_path: Path) -> Path: return path -def _doc(db: Path, **database: Any) -> dict[str, Any]: +def _doc(db: Path, frame: Optional[dict] = None, **database: Any) -> dict[str, Any]: + spec: dict[str, Any] = { + "identity_field": "id", + "side_records": [ + { + "surface": "history_data", + "link_field": "pid", + "of_surface": "patient_data", + "of_field": "pid", + } + ], + "attribution": { + **ATTRIBUTION, + "read": { + "kind": "sql", + "sqlite_database": str(db), + "sql_query": "SELECT * FROM log", + }, + }, + "database": { + "dialect": "sqlite", + "read": {"sqlite_database": str(db)}, + "allow_tables": ["log"], + "views": {"patient_data": ""}, + "new_row_fields": { + "patient_data": ["pid", "regdate"], + "history_data": ["date"], + }, + **database, + }, + } + spec.update(frame or {}) return { "schema_version": "openadapt.verify/v1", "verifier": { @@ -59,110 +113,219 @@ def _doc(db: Path, **database: Any) -> dict[str, Any]: "match": ["email", "lname"], "fields": {"fname": "fname", "lname": "lname", "email": "email"}, }, - "frame": { - "identity_field": "id", - "side_records": [ - { - "surface": "history_data", - "link_field": "pid", - "of_surface": "patient_data", - "of_field": "pid", - } - ], - "database": { - "dialect": "sqlite", - "read": {"sqlite_database": str(db)}, - "allow_tables": ["log"], - "views": {"patient_data": ""}, - "new_row_fields": { - "patient_data": ["pid", "regdate"], - "history_data": ["date"], - }, - **database, - }, - }, + "frame": spec, } -def _save(con: sqlite3.Connection) -> None: - """The app's own save: the chart, its history row, a log line.""" +def _save(con: sqlite3.Connection, cols: str = "", vals: str = "") -> None: + """The app's own save: the chart, its history row, the bot's log lines.""" con.execute( - "INSERT INTO patient_data (id, pid, fname, lname, email, regdate) " - "VALUES (2, 2, 'Ada', 'Quill', 'ada@example.test', '2026-10-25')" + "INSERT INTO patient_data (id, pid, fname, lname, email, regdate" + + cols + + ") VALUES (2, 2, 'Ada', 'Quill', 'ada@example.test', '2026-10-25'" + + vals + + ")" ) con.execute("INSERT INTO history_data (pid, date) VALUES (2, '2026-10-25')") - con.execute("INSERT INTO log (user, patient_id) VALUES ('bot', 2)") + con.execute( + "INSERT INTO log (user, patient_id, event, tbl) VALUES " + "('bot', 2, 'insert', 'patient_data'), ('bot', 2, 'insert', 'history_data')" + ) -def _outcome( +def _run( tmp_path: Path, - after: Callable[[sqlite3.Connection], None], + after: Callable[[sqlite3.Connection], Any] = lambda con: None, + save: Callable[[sqlite3.Connection], None] = _save, + frame: Optional[dict] = None, **database: Any, ) -> dict[str, Any]: db = _db(tmp_path) - cfg = parse_verify_config(_doc(db, **database)) + cfg = parse_verify_config(_doc(db, frame, **database)) capture_baseline(cfg, NEW, tmp_path / "baseline.json") con = sqlite3.connect(db) - _save(con) + save(con) after(con) con.commit() con.close() - result = verify_run( + return verify_run( cfg, params=NEW, run_record={"executor_kind": "agent", "executor_id": "bot"}, authorization_ref="test", baseline_path=tmp_path / "baseline.json", ) - return result -def _status(result: dict[str, Any]) -> str: +def _frame(result: dict[str, Any]) -> str: return result["receipt"]["checked"]["frame"] def test_a_clean_save_holds(tmp_path: Path) -> None: - result = _outcome(tmp_path, lambda con: None) - assert _status(result) == "held" + result = _run(tmp_path) + assert _frame(result) == "held" assert result["receipt"]["outcome"] == "verified" @pytest.mark.parametrize( "stray", [ - # A column the effects don't name, set on the new record. "UPDATE patient_data SET phone_cell = '617-555-0142' WHERE pid = 2", - # A field on the new record's side row. "UPDATE history_data SET tobacco = 'Never' WHERE pid = 2", - # A row in a table no surface reads. "INSERT INTO calendar (pid, title) VALUES (2, 'New Patient')", - # A second side row. "INSERT INTO history_data (pid, date) VALUES (2, '2026-10-25')", - # A table fingerprinted rather than read row by row. - "INSERT INTO codes VALUES ('D')", + "UPDATE patient_data SET email = 'x@example.test' WHERE pid = 1", + "DELETE FROM codes WHERE code = 'A'", ], ) def test_any_write_outside_the_declared_delta_refutes( tmp_path: Path, stray: str ) -> None: - result = _outcome(tmp_path, lambda con: con.execute(stray), max_rows=2) - assert _status(result) == "refuted", stray + result = _run(tmp_path, lambda con: con.execute(stray)) + assert _frame(result) == "refuted", stray assert result["receipt"]["outcome"] != "verified" -def test_the_allow_list_is_the_only_exemption(tmp_path: Path) -> None: - # Bookkeeping the app writes on its own: allowed by name. - more_log = "INSERT INTO log (user, patient_id) VALUES ('bot', 2)" - assert _status(_outcome(tmp_path, lambda con: con.execute(more_log))) == "held" +# -- the review's silent passes, one test each ------------------------------------ -def test_an_unreadable_database_is_not_checked(tmp_path: Path) -> None: +def test_1_a_value_counts_as_default_only_if_it_is_the_declared_one( + tmp_path: Path, +) -> None: + # active defaults to 1; a new chart saved inactive is a write. + result = _run(tmp_path, save=lambda con: _save(con, ", active", ", 0")) + assert _frame(result) == "refuted" + + +def test_2_a_clock_default_must_fall_in_the_window(tmp_path: Path) -> None: + backdated = ", '2001-01-01 00:00:00'" + result = _run(tmp_path, save=lambda con: _save(con, ", created", backdated)) + assert _frame(result) == "refuted" + + +def test_3_ignore_fields_apply_to_surfaces_not_tables(tmp_path: Path) -> None: + other = "UPDATE patient_data SET regdate = '1999-01-01' WHERE pid = 1" + result = _run( + tmp_path, lambda con: con.execute(other), frame={"ignore_fields": ["regdate"]} + ) + assert _frame(result) == "refuted" + + +def test_4_a_row_written_and_removed_refutes(tmp_path: Path) -> None: + def write_then_delete(con: sqlite3.Connection) -> None: + con.execute("INSERT INTO calendar (pid, title) VALUES (1, 'x')") + con.execute("DELETE FROM calendar") + + assert _frame(_run(tmp_path, write_then_delete)) == "refuted" + + +def test_5_a_trigger_created_in_the_window_refutes(tmp_path: Path) -> None: + trigger = ( + "CREATE TRIGGER t AFTER INSERT ON history_data BEGIN " + "UPDATE patient_data SET email = 'x@example.test'; END" + ) + assert _frame(_run(tmp_path, lambda con: con.execute(trigger))) == "refuted" + + +def test_6_only_a_generated_key_may_hold_any_value(tmp_path: Path) -> None: + side = [ + { + "surface": "history_data", + "link_field": "pid", + "of_surface": "patient_data", + "of_field": "pid", + }, + { + "surface": "access", + "link_field": "pid", + "of_surface": "patient_data", + "of_field": "pid", + }, + ] + outside = "INSERT INTO access (pid, user) VALUES (2, 'outside_user')" + result = _run( + tmp_path, lambda con: con.execute(outside), frame={"side_records": side} + ) + assert _frame(result) == "refuted" + + +def test_7_a_view_entry_never_excuses_a_write(tmp_path: Path) -> None: + def viewed_then_written(con: sqlite3.Connection) -> None: + con.execute( + "INSERT INTO log (user, patient_id, event, tbl) " + "VALUES ('clinic2', 1, 'view', 'history_data')" + ) + con.execute("UPDATE history_data SET tobacco = 'Never' WHERE pid = 1") + + assert _frame(_run(tmp_path, viewed_then_written)) == "refuted" + + +def test_7_a_write_entry_for_another_table_never_excuses_a_write( + tmp_path: Path, +) -> None: + def other_table(con: sqlite3.Connection) -> None: + con.execute( + "INSERT INTO log (user, patient_id, event, tbl) " + "VALUES ('clinic2', 1, 'update', 'patient_data')" + ) + con.execute("UPDATE history_data SET tobacco = 'Never' WHERE pid = 1") + + assert _frame(_run(tmp_path, other_table)) == "refuted" + + +def test_another_users_write_to_another_chart_is_concurrent(tmp_path: Path) -> None: + def colleague(con: sqlite3.Connection) -> None: + con.execute( + "INSERT INTO log (user, patient_id, event, tbl) " + "VALUES ('clinic2', 1, 'update', 'history_data')" + ) + con.execute("UPDATE history_data SET tobacco = 'Never' WHERE pid = 1") + + assert _frame(_run(tmp_path, colleague)) == "held" + + +def test_another_users_later_write_to_the_new_chart_is_theirs(tmp_path: Path) -> None: + def colleague(con: sqlite3.Connection) -> None: + con.execute("UPDATE patient_data SET phone_cell = '617-555-0199' WHERE pid = 2") + con.execute( + "INSERT INTO log (user, patient_id, event, tbl) " + "VALUES ('clinic2', 2, 'update', 'patient_data')" + ) + + assert _frame(_run(tmp_path, colleague)) == "held" + + +def test_the_bots_own_later_write_to_the_new_chart_still_fails(tmp_path: Path) -> None: + def both(con: sqlite3.Connection) -> None: + con.execute("UPDATE patient_data SET phone_cell = '617-555-0199' WHERE pid = 2") + con.execute( + "INSERT INTO log (user, patient_id, event, tbl) VALUES " + "('clinic2', 2, 'update', 'patient_data'), " + "('bot', 2, 'update', 'patient_data')" + ) + + assert _frame(_run(tmp_path, both)) == "refuted" + + +# -- append-only bookkeeping, size, consistency, PHI ------------------------------- + + +def test_the_audit_log_is_append_only(tmp_path: Path) -> None: db = _db(tmp_path) - doc = _doc(db) - doc["frame"]["database"]["read"] = {"sqlite_database": str(tmp_path / "gone")} - cfg = parse_verify_config(doc) + con = sqlite3.connect(db) + con.execute( + "INSERT INTO log (user, patient_id, event, tbl) VALUES ('x', 1, 'view', 'p')" + ) + con.commit() + con.close() + cfg = parse_verify_config(_doc(db)) capture_baseline(cfg, NEW, tmp_path / "baseline.json") + con = sqlite3.connect(db) + _save(con) + con.execute("DELETE FROM log WHERE user = 'x'") + con.commit() + con.close() result = verify_run( cfg, params=NEW, @@ -170,28 +333,63 @@ def test_an_unreadable_database_is_not_checked(tmp_path: Path) -> None: authorization_ref="test", baseline_path=tmp_path / "baseline.json", ) - assert _status(result) == "not_checked" - assert result["receipt"]["outcome"] != "verified" + assert _frame(result) == "refuted" + + +def test_skipping_a_table_needs_a_reason(tmp_path: Path) -> None: + db = _db(tmp_path) + with pytest.raises(Exception, match="reason"): + parse_verify_config(_doc(db, allow_tables=[{"table": "log", "mode": "skip"}])) + cfg = parse_verify_config( + _doc(db, allow_tables=[{"table": "log", "mode": "skip", "reason": "test"}]) + ) + assert cfg.frame is not None and cfg.frame.database is not None -def test_the_snapshot_reads_every_table_in_three_statements(tmp_path: Path) -> None: +def test_a_large_table_holds_a_clean_save_and_catches_a_stray_one( + tmp_path: Path, +) -> None: + # Over max_rows, existing rows are compared by key bucket and the rows + # the save adds are read in full. + assert _frame(_run(tmp_path / "clean", max_rows=1)) == "held" + stray = "UPDATE patient_data SET email = 'x@example.test' WHERE pid = 1" + result = _run(tmp_path / "stray", lambda con: con.execute(stray), max_rows=1) + assert _frame(result) == "refuted" + cell = "UPDATE patient_data SET phone_cell = '1' WHERE pid = 2" + result = _run(tmp_path / "cell", lambda con: con.execute(cell), max_rows=1) + assert _frame(result) == "refuted" + + +def test_the_snapshot_is_one_consistent_read(tmp_path: Path) -> None: db = _db(tmp_path) - statements: list[str] = [] + opened: list[int] = [] + + class Interrupted: + """A connection whose reads let another writer commit midway.""" - class Spy: def __init__(self) -> None: + opened.append(1) self.con = sqlite3.connect(db) + self.reads = 0 def cursor(self) -> Any: outer = self class Cur: + description: Any = None + def __init__(self) -> None: self.cur = outer.con.cursor() def execute(self, sql: str) -> None: - statements.append(sql) + outer.reads += 1 + if outer.reads == 5: + w = sqlite3.connect(db) + w.execute("INSERT INTO calendar (pid, title) VALUES (1, 'x')") + w.commit() + w.close() self.cur.execute(sql) + self.description = self.cur.description def fetchall(self) -> list: return self.cur.fetchall() @@ -201,26 +399,54 @@ def fetchall(self) -> list: def close(self) -> None: self.con.close() - snap = take_snapshot(Spy, "sqlite", skip=["log"]) - assert set(snap.tables) == {"patient_data", "history_data", "calendar", "codes"} - assert snap.tables["calendar"].rows == [] - reads = [s for s in statements if not s.startswith(("PRAGMA", "SELECT name"))] - assert len(reads) == 2 # counts, then every row + snap = take_snapshot(Interrupted, "sqlite", key=b"k") + assert len(opened) == 1 + calendar = snap.tables["calendar"] + assert calendar.count == len(calendar.rows or {}) + + +def test_the_baseline_holds_no_values(tmp_path: Path) -> None: + _run(tmp_path) + detail = json.loads((tmp_path / "baseline.json").read_text())["state"]["detail"] + snapshot = json.dumps(detail["openadapt:frame_database"]) + assert "Wendell" not in snapshot and "w@example.test" not in snapshot + assert "clinic2" not in json.dumps(detail["openadapt:frame_audit"]) -def test_default_values_are_unset() -> None: - assert is_default("", None) - assert is_default("0000-00-00", None) - assert is_default("NO", "'NO'") - assert not is_default("617-555-0142", "''") - # Set by the database at insert: any value is its default. - assert is_default("2026-10-25 11:00:00", "CURRENT_TIMESTAMP") - assert is_default("2026-10-25 11:00:00", "current_timestamp()") +def test_the_receipt_counts_tables_compared_and_allowed(tmp_path: Path) -> None: + frame = _run(tmp_path)["evidence"]["frame"] + counts = frame["database"] + assert counts["tables_compared"] == 5 and counts["tables_append_only"] == 1 + assert counts["tables_skipped"] == 0 + assert counts["allow_list_sha256"].startswith("sha256:") + + +def test_an_unreadable_database_is_not_checked(tmp_path: Path) -> None: + db = _db(tmp_path) + doc = _doc(db) + doc["frame"]["database"]["read"] = {"sqlite_database": str(tmp_path / "gone")} + cfg = parse_verify_config(doc) + capture_baseline(cfg, NEW, tmp_path / "baseline.json") + result = verify_run( + cfg, + params=NEW, + run_record={"executor_kind": "agent", "executor_id": "bot"}, + authorization_ref="test", + baseline_path=tmp_path / "baseline.json", + ) + assert _frame(result) == "not_checked" + assert result["receipt"]["outcome"] != "verified" + + +def test_frame_database_needs_write_attribution(tmp_path: Path) -> None: + doc = _doc(_db(tmp_path)) + for key in ("event_field", "write_events", "insert_events", "table_field"): + doc["frame"]["attribution"].pop(key) + with pytest.raises(Exception, match="write"): + parse_verify_config(doc) def test_a_table_wider_than_one_json_call_is_read(tmp_path: Path) -> None: - # SQLite caps a function at 127 arguments; OpenEMR's chart has more - # columns than that. cols = ", ".join(f"c{i} TEXT" for i in range(150)) path = tmp_path / "wide.sqlite" con = sqlite3.connect(path) @@ -228,6 +454,24 @@ def test_a_table_wider_than_one_json_call_is_read(tmp_path: Path) -> None: con.execute("INSERT INTO wide (id, c149) VALUES (1, 'last')") con.commit() con.close() - snap = take_snapshot(lambda: sqlite3.connect(path), "sqlite") - (row,) = snap.tables["wide"].rows or [] + snap = take_snapshot(lambda: sqlite3.connect(path), "sqlite", key=b"k") + (row,) = snap.tables["wide"].values.values() assert row["c149"] == "last" and len(row) == 151 + assert "last" not in json.dumps(snap.to_json()) + + +def test_declared_defaults() -> None: + assert is_default(None, None) + assert not is_default("0", "1") and is_default("1", "1") and is_default(1, "1") + assert not is_default("", None) + assert is_default("NO", "NO") + assert is_default( + "2026-10-25 11:00:00", + CLOCK_DEFAULT, + ("2026-10-25 10:59:59", "2026-10-25 11:00:05"), + ) + assert not is_default( + "2001-01-01 00:00:00", + CLOCK_DEFAULT, + ("2026-10-25 10:59:59", "2026-10-25 11:00:05"), + ) From 02fb0d93e556f7a85c0d41df8619db9a30c01c33 Mon Sep 17 00:00:00 2001 From: abrichr Date: Wed, 28 Oct 2026 15:16:47 -0400 Subject: [PATCH 07/19] frame: excuse another actor's change to a declared record only through a write to its table, and never on a record the bot also wrote Co-Authored-By: Claude Opus 5.5 --- .../openadapt_flow/runtime/effects/frame.py | 19 +++++++++++++++---- 1 file changed, 15 insertions(+), 4 deletions(-) diff --git a/packages/flow/openadapt_flow/runtime/effects/frame.py b/packages/flow/openadapt_flow/runtime/effects/frame.py index 0d205ac5f7..c98643ff2d 100644 --- a/packages/flow/openadapt_flow/runtime/effects/frame.py +++ b/packages/flow/openadapt_flow/runtime/effects/frame.py @@ -898,10 +898,21 @@ def declared(record: Mapping[str, Any], hashed: bool = False) -> bool: fields = changed(name, before, after) # type: ignore[arg-type] stray = [f for f in fields if f not in may_change] if other_on_target(name, key, before, after): - if fields and not (set(fields) & may_change): - # Another actor wrote only fields this save doesn't - # declare: theirs. A declared field moved by another - # actor never passes: the saved value may not stand. + ref, hashed = ref_of(name, key, before, after) + if ( + fields + and not (set(fields) & may_change) + and attribution is not None + and attribution.write_events + and attribution.table_field + and not (actors(name, ref, hashed) & self_ids) + ): + # Another actor wrote, by a write to this table, only + # fields this save doesn't declare, and the bot wrote + # nothing to this record: theirs. A declared field + # moved by another actor never passes (the saved + # value may not stand), and nor does a record both + # wrote, since nothing says whose change is whose. concurrent.append( FrameChange(name, "changed", "other", fields, key=key) ) From ae62befb5a3fe17e6fa5a78932c1372286464510 Mon Sep 17 00:00:00 2001 From: abrichr Date: Wed, 28 Oct 2026 15:28:13 -0400 Subject: [PATCH 08/19] frame: a DEFAULT NULL column (shown as the text NULL) has no default Co-Authored-By: Claude Opus 5.5 --- .../flow/openadapt_flow/runtime/effects/db_snapshot.py | 4 ++-- packages/flow/tests/test_frame_database.py | 8 ++++++++ 2 files changed, 10 insertions(+), 2 deletions(-) diff --git a/packages/flow/openadapt_flow/runtime/effects/db_snapshot.py b/packages/flow/openadapt_flow/runtime/effects/db_snapshot.py index 4f1bfd776a..439e9900a5 100644 --- a/packages/flow/openadapt_flow/runtime/effects/db_snapshot.py +++ b/packages/flow/openadapt_flow/runtime/effects/db_snapshot.py @@ -265,8 +265,8 @@ def _default_text(raw: Any, dialect: Dialect) -> Optional[str]: ("current_timestamp", "now", "current_date", "localtime", "datetime('now'") ): return CLOCK_DEFAULT - if dialect == "mysql" and text.upper() == "NULL": - return None # MariaDB shows a NULL default as the text NULL + if text.upper() == "NULL": + return None # SQLite and MariaDB show DEFAULT NULL as the text NULL if len(text) >= 2 and text[0] == text[-1] and text[0] in "'\"": return text[1:-1].replace("''", "'") return text diff --git a/packages/flow/tests/test_frame_database.py b/packages/flow/tests/test_frame_database.py index 25c1f40bbe..4e411cb7ae 100644 --- a/packages/flow/tests/test_frame_database.py +++ b/packages/flow/tests/test_frame_database.py @@ -475,3 +475,11 @@ def test_declared_defaults() -> None: CLOCK_DEFAULT, ("2026-10-25 10:59:59", "2026-10-25 11:00:05"), ) + + +def test_a_null_default_is_no_default(tmp_path: Path) -> None: + from openadapt_flow.runtime.effects.db_snapshot import _default_text + + assert _default_text("NULL", "sqlite") is None + assert _default_text("NULL", "mysql") is None + assert _default_text("'NULL'", "sqlite") == "NULL" # the text NULL From 7f42842307dab11ef561d601e2450c390c5c53b6 Mon Sep 17 00:00:00 2001 From: abrichr Date: Wed, 28 Oct 2026 15:33:33 -0400 Subject: [PATCH 09/19] frame: a side row of another actor's new record is theirs; report their additions by surface id OpenEMR logs a contact row with no patient, so no write event attributes it; its link to a chart another user created in the window does, the same way side records attach to the bot's chart. FrameVerdict.added_by_others names such records as their surface does, for checks that read the surface. Co-Authored-By: Claude Opus 5.5 --- .../openadapt_flow/runtime/effects/frame.py | 51 +++++++++++++++---- packages/flow/tests/test_frame_database.py | 47 ++++++++++++++++- 2 files changed, 86 insertions(+), 12 deletions(-) diff --git a/packages/flow/openadapt_flow/runtime/effects/frame.py b/packages/flow/openadapt_flow/runtime/effects/frame.py index c98643ff2d..094971350f 100644 --- a/packages/flow/openadapt_flow/runtime/effects/frame.py +++ b/packages/flow/openadapt_flow/runtime/effects/frame.py @@ -374,6 +374,9 @@ class FrameVerdict: #: ``frame.database`` only: tables compared, append-only and skipped, #: and the allow-list's digest. database: Optional[dict[str, Any]] = None + #: In memory only: (surface, identity) of records another actor added, + #: as their surface names them, when a table is judged in its place. + added_by_others: set[tuple[str, str]] = field(default_factory=set) @property def held(self) -> bool: @@ -759,6 +762,8 @@ def attribute( collateral: list[FrameChange] = [] concurrent: list[FrameChange] = [] + added_by_others: set[tuple[str, str]] = set() + views_of = dict(database.views) if database is not None else {} declared_keys: set[str] = set() compared = 0 @@ -796,10 +801,15 @@ def new_row( collateral.append(change) # Side records: the links each new declared record gives its side rows. + # A new record another actor created gives its side rows the same way: + # those rows are that actor's too (OpenEMR logs a contact row with no + # patient, so only its link to the colleague's chart attributes it). side_links: list[set[str]] = [] + other_links: list[set[str]] = [] side_found: list[dict[str, int]] = [] for side in spec.side_records: links: set[str] = set() + theirs: set[str] = set() before_state, after_records = slices.get(side.of_surface, (None, None)) if before_state is not None and after_records is not None: sels = selectors_for(side.of_surface) @@ -807,11 +817,20 @@ def new_row( for record in after_records: if key_of(record, side.of_surface) in old: continue + value = record.get(side.of_field) + if value in (None, ""): + continue if any(record_matches(dict(record), s) for s in sels): - value = record.get(side.of_field) - if value not in (None, ""): - links.add(str(value)) + links.add(str(value)) + elif ( + attribute( + side.of_surface, key_of(record, side.of_surface), None, record + ) + == "other" + ): + theirs.add(str(value)) side_links.append(links) + other_links.append(theirs - links) side_found.append({link: 0 for link in links}) for name, (before_state, after_records) in slices.items(): @@ -959,15 +978,24 @@ def declared(record: Mapping[str, Any], hashed: bool = False) -> bool: declared_keys.add(str(ref_of(name, key, None, after)[0])) new_row(name, key, after, (side.link_field,)) continue - note( - FrameChange( - name, - "added", - attribute(name, key, None, after), - sorted(_without(after, ignore_for(name))), - key=key, - ) + theirs = any( + side.surface == name + and str(after.get(side.link_field)) in other_links[i] + for i, side in enumerate(spec.side_records) + ) + change = FrameChange( + name, + "added", + "other" if theirs else attribute(name, key, None, after), + sorted(_without(after, ignore_for(name))), + key=key, ) + note(change) + view = views_of.get(name) + if change.attribution == "other" and view and spec.identity_field: + # The same record as its surface names it, for checks + # that read the surface (an exact_new_set's additions). + added_by_others.add((view, str(after.get(spec.identity_field)))) elif after is None and before is not None: note( FrameChange( @@ -1033,6 +1061,7 @@ def declared(record: Mapping[str, Any], hashed: bool = False) -> bool: collateral=collateral, concurrent=concurrent, database=database_evidence, + added_by_others=added_by_others, ) verdict.reason = _reason(verdict) return verdict diff --git a/packages/flow/tests/test_frame_database.py b/packages/flow/tests/test_frame_database.py index 4e411cb7ae..2d3a8a559c 100644 --- a/packages/flow/tests/test_frame_database.py +++ b/packages/flow/tests/test_frame_database.py @@ -38,6 +38,7 @@ CREATE TABLE access (pid INTEGER, user TEXT, role TEXT DEFAULT '', PRIMARY KEY (pid, user)); CREATE TABLE codes (id INTEGER PRIMARY KEY, code TEXT); +CREATE TABLE contact (id INTEGER PRIMARY KEY, foreign_id INTEGER); CREATE TABLE log (id INTEGER PRIMARY KEY, user TEXT, patient_id INTEGER, event TEXT, tbl TEXT); INSERT INTO patient_data (id, pid, fname, lname, email, regdate) @@ -416,7 +417,7 @@ def test_the_baseline_holds_no_values(tmp_path: Path) -> None: def test_the_receipt_counts_tables_compared_and_allowed(tmp_path: Path) -> None: frame = _run(tmp_path)["evidence"]["frame"] counts = frame["database"] - assert counts["tables_compared"] == 5 and counts["tables_append_only"] == 1 + assert counts["tables_compared"] == 6 and counts["tables_append_only"] == 1 assert counts["tables_skipped"] == 0 assert counts["allow_list_sha256"].startswith("sha256:") @@ -483,3 +484,47 @@ def test_a_null_default_is_no_default(tmp_path: Path) -> None: assert _default_text("NULL", "sqlite") is None assert _default_text("NULL", "mysql") is None assert _default_text("'NULL'", "sqlite") == "NULL" # the text NULL + + +def test_a_side_row_of_another_users_new_chart_is_theirs(tmp_path: Path) -> None: + contact = { + "surface": "contact", + "link_field": "foreign_id", + "of_surface": "patient_data", + "of_field": "pid", + } + history = { + "surface": "history_data", + "link_field": "pid", + "of_surface": "patient_data", + "of_field": "pid", + } + + def save(con: sqlite3.Connection) -> None: + _save(con) + con.execute("INSERT INTO contact (foreign_id) VALUES (2)") + + def colleague(con: sqlite3.Connection) -> None: + # Another user's new chart: logged per table, but its contact row is + # logged with no patient (as OpenEMR does). + con.execute( + "INSERT INTO patient_data (id, pid, fname, lname, email, regdate) " + "VALUES (3, 3, 'Theo', 'Marsh', 't@example.test', '2026-10-25')" + ) + con.execute("INSERT INTO history_data (pid, date) VALUES (3, '2026-10-25')") + con.execute("INSERT INTO contact (foreign_id) VALUES (3)") + con.execute( + "INSERT INTO log (user, patient_id, event, tbl) VALUES " + "('clinic2', 3, 'insert', 'patient_data'), " + "('clinic2', 3, 'insert', 'history_data'), " + "('clinic2', 0, 'insert', 'contact')" + ) + + frame = {"side_records": [history, contact]} + assert _frame(_run(tmp_path / "a", colleague, save=save, frame=frame)) == "held" + # A contact row for a chart nobody created in the window still refutes. + stray = "INSERT INTO contact (foreign_id) VALUES (1)" + result = _run( + tmp_path / "b", lambda con: con.execute(stray), save=save, frame=frame + ) + assert _frame(result) == "refuted" From 49bc9acc732432766a46a3a795c7b8fbec436559 Mon Sep 17 00:00:00 2001 From: abrichr Date: Wed, 28 Oct 2026 16:12:17 -0400 Subject: [PATCH 10/19] frame: the MySQL snapshot sends no statement that can fail A driver may end the session on an error, so the snapshot checks for MySQL 8's information_schema_stats_expiry before setting it, instead of trying it and catching MariaDB's error. Co-Authored-By: Claude Opus 5.5 --- .../runtime/effects/db_snapshot.py | 17 +++++++++-------- 1 file changed, 9 insertions(+), 8 deletions(-) diff --git a/packages/flow/openadapt_flow/runtime/effects/db_snapshot.py b/packages/flow/openadapt_flow/runtime/effects/db_snapshot.py index 439e9900a5..e522e16578 100644 --- a/packages/flow/openadapt_flow/runtime/effects/db_snapshot.py +++ b/packages/flow/openadapt_flow/runtime/effects/db_snapshot.py @@ -293,14 +293,15 @@ def __init__(self, connect: Callable[[], Any], dialect: Dialect) -> None: self.con = connect() self.cur = self.con.cursor() if dialect == "mysql": - for statement in ( - "SET SESSION information_schema_stats_expiry = 0", # MySQL 8 - "SET SESSION group_concat_max_len = 1073741824", - ): - try: - self.cur.execute(statement) - except Exception: # noqa: BLE001 - MariaDB lacks the first - pass + # Only statements that can't fail: a driver may end the session + # on any error. MySQL 8 caches AUTO_INCREMENT unless told not to; + # MariaDB has no such variable. + self.cur.execute( + "SHOW SESSION VARIABLES LIKE 'information_schema_stats_expiry'" + ) + if self.cur.fetchall(): + self.cur.execute("SET SESSION information_schema_stats_expiry = 0") + self.cur.execute("SET SESSION group_concat_max_len = 1073741824") self.cur.execute("SET SESSION TRANSACTION ISOLATION LEVEL REPEATABLE READ") self.cur.execute("START TRANSACTION WITH CONSISTENT SNAPSHOT, READ ONLY") else: From f07a3d3d23b3893eea1e75fa66fd23d7d7f0a3cd Mon Sep 17 00:00:00 2001 From: abrichr Date: Wed, 28 Oct 2026 16:31:12 -0400 Subject: [PATCH 11/19] frame: rows by key in every table, attribution by kind and count, MAC key outside the baseline From the second review of #1582: - Every table keeps, per row, a 128-bit MAC of its key and of its contents (without ignore_columns); cell MACs for all columns up to max_rows (now 2000), and for the reference columns above it. A changed row in a large table goes through attribution by its key; ignore_columns hold; an added row is found by key for any primary key. Buckets are gone. - Attribution: the record is named as it was before the change; an added row needs an insert entry, a removed one a delete entry (delete_events), a changed one an update; each entry explains at most one row; a row whose record key changed is never explained. The log's own limits are documented. - MACs: BLAKE2b, 128 bits, bound to table and column, keyed by the salt and a host secret (OPENADAPT_FRAME_SECRET, or an owner-only key file) that is never in the baseline. Audit-entry ids use the same key. - MySQL: AUTO_INCREMENT is read just before the consistent snapshot starts, on the column the database numbers whether or not it is the key (the live run's false hold on patient_data); no GROUP_CONCAT; the frame is not checked when the read account can't see triggers (it needs TRIGGER); a required schema the account can't see is refused. Co-Authored-By: Claude Opus 5.5 --- .../runtime/effects/db_snapshot.py | 454 ++++++++++-------- .../openadapt_flow/runtime/effects/frame.py | 197 ++++++-- packages/flow/openadapt_flow/verify_run.py | 84 +++- packages/flow/tests/test_frame_database.py | 138 +++++- 4 files changed, 606 insertions(+), 267 deletions(-) diff --git a/packages/flow/openadapt_flow/runtime/effects/db_snapshot.py b/packages/flow/openadapt_flow/runtime/effects/db_snapshot.py index e522e16578..ec69f3ea25 100644 --- a/packages/flow/openadapt_flow/runtime/effects/db_snapshot.py +++ b/packages/flow/openadapt_flow/runtime/effects/db_snapshot.py @@ -8,64 +8,102 @@ What a snapshot holds, and why: -- **No values on disk.** Each cell is a keyed hash (keyed BLAKE2b with the - run's salt), so a baseline is not a copy of the records. Plain values are - read only for rows the save added, and held in memory to judge them. +- **No values on disk.** Each row is kept as a keyed MAC of its key and of + its contents (without ``ignore_columns``), and each cell as a keyed MAC + bound to its table and column: BLAKE2b, 128 bits, keyed by + :func:`mac_key` from the run's salt and a host secret that is never in the + baseline. A table over ``max_rows`` keeps cell MACs only for the columns + attribution and side records read (``ref_columns``). Plain values are read + in full, but kept in memory only for rows that differ from the baseline. +- **Rows by key.** A changed row is known by its key in every table, so it + goes through attribution, and an added row is found by key for any + primary key. - **One consistent read.** Everything is read on one connection inside one read-only transaction (``START TRANSACTION WITH CONSISTENT SNAPSHOT, READ - ONLY`` on MySQL and MariaDB; ``BEGIN`` on SQLite), so a row committed - mid-snapshot is never half-seen. The audit log is read in the same - transaction (``extra_queries``). -- **Large tables by bucket.** A table over ``max_rows`` with a single - integer key is digested in buckets of 1024 keys (salted SHA-256 of its rows' - hashes). Rows the save adds sit above the baseline's largest key and are - read in full; a changed bucket below it means an existing row moved. - A large table without an integer key gets one digest. -- **Schema and code.** Each table's definition (column types, defaults, - nullability, extras) and the database's triggers, views, routines and - events are digested; a change is reported, never ignored. -- **Counters.** ``sqlite_sequence`` and ``AUTO_INCREMENT`` show a row that - was written and deleted inside the window. + ONLY`` on MySQL and MariaDB; ``BEGIN`` on SQLite). The audit log is read in + the same transaction (``extra_queries``). MySQL's ``AUTO_INCREMENT`` isn't + part of a snapshot, so it is read just before the snapshot starts: a row + committed in between is in the snapshot, never missing from it. +- **Schema and code.** Each table's definition and the database's triggers, + views, routines and events are digested. On MySQL a trigger is visible + only to an account with ``TRIGGER``; without it the snapshot says so + (``code_visible``), and the frame is not checked. +- **Counters.** ``sqlite_sequence`` and ``AUTO_INCREMENT`` (on the column the + database numbers, key or not) show a row written and deleted in the window. - **The clock.** ``CURRENT_TIMESTAMP`` at the snapshot, so a column the database fills with the time can be held to the window. - **Allow-listed tables** are append-only by default: their existing rows - must not change (the audit log is the attribution channel). ``skip`` mode - reads nothing and needs a stated reason. + must not change or go. ``skip`` reads nothing and needs a stated reason. Read-only: SQLite files open with ``mode=ro``; a driver connection should use -a SELECT-only account with SELECT on every schema the app can write. +an account with SELECT (and, on MySQL, TRIGGER and SHOW VIEW) on every schema +the app can write. The baseline must still be protected like the records: +whoever holds both it and the host secret can test guesses against the MACs. """ from __future__ import annotations import hashlib import json +import os +import secrets from collections.abc import Callable, Mapping, Sequence from dataclasses import dataclass, field from decimal import Decimal, InvalidOperation +from pathlib import Path from typing import Any, Literal, Optional Dialect = Literal["sqlite", "mysql"] TableMode = Literal["compare", "append_only", "skip"] -#: Keys per bucket in a large table's digest. -BUCKET = 1024 #: Columns per JSON object call: SQLite caps a function at 127 arguments. _CHUNK = 50 #: The synthetic identity field of a database slice's records. KEY = "__row_key__" +#: The row's contents MAC in a database slice's records. +ROW = "__row_mac__" #: A column default meaning any value: the table's own generated key. ANY_VALUE = "\x00any" #: A column default the database computes as the current time at insert. CLOCK_DEFAULT = "\x00clock" _SYSTEM_SCHEMAS = ("information_schema", "mysql", "performance_schema", "sys") +#: Hex characters per MAC (128 bits). +MAC_HEX = 32 class SnapshotError(RuntimeError): """The database can't be snapshotted as the frame needs.""" -# -- hashing ------------------------------------------------------------------ +# -- keys and MACs ------------------------------------------------------------ + + +def _host_secret() -> bytes: + """The host's frame secret: ``OPENADAPT_FRAME_SECRET`` (hex), the file + ``OPENADAPT_FRAME_SECRET_FILE``, or ``~/.config/openadapt/frame.key`` + (created owner-only on first use). Keep it unreadable to the agent's + account; it is never written into a baseline.""" + + env = os.environ.get("OPENADAPT_FRAME_SECRET") + if env: + return bytes.fromhex(env) + path = Path( + os.environ.get("OPENADAPT_FRAME_SECRET_FILE") + or Path.home() / ".config" / "openadapt" / "frame.key" + ) + if not path.exists(): + path.parent.mkdir(parents=True, exist_ok=True) + fd = os.open(path, os.O_WRONLY | os.O_CREAT | os.O_EXCL, 0o600) + with os.fdopen(fd, "wb") as handle: + handle.write(secrets.token_bytes(32)) + return path.read_bytes() + + +def mac_key(salt: bytes) -> bytes: + """The run's MAC key: the salt (in the baseline) keyed by the host + secret (not in it), so the baseline alone can't be tested against.""" + + return hashlib.blake2b(salt, key=_host_secret()[:64], digest_size=32).digest() def canonical(value: Any) -> str: @@ -83,30 +121,42 @@ def canonical(value: Any) -> str: return str(value) -#: Hex characters per cell MAC (32 bits). A cell's MAC is only compared -#: with the same cell's MAC, and is keyed, so it can't be forged; the row -#: key MAC is 128 bits so distinct rows never share one. -CELL_HEX = 8 +def _mac(key: bytes, text: str) -> str: + return hashlib.blake2b( + text.encode(), digest_size=MAC_HEX // 2, key=key[:64] + ).hexdigest() + + +def cell_hash(key: bytes, value: Any, table: str = "", column: str = "") -> str: + """A cell's keyed MAC, bound to its table and column, so equal values in + different places don't share one.""" + return _mac(key, f"c\x1f{table}\x1f{column}\x1f{canonical(value)}") -def cell_hash(key: bytes, value: Any) -> str: - """A cell's keyed MAC (BLAKE2b, 32 bits).""" - return hashlib.blake2b( - canonical(value).encode(), digest_size=CELL_HEX // 2, key=key[:64] - ).hexdigest() +def row_key_hash(key: bytes, text: str, table: str = "") -> str: + """A row key's keyed MAC.""" + return _mac(key, f"k\x1f{table}\x1f{text}") -def row_key_hash(key: bytes, text: str) -> str: - """A row key's keyed MAC (BLAKE2b, 128 bits).""" - return hashlib.blake2b(text.encode(), digest_size=16, key=key[:64]).hexdigest() +def row_hash( + key: bytes, table: str, row: Mapping[str, Any], ignore: Sequence[str] = () +) -> str: + """A row's contents MAC, without ``ignore`` columns.""" + + skip = set(ignore) + body = json.dumps( + sorted((c, canonical(v)) for c, v in row.items() if c not in skip), + separators=(",", ":"), + ) + return _mac(key, f"r\x1f{table}\x1f{body}") def unpack(columns: Sequence[str], packed: str) -> dict[str, str]: """A stored row's cell MACs by column (stored as one string per row).""" - return {c: packed[i * CELL_HEX : (i + 1) * CELL_HEX] for i, c in enumerate(columns)} + return {c: packed[i * MAC_HEX : (i + 1) * MAC_HEX] for i, c in enumerate(columns)} # -- shapes ------------------------------------------------------------------- @@ -127,18 +177,17 @@ class Table: count: int = 0 #: ``sqlite_sequence`` or ``AUTO_INCREMENT``, when the table has one. counter: Optional[int] = None - #: The largest generated key (None for an empty table or no such key). - max_key: Optional[int] = None - #: Row key MAC -> the row's cell MACs in column order, concatenated - #: (:func:`unpack`), for a table read row by row. - rows: Optional[dict[str, str]] = None - #: Bucket -> digest of the rows at or below ``max_key`` (large and - #: append-only tables with a generated key). - buckets: Optional[dict[str, str]] = None - #: One digest of the whole table (large tables without one). - digest: Optional[str] = None - #: In memory only, never saved: plain values of rows read in full, by - #: row key hash (a small table's rows; a large table's added rows). + #: The column the database numbers, and its largest value. + auto_column: Optional[str] = None + max_auto: Optional[int] = None + #: The columns each row keeps cell MACs for (all, or ``ref_columns`` for + #: a table over ``max_rows``; none for an append-only table). + cell_columns: list[str] = field(default_factory=list) + #: Row key MAC -> [row contents MAC, cell MACs of ``cell_columns`` + #: concatenated (:func:`unpack`)]. + rows: dict[str, list[str]] = field(default_factory=dict) + #: In memory only, never saved: plain values of the rows that differ + #: from the baseline (all rows of a read with no baseline: none kept). values: dict[str, dict[str, Any]] = field(default_factory=dict, repr=False) #: The engine keeps no snapshot isolation for it (MyISAM, MEMORY). non_transactional: bool = False @@ -148,19 +197,24 @@ class Table: class Snapshot: dialect: str = "sqlite" clock: str = "" - #: Digest of triggers, views, routines and events. + #: Digest of triggers, views, routines and events, and whether the + #: account could see triggers at all. objects: str = "" + code_visible: bool = True + schemas: list[str] = field(default_factory=list) tables: dict[str, Table] = field(default_factory=dict) #: In memory only: rows of ``extra_queries``, read in the same transaction. extra: dict[str, list[dict[str, Any]]] = field(default_factory=dict, repr=False) def to_json(self) -> dict[str, Any]: - """The persisted form: hashes and schema only, never values.""" + """The persisted form: MACs and schema only, never values.""" return { "dialect": self.dialect, "clock": self.clock, "objects": self.objects, + "code_visible": self.code_visible, + "schemas": self.schemas, "tables": { name: { "mode": t.mode, @@ -171,10 +225,10 @@ def to_json(self) -> dict[str, Any]: "defaults": t.defaults, "count": t.count, "counter": t.counter, - "max_key": t.max_key, + "auto_column": t.auto_column, + "max_auto": t.max_auto, + "cell_columns": t.cell_columns, "rows": t.rows, - "buckets": t.buckets, - "digest": t.digest, "non_transactional": t.non_transactional, } for name, t in self.tables.items() @@ -187,6 +241,8 @@ def from_json(cls, data: Mapping[str, Any]) -> "Snapshot": dialect=str(data.get("dialect", "sqlite")), clock=str(data.get("clock", "")), objects=str(data.get("objects", "")), + code_visible=bool(data.get("code_visible", True)), + schemas=list(data.get("schemas", [])), tables={ name: Table( mode=t["mode"], @@ -197,10 +253,10 @@ def from_json(cls, data: Mapping[str, Any]) -> "Snapshot": defaults=dict(t["defaults"]), count=int(t["count"]), counter=t.get("counter"), - max_key=t.get("max_key"), - rows=t.get("rows"), - buckets=t.get("buckets"), - digest=t.get("digest"), + auto_column=t.get("auto_column"), + max_auto=t.get("max_auto"), + cell_columns=list(t.get("cell_columns", [])), + rows={k: list(v) for k, v in (t.get("rows") or {}).items()}, non_transactional=bool(t.get("non_transactional")), ) for name, t in data["tables"].items() @@ -286,13 +342,15 @@ def _literal(text: str) -> str: class _Session: - """One connection, one read-only transaction.""" + """One connection, one read-only transaction (opened by :meth:`begin`).""" def __init__(self, connect: Callable[[], Any], dialect: Dialect) -> None: self.dialect = dialect self.con = connect() self.cur = self.con.cursor() - if dialect == "mysql": + + def begin(self) -> None: + if self.dialect == "mysql": # Only statements that can't fail: a driver may end the session # on any error. MySQL 8 caches AUTO_INCREMENT unless told not to; # MariaDB has no such variable. @@ -301,7 +359,6 @@ def __init__(self, connect: Callable[[], Any], dialect: Dialect) -> None: ) if self.cur.fetchall(): self.cur.execute("SET SESSION information_schema_stats_expiry = 0") - self.cur.execute("SET SESSION group_concat_max_len = 1073741824") self.cur.execute("SET SESSION TRANSACTION ISOLATION LEVEL REPEATABLE READ") self.cur.execute("START TRANSACTION WITH CONSISTENT SNAPSHOT, READ ONLY") else: @@ -332,6 +389,9 @@ class _Def: binary: set[str] schema: str non_transactional: bool = False + #: The column the database numbers (AUTO_INCREMENT, or SQLite's rowid + #: alias), whether or not it is the primary key. + auto_column: Optional[str] = None def _digest(parts: Sequence[Any]) -> str: @@ -376,6 +436,7 @@ def _definitions(s: _Session) -> tuple[dict[str, _Def], str]: d.primary_key.append(str(column)) if "auto_increment" in str(extra).lower(): d.generated_key = str(column) + d.auto_column = str(column) if "binary" in str(dtype).lower() or "blob" in str(dtype).lower(): d.binary.add(str(column)) d.schema = _digest( @@ -425,6 +486,7 @@ def _definitions(s: _Session) -> tuple[dict[str, _Def], str]: {str(r[1]): _default_text(r[4], "sqlite") for r in info}, set(), _digest([sql]), + auto_column=generated, ) code = [r for r in master if r[0] in ("trigger", "view")] return out, _digest(code) @@ -475,52 +537,24 @@ def _row_key(d: _Def, row: Mapping[str, Any], seen: dict[str, int]) -> str: return f"{digest}#{seen[digest]}" -def _hash_rows( - key: bytes, d: _Def, rows: Sequence[Mapping[str, Any]] -) -> tuple[dict[str, str], dict[str, dict[str, Any]]]: - hashed: dict[str, str] = {} - plain: dict[str, dict[str, Any]] = {} - seen: dict[str, int] = {} - for row in rows: - k = row_key_hash(key, _row_key(d, row, seen)) - hashed[k] = "".join(cell_hash(key, row.get(c)) for c in d.columns) - plain[k] = dict(row) - return hashed, plain +def _mysql_counters(s: _Session) -> dict[str, int]: + default = s.rows("SELECT DATABASE()")[0][0] + out: dict[str, int] = {} + for schema, table, value in s.rows( + "SELECT TABLE_SCHEMA, TABLE_NAME, AUTO_INCREMENT FROM information_schema.TABLES " + "WHERE AUTO_INCREMENT IS NOT NULL" + ): + name = str(table) if schema == default else f"{schema}.{table}" + out[name] = int(value) + return out -def _bucket_digests( - s: _Session, name: str, d: _Def, key: bytes, upto: int -) -> dict[str, str]: - """Salted digest per bucket of ``BUCKET`` keys, over keys <= ``upto``.""" +def _sees_triggers(s: _Session) -> bool: + """Can this MySQL account see triggers (it needs TRIGGER)?""" - k = _quote(s.dialect, d.generated_key or "") - if s.dialect == "mysql": - row = "CONCAT(" + ", ".join(_row_json("mysql", d)) + ")" - salt = _literal(key.hex()) - return { - str(b): str(h) - for b, h in s.rows( - f"SELECT {k} DIV {BUCKET} AS b, SHA2(CONCAT({salt}, GROUP_CONCAT(" - f"SHA2({row}, 256) ORDER BY {k} SEPARATOR '')), 256) " - f"FROM {_quote('mysql', name)} WHERE {k} <= {int(upto)} GROUP BY b" - ) - } - out: dict[str, hashlib._Hash] = {} - for row in _read_rows(s, name, d, f"WHERE {k} <= {int(upto)} ORDER BY {k}"): - bucket = str(int(row[d.generated_key or ""]) // BUCKET) - h = out.setdefault(bucket, hashlib.sha256(key)) - h.update(json.dumps(row, sort_keys=True, default=str).encode()) - return {b: h.hexdigest() for b, h in out.items()} - - -def _table_digest(s: _Session, name: str, d: _Def, key: bytes) -> str: - h = hashlib.sha256(key) - rows = sorted( - json.dumps(r, sort_keys=True, default=str) for r in _read_rows(s, name, d) - ) - for r in rows: - h.update(r.encode()) - return h.hexdigest() + grants = " ".join(str(r[0]) for r in s.rows("SHOW GRANTS FOR CURRENT_USER()")) + upper = grants.upper() + return "ALL PRIVILEGES" in upper or "TRIGGER" in upper def take_snapshot( @@ -529,23 +563,30 @@ def take_snapshot( *, key: bytes, modes: Mapping[str, TableMode] = {}, - max_rows: int = 20000, + max_rows: int = 2000, before: Optional[Snapshot] = None, extra_queries: Mapping[str, str] = {}, + ignore: Mapping[str, Sequence[str]] = {}, + ref_columns: Sequence[str] = (), + schemas: Sequence[str] = (), ) -> Snapshot: """Read every table in one read-only transaction. Args: connect: Opens one connection (DB-API 2.0: ``cursor()``, ``close()``). - key: The run's salt, which keys every hash. + key: The MAC key (:func:`mac_key`). modes: Table -> ``append_only`` or ``skip`` (allow-listed tables); every other table is compared. - max_rows: Over this many rows, a table is digested by bucket. - before: The baseline, when this is the read after the save: a large - or append-only table's existing rows are digested up to the - baseline's largest key, and rows above it are read in full. + max_rows: Over this many rows, a table keeps cell MACs only for + ``ref_columns`` (and a contents MAC for every row). + before: The baseline, when this is a read after the save: plain + values are kept for the rows that differ from it. extra_queries: Name -> a SELECT to run in the same transaction (the audit log); rows land in :attr:`Snapshot.extra`, in memory only. + ignore: Table -> columns left out of each row's contents MAC. + ref_columns: Columns attribution and side records read (a record + key, a link field): their cell MACs are kept in every table. + schemas: Schemas the read must see (MySQL); refused if one isn't. Raises: SnapshotError: the database can't be read as the frame needs it. @@ -554,9 +595,11 @@ def take_snapshot( try: s = _Session(connect, dialect) except Exception as exc: # noqa: BLE001 - raise SnapshotError(f"could not open a read transaction: {exc}") from exc + raise SnapshotError(f"could not open a connection: {exc}") from exc try: - return _snapshot(s, key, modes, max_rows, before, extra_queries) + return _snapshot( + s, key, modes, max_rows, before, extra_queries, ignore, ref_columns, schemas + ) except SnapshotError: raise except Exception as exc: # noqa: BLE001 @@ -572,12 +615,43 @@ def _snapshot( max_rows: int, before: Optional[Snapshot], extra_queries: Mapping[str, str], + ignore: Mapping[str, Sequence[str]], + ref_columns: Sequence[str], + schemas: Sequence[str], ) -> Snapshot: dialect = s.dialect + counters: dict[str, int] = {} + visible = True + if dialect == "mysql": + # Before the snapshot starts: a row committed after this read is in + # the snapshot, so the counter never runs ahead of what it shows. + counters = _mysql_counters(s) + visible = _sees_triggers(s) + s.begin() snap = Snapshot( - dialect=dialect, clock=_clock_text(s.rows("SELECT CURRENT_TIMESTAMP")[0][0]) + dialect=dialect, + clock=_clock_text(s.rows("SELECT CURRENT_TIMESTAMP")[0][0]), + code_visible=visible, ) defs, snap.objects = _definitions(s) + if dialect == "mysql": + default = str(s.rows("SELECT DATABASE()")[0][0]) + seen = {default} | {t.split(".", 1)[0] for t in defs if "." in t} + missing = sorted(set(schemas) - seen) + if missing: + raise SnapshotError( + "the read account can't see schema(s): " + ", ".join(missing) + ) + snap.schemas = sorted(seen) + else: + if any( + n == "sqlite_sequence" + for (n,) in s.rows("SELECT name FROM sqlite_master WHERE type = 'table'") + ): + counters = { + str(n): int(v) + for n, v in s.rows("SELECT name, seq FROM sqlite_sequence") + } for name, sql in extra_queries.items(): snap.extra[name] = s.dicts(sql) unknown = sorted(set(modes) - set(defs)) @@ -600,43 +674,25 @@ def _snapshot( ) ) } - keyed = [t for t in names if defs[t].generated_key] + numbered = [t for t in names if defs[t].auto_column] maxes = ( { str(t): (None if m is None else int(m)) for t, m in s.rows( " UNION ALL ".join( - f"SELECT {_literal(t)}, MAX({_quote(dialect, defs[t].generated_key or '')}) " + f"SELECT {_literal(t)}, MAX({_quote(dialect, defs[t].auto_column or '')}) " f"FROM {_quote(dialect, t)}" - for t in keyed + for t in numbered ) ) } - if keyed + if numbered else {} ) - counters: dict[str, int] = {} - if dialect == "sqlite": - if any( - n == "sqlite_sequence" - for (n,) in s.rows("SELECT name FROM sqlite_master WHERE type = 'table'") - ): - counters = { - str(n): int(v) - for n, v in s.rows("SELECT name, seq FROM sqlite_sequence") - } - else: - default = s.rows("SELECT DATABASE()")[0][0] - for schema, table, value in s.rows( - "SELECT TABLE_SCHEMA, TABLE_NAME, AUTO_INCREMENT FROM information_schema.TABLES " - "WHERE AUTO_INCREMENT IS NOT NULL" - ): - name = str(table) if schema == default else f"{schema}.{table}" - counters[name] = int(value) + refs = set(ref_columns) for t in names: d = defs[t] mode: TableMode = "append_only" if modes.get(t) == "append_only" else "compare" - old = before.tables.get(t) if before is not None else None table = Table( mode, d.schema, @@ -646,39 +702,33 @@ def _snapshot( dict(d.defaults), counts.get(t, 0), counters.get(t), + d.auto_column, maxes.get(t), non_transactional=d.non_transactional, ) if d.generated_key: # Only the table's own generated key may hold any value. table.defaults[d.generated_key] = ANY_VALUE - if mode == "append_only" and not d.generated_key: - raise SnapshotError( - f"append-only table {t!r} has no generated key; mark it skip with " - "a reason, or compare it" - ) - # Read after the save the way the baseline was read, so a table that - # crossed max_rows in the window is still compared like for like. - if old is not None and old.mode == mode and old.schema == d.schema: - by_bucket = old.buckets is not None - whole = old.digest is not None - else: - by_bucket = bool(d.generated_key) and ( - mode == "append_only" or table.count > max_rows - ) - whole = not by_bucket and table.count > max_rows - if by_bucket: - upto = (old.max_key if old is not None else table.max_key) or 0 - table.buckets = _bucket_digests(s, t, d, key, upto) - if before is not None and mode == "compare": - k = _quote(dialect, d.generated_key or "") - added = _read_rows(s, t, d, f"WHERE {k} > {int(upto)}") - _hashed, table.values = _hash_rows(key, d, added) - elif whole: - table.digest = _table_digest(s, t, d, key) + old = before.tables.get(t) if before is not None else None + if mode == "append_only": + table.cell_columns = [] + elif old is not None and old.mode == mode and old.schema == d.schema: + table.cell_columns = list(old.cell_columns) # read like the baseline + elif table.count > max_rows: + table.cell_columns = [c for c in d.columns if c in refs] else: - rows = _read_rows(s, t, d) if table.count else [] - table.rows, table.values = _hash_rows(key, d, rows) + table.cell_columns = list(d.columns) + skip = ignore.get(t, ()) + seen_keys: dict[str, int] = {} + for row in _read_rows(s, t, d) if table.count else []: + k = row_key_hash(key, _row_key(d, row, seen_keys), t) + mac = row_hash(key, t, row, skip) + table.rows[k] = [ + mac, + "".join(cell_hash(key, row.get(c), t, c) for c in table.cell_columns), + ] + if old is not None and (k not in old.rows or old.rows[k][0] != mac): + table.values[k] = dict(row) snap.tables[t] = table return snap @@ -695,9 +745,9 @@ class TableDiff: removed: int = 0 changed: int = 0 changed_columns: list[str] = field(default_factory=list) - #: Moved, but not row by row: why (a schema change, a changed bucket of a - #: large table, an append-only table whose existing rows moved, a - #: counter that moved past the rows added). + #: Moved in a way no row comparison explains: why (a definition change, + #: an append-only table whose existing rows moved, a row written and + #: removed). unread: list[str] = field(default_factory=list) @@ -715,23 +765,15 @@ def unread_reasons( if old.schema != new.schema or old.columns != new.columns: return ["definition changed"] reasons = [] - if old.buckets is not None or new.buckets is not None: - if old.buckets != new.buckets: - reasons.append("existing rows changed (by bucket)") - elif old.digest is not None or new.digest is not None: - if old.digest is None or new.digest is None: - raise SnapshotError("a large table has no digest") - if old.digest != new.digest: - reasons.append("rows changed (whole-table digest)") + if old.mode == "append_only" and any( + k not in new.rows or new.rows[k][0] != v[0] for k, v in old.rows.items() + ): + reasons.append("existing rows changed or removed") if new.counter is not None: - # sqlite_sequence holds the last key used; AUTO_INCREMENT the next. + # sqlite_sequence holds the last number used; AUTO_INCREMENT the next. step = 1 if dialect == "mysql" else 0 expected = max( - v - for v in ( - old.counter or 0, - 0 if new.max_key is None else new.max_key + step, - ) + old.counter or 0, 0 if new.max_auto is None else new.max_auto + step ) if new.counter > expected: reasons.append("a row was written and removed (counter moved)") @@ -758,37 +800,35 @@ def diff( if name in skip: continue a, b = before.tables.get(name), after.tables.get(name) - reasons = unread_reasons(a, b, after.dialect) - moved = TableDiff(unread=reasons) - if ( - a is not None - and b is not None - and a.mode == "compare" - and not ( - reasons and reasons[0] in ("definition changed", "created or dropped") - ) - ): - old = {k: unpack(a.columns, v) for k, v in (a.rows or {}).items()} - new = ( - {k: unpack(b.columns, v) for k, v in b.rows.items()} - if b.rows is not None - else { - k: {c: cell_hash(key, v.get(c)) for c in b.columns} - for k, v in b.values.items() - } - ) - if a.rows is not None: - moved.added = len(new.keys() - old.keys()) - moved.removed = len(old.keys() - new.keys()) - else: - moved.added = len(new) - columns: set[str] = set() - for k in old.keys() & new.keys(): - fields = [c for c in b.columns if old[k].get(c) != new[k].get(c)] - if fields: + moved = TableDiff(unread=unread_reasons(a, b, after.dialect)) + if a is not None and b is not None and a.mode == "compare" == b.mode: + if a.columns == b.columns and a.cell_columns == b.cell_columns: + moved.added = len(b.rows.keys() - a.rows.keys()) + moved.removed = len(a.rows.keys() - b.rows.keys()) + columns: set[str] = set() + for k in a.rows.keys() & b.rows.keys(): + if a.rows[k][0] == b.rows[k][0]: + continue moved.changed += 1 - columns.update(fields) - moved.changed_columns = sorted(columns) + old = unpack(a.cell_columns, a.rows[k][1]) + new = unpack(b.cell_columns, b.rows[k][1]) + cols = [c for c in a.cell_columns if old[c] != new[c]] + columns.update(cols or ["(columns not compared)"]) + moved.changed_columns = sorted(columns) if moved.added or moved.removed or moved.changed or moved.unread: out[name] = moved return out + + +def same_state(a: Snapshot, b: Snapshot) -> bool: + """Do two reads taken after the save show the same database?""" + + if (a.objects, a.code_visible) != (b.objects, b.code_visible): + return False + if set(a.tables) != set(b.tables): + return False + return all( + (x.mode, x.schema, x.count, x.counter, x.max_auto, x.rows) + == (y.mode, y.schema, y.count, y.counter, y.max_auto, y.rows) + for x, y in ((a.tables[n], b.tables[n]) for n in a.tables) + ) diff --git a/packages/flow/openadapt_flow/runtime/effects/frame.py b/packages/flow/openadapt_flow/runtime/effects/frame.py index 094971350f..1623febaff 100644 --- a/packages/flow/openadapt_flow/runtime/effects/frame.py +++ b/packages/flow/openadapt_flow/runtime/effects/frame.py @@ -37,6 +37,16 @@ the saved value may no longer stand, so it blocks like collateral. Only changes outside the declared delta can be concurrent edits. +With ``frame.database``, another actor's entry explains a row only when it +is a write of the matching kind (insert for an added row, delete for a +removed one, update for a changed one) on the same table, naming the record +as it was before the change; each entry explains at most one row, and a row +whose record key changed is never explained. What the log can't show stays a +limit: an entry the app files under the wrong record (a write logged against +the chart the bot has open), or one colleague insert whose own row never +landed, still reads as that colleague's. List every app account the bot +holds in ``self``. + The judge reads nothing itself; callers pass the snapshots. Fail-safe: a missing baseline, an unreadable read, a record without its identity field or an unresolved self id makes the frame ``not_checked``, never ``held``. @@ -116,6 +126,10 @@ class FrameAttribution(BaseModel): #: Another actor's later write to a record the bot created can be told #: apart from the bot's own insert only through these. insert_events: list[str] = Field(default_factory=list) + #: The write events that remove a record (a subset of ``write_events``). + #: An added row is explained only by an insert, a removed one only by a + #: delete, a changed one only by the other write events. + delete_events: list[str] = Field(default_factory=list) #: ``audit_read`` only: the entry field naming the table written. Set, #: an entry attributes a change only to a row of that table. Required #: with ``frame.database``. @@ -152,8 +166,10 @@ def _shape(self) -> "FrameAttribution": raise ValueError("server_set applies to record_field attribution") if bool(self.event_field) != bool(self.write_events): raise ValueError("event_field and write_events go together") - if not set(self.insert_events) <= set(self.write_events): - raise ValueError("insert_events must be among write_events") + if not (set(self.insert_events) | set(self.delete_events)) <= set( + self.write_events + ): + raise ValueError("insert_events and delete_events must be write_events") elif self.record_keys or self.write_events or self.table_field: raise ValueError( "record_keys, write_events and table_field apply to audit_read" @@ -253,9 +269,12 @@ class FrameDatabase(BaseModel): #: Table -> columns that change on their own on every row (a last-seen #: time). ``frame.ignore_fields`` applies to surfaces only. ignore_columns: dict[str, list[str]] = Field(default_factory=dict) - #: Over this many rows a table is digested by key bucket: its existing - #: rows by digest, the rows the save adds in full. - max_rows: int = Field(default=20000, ge=1) + #: Over this many rows a table keeps, per row, its key and contents MACs + #: and cell MACs only for the columns attribution and side records read. + max_rows: int = Field(default=2000, ge=1) + #: Schemas the read account must see (MySQL); the snapshot refuses when + #: one isn't visible, so a schema the app writes can't be left out. + schemas: list[str] = Field(default_factory=list) #: Seconds to wait after the save before the read, so writes the app #: makes a moment later (background jobs) fall inside the window. settle_s: float = Field(default=0.0, ge=0) @@ -496,7 +515,7 @@ def judge_frame( identities: Optional[Mapping[str, str]] = None, new_row_defaults: Optional[Mapping[str, Mapping[str, Optional[str]]]] = None, unread_changes: Sequence[tuple[str, str]] = (), - hashers: Optional[Mapping[str, Callable[[Any], str]]] = None, + hashers: Optional[Mapping[str, Callable[[str, Any], str]]] = None, clock: tuple[str, str] = ("", ""), database_evidence: Optional[dict[str, Any]] = None, ) -> FrameVerdict: @@ -523,14 +542,14 @@ def judge_frame( ``CLOCK_DEFAULT`` a column the database fills with the time. unread_changes: (table, why) for tables that moved in a way no row comparison explains. Each is collateral. - hashers: Slice -> the keyed hash its baseline records hold instead of + hashers: Slice -> (column, value) -> the keyed MAC its baseline records hold instead of values (database tables). A post-save record is plain; it is hashed to compare with the baseline. clock: The database clock at the baseline and after the save. database_evidence: Counts for the receipt (tables compared, ...). """ - from openadapt_flow.runtime.effects.db_snapshot import KEY, is_default + from openadapt_flow.runtime.effects.db_snapshot import KEY, ROW, is_default attribution = spec.attribution mode: str = attribution.kind if attribution is not None else "none" @@ -585,18 +604,23 @@ def key_of(record: Mapping[str, Any], name: str = "") -> Any: def changed( name: str, before: Mapping[str, Any], after: Mapping[str, Any] ) -> list[str]: - """Fields that differ. A database baseline holds hashes, so the - post-save value is hashed to compare.""" + """Fields that differ. A database baseline row holds cell MACs (for + all columns, or only the reference columns of a large table) and its + contents MAC: the post-save value is MACed to compare, and a row + whose contents moved in no compared column says so.""" ignore = ignore_for(name) hasher = hashers.get(name) if hasher is None: return _changed_fields(before, after, ignore) - return sorted( + fields = sorted( k - for k in set(before) | set(after) - if k not in ignore and before.get(k) != hasher(after.get(k)) + for k in before + if k not in ignore and k != ROW and before.get(k) != hasher(k, after.get(k)) ) + if not fields and before.get(ROW) != after.get(ROW): + fields = ["(columns not compared)"] + return fields row_fields = database.new_row_fields if database is not None else {} @@ -608,7 +632,13 @@ def undeclared_columns( defaults = (new_row_defaults or {}).get(name) if defaults is None: return [] - allowed = written | set(row_fields.get(name, ())) | ignore_for(name) | set(also) + allowed = ( + written + | set(row_fields.get(name, ())) + | ignore_for(name) + | {ROW} + | set(also) + ) return sorted( c for c, v in record.items() @@ -635,26 +665,50 @@ def undeclared_columns( def table_of(name: str) -> Optional[str]: return name if name in hashers else surface_tables.get(name) - def entries_for(name: str, ref: Any, hashed: bool) -> list[dict[str, Any]]: + def event_kinds(change: Optional[str]) -> Optional[set[str]]: + """The write events that can explain this kind of change: an insert + for an added row, a delete for a removed one, an update otherwise. + None: any write (the frame names no event kinds).""" + + if attribution is None or not attribution.write_events or change is None: + return None + inserts, deletes = ( + set(attribution.insert_events), + set(attribution.delete_events), + ) + if change == "added": + return inserts + if change == "removed": + return deletes + return set(attribution.write_events) - inserts - deletes + + def entries_for( + name: str, ref: Any, field: Optional[str], kinds: Optional[set[str]] = None + ) -> list[dict[str, Any]]: """Window entries that name this record (and its table, when the - attribution says which table an entry wrote).""" + attribution says which table an entry wrote). ``field`` is set when + ``ref`` is a baseline MAC of that column.""" if window_entries is None or ref is None or attribution is None: return [] - hasher = hashers.get(name) if hashed else None + hasher = hashers.get(name) if field else None table = table_of(name) out = [] for e in window_entries: entry_ref = get_path(e, attribution.record_field or "") if entry_ref in (None, ""): continue - if (hasher(entry_ref) if hasher else str(entry_ref)) != str(ref): + if (hasher(field, entry_ref) if hasher else str(entry_ref)) != str(ref): continue if attribution.table_field: if table is None or str(get_path(e, attribution.table_field)) != table: continue if get_path(e, attribution.actor_field) in (None, ""): continue + if kinds is not None and ( + str(get_path(e, attribution.event_field or "")) not in kinds + ): + continue out.append(e) return out @@ -663,24 +717,46 @@ def ref_of( key: Any, before: Optional[Mapping[str, Any]], after: Optional[Mapping[str, Any]], - ) -> tuple[Any, bool]: - """What an audit entry names for this record, and whether it is a - baseline hash (the record is gone from the post-save read).""" + ) -> tuple[Any, Optional[str]]: + """What an audit entry names for this record, taken from the record + as it was BEFORE the change when there was one (so a write can't pick + its own excuse by moving a row), and the column when that value is a + baseline MAC.""" if attribution is None: - return key, False + return key, None field = attribution.record_keys.get(name, attribution.record_key) if not field: - return key, name in hashers and after is None - record = after if after is not None else before + return key, None + record = before if before is not None else after value = None if record is None else record.get(field) - hashed = name in hashers and after is None - return (None if value in (None, "") else str(value)), hashed + hashed = name in hashers and before is not None + return (None if value in (None, "") else str(value)), ( + field if hashed else None + ) - def actors(name: str, ref: Any, hashed: bool) -> set[str]: + def key_moved( + name: str, before: Mapping[str, Any], after: Mapping[str, Any] + ) -> bool: + """Did the column an audit entry names change? Then no entry about + either chart can explain the row.""" + + if attribution is None or attribution.kind != "audit_read": + return False + field = attribution.record_keys.get(name, attribution.record_key) + if not field or field not in before: + return False + hasher = hashers.get(name) + if hasher is not None: + return before.get(field) != hasher(field, after.get(field)) + return not _same(before.get(field), after.get(field)) + + def actors( + name: str, ref: Any, field: Optional[str], kinds: Optional[set[str]] = None + ) -> set[str]: return { str(get_path(e, attribution.actor_field)) # type: ignore[union-attr] - for e in entries_for(name, ref, hashed) + for e in entries_for(name, ref, field, kinds) } def other_on_target( @@ -694,8 +770,8 @@ def other_on_target( if attribution is None: return False if attribution.kind == "audit_read": - ref, hashed = ref_of(name, key, before, after) - return bool(actors(name, ref, hashed) - self_ids) + ref, field = ref_of(name, key, before, after) + return bool(actors(name, ref, field) - self_ids) if after is None: return False if before is not None and not changed(name, before, after): @@ -712,13 +788,31 @@ def bot_only_inserted(name: str, ref: Any) -> bool: inserts = set(attribution.insert_events) mine = [ e - for e in entries_for(name, ref, False) + for e in entries_for(name, ref, None) if str(get_path(e, attribution.actor_field)) in self_ids ] return all( str(get_path(e, attribution.event_field or "")) in inserts for e in mine ) + def inserted_by_other(name: str, key: Any, record: Mapping[str, Any]) -> bool: + """Does another actor's insert entry name this new record? (A look + only: it doesn't use the entry up.)""" + + if attribution is None or attribution.kind != "audit_read": + return False + ref, field = ref_of(name, key, None, record) + found = actors(name, ref, field, event_kinds("added")) + return ( + bool(found) + and not (found & self_ids) + and not (actors(name, ref, field) & self_ids) + ) + + # Each other actor's entry explains at most one row: a colleague's one + # insert can't cover two stray rows. + used: set[int] = set() + def attribute( name: str, key: Any, @@ -728,13 +822,31 @@ def attribute( if attribution is None: return "unattributed" if attribution.kind == "audit_read": - ref, hashed = ref_of(name, key, before, after) + ref, field = ref_of(name, key, before, after) if window_entries is None or ref is None: return "unattributed" - who = actors(name, ref, hashed) - if who & self_ids: + if ( + before is not None + and after is not None + and key_moved(name, before, after) + ): + return "unattributed" # the row moved between charts + if actors(name, ref, field) & self_ids: return "self" - return "other" if who else "unattributed" + change = ( + "added" if before is None else "removed" if after is None else "changed" + ) + kinds = event_kinds(change) + for e in entries_for(name, ref, field, kinds): + if ( + id(e) in used + or str(get_path(e, attribution.actor_field)) in self_ids + ): + continue + if attribution.write_events: + used.add(id(e)) + return "other" + return "unattributed" # record_field: the record says who last changed it. if after is None: return "unattributed" # a removed record can't say who removed it @@ -792,7 +904,7 @@ def new_row( if not extra: return ref, _hashed = ref_of(name, key, None, after) - others = actors(name, ref, False) - self_ids + others = actors(name, ref, None) - self_ids change = FrameChange(name, "added", "self", extra, on_target=True, key=key) if others and bot_only_inserted(name, ref): change.attribution = "other" @@ -822,11 +934,8 @@ def new_row( continue if any(record_matches(dict(record), s) for s in sels): links.add(str(value)) - elif ( - attribute( - side.of_surface, key_of(record, side.of_surface), None, record - ) - == "other" + elif inserted_by_other( + side.of_surface, key_of(record, side.of_surface), record ): theirs.add(str(value)) side_links.append(links) @@ -850,7 +959,7 @@ def new_row( def declared(record: Mapping[str, Any], hashed: bool = False) -> bool: if hashed and hasher is not None: return any( - all(record.get(k) == hasher(v) for k, v in s.items()) + all(record.get(k) == hasher(k, v) for k, v in s.items()) for s in selectors ) return any(record_matches(dict(record), s) for s in selectors) @@ -917,14 +1026,14 @@ def declared(record: Mapping[str, Any], hashed: bool = False) -> bool: fields = changed(name, before, after) # type: ignore[arg-type] stray = [f for f in fields if f not in may_change] if other_on_target(name, key, before, after): - ref, hashed = ref_of(name, key, before, after) + ref, field = ref_of(name, key, before, after) if ( fields and not (set(fields) & may_change) and attribution is not None and attribution.write_events and attribution.table_field - and not (actors(name, ref, hashed) & self_ids) + and not (actors(name, ref, field) & self_ids) ): # Another actor wrote, by a write to this table, only # fields this save doesn't declare, and the bot wrote diff --git a/packages/flow/openadapt_flow/verify_run.py b/packages/flow/openadapt_flow/verify_run.py index 146dc3df3c..25834dd962 100644 --- a/packages/flow/openadapt_flow/verify_run.py +++ b/packages/flow/openadapt_flow/verify_run.py @@ -460,12 +460,29 @@ def _audit_sql(cfg: VerifyConfig) -> dict[str, str]: return {} +def _ref_columns(cfg: VerifyConfig) -> list[str]: + """Columns attribution and side records read: kept as cell MACs in + every table, however large.""" + + frame = cfg.frame + if frame is None: + return [] + cols = {frame.identity_field} if frame.identity_field else set() + a = frame.attribution + if a is not None: + cols |= {a.record_key} if a.record_key else set() + cols |= set(a.record_keys.values()) + for side in frame.side_records: + cols |= {side.link_field, side.of_field} + return sorted(c for c in cols if c) + + def _read_database( cfg: VerifyConfig, salt: bytes, before: Optional[Any] = None ) -> Optional[Any]: """The whole-database snapshot, or None when it can't be read.""" - from openadapt_flow.runtime.effects.db_snapshot import take_snapshot + from openadapt_flow.runtime.effects.db_snapshot import mac_key, take_snapshot database = cfg.frame.database if cfg.frame is not None else None if database is None: @@ -474,21 +491,32 @@ def _read_database( return take_snapshot( _db_connect(database.read), database.dialect, - key=salt, + key=mac_key(salt), modes=database.modes(), # type: ignore[arg-type] max_rows=database.max_rows, before=before, extra_queries=_audit_sql(cfg), + ignore=database.ignore_columns, + ref_columns=_ref_columns(cfg), + schemas=database.schemas, ) except Exception: # noqa: BLE001 - unreadable -> the frame is not checked return None +def _mac_key(salt: bytes) -> bytes: + from openadapt_flow.runtime.effects.db_snapshot import mac_key + + return mac_key(salt) + + def _audit_ids_each(entries: Sequence[Mapping[str, Any]], salt: bytes) -> list[str]: from openadapt_flow.runtime.effects.effect import stable_id return [ - hmac.new(salt, repr(stable_id(dict(e))).encode(), hashlib.sha256).hexdigest() + hmac.new( + _mac_key(salt), repr(stable_id(dict(e))).encode(), hashlib.sha256 + ).hexdigest() for e in entries ] @@ -500,7 +528,9 @@ def _audit_ids(entries: Sequence[Mapping[str, Any]], salt: bytes) -> list[str]: from openadapt_flow.runtime.effects.effect import stable_id return sorted( - hmac.new(salt, repr(stable_id(dict(e))).encode(), hashlib.sha256).hexdigest() + hmac.new( + _mac_key(salt), repr(stable_id(dict(e))).encode(), hashlib.sha256 + ).hexdigest() for e in entries ) @@ -514,23 +544,30 @@ class _DatabaseFrame: clock: tuple[str, str] evidence: dict[str, Any] audit_after: Optional[list[dict[str, Any]]] + #: The read account can't see triggers: what the save caused can't be + #: ruled out, so the frame is not checked. + blind: bool = False def _database_frame( cfg: VerifyConfig, before: EffectState, salt: bytes ) -> Optional[_DatabaseFrame]: - """Every table as a slice, from the baseline snapshot and one read now. + """The rows that moved, as slices, from the baseline and one read now. - None when there is no ``frame.database``. A baseline without a readable - snapshot, or an unreadable read now, gives one unreachable slice, so the - frame is not checked. + Only rows whose contents MAC differs (changed), and rows on one side only + (added or removed), reach the judge: a baseline row as its MACs, a row now + as read. None when there is no ``frame.database``. A baseline without a + readable snapshot, or an unreadable read now, gives one unreachable + slice, so the frame is not checked. """ from openadapt_flow.runtime.effects.db_snapshot import ( KEY, + ROW, Snapshot, SnapshotError, cell_hash, + mac_key, unpack, unread_reasons, ) @@ -558,6 +595,11 @@ def _database_frame( new = _read_database(cfg, salt, before=old) if new is None: return unreachable + if not (old.code_visible and new.code_visible): + blind = _DatabaseFrame({}, {}, [], {}, ("", ""), {}, None) + blind.blind = True + return blind + key = mac_key(salt) out = _DatabaseFrame({}, {}, [], {}, (old.clock, new.clock), {}, None) if "audit" in new.extra: out.audit_after = list(new.extra["audit"]) @@ -565,7 +607,6 @@ def _database_frame( out.unread.append( ("(database code)", "triggers, views, routines or events changed") ) - hasher = lambda value: cell_hash(salt, value) # noqa: E731 counts = {"compare": 0, "append_only": 0, "skip": 0} for name in sorted(set(old.tables) | set(new.tables)): a, b = old.tables.get(name), new.tables.get(name) @@ -580,21 +621,26 @@ def _database_frame( a is None or b is None or a.mode != "compare" + or b.mode != "compare" + or a.cell_columns != b.cell_columns or (reasons and reasons[0] in ("definition changed", "created or dropped")) ): continue - if b.rows is None and b.buckets is None: - continue # a whole-table digest: moved or not, nothing row by row + gone = [k for k in a.rows if k not in b.rows or b.rows[k][0] != a.rows[k][0]] + now = [k for k in b.rows if k not in a.rows or a.rows[k][0] != b.rows[k][0]] before_rows = [ - {KEY: k, **unpack(a.columns, cells)} for k, cells in (a.rows or {}).items() + {KEY: k, ROW: a.rows[k][0], **unpack(a.cell_columns, a.rows[k][1])} + for k in gone ] - after_rows = [{KEY: k, **row} for k, row in b.values.items()] + after_rows = [{KEY: k, ROW: b.rows[k][0], **b.values.get(k, {})} for k in now] out.slices[name] = ( EffectState(substrate="database", reachable=True, records=before_rows), after_rows, ) out.defaults[name] = dict(b.defaults) - out.hashers[name] = hasher + out.hashers[name] = ( + lambda table: lambda column, value: cell_hash(key, value, table, column) + )(name) out.evidence = { "tables_compared": counts.get("compare", 0), "tables_append_only": counts.get("append_only", 0), @@ -602,6 +648,7 @@ def _database_frame( "tables_without_snapshot_isolation": sum( 1 for t in new.tables.values() if t.non_transactional ), + "schemas": list(new.schemas), "allow_list_sha256": database.allow_list_sha256(), } return out @@ -679,6 +726,15 @@ def read_of(name: str) -> str: slices[""] = (before, _post_records(after)) identities: Optional[dict[str, str]] = None db = _database_frame(cfg, before, salt) + if db is not None and db.blind: + return FrameVerdict( + status="not_checked", + reason=( + "the database read account can't see triggers (on MySQL it " + "needs TRIGGER), so a trigger planted in the window could pass" + ), + attribution=spec.attribution.kind if spec.attribution else "none", + ) if db is not None: from openadapt_flow.runtime.effects.db_snapshot import KEY diff --git a/packages/flow/tests/test_frame_database.py b/packages/flow/tests/test_frame_database.py index 2d3a8a559c..b403600e50 100644 --- a/packages/flow/tests/test_frame_database.py +++ b/packages/flow/tests/test_frame_database.py @@ -18,8 +18,10 @@ from openadapt_flow.runtime.effects.db_snapshot import ( CLOCK_DEFAULT, + cell_hash, is_default, take_snapshot, + unpack, ) from openadapt_flow.verify_run import ( capture_baseline, @@ -56,6 +58,7 @@ "event_field": "event", "write_events": ["insert", "update", "delete"], "insert_events": ["insert"], + "delete_events": ["delete"], "table_field": "tbl", } @@ -456,8 +459,12 @@ def test_a_table_wider_than_one_json_call_is_read(tmp_path: Path) -> None: con.commit() con.close() snap = take_snapshot(lambda: sqlite3.connect(path), "sqlite", key=b"k") - (row,) = snap.tables["wide"].values.values() - assert row["c149"] == "last" and len(row) == 151 + table = snap.tables["wide"] + ((_row_mac, cells),) = table.rows.values() + assert unpack(table.cell_columns, cells)["c149"] == cell_hash( + b"k", "last", "wide", "c149" + ) + assert len(table.cell_columns) == 151 assert "last" not in json.dumps(snap.to_json()) @@ -528,3 +535,130 @@ def colleague(con: sqlite3.Connection) -> None: tmp_path / "b", lambda con: con.execute(stray), save=save, frame=frame ) assert _frame(result) == "refuted" + + +# -- round 2 of the review --------------------------------------------------------- + + +def _colleague(pid: int, event: str, table: str = "history_data") -> str: + return ( + "INSERT INTO log (user, patient_id, event, tbl) " + f"VALUES ('clinic2', {pid}, '{event}', '{table}')" + ) + + +def test_l1_a_colleagues_write_on_a_large_table_is_concurrent(tmp_path: Path) -> None: + def colleague(con: sqlite3.Connection) -> None: + con.execute("UPDATE patient_data SET phone_cell = '1' WHERE pid = 1") + con.execute(_colleague(1, "update", "patient_data")) + + assert _frame(_run(tmp_path, colleague, max_rows=1)) == "held" + + +def test_l2_ignore_columns_hold_on_a_large_table(tmp_path: Path) -> None: + moved = "UPDATE patient_data SET phone_cell = 'seen' WHERE pid = 1" + result = _run( + tmp_path, + lambda con: con.execute(moved), + max_rows=1, + ignore_columns={"patient_data": ["phone_cell"]}, + ) + assert _frame(result) == "held" + + +def test_a1_a_row_moved_to_a_colleagues_chart_is_not_excused(tmp_path: Path) -> None: + def moved(con: sqlite3.Connection) -> None: + con.execute("UPDATE history_data SET pid = 3 WHERE id = 1") + con.execute(_colleague(3, "update")) + + assert _frame(_run(tmp_path, moved)) == "refuted" + + +def test_a2_an_update_entry_never_excuses_an_added_row(tmp_path: Path) -> None: + def stray(con: sqlite3.Connection) -> None: + con.execute("INSERT INTO history_data (pid, date) VALUES (1, '2026-10-25')") + con.execute(_colleague(1, "update")) + + assert _frame(_run(tmp_path, stray)) == "refuted" + + +def test_a3_one_insert_entry_excuses_one_row(tmp_path: Path) -> None: + def two(con: sqlite3.Connection) -> None: + con.execute("INSERT INTO history_data (pid, date) VALUES (1, '2026-10-25')") + con.execute("INSERT INTO history_data (pid, date) VALUES (1, '2026-10-26')") + con.execute(_colleague(1, "insert")) + + assert _frame(_run(tmp_path, two)) == "refuted" + + +def test_a4_an_update_entry_never_excuses_a_removed_row(tmp_path: Path) -> None: + def gone(con: sqlite3.Connection) -> None: + con.execute("DELETE FROM history_data WHERE pid = 1") + con.execute(_colleague(1, "update")) + + assert _frame(_run(tmp_path, gone)) == "refuted" + + +def test_a_delete_entry_excuses_a_removed_row(tmp_path: Path) -> None: + def gone(con: sqlite3.Connection) -> None: + con.execute("DELETE FROM history_data WHERE pid = 1") + con.execute(_colleague(1, "delete")) + + assert _frame(_run(tmp_path, gone)) == "held" + + +def test_the_baseline_alone_cant_be_tested_against(tmp_path: Path) -> None: + from openadapt_flow.runtime.effects.db_snapshot import cell_hash as mac + + _run(tmp_path) + base = json.loads((tmp_path / "baseline.json").read_text()) + salt = bytes.fromhex(base["salt"]) + table = base["state"]["detail"]["openadapt:frame_database"]["tables"][ + "patient_data" + ] + cells = [ + unpack(table["cell_columns"], packed)["fname"] + for _row, packed in table["rows"].values() + ] + # The salt in the baseline is not the MAC key: guesses can't be tested. + assert mac(salt, "Wendell", "patient_data", "fname") not in cells + + +def test_a_counter_on_a_column_that_is_not_the_key(tmp_path: Path) -> None: + db = tmp_path / "auto.sqlite" + con = sqlite3.connect(db) + con.executescript( + "CREATE TABLE t (id INTEGER PRIMARY KEY AUTOINCREMENT, v TEXT);" + "INSERT INTO t (v) VALUES ('a');" + ) + con.commit() + con.close() + before = take_snapshot(lambda: sqlite3.connect(db), "sqlite", key=b"k") + con = sqlite3.connect(db) + con.execute("INSERT INTO t (v) VALUES ('b')") + con.commit() + con.close() + after = take_snapshot( + lambda: sqlite3.connect(db), "sqlite", key=b"k", before=before + ) + from openadapt_flow.runtime.effects.db_snapshot import unread_reasons + + assert unread_reasons(before.tables["t"], after.tables["t"]) == [] + + +def test_a_read_that_cant_see_triggers_is_not_checked( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + import openadapt_flow.runtime.effects.db_snapshot as ds + + real = ds.take_snapshot + + def blind(*args: Any, **kwargs: Any) -> Any: + snap = real(*args, **kwargs) + snap.code_visible = False + return snap + + monkeypatch.setattr(ds, "take_snapshot", blind) + result = _run(tmp_path) + assert _frame(result) == "not_checked" + assert result["receipt"]["outcome"] != "verified" From e75b1d025583491e9c07bd9c955c31a7f0a7877a Mon Sep 17 00:00:00 2001 From: abrichr Date: Wed, 28 Oct 2026 17:04:42 -0400 Subject: [PATCH 12/19] frame: tests use their own host secret; document that a deployment keeps it from the agent The test suite created the real ~/.config/openadapt/frame.key; an autouse fixture now sets OPENADAPT_FRAME_SECRET per test. The default key file only keeps the secret out of the baseline, since a same-user agent can read it: a deployment sets the secret from a store the agent can't read. Co-Authored-By: Claude Opus 5.5 --- .../flow/openadapt_flow/runtime/effects/db_snapshot.py | 9 +++++++-- packages/flow/tests/conftest.py | 4 ++++ 2 files changed, 11 insertions(+), 2 deletions(-) diff --git a/packages/flow/openadapt_flow/runtime/effects/db_snapshot.py b/packages/flow/openadapt_flow/runtime/effects/db_snapshot.py index ec69f3ea25..52e1bc8b66 100644 --- a/packages/flow/openadapt_flow/runtime/effects/db_snapshot.py +++ b/packages/flow/openadapt_flow/runtime/effects/db_snapshot.py @@ -81,8 +81,13 @@ class SnapshotError(RuntimeError): def _host_secret() -> bytes: """The host's frame secret: ``OPENADAPT_FRAME_SECRET`` (hex), the file ``OPENADAPT_FRAME_SECRET_FILE``, or ``~/.config/openadapt/frame.key`` - (created owner-only on first use). Keep it unreadable to the agent's - account; it is never written into a baseline.""" + (created owner-only on first use). It is never written into a baseline. + + The default file only keeps the secret out of the baseline: an agent + running as the same OS user can read it. A deployment sets + ``OPENADAPT_FRAME_SECRET`` (or the file) from a store the agent's account + can't read, so a baseline the agent can see still can't be tested + against guesses.""" env = os.environ.get("OPENADAPT_FRAME_SECRET") if env: diff --git a/packages/flow/tests/conftest.py b/packages/flow/tests/conftest.py index 84da92ac8e..c4ccc8e684 100644 --- a/packages/flow/tests/conftest.py +++ b/packages/flow/tests/conftest.py @@ -45,3 +45,7 @@ def _isolate_durable_authority(tmp_path: Path, monkeypatch: pytest.MonkeyPatch) # Offline Seals are on by default; keep each test's Seal key and batch # queue out of the real home directory. monkeypatch.setenv("OPENADAPT_SEAL_HOME", str(tmp_path / ".seal-home")) + # The frame's host secret: a per-test value, so a test never creates or + # reads the real ~/.config/openadapt/frame.key. + monkeypatch.setenv("OPENADAPT_FRAME_SECRET", "11" * 32) + monkeypatch.delenv("OPENADAPT_FRAME_SECRET_FILE", raising=False) From cc06e4ee1a3f01bf30f85d5609e978787cfceb76 Mon Sep 17 00:00:00 2001 From: abrichr Date: Wed, 28 Oct 2026 17:20:38 -0400 Subject: [PATCH 13/19] frame: the host secret is created whole and race-free on first use Two processes starting together could read a half-written key file and MAC the baseline and the read after it with different keys. The secret is now written to a private temp file and linked into place; the loser reads the winner's. Co-Authored-By: Claude Opus 5.5 --- .../runtime/effects/db_snapshot.py | 17 +++++++++-- packages/flow/tests/test_frame_secret.py | 30 +++++++++++++++++++ 2 files changed, 45 insertions(+), 2 deletions(-) create mode 100644 packages/flow/tests/test_frame_secret.py diff --git a/packages/flow/openadapt_flow/runtime/effects/db_snapshot.py b/packages/flow/openadapt_flow/runtime/effects/db_snapshot.py index 52e1bc8b66..f25da29ec8 100644 --- a/packages/flow/openadapt_flow/runtime/effects/db_snapshot.py +++ b/packages/flow/openadapt_flow/runtime/effects/db_snapshot.py @@ -97,11 +97,24 @@ def _host_secret() -> bytes: or Path.home() / ".config" / "openadapt" / "frame.key" ) if not path.exists(): + # Written whole to a private temp file, then linked into place: a + # process that loses the race reads the winner's secret, never a + # half-written one. path.parent.mkdir(parents=True, exist_ok=True) - fd = os.open(path, os.O_WRONLY | os.O_CREAT | os.O_EXCL, 0o600) + tmp = path.with_name(f".{path.name}.{os.getpid()}.{secrets.token_hex(4)}") + fd = os.open(tmp, os.O_WRONLY | os.O_CREAT | os.O_EXCL, 0o600) with os.fdopen(fd, "wb") as handle: handle.write(secrets.token_bytes(32)) - return path.read_bytes() + try: + os.link(tmp, path) + except FileExistsError: + pass + finally: + os.unlink(tmp) + secret = path.read_bytes() + if len(secret) < 32: + raise SnapshotError(f"the frame secret {path} is too short") + return secret def mac_key(salt: bytes) -> bytes: diff --git a/packages/flow/tests/test_frame_secret.py b/packages/flow/tests/test_frame_secret.py new file mode 100644 index 0000000000..413a4f293c --- /dev/null +++ b/packages/flow/tests/test_frame_secret.py @@ -0,0 +1,30 @@ +"""The frame's host secret: created once, whole, whatever the race.""" + +from __future__ import annotations + +import threading +from pathlib import Path + +import pytest + +from openadapt_flow.runtime.effects import db_snapshot + + +def test_racing_first_uses_share_one_whole_secret( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + monkeypatch.delenv("OPENADAPT_FRAME_SECRET", raising=False) + path = tmp_path / "frame.key" + monkeypatch.setenv("OPENADAPT_FRAME_SECRET_FILE", str(path)) + seen: list[bytes] = [] + threads = [ + threading.Thread(target=lambda: seen.append(db_snapshot._host_secret())) + for _ in range(8) + ] + for t in threads: + t.start() + for t in threads: + t.join() + assert len(set(seen)) == 1 and len(seen[0]) == 32 + assert path.stat().st_mode & 0o777 == 0o600 + assert [p.name for p in tmp_path.iterdir()] == ["frame.key"] From 8c22c39171d487cdeda0ddbfce369ab625171d91 Mon Sep 17 00:00:00 2001 From: abrichr Date: Wed, 28 Oct 2026 19:21:16 -0400 Subject: [PATCH 14/19] frame: narrow types for mypy (renames only, no behaviour change) --- .../flow/openadapt_flow/runtime/effects/db_snapshot.py | 4 ++-- packages/flow/openadapt_flow/runtime/effects/frame.py | 8 +++++--- packages/flow/openadapt_flow/verify_run.py | 6 +++--- 3 files changed, 10 insertions(+), 8 deletions(-) diff --git a/packages/flow/openadapt_flow/runtime/effects/db_snapshot.py b/packages/flow/openadapt_flow/runtime/effects/db_snapshot.py index f25da29ec8..8bfff54f04 100644 --- a/packages/flow/openadapt_flow/runtime/effects/db_snapshot.py +++ b/packages/flow/openadapt_flow/runtime/effects/db_snapshot.py @@ -506,8 +506,8 @@ def _definitions(s: _Session) -> tuple[dict[str, _Def], str]: _digest([sql]), auto_column=generated, ) - code = [r for r in master if r[0] in ("trigger", "view")] - return out, _digest(code) + sqlite_code = [r for r in master if r[0] in ("trigger", "view")] + return out, _digest(sqlite_code) def _cell(dialect: Dialect, column: str, binary: bool) -> str: diff --git a/packages/flow/openadapt_flow/runtime/effects/frame.py b/packages/flow/openadapt_flow/runtime/effects/frame.py index 1623febaff..4b93602748 100644 --- a/packages/flow/openadapt_flow/runtime/effects/frame.py +++ b/packages/flow/openadapt_flow/runtime/effects/frame.py @@ -698,7 +698,9 @@ def entries_for( entry_ref = get_path(e, attribution.record_field or "") if entry_ref in (None, ""): continue - if (hasher(field, entry_ref) if hasher else str(entry_ref)) != str(ref): + if ( + hasher(field, entry_ref) if hasher and field else str(entry_ref) + ) != str(ref): continue if attribution.table_field: if table is None or str(get_path(e, attribution.table_field)) != table: @@ -1087,7 +1089,7 @@ def declared(record: Mapping[str, Any], hashed: bool = False) -> bool: declared_keys.add(str(ref_of(name, key, None, after)[0])) new_row(name, key, after, (side.link_field,)) continue - theirs = any( + theirs_side = any( side.surface == name and str(after.get(side.link_field)) in other_links[i] for i, side in enumerate(spec.side_records) @@ -1095,7 +1097,7 @@ def declared(record: Mapping[str, Any], hashed: bool = False) -> bool: change = FrameChange( name, "added", - "other" if theirs else attribute(name, key, None, after), + "other" if theirs_side else attribute(name, key, None, after), sorted(_without(after, ignore_for(name))), key=key, ) diff --git a/packages/flow/openadapt_flow/verify_run.py b/packages/flow/openadapt_flow/verify_run.py index 85099ce32f..badc88aa63 100644 --- a/packages/flow/openadapt_flow/verify_run.py +++ b/packages/flow/openadapt_flow/verify_run.py @@ -749,10 +749,10 @@ def read_of(name: str) -> str: ) if routes is None: routes = {str(i): [""] for i in range(len(bound_effects))} - views = spec.database.views if spec.database is not None else {} + db_views = spec.database.views if spec.database is not None else {} routes = {k: list(v) for k, v in routes.items()} - for own in routes.values(): - own.extend(t for t, surface in views.items() if surface in own) + for route in routes.values(): + route.extend(t for t, surface in db_views.items() if surface in route) slices.update(db.slices) identities = {name: KEY for name in db.slices} audit_before: Optional[list[dict[str, Any]]] = None From 45c8f5e826ee05f4fe449f9b460ea5bc345eb958 Mon Sep 17 00:00:00 2001 From: abrichr Date: Wed, 28 Oct 2026 19:38:17 -0400 Subject: [PATCH 15/19] frame: record_key refs never excuse what they don't name Adversarial review of #1570 found three silent passes and one more held: - a row whose record_key changed (a note refiled to another patient) was matched by its new value only; it now names neither and is collateral; - a bot entry was set aside when its ref equalled a moved row's id; moved rows are now compared by their record_key value; - a declared record with no record_key value added "None" to the declared set, excusing every bot entry without a patient; the frame is now not_checked; - a side record linked to an existing record's id was part of the save; a link already in the baseline is no longer a new record's. Nothing changes when record_key and side_records are unset. --- .../openadapt_flow/runtime/effects/frame.py | 46 +++++++++-- packages/flow/tests/test_frame_links.py | 77 +++++++++++++++++++ 2 files changed, 116 insertions(+), 7 deletions(-) diff --git a/packages/flow/openadapt_flow/runtime/effects/frame.py b/packages/flow/openadapt_flow/runtime/effects/frame.py index d36c0b11f2..cae81843f3 100644 --- a/packages/flow/openadapt_flow/runtime/effects/frame.py +++ b/packages/flow/openadapt_flow/runtime/effects/frame.py @@ -420,9 +420,15 @@ def ref_of( if attribution is None or not attribution.record_key: return key - record = after if after is not None else before - value = None if record is None else record.get(attribution.record_key) - return None if value in (None, "") else str(value) + values = { + None if v in (None, "") else str(v) + for v in ( + r.get(attribution.record_key) for r in (before, after) if r is not None + ) + } + # A record moved to another ``record_key`` (a note refiled to another + # patient) names neither: no entry can explain it. + return values.pop() if len(values) == 1 else None def other_on_target( key: Any, @@ -496,6 +502,8 @@ def attribute( collateral: list[FrameChange] = [] concurrent: list[FrameChange] = [] declared_keys: set[str] = set() + #: ``record_key`` only: what an audit entry names for each compared record. + refs: dict[tuple[str, str], Optional[str]] = {} compared = 0 def note(change: FrameChange) -> None: @@ -521,12 +529,19 @@ def selectors_for(name: str) -> list[Any]: if before_state is not None and after_records is not None: sels = selectors_for(side.of_surface) old = {key_of(r) for r in before_state.records} + # A link an existing record already has is not a new record's: a + # side row for it belongs to that record's chart. + taken = { + str(r.get(side.of_field)) + for r in before_state.records + if r.get(side.of_field) not in (None, "") + } for record in after_records: if key_of(record) in old: continue if any(record_matches(dict(record), s) for s in sels): value = record.get(side.of_field) - if value not in (None, ""): + if value not in (None, "") and str(value) not in taken: links.add(str(value)) side_links.append(links) side_found.append({link: 0 for link in links}) @@ -577,8 +592,16 @@ def declared(record: Mapping[str, Any]) -> bool: is_declared = (before is not None and declared(before)) or ( after is not None and declared(after) ) + ref = ref_of(key, before, after) + refs[(name, str(key))] = None if ref is None else str(ref) if is_declared: - declared_keys.add(str(ref_of(key, before, after))) + if ref is None: + return _not_checked( + "a declared record carries no record_key value (or it " + "changed), so its log entries can't be matched", + mode, + ) + declared_keys.add(str(ref)) if other_on_target(key, before, after): # The saved value may not stand: never excused. collateral.append( @@ -626,7 +649,8 @@ def declared(record: Mapping[str, Any]) -> bool: # A side record of a new declared record: part of the save. link = str(after.get(spec.side_records[side_index].link_field)) side_found[side_index][link] += 1 - declared_keys.add(str(ref_of(key, None, after))) + if ref is not None: + declared_keys.add(str(ref)) continue note( FrameChange( @@ -674,7 +698,15 @@ def declared(record: Mapping[str, Any]) -> bool: # the bot's own log entries can't be checked: say so. mode = "audit_read_unavailable" else: - moved = {str(c.key) for c in collateral + concurrent} + if attribution.record_key: # type: ignore[union-attr] + # Compare like with like: entries name ``record_key`` values. + moved = { + r + for c in collateral + concurrent + if (r := refs.get((c.surface, str(c.key)))) is not None + } + else: + moved = {str(c.key) for c in collateral + concurrent} for entry in window_entries: actor = get_path(entry, attribution.actor_field) # type: ignore[union-attr] ref = get_path(entry, attribution.record_field or "") # type: ignore[union-attr] diff --git a/packages/flow/tests/test_frame_links.py b/packages/flow/tests/test_frame_links.py index be00c6bbef..47e0a1df77 100644 --- a/packages/flow/tests/test_frame_links.py +++ b/packages/flow/tests/test_frame_links.py @@ -102,3 +102,80 @@ def test_a_missing_or_doubled_side_record_refutes() -> None: def test_a_row_for_another_chart_is_still_collateral() -> None: assert _with_history({"id": 6, "pid": 2}, {"id": 7, "pid": 1}) == "refuted" + + +# Adversarial review of #1570: each of these ended ``held``. + +_TARGET = [Effect(kind=EffectKind.RECORD_WRITTEN, match={"id": "10"}, field="body")] +_BOT_ON_1 = {"id": 100, "user": "bot", "patient_id": "1"} + + +def _judge(before: list[dict], after: list[dict], audit: list[dict]) -> str: + spec = FrameSpec(identity_field="id", attribution=ATTRIBUTION) + return judge_frame( + spec, + _TARGET, + {"": (_state(*before), after)}, + audit_before=[], + audit_after=audit, + ).status + + +def test_a_row_moved_to_another_record_key_is_never_excused() -> None: + moved = {"id": 11, "pid": 1, "body": "allergy"} + status = _judge( + [NOTE | {"body": "old"}, moved], + [NOTE, {**moved, "pid": 2}], + [_BOT_ON_1, {"id": 101, "user": "nurse", "patient_id": "2"}], + ) + assert status == "refuted" + + +def test_a_bot_entry_is_not_excused_by_a_row_id_that_equals_it() -> None: + other = {"id": 5, "pid": 3, "body": "x"} + status = _judge( + [NOTE | {"body": "old"}, other], + [NOTE, {**other, "body": "y"}], + [ + _BOT_ON_1, + {"id": 101, "user": "nurse", "patient_id": "3"}, + {"id": 102, "user": "bot", "patient_id": "5"}, + ], + ) + assert status == "refuted" + + +def test_a_declared_record_without_its_record_key_is_not_checked() -> None: + status = _judge( + [{"id": 10, "body": "old"}], + [{"id": 10, "body": "new"}], + [{"id": 102, "user": "bot"}], + ) + assert status == "not_checked" + + +def test_a_side_record_for_an_existing_link_is_collateral() -> None: + side = FrameSideRecord( + surface="history", link_field="pid", of_surface="patient", of_field="pid" + ) + old = {"id": 1, "pid": 1, "lname": "Old"} + verdict = judge_frame( + FrameSpec(identity_field="id", side_records=[side]), + [ + Effect( + kind=EffectKind.RECORD_WRITTEN, + match={"lname": "Newchart"}, + count_new_only=True, + probe="surface:patient", + ) + ], + { + "patient": (_state(old), [old, {"id": 2, "pid": 1, "lname": "Newchart"}]), + "history": ( + _state({"id": 5, "pid": 1}), + [{"id": 5, "pid": 1}, {"id": 6, "pid": 1}], + ), + }, + routes={"0": ["patient"]}, + ) + assert verdict.status == "refuted" From dc4cc3d6e05dd0a1a85e7ea96c8e19616a73e646 Mon Sep 17 00:00:00 2001 From: abrichr Date: Wed, 28 Oct 2026 19:55:42 -0400 Subject: [PATCH 16/19] frame: a side row must be filed under its parent's record_key Re-review of #1570: with link_field other than record_key, a side row linked to the new chart but filed under another patient was part of the save, and its record_key value excused that patient's bot log entries. It is now an ordinary unexplained addition. --- .../openadapt_flow/runtime/effects/frame.py | 11 +++++ packages/flow/tests/test_frame_links.py | 41 +++++++++++++++++++ 2 files changed, 52 insertions(+) diff --git a/packages/flow/openadapt_flow/runtime/effects/frame.py b/packages/flow/openadapt_flow/runtime/effects/frame.py index cae81843f3..c768838c86 100644 --- a/packages/flow/openadapt_flow/runtime/effects/frame.py +++ b/packages/flow/openadapt_flow/runtime/effects/frame.py @@ -523,8 +523,11 @@ def selectors_for(name: str) -> list[Any]: # Side records: the links each new declared record gives its side rows. side_links: list[set[str]] = [] side_found: list[dict[str, int]] = [] + #: Each link's parent ``ref_of`` value: a side row must name the same. + side_parent: list[dict[str, Any]] = [] for side in spec.side_records: links: set[str] = set() + parents: dict[str, Any] = {} before_state, after_records = slices.get(side.of_surface, (None, None)) if before_state is not None and after_records is not None: sels = selectors_for(side.of_surface) @@ -543,7 +546,9 @@ def selectors_for(name: str) -> list[Any]: value = record.get(side.of_field) if value not in (None, "") and str(value) not in taken: links.add(str(value)) + parents[str(value)] = ref_of(key_of(record), None, record) side_links.append(links) + side_parent.append(parents) side_found.append({link: 0 for link in links}) for name, (before_state, after_records) in slices.items(): @@ -642,6 +647,12 @@ def declared(record: Mapping[str, Any]) -> bool: for i, side in enumerate(spec.side_records) if side.surface == name and str(after.get(side.link_field)) in side_links[i] + # Filed under its parent's record_key, not another's. + and ( + attribution is None + or not attribution.record_key + or ref == side_parent[i][str(after.get(side.link_field))] + ) ), None, ) diff --git a/packages/flow/tests/test_frame_links.py b/packages/flow/tests/test_frame_links.py index 47e0a1df77..57002b5c33 100644 --- a/packages/flow/tests/test_frame_links.py +++ b/packages/flow/tests/test_frame_links.py @@ -179,3 +179,44 @@ def test_a_side_record_for_an_existing_link_is_collateral() -> None: routes={"0": ["patient"]}, ) assert verdict.status == "refuted" + + +def test_a_side_row_filed_under_another_record_key_is_collateral() -> None: + """Re-review of #1570: linked by row id, filed under another patient.""" + + side = FrameSideRecord( + surface="history", link_field="patient_row", of_surface="patient", of_field="id" + ) + old = {"id": 1, "pid": 1, "lname": "Old"} + history = {"id": 5, "pid": 1, "patient_row": 1} + + def status(pid: int) -> str: + return judge_frame( + FrameSpec( + identity_field="id", side_records=[side], attribution=ATTRIBUTION + ), + [ + Effect( + kind=EffectKind.RECORD_WRITTEN, + match={"lname": "New"}, + count_new_only=True, + probe="surface:patient", + ) + ], + { + "patient": (_state(old), [old, {"id": 2, "pid": 2, "lname": "New"}]), + "history": ( + _state(history), + [history, {"id": 6, "pid": pid, "patient_row": 2}], + ), + }, + routes={"0": ["patient"]}, + audit_before=[], + audit_after=[ + {"id": 1, "user": "bot", "patient_id": "2"}, + {"id": 2, "user": "bot", "patient_id": "7"}, + ], + ).status + + assert status(7) == "refuted" + assert status(1) == "refuted" From bf591b94d7c1dc90289faf8f1accebf6e1fdf287 Mon Sep 17 00:00:00 2001 From: abrichr Date: Wed, 28 Oct 2026 19:57:49 -0400 Subject: [PATCH 17/19] frame: a side row with no record_key value can still be its parent's --- packages/flow/openadapt_flow/runtime/effects/frame.py | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/packages/flow/openadapt_flow/runtime/effects/frame.py b/packages/flow/openadapt_flow/runtime/effects/frame.py index c768838c86..8da37ff0c1 100644 --- a/packages/flow/openadapt_flow/runtime/effects/frame.py +++ b/packages/flow/openadapt_flow/runtime/effects/frame.py @@ -647,11 +647,12 @@ def declared(record: Mapping[str, Any]) -> bool: for i, side in enumerate(spec.side_records) if side.surface == name and str(after.get(side.link_field)) in side_links[i] - # Filed under its parent's record_key, not another's. + # Filed under its parent's record_key (or none), not another's. and ( attribution is None or not attribution.record_key - or ref == side_parent[i][str(after.get(side.link_field))] + or ref + in (None, side_parent[i][str(after.get(side.link_field))]) ) ), None, From 95510abaca8a11ad28f90eb4ef9c14bf10538e96 Mon Sep 17 00:00:00 2001 From: abrichr Date: Wed, 28 Oct 2026 20:26:30 -0400 Subject: [PATCH 18/19] frame: with record_key, side records link by it; only collateral excuses Re-review round 3 of #1570 and of its port to #1582: - the parent-ref rule let an extra side row filed under another patient fall through to attribution, where a colleague's entry excused it. With record_key set, a side record must now link by record_key on both sides (validated), so a side row's link and its record_key are the same value; the parent-ref rule is gone. - moved records compared by record_key let another user's concurrent change to a patient excuse the bot's own entry for that patient (a write the frame doesn't read). Only collateral records excuse entries now. --- .../openadapt_flow/runtime/effects/frame.py | 25 ++++---- packages/flow/tests/test_frame_links.py | 58 ++++++++----------- 2 files changed, 36 insertions(+), 47 deletions(-) diff --git a/packages/flow/openadapt_flow/runtime/effects/frame.py b/packages/flow/openadapt_flow/runtime/effects/frame.py index 8da37ff0c1..ab2e099c7c 100644 --- a/packages/flow/openadapt_flow/runtime/effects/frame.py +++ b/packages/flow/openadapt_flow/runtime/effects/frame.py @@ -199,6 +199,15 @@ def _identity_for_audit(self) -> "FrameSpec": "audit_read attribution compares log entries with records by " "identity_field; set it" ) + key = self.attribution.record_key if self.attribution is not None else None + for side in self.side_records if key else (): + if side.link_field != key or side.of_field != key: + # Otherwise a side row's record_key and its link can name + # different records, and either can excuse the other's entries. + raise ValueError( + f"with record_key {key!r}, a side record links by it: set " + f"link_field and of_field to {key!r}" + ) return self def digest(self) -> str: @@ -523,11 +532,8 @@ def selectors_for(name: str) -> list[Any]: # Side records: the links each new declared record gives its side rows. side_links: list[set[str]] = [] side_found: list[dict[str, int]] = [] - #: Each link's parent ``ref_of`` value: a side row must name the same. - side_parent: list[dict[str, Any]] = [] for side in spec.side_records: links: set[str] = set() - parents: dict[str, Any] = {} before_state, after_records = slices.get(side.of_surface, (None, None)) if before_state is not None and after_records is not None: sels = selectors_for(side.of_surface) @@ -546,9 +552,7 @@ def selectors_for(name: str) -> list[Any]: value = record.get(side.of_field) if value not in (None, "") and str(value) not in taken: links.add(str(value)) - parents[str(value)] = ref_of(key_of(record), None, record) side_links.append(links) - side_parent.append(parents) side_found.append({link: 0 for link in links}) for name, (before_state, after_records) in slices.items(): @@ -647,13 +651,6 @@ def declared(record: Mapping[str, Any]) -> bool: for i, side in enumerate(spec.side_records) if side.surface == name and str(after.get(side.link_field)) in side_links[i] - # Filed under its parent's record_key (or none), not another's. - and ( - attribution is None - or not attribution.record_key - or ref - in (None, side_parent[i][str(after.get(side.link_field))]) - ) ), None, ) @@ -712,9 +709,11 @@ def declared(record: Mapping[str, Any]) -> bool: else: if attribution.record_key: # type: ignore[union-attr] # Compare like with like: entries name ``record_key`` values. + # Only records that already refute: another user's concurrent + # change to a patient never excuses the bot's own entry for it. moved = { r - for c in collateral + concurrent + for c in collateral if (r := refs.get((c.surface, str(c.key)))) is not None } else: diff --git a/packages/flow/tests/test_frame_links.py b/packages/flow/tests/test_frame_links.py index 57002b5c33..8c6d46a62b 100644 --- a/packages/flow/tests/test_frame_links.py +++ b/packages/flow/tests/test_frame_links.py @@ -181,42 +181,32 @@ def test_a_side_record_for_an_existing_link_is_collateral() -> None: assert verdict.status == "refuted" -def test_a_side_row_filed_under_another_record_key_is_collateral() -> None: - """Re-review of #1570: linked by row id, filed under another patient.""" +def test_with_a_record_key_a_side_record_links_by_it() -> None: + """Re-review of #1570: a side row linked by row id but filed under another + patient could excuse that patient's entries. Such a frame is refused.""" + + import pytest side = FrameSideRecord( surface="history", link_field="patient_row", of_surface="patient", of_field="id" ) - old = {"id": 1, "pid": 1, "lname": "Old"} - history = {"id": 5, "pid": 1, "patient_row": 1} + with pytest.raises(ValueError, match="links by it"): + FrameSpec(identity_field="id", side_records=[side], attribution=ATTRIBUTION) + FrameSpec(identity_field="id", side_records=[side]) # no record_key: allowed - def status(pid: int) -> str: - return judge_frame( - FrameSpec( - identity_field="id", side_records=[side], attribution=ATTRIBUTION - ), - [ - Effect( - kind=EffectKind.RECORD_WRITTEN, - match={"lname": "New"}, - count_new_only=True, - probe="surface:patient", - ) - ], - { - "patient": (_state(old), [old, {"id": 2, "pid": 2, "lname": "New"}]), - "history": ( - _state(history), - [history, {"id": 6, "pid": pid, "patient_row": 2}], - ), - }, - routes={"0": ["patient"]}, - audit_before=[], - audit_after=[ - {"id": 1, "user": "bot", "patient_id": "2"}, - {"id": 2, "user": "bot", "patient_id": "7"}, - ], - ).status - - assert status(7) == "refuted" - assert status(1) == "refuted" + +def test_a_colleagues_change_never_excuses_the_bots_entry_for_that_patient() -> None: + """Re-review of the #1582 port: the bot's entry for patient 7 (a write the + frame doesn't read) stays collateral though a nurse changed patient 7.""" + + other = {"id": 5, "pid": 7, "body": "x"} + status = _judge( + [NOTE | {"body": "old"}, other], + [NOTE, {**other, "body": "y"}], + [ + _BOT_ON_1, + {"id": 101, "user": "nurse", "patient_id": "7"}, + {"id": 102, "user": "bot", "patient_id": "7"}, + ], + ) + assert status == "refuted" From 0a53b852c426a57309b50c1108903644f3202186 Mon Sep 17 00:00:00 2001 From: abrichr Date: Wed, 28 Oct 2026 20:48:37 -0400 Subject: [PATCH 19/19] frame: at most count side rows per link of another actor's new chart are theirs Re-review of the #1570 port, round 2: a side row linked to a colleague's new chart was theirs with no count, so the bot's extra contact or history row for that chart ended held. Rows beyond the side record's count are now unattributed collateral. --- .../openadapt_flow/runtime/effects/frame.py | 26 ++++++++++++++----- packages/flow/tests/test_frame_database.py | 15 +++++++++++ 2 files changed, 35 insertions(+), 6 deletions(-) diff --git a/packages/flow/openadapt_flow/runtime/effects/frame.py b/packages/flow/openadapt_flow/runtime/effects/frame.py index da6118f994..e7ab381836 100644 --- a/packages/flow/openadapt_flow/runtime/effects/frame.py +++ b/packages/flow/openadapt_flow/runtime/effects/frame.py @@ -940,6 +940,9 @@ def new_row( side_links: list[set[str]] = [] other_links: list[set[str]] = [] side_found: list[dict[str, int]] = [] + #: Side rows seen per link of another actor's new record: at most + #: ``count`` each are theirs, the rest are unexplained. + other_found: list[dict[str, int]] = [] for side in spec.side_records: links: set[str] = set() theirs: set[str] = set() @@ -974,6 +977,7 @@ def new_row( side_links.append(links) other_links.append(theirs - links) side_found.append({link: 0 for link in links}) + other_found.append({link: 0 for link in theirs - links}) for name, (before_state, after_records) in slices.items(): if not before_state.reachable: @@ -1130,15 +1134,25 @@ def declared(record: Mapping[str, Any], hashed: bool = False) -> bool: declared_keys.add(str(side_ref)) new_row(name, key, after, (side.link_field,)) continue - theirs_side = any( - side.surface == name - and str(after.get(side.link_field)) in other_links[i] - for i, side in enumerate(spec.side_records) - ) + theirs_side = False + extra_side = False + for i, side in enumerate(spec.side_records): + other_link = str(after.get(side.link_field)) + if side.surface == name and other_link in other_links[i]: + other_found[i][other_link] += 1 + # Beyond the count, a side row of their chart is no + # more theirs than anyone's: unexplained. + theirs_side = other_found[i][other_link] <= side.count + extra_side = not theirs_side + break change = FrameChange( name, "added", - "other" if theirs_side else attribute(name, key, None, after), + "other" + if theirs_side + else "unattributed" + if extra_side + else attribute(name, key, None, after), sorted(_without(after, ignore_for(name))), key=key, ) diff --git a/packages/flow/tests/test_frame_database.py b/packages/flow/tests/test_frame_database.py index 6296f5fa88..c9f265988c 100644 --- a/packages/flow/tests/test_frame_database.py +++ b/packages/flow/tests/test_frame_database.py @@ -542,6 +542,21 @@ def colleague(con: sqlite3.Connection) -> None: tmp_path / "b", lambda con: con.execute(stray), save=save, frame=frame ) assert _frame(result) == "refuted" + # A second contact or history row for their chart is no more theirs than + # anyone's (re-review of the #1570 port). + for i, extra in enumerate( + ( + "INSERT INTO contact (foreign_id) VALUES (3)", + "INSERT INTO history_data (pid, date) VALUES (3, '2026-10-25')", + ) + ): + + def doubled(con: sqlite3.Connection, extra: str = extra) -> None: + colleague(con) + con.execute(extra) + + result = _run(tmp_path / f"c{i}", doubled, save=save, frame=frame) + assert _frame(result) == "refuted" # -- round 2 of the review ---------------------------------------------------------