Two review comments on this PR, both addressed:
- gemini-code-assist flagged that maybe_autoheal_fts5_index's default
progress=print goes straight to stdout. _validate_palace_fts5_after_mine
runs inside the MCP server process too (mcp_server.tool_mine ->
miner.mine), where stdout is the JSON-RPC transport -- a stray print()
there would corrupt the protocol stream and crash the connection. Pass
progress=logger.info instead; palace.py already has the module logger.
- nikkunikku corroborated the fix from a real 1.4GB production palace (278
repeated abort-loop iterations before the fix) and pointed out a real
test gap: the fixture-based auto-heal tests fabricate real FTS5
corruption via direct shadow-table writes, which some SQLite builds
refuse (existing pytest.skip paths in test_miner_fts5_validation.py,
related to #1925) -- so on those builds the auto-heal wiring in
_validate_palace_fts5_after_mine is never actually exercised. Added
their suggested build-independent tests, adapted to this file's fixture
helpers: test_validator_suppresses_raise_when_autoheal_clears and
test_validator_still_raises_when_autoheal_cannot_clear, stubbing
mempalace.repair.sqlite_integrity_errors/maybe_autoheal_fts5_index
directly instead of fabricating corruption.
Added a third test, test_validator_passes_logger_progress_not_print_to_autoheal,
covering the specific progress= wiring: the two tests above mock
maybe_autoheal_fts5_index entirely and discard its kwargs, so neither would
have caught the progress=print regression this commit actually fixes. The
new test captures the real kwargs and asserts progress is a bound method of
palace.py's own logger (not print), without pinning to logger.info
specifically -- severity level is a verbosity choice, not a correctness
requirement, so the assertion shouldn't fail on a reasonable future change
to e.g. logger.debug. Verified both directions: fails against the
pre-fix `print` default (and shows the leaked stdout line to prove it),
passes at .info and at .debug alike.
Full suite: 3221 passed, 20 skipped (unchanged skip count). ruff
check/format clean.
_errors_are_isolated_fts5 gated auto-heal on one specific message shape:
malformed inverted index for FTS5 table
SQLite >= ~3.5x (confirmed on 3.53.2 / Python 3.13.7) reports the same
isolated-FTS5 condition with different wording instead:
fts5: corruption found reading blob N from table "embedding_fulltext_search"
The narrow regex never matched this phrasing, so maybe_autoheal_fts5_index
silently declined to heal on any machine running a recent-enough SQLite,
falling straight through to the hard-abort path -- the exact condition
the whole auto-heal feature (#1926/#1928) exists to avoid. Widened the
pattern to match either wording.
Caught by running this repo's own test suite on this machine:
test_repair.py's two auto-heal tests were failing (not, as assumed
earlier, pre-existing/unrelated flakiness -- that assumption was never
actually verified). Traced to this exact classification gap.
Fixing this correctly also exposed that four tests in
test_miner_fts5_validation.py had been passing for the wrong reason: they
manufacture the exact "reporter-shaped" isolated-FTS5 corruption (#1926's
actual bug shape) and asserted mine() must raise MineValidationError for
it -- true only because the classifier bug prevented auto-heal from ever
engaging. With the classifier fixed, that corruption is now correctly
auto-healed and mine() succeeds instead, so those tests' expectations
were stale, not their fixtures being invalid:
- test_helper_raises_on_fts5_segment_corruption -> renamed
test_helper_auto_heals_fts5_segment_corruption; asserts no raise + a
clean post-heal quick_check, instead of expecting a raise.
- test_full_chain_raises_through_mine_impl and
test_mine_impl_does_not_print_partial_summary_on_validation_error: their
real purpose is exception-passthrough / banner-suppression when the
validator DOES raise, not proving any particular corruption triggers it.
Switched from real file corruption to a monkeypatched raise. (Tried
swapping to _page_mangle's non-isolated corruption first -- that made
ChromaDB's own Rust bindings panic just opening the file for the
re-mine's get_collection() call, a native crash rather than a catchable
Python exception, before the validator ever ran. Different failure mode
than what these tests are about, and not reliable to depend on.)
- test_mine_formats_full_chain_raises_when_fts5_corrupt: same fix, mirrors
the miner-path change for the extract path.
- Added test_full_chain_auto_heals_isolated_fts5_corruption and
test_mine_formats_full_chain_auto_heals_isolated_fts5_corruption as
companions, proving the full mine()/mine_formats() chain -- not just
the standalone validator -- actually auto-heals and succeeds end-to-end
for the isolated case now that it's correctly classified.
- test_errors_are_isolated_fts5_classification: added the new message
wording as an explicit regression fixture (pinned literally, not
dependent on whatever this machine's SQLite happens to emit).
Full suite: 3302 passed, 20 skipped, 0 failed -- first fully clean run
this session. ruff check / ruff format -- clean.
Wires _validate_palace_fts5_after_mine into all three mine entry
points so corrupted-FTS5 palaces cannot silently exit 0 from any
of them:
- _mine_impl (mempalace/miner.py) — project file miner
- mine_convos (mempalace/convo_miner.py) — conversation exports
- mine_formats (mempalace/format_miner.py) — binary office documents
via --mode extract, introduced on develop by #1555 (3.3.6 release)
between this PR's open date and its rebase
cmd_mine surfaces MineValidationError as exit 1 + the same
print_sqlite_integrity_abort banner cmd_repair already prints,
appended with a mine-specific stderr note that hedges attribution
(quick_check cannot tell pre-existing corruption from corruption
this mine produced). 17 tests in tests/test_miner_fts5_validation.py
cover the helper, the three call sites, dry-run / KeyboardInterrupt
skip semantics, and the MineValidationError constructor invariants.
Closes#1537.
Co-authored-by: Caleb Wells <15988028+calebcwells@users.noreply.github.com>
J.R. Murray
Igor Lins e Silva
mvalentsev