refactor(storage): consolidate table/column/index exists checks into introspection - #3690
Merged
Sinity merged 4 commits intoAug 3, 2026
Merged
Conversation
…dex checks Ref polylogue-48h, polylogue-a7xr.9 Problem: polylogue-48h found ~25 independently maintained _table_exists/_column_exists/_index_exists copies (plus sync/async and schema-qualified variants) scattered across cli/, daemon/, storage/, sources/, insights/, and operations/ -- each trivially small and subtly different. A prior PR (#2912) had already consolidated table_exists/ table_exists_async into polylogue/storage/table_existence.py and redirected most call sites, but column_exists/index_exists were never added and ~25 call sites still carried their own local definition. Solution: rename polylogue/storage/table_existence.py to polylogue/storage/introspection.py (kept at storage/ root, not storage/sqlite/, for the same documented circular-import reason as before: storage/sqlite/__init__.py eagerly imports the heavy async backend stack), and add column_exists/column_exists_async/index_exists/index_exists_async alongside the existing table_exists/table_exists_async. Schema-qualification (the schema= kwarg) is omitted for the default "main" schema so every pre-consolidation bare `PRAGMA table_info(...)`/`sqlite_master` query stays byte-identical -- several call sites redirected in the next commits are covered by mock-based tests that pattern-match exact query text.
…ospection Ref polylogue-48h Mechanical import-path update for the 25 modules that already imported table_exists/table_exists_async from the (now renamed) storage.table_existence module, following the rename in the previous commit. No behavior change.
…exists checks
Ref polylogue-48h
Replace the ~25 remaining independently maintained _table_exists/
_column_exists/_index_exists definitions (and sync/async pairs) with imports
from polylogue.storage.introspection. Several copies checked
`type IN ('table', 'virtual table')` or `type IN ('table', 'shadow')`;
neither value is ever a real sqlite_master `type` (virtual tables, including
FTS5 shadow tables, register with type='table'), so redirecting to the
canonical `type='table'` check is behavior-preserving -- confirmed directly
against a live FTS5 vtable (all 6 shadow tables show type='table').
Genuinely distinct behavior is kept as a thin, differently-named wrapper
delegating to the canonical function rather than folded away:
- usage.py's _table_exists_in_schema and daemon/metrics.py's
_attached_table_exists swallow sqlite3.Error for a schema alias that may
not be ATTACHed yet.
- embeddings/support.py's table_exists_sync_missing_safe/
table_exists_async_missing_safe swallow a still-in-flight
sqlite3.OperationalError ("no such table").
- cli/commands/status.py and storage/archive_readiness.py keep their local
_schema_object_exists/_view_exists (view support isn't part of this
bead's scope) and only redirect _table_exists/_column_exists.
- storage/sqlite/run_projection_relations.py's table_exists_sync had no
callers anywhere in the repo; deleted outright instead of redirected.
Test fixes for query-text-sensitive mocks broken by the consolidation:
- tests/unit/daemon/test_daemon_cli.py: FakeConnection assertions expected
the old `type IN ('table', 'virtual table')` text; updated to the
behaviorally-identical `type='table'` text the canonical function emits.
- tests/unit/storage/test_embedding_contracts.py: _VeclessConnection.execute
dropped bind parameters entirely, which only worked because the old
support.py table_exists_sync interpolated the table name into the SQL
string instead of binding it. The canonical function binds `name` as a
real parameter, so the fake now forwards parameters to the real
sqlite3.Connection.execute -- the fake was silently wrong before, not the
production code.
- tests/unit/cli/commands/test_status.py: mypy --strict's no-implicit-
reexport rule correctly rejects importing _table_exists/_column_exists
through status.py's own re-import; the test now imports table_exists/
column_exists directly from polylogue.storage.introspection, matching the
existing polylogue-ogn1 precedent for _archive_readiness_status.
…e updates Ref polylogue-48h Add devtools/verify_table_exists_duplication.py: a grep/AST-based tripwire wired into `devtools verify --quick` (via the "lab policy" static-check block) that forbids a new top-level def named table_exists/column_exists/ index_exists (or a _-prefixed/_sync/_async variant) outside polylogue/storage/introspection.py -- the same pattern the bead's consolidation removed. Registered as `devtools lab policy table-exists-duplication` in command_catalog.py; docs/devtools.md regenerated via `devtools render devtools-reference`. docs/plans/layering-surface-baseline.json: renamed the 5 existing table_existence baseline entries to introspection, then added 5 new entries for modules (cli/commands/status.py, daemon/embedding_backlog.py, daemon/fts_automerge.py, daemon/fts_startup.py, daemon/fts_status.py) that previously defined their own _table_exists and so were never in the baseline for this import -- now that they import the shared substrate module, `devtools verify layering`'s cli/daemon-may-not-import-storage ratchet correctly requires them to be declared.
|
Warning Review limit reached
Next review available in: 5 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (60)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Sinity
deleted the
feature/storage/consolidate-table-exists-introspection
branch
August 3, 2026 21:03
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.
Summary
Consolidates ~30 independently maintained
_table_exists/_column_exists/_index_existscopies (and sync/async variants) into one canonical module,polylogue/storage/introspection.py, and adds a grep/AST-based lint tripwire todevtools verify --quickthat forbids reintroducing the duplication.Problem
polylogue-48h found the pattern grown to ~25 near-identical
_table_exists-shaped helpers scattered acrosscli/,daemon/,storage/,sources/,insights/, andoperations/, each trivially small and subtly different (aschema=kwarg on some,type IN (...)alternatives that never actually match anything insqlite_masteron others, unparameterized vs. parameterized SQL on others). A prior PR (#2912) had already consolidatedtable_exists/table_exists_asyncintopolylogue/storage/table_existence.pyand redirected ~25 call sites, butcolumn_exists/index_existswere never added, and the remaining ~25 call sites (not covered by that PR) each still carried their own local definition — the bead's own live-count corroboration found_table_existsstill appearing 11x andtable_exists29x across the tree.Solution
polylogue/storage/table_existence.pytopolylogue/storage/introspection.py, keeping it atstorage/root rather thanstorage/sqlite/for the same documented reason as before:storage/sqlite/__init__.pyeagerly imports the heavy async backend stack, so a low-level module importing astorage.sqlitesibling triggers a real circular import.column_exists/column_exists_async/index_exists/index_exists_asyncalongside the existingtable_exists/table_exists_async. Schema-qualification (schema=kwarg) is omitted for the default"main"schema so every pre-consolidation barePRAGMA table_info(...)/sqlite_masterquery stays byte-identical — several call sites are covered by mock-based tests that pattern-match exact query text.type IN ('table', 'virtual table')ortype IN ('table', 'shadow'); neither value is ever a realsqlite_mastertype(virtual tables, including FTS5 shadow tables, register withtype='table'— confirmed directly against a live FTS5 vtable), so redirecting to the canonicaltype='table'check is behavior-preserving.usage.py's_table_exists_in_schemaanddaemon/metrics.py's_attached_table_existsswallowsqlite3.Errorfor a schema alias that may not be ATTACHed yet;embeddings/support.py'stable_exists_sync_missing_safe/table_exists_async_missing_safeswallow a still-in-flightsqlite3.OperationalError;cli/commands/status.pyandstorage/archive_readiness.pykeep their local_schema_object_exists/_view_exists(view support is out of this bead's scope) and only redirect_table_exists/_column_exists.storage/sqlite/run_projection_relations.py'stable_exists_synchad no callers anywhere in the repo; deleted outright.devtools/verify_table_exists_duplication.py, a grep-based tripwire wired intodevtools verify --quick(thelab policystatic-check block) that forbids a new top-leveldefnamedtable_exists/column_exists/index_exists(or a_-prefixed/_sync/_asyncvariant) outside the canonical module. Registered asdevtools lab policy table-exists-duplication.docs/plans/layering-surface-baseline.json: renamed the 5 existingtable_existencebaseline entries and added 5 new entries for modules that previously defined their own_table_exists(so were never in the baseline for this substrate import) and now correctly need one.Alternatives rejected
polylogue/storage/sqlite/introspection.pyas literally named in the bead's original design note — rejected because it would reintroduce the exact circular-import problem the existingtable_existence.pymodule's docstring already documents and was placed atstorage/root specifically to avoid.PRAGMA/sqlite_masterquery unconditionally (matching the pre-existingtable_existence.pybehavior) — rejected after it broke several mock-based tests that pattern-match exact query text; omitting the qualifier for the default"main"schema is behaviorally identical in real SQLite (an unqualifiedsqlite_masterread never merges attached-database schemas) and keeps every existing call site's query text unchanged.Verification
mypy --strict polylogue devtools tests/unit/storage/test_introspection.py tests/unit/cli/commands/test_status.py tests/unit/storage/test_embedding_contracts.py tests/unit/daemon/test_daemon_cli.py→Success: no issues found in 1342 source filesruff check polylogue/ devtools/ tests/→All checks passed!;ruff format --check→ all formatteddevtools lab policy table-exists-duplication→Table/column/index existence-check consolidation intact: no module outside polylogue/storage/introspection.py redefines table_exists/column_exists/index_exists (polylogue-48h).devtools test tests/unit/storage/test_introspection.py→13 passeddevtools testruns across every touched module's associated test files (~1850 tests total across several batches) → all green except two failures independently confirmed pre-existing and unrelated (verified againstorigin/masterbefore this branch's changes):tests/unit/cli/test_status.py::test_archive_facade_route_catalog_covers_public_async_facade(an unrelated MCP route-catalog mismatch) andtests/unit/daemon/test_health_contract.py::TestTierInventoryContract::test_medium_tier_inventory_pinned(an unrelated pinned health-check inventory drift);tests/unit/daemon/test_metrics_endpoint.py::...test_embedding_backlog_and_latest_catchup_statefails inpolylogue/storage/sqlite/archive_tiers/ops_write.py(a file this PR never touches) on an unrelatedstatus IN ('running', 'completed', 'failed', 'interrupted')CHECK constraint /"cancelled"enum mismatch.devtools verify --quick→exit_code: 0(post-rebase onto latestorigin/master)Ref polylogue-48h, polylogue-a7xr.9