fix(web): the CSV export sent a body on HEAD, dropped connections, an… - #723
Merged
Conversation
…d truncated itself (#703) Review findings on #703, which merged before these landed. Three of them are on the export route, and all three are the kind static analysis cannot see. A HEAD RETURNED THE WHOLE FILE. `_send`, `_send_json` and `_serve_static` all end their write with `if self.command != "HEAD"`. `_send_csv` did not, and `do_HEAD` delegates to `do_GET` -- so `HEAD /api/timeline/export.csv` answered with the header block plus 468 bytes of CSV. On a keep-alive connection those bytes are framed as the NEXT response: a same-origin response desync, not merely a wasted transfer, and a violation of RFC 9110 §9.3.2. The first test written for this passed against the bug. `http.client` knows a HEAD carries no body and never reads one, so it reports an empty body whether or not the server sent bytes -- the test now reads a raw socket, and says why in its docstring. THE ROUTE DROPPED CONNECTIONS. `respond` is what normally guarantees this server never answers a GET by raising: `ApiRefusal` becomes a 400, anything else a 500 envelope. The CSV branch does not go through it, so `?limit=abc` propagated out of the handler -- the client saw a reset connection with no response, and a traceback of absolute source paths reached the stderr `log_message` is overridden to keep quiet. A first-run machine with no config.yaml did the same, where `/api/timeline` answers 200 with `engine: "stopped"`. Wrapped now, answering the JSON envelope: the failure is not a file, and a browser handed a truncated download learns nothing. THE EXPORT WAS SILENTLY TRUNCATED. It inherited the paged route's `?limit=` -- default 200, ceiling 2000 -- and the file said nothing about it. A 5,000-event deployment exported its most recent 200 rows, to be handed to an auditor or a tax preparer as a complete record. The cap exists because the console POLLS the JSON route every 15 seconds; an export is a deliberate download, requested once. `export_rows` reads the whole scope, and the scope -- the operator's own choice of today/7d/all -- is what bounds it. A SORT COLUMN THAT ORDERED NOTHING. `sortable=("ts", ...)` while the payload emits `at`, so `?sort=ts` was accepted, echoed back as applied, and left every row where it was -- the exact failure refusing an unknown column exists to prevent. The pin for this rot already existed at `test_api.py:526` and covered two routes; `/api/timeline` was simply not in it, and is now. AN UNREADABLE LOG LOOKED LIKE AN IDLE ENGINE. `read_log_window` returns a `LogWindow` for every outcome and carries the result in `status`; that status was discarded, so a log that could not be read (permissions, a rotation race) yielded zero lines -- dropping every `system` row AND dropping `system` from the chips. In the CSV an auditor opens, that is indistinguishable from "the engine never ran". `activity.py`'s own docstring names the failure: silently discarding input is how a feed comes to under-report reality while looking healthy. It now raises, and the route turns it into a stated failure. The `except OSError` around it was dead -- `read_log_window` catches its own -- while the call that does raise on a real deployment, `load_config`, sat outside it. LEADING WHITESPACE SMUGGLED FORMULAS PAST, and a test pinned the gap open. `" =cmd|..."` is a legal `coinbase_id` out of an imported venue CSV and lands in a cell by itself; Sheets and LibreOffice trim before deciding whether a cell is a formula. The check now strips SPACES only -- a bare `lstrip()` consumes tab and carriage return, which are themselves triggers, and broke two existing tests when tried. `\\n` joins the trigger list. TWO TESTS THAT PASSED FOR THE WRONG REASON. Removing `csv_safe` from nine of its ten cells left the whole suite green: only `reference` was pinned, so the module's load-bearing claim -- "applied to EVERY text cell, not to a list of the risky ones" -- was untested for nine columns including `summary`, which carries an imported `notes` field verbatim. And deleting the newest-first sort left the suite green too, because the fixtures seeded oldest-to-newest in the same order the sources are concatenated. Both are pinned now with scrambled fixtures and a whole-row export assertion, and both die under mutation, along with the HEAD guard and the export cap. Smaller: an unrecognised `?kind=` is collapsed to "every kind" rather than applied, so the plural typo no longer returns a page that looks like an empty deployment -- the outcome `normalise_scope`, which it sits beside, exists to never produce; the cap's docstring said the READ was bounded when it bounds the response slice (four unfiltered SELECTs underneath); and the export URL is built with `URL`/`searchParams` like every other URL in the client. That last one was caught by the call-resolution scanner added in #701: `URL` was not in its browser-globals list, so the guard failed the build on its own author's new code. `URL` is a real browser API and now belongs to that list. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
… route (#703) Second review round on the fix PR. Two of its own fixes were wrong, and one of them was wrong in exactly the way the PR exists to correct. THE EXPORT WAS NEVER UNCAPPED. `export_rows` passed `_UNCAPPED = 2**31` into `gather_timeline`, which clamps with `min(int(limit), MAX_TIMELINE_LIMIT)` -- so the "whole scope" export stopped at 2000 rows. The cap moved from 200 to 2000 and a docstring was written saying it was gone. The test could not see it: it seeded `DEFAULT_TIMELINE_LIMIT + 25` = 225 rows, under the real cap. A fixture below the threshold it is testing cannot test the threshold -- which is the precise failure this PR was opened to fix, committed inside the fix. Seeded above `MAX_TIMELINE_LIMIT` now, and it fails at 2000 of 2025 without the change. The slice is genuinely skippable (`limit: int | None`) rather than a large number pushed through a clamp. A sentinel the clamp silently eats is not a sentinel. And the memory cost of a real uncapped export -- the whole CSV as `str` then `bytes`, because the response carries a `Content-Length` -- is now written down as the accepted cost of a download an operator asked for once, which is also why the paged route keeps its cap. THE LOG FIX TOOK OUT THE PAGE IT WAS MEANT TO MAKE HONEST. Raising on any read status but `ok`/`missing` looked right against `unreadable`. But `read_log_window` also returns `empty` -- an ordinary state, a freshly created handler or the moment after a rotation -- and `oversized` for one long record. Neither is a read failure, and both now 500'd the whole Timeline page and raised in the export: orders, flows and attestations lost to report a non-problem, while `/api/activity` renders those same statuses as a stated feed state. Both earlier attempts were wrong in opposite directions -- discarding the status under-reported reality while looking healthy, raising on it failed a page over a healthy log -- so the report carries `log_status` and the answer is stated rather than acted on. `log_gap` separates "this deployment has never run" from "the file is there and its contents did not reach this report". The CSV writes a `# NOTE:` line above the header when there is a gap, because a file that leaves the application cannot be asked what is missing from it, and absent rows look identical to rows that never existed. THE CLIENT KEPT THE PRE-RENAME SORT KEY. The server moved `sortable` from `ts` to `at` correctly; the table header still declared `key: "ts"`, and `headerCell` only draws a sort control for a key the server declares -- so the timestamp column of a CHRONOLOGY page became an unclickable label. (`api.sortable_columns()` exists for this cross-check and has no callers; wiring it up is worth its own change.) TWO MORE TESTS THAT PASSED FOR THE WRONG REASON. The tie-break test asserted only that two calls agree, which holds with no tie-break at all because `list.sort` is stable and the merge order deterministic -- it now asserts the ordering's content. And the "every cell" test pinned three of ten columns, because the fixture left the other seven keel-written; `product_id`, `side` and `status` now arrive hostile, and dropping `csv_safe` from `product_id` fails. Also: `\\n`'s place in the trigger list is explained rather than merely present, and the `ApiRefusal` arm on the export branch is removed -- nothing on that path raises one now that it reads no `?limit=` or `?sort=`, and an error path nothing exercises rots. Re-add it the day a refusing helper joins that path. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
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.
…d truncated itself (#703)
Review findings on #703, which merged before these landed. Three of them are on the export route, and all three are the kind static analysis cannot see.
A HEAD RETURNED THE WHOLE FILE.
_send,_send_jsonand_serve_staticall end their write withif self.command != "HEAD"._send_csvdid not, anddo_HEADdelegates todo_GET-- soHEAD /api/timeline/export.csvanswered with the header block plus 468 bytes of CSV. On a keep-alive connection those bytes are framed as the NEXT response: a same-origin response desync, not merely a wasted transfer, and a violation of RFC 9110 §9.3.2.The first test written for this passed against the bug.
http.clientknows a HEAD carries no body and never reads one, so it reports an empty body whether or not the server sent bytes -- the test now reads a raw socket, and says why in its docstring.THE ROUTE DROPPED CONNECTIONS.
respondis what normally guarantees this server never answers a GET by raising:ApiRefusalbecomes a 400, anything else a 500 envelope. The CSV branch does not go through it, so?limit=abcpropagated out of the handler -- the client saw a reset connection with no response, and a traceback of absolute source paths reached the stderrlog_messageis overridden to keep quiet. A first-run machine with no config.yaml did the same, where/api/timelineanswers 200 withengine: "stopped". Wrapped now, answering the JSON envelope: the failure is not a file, and a browser handed a truncated download learns nothing.THE EXPORT WAS SILENTLY TRUNCATED. It inherited the paged route's
?limit=-- default 200, ceiling 2000 -- and the file said nothing about it. A 5,000-event deployment exported its most recent 200 rows, to be handed to an auditor or a tax preparer as a complete record. The cap exists because the console POLLS the JSON route every 15 seconds; an export is a deliberate download, requested once.export_rowsreads the whole scope, and the scope -- the operator's own choice of today/7d/all -- is what bounds it.A SORT COLUMN THAT ORDERED NOTHING.
sortable=("ts", ...)while the payload emitsat, so?sort=tswas accepted, echoed back as applied, and left every row where it was -- the exact failure refusing an unknown column exists to prevent. The pin for this rot already existed attest_api.py:526and covered two routes;/api/timelinewas simply not in it, and is now.AN UNREADABLE LOG LOOKED LIKE AN IDLE ENGINE.
read_log_windowreturns aLogWindowfor every outcome and carries the result instatus; that status was discarded, so a log that could not be read (permissions, a rotation race) yielded zero lines -- dropping everysystemrow AND droppingsystemfrom the chips. In the CSV an auditor opens, that is indistinguishable from "the engine never ran".activity.py's own docstring names the failure: silently discarding input is how a feed comes to under-report reality while looking healthy. It now raises, and the route turns it into a stated failure. Theexcept OSErroraround it was dead --read_log_windowcatches its own -- while the call that does raise on a real deployment,load_config, sat outside it.LEADING WHITESPACE SMUGGLED FORMULAS PAST, and a test pinned the gap open.
" =cmd|..."is a legalcoinbase_idout of an imported venue CSV and lands in a cell by itself; Sheets and LibreOffice trim before deciding whether a cell is a formula. The check now strips SPACES only -- a barelstrip()consumes tab and carriage return, which are themselves triggers, and broke two existing tests when tried.\\njoins the trigger list.TWO TESTS THAT PASSED FOR THE WRONG REASON. Removing
csv_safefrom nine of its ten cells left the whole suite green: onlyreferencewas pinned, so the module's load-bearing claim -- "applied to EVERY text cell, not to a list of the risky ones" -- was untested for nine columns includingsummary, which carries an importednotesfield verbatim. And deleting the newest-first sort left the suite green too, because the fixtures seeded oldest-to-newest in the same order the sources are concatenated. Both are pinned now with scrambled fixtures and a whole-row export assertion, and both die under mutation, along with the HEAD guard and the export cap.Smaller: an unrecognised
?kind=is collapsed to "every kind" rather than applied, so the plural typo no longer returns a page that looks like an empty deployment -- the outcomenormalise_scope, which it sits beside, exists to never produce; the cap's docstring said the READ was bounded when it bounds the response slice (four unfiltered SELECTs underneath); and the export URL is built withURL/searchParamslike every other URL in the client.That last one was caught by the call-resolution scanner added in #701:
URLwas not in its browser-globals list, so the guard failed the build on its own author's new code.URLis a real browser API and now belongs to that list.What & why
Tests-first evidence
Gates (all must pass)
uv run ruff checkcleanuv run mypycleanuv run pytest -qgreenScope check
leave checked only if true, and if so: cite the source and open the discussion
BEFORE review (CONTRIBUTING.md, "Governance: rulings vs. machinery").