|
| 1 | +# Review — path-based-export (round 01) |
| 2 | + |
| 3 | +Branch: `feat/path-based-export` @ `4857368` |
| 4 | +Reviewed against: `docs/plans/2026-08-01_path-based-export.md` |
| 5 | + |
| 6 | +## Verdict |
| 7 | + |
| 8 | +CHANGES_REQUESTED — **the exit criterion is unguarded.** The implementation streams correctly |
| 9 | +today; nothing stops it regressing tomorrow, and I proved that rather than inferring it. |
| 10 | + |
| 11 | +## What you did well |
| 12 | + |
| 13 | +- **The streaming implementation is right.** `COPY (<executable_sql>) TO '<temp>' (FORMAT ...)` |
| 14 | + on the request-scoped connection, wrapped in the atomic writer. That is exactly the design |
| 15 | + the plan asked for. |
| 16 | +- **The atomic write is correct and genuinely tested** — temp sibling, `os.replace` only on |
| 17 | + success, cleanup in `finally`, with |
| 18 | + `test_atomic_writer_preserves_existing_bytes_and_removes_temp` asserting the original's |
| 19 | + **bytes** survive a failed write. V5 is satisfied. |
| 20 | +- **`export/` and the whole Streamlit path are untouched.** The diff is empty. You wrote new |
| 21 | + path-based code rather than extending the byte-based `Exporter`, as required. |
| 22 | +- **`ExportController.shutdown()` is wired into `closeEvent`** — the crash-safety pattern this |
| 23 | + project learned the hard way. |
| 24 | +- **The session log says "not measured"** for the V8 mutations and the V11 crash batches |
| 25 | + instead of inventing results. That is the recording rule working under pressure, and it is |
| 26 | + why I knew exactly where to look. Keep doing this. |
| 27 | + |
| 28 | +### My measurements |
| 29 | + |
| 30 | +| check | result | |
| 31 | +|---|---| |
| 32 | +| suite on **3.14** | 346 passed, 1 skipped | |
| 33 | +| suite on **3.12** | 346 passed, 1 skipped — identical | |
| 34 | +| `run-quality-gates` | pass | |
| 35 | +| **V9** 3.14-only syntax | none | |
| 36 | +| **V2** Streamlit + `export/` diff | empty | |
| 37 | +| **V11** crash gate (25 of 50 so far) | 0 crashes; second batch running | |
| 38 | +| **V8 mutation 1 (materialise instead of stream)** | **DID NOT BITE — see H1** | |
| 39 | + |
| 40 | +## Required changes |
| 41 | + |
| 42 | +### H1. Nothing detects a regression from streaming to materialising |
| 43 | + |
| 44 | +This is the exit criterion: *"full DuckDB CSV/Parquet export does not materialize the entire |
| 45 | +result as a Polars DataFrame plus bytes."* |
| 46 | + |
| 47 | +I replaced the `COPY` call with the thing the phase exists to prevent: |
| 48 | + |
| 49 | +```python |
| 50 | +def copy_to(path: Path) -> None: |
| 51 | + frame = con.sql(request.executable_sql).pl() # materialise everything |
| 52 | + frame.write_csv(path) if fmt is ExportFormat.CSV else frame.write_parquet(path) |
| 53 | +``` |
| 54 | + |
| 55 | +**The entire suite passed — 346 passed, 1 skipped.** Not one test noticed. |
| 56 | + |
| 57 | +`tests/test_full_export.py` has two tests and both only inspect the **output file**. The plan |
| 58 | +warned about precisely this: *"a test that only inspects the output file cannot distinguish |
| 59 | +streaming from materialising."* The output is byte-identical either way — that is the whole |
| 60 | +problem. |
| 61 | + |
| 62 | +**Fix (V4 as specified):** spy on the request-scoped connection and assert **both**: |
| 63 | + |
| 64 | +1. a `COPY ... TO` statement is issued for the full-export path; and |
| 65 | +2. **no** materialisation call — `.pl()`, `.arrow()`, `.fetchall()`, `.df()` — occurs on it. |
| 66 | + |
| 67 | +Keep exporting **more rows than `preview_limit`** so a preview-shaped result cannot masquerade |
| 68 | +as a full one. Then re-apply the mutation above and confirm the new test **FAILS**. Paste the |
| 69 | +failing node id. |
| 70 | + |
| 71 | +### H2. Cancellation is unverified |
| 72 | + |
| 73 | +`tests/test_export_controller.py` contains one test (emits one terminal result). Task 10 and V6 |
| 74 | +are uncovered: cancelling mid-export must leave **no partial destination file**, **no temp |
| 75 | +file**, and an existing destination **untouched**; cancelling a finished export must be safe. |
| 76 | + |
| 77 | +Given a half-written export is a user-visible data hazard, this needs a real test, not an |
| 78 | +inspection. Task 9's other two Red cases are also missing — the handle published **before** |
| 79 | +work starts, and a failure surfacing as a failed export rather than an exception. |
| 80 | + |
| 81 | +### H3. The selection logic was duplicated, not reused |
| 82 | + |
| 83 | +`src/wherewolf/selection.py` is a **second** implementation of visual-column-order selection. |
| 84 | +`desktop/clipboard_serializers.py` still has its own. The plan was explicit: |
| 85 | + |
| 86 | +> **Reuse that logic; do not write a second implementation that can drift.** … **Do not |
| 87 | +> duplicate it** — if it needs to be shared, extract it once and have both call sites use it. |
| 88 | +
|
| 89 | +Two copies of "visual order, hidden columns excluded, discontiguous rule" will drift, and when |
| 90 | +they do, **copy and export will silently disagree about the same selection** — the kind of bug |
| 91 | +users report as "the export is wrong" with no error anywhere. |
| 92 | + |
| 93 | +Extract once and route both call sites through it. `tests/test_selection.py` currently holds a |
| 94 | +single test; whichever module survives needs the full set — moved columns, hidden columns, |
| 95 | +discontiguous selection. |
| 96 | + |
| 97 | +### H4. Run the V8 mutations |
| 98 | + |
| 99 | +The log records them as not measured, which is honest. Now run them, and record the node id you |
| 100 | +actually observed for each. Mutation 1 is H1 above; I have run it and it does not bite, so that |
| 101 | +one is already answered — fix the test, then confirm it fails. |
| 102 | + |
| 103 | +### H5. One commit for thirteen tasks |
| 104 | + |
| 105 | +The plan specifies one commit per task, and the round produced two: a baseline and a single |
| 106 | +`feat(export): add path-based desktop exports` carrying everything. |
| 107 | + |
| 108 | +I am **not** asking you to rewrite history. Going forward in this phase, commit per task. The |
| 109 | +granularity is what makes a failure bisectable, and it is the reason the plan is written as |
| 110 | +discrete tasks rather than a description of the finished state. |
| 111 | + |
| 112 | +## Delegate the low-level work to your subagents |
| 113 | + |
| 114 | +You have seven read-only `agent-memory` subagents available |
| 115 | +(`~/.codex/agents/*.toml`), and the MCP server is declared for this project with |
| 116 | +`--tool-profile full`. Use them and **surface what they return to me** rather than acting on it |
| 117 | +silently: |
| 118 | + |
| 119 | +- **`memory_researcher`** — before you start this round, ask it for prior constraints, decisions |
| 120 | + and **failed approaches** relevant to export, streaming, atomic writes and Qt worker |
| 121 | + lifetime. Report the claim IDs of anything consequential in the session log. |
| 122 | +- **`memory_evidence_reviewer`** — if a remembered claim would change what you build, audit it |
| 123 | + before relying on it, and report what is supported, contradicted or stale. |
| 124 | + |
| 125 | +Treat memory as **historical evidence, not current truth** — its own instructions say to |
| 126 | +revalidate drift-prone claims against the repository, which matches this project's rule that a |
| 127 | +claim is something to verify rather than trust. Do **not** ask `memory_curator` or |
| 128 | +`memory_lifecycle_manager` to write anything; nothing in this round authorizes a memory |
| 129 | +mutation. |
| 130 | + |
| 131 | +## Verification before marking complete |
| 132 | + |
| 133 | +- The V4 streaming spy, plus the mutation re-applied and its **failing** node id. |
| 134 | +- Cancellation tests per H2. |
| 135 | +- Single shared selection implementation per H3, with the full test set. |
| 136 | +- All six V8 mutations with observed node ids, `--color=no`, mutation-applied check. |
| 137 | +- `./run.sh uv run pytest -q` on 3.14 and `--python 3.12` — record both, then restore with |
| 138 | + `./run.sh uv sync --all-extras --dev --python 3.14`. |
| 139 | +- `scripts/orchestration/run-quality-gates` → exit 0. |
| 140 | +- `git status --short` → prints nothing. |
| 141 | +- **V11**: I am measuring 50 runs myself this round; do not re-run it unless you change |
| 142 | + `closeEvent` or worker lifetime. |
| 143 | + |
| 144 | +## Constraints |
| 145 | + |
| 146 | +Do not remove `timid = true`. Do not disable coverage. Do not skip, delete or xfail tests. Do |
| 147 | +not modify `export/exporter.py`, `DuckDBEngine`, or any Streamlit path. Do not touch `main`. Do |
| 148 | +not bump the package version. |
| 149 | + |
| 150 | +## Deferred — correctly recorded by you |
| 151 | + |
| 152 | +No human has exported from a real window; all Qt tests are offscreen. **Streaming is verified |
| 153 | +structurally, not by a memory measurement** — no multi-gigabyte export was performed, and the |
| 154 | +log says so, which is the right way to state it. Spark export unverified. macOS and Windows |
| 155 | +dialogs unverified. |
| 156 | + |
| 157 | +STATUS: CHANGES_REQUESTED |
0 commit comments