fix(mcp): a reader that does not migrate must report the gap, not raise on it (#751) - #752
Merged
Merged
Conversation
…se 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) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KZZxmspQXe5qJ9FAsG13s6
…erage 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) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01KZZxmspQXe5qJ9FAsG13s6
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #751.
keel mcp'sdoctortool raisedsqlite3.OperationalError: no such table: venue_cash_posturesagainst 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_findingsstates 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 atable_presentcheck and an OK finding namingkeel migrate. Three migrations later,venue_cash_postures(v18) andequity_points(v19) had no such guard.The check moves from
audit.pytodb.table_present, where the schema is defined and every reader can reach it, plusRepository.table_presentfor the reasonaudit_chainis exposed there: what this record contains is a question about the record.Checked, not caught.
OperationalErrorcovers "no such table" and "database disk image is malformed" under one clause, and that distinction is the difference betweenkeel migrateand stop trading.An absent table is not an unattested posture
A bare
Nonewould 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_findingsays the true thing instead, and OK rather than WARN — the engine that enforces rail 22 migrates on the way in, so a live cycle 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_onasserts 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
doctoralone — a tool added later inherits it, a table added later joins one tuple. Verified by mutation: forcingtable_presenttoTruefails all three.And the docstring that caused it
audit_chain_findingssaid "both the web server andkeel mcpopen a repo WITHOUT migrating". Only the per-request open skips it;web/server.servecallsensure_schemaonce at bind, so a served database is never behind. That 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. Both are corrected.Checks
6,308 passed / 3 skipped (6,305 + the three added here), ruff and mypy clean.
Not a blocker for v0.14.0
Present on
mainbefore the bump; the release notes already name it and tell operators to runkeel migratebefore pointing an MCP client at an upgraded deployment.🤖 Generated with Claude Code
https://claude.ai/code/session_01KZZxmspQXe5qJ9FAsG13s6