From 364b002f0cfd8572e9114cf2b5a62a7752e4aeb7 Mon Sep 17 00:00:00 2001 From: Elmehdi Aitbrahim Date: Mon, 7 Sep 2026 09:21:02 -0400 Subject: [PATCH 1/2] fix(mcp): a reader that does not migrate must report the gap, not raise on it (#751) `keel mcp`'s `doctor` tool raised `sqlite3.OperationalError: no such table: venue_cash_postures` against a database nobody had migrated -- out of the handler, taking every other finding with it, on the first tool a client typically calls. THE RULE WAS ALREADY WRITTEN NEXT DOOR `audit_chain_findings` states it in its own docstring: a reader that does not migrate meets an un-upgraded schema as an ordinary deployment state, not an error. It backs that with a `table_present` check and an OK finding naming `keel migrate`. Three migrations later, `venue_cash_postures` (v18) and `equity_points` (v19) had no such guard. The check moved from `audit.py` to `db.table_present`, where the schema is defined and every reader can reach it, plus `Repository.table_present` for the same reason `audit_chain` is exposed there: what this record contains is a question about the record. Checked, not caught. `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. AN ABSENT TABLE IS NOT AN UNATTESTED POSTURE A bare `None` would have been worse than the crash. `cash_posture_findings(None)` is FAIL "cash posture never attested -- rail 22 vetoes every live ENTRY", which is false about a database that has no posture table: nothing has lapsed, the schema simply predates the rail. It would have sent an operator to fix a rail that is not the problem. `cash_posture_schema_finding` says the true thing instead, OK, because the engine that enforces rail 22 migrates on the way in and so can never be running against this schema. WHY THE EXISTING TEST COULD NOT SEE IT `test_a_database_without_the_table_is_reported_not_crashed_on` asserts over a fabricated `_chain(table_present=False)`. A hand-built state object can only exercise the reader that already has the flag, so the readers that lacked one stayed invisible to it. The new tests open a REAL database wound back to what 0.13.3 shipped, and the MCP one sweeps EVERY tool rather than `doctor` alone -- a tool added later inherits it, a table added later joins one tuple. Verified by mutation: forcing `table_present` to `True` fails all three. AND THE DOCSTRING THAT CAUSED IT `audit_chain_findings` said "both the web server and `keel mcp` open a repo WITHOUT migrating". Only the PER-REQUEST open skips it; `web/server.serve` calls `ensure_schema` once at bind, so a served database is never behind. The imprecision is what left the sibling readers looking already covered -- and it had propagated into v0.14.0's release notes before this was traced. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01KZZxmspQXe5qJ9FAsG13s6 --- keel/commands/doctor.py | 50 ++++++++++++++++++++++++----- keel/data/audit.py | 13 +++++--- keel/data/db.py | 25 +++++++++++++++ keel/data/repository.py | 17 ++++++++++ tests/commands/test_doctor.py | 60 +++++++++++++++++++++++++++++++++++ tests/mcp/test_tools.py | 52 ++++++++++++++++++++++++++++++ 6 files changed, 205 insertions(+), 12 deletions(-) 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..2b1a3b69 100644 --- a/keel/data/db.py +++ b/keel/data/db.py @@ -1097,6 +1097,31 @@ 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**, which is the whole reason this is a function and not a + `try`/`except`. `sqlite3.OperationalError` covers "no such table" and "database disk image is + malformed" under one clause, so catching it would report an un-upgraded database where the + truth is a corrupted one. That distinction is the difference between `keel migrate` and stop + trading, and no caller can recover it downstream. + + 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..2e2a326b 100644 --- a/tests/commands/test_doctor.py +++ b/tests/commands/test_doctor.py @@ -1127,3 +1127,63 @@ 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) -------------------------------------------- + +#: What v17-v20 added, in the order a 0.13.3 deployment did not have them. Dropping these from a +#: current database and winding `user_version` back reproduces exactly what an operator who +#: installed 0.14.0 and has not yet run `keel migrate` hands a reader: the OLD `_SCHEMA_STATEMENTS` +#: created none of them, and no migration has run. +_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): + """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. + """ + conn = connect(str(db_path)) + 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() + return Repository(conn) + + +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. + """ + repo = _repo_stopped_at_v16(tmp_path / "keel.db") + findings = gather_findings(repo, load_config(valid_config_path), [], NOW) + assert findings, "an un-migrated database must still produce a report" + + +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. + """ + repo = _repo_stopped_at_v16(tmp_path / "keel.db") + findings = gather_findings(repo, load_config(valid_config_path), [], 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..73b9bdcb 100644 --- a/tests/mcp/test_tools.py +++ b/tests/mcp/test_tools.py @@ -189,3 +189,55 @@ 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 from a current database, this is what a deployment that +#: installed 0.14.0 and has not run `keel migrate` hands the MCP surface. +_TABLES_ADDED_SINCE_0_13_3 = ( + "candle_series_feed", + "venue_cash_postures", + "equity_points", + "audit_events", +) + + +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. + """ + 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 tools, "no tools built" + + for tool in tools: + try: + tool.handler({}) + except sqlite3.OperationalError as exc: # the failure this test exists for + raise AssertionError( + f"MCP tool {tool.name!r} raised against an un-migrated database: {exc}. " + "A reader that does not migrate must report the gap, not propagate it." + ) from exc + except Exception: + # Anything else is that tool's own argument or environment contract, not schema + # tolerance, and is covered by its own test above. + pass From 2e57349f980ae336786a9159d9c63e24454c02ef Mon Sep 17 00:00:00 2001 From: Elmehdi Aitbrahim Date: Mon, 7 Sep 2026 09:45:11 -0400 Subject: [PATCH 2/2] test(mcp): the fixture dropped two tables nothing reads, which is coverage in appearance only Review of this PR's own diff. Four findings, and the first is the failure this series keeps finding in other people's tests. TWO OF THE FOUR DROPPED TABLES WERE INERT `_TABLES_ADDED_SINCE_0_13_3` names four tables and reads as coverage of four. Instrumented against the fixture, `gather_findings` calls: get_series_feeds: 0 get_equity_points: 0 table_present: 1 `feed_scope_findings` reads provenance only for a series with `n_candles > 0`, and the fixture had no candles -- so `candle_series_feed` (v17) was dropped from a database no reader in this seam ever asked about. `equity_points` (v19) is read by nothing in `gather_findings` at all. The fixture now seeds a bar in the product and granularity `gather_findings` actually asks about, so the v17 reader is on the path, and a new test pins that it is REACHED rather than merely present -- deleting the seeding line fails it with "get_series_feeds was never reached". `equity_points` stays dropped for fidelity and the comment says that is all it is: the reader that would exercise it lives in `keel/web`, behind `ensure_schema`. THE NEW DOCSTRING CONTRADICTED A SIBLING TWELVE LINES AWAY `db.table_present` said catching `OperationalError` "would report an un-upgraded database where the truth is a corrupted one". `Repository.get_series_feeds` catches exactly that and re-raises unless the message contains "no such table", which preserves the distinction the docstring claimed catching destroys. It is a second correct answer, not a bug. Narrowed to the claim that survives: the catch costs a dependency on the TEXT of a sqlite error message, which is not part of sqlite's API. Presence-checking needs no such match. Preferred for a new reader; the existing catch is left alone and is now named rather than implicitly contradicted. THE SWEEP COULD ONLY PROVE "NO OperationalError" `except sqlite3.OperationalError: raise` over a bare `except Exception: pass` would have stayed green for a tool that began wrapping its database errors, or that broke outright for an unrelated reason. Every handler must now RETURN A DOCUMENT, and the tool set itself is pinned so a new tool joins the sweep deliberately rather than silently. All eight pass. AND A COMMENT DESCRIBING THE MECHANISM IT NO LONGER USES "winding `user_version` back" -- the code does `UPDATE schema_version SET version = 16`. `user_version` is a PRAGMA this codebase does not use for schema versioning; the row `migrate` reads is the one that matters. 6,309 passed / 3 skipped. The `table_present -> True` mutant is killed by all four tests, and the seeded candle by its own. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01KZZxmspQXe5qJ9FAsG13s6 --- keel/data/db.py | 17 +++++--- tests/commands/test_doctor.py | 75 ++++++++++++++++++++++++++++++----- tests/mcp/test_tools.py | 45 ++++++++++++++------- 3 files changed, 106 insertions(+), 31 deletions(-) diff --git a/keel/data/db.py b/keel/data/db.py index 2b1a3b69..450d6b89 100644 --- a/keel/data/db.py +++ b/keel/data/db.py @@ -1106,11 +1106,18 @@ def table_present(conn: sqlite3.Connection, name: str) -> bool: 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**, which is the whole reason this is a function and not a - `try`/`except`. `sqlite3.OperationalError` covers "no such table" and "database disk image is - malformed" under one clause, so catching it would report an un-upgraded database where the - truth is a corrupted one. That distinction is the difference between `keel migrate` and stop - trading, and no caller can recover it downstream. + **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 diff --git a/tests/commands/test_doctor.py b/tests/commands/test_doctor.py index 2e2a326b..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 @@ -1131,10 +1133,16 @@ def test_a_database_without_the_table_is_reported_not_crashed_on() -> None: # -- an un-migrated database reaching a reader (#751) -------------------------------------------- -#: What v17-v20 added, in the order a 0.13.3 deployment did not have them. Dropping these from a -#: current database and winding `user_version` back reproduces exactly what an operator who -#: installed 0.14.0 and has not yet run `keel migrate` hands a reader: the OLD `_SCHEMA_STATEMENTS` -#: created none of them, and no migration has run. +#: 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 @@ -1143,21 +1151,46 @@ def test_a_database_without_the_table_is_reported_not_crashed_on() -> None: ) -def _repo_stopped_at_v16(db_path: Path): +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 Repository(conn) + return repo def test_gather_findings_survives_a_database_nobody_has_migrated(tmp_path, valid_config_path): @@ -1166,11 +1199,30 @@ def test_gather_findings_survives_a_database_nobody_has_migrated(tmp_path, valid 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. """ - repo = _repo_stopped_at_v16(tmp_path / "keel.db") - findings = gather_findings(repo, load_config(valid_config_path), [], NOW) + 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 ): @@ -1181,8 +1233,9 @@ def test_an_unmigrated_posture_table_is_not_reported_as_an_unattested_posture( lapsed, the schema simply predates rail 22. Telling an operator to re-attest sends them to fix a rail that is not the problem. """ - repo = _repo_stopped_at_v16(tmp_path / "keel.db") - findings = gather_findings(repo, load_config(valid_config_path), [], NOW) + 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" diff --git a/tests/mcp/test_tools.py b/tests/mcp/test_tools.py index 73b9bdcb..b414302a 100644 --- a/tests/mcp/test_tools.py +++ b/tests/mcp/test_tools.py @@ -193,13 +193,14 @@ def test_trials_chain_errors_are_tail_bounded(tmp_path: Path, monkeypatch: Any) # -- an un-migrated database, across the whole tool surface (#751) -------------------------------- -#: Everything v17-v20 added. Dropped from a current database, this is what a deployment that -#: installed 0.14.0 and has not run `keel migrate` hands the MCP surface. +#: 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", - "venue_cash_postures", - "equity_points", - "audit_events", + "candle_series_feed", # v17 + "venue_cash_postures", # v18 + "equity_points", # v19 + "audit_events", # v20 ) @@ -212,6 +213,13 @@ def test_no_tool_raises_against_a_database_nobody_has_migrated(tmp_path, valid_c 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 @@ -227,17 +235,24 @@ def test_no_tool_raises_against_a_database_nobody_has_migrated(tmp_path, valid_c 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 tools, "no tools built" + 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: - tool.handler({}) - except sqlite3.OperationalError as exc: # the failure this test exists for + document = tool.handler({}) + except Exception as exc: raise AssertionError( - f"MCP tool {tool.name!r} raised against an un-migrated database: {exc}. " - "A reader that does not migrate must report the gap, not propagate it." + 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 - except Exception: - # Anything else is that tool's own argument or environment contract, not schema - # tolerance, and is covered by its own test above. - pass + assert isinstance(document, dict), f"{tool.name} answered {type(document).__name__}"