From b74d6fe554f572663a69d3da671f37f292d9d608 Mon Sep 17 00:00:00 2001 From: abrichr Date: Fri, 30 Oct 2026 12:15:19 -0400 Subject: [PATCH 1/7] WIP lost-update version check --- .../openadapt_flow/runtime/effects/frame.py | 21 ++ .../runtime/effects/versions.py | 328 ++++++++++++++++++ packages/flow/openadapt_flow/verifier.py | 26 +- packages/flow/openadapt_flow/verify_run.py | 89 +++++ packages/flow/tests/test_frame_versions.py | 233 +++++++++++++ 5 files changed, 695 insertions(+), 2 deletions(-) create mode 100644 packages/flow/openadapt_flow/runtime/effects/versions.py create mode 100644 packages/flow/tests/test_frame_versions.py diff --git a/packages/flow/openadapt_flow/runtime/effects/frame.py b/packages/flow/openadapt_flow/runtime/effects/frame.py index 40db9e0a68..12c0182cfe 100644 --- a/packages/flow/openadapt_flow/runtime/effects/frame.py +++ b/packages/flow/openadapt_flow/runtime/effects/frame.py @@ -81,6 +81,7 @@ record_matches, stable_id, ) +from openadapt_flow.runtime.effects.versions import FrameVersions, VersionVerdict FrameStatus = Literal["held", "refuted", "not_checked"] Attribution = Literal["other", "self", "unattributed"] @@ -450,6 +451,19 @@ class FrameSpec(BaseModel): side_records: list[FrameSideRecord] = Field(default_factory=list) #: Diff the whole database (``FrameDatabase``). database: Optional[FrameDatabase] = None + #: The target's versions, read before and after the save + #: (``versions.FrameVersions``): a version in the window that isn't the + #: agent's own is a lost update. + versions: Optional[FrameVersions] = None + + @model_serializer(mode="wrap") + def _compact(self, handler: Any) -> dict[str, Any]: + # Unset, ``versions`` is left out, so a contract that doesn't use it + # keeps its digests (``frame_sha256``, the check digest). + data: dict[str, Any] = handler(self) + if data.get("versions") is None: + data.pop("versions", None) + return data @model_validator(mode="after") def _database_attribution(self) -> "FrameSpec": @@ -591,6 +605,8 @@ class FrameVerdict: added_by_others: set[tuple[str, str]] = field(default_factory=set) #: In memory only: app profile rules that excused a row change. excused_by: set[str] = field(default_factory=set) + #: ``frame.versions`` only: how the target's versions came out. + versions: Optional[VersionVerdict] = None #: In memory only, set when the frame held and ``recheck_after_s`` is #: set: what a later recheck compares with (the settled post-save #: snapshot as keyed MACs, the audit entries seen then as keyed hashes, @@ -621,6 +637,11 @@ def evidence(self, spec: FrameSpec) -> dict[str, Any]: 1 for c in self.collateral if c.other_on_target ), **({"database": self.database} if self.database is not None else {}), + **( + {"versions": self.versions.evidence()} + if self.versions is not None + else {} + ), } diff --git a/packages/flow/openadapt_flow/runtime/effects/versions.py b/packages/flow/openadapt_flow/runtime/effects/versions.py new file mode 100644 index 0000000000..006850114e --- /dev/null +++ b/packages/flow/openadapt_flow/runtime/effects/versions.py @@ -0,0 +1,328 @@ +"""Version history of the target: catch a lost update. + +Two snapshots can't see a lost update. The agent reads a record, another +user saves a change to it, and the agent saves its stale copy over theirs. +The record after the save looks like "the agent changed one field, the rest +is unchanged", which is what a correct update looks like too. Only the +record's versions show the second writer. + +``frame.versions`` names a read-only read that returns one row per version of +a record (FHIR ``_history`` or HAPI's ``hfj_res_ver``; a SQL ``version`` +column; the app's audit or action-history table), and which fields of a row +name the record, the version and, where the app records one, its author. +The verifier reads it before the write and after it. For every target record +that already existed (one a confirmed write effect names), the versions that +are new in the window must be the agent's own: + +- With ``author_field``: every new version's author must be one of ``self``. + A version by anyone else is another user's write inside the window: the + frame is refuted. A version with no author can't be attributed: not + checked. +- Without it: versions must be whole numbers that step by one per save + (HAPI's ``res_ver``, a ``version_number`` column), and exactly one step may + have happened (zero when nothing was written). Two steps mean someone else + saved too: refuted. A version that isn't a whole number: not checked. + +Anything the reads can't explain stops the run: a read that fails, a target +that existed but has no version, or one whose versions vanished. Never a +pass. Versions are stored in the baseline as keyed MACs of the record and the +version; only a whole-number version's value is kept, to count the steps. +""" + +from __future__ import annotations + +import hashlib +import re +from collections.abc import Mapping, Sequence +from dataclasses import dataclass, field +from typing import Any, Literal, Optional + +from pydantic import BaseModel, ConfigDict, Field, model_validator + +from openadapt_flow.runtime.effects.effect import ValueExpr + +VersionStatus = Literal["held", "refuted", "not_checked"] + +#: Why the version check stopped the run (closed values, for the evidence). +PROBLEMS = ( + "unreadable", + "no_target_id", + "missing", + "another_author", + "no_author", + "not_a_number", + "extra_steps", + "went_back", + "self_unresolved", +) + +_WHOLE = re.compile(r"^(0|[1-9][0-9]*)$") + + +class FrameVersions(BaseModel): + """``frame.versions``: where the target's versions are read.""" + + model_config = ConfigDict(extra="forbid", populate_by_name=True) + + #: A read-only read, the same shape as an ``effects:`` verifier section, + #: that returns one record per version (built by ``verify-run``). + read: dict[str, Any] + #: The version row's field naming the record it is a version of. + record_field: str = Field(min_length=1) + #: The target record's field (in the contract's read set) that equals + #: ``record_field``. Unset: ``frame.identity_field``, else ``id``. + target_field: Optional[str] = None + #: The version row's version id (``meta.versionId``, ``res_ver``, an + #: audit entry's id). + version_field: str = Field(min_length=1) + #: The version row's author, when the app records who saved each + #: version. Unset: versions must be whole numbers that step by one. + author_field: Optional[str] = None + #: The agent's own user ids, as the author field names them. Literals or + #: run parameters. Required with ``author_field``. + self_actors: list[ValueExpr] = Field(default_factory=list, alias="self") + + @model_validator(mode="before") + @classmethod + def _coerce_self(cls, data: Any) -> Any: + if isinstance(data, dict): + key = "self" if "self" in data else "self_actors" + values = data.get(key) + if isinstance(values, list): + data = dict(data) + data[key] = [ + ValueExpr(literal=v) if isinstance(v, str) else v for v in values + ] + return data + + @model_validator(mode="after") + def _shape(self) -> "FrameVersions": + if bool(self.author_field) != bool(self.self_actors): + raise ValueError( + "frame.versions: author_field and self go together (whose " + "versions are the agent's own)" + ) + return self + + +@dataclass +class VersionVerdict: + status: VersionStatus + #: One of :data:`PROBLEMS`, or None when the check held. + problem: Optional[str] = None + targets: int = 0 + #: Versions new in the window, summed over the targets. + new_versions: int = 0 + #: Of those, the agent's own (by author), when authors are read. + own_versions: int = 0 + #: In memory only: the reason line. + reason: str = "" + extra: dict[str, Any] = field(default_factory=dict) + + def evidence(self) -> dict[str, Any]: + """Closed fields only: counts and a problem code, no ids or values.""" + + return { + "status": self.status, + "problem": self.problem, + "targets": self.targets, + "new_versions": self.new_versions, + "own_versions": self.own_versions, + } + + +def _get(row: Mapping[str, Any], path: str) -> Any: + if path in row: + return row[path] + node: Any = row + for part in path.split("."): + if not isinstance(node, Mapping) or part not in node: + return None + node = node[part] + return node + + +def _text(value: Any) -> Optional[str]: + if value is None: + return None + text = str(value) + return text if text != "" else None + + +def _mac(key: bytes, label: str, *parts: str) -> str: + body = "\x1f".join((label, *parts)).encode("utf-8") + return hashlib.blake2b(body, key=key[:64], digest_size=16).hexdigest() + + +def seal_versions( + spec: FrameVersions, rows: Optional[Sequence[Mapping[str, Any]]], key: bytes +) -> dict[str, Any]: + """The baseline's record of versions: per record MAC, the version MACs + and (whole numbers only) the highest version.""" + + if rows is None: + return {"reachable": False} + records: dict[str, dict[str, Any]] = {} + for row in rows: + record = _text(_get(row, spec.record_field)) + version = _text(_get(row, spec.version_field)) + if record is None: + continue # a row that names no record is no record's version + entry = records.setdefault( + _mac(key, "record", record), {"versions": [], "max": None, "whole": True} + ) + if version is None: + entry["whole"] = False + entry["versions"].append(_mac(key, "version", record, "\x00null")) + continue + entry["versions"].append(_mac(key, "version", record, version)) + if _WHOLE.match(version): + entry["max"] = max(int(version), entry["max"] or 0) + else: + entry["whole"] = False + for entry in records.values(): + entry["versions"] = sorted(set(entry["versions"])) + return {"reachable": True, "records": records} + + +def _resolve_self(spec: FrameVersions, params: Mapping[str, object]) -> Optional[set[str]]: + out: set[str] = set() + for expr in spec.self_actors: + value = expr.resolve(params) + if value is None or value == "": + return None + out.add(str(value)) + return out + + +def judge_versions( + spec: FrameVersions, + stored: Mapping[str, Any], + rows: Optional[Sequence[Mapping[str, Any]]], + targets: Sequence[tuple[Optional[str], bool]], + key: bytes, + params: Mapping[str, object] = {}, +) -> VersionVerdict: + """Judge the targets' versions in the window. + + ``stored``: :func:`seal_versions` of the baseline read. ``rows``: the read + after the save (None when it failed). ``targets``: per record a confirmed + write effect matched, its ``target_field`` value (None when the record + has none) and whether it existed before the write. + """ + + if not stored.get("reachable") or rows is None: + return VersionVerdict( + "not_checked", + "unreadable", + reason="the target's versions could not be read, before or after the write", + ) + mine: set[str] = set() + if spec.author_field: + resolved = _resolve_self(spec, params) + if resolved is None: + return VersionVerdict( + "not_checked", + "self_unresolved", + reason="frame.versions.self references a parameter the run did not supply", + ) + mine = resolved + before: Mapping[str, Any] = stored.get("records") or {} + out = VersionVerdict("held") + worst: Optional[VersionVerdict] = None + + def stop(status: VersionStatus, problem: str, reason: str) -> None: + nonlocal worst + if worst is None or (worst.status == "not_checked" and status == "refuted"): + worst = VersionVerdict(status, problem, reason=reason) + + seen: set[Optional[str]] = set() + for target, existed in targets: + if target in seen: + continue + seen.add(target) + if target is None: + stop( + "not_checked", + "no_target_id", + f"a target record has no {spec.target_field or 'identity'} field, " + "so its versions can't be found", + ) + continue + base = before.get(_mac(key, "record", target)) + if base is None: + if existed: + stop( + "not_checked", + "missing", + "the target existed before the write but had no version", + ) + continue # created in the window: no earlier version to overwrite + out.targets += 1 + now = [r for r in rows if _text(_get(r, spec.record_field)) == target] + if not now: + stop("not_checked", "missing", "the target has no version after the write") + continue + old = set(base.get("versions") or []) + fresh = [] + for r in now: + version = _text(_get(r, spec.version_field)) + mac = _mac(key, "version", target, "\x00null" if version is None else version) + if mac not in old: + fresh.append((version, r)) + out.new_versions += len(fresh) + if spec.author_field: + for _version, r in fresh: + author = _text(_get(r, spec.author_field)) + if author is None: + stop( + "not_checked", + "no_author", + "a new version of the target names no author, so it " + "can't be told from another user's save", + ) + elif author in mine: + out.own_versions += 1 + else: + stop( + "refuted", + "another_author", + "lost update: another user saved a version of the target " + "inside the window, and the agent's save came after it " + "or beside it", + ) + continue + # No authors: count whole-number steps. + values = [_text(_get(r, spec.version_field)) for r in now] + if not base.get("whole") or any(v is None or not _WHOLE.match(v) for v in values): + stop( + "not_checked", + "not_a_number", + "the target's versions aren't whole numbers, and no author is " + "read, so the steps in the window can't be counted", + ) + continue + steps = max(int(v) for v in values if v is not None) - int(base["max"] or 0) + if steps < 0 or (steps == 0 and fresh): + stop( + "refuted", + "went_back", + "the target's version went back or was replaced in the window", + ) + elif steps > 1 or len(fresh) > 1: + stop( + "refuted", + "extra_steps", + f"lost update: the target moved {max(steps, len(fresh))} versions " + "in the window; one save is the agent's, and no author is read " + "to say whose the others are", + ) + if worst is not None: + worst.targets, worst.new_versions, worst.own_versions = ( + out.targets, + out.new_versions, + out.own_versions, + ) + return worst + out.reason = f"{out.targets} target(s): every new version is the agent's own" + return out diff --git a/packages/flow/openadapt_flow/verifier.py b/packages/flow/openadapt_flow/verifier.py index 0119f8bc57..9c07831b64 100644 --- a/packages/flow/openadapt_flow/verifier.py +++ b/packages/flow/openadapt_flow/verifier.py @@ -1562,6 +1562,24 @@ def _scope( ) +#: ``frame.versions`` problems (``runtime/effects/versions.py``), as reasons. +_VERSION_REASONS = { + "another_author": "lost update: another user saved a version of the target " + "inside the window, so the agent's save may have overwritten it", + "extra_steps": "lost update: the target moved more than one version in the " + "window, and only one save is the agent's", + "went_back": "the target's version went back or was replaced in the window", + "no_author": "a new version of the target names no author, so it can't be " + "told from another user's save", + "not_a_number": "the target's versions aren't whole numbers and no author " + "is read, so the saves in the window can't be counted", + "missing": "the target existed before the write but has no version to compare", + "no_target_id": "a target record has no field naming it in the versions read", + "unreadable": "the target's versions could not be read, before or after the write", + "self_unresolved": "frame.versions.self names a parameter the run did not supply", +} + + def _reasons(evidence: Mapping[str, Any], receipt_outcome: str) -> list[str]: out: list[str] = [] for row in (evidence.get("readback") or {}).get("effects") or []: @@ -1573,14 +1591,18 @@ def _reasons(evidence: Mapping[str, Any], receipt_outcome: str) -> list[str]: f"{row.get('expected_count')} expected record(s)" ) frame = evidence.get("frame") or {} - if frame.get("status") == "refuted": + versions = frame.get("versions") or {} + problem = versions.get("problem") if isinstance(versions, Mapping) else None + if problem: + out.append(_VERSION_REASONS.get(str(problem), f"the target's versions: {problem}")) + if frame.get("status") == "refuted" and frame.get("collateral_count", 0): kinds = ", ".join(frame.get("collateral_kinds") or []) out.append( f"{frame.get('collateral_count', 0)} record(s) outside the declared " f"write moved ({kinds}); " f"{frame.get('unattributed_count', 0)} of them no actor explains" ) - elif frame.get("status") == "not_checked": + elif frame.get("status") == "not_checked" and not problem: out.append("what else changed could not be read, so the frame is not checked") source = evidence.get("source") or {} if isinstance(source, Mapping) and source.get("status") == "refuted": diff --git a/packages/flow/openadapt_flow/verify_run.py b/packages/flow/openadapt_flow/verify_run.py index bfd4001547..c9cf4ccdb5 100644 --- a/packages/flow/openadapt_flow/verify_run.py +++ b/packages/flow/openadapt_flow/verify_run.py @@ -150,6 +150,9 @@ FRAME_AUDIT_DETAIL = "openadapt:frame_audit" #: Where the baseline keeps the whole-database snapshot (``frame.database``). FRAME_DB_DETAIL = "openadapt:frame_database" +#: Where the baseline keeps the target versions read (``frame.versions``), as +#: keyed MACs. +FRAME_VERSIONS_DETAIL = "openadapt:frame_versions" #: Where the baseline keeps the read set as keyed MACs (``effects/sealed.py``). SEALED_DETAIL = "openadapt:sealed_records" @@ -429,6 +432,83 @@ def _audit_verifier(cfg: VerifyConfig, params: Mapping[str, object]) -> Any: raise VerifyRunError(f"cannot build the audit read: {exc}") from exc +def _read_versions( + cfg: VerifyConfig, params: Mapping[str, object] +) -> Optional[list[dict[str, Any]]]: + """The ``frame.versions`` read: one record per version, or None when it + can't be read.""" + + spec = cfg.frame.versions if cfg.frame is not None else None + if spec is None: + return None + try: + reader = build_effect_verifier(EffectsConfig.model_validate(spec.read), params) + except ValueError as exc: + raise VerifyRunError(f"cannot build the versions read: {exc}") from exc + try: + state = reader.capture_pre_state() if reader is not None else None + except Exception: # noqa: BLE001 - unreadable -> the check stops the run + return None + if not isinstance(state, EffectState) or not state.reachable: + return None + return [dict(r) for r in state.records] + + +def _judge_versions( + cfg: VerifyConfig, + frame: FrameVerdict, + before: EffectState, + effects: Sequence[Effect], + verdicts: Sequence[EffectVerdict], + params: Mapping[str, object], + salt: bytes, +) -> FrameVerdict: + """Add ``frame.versions`` to the frame: a version of a target in the + window that isn't the agent's own refutes it; one that can't be read or + attributed leaves it not checked.""" + + from openadapt_flow.runtime.effects.versions import judge_versions + + spec = cfg.frame.versions if cfg.frame is not None else None + if spec is None: + return frame + field_name = spec.target_field or cfg.frame.identity_field or "id" # type: ignore[union-attr] + old = { + str(r.get(field_name)) + for r in before.records + if r.get(field_name) not in (None, "") + } + targets: list[tuple[Optional[str], bool]] = [] + for effect, verdict in zip(effects, verdicts): + if effect.kind not in (EffectKind.RECORD_WRITTEN, EffectKind.FIELD_EQUALS): + continue + if effect.kind is EffectKind.RECORD_WRITTEN and effect.expected_count == 0: + continue # a guard: "nothing here" + if not verdict.confirmed: + continue + for record in verdict.matched_records: + value = record.get(field_name) + text = None if value in (None, "") else str(value) + targets.append((text, text in old)) + stored = (before.detail or {}).get(FRAME_VERSIONS_DETAIL) + result = judge_versions( + spec, + stored if isinstance(stored, Mapping) else {}, + _read_versions(cfg, params), + targets, + _mac_key(salt), + params, + ) + frame.versions = result + if result.status == "refuted": + frame.status = "refuted" + frame.reason = result.reason + elif result.status == "not_checked" and frame.status == "held": + frame.status = "not_checked" + frame.reason = result.reason + return frame + + def _db_connect(read: Mapping[str, Any]) -> Any: """A connect function for ``frame.database.read`` (read-only).""" @@ -1297,6 +1377,12 @@ def capture_baseline( "reachable": entries is not None, "ids": _audit_ids(entries or [], salt), } + if cfg.frame is not None and cfg.frame.versions is not None: + from openadapt_flow.runtime.effects.versions import seal_versions + + detail[FRAME_VERSIONS_DETAIL] = seal_versions( + cfg.frame.versions, _read_versions(cfg, params), _mac_key(salt) + ) if detail: state = state.model_copy(update={"detail": {**state.detail, **detail}}) state = _seal_state(cfg, state, salt) @@ -1727,6 +1813,9 @@ def verify_run( run_actors=[str(run_record.get("executor_id") or "")], salt=salt, ) + frame = _judge_versions( + cfg, frame, before, bound_effects, verdicts, params, salt + ) else: frame = FrameVerdict( status="not_checked", diff --git a/packages/flow/tests/test_frame_versions.py b/packages/flow/tests/test_frame_versions.py new file mode 100644 index 0000000000..849f1b63bf --- /dev/null +++ b/packages/flow/tests/test_frame_versions.py @@ -0,0 +1,233 @@ +"""``frame.versions``: a lost update stops the run. + +The agent reads a record, another user saves a change, and the agent saves +its stale copy over it. Before and after look like a correct update; only the +record's versions show the second save. Each test runs ``verify_run`` on a +real SQLite database whose app keeps one version row per save (HAPI's +``hfj_res_ver`` shape: record, version number and, in some apps, the author). +Records are made up. +""" + +from __future__ import annotations + +import sqlite3 +from pathlib import Path +from typing import Any, Callable, Optional + +import pytest + +from openadapt_flow.runtime.effects.frame import FrameSpec +from openadapt_flow.verifier import _reasons +from openadapt_flow.verify_run import capture_baseline, parse_verify_config, verify_run + +SCHEMA = """ +CREATE TABLE person (id INTEGER PRIMARY KEY, name TEXT, phone TEXT, addr TEXT, + ver INTEGER); +CREATE TABLE person_version (id INTEGER PRIMARY KEY AUTOINCREMENT, + person_id INTEGER, ver TEXT, author TEXT); +INSERT INTO person VALUES (1, 'Maria Alvarez', '555-0100', '1 Elm St', 1), + (2, 'Yuki Tanaka', '555-0200', '2 Oak St', 1); +INSERT INTO person_version (person_id, ver, author) VALUES (1, '1', 'admin'), + (2, '1', 'admin'); +""" +PARAMS = {"phone": "555-0199", "bot": "bot"} + + +def _save(con: sqlite3.Connection, pid: int, author: Optional[str], **values: str) -> None: + """The app's save: the row, its version number, one version row.""" + + sets = ", ".join(f"{k} = ?" for k in values) + con.execute( + f"UPDATE person SET {sets}, ver = ver + 1 WHERE id = ?", [*values.values(), pid] + ) + (ver,) = con.execute("SELECT ver FROM person WHERE id = ?", [pid]).fetchone() + con.execute( + "INSERT INTO person_version (person_id, ver, author) VALUES (?, ?, ?)", + [pid, str(ver), author], + ) + + +def _doc(db: Path, versions: dict[str, Any]) -> dict[str, Any]: + return { + "schema_version": "openadapt.verify/v1", + "verifier": { + "kind": "sql", + "sqlite_database": str(db), + "sql_query": "SELECT * FROM person", + }, + "effects": [ + { + "kind": "record_written", + "match": {"id": "1", "phone": {"param": "phone"}}, + "expected_count": 1, + } + ], + "frame": { + "identity_field": "id", + "target_fields": ["phone", "ver"], + "versions": { + "read": { + "kind": "sql", + "sqlite_database": str(db), + "sql_query": "SELECT person_id, ver, author FROM person_version", + }, + "record_field": "person_id", + "version_field": "ver", + **versions, + }, + }, + } + + +AUTHORS = {"author_field": "author", "self": [{"param": "bot"}]} + + +def _run( + tmp_path: Path, + window: Callable[[sqlite3.Connection], None], + versions: Optional[dict[str, Any]] = None, + setup: Callable[[sqlite3.Connection], None] = lambda con: None, +) -> dict[str, Any]: + tmp_path.mkdir(parents=True, exist_ok=True) + db = tmp_path / "app.sqlite" + con = sqlite3.connect(db) + con.executescript(SCHEMA) + setup(con) + con.commit() + cfg = parse_verify_config(_doc(db, {} if versions is None else versions)) + capture_baseline(cfg, PARAMS, tmp_path / "baseline.json") + window(con) + con.commit() + con.close() + return verify_run( + cfg, + params=PARAMS, + run_record={"executor_kind": "agent", "executor_id": "bot"}, + authorization_ref="test", + baseline_path=tmp_path / "baseline.json", + ) + + +def _versions(result: dict[str, Any]) -> dict[str, Any]: + return result["evidence"]["frame"]["versions"] + + +def _correct(con: sqlite3.Connection) -> None: + _save(con, 1, "bot", phone="555-0199") + + +def _lost_update(con: sqlite3.Connection) -> None: + # The front desk saves a new address; the bot saves its stale copy + # (the old address) with the new phone, reverting the address. + _save(con, 1, "frontdesk", addr="9 New Rd") + _save(con, 1, "bot", phone="555-0199", addr="1 Elm St") + + +@pytest.mark.parametrize("versions", [None, AUTHORS], ids=["steps", "authors"]) +def test_a_correct_update_is_verified(tmp_path: Path, versions: Any) -> None: + result = _run(tmp_path, _correct, versions) + assert result["receipt"]["outcome"] == "verified" + assert _versions(result)["status"] == "held" + assert _versions(result)["new_versions"] == 1 + + +def test_a_lost_update_without_authors_is_refuted_by_its_steps(tmp_path: Path) -> None: + result = _run(tmp_path, _lost_update) + assert result["receipt"]["checked"]["frame"] == "refuted" + assert result["receipt"]["outcome"] == "reconciliation_required" + assert _versions(result)["problem"] == "extra_steps" + assert any("lost update" in r for r in _reasons(result["evidence"], "x")) + + +def test_a_lost_update_with_authors_is_refuted_by_the_author(tmp_path: Path) -> None: + result = _run(tmp_path, _lost_update, AUTHORS) + assert result["receipt"]["checked"]["frame"] == "refuted" + assert _versions(result)["problem"] == "another_author" + assert _versions(result)["own_versions"] == 1 + + +def test_a_current_version_only_read_still_counts_the_steps(tmp_path: Path) -> None: + # The read shows only the record's current version (a version column). + result = _run( + tmp_path, + _lost_update, + {"read": {"kind": "sql", "sqlite_database": str(tmp_path / "app.sqlite"), + "sql_query": "SELECT id AS person_id, ver FROM person"}}, + ) # fmt: skip + assert _versions(result)["problem"] == "extra_steps" + + +def test_another_users_edit_of_an_unrelated_record_is_not_a_lost_update( + tmp_path: Path, +) -> None: + def window(con: sqlite3.Connection) -> None: + _save(con, 2, "frontdesk", phone="555-0222") + _correct(con) + + for versions in (None, AUTHORS): + result = _run(tmp_path / str(bool(versions)), window, versions) + # Only the target's versions are judged. (The frame has no + # attribution here, so record 2's change refutes it as collateral, + # as before: the version check adds nothing to that.) + assert _versions(result)["status"] == "held" + assert _versions(result)["problem"] is None + + +def test_a_version_with_no_author_is_not_checked(tmp_path: Path) -> None: + result = _run(tmp_path, lambda con: _save(con, 1, None, phone="555-0199"), AUTHORS) + assert result["receipt"]["checked"]["frame"] == "not_checked" + assert result["receipt"]["outcome"] != "verified" + assert _versions(result)["problem"] == "no_author" + + +def test_a_version_that_isnt_a_number_is_not_checked(tmp_path: Path) -> None: + def window(con: sqlite3.Connection) -> None: + _correct(con) + con.execute("UPDATE person_version SET ver = 'W/\"2\"' WHERE ver = '2'") + + result = _run(tmp_path, window) + assert _versions(result)["problem"] == "not_a_number" + assert result["receipt"]["outcome"] != "verified" + + +def test_an_unreadable_versions_read_is_not_checked(tmp_path: Path) -> None: + def window(con: sqlite3.Connection) -> None: + _correct(con) + con.execute("ALTER TABLE person_version RENAME TO gone") + + result = _run(tmp_path, window) + assert _versions(result)["problem"] == "unreadable" + assert result["receipt"]["outcome"] != "verified" + + +def test_a_target_with_no_version_is_not_checked(tmp_path: Path) -> None: + result = _run( + tmp_path, + _correct, + setup=lambda con: con.execute("DELETE FROM person_version WHERE person_id = 1"), + ) + # The bot's save writes version 2, but the baseline had none for it. + assert _versions(result)["problem"] == "missing" + assert result["receipt"]["outcome"] != "verified" + + +def test_versions_that_go_back_are_refuted(tmp_path: Path) -> None: + def window(con: sqlite3.Connection) -> None: + con.execute("UPDATE person SET phone = '555-0199' WHERE id = 1") + con.execute("UPDATE person_version SET ver = '0' WHERE person_id = 1") + + result = _run(tmp_path, window) + assert _versions(result)["problem"] == "went_back" + + +def test_author_and_self_go_together() -> None: + with pytest.raises(ValueError, match="author_field and self go together"): + FrameSpec.model_validate( + {"versions": {"read": {}, "record_field": "a", "version_field": "b", + "author_field": "c"}} + ) # fmt: skip + + +def test_a_frame_without_versions_keeps_its_digest() -> None: + spec = FrameSpec.model_validate({"identity_field": "id"}) + assert "versions" not in spec.model_dump(mode="json", by_alias=True) From 18ea9cf310f8a75ef6b06fdfc960a5a0dce11c4a Mon Sep 17 00:00:00 2001 From: abrichr Date: Fri, 30 Oct 2026 12:18:55 -0400 Subject: [PATCH 2/7] WIP lost-update docs --- packages/flow/docs/VERIFY_RUN.md | 61 ++++++++++++++++++++++++++++++++ 1 file changed, 61 insertions(+) diff --git a/packages/flow/docs/VERIFY_RUN.md b/packages/flow/docs/VERIFY_RUN.md index 7b7087806c..9c63c83227 100644 --- a/packages/flow/docs/VERIFY_RUN.md +++ b/packages/flow/docs/VERIFY_RUN.md @@ -244,6 +244,67 @@ file sets them explicitly; the frame covers the rest. Without a baseline, `forbid_collateral_change` is turned on as before, though it can't refute without a baseline either. +## Lost updates + +Two snapshots can't see a lost update. The agent reads a record, another +user saves a change to it, and the agent then saves its stale copy over +theirs. After the save the record looks like "the agent changed the phone, +the address is unchanged", which is also what a correct update looks like. +Only the record's versions show the second save. + +`frame.versions` names a read that returns one row per version of a record, +and the fields of a row that name the record, the version and, where the app +records it, the author: + +```yaml +frame: + identity_field: id + versions: + read: # read-only, the same shape as `verifier:` + kind: sql + sql_driver: psycopg2 + sql_connect_args: {host: 127.0.0.1, port: 5432, user: reader, dbname: hapi} + sql_password_env: HAPI_DB_PASSWORD + sql_query: >- + SELECT r.res_type || '/' || r.fhir_id AS id, v.res_ver::text AS ver + FROM hfj_res_ver v JOIN hfj_resource r ON r.res_id = v.res_id + record_field: id # the version row's record + target_field: id # the target's field in the read set (default: identity_field) + version_field: ver + # author_field: user_id # when the app records who saved each version + # self: [{param: bot_user_id}] +``` + +The versions are read before the write and after it. Every record that a +confirmed write effect names and that existed before the write is a target. +The versions of a target that are new in the window must be the agent's: + +- With `author_field`, every new version's author must be one of `self`. A + version by anyone else is refuted as a lost update. A version with no + author is not checked. +- Without it, versions must be whole numbers that step by one per save + (HAPI's `res_ver`, a `version_number` + column). At most one step may happen. Two or more steps are refuted: the + agent saved once, so someone else saved too. A version that went back is + refuted. A version that isn't a whole number (an opaque ETag) is not + checked: without authors the saves can't be counted. A read of only the + current version works too, since the step count is the difference. + +Anything else stops the run, never a pass: a versions read that fails before +or after the write, a target that existed but has no version, or a target +record without the field that names it. A record created in the window has +no earlier version to overwrite and isn't judged. Only targets are judged: +another user's save to an unrelated record leaves this check as it was (the +frame and its attribution judge that record, as before). + +The baseline keeps versions as keyed MACs of the record and the version; +only a whole-number version's value is kept, to count steps. The receipt's +`evidence.frame.versions` holds counts and a problem code (`another_author`, +`extra_steps`, `went_back`, `no_author`, `not_a_number`, `missing`, +`no_target_id`, `unreadable`, `self_unresolved`), never ids or values. A +refuted check refutes the frame (`checked.frame: refuted`); one that can't be +judged leaves the frame `not_checked`. + ## What the receipt says was checked The receipt's `checked` field names each part of the claim behind From 2807fa1af6b3abb7a5cbeb068639d677d014d9b8 Mon Sep 17 00:00:00 2001 From: abrichr Date: Fri, 30 Oct 2026 12:20:41 -0400 Subject: [PATCH 3/7] format --- .../flow/openadapt_flow/runtime/effects/versions.py | 12 +++++++++--- packages/flow/openadapt_flow/verifier.py | 4 +++- packages/flow/tests/test_frame_versions.py | 4 +++- 3 files changed, 15 insertions(+), 5 deletions(-) diff --git a/packages/flow/openadapt_flow/runtime/effects/versions.py b/packages/flow/openadapt_flow/runtime/effects/versions.py index 006850114e..c79f0e11d0 100644 --- a/packages/flow/openadapt_flow/runtime/effects/versions.py +++ b/packages/flow/openadapt_flow/runtime/effects/versions.py @@ -185,7 +185,9 @@ def seal_versions( return {"reachable": True, "records": records} -def _resolve_self(spec: FrameVersions, params: Mapping[str, object]) -> Optional[set[str]]: +def _resolve_self( + spec: FrameVersions, params: Mapping[str, object] +) -> Optional[set[str]]: out: set[str] = set() for expr in spec.self_actors: value = expr.resolve(params) @@ -267,7 +269,9 @@ def stop(status: VersionStatus, problem: str, reason: str) -> None: fresh = [] for r in now: version = _text(_get(r, spec.version_field)) - mac = _mac(key, "version", target, "\x00null" if version is None else version) + mac = _mac( + key, "version", target, "\x00null" if version is None else version + ) if mac not in old: fresh.append((version, r)) out.new_versions += len(fresh) @@ -294,7 +298,9 @@ def stop(status: VersionStatus, problem: str, reason: str) -> None: continue # No authors: count whole-number steps. values = [_text(_get(r, spec.version_field)) for r in now] - if not base.get("whole") or any(v is None or not _WHOLE.match(v) for v in values): + if not base.get("whole") or any( + v is None or not _WHOLE.match(v) for v in values + ): stop( "not_checked", "not_a_number", diff --git a/packages/flow/openadapt_flow/verifier.py b/packages/flow/openadapt_flow/verifier.py index 9c07831b64..15cde7b240 100644 --- a/packages/flow/openadapt_flow/verifier.py +++ b/packages/flow/openadapt_flow/verifier.py @@ -1594,7 +1594,9 @@ def _reasons(evidence: Mapping[str, Any], receipt_outcome: str) -> list[str]: versions = frame.get("versions") or {} problem = versions.get("problem") if isinstance(versions, Mapping) else None if problem: - out.append(_VERSION_REASONS.get(str(problem), f"the target's versions: {problem}")) + out.append( + _VERSION_REASONS.get(str(problem), f"the target's versions: {problem}") + ) if frame.get("status") == "refuted" and frame.get("collateral_count", 0): kinds = ", ".join(frame.get("collateral_kinds") or []) out.append( diff --git a/packages/flow/tests/test_frame_versions.py b/packages/flow/tests/test_frame_versions.py index 849f1b63bf..7a27d6890f 100644 --- a/packages/flow/tests/test_frame_versions.py +++ b/packages/flow/tests/test_frame_versions.py @@ -33,7 +33,9 @@ PARAMS = {"phone": "555-0199", "bot": "bot"} -def _save(con: sqlite3.Connection, pid: int, author: Optional[str], **values: str) -> None: +def _save( + con: sqlite3.Connection, pid: int, author: Optional[str], **values: str +) -> None: """The app's save: the row, its version number, one version row.""" sets = ", ".join(f"{k} = ?" for k in values) From 4506e3ac498940e02e50be9d056142f1e65c5d74 Mon Sep 17 00:00:00 2001 From: abrichr Date: Fri, 30 Oct 2026 13:07:03 -0400 Subject: [PATCH 4/7] Lost-update check: EspoCRM example, CAPABILITIES and changelog --- CAPABILITIES.md | 1 + llms-full.txt | 1 + packages/flow/CHANGELOG.md | 2 ++ packages/flow/docs/VERIFY_RUN.md | 24 ++++++++++++++++++++++-- 4 files changed, 26 insertions(+), 2 deletions(-) diff --git a/CAPABILITIES.md b/CAPABILITIES.md index cc01c031bd..481f8cbaec 100644 --- a/CAPABILITIES.md +++ b/CAPABILITIES.md @@ -156,6 +156,7 @@ improvising or retrying when something is off. | Check one write from Python, without the CLI: one API (`openadapt_flow.verifier`: `begin_write`, `verify_write`; 2.4.1's `capture` form still works, deprecated) with one outcome (`CONFIRMED`, `REFUTED`, `UNCERTAIN`), the receipt, which parts were checked, and the result's scope (read path, stores read, time window, what it doesn't cover) | On main | Synthetic (SQLite; MockMed's fault server over real HTTP) | `openadapt_flow/verifier.py`, [VERIFIER_API.md](packages/flow/docs/VERIFIER_API.md) | | Never send a write twice while its outcome is unknown: an append-only ledger on local disk (pluggable backend), keyed by the write's identity, that every adapter, the MCP tools and the CLI consult before sending; a read-back against the attempt's own baseline resolves it, and a refuted write waits for a person (`openadapt verify ledger list`, `reconcile`, `release`) | On main | Synthetic (MockMed's fault server: a worker killed between the send and the read-back, killed during a timed-out send, killed before the send, a late write, and two worker processes started together on one write) | `openadapt_flow/write_ledger.py`, [VERIFIER_API.md](packages/flow/docs/VERIFIER_API.md#the-uncertain-write-ledger) | | Catch a write that lands after the verdict (a background job, a trigger, a delayed side record): the receipt says when a recheck is due, and the recheck reads the whole database again and refutes the earlier result when a change since the post-save read has no explanation (`frame.database.recheck_after_s`, `verifier.recheck`) | On main ([#1639](https://github.com/OpenAdaptAI/OpenAdapt/pull/1639)) | Synthetic (SQLite); opt-in live OpenEMR test | `openadapt_flow/verifier.py`, `runtime/effects/late_write.py`, [VERIFY_RUN.md](packages/flow/docs/VERIFY_RUN.md#writes-that-land-later) | +| Catch a lost update: another user saves a change to the record while the agent works, and the agent saves its stale copy over it. Before and after look like a correct update, so `frame.versions` reads the target's versions (FHIR or SQL version history, a version column, or the app's audit or action-history table) before and after the write. A version by another author, or more than one version step when no author is read, refutes the write; a version that can't be read or attributed stops it | On main ([#PR2]) | Real app (HAPI FHIR and EspoCRM, first-contact study fault scripts: both lost updates refuted, correct updates verified, every other case's verdict unchanged) | [VERIFY_RUN.md](packages/flow/docs/VERIFY_RUN.md#lost-updates), `runtime/effects/versions.py` | | Guarded save: a safety layer any agent wraps around one write (check the record, hold for approval, read back) | On main ([#1258](https://github.com/OpenAdaptAI/OpenAdapt/pull/1258)) | Synthetic | `GUARDED_SAVE.md` | | A person's approval in Desktop is signed with a key kept behind Touch ID on a Mac: one-prompt setup in the app, a prompt for every answer, and Flow never sees the key | On main ([#1500](https://github.com/OpenAdaptAI/OpenAdapt/pull/1500)) | Unit tests; opt-in test on a real Secure Enclave | `packages/desktop/openadapt_desktop/approver_keychain.py`, [APPROVAL_MODES.md](packages/flow/docs/APPROVAL_MODES.md#the-approver-key) | | The approver root Desktop trusts can be pinned by an administrator, in a file this user can't write (macOS, Linux, Windows); Desktop checks the owner, mode or ACL and refuses links. A Mac without one uses the root it set up behind Touch ID, read with Keychain prompts off; the two disagreeing is refused | On main | Unit tests per attack on real files; Windows ACL rules unit-tested, real-ACL test on Windows CI; opt-in test against the real login Keychain | `openadapt-desktop approver-pin`, `packages/desktop/openadapt_desktop/approver_anchor.py`, [APPROVAL_MODES.md](packages/flow/docs/APPROVAL_MODES.md#the-approver-key) | diff --git a/llms-full.txt b/llms-full.txt index 07dde670d5..9ae0ddf89c 100644 --- a/llms-full.txt +++ b/llms-full.txt @@ -553,6 +553,7 @@ improvising or retrying when something is off. | Check one write from Python, without the CLI: one API (`openadapt_flow.verifier`: `begin_write`, `verify_write`; 2.4.1's `capture` form still works, deprecated) with one outcome (`CONFIRMED`, `REFUTED`, `UNCERTAIN`), the receipt, which parts were checked, and the result's scope (read path, stores read, time window, what it doesn't cover) | On main | Synthetic (SQLite; MockMed's fault server over real HTTP) | `openadapt_flow/verifier.py`, [VERIFIER_API.md](https://github.com/OpenAdaptAI/OpenAdapt/blob/main/packages/flow/docs/VERIFIER_API.md) | | Never send a write twice while its outcome is unknown: an append-only ledger on local disk (pluggable backend), keyed by the write's identity, that every adapter, the MCP tools and the CLI consult before sending; a read-back against the attempt's own baseline resolves it, and a refuted write waits for a person (`openadapt verify ledger list`, `reconcile`, `release`) | On main | Synthetic (MockMed's fault server: a worker killed between the send and the read-back, killed during a timed-out send, killed before the send, a late write, and two worker processes started together on one write) | `openadapt_flow/write_ledger.py`, [VERIFIER_API.md](https://github.com/OpenAdaptAI/OpenAdapt/blob/main/packages/flow/docs/VERIFIER_API.md#the-uncertain-write-ledger) | | Catch a write that lands after the verdict (a background job, a trigger, a delayed side record): the receipt says when a recheck is due, and the recheck reads the whole database again and refutes the earlier result when a change since the post-save read has no explanation (`frame.database.recheck_after_s`, `verifier.recheck`) | On main ([#1639](https://github.com/OpenAdaptAI/OpenAdapt/pull/1639)) | Synthetic (SQLite); opt-in live OpenEMR test | `openadapt_flow/verifier.py`, `runtime/effects/late_write.py`, [VERIFY_RUN.md](https://github.com/OpenAdaptAI/OpenAdapt/blob/main/packages/flow/docs/VERIFY_RUN.md#writes-that-land-later) | +| Catch a lost update: another user saves a change to the record while the agent works, and the agent saves its stale copy over it. Before and after look like a correct update, so `frame.versions` reads the target's versions (FHIR or SQL version history, a version column, or the app's audit or action-history table) before and after the write. A version by another author, or more than one version step when no author is read, refutes the write; a version that can't be read or attributed stops it | On main ([#PR2]) | Real app (HAPI FHIR and EspoCRM, first-contact study fault scripts: both lost updates refuted, correct updates verified, every other case's verdict unchanged) | [VERIFY_RUN.md](https://github.com/OpenAdaptAI/OpenAdapt/blob/main/packages/flow/docs/VERIFY_RUN.md#lost-updates), `runtime/effects/versions.py` | | Guarded save: a safety layer any agent wraps around one write (check the record, hold for approval, read back) | On main ([#1258](https://github.com/OpenAdaptAI/OpenAdapt/pull/1258)) | Synthetic | `GUARDED_SAVE.md` | | A person's approval in Desktop is signed with a key kept behind Touch ID on a Mac: one-prompt setup in the app, a prompt for every answer, and Flow never sees the key | On main ([#1500](https://github.com/OpenAdaptAI/OpenAdapt/pull/1500)) | Unit tests; opt-in test on a real Secure Enclave | `packages/desktop/openadapt_desktop/approver_keychain.py`, [APPROVAL_MODES.md](https://github.com/OpenAdaptAI/OpenAdapt/blob/main/packages/flow/docs/APPROVAL_MODES.md#the-approver-key) | | The approver root Desktop trusts can be pinned by an administrator, in a file this user can't write (macOS, Linux, Windows); Desktop checks the owner, mode or ACL and refuses links. A Mac without one uses the root it set up behind Touch ID, read with Keychain prompts off; the two disagreeing is refused | On main | Unit tests per attack on real files; Windows ACL rules unit-tested, real-ACL test on Windows CI; opt-in test against the real login Keychain | `openadapt-desktop approver-pin`, `packages/desktop/openadapt_desktop/approver_anchor.py`, [APPROVAL_MODES.md](https://github.com/OpenAdaptAI/OpenAdapt/blob/main/packages/flow/docs/APPROVAL_MODES.md#the-approver-key) | diff --git a/packages/flow/CHANGELOG.md b/packages/flow/CHANGELOG.md index c0c7a8b828..34995dae3e 100644 --- a/packages/flow/CHANGELOG.md +++ b/packages/flow/CHANGELOG.md @@ -19,6 +19,8 @@ ### Features +- A lost update now stops the run. `frame.versions` names a read of the target's versions (HAPI's `hfj_res_ver`, a version column, or an app's action-history table) and, where the app records one, the author of each version. The verifier reads them before and after the write; a version of a pre-existing target by anyone other than the agent, or more than one version step when no author is read, refutes the frame. A versions read that fails, a target with no version, or a version with no author leaves the frame not checked. The receipt holds counts and a problem code (`evidence.frame.versions`). Contracts without `frame.versions` keep their digests. + - A write that lands after the verdict can now be caught. With `frame.database.recheck_after_s`, a verified write's receipt says when a recheck is due (`recheck_due_at`), and `openadapt_flow.verifier.recheck` reads the whole database again at or after that time. It judges every change since the post-save read with the frame's rules: the app profile's bookkeeping, other users' writes the audit log attributes, and changes a later write by the same actor under the same contract saw in its own window (`excused_by`, any outcome). Anything else turns the earlier result into `REFUTED` (late). A change that may be what refuted a later write, a database that can't be read, or a saved state that wasn't issued here or doesn't match its receipt gives `UNCERTAIN`. The saved state holds keyed hashes, not values. `frame.database.settle_attempts` lets the reads after a save, and at a recheck, retry within a stated budget until two in a row match; rows added to append-only tables between them don't count as movement. Without these settings, nothing changes ([#1639](https://github.com/OpenAdaptAI/OpenAdapt/pull/1639)). ## v2.4.1 (2026-10-28) diff --git a/packages/flow/docs/VERIFY_RUN.md b/packages/flow/docs/VERIFY_RUN.md index 9c63c83227..b800a9e274 100644 --- a/packages/flow/docs/VERIFY_RUN.md +++ b/packages/flow/docs/VERIFY_RUN.md @@ -275,6 +275,26 @@ frame: # self: [{param: bot_user_id}] ``` +An app that logs each save with its user can name the author instead. EspoCRM's +`action_history_record` logs every API save; a lead has no version column: + +```yaml + versions: + read: + kind: sql + sql_driver: pymysql + sql_connect_args: {host: 127.0.0.1, port: 3306, user: reader, database: espocrm} + sql_password_env: ESPOCRM_DB_RO_PASSWORD + sql_query: >- + SELECT CAST(CONCAT(LOWER(target_type), ':', target_id) AS CHAR) AS id, + CAST(number AS CHAR) AS ver, CAST(user_id AS CHAR) AS author + FROM action_history_record WHERE action <> 'read' AND deleted = 0 + record_field: id # equals the read set's id (lead:) + version_field: ver + author_field: author + self: [{param: bot_user_id}] +``` + The versions are read before the write and after it. Every record that a confirmed write effect names and that existed before the write is a target. The versions of a target that are new in the window must be the agent's: @@ -283,8 +303,8 @@ The versions of a target that are new in the window must be the agent's: version by anyone else is refuted as a lost update. A version with no author is not checked. - Without it, versions must be whole numbers that step by one per save - (HAPI's `res_ver`, a `version_number` - column). At most one step may happen. Two or more steps are refuted: the + (HAPI's `res_ver`, a `version_number` column). At most one step may + happen. Two or more steps are refuted: the agent saved once, so someone else saved too. A version that went back is refuted. A version that isn't a whole number (an opaque ETag) is not checked: without authors the saves can't be counted. A read of only the From 6df1a400abcecd65256f40eabc3080da23244d5a Mon Sep 17 00:00:00 2001 From: abrichr Date: Fri, 30 Oct 2026 13:07:17 -0400 Subject: [PATCH 5/7] CAPABILITIES: link #1662 --- CAPABILITIES.md | 2 +- llms-full.txt | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/CAPABILITIES.md b/CAPABILITIES.md index 481f8cbaec..3ca616216f 100644 --- a/CAPABILITIES.md +++ b/CAPABILITIES.md @@ -156,7 +156,7 @@ improvising or retrying when something is off. | Check one write from Python, without the CLI: one API (`openadapt_flow.verifier`: `begin_write`, `verify_write`; 2.4.1's `capture` form still works, deprecated) with one outcome (`CONFIRMED`, `REFUTED`, `UNCERTAIN`), the receipt, which parts were checked, and the result's scope (read path, stores read, time window, what it doesn't cover) | On main | Synthetic (SQLite; MockMed's fault server over real HTTP) | `openadapt_flow/verifier.py`, [VERIFIER_API.md](packages/flow/docs/VERIFIER_API.md) | | Never send a write twice while its outcome is unknown: an append-only ledger on local disk (pluggable backend), keyed by the write's identity, that every adapter, the MCP tools and the CLI consult before sending; a read-back against the attempt's own baseline resolves it, and a refuted write waits for a person (`openadapt verify ledger list`, `reconcile`, `release`) | On main | Synthetic (MockMed's fault server: a worker killed between the send and the read-back, killed during a timed-out send, killed before the send, a late write, and two worker processes started together on one write) | `openadapt_flow/write_ledger.py`, [VERIFIER_API.md](packages/flow/docs/VERIFIER_API.md#the-uncertain-write-ledger) | | Catch a write that lands after the verdict (a background job, a trigger, a delayed side record): the receipt says when a recheck is due, and the recheck reads the whole database again and refutes the earlier result when a change since the post-save read has no explanation (`frame.database.recheck_after_s`, `verifier.recheck`) | On main ([#1639](https://github.com/OpenAdaptAI/OpenAdapt/pull/1639)) | Synthetic (SQLite); opt-in live OpenEMR test | `openadapt_flow/verifier.py`, `runtime/effects/late_write.py`, [VERIFY_RUN.md](packages/flow/docs/VERIFY_RUN.md#writes-that-land-later) | -| Catch a lost update: another user saves a change to the record while the agent works, and the agent saves its stale copy over it. Before and after look like a correct update, so `frame.versions` reads the target's versions (FHIR or SQL version history, a version column, or the app's audit or action-history table) before and after the write. A version by another author, or more than one version step when no author is read, refutes the write; a version that can't be read or attributed stops it | On main ([#PR2]) | Real app (HAPI FHIR and EspoCRM, first-contact study fault scripts: both lost updates refuted, correct updates verified, every other case's verdict unchanged) | [VERIFY_RUN.md](packages/flow/docs/VERIFY_RUN.md#lost-updates), `runtime/effects/versions.py` | +| Catch a lost update: another user saves a change to the record while the agent works, and the agent saves its stale copy over it. Before and after look like a correct update, so `frame.versions` reads the target's versions (FHIR or SQL version history, a version column, or the app's audit or action-history table) before and after the write. A version by another author, or more than one version step when no author is read, refutes the write; a version that can't be read or attributed stops it | On main ([#1662](https://github.com/OpenAdaptAI/OpenAdapt/pull/1662)) | Real app (HAPI FHIR and EspoCRM, first-contact study fault scripts: both lost updates refuted, correct updates verified, every other case's verdict unchanged) | [VERIFY_RUN.md](packages/flow/docs/VERIFY_RUN.md#lost-updates), `runtime/effects/versions.py` | | Guarded save: a safety layer any agent wraps around one write (check the record, hold for approval, read back) | On main ([#1258](https://github.com/OpenAdaptAI/OpenAdapt/pull/1258)) | Synthetic | `GUARDED_SAVE.md` | | A person's approval in Desktop is signed with a key kept behind Touch ID on a Mac: one-prompt setup in the app, a prompt for every answer, and Flow never sees the key | On main ([#1500](https://github.com/OpenAdaptAI/OpenAdapt/pull/1500)) | Unit tests; opt-in test on a real Secure Enclave | `packages/desktop/openadapt_desktop/approver_keychain.py`, [APPROVAL_MODES.md](packages/flow/docs/APPROVAL_MODES.md#the-approver-key) | | The approver root Desktop trusts can be pinned by an administrator, in a file this user can't write (macOS, Linux, Windows); Desktop checks the owner, mode or ACL and refuses links. A Mac without one uses the root it set up behind Touch ID, read with Keychain prompts off; the two disagreeing is refused | On main | Unit tests per attack on real files; Windows ACL rules unit-tested, real-ACL test on Windows CI; opt-in test against the real login Keychain | `openadapt-desktop approver-pin`, `packages/desktop/openadapt_desktop/approver_anchor.py`, [APPROVAL_MODES.md](packages/flow/docs/APPROVAL_MODES.md#the-approver-key) | diff --git a/llms-full.txt b/llms-full.txt index 9ae0ddf89c..e00141fae7 100644 --- a/llms-full.txt +++ b/llms-full.txt @@ -553,7 +553,7 @@ improvising or retrying when something is off. | Check one write from Python, without the CLI: one API (`openadapt_flow.verifier`: `begin_write`, `verify_write`; 2.4.1's `capture` form still works, deprecated) with one outcome (`CONFIRMED`, `REFUTED`, `UNCERTAIN`), the receipt, which parts were checked, and the result's scope (read path, stores read, time window, what it doesn't cover) | On main | Synthetic (SQLite; MockMed's fault server over real HTTP) | `openadapt_flow/verifier.py`, [VERIFIER_API.md](https://github.com/OpenAdaptAI/OpenAdapt/blob/main/packages/flow/docs/VERIFIER_API.md) | | Never send a write twice while its outcome is unknown: an append-only ledger on local disk (pluggable backend), keyed by the write's identity, that every adapter, the MCP tools and the CLI consult before sending; a read-back against the attempt's own baseline resolves it, and a refuted write waits for a person (`openadapt verify ledger list`, `reconcile`, `release`) | On main | Synthetic (MockMed's fault server: a worker killed between the send and the read-back, killed during a timed-out send, killed before the send, a late write, and two worker processes started together on one write) | `openadapt_flow/write_ledger.py`, [VERIFIER_API.md](https://github.com/OpenAdaptAI/OpenAdapt/blob/main/packages/flow/docs/VERIFIER_API.md#the-uncertain-write-ledger) | | Catch a write that lands after the verdict (a background job, a trigger, a delayed side record): the receipt says when a recheck is due, and the recheck reads the whole database again and refutes the earlier result when a change since the post-save read has no explanation (`frame.database.recheck_after_s`, `verifier.recheck`) | On main ([#1639](https://github.com/OpenAdaptAI/OpenAdapt/pull/1639)) | Synthetic (SQLite); opt-in live OpenEMR test | `openadapt_flow/verifier.py`, `runtime/effects/late_write.py`, [VERIFY_RUN.md](https://github.com/OpenAdaptAI/OpenAdapt/blob/main/packages/flow/docs/VERIFY_RUN.md#writes-that-land-later) | -| Catch a lost update: another user saves a change to the record while the agent works, and the agent saves its stale copy over it. Before and after look like a correct update, so `frame.versions` reads the target's versions (FHIR or SQL version history, a version column, or the app's audit or action-history table) before and after the write. A version by another author, or more than one version step when no author is read, refutes the write; a version that can't be read or attributed stops it | On main ([#PR2]) | Real app (HAPI FHIR and EspoCRM, first-contact study fault scripts: both lost updates refuted, correct updates verified, every other case's verdict unchanged) | [VERIFY_RUN.md](https://github.com/OpenAdaptAI/OpenAdapt/blob/main/packages/flow/docs/VERIFY_RUN.md#lost-updates), `runtime/effects/versions.py` | +| Catch a lost update: another user saves a change to the record while the agent works, and the agent saves its stale copy over it. Before and after look like a correct update, so `frame.versions` reads the target's versions (FHIR or SQL version history, a version column, or the app's audit or action-history table) before and after the write. A version by another author, or more than one version step when no author is read, refutes the write; a version that can't be read or attributed stops it | On main ([#1662](https://github.com/OpenAdaptAI/OpenAdapt/pull/1662)) | Real app (HAPI FHIR and EspoCRM, first-contact study fault scripts: both lost updates refuted, correct updates verified, every other case's verdict unchanged) | [VERIFY_RUN.md](https://github.com/OpenAdaptAI/OpenAdapt/blob/main/packages/flow/docs/VERIFY_RUN.md#lost-updates), `runtime/effects/versions.py` | | Guarded save: a safety layer any agent wraps around one write (check the record, hold for approval, read back) | On main ([#1258](https://github.com/OpenAdaptAI/OpenAdapt/pull/1258)) | Synthetic | `GUARDED_SAVE.md` | | A person's approval in Desktop is signed with a key kept behind Touch ID on a Mac: one-prompt setup in the app, a prompt for every answer, and Flow never sees the key | On main ([#1500](https://github.com/OpenAdaptAI/OpenAdapt/pull/1500)) | Unit tests; opt-in test on a real Secure Enclave | `packages/desktop/openadapt_desktop/approver_keychain.py`, [APPROVAL_MODES.md](https://github.com/OpenAdaptAI/OpenAdapt/blob/main/packages/flow/docs/APPROVAL_MODES.md#the-approver-key) | | The approver root Desktop trusts can be pinned by an administrator, in a file this user can't write (macOS, Linux, Windows); Desktop checks the owner, mode or ACL and refuses links. A Mac without one uses the root it set up behind Touch ID, read with Keychain prompts off; the two disagreeing is refused | On main | Unit tests per attack on real files; Windows ACL rules unit-tested, real-ACL test on Windows CI; opt-in test against the real login Keychain | `openadapt-desktop approver-pin`, `packages/desktop/openadapt_desktop/approver_anchor.py`, [APPROVAL_MODES.md](https://github.com/OpenAdaptAI/OpenAdapt/blob/main/packages/flow/docs/APPROVAL_MODES.md#the-approver-key) | From 4182a0650bab3b18d4433bf201eb3ed304a42efe Mon Sep 17 00:00:00 2001 From: abrichr Date: Fri, 30 Oct 2026 13:19:57 -0400 Subject: [PATCH 6/7] frame.versions: stop on vanished, null or duplicate versions and on a changed target with no new version; read versions first --- packages/flow/docs/VERIFY_RUN.md | 53 +++++++---- .../runtime/effects/versions.py | 63 +++++++++++--- packages/flow/openadapt_flow/verify_run.py | 29 +++++-- packages/flow/tests/test_frame_versions.py | 87 +++++++++++++++++++ 4 files changed, 195 insertions(+), 37 deletions(-) diff --git a/packages/flow/docs/VERIFY_RUN.md b/packages/flow/docs/VERIFY_RUN.md index b800a9e274..790fb2064f 100644 --- a/packages/flow/docs/VERIFY_RUN.md +++ b/packages/flow/docs/VERIFY_RUN.md @@ -301,26 +301,47 @@ The versions of a target that are new in the window must be the agent's: - With `author_field`, every new version's author must be one of `self`. A version by anyone else is refuted as a lost update. A version with no - author is not checked. -- Without it, versions must be whole numbers that step by one per save - (HAPI's `res_ver`, a `version_number` column). At most one step may - happen. Two or more steps are refuted: the - agent saved once, so someone else saved too. A version that went back is - refuted. A version that isn't a whole number (an opaque ETag) is not - checked: without authors the saves can't be counted. A read of only the - current version works too, since the step count is the difference. - -Anything else stops the run, never a pass: a versions read that fails before -or after the write, a target that existed but has no version, or a target -record without the field that names it. A record created in the window has -no earlier version to overwrite and isn't judged. Only targets are judged: -another user's save to an unrelated record leaves this check as it was (the -frame and its attribution judge that record, as before). + author is not checked. The read must be a history that only grows: every + version read before the write must still be there after it, or the check + is not checked. A read of only the current version and its last author + would hide the other user's save. +- Without it, versions must be whole numbers that step by one per save of + that record (HAPI's `res_ver`, a `version_number` column). At most one + step may happen. Two or more steps are refuted: the agent saved once, so + someone else saved too. A version that went back is refuted. A version + that isn't a whole number (an opaque ETag) is not checked: without authors + the saves can't be counted. A read of only the current version works here, + since the step count is the difference. The numbers must be per record and + contiguous: a global sequence, such as an audit table's row id, also moves + for other records' saves, so a correct update would be refuted. Use + `author_field` with such a sequence. + +Anything else stops the run, never a pass: + +- a versions read that fails before or after the write; +- a target that existed but has no version, or a target record without the + field that names it; +- a version that reads as null, before or after (`null_version`); +- two rows of one record with the same version (`duplicate_version`): a + save could hide behind another; +- a target whose record changed in the window with no new version + (`no_new_version`): the app saved without a version row, so who saved + can't be told. A target that didn't change (already right, nothing + written) needs no new version. + +A record created in the window has no earlier version to overwrite and isn't +judged. Only targets are judged: another user's save to an unrelated record +leaves this check as it was (the frame and its attribution judge that +record, as before). The versions are read before the records at the +baseline, so a save between the two reads shows in the record baseline. The +window starts at the baseline: a copy the agent read before `begin_write` is +not covered. The baseline keeps versions as keyed MACs of the record and the version; only a whole-number version's value is kept, to count steps. The receipt's `evidence.frame.versions` holds counts and a problem code (`another_author`, -`extra_steps`, `went_back`, `no_author`, `not_a_number`, `missing`, +`extra_steps`, `went_back`, `no_author`, `not_a_number`, `null_version`, +`duplicate_version`, `no_new_version`, `history_changed`, `missing`, `no_target_id`, `unreadable`, `self_unresolved`), never ids or values. A refuted check refutes the frame (`checked.frame: refuted`); one that can't be judged leaves the frame `not_checked`. diff --git a/packages/flow/openadapt_flow/runtime/effects/versions.py b/packages/flow/openadapt_flow/runtime/effects/versions.py index c79f0e11d0..01e5a91bb4 100644 --- a/packages/flow/openadapt_flow/runtime/effects/versions.py +++ b/packages/flow/openadapt_flow/runtime/effects/versions.py @@ -54,6 +54,10 @@ "extra_steps", "went_back", "self_unresolved", + "null_version", + "duplicate_version", + "no_new_version", + "history_changed", ) _WHOLE = re.compile(r"^(0|[1-9][0-9]*)$") @@ -169,13 +173,18 @@ def seal_versions( if record is None: continue # a row that names no record is no record's version entry = records.setdefault( - _mac(key, "record", record), {"versions": [], "max": None, "whole": True} + _mac(key, "record", record), + {"versions": [], "max": None, "whole": True, "null": False, "dup": False}, ) if version is None: + # A null version can't tell two saves apart. entry["whole"] = False - entry["versions"].append(_mac(key, "version", record, "\x00null")) + entry["null"] = True continue - entry["versions"].append(_mac(key, "version", record, version)) + mac = _mac(key, "version", record, version) + if mac in entry["versions"]: + entry["dup"] = True # two rows, one version: a save could hide + entry["versions"].append(mac) if _WHOLE.match(version): entry["max"] = max(int(version), entry["max"] or 0) else: @@ -201,7 +210,7 @@ def judge_versions( spec: FrameVersions, stored: Mapping[str, Any], rows: Optional[Sequence[Mapping[str, Any]]], - targets: Sequence[tuple[Optional[str], bool]], + targets: Sequence[tuple[Optional[str], bool, bool]], key: bytes, params: Mapping[str, object] = {}, ) -> VersionVerdict: @@ -210,7 +219,8 @@ def judge_versions( ``stored``: :func:`seal_versions` of the baseline read. ``rows``: the read after the save (None when it failed). ``targets``: per record a confirmed write effect matched, its ``target_field`` value (None when the record - has none) and whether it existed before the write. + has none), whether it existed before the write, and whether the record + changed in the window. """ if not stored.get("reachable") or rows is None: @@ -239,7 +249,7 @@ def stop(status: VersionStatus, problem: str, reason: str) -> None: worst = VersionVerdict(status, problem, reason=reason) seen: set[Optional[str]] = set() - for target, existed in targets: + for target, existed, changed in targets: if target in seen: continue seen.add(target) @@ -266,15 +276,42 @@ def stop(status: VersionStatus, problem: str, reason: str) -> None: stop("not_checked", "missing", "the target has no version after the write") continue old = set(base.get("versions") or []) - fresh = [] - for r in now: - version = _text(_get(r, spec.version_field)) - mac = _mac( - key, "version", target, "\x00null" if version is None else version + versions = [_text(_get(r, spec.version_field)) for r in now] + if base.get("null") or any(v is None for v in versions): + stop( + "not_checked", + "null_version", + "a version of the target reads as null, so saves can't be told apart", + ) + continue + macs = [_mac(key, "version", target, str(v)) for v in versions] + if base.get("dup") or len(set(macs)) != len(macs): + stop( + "not_checked", + "duplicate_version", + "two rows of the target's versions hold the same version, so a " + "save could hide behind another", ) - if mac not in old: - fresh.append((version, r)) + continue + fresh = [(v, r) for v, r, m in zip(versions, now, macs) if m not in old] out.new_versions += len(fresh) + if changed and not fresh: + stop( + "not_checked", + "no_new_version", + "the target changed in the window but no new version of it was " + "read, so who saved it can't be told", + ) + continue + if spec.author_field and not old <= set(macs): + stop( + "not_checked", + "history_changed", + "a version of the target read before the write is gone after it: " + "the read isn't a history that only grows, so a save in the " + "window could be hidden", + ) + continue if spec.author_field: for _version, r in fresh: author = _text(_get(r, spec.author_field)) diff --git a/packages/flow/openadapt_flow/verify_run.py b/packages/flow/openadapt_flow/verify_run.py index c9cf4ccdb5..1384743192 100644 --- a/packages/flow/openadapt_flow/verify_run.py +++ b/packages/flow/openadapt_flow/verify_run.py @@ -473,12 +473,12 @@ def _judge_versions( if spec is None: return frame field_name = spec.target_field or cfg.frame.identity_field or "id" # type: ignore[union-attr] - old = { - str(r.get(field_name)) - for r in before.records - if r.get(field_name) not in (None, "") - } - targets: list[tuple[Optional[str], bool]] = [] + old: dict[str, dict[str, Any]] = {} + for r in before.records: + if r.get(field_name) not in (None, ""): + old.setdefault(str(r.get(field_name)), dict(r)) + ignored = set(cfg.frame.ignore_fields) # type: ignore[union-attr] + targets: list[tuple[Optional[str], bool, bool]] = [] for effect, verdict in zip(effects, verdicts): if effect.kind not in (EffectKind.RECORD_WRITTEN, EffectKind.FIELD_EQUALS): continue @@ -489,7 +489,13 @@ def _judge_versions( for record in verdict.matched_records: value = record.get(field_name) text = None if value in (None, "") else str(value) - targets.append((text, text in old)) + was = old.get(text) if text is not None else None + # Changed in the window: a field read before differs now (a + # sealed baseline marks a changed field, so it differs too). + changed = was is not None and any( + record.get(k) != v for k, v in was.items() if k not in ignored + ) + targets.append((text, was is not None, changed)) stored = (before.detail or {}).get(FRAME_VERSIONS_DETAIL) result = judge_versions( spec, @@ -1357,6 +1363,13 @@ def capture_baseline( execution_id = _new_execution_id() _tag_reads(verifier, execution_id) salt = secrets.token_bytes(32) + # The versions first: a save between this read and the record read is + # then in the record baseline too, never hidden in the versions one. + versions_rows = ( + _read_versions(cfg, params) + if cfg.frame is not None and cfg.frame.versions is not None + else None + ) state = verifier.capture_pre_state() if not isinstance(state, EffectState): raise VerifyRunError("verifier returned no readable baseline") @@ -1381,7 +1394,7 @@ def capture_baseline( from openadapt_flow.runtime.effects.versions import seal_versions detail[FRAME_VERSIONS_DETAIL] = seal_versions( - cfg.frame.versions, _read_versions(cfg, params), _mac_key(salt) + cfg.frame.versions, versions_rows, _mac_key(salt) ) if detail: state = state.model_copy(update={"detail": {**state.detail, **detail}}) diff --git a/packages/flow/tests/test_frame_versions.py b/packages/flow/tests/test_frame_versions.py index 7a27d6890f..4f2d687b23 100644 --- a/packages/flow/tests/test_frame_versions.py +++ b/packages/flow/tests/test_frame_versions.py @@ -233,3 +233,90 @@ def test_author_and_self_go_together() -> None: def test_a_frame_without_versions_keeps_its_digest() -> None: spec = FrameSpec.model_validate({"identity_field": "id"}) assert "versions" not in spec.model_dump(mode="json", by_alias=True) + + +# -- review round 1 (#1662): each attack ends in a stop ------------------------ + + +def test_r1_a1_authors_with_a_current_version_only_read_is_not_checked( + tmp_path: Path, +) -> None: + # The read shows only the record's current version and its last author: + # the front desk's version is gone after the write. + db = tmp_path / "app.sqlite" + read = { + "kind": "sql", + "sqlite_database": str(db), + "sql_query": "SELECT p.id AS person_id, p.ver AS ver, (SELECT author FROM " + "person_version v WHERE v.person_id = p.id ORDER BY v.id DESC LIMIT 1) " + "AS author FROM person p", + } + result = _run(tmp_path, _lost_update, {**AUTHORS, "read": read}) + assert result["receipt"]["outcome"] != "verified" + assert _versions(result)["problem"] == "history_changed" + + +@pytest.mark.parametrize("versions", [AUTHORS, {}], ids=["authors", "steps"]) +def test_r1_a2_a_null_version_is_not_checked(tmp_path: Path, versions: Any) -> None: + result = _run(tmp_path, _lost_update, {**versions, "version_field": "version"}) + assert result["receipt"]["outcome"] != "verified" + assert _versions(result)["problem"] == "null_version" + + +def test_r1_a3_a_duplicate_version_is_not_checked(tmp_path: Path) -> None: + def window(con: sqlite3.Connection) -> None: + _lost_update(con) + con.execute("UPDATE person_version SET ver = '1' WHERE author = 'frontdesk'") + + result = _run(tmp_path, window, AUTHORS) + assert result["receipt"]["outcome"] != "verified" + assert _versions(result)["problem"] == "duplicate_version" + + +def test_r1_a4_versions_are_read_before_the_records( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + import openadapt_flow.verify_run as vr + + real = vr._read_versions + calls = {"n": 0} + + def wrapped(cfg: Any, params: Any) -> Any: + calls["n"] += 1 + rows = real(cfg, params) + if calls["n"] == 1: # another user saves between the two baseline reads + con = sqlite3.connect(tmp_path / "app.sqlite") + _save(con, 1, "frontdesk", addr="9 New Rd") + con.commit() + con.close() + return rows + + monkeypatch.setattr(vr, "_read_versions", wrapped) + result = _run( + tmp_path, lambda con: _save(con, 1, "bot", phone="555-0199", addr="1 Elm St") + ) + assert result["receipt"]["outcome"] != "verified" + + +@pytest.mark.parametrize("versions", [AUTHORS, {}], ids=["authors", "steps"]) +def test_r1_a_changed_target_with_no_new_version_is_not_checked( + tmp_path: Path, versions: Any +) -> None: + # The app saved without writing a version row. + result = _run( + tmp_path, + lambda con: con.execute("UPDATE person SET phone = '555-0199' WHERE id = 1"), + versions, + ) + assert result["receipt"]["outcome"] != "verified" + assert _versions(result)["problem"] == "no_new_version" + + +def test_r1_an_unchanged_target_with_no_new_version_holds(tmp_path: Path) -> None: + # Already right, nothing written: no save could overwrite anything. + def setup(con: sqlite3.Connection) -> None: + con.execute("UPDATE person SET phone = '555-0199' WHERE id = 1") + + result = _run(tmp_path, lambda con: None, AUTHORS, setup=setup) + assert result["receipt"]["outcome"] == "verified" + assert _versions(result)["new_versions"] == 0 From 19a1e6ef467b9935f8911cf71cc1764221be1df1 Mon Sep 17 00:00:00 2001 From: abrichr Date: Fri, 30 Oct 2026 13:45:34 -0400 Subject: [PATCH 7/7] frame.versions: tell an existing target by its identity; reason text for the new codes; docs on step mode's assumptions --- packages/flow/docs/VERIFY_RUN.md | 21 ++++++++++++------ packages/flow/openadapt_flow/verifier.py | 6 ++++++ packages/flow/openadapt_flow/verify_run.py | 20 +++++++++++------ packages/flow/tests/test_frame_versions.py | 25 ++++++++++++++++++++++ 4 files changed, 60 insertions(+), 12 deletions(-) diff --git a/packages/flow/docs/VERIFY_RUN.md b/packages/flow/docs/VERIFY_RUN.md index 790fb2064f..5c93dc77e5 100644 --- a/packages/flow/docs/VERIFY_RUN.md +++ b/packages/flow/docs/VERIFY_RUN.md @@ -307,11 +307,17 @@ The versions of a target that are new in the window must be the agent's: would hide the other user's save. - Without it, versions must be whole numbers that step by one per save of that record (HAPI's `res_ver`, a `version_number` column). At most one - step may happen. Two or more steps are refuted: the agent saved once, so - someone else saved too. A version that went back is refuted. A version + step may happen. Two or more steps are refuted: step mode assumes the + agent saves each record once per run, so a second step is someone else's. + An agent that saves the same record twice (correcting its own write) needs + `author_field`. A version that went back is refuted. A version that isn't a whole number (an opaque ETag) is not checked: without authors - the saves can't be counted. A read of only the current version works here, - since the step count is the difference. The numbers must be per record and + the saves can't be counted. Prefer a history read, one row per version. A + read of only the current version counts steps as the difference, which + trusts the app's numbering: an app that numbers a save from the client's + copy (two saves from version 1 both write version 2) hides a lost update + there, while a history read shows the two rows as `duplicate_version`. + The numbers must be per record and contiguous: a global sequence, such as an audit table's row id, also moves for other records' saves, so a correct update would be refuted. Use `author_field` with such a sequence. @@ -327,10 +333,13 @@ Anything else stops the run, never a pass: - a target whose record changed in the window with no new version (`no_new_version`): the app saved without a version row, so who saved can't be told. A target that didn't change (already right, nothing - written) needs no new version. + written) needs no new version; this relies on the read recording every + save, since a change reverted by a save the read misses looks unchanged. A record created in the window has no earlier version to overwrite and isn't -judged. Only targets are judged: another user's save to an unrelated record +judged. Whether a target existed is told by `frame.identity_field` (default +`id`), never by `target_field`'s value after the write: a target that existed +with no version under its current `target_field` value is `missing`. Only targets are judged: another user's save to an unrelated record leaves this check as it was (the frame and its attribution judge that record, as before). The versions are read before the records at the baseline, so a save between the two reads shows in the record baseline. The diff --git a/packages/flow/openadapt_flow/verifier.py b/packages/flow/openadapt_flow/verifier.py index 15cde7b240..b2b3916b92 100644 --- a/packages/flow/openadapt_flow/verifier.py +++ b/packages/flow/openadapt_flow/verifier.py @@ -1577,6 +1577,12 @@ def _scope( "no_target_id": "a target record has no field naming it in the versions read", "unreadable": "the target's versions could not be read, before or after the write", "self_unresolved": "frame.versions.self names a parameter the run did not supply", + "null_version": "a version of the target reads as null, so saves can't be told apart", + "duplicate_version": "two rows of the target's versions hold the same version, so a " + "save could hide behind another", + "no_new_version": "the target changed in the window but no new version of it was read", + "history_changed": "a version of the target read before the write is gone after it, " + "so a save in the window could be hidden", } diff --git a/packages/flow/openadapt_flow/verify_run.py b/packages/flow/openadapt_flow/verify_run.py index 1384743192..dfce34b2bf 100644 --- a/packages/flow/openadapt_flow/verify_run.py +++ b/packages/flow/openadapt_flow/verify_run.py @@ -473,10 +473,14 @@ def _judge_versions( if spec is None: return frame field_name = spec.target_field or cfg.frame.identity_field or "id" # type: ignore[union-attr] + # Whether a target existed is told by its identity, which a save doesn't + # change, never by the target field's value after the write: a target + # field the write changed would make an old record look created. + identity = cfg.frame.identity_field or "id" # type: ignore[union-attr] old: dict[str, dict[str, Any]] = {} for r in before.records: - if r.get(field_name) not in (None, ""): - old.setdefault(str(r.get(field_name)), dict(r)) + if r.get(identity) not in (None, ""): + old.setdefault(str(r.get(identity)), dict(r)) ignored = set(cfg.frame.ignore_fields) # type: ignore[union-attr] targets: list[tuple[Optional[str], bool, bool]] = [] for effect, verdict in zip(effects, verdicts): @@ -489,13 +493,17 @@ def _judge_versions( for record in verdict.matched_records: value = record.get(field_name) text = None if value in (None, "") else str(value) - was = old.get(text) if text is not None else None + key = record.get(identity) + was = None if key in (None, "") else old.get(str(key)) + # No identity to look up: never assume the record is new. + existed = was is not None or key in (None, "") # Changed in the window: a field read before differs now (a # sealed baseline marks a changed field, so it differs too). - changed = was is not None and any( - record.get(k) != v for k, v in was.items() if k not in ignored + changed = existed and ( + was is None + or any(record.get(k) != v for k, v in was.items() if k not in ignored) ) - targets.append((text, was is not None, changed)) + targets.append((text, existed, changed)) stored = (before.detail or {}).get(FRAME_VERSIONS_DETAIL) result = judge_versions( spec, diff --git a/packages/flow/tests/test_frame_versions.py b/packages/flow/tests/test_frame_versions.py index 4f2d687b23..56b0fca0be 100644 --- a/packages/flow/tests/test_frame_versions.py +++ b/packages/flow/tests/test_frame_versions.py @@ -320,3 +320,28 @@ def setup(con: sqlite3.Connection) -> None: result = _run(tmp_path, lambda con: None, AUTHORS, setup=setup) assert result["receipt"]["outcome"] == "verified" assert _versions(result)["new_versions"] == 0 + + +# -- review round 2 (#1662) ------------------------------------------------------ + + +def test_r2_i2_a_target_field_the_write_changes_is_not_a_new_record( + tmp_path: Path, +) -> None: + # The versions are keyed by the phone, which the agent's write changes: + # the record existed (by its identity), so no baseline under the new + # phone is a missing version, not a record created in the window. + db = tmp_path / "app.sqlite" + read = { + "kind": "sql", + "sqlite_database": str(db), + "sql_query": "SELECT p.phone AS phone, v.ver FROM person_version v " + "JOIN person p ON p.id = v.person_id", + } + result = _run( + tmp_path, + _lost_update, + {"target_field": "phone", "record_field": "phone", "read": read}, + ) + assert result["receipt"]["outcome"] != "verified" + assert _versions(result)["problem"] == "missing"