Skip to content

fix(commands): research's read-only commands stop opening the db read-write - #620

Merged
eaitbrahim merged 1 commit into
mainfrom
fix-610-research-readonly
Aug 29, 2026
Merged

fix(commands): research's read-only commands stop opening the db read-write#620
eaitbrahim merged 1 commit into
mainfrom
fix-610-research-readonly

Conversation

@eaitbrahim

Copy link
Copy Markdown
Contributor

Closes #610.

Premise checked against origin/main

Confirmed: keel research significance --from deployment, factors and independence all reach their data through _open_repo (keel/commands/_common.py), which calls migrate(conn), and migrate commits unconditionally. keel research pooled-review, in the same file, already connects every profile db mode=ro via its own _connect_ro and never migrates. The inconsistency the issue describes is real and unchanged since filing.

The two decisions the issue left open

1. What a read-only command should do against a database that needs migrating. Refuse, before any data is read. A database stamped below this binary's SCHEMA_VERSION is a shape this binary was never tested against -- reading it quietly would be answering a question with evidence of unknown provenance. The issue's own instinct (refusal as operator error, not an evidence refusal -- #601's distinction) is what I implemented: _open_repo_ro raises a ClickException naming the stored version, this binary's version, and the remedy (run a command that writes, e.g. keel migrate, or run a binary that matches its version) rather than silently reading a schema Repository was never built against.

2. How wide to take the seam. The seam (_open_repo_ro in keel/commands/_common.py) plus the three research/ commands, exactly as the issue's own inclination suggested -- nothing in insights, status, pnl, activity was touched. They share the same read-write shape and are explicitly left as a follow-up once this seam has a track record, per the issue's own "doing all of them is a bigger change than this issue needs" framing.

What changed

  • keel/commands/_common.py -- _open_repo_ro(ctx): pooled-review's own mode=ro URI shape, promoted to a shared seam, with two refusals before any data is read: a missing file (mirrors keel/mcp/tools.py::_open_readonly_repo's existence check -- mode=ro alone already refuses to create the file, but the raw sqlite3.OperationalError names neither the file nor the reason) and a stale schema version (decision 1, above).
  • keel/commands/research.py -- research_significance, research_factors, research_independence now call _open_repo_ro instead of _open_repo. pooled-review's own _connect_ro/read_orders/read_ledger are untouched -- they already had the right shape.
  • docs/research-toolkit.md -- the "which of these open your database read-write" hazard section is rewritten to state what's now true (all four research commands that touch orders/trade_outcomes are read-only, the stale-schema refusal message, and the explicit remaining-scope boundary).
  • tests/commands/test_research_commands.py -- three new tests (below).

Tests, mutation-verified (each restored after)

  • test_significance_factors_independence_never_write_to_the_database -- the same shape as the existing test_pooled_review_never_writes_to_the_profile_dbs: hash the db file before/after a real CLI invocation of each of the three commands.
  • test_research_readonly_commands_refuse_a_database_older_than_this_binary -- builds a db migrated to the current version, then rewrites the stamp down by one. Mutation: removing the if current < SCHEMA_VERSION guard ->
    AssertionError: ['research', 'significance', '--from', 'deployment'] should refuse a stale-schema database
    assert 0 != 0
    
  • test_research_readonly_commands_refuse_a_missing_database -- pins the message, not just the exit code, because mode=ro against a missing path already refuses on its own (bare sqlite3.OperationalError). Mutation: removing the explicit existence check ->
    AssertionError: assert 'no database at' in ''
    Result OperationalError('unable to open database file')
    

Results

  • uv run pytest -q: 4570 passed, 3 skipped in 149.30s
  • uv run ruff check keel tests packages: All checks passed!
  • uv run mypy: Success: no issues found in 359 source files

🤖 Generated with Claude Code

https://claude.ai/code/session_01T6yA5khYnJ2qzheArRToQ2

…db read-write (#610)

keel research significance --from deployment, factors and independence
are questions about a deployment's state -- "is this edge distinguishable
from zero", "do these factors correlate" -- but all three reached their
data through _open_repo, which calls migrate(conn), and migrate commits
unconditionally. Asking a question opened the database read-write, and
on a binary newer than the file, migrated its schema as a side effect of
being asked. keel research pooled-review, in the same file, already does
the opposite on the same tables (orders/trade_outcomes): mode=ro, no
migrate. Nothing explained why one command in research.py reads a
deployment database safely and its three siblings do not, and
docs/research-toolkit.md's new page was about to invite operators to
point all four at live ledgers without saying so.

_open_repo_ro (keel/commands/_common.py) is pooled-review's own mode=ro
shape promoted to a shared seam, with two refusals neither predecessor
had to make explicit because pooled-review is only ever pointed at
databases this binary already understands:

- a missing file refuses with a plain message instead of letting
  sqlite3.connect silently create one (mode=ro alone already refuses to
  create it -- OperationalError: unable to open database file -- but
  that is not a message an operator can act on, so the seam checks first
  and says so);
- a database stamped below this binary's SCHEMA_VERSION refuses before
  any data is read, rather than being read under a schema this binary
  was never tested against. That gap is an operator's to close -- run a
  command that writes (keel migrate), or run a binary that matches the
  file -- not a question these commands can answer quietly. #601 already
  drew this line (an operator mistake is not a result the evidence
  produced); this is the same call applied to a new seam.

Scope, the other open decision: the seam plus the three research/
commands, nothing wider. insights, status, pnl and activity share the
same read-write shape and are explicitly left for a follow-up once this
seam has proven itself, rather than widened here past what #610 asked.

docs/research-toolkit.md's hazard section is rewritten from "these three
are inconsistent with pooled-review, fix filed separately" to stating
what is now true, including the refusal message and the remaining scope
boundary.

New tests in tests/commands/test_research_commands.py, mutation-verified
(each restored after):

- test_significance_factors_independence_never_write_to_the_database --
  the pooled-review pin's own shape (hash the db file before and after a
  real CLI run).
- test_research_readonly_commands_refuse_a_database_older_than_this_binary
  -- removing the `if current < SCHEMA_VERSION` guard:
  `AssertionError: ['research', 'significance', '--from', 'deployment']
  should refuse a stale-schema database / assert 0 != 0`
- test_research_readonly_commands_refuse_a_missing_database -- removing
  the explicit existence check (sqlite3's own mode=ro refusal alone is
  not a message naming the file or the reason):
  `AssertionError: assert 'no database at' in ''` against
  `Result OperationalError('unable to open database file')`.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01T6yA5khYnJ2qzheArRToQ2
@eaitbrahim
eaitbrahim merged commit 8bac05c into main Aug 29, 2026
4 checks passed
@eaitbrahim
eaitbrahim deleted the fix-610-research-readonly branch August 29, 2026 22:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Read-only research commands open the deployment database read-write, and migrate it

1 participant