test: add reindex derived-model differential - #3726
Merged
Merged
Conversation
Problem: incremental convergence and full inactive-generation rebuilds lacked a common derived-model oracle. What changed: add a DDL-censused, volatility-aware snapshot helper and a real temporary-archive survivor covering two full rebuilds, incremental convergence, public reads, FTS, materialization markers, debt, and four mutation controls. Ref polylogue-5wp. Ref polylogue-hjwr. Co-Authored-By: Codex <noreply@openai.com>
📝 WalkthroughWalkthroughAdds a read-only differential snapshot framework for archive-derived models and an end-to-end test that compares incremental convergence with full reindex generations, including FTS, public reads, markers, and convergence debt. ChangesDerived model differential verification
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Archive
participant ReplayAndRepair
participant IndexGeneration
participant SnapshotFramework
participant DifferentialAssertions
Archive->>ReplayAndRepair: provide source evidence
ReplayAndRepair->>IndexGeneration: replay and repair derived state
SnapshotFramework->>IndexGeneration: read tables, FTS, markers, and debt
SnapshotFramework->>ReplayAndRepair: read public insights and searches
SnapshotFramework-->>DifferentialAssertions: return normalized snapshots
DifferentialAssertions->>DifferentialAssertions: compare generations and readiness
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/infra/reindex_differential.py`:
- Around line 26-29: Update the census parsing around _CREATE_TABLE and
_CREATE_VIRTUAL_TABLE so every CREATE TABLE declaration is either classified or
explicitly rejected. Support valid declarations without IF NOT EXISTS and quoted
object names, preferably by reading names from a temporary SQLite catalog;
otherwise detect unparsed CREATE TABLE statements and fail instead of omitting
them from the census and missing/stale checks.
- Around line 185-192: Update assert_derived_model_ready to reject any FTS
ledger whose state is not ready and any nonzero missing_rows, excess_rows, or
duplicate_rows, in addition to the existing source/indexed row-count check. Use
the corresponding fields on snapshot.fts and preserve the current AssertionError
behavior and messages where applicable.
In `@tests/unit/storage/test_reindex_derived_model_differential.py`:
- Around line 149-151: Update _assert_control_rejected and its related cases so
the control differs only in public_reads while tables, FTS, markers, and debt
remain equal. Ensure the assertion reaches public-read comparison and verifies
the expected “public insight reads differ” failure, rather than passing due to
an earlier table mismatch.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: fe962aff-f8db-4b0b-a0e2-3e24e998d437
📒 Files selected for processing (2)
tests/infra/reindex_differential.pytests/unit/storage/test_reindex_derived_model_differential.py
Comment on lines
+26
to
+29
| _CREATE_TABLE = re.compile(r"CREATE\s+TABLE\s+IF\s+NOT\s+EXISTS\s+([A-Za-z_][A-Za-z0-9_]*)", re.IGNORECASE) | ||
| _CREATE_VIRTUAL_TABLE = re.compile( | ||
| r"CREATE\s+VIRTUAL\s+TABLE\s+IF\s+NOT\s+EXISTS\s+([A-Za-z_][A-Za-z0-9_]*)", re.IGNORECASE | ||
| ) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Reject unparsed DDL in the census.
_CREATE_TABLE and _CREATE_VIRTUAL_TABLE accept only unquoted names with IF NOT EXISTS. A valid table declaration that omits the clause or quotes the name is absent from both census sets. The missing and stale checks then pass without requiring a classification.
Read object names from a temporary SQLite catalog, or reject every CREATE TABLE statement that the parser cannot classify.
Also applies to: 111-123
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/infra/reindex_differential.py` around lines 26 - 29, Update the census
parsing around _CREATE_TABLE and _CREATE_VIRTUAL_TABLE so every CREATE TABLE
declaration is either classified or explicitly rejected. Support valid
declarations without IF NOT EXISTS and quoted object names, preferably by
reading names from a temporary SQLite catalog; otherwise detect unparsed CREATE
TABLE statements and fail instead of omitting them from the census and
missing/stale checks.
Comment on lines
+185
to
+192
| def assert_derived_model_ready(snapshot: DerivedModelSnapshot) -> None: | ||
| """Keep a matching but jointly stale generation from passing the lane.""" | ||
| if snapshot.fts.source_rows != snapshot.fts.indexed_rows: | ||
| raise AssertionError( | ||
| f"FTS is not ready: source_rows={snapshot.fts.source_rows}, indexed_rows={snapshot.fts.indexed_rows}" | ||
| ) | ||
| if snapshot.open_debt: | ||
| raise AssertionError(f"convergence debt remains: {snapshot.open_debt}") |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Check all FTS freshness fields.
Equal row counts do not prove a correct FTS index. One missing source block plus one duplicate or excess FTS row keeps the totals equal. The ledger already includes state, missing_rows, excess_rows, and duplicate_rows, but this readiness check ignores them.
Reject a non-ready ledger state and any nonzero discrepancy count.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/infra/reindex_differential.py` around lines 185 - 192, Update
assert_derived_model_ready to reject any FTS ledger whose state is not ready and
any nonzero missing_rows, excess_rows, or duplicate_rows, in addition to the
existing source/indexed row-count check. Use the corresponding fields on
snapshot.fts and preserve the current AssertionError behavior and messages where
applicable.
Comment on lines
+149
to
+151
| def _assert_control_rejected(expected: DerivedModelSnapshot, root: Path, index_path: Path) -> None: | ||
| with pytest.raises(AssertionError): | ||
| assert_derived_models_equivalent(expected, _snapshot(root, index_path)) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Isolate the public_reads control.
The session_profiles update changes a compared table. assert_derived_models_equivalent() raises during the table comparison before it reaches public_reads. _assert_control_rejected() therefore succeeds even if public-read collection or comparison is removed.
Add a control with equal tables, FTS, markers, and debt but a different public read. Alternatively, assert the expected public insight reads differ failure.
Also applies to: 236-243
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/unit/storage/test_reindex_derived_model_differential.py` around lines
149 - 151, Update _assert_control_rejected and its related cases so the control
differs only in public_reads while tables, FTS, markers, and debt remain equal.
Ensure the assertion reaches public-read comparison and verifies the expected
“public insight reads differ” failure, rather than passing due to an earlier
table mismatch.
This was referenced Aug 7, 2026
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
Adds a test-only, DDL-censused oracle that compares incremental convergence with two independently rebuilt inactive index generations.
Problem
Rebuildable index state had no real-route assertion that incremental convergence and repeat full rebuilds produce the same logical derived model.
Solution
The new test infrastructure classifies every ordinary current-DDL table, declares volatile columns explicitly, compares logical rows and public insight reads, and separately checks FTS readiness, materialization markers, and unresolved convergence debt. The survivor seeds only a temporary archive through production raw replay and repair routes, then proves four corruptions are rejected.
Acceptance matrix
session_profiles, stale FTS text, and a same-row-count stale profile.Verification
direnv exec . devtools test tests/unit/storage/test_reindex_derived_model_differential.py: 1 passed in 1.40s.direnv exec . devtools verify --quick: success, including format, lint, strict mypy, generated-surface checks, and policy checks.Ref polylogue-5wp. Ref polylogue-hjwr.
Summary by CodeRabbit