Purge legacy SQLite / state.db / DVC compat residue (follow-up to #455) - #458
Open
sjawhar wants to merge 14 commits into
Open
Purge legacy SQLite / state.db / DVC compat residue (follow-up to #455)#458sjawhar wants to merge 14 commits into
sjawhar wants to merge 14 commits into
Conversation
StateDB has always used LMDB (state.lmdb/ directory), but callers passed a "state.db" path that was never created. This caused a bug in explain.py where `if state_db_path.exists()` always returned False, skipping the O(1) generation-based skip detection and falling through to expensive per-file rehashing — making `pivot status`, `pivot repro -n`, and `pivot repro -n -e` extremely slow. Changes: - StateDB.__init__ now takes state_dir directly instead of deriving it from a phantom db_path.parent - Remove get_state_db_path() — callers use get_state_dir() directly - Remove existence guard in explain.py so generation skip always runs - Remove state.db from .gitignore template - Update docs to reference state.lmdb/ instead of state.db Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
DVC-export bridge was a one-way migration helper. Deleting it removes 389+42 lines of production code, 465+248 lines of tests, 2 unused exception classes, and 1 ~400-line vestigial DVC-import path in dvc_compat.py that duplicated dvc_import.py. BREAKING: pivot export command removed. Use dvc_import.py / pivot import-dvc for the opposite direction if needed.
The parameter was accepted but ignored. No caller passes it.
Lock files from the pre-LMDB SQLite schema contained a dep_generations field that lives only in StateDB post-migration. Rejecting them at parse time makes callers treat them as 'no previous run' and regenerate cleanly rather than silently proceeding with partial data.
StageLock.is_changed was a 3-line wrapper around is_changed_with_lock_data used only by tests. Inlined as a test-only helper in test_lock.py. Both functions had out_paths: list[str] | None = None defaults that silently skipped output-path validation when None — the same failure mode as the state.db guard. out_paths is now required.
Split from the fixture deletion (next commit) so test collection stays green at every intermediate revision: decorators referencing a missing fixture would cause collection errors, so decorator removal must strictly precede fixture deletion.
3 tasks
There was a problem hiding this comment.
Pull request overview
Follow-up cleanup to the LMDB migration work in #455, removing remaining legacy state.db / backcompat shims and eliminating the deprecated DVC export/compat surface.
Changes:
- Standardize StateDB usage on a state directory (
state_dir) and purgestate.db-path references across core code + tests. - Tighten/clean lock-file handling (reject stale
dep_generations, requireout_paths, inline removed wrapper into tests). - Remove legacy/backcompat surfaces:
pivot export+dvc_compat,clean_registryno-op fixture usage, and related docs.
Reviewed changes
Copilot reviewed 64 out of 64 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| packages/pivot/src/pivot/storage/state.py | StateDB now takes state_dir and uses state_dir/state.lmdb. |
| packages/pivot/src/pivot/storage/lock.py | Reject dep_generations, remove StageLock.is_changed, require out_paths. |
| packages/pivot/src/pivot/explain.py | Remove backcompat re-exports; gate generation-skip on existing state.lmdb. |
| packages/pivot/src/pivot/fingerprint.py | Switch to get_state_dir() for StateDB access. |
| packages/pivot/src/pivot/status.py | Switch to get_state_dir() for StateDB access. |
| packages/pivot/src/pivot/executor/worker.py | Use state_dir directly when opening StateDB. |
| packages/pivot/src/pivot/executor/core.py | Use get_state_dir() when opening StateDB. |
| packages/pivot/src/pivot/executor/commit.py | Use stage state_dir directly when opening StateDB. |
| packages/pivot/src/pivot/engine/engine.py | Key StateDB caches by stage_state_dir and open StateDB with directories. |
| packages/pivot/src/pivot/exceptions.py | Remove DVC export exceptions; keep DVCImportError as PivotError. |
| packages/pivot/src/pivot/config/io.py | Remove get_state_db_path(). |
| packages/pivot/src/pivot/config/init.py | Stop re-exporting get_state_db_path. |
| packages/pivot/src/pivot/cli/init.py | Remove export command registration. |
| packages/pivot/src/pivot/cli/export.py | Delete pivot export command implementation. |
| packages/pivot/src/pivot/dvc_compat.py | Delete DVC compat/export module. |
| packages/pivot/src/pivot/cli/init.py | Remove state.db from init gitignore template. |
| packages/pivot/src/pivot/cli/history.py | Use get_state_dir() when opening StateDB. |
| packages/pivot/src/pivot/cli/remote.py | Use get_state_dir() when opening StateDB. |
| packages/pivot/src/pivot/cli/fingerprint.py | Use get_state_dir() when opening StateDB. |
| packages/pivot/src/pivot/pipeline/pipeline.py | Docs/strings updated from state.db to LMDB/state-dir wording. |
| packages/pivot/src/pivot/storage/AGENTS.md | Update guidance: stale dep_generations locks are rejected. |
| packages/pivot/src/pivot/executor/AGENTS.md | Update worker path-derivation docs to state_dir. |
| packages/pivot/tests/storage/test_lock.py | Replace removed StageLock.is_changed with test helper; update out_paths handling; assert stale locks rejected. |
| packages/pivot/tests/core/test_explain.py | Update diff_* calls to use skip.*; add no-side-effects test for explain fallback. |
| packages/pivot/tests/conftest.py | Remove no-op clean_registry fixture. |
| packages/pivot-tui/tests/conftest.py | Remove no-op clean_registry fixture. |
| packages/pivot/tests/engine/test_graph.py | Remove @usefixtures("clean_registry") decorators. |
| packages/pivot/tests/compat/test_dvc_compat.py | Delete tests for removed DVC compat/export module. |
| packages/pivot/tests/cli/test_cli_export.py | Delete tests for removed pivot export command. |
| packages/pivot/tests/cli/test_cli.py | Remove pivot export CLI behavior test. |
| packages/pivot/tests/cli/test_cli_init.py | Remove expectation that init gitignore includes state.db. |
| packages/pivot/tests/** (multiple) | Update StateDB construction to pass state directories, not state.db paths. |
| packages/pivot-tui/src/pivot_tui/widgets/panels.py | Remove deprecated/ignored stage_data_provider parameter. |
| docs/gen_ref_pages.py | Remove pivot.dvc_compat from reference docs generation. |
| docs/cli/index.md | Remove documentation for pivot export. |
| docs/migrating-from-dvc.md | Remove pivot export migration/validation instructions. |
| docs/comparison.md | Remove DVC export-based migration comparison section. |
| docs/getting-started/installation.md | Remove pivot export-driven DVC install guidance. |
| docs/getting-started/quickstart.md | Update gitignore snippet to state.lmdb/. |
| docs/concepts/pipelines.md | Update state tree docs from state.db to state.lmdb/. |
| docs/concepts/caching.md | Update caching docs from state.db to state.lmdb/. |
| docs/architecture/overview.md | Update architecture tree docs from state.db to state.lmdb/. |
| docs/superpowers/plans/2026-04-18-burn-legacy-state-db.md | Add implementation plan document for this cleanup. |
Comments suppressed due to low confidence (1)
packages/pivot/src/pivot/storage/state.py:180
- StateDB now assumes the argument is a directory. If a caller accidentally passes the legacy file-like path (e.g.
.pivot/state.db), the current logic willmkdir()a directory namedstate.dband store LMDB understate.db/state.lmdb, which is a silent and confusing failure mode. Consider validatingstate_dirup front (e.g., rejectstate_dir.name == "state.db"/state_dir.suffix == ".db", and/or raise ifstate_dir.exists()but is not a directory) so misuse fails loudly with a clear message.
|
|
||
| def _state_db_exists(state_dir: pathlib.Path) -> bool: | ||
| """Return True when the LMDB directory already exists.""" | ||
| return (state_dir / "state.lmdb").exists() |
There was a problem hiding this comment.
_state_db_exists()’s docstring says it checks whether the LMDB directory exists, but it currently uses .exists(). If a file named state.lmdb is present (corrupt/partial state), this returns True and StateDB(...) will likely raise. Consider using .is_dir() (and optionally guarding against non-dir paths) to keep the predicate consistent with its intent and avoid surprising crashes in explain when state is malformed.
Suggested change
| return (state_dir / "state.lmdb").exists() | |
| return (state_dir / "state.lmdb").is_dir() |
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
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
Follow-up cleanup stacked on top of #455. After #455 removed the phantom
state.dbsentinel path, an audit of the rest of the tree found the same failure patterns (deprecated-but-kept code, thin wrappers, optional-None defaults hiding real checks, re-exports "for backward compatibility", no-op fixtures, migration-flavored DVC bridge). This PR burns all of them.Depends on #455. The bottom 4 commits of this PR are identical to #455 — once that merges, they'll drop cleanly and only the 10 cleanup commits will remain in the GitHub diff view.
What's deleted
Breaking change
`pivot export` CLI command is removed. Pre-alpha project (`AGENTS.md`: "breaking changes acceptable, no migration code or compatibility shims needed"). DVC compatibility moves in one direction only — import via `pivot import-dvc` — and exports had zero real users.
Stats
26 files changed, +1169 / −1455 (−286 lines net, or ≈−1363 excluding the plan doc).
Quality gate
Review
Each commit had per-task spec + quality reviews. Final whole-stack review by kieran-python-reviewer: Approved with Notes (2 notes addressed inline: expanded commit bodies on `zylkkkql` and `ktxypkwn` to document the "why" and the ordering invariant).
Plan document
The first commit includes `docs/superpowers/plans/2026-04-18-burn-legacy-state-db.md` — the full plan this PR implements, including pre-conditions, per-task TDD steps, self-review, and acceptance criteria. Happy to drop it from history if undesired, but it was useful as single source of truth during execution and may help future readers understand what "legacy" meant in 04/2026.
If #455 conflicts or is redesigned
`jj rebase -r cleanup/purge-legacy-state-db -d ` and force-push. Happy to split into multiple smaller PRs if the `feat!:` breaking change deserves a separate landing — just let me know.