fix: fail closed on empty top exports - #19
Merged
Merged
Conversation
Owner
Author
🔍 Local review (cycle 1) — round d98c10e5-b896-4eca-aa0d-9bc80d2c9ed3Reviewed locally (
Proceeding to fix the codex finding before continuing. |
…ownload DOM re-check _select_view already hard-gates TABLE_ROW_SELECTOR.length > 0 on the DOM before "Скачать" is clicked for TOP_POPULAR/TOP_RELATED — a phrase with genuinely zero rows never reaches the download step, it dies inside _select_view's retry loop. So an empty CSV reaching _collect_one's parse step already contradicts a state the code itself proved moments earlier. The previous gate re-queried the DOM after download (an unbounded polling window) and only raised when that later read still showed rows. That re-read can observe a table that has since emptied (rerender, auth transition, page degradation) and silently wave the corrupted export through as an apparently valid row_count: 0 — the exact silent-corruption path the fail-closed fix for issue #11 was meant to close. Extract the predicate into _is_untrustworthy_empty_export(view, dataset), a pure function taking no page/session argument, so it is unit-testable without a CDP/browser harness — closing the "collector.py is untested" gap for this specific gate. Adds regression tests covering top_popular/ top_related (always rejected when empty) and dynamics/regions (untouched by this gate, per issue #11's live-CDP data showing dynamics consistently populated). Found by Codex review during the cycle-review pass on PR #19.
Owner
Author
🔍 Local review (cycle 2) — round 0b252bc1-be5b-47f3-b057-2cf2e80faa12Reviewed locally (
Proceeding to fix the codex finding before continuing. |
_select_view's pre-download hard gate (TABLE_ROW_SELECTOR.length > 0) is keyed off `view != WordstatView.REGIONS` — it applies identically to TOP_POPULAR, TOP_RELATED, and DYNAMICS, exempting only REGIONS (which has no table in its DOM). _is_untrustworthy_empty_export's tuple, however, only checked (TOP_POPULAR, TOP_RELATED), leaving DYNAMICS unguarded: an empty dynamics CSV could still pass through, get written as a valid row_count: 0 export, and contribute to a manifest status of "complete" — the exact silent-corruption path the cycle-1 fix (for issue #11) was meant to close. The previous exclusion of DYNAMICS rested on an empirical/frequency argument (issue #11's live-CDP data showed it consistently populated, 24 rows across three runs) rather than the gate's actual structural premise (the DOM already proved rows > 0 immediately before the download click, so a later empty read is always a lie) — which holds for DYNAMICS exactly as it does for the other two table-based views. Selecting views by observed frequency of emptiness instead of by the structural gate they share was the bug. Widen the tuple to include DYNAMICS, and rewrite the predicate's docstring to state the rule structurally (every view _select_view gates on TABLE_ROW_SELECTOR.length > 0, i.e. all but REGIONS) so it can't drift from _select_view again. Update the regression tests in test_collector_view.py accordingly. Found by Codex review during round 2 of the cycle-review pass on PR #19.
Owner
Author
📋 Review summary — all cycles
Totals: 2 FIX (both resolved), 1 IRRELEVANT, 0 SKIP, 0 UNVERIFIED. 3 review cycles completed. |
This was referenced Aug 21, 2026
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
1 participant
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
Closes #11 by refusing to treat a header-only
top_popular/top_relatedCSV as a successful export when the active page has rendered rows.Live CDP investigation found that Wordstat itself currently creates a header-only
wordstat_similar_queries.csvblob for both top views, despite the DOM showing data. The same was reproduced by clicking the export link directly in the attached browser, so this is not a collector readiness race. The collector now raises a visible failure, preserves the raw CSV for inspection, and leavesmanifest.jsonincomplete instead of silently producingstatus: complete.Verification
http://127.0.0.1:9223),купить телефон: failed closed withWordstat returned an empty top_popular CSV while the page rendered 200 table rows; export is not trustworthy; raw CSV 211 bytes; manifest statusincomplete.http://127.0.0.1:9223),курсы английского языка: same failure; raw CSV 228 bytes; manifest statusincomplete.top_popularandtop_relatedboth produced header-onlywordstat_similar_queries.csv;dynamicsandregionsproduced populated files (1094 and 41687 bytes respectively).uv run pytest -q— 112 passeduv run ruff check .— passedRisk / follow-up
This is intentionally fail-closed rather than a fabricated fallback: the upstream Wordstat export is empty while the rendered table is populated. A later fix can add a supported export/data source once the upstream behavior is understood.