fix(mcp): a reader that does not migrate must report the gap, not raise on it (#751)

keel/commands/doctor.py

@@ -305,9 +305,12 @@ def audit_chain_findings(state: Any) -> list[Finding]:
 
     THREE readings, and the middle one is the whole reason this is not a boolean:
 
-    * **no `audit_events` table** -- a database this build has not migrated. Both the web server
-      and `keel mcp` open a repo WITHOUT migrating (a view must not take a schema write lock), so
-      an un-upgraded database reaching a reader is ordinary, not an error.
+    * **no `audit_events` table** -- a database this build has not migrated. `keel mcp` opens a
+      repo WITHOUT migrating (a view must not take a schema write lock), so an un-upgraded
+      database reaching a reader is ordinary, not an error. (This said "both the web server and
+      `keel mcp`" until #751. Only the PER-REQUEST open skips migration; `web/server.serve` calls
+      `ensure_schema` once at bind time, so a served database is never behind. The imprecision
+      mattered: it is what left the sibling readers looking already covered.)
     * **a table with no events** -- nothing has been written since the chain shipped. Reported as
       OK, and the headline says UNVERIFIED rather than verified: an empty chain has no breaks
       because it has nothing in it to break, and calling that "verified" is a positive claim over
@@ -391,6 +394,30 @@ def _utc_date(ts: int) -> str:
     return datetime.fromtimestamp(ts, tz=UTC).date().isoformat()
 
 
+def cash_posture_schema_finding(venue: str) -> list[Finding]:
+    """Rail 22's posture table is not on this database (#751).
+
+    The same shape `audit_chain_findings` gives an absent `audit_events`, and for the same
+    reason: `keel mcp` opens a repo WITHOUT migrating, so an un-upgraded database reaching a
+    reader is ordinary, not an error.
+
+    OK rather than WARN. Nothing has lapsed and nothing is unsafe -- the engine that would
+    enforce rail 22 migrates on the way in, so a live cycle can never be running against this
+    schema. What is true is only that this READER is looking at a database older than the rail,
+    and the fix is one idempotent command.
+    """
+    return [
+        Finding(
+            "attest.cash_posture",
+            OK,
+            "cash posture not on this database",
+            f"schema predates rail 22's posture record, so there is nothing recorded for "
+            f"{venue} to check -- this reader does not migrate; the engine does",
+            "keel migrate",
+        )
+    ]
+
+
 def cash_posture_findings(
     record: VenueCashPosture | None, *, venue: str, now_ts: int
 ) -> list[Finding]:
@@ -1524,8 +1551,10 @@ def gather_findings(repo: Any, config: Any, log_lines: Iterable[str], now_ts: in
     that counts `sqlite3`'s own change counter around a call, because "the tool is read-only"
     is a property of the gather, not of whoever happens to call it this time.
 
-    The caller owns opening: the command migrates on the way in, the MCP tool deliberately
-    does not (the `keel/web/server.py` rule -- a view must not take a schema write lock).
+    The caller owns opening, and they do not open alike: the command migrates on the way in and
+    `keel serve` migrates once at bind, but the MCP tool deliberately does not at all (the
+    `keel/web/server.py` rule -- a view must not take a schema write lock). So every read below
+    must tolerate a schema older than this build; `repo.table_present` is how (#751).
     `log_lines` are the engine log's own lines, read by the caller so each front-end can
     point at the file it was wired with.
     """
@@ -1577,9 +1606,14 @@ def gather_findings(repo: Any, config: Any, log_lines: Iterable[str], now_ts: in
     findings += trade_scope_findings(repo.get_venue_trade_scope(venue), venue)
     # #691. Venue-keyed the same way, and reported BEFORE it bites: rail 22 vetoes silently on
     # a lapse, and the live profile runs unattended.
-    findings += cash_posture_findings(
-        repo.get_venue_cash_posture(venue), venue=venue, now_ts=now_ts
-    )
+    # The table itself may be absent (#751): this seam is shared with `keel mcp`, which opens a
+    # repo without migrating, and `get_venue_cash_posture` would raise rather than answer.
+    if not repo.table_present("venue_cash_postures"):
+        findings += cash_posture_schema_finding(venue)
+    else:
+        findings += cash_posture_findings(
+            repo.get_venue_cash_posture(venue), venue=venue, now_ts=now_ts
+        )
     findings += rail_state_findings(
         kill_switch=bool(repo.get_state("kill_switch", default=False)),
         streak_halt_until=int(repo.get_state("streak_halt_until", default=0) or 0),

keel/data/audit.py

@@ -320,10 +320,15 @@ def _latest(rows: list[sqlite3.Row]) -> dict[tuple[str, str], EntityHash]:
 
 
 def _table_present(conn: sqlite3.Connection) -> bool:
-    row = conn.execute(
-        "SELECT 1 FROM sqlite_master WHERE type = 'table' AND name = 'audit_events'"
-    ).fetchone()
-    return row is not None
+    """Delegates to `db.table_present` (#751), which is the same check generalised.
+
+    It lived here first, as the only reader that had one. Three later tables (`venue_cash_postures`,
+    `equity_points`, `candle_series_feed`) needed the identical guard and did not get it, so the
+    check moved to where the schema is defined and every reader can reach it.
+    """
+    from keel.data.db import table_present
+
+    return table_present(conn, "audit_events")
 
 
 def chain_state(conn: sqlite3.Connection) -> ChainState:

keel/data/db.py

@@ -1097,6 +1097,38 @@ def connect(path: str | Path = "keel.db") -> sqlite3.Connection:
     return conn
 
 
+def table_present(conn: sqlite3.Connection, name: str) -> bool:
+    """Does `name` exist on this database?
+
+    **The question a reader asks, because a reader may not migrate.** Every CLI command migrates
+    on the way in, and `keel serve` does it once at bind time (`web/server.ensure_schema`). But
+    `keel mcp` opens a repo without migrating at all -- a view must not take a schema write lock
+    on a database the agent may be mid-cycle on -- so meeting a table that a later `SCHEMA_VERSION`
+    added is an ordinary deployment state for it, not an error.
+
+    **Checked rather than caught.** `sqlite3.OperationalError` covers "no such table" and
+    "database disk image is malformed" under one clause, and that distinction is the difference
+    between `keel migrate` and stop trading.
+
+    The narrower claim, because a sibling already does it the other way and does it correctly:
+    `Repository.get_series_feeds` catches `OperationalError` and re-raises unless the message
+    contains "no such table", which preserves exactly the distinction above. That works. What it
+    costs is a dependency on the TEXT of a sqlite error message, which is not part of sqlite's
+    API and has been reworded across releases before. Asking `sqlite_master` needs no such
+    match, and reads as the question being asked rather than as a filter over a failure -- so it
+    is the preferred idiom for a NEW reader. The existing catch is not a bug to go fix; it is a
+    second correct answer that this one supersedes.
+
+    Asking `sqlite_master` rather than the `schema_version` row on purpose: the row says which
+    migrations were RUN, and the tables are also created by `_SCHEMA_STATEMENTS` on any fresh
+    database regardless of version. What a reader needs to know is whether the table is there.
+    """
+    row = conn.execute(
+        "SELECT 1 FROM sqlite_master WHERE type = 'table' AND name = ?", (name,)
+    ).fetchone()
+    return row is not None
+
+
 def migrate(conn: sqlite3.Connection) -> None:
     """Create all tables + indexes if absent, then run any outstanding migration steps.
 

keel/data/repository.py

@@ -401,6 +401,23 @@ def audit_chain(self) -> ChainState:
         """
         return chain_state(self._conn)
 
+    def table_present(self, name: str) -> bool:
+        """Does this database have `name`? For readers that may not have migrated (#751).
+
+        On the repository rather than reached for through the connection, for the reason
+        `audit_chain` is: "what does this record actually contain" is a question about the record,
+        and the record is what this class owns.
+
+        A reader asks this to tell an ABSENT table from an EMPTY one, which are different
+        statements about a deployment and want different sentences. `get_venue_cash_posture`
+        returning `None` means no human has attested the posture and rail 22 is vetoing entries;
+        the table being absent means the schema predates rail 22 and nothing is wrong yet. Saying
+        the first about the second sends an operator to fix a rail that is not the problem.
+        """
+        from keel.data.db import table_present
+
+        return table_present(self._conn, name)
+
     # -- transactions ---------------------------------------------------
 
     def upsert_transaction(self, tx: dict[str, Any]) -> None:

tests/commands/test_doctor.py

@@ -14,9 +14,11 @@
 import json
 from decimal import Decimal
 from pathlib import Path
+from typing import Any
 
 from keel_core.trade_scope import READ_ONLY, TRADING, TradeScopeState, VenueTradeScope
 
+from keel.commands._products import _default_sim_products
 from keel.commands.doctor import (
     OK,
     AdmissibilityRow,
@@ -44,7 +46,7 @@
 from keel.data.db import connect, migrate
 from keel.data.freshness import Freshness
 from keel.data.repository import Repository
-from keel.types import Granularity
+from keel.types import Candle, Granularity
 
 NOW = 1_784_500_000
 DAY = 86_400
@@ -1127,3 +1129,114 @@ def test_a_database_without_the_table_is_reported_not_crashed_on() -> None:
     (finding,) = audit_chain_findings(_chain(table_present=False))
     assert finding.status == "ok"
     assert finding.fix == "keel migrate"
+
+
+# -- an un-migrated database reaching a reader (#751) --------------------------------------------
+
+#: Everything v17-v20 added, dropped together so the fixture is a faithful `v0.13.3` schema: the
+#: OLD `_SCHEMA_STATEMENTS` created none of them, and no migration has run.
+#:
+#: **Faithful is not the same as covered**, and the difference is spelled out because the first
+#: cut of this test got it wrong. Dropping a table only proves something if a reader in this seam
+#: READS it: `venue_cash_postures` (v18) is read on every gather and `audit_events` (v20) likewise,
+#: but `candle_series_feed` (v17) is read only for a series that HAS bars -- hence the seeded
+#: candle below -- and nothing in `gather_findings` touches `equity_points` (v19) at all. That
+#: last one is dropped for fidelity alone; the reader that would exercise it lives in `keel/web`,
+#: behind `ensure_schema`.
+_TABLES_ADDED_SINCE_0_13_3 = (
+    "candle_series_feed",  # v17
+    "venue_cash_postures",  # v18
+    "equity_points",  # v19
+    "audit_events",  # v20
+)
+
+
+def _repo_stopped_at_v16(db_path: Path, config: Any):
+    """A repo over a database at the schema `v0.13.3` shipped -- opened WITHOUT migrating.
+
+    Deliberately a REAL database rather than a fabricated state object. The guard next door
+    (`test_a_database_without_the_table_is_reported_not_crashed_on`) asserts over
+    `_chain(table_present=False)`, which can only ever exercise the one reader that already has
+    the flag -- so the sibling readers that lacked it stayed invisible to it for three migrations.
+
+    **The candle is load-bearing.** `feed_scope_findings` reads provenance only for a series with
+    `n_candles > 0`, so against an empty database `get_series_feeds` is never called and dropping
+    `candle_series_feed` would prove exactly nothing. Seeded before the drop, in the product and
+    granularity `gather_findings` will actually ask about, so the v17 reader is on the path.
+
+    The version is wound back in the `schema_version` TABLE, which is what `migrate` reads. Not
+    `PRAGMA user_version`, which this codebase does not use for schema versioning.
+    """
+    conn = connect(str(db_path))
+    migrate(conn)
+    repo = Repository(conn)
+    product = _default_sim_products(config)[0]
+    granularity = list(config.market_data.granularities)[0]
+    repo.upsert_candles(
+        product,
+        granularity,
+        [
+            Candle(
+                ts=NOW - 3_600,
+                open=Decimal("1"),
+                high=Decimal("1"),
+                low=Decimal("1"),
+                close=Decimal("1"),
+                volume=Decimal("1"),
+            )
+        ],
+    )
+    for table in _TABLES_ADDED_SINCE_0_13_3:
+        conn.execute(f"DROP TABLE IF EXISTS {table}")
+    conn.execute("UPDATE schema_version SET version = 16")
+    conn.commit()
+    return repo
+
+
+def test_gather_findings_survives_a_database_nobody_has_migrated(tmp_path, valid_config_path):
+    """`keel mcp` opens a repo without migrating, so this is an ordinary deployment state.
+
+    It raised `sqlite3.OperationalError: no such table: venue_cash_postures` -- out of the MCP
+    handler, taking every other finding with it, on the first tool a client typically calls.
+    """
+    config = load_config(valid_config_path)
+    repo = _repo_stopped_at_v16(tmp_path / "keel.db", config)
+    findings = gather_findings(repo, config, [], NOW)
+    assert findings, "an un-migrated database must still produce a report"
+
+
+def test_the_v17_provenance_reader_is_actually_on_the_path(tmp_path, valid_config_path):
+    """The pin on the fixture's seeded candle, so `candle_series_feed` is covered and not merely
+    dropped.
+
+    Without a bar, `feed_scope_findings` is handed an empty mapping and `get_series_feeds` is
+    never called -- measured: 0 calls. The tuple above would then name a table nothing reads,
+    which is coverage in appearance only.
+    """
+    config = load_config(valid_config_path)
+    repo = _repo_stopped_at_v16(tmp_path / "keel.db", config)
+    calls = []
+    inner = repo.get_series_feeds
+    repo.get_series_feeds = lambda *a, **k: (calls.append(a), inner(*a, **k))[1]  # type: ignore[method-assign]
+    findings = gather_findings(repo, config, [], NOW)
+    assert calls, "get_series_feeds was never reached -- the dropped v17 table proves nothing"
+    assert [f for f in findings if f.name == "data.feed_scope"]
+
+
+def test_an_unmigrated_posture_table_is_not_reported_as_an_unattested_posture(
+    tmp_path, valid_config_path
+):
+    """The distinction the audit chain already draws, and the reason a bare `None` will not do.
+
+    `cash_posture_findings(None)` is FAIL "cash posture never attested -- rail 22 vetoes every
+    live ENTRY". That sentence is false about a database that has no posture TABLE: nothing has
+    lapsed, the schema simply predates rail 22. Telling an operator to re-attest sends them to
+    fix a rail that is not the problem.
+    """
+    config = load_config(valid_config_path)
+    repo = _repo_stopped_at_v16(tmp_path / "keel.db", config)
+    findings = gather_findings(repo, config, [], NOW)
+    (posture,) = [f for f in findings if f.name == "attest.cash_posture"]
+    assert posture.status == OK, posture.detail
+    assert posture.fix == "keel migrate"
+    assert "never attested" not in posture.headline

tests/mcp/test_tools.py

@@ -189,3 +189,70 @@ def test_trials_chain_errors_are_tail_bounded(tmp_path: Path, monkeypatch: Any)
     assert all(error.startswith("row ") for error in result["chain_errors"][:20])
     assert result["chain_errors"][-1] == f"+{total - 20} more chain errors"
     assert result["rows"] == total
+
+
+# -- an un-migrated database, across the whole tool surface (#751) --------------------------------
+
+#: Everything v17-v20 added, dropped together so the fixture is a faithful `v0.13.3` schema.
+#: Which of them any given tool actually READS is the point of the sweep below -- see
+#: `tests/commands/test_doctor.py` for why dropping a table nobody reads proves nothing.
+_TABLES_ADDED_SINCE_0_13_3 = (
+    "candle_series_feed",  # v17
+    "venue_cash_postures",  # v18
+    "equity_points",  # v19
+    "audit_events",  # v20
+)
+
+
+def test_no_tool_raises_against_a_database_nobody_has_migrated(tmp_path, valid_config_path) -> None:
+    """`_open_readonly_repo` deliberately does not migrate, so every handler must tolerate a
+    schema older than itself.
+
+    **Swept across the whole surface rather than asserted on `doctor` alone**, which is the gap
+    that let this ship: `venue_cash_postures` (v18) and `equity_points` (v19) both landed without
+    the guard `audit_events` (v20) has, and the only test of that guard fabricated a state object,
+    so it could never see a reader that lacked one. A tool added later inherits this sweep for
+    free; a table added later needs only to join the tuple above.
+
+    **Every handler must RETURN A DOCUMENT, not merely fail to raise one exception type.** The
+    first cut asserted `except sqlite3.OperationalError: raise` with a bare `except Exception:
+    pass` beneath it, which could only ever prove "no `OperationalError`" -- so a tool that began
+    wrapping its database errors, or that broke outright for an unrelated reason, would have kept
+    this green while the bug came back. A handler that cannot answer over this schema is a
+    failure here whatever it raises.
+    """
+    from keel.data.db import connect, migrate
+
+    db = tmp_path / "keel.db"
+    conn = connect(str(db))
+    migrate(conn)
+    for table in _TABLES_ADDED_SINCE_0_13_3:
+        conn.execute(f"DROP TABLE IF EXISTS {table}")
+    conn.execute("UPDATE schema_version SET version = 16")
+    conn.commit()
+    conn.close()
+
+    log = tmp_path / "keel.log"
+    log.write_text("")
+    tools = build_tools(db_path=str(db), config_path=str(valid_config_path), log_path=str(log))
+    assert {tool.name for tool in tools} == {
+        "doctor",
+        "capabilities",
+        "profiles",
+        "orders",
+        "veto_log",
+        "purification",
+        "trials",
+        "reports",
+    }, "the sweep must cover the whole surface -- a new tool joins this set deliberately"
+
+    for tool in tools:
+        try:
+            document = tool.handler({})
+        except Exception as exc:
+            raise AssertionError(
+                f"MCP tool {tool.name!r} raised {type(exc).__name__} against an un-migrated "
+                f"database: {exc}. A reader that does not migrate must report the gap, not "
+                "propagate it."
+            ) from exc
+        assert isinstance(document, dict), f"{tool.name} answered {type(document).__name__}"