Repository navigation
Fix/history prune data loss - #69
Merged
Merged
Conversation
The "Reference engine (Python 1.5.0)" workflow had been failing on every
push since April, for a self-inflicted reason: it set
PYTEST_DISABLE_PLUGIN_AUTOLOAD=1, which unloads pytest-cov, and then
passed --cov to pytest.
pytest: error: unrecognized arguments: --cov=. --cov-report=term-missing
That env var is a workaround for stray plugins in a local developer
environment and has no business on a clean runner. Dropped, along with
coverage: measuring coverage of retired code that will never ship again
is noise.
rust.yml and tests.yml are consolidated into ci.yml, so the Actions tab
reflects what this project is. The reference engine's own 76 tests now
run as a step inside the parity job, where they belong — they exist to
guard the implementation the parity harnesses measure against, so if they
break, every parity result below them is meaningless.
Verified the whole legacy suite runs headless with no PyQt6 installed:
the four tests that touch Qt inject stubs into sys.modules rather than
importing it. Blocked the real PyQt6 with a meta_path finder and got
76/76, so no --ignore flags and no PyQt6 install are needed.
Also refreshed the test counts in README (246 -> 251) and pointed the
code checklist audit at the files that exist now rather than at
tests.yml, pyinstaller.yml and AutoTidy.spec, all removed in the rewrite.
Three fixes from the first CI run of the consolidated workflow.
Clippy failed on both platforms. My local toolchain was 1.93 and the
runner is 1.97, which added `manual_sort_by`:
error: consider using `sort_by_key`
--> crates/autotidy-core/src/undo.rs:93
Updated the local toolchain to reproduce it rather than patching blind,
which surfaced a third lint the run had not reached: `byte_char_slices`
in src-tauri. That one was never going to be caught, because the Engine
job lints only the two engine crates and clippy lints are not emitted by
rustc — so `-D warnings` on the app's build step does not cover them. The
app job now runs clippy too. It stays out of the Linux Engine job because
the Tauri crate needs GTK/WebKit libraries that runner does not have.
The legacy suite no longer runs in CI. It covers retired 1.5.0 modules —
the PyQt dialogs, config_manager, startup_manager — that nothing imports
and that will never ship, and one of its tests asserts on a path shape
that breaks wherever the username exceeds eight characters:
'C:\Users\RUNNER~1\...\monitor_me' not found in
['C:\Users\runneradmin\...\monitor_me']
A red X for an 8.3 filename quirk in a test for a dead dialog is noise.
The part of the reference engine that matters is utils.py and
constants.py, which both parity harnesses import directly and exercise
across 23 rule variants — if those break, parity breaks. The suite stays
runnable locally and is documented in legacy/README.md.
`Engine (ubuntu-latest)` failed on one test while Windows passed:
scan::tests::destination_outside_the_tree_needs_no_guard
The fixture asked whether `D:/elsewhere/{YYYY}` needs a guard. On Windows
that is an absolute path outside the monitored tree, so the answer is no.
On Unix there are no drive letters: `D:/elsewhere` is a *relative* path
naming a directory called `D:`, so `guard_paths` correctly resolved it
inside the monitored folder and produced a guard.
The engine was right both times; the test was Windows-only in disguise
and silently asserted the opposite of its name on Linux. Fixtures now go
through `monitored()` and `elsewhere()` helpers that spell an absolute
path for the host platform.
This is what the ubuntu leg of the matrix is for. Swept the engine crates
for other hardcoded drive letters and found none; the remaining ones are
in src-tauri, which is Windows-only by nature and only ever built there.
Launching AutoTidy ran a 90-day prune before the user had asked it to do anything, and the prune had no floor and kept no copy. A history where every record predates the window was therefore reduced to zero bytes on startup. This is not hypothetical. It happened here to a real 776-record file, every entry 9+ months old. There was no backup and nothing to recover from. That file is the only reason undo works, so the case where age-based deletion looks most justified is the case where it does the most damage. Two changes: * A floor. The most recent MIN_RETAINED_RECORDS (500) survive whatever their age, so coming back to the app after a long break can no longer cost you every record you had. * A backup. Any prune that would discard something copies the file to `autotidy_history.jsonl.pruned.bak` first, written atomically. A prune that discards nothing does not touch the file at all, and takes no backup — so the copy that survives is from the last prune that actually removed something, not an already-pruned one. `prune` now returns a PruneOutcome (removed / kept / backup) instead of `()`, and the shell logs at WARN when anything was discarded. Silently deleting a user's records was the underlying problem; reporting it is part of the fix. Verified end to end by rebuilding the incident — 776 records, all 280 days old, launched against the real app: before 0 records survived, no backup after 500 records survived, 296 KB backup holding all 776 prune_keeps_unparseable_lines was rewritten rather than deleted: its intent (a line we cannot decode is never what we throw away) still holds, but it had also encoded "the old record is dropped", which the floor now prevents. It seeds past the floor so the prune genuinely discards, with the corrupt lines in the region that would have gone. 255 tests.
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
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.
Description
Please include a summary of the change and which issue is fixed. Please also include relevant motivation and context. List any dependencies that are required for this change.
Fixes # (issue)
Type of change
Please delete options that are not relevant.
How Has This Been Tested?
Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration
Test Configuration:
Checklist: