diff --git a/keel/commands/doctor.py b/keel/commands/doctor.py index 8e5bc678..d2b1a850 100644 --- a/keel/commands/doctor.py +++ b/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), diff --git a/keel/data/audit.py b/keel/data/audit.py index cc1a274c..dc87ad6b 100644 --- a/keel/data/audit.py +++ b/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: diff --git a/keel/data/db.py b/keel/data/db.py index bd09cffd..450d6b89 100644 --- a/keel/data/db.py +++ b/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. diff --git a/keel/data/repository.py b/keel/data/repository.py index 61f636c7..48716524 100644 --- a/keel/data/repository.py +++ b/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: diff --git a/tests/commands/test_doctor.py b/tests/commands/test_doctor.py index f20e5847..5d24a587 100644 --- a/tests/commands/test_doctor.py +++ b/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 diff --git a/tests/mcp/test_tools.py b/tests/mcp/test_tools.py index c12921e8..b414302a 100644 --- a/tests/mcp/test_tools.py +++ b/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__}"