|
| 1 | +# Review — history-v2-and-preferences (round 01) |
| 2 | + |
| 3 | +Branch: `feat/history-v2-and-preferences` @ `e1891de` |
| 4 | +Reviewed against: `docs/plans/2026-08-01_history-v2-and-preferences.md` |
| 5 | + |
| 6 | +## Verdict |
| 7 | + |
| 8 | +CHANGES_REQUESTED — **one required change, and it is a test-isolation defect you already |
| 9 | +worked around rather than fixed.** The implementation itself is correct and I found no |
| 10 | +functional defects. |
| 11 | + |
| 12 | +## What you did well |
| 13 | + |
| 14 | +- **Fourteen atomic commits in plan order**, with the entire storage layer landing Qt-free |
| 15 | + before the dock existed. |
| 16 | +- **The storage tests map 1:1 to the plan's requirements** — versioned ids, migration in order |
| 17 | + and only once, existing-v2 untouched, failed migration leaving the original intact, malformed |
| 18 | + isolation, unparseable file not deleted, missing-key skipped, `get_by_id`, cap evicting |
| 19 | + oldest, and a **parametrized** Streamlit-shape test covering both migrated and fresh files. |
| 20 | + That last one is the compatibility guarantee and you tested both paths, which is what the |
| 21 | + plan asked for and easy to half-do. |
| 22 | +- **Migration is tested at realistic scale** — 100 records, order preserved, idempotency |
| 23 | + asserted. A single-record test would have proven nothing about ordering or the cap. |
| 24 | +- **The dock selects by stable id**, carrying `record["id"]` in `Qt.ItemDataRole.UserRole` and |
| 25 | + resolving through `get_by_id`. This is the exit criterion and the design is right. |
| 26 | +- **`app.py` is untouched** and the Streamlit path diff is empty. The compatibility constraint |
| 27 | + held. |
| 28 | +- **The session log is the best this project has produced.** Both baselines, both final |
| 29 | + tallies, mutation node ids, and — importantly — *"`closeEvent` was not changed, so V10 was |
| 30 | + not applicable"* rather than claiming an unmeasured result. **I verified that claim and it is |
| 31 | + accurate**: `closeEvent` appears in the diff only as unchanged context. That is exactly the |
| 32 | + recording discipline the plan asked for. |
| 33 | + |
| 34 | +### My measurements |
| 35 | + |
| 36 | +| check | result | |
| 37 | +|---|---| |
| 38 | +| suite on **3.14** | 333 passed, 1 skipped | |
| 39 | +| suite on **3.12** | 333 passed, 1 skipped — identical | |
| 40 | +| `run-quality-gates` | pass | |
| 41 | +| **V10** crash gate 25 + 25 | **0 native crashes / 50** | |
| 42 | +| **V9** 3.14-only syntax | none | |
| 43 | +| V2 Streamlit path diff | empty | |
| 44 | +| V8 mutation 1 (select by list index) | FAILED `test_history_dock_selects_duplicate_labels_by_stable_id` | |
| 45 | +| V8 mutation 3 (malformed discards all) | FAILED `test_malformed_records_are_isolated_from_valid_history`, `test_record_missing_required_key_is_skipped` | |
| 46 | + |
| 47 | +I ran V10 despite it being "not applicable" — a new dock is new QObject lifetime, and the |
| 48 | +result is clean at 0/50. You do not need to re-run it. |
| 49 | + |
| 50 | +## Required change |
| 51 | + |
| 52 | +### F1. 37 tests now read the user's real history file |
| 53 | + |
| 54 | +`HistoryDock.__init__` ends with `self.refresh()`, which reads from disk, and `MainWindow` |
| 55 | +constructs the dock unconditionally with `history_manager or HistoryManager()` — |
| 56 | +whose `DEFAULT_PATH` is `~/.wherewolf/history.json`. |
| 57 | + |
| 58 | +**37 tests construct `MainWindow` without injecting a history manager.** Every one of them now |
| 59 | +reads the developer's real history file. This is **new in this phase**: I checked `dev`, which |
| 60 | +had zero reads at construction. |
| 61 | + |
| 62 | +You already hit this. Your own session log records it: |
| 63 | + |
| 64 | +> The initial unmodified 3.14 attempt without that isolated home failed with **six test |
| 65 | +> failures and one Qt teardown error** because `/home/beallio/.wherewolf` is read-only in this |
| 66 | +> sandbox. |
| 67 | +
|
| 68 | +You solved it by pointing `HOME` at a writable temporary directory. That got the round done, |
| 69 | +but it treated the symptom: the suite's correctness now depends on ambient user state. It |
| 70 | +passes on this machine because my `~/.wherewolf/history.json` happens to be small and |
| 71 | +well-formed. On a machine whose history is large, or contains one of the malformed records |
| 72 | +this very phase teaches the reader to expect, tests that never intended to touch it would fail |
| 73 | +in ways that look like real regressions. |
| 74 | + |
| 75 | +**This matters more now, not less.** `ORCH_ADD_DIRS` has since been granted |
| 76 | +`~/.wherewolf` and `~/.config/Wherewolf` so the sandbox no longer blocks writes. The sandbox |
| 77 | +was accidentally acting as a safety net. A future test that triggers a destructive path on a |
| 78 | +default-constructed `MainWindow` can now write to the real file. Your Clear History test |
| 79 | +correctly injects a `tmp_path` manager — keep that discipline, but do not rely on every future |
| 80 | +test remembering. |
| 81 | + |
| 82 | +**Fix:** an autouse fixture in `tests/conftest.py` that redirects the history default path (and |
| 83 | +`QSettings` storage, which has the same exposure through `SettingsService`) to a per-test |
| 84 | +temporary location. Then assert it: a test proving that constructing a bare `MainWindow()` does |
| 85 | +**not** touch `~/.wherewolf`. Given this phase's entire purpose is not mishandling user history, |
| 86 | +that guarantee belongs in the suite. |
| 87 | + |
| 88 | +## Already handled — no action needed |
| 89 | + |
| 90 | +`.codex/config.toml`, a machine-specific codex MCP config, was left untracked in the repo root |
| 91 | +and was breaking the `git status --short` must-print-nothing check that every round's |
| 92 | +verification depends on. I added `.codex/` to `.gitignore` myself (`cef3342`) since it is |
| 93 | +tooling hygiene rather than phase work. The tree is clean. |
| 94 | + |
| 95 | +## Verification before marking complete |
| 96 | + |
| 97 | +- The autouse isolation fixture, plus the test proving a bare `MainWindow()` does not touch |
| 98 | + `~/.wherewolf`. |
| 99 | +- `./run.sh uv run pytest -q` on 3.14 and `--python 3.12` — record both. |
| 100 | + **Remember:** `uv run --python 3.12` re-syncs the shared venv; restore with |
| 101 | + `./run.sh uv sync --all-extras --dev --python 3.14`. |
| 102 | +- `scripts/orchestration/run-quality-gates` → exit 0. |
| 103 | +- `git status --short` → prints nothing. |
| 104 | + |
| 105 | +**Do not re-run V10** (0/50 measured above) or the V8 mutations unless you change source. |
| 106 | + |
| 107 | +## Constraints |
| 108 | + |
| 109 | +Do not remove `timid = true`. Do not disable coverage. Do not skip, delete or xfail tests. Do |
| 110 | +not modify `app.py` or the Streamlit path beyond `storage/history.py`. Do not regress the |
| 111 | +atomic write. Do not touch `main`. Do not bump the package version. |
| 112 | + |
| 113 | +## Deferred — correctly recorded by you |
| 114 | + |
| 115 | +No human has seen the dock or a restored layout; all Qt tests are offscreen. No migration has |
| 116 | +been run against a real user's history file. No performance measurement. macOS and Windows |
| 117 | +unverified — `QSettings` backends differ per platform and this is Linux-only verification. |
| 118 | +Export is Phase 12, Spark Phase 13, Streamlit removal Phase 14. |
| 119 | + |
| 120 | +STATUS: CHANGES_REQUESTED |
0 commit comments