From 6c3c3a8f133b8bc110f8b6ef99e676913f558844 Mon Sep 17 00:00:00 2001 From: axisrow Date: Fri, 21 Aug 2026 11:03:37 +0700 Subject: [PATCH 1/3] fix: fail closed on empty top exports --- src/wordstat/collector.py | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/src/wordstat/collector.py b/src/wordstat/collector.py index c08fe60..6495a16 100644 --- a/src/wordstat/collector.py +++ b/src/wordstat/collector.py @@ -337,6 +337,15 @@ async def _collect_one( # Convert before disposing of the download, so a parse or # write failure leaves the raw CSV on disk to inspect. dataset = parse_wordstat_csv(source, view) + if view in (WordstatView.TOP_POPULAR, WordstatView.TOP_RELATED) and not dataset.rows: + rendered_rows = await page.evaluate( + f"() => document.querySelectorAll({json.dumps(TABLE_ROW_SELECTOR)}).length" + ) + if int(rendered_rows) > 0: + raise InterfaceChangedError( + f"Wordstat returned an empty {view.value} CSV while the page rendered " + f"{rendered_rows} table rows; export is not trustworthy" + ) data_path, dtypes = write_dataset(dataset, run_directory) raw_path = finalize_raw(source, run_directory, view, self.keep_raw) except Exception: # noqa: BLE001 From 5bfb556638cef9c0c09782af99b0992b83d36d96 Mon Sep 17 00:00:00 2001 From: axisrow Date: Fri, 21 Aug 2026 12:40:18 +0700 Subject: [PATCH 2/3] fix: reject empty top exports unconditionally, drop unreliable post-download DOM re-check MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit _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. --- src/wordstat/collector.py | 45 +++++++++++++++++++++++++++++------- tests/test_collector_view.py | 38 ++++++++++++++++++++++++++++-- 2 files changed, 73 insertions(+), 10 deletions(-) diff --git a/src/wordstat/collector.py b/src/wordstat/collector.py index 6495a16..d8b0e48 100644 --- a/src/wordstat/collector.py +++ b/src/wordstat/collector.py @@ -23,6 +23,7 @@ BatchCollectionResult, CollectionManifest, CollectionResult, + CsvDataset, ExportSummary, PhraseFailure, WordstatView, @@ -53,6 +54,38 @@ } +def _is_untrustworthy_empty_export(view: WordstatView, dataset: CsvDataset) -> bool: + """True if an empty CSV for this view can never be a legitimate export. + + Only TOP_POPULAR/TOP_RELATED are checked: _select_view already hard-gates + TABLE_ROW_SELECTOR.length > 0 on the DOM before the "Скачать" click is + ever issued for these two table-based views (see the docstring on + _select_view and CLAUDE.md's issue #3/#13 section) — a phrase with + genuinely zero rows never reaches the download step at all, it dies + inside _select_view's retry loop with InterfaceChangedError. So by the + time a dataset for one of these views is parsed here, an empty + dataset.rows already contradicts a state the code itself proved moments + earlier; there is no code path left where that emptiness is legitimate. + + This used to be conditioned on a second, post-download DOM read + (`rendered_rows > 0`) — but re-querying the DOM after the download's + unbounded polling window can observe a table that has since emptied + (rerender, auth transition, page degradation), which let the empty + export through as an apparently valid `row_count: 0` and silently + corrupt the manifest into `status: "complete"` — the exact failure mode + issue #11's fix was meant to close. The pre-download gate is already + proof enough; no second opinion from a later DOM read is needed or + trustworthy. See tests/test_collector_view.py. + + DYNAMICS is deliberately excluded: issue #11's live-CDP data showed it + consistently populated (24 rows across all three runs) while + TOP_POPULAR/TOP_RELATED alone were empty, and the problem is specific to + those two views, not a general "any table view can be empty" case. + REGIONS has no table in its DOM at all and is unrelated. + """ + return view in (WordstatView.TOP_POPULAR, WordstatView.TOP_RELATED) and not dataset.rows + + def _without_traceback(error: Exception) -> Exception: """Drop the traceback before an exception is stashed in PhraseFailure. @@ -337,15 +370,11 @@ async def _collect_one( # Convert before disposing of the download, so a parse or # write failure leaves the raw CSV on disk to inspect. dataset = parse_wordstat_csv(source, view) - if view in (WordstatView.TOP_POPULAR, WordstatView.TOP_RELATED) and not dataset.rows: - rendered_rows = await page.evaluate( - f"() => document.querySelectorAll({json.dumps(TABLE_ROW_SELECTOR)}).length" + if _is_untrustworthy_empty_export(view, dataset): + raise InterfaceChangedError( + f"Wordstat returned an empty {view.value} CSV, but the page had rendered at " + "least one table row before the download was triggered; export is not trustworthy" ) - if int(rendered_rows) > 0: - raise InterfaceChangedError( - f"Wordstat returned an empty {view.value} CSV while the page rendered " - f"{rendered_rows} table rows; export is not trustworthy" - ) data_path, dtypes = write_dataset(dataset, run_directory) raw_path = finalize_raw(source, run_directory, view, self.keep_raw) except Exception: # noqa: BLE001 diff --git a/tests/test_collector_view.py b/tests/test_collector_view.py index f8bda31..3934793 100644 --- a/tests/test_collector_view.py +++ b/tests/test_collector_view.py @@ -4,9 +4,9 @@ import pytest -from wordstat.collector import WordstatCollector +from wordstat.collector import WordstatCollector, _is_untrustworthy_empty_export from wordstat.errors import InterfaceChangedError -from wordstat.models import WordstatView +from wordstat.models import CsvDataset, WordstatView def test_select_view_retries_once_when_active_marker_does_not_change(monkeypatch, tmp_path): @@ -148,6 +148,40 @@ async def snapshot(self, page): assert soft_required is False +def _dataset(view: WordstatView, rows: list[dict[str, str]]) -> CsvDataset: + return CsvDataset(view=view, headers=["query", "count"], rows=rows) + + +@pytest.mark.parametrize("view", [WordstatView.TOP_POPULAR, WordstatView.TOP_RELATED]) +def test_empty_export_is_untrustworthy_for_top_views_regardless_of_dom_state(view): + # Regression guard for issue #11: _select_view already hard-gates + # TABLE_ROW_SELECTOR.length > 0 on the DOM before "Скачать" is ever + # clicked for these two views, so an empty CSV reaching this point is + # already a contradiction — it must be rejected unconditionally, with no + # second, later DOM read able to wave it through as "legitimately empty" + # (that re-read can observe a table that has since emptied and silently + # accept a corrupted export — the exact bug this predicate replaces). + assert _is_untrustworthy_empty_export(view, _dataset(view, [])) is True + + +@pytest.mark.parametrize("view", [WordstatView.TOP_POPULAR, WordstatView.TOP_RELATED]) +def test_non_empty_export_is_trusted_for_top_views(view): + assert _is_untrustworthy_empty_export(view, _dataset(view, [{"query": "a", "count": "1"}])) is False + + +def test_empty_dynamics_export_is_not_flagged_by_this_gate(): + # Deliberately out of scope: issue #11's live-CDP data showed dynamics + # consistently populated (24 rows across all three runs) while + # top_popular/top_related alone were empty; the problem is specific to + # those two views. See _is_untrustworthy_empty_export's docstring. + assert _is_untrustworthy_empty_export(WordstatView.DYNAMICS, _dataset(WordstatView.DYNAMICS, [])) is False + + +def test_empty_regions_export_is_not_flagged_by_this_gate(): + # regions (map view) has no table in its DOM at all. + assert _is_untrustworthy_empty_export(WordstatView.REGIONS, _dataset(WordstatView.REGIONS, [])) is False + + def test_select_view_does_not_wait_for_table_rows_on_map_view(monkeypatch, tmp_path): # The map view (WordstatView.REGIONS) has no table rows in its DOM at # all, so gating on row presence would hang forever; it must only wait From 536136528fc5ff86993b8c656fffd48c37f06d24 Mon Sep 17 00:00:00 2001 From: axisrow Date: Fri, 21 Aug 2026 12:45:11 +0700 Subject: [PATCH 3/3] fix: include DYNAMICS in the untrustworthy-empty-export gate MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit _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. --- src/wordstat/collector.py | 39 ++++++++++++++++++++++++------------ tests/test_collector_view.py | 29 ++++++++++++--------------- 2 files changed, 39 insertions(+), 29 deletions(-) diff --git a/src/wordstat/collector.py b/src/wordstat/collector.py index d8b0e48..3ffc7ad 100644 --- a/src/wordstat/collector.py +++ b/src/wordstat/collector.py @@ -57,15 +57,23 @@ def _is_untrustworthy_empty_export(view: WordstatView, dataset: CsvDataset) -> bool: """True if an empty CSV for this view can never be a legitimate export. - Only TOP_POPULAR/TOP_RELATED are checked: _select_view already hard-gates - TABLE_ROW_SELECTOR.length > 0 on the DOM before the "Скачать" click is - ever issued for these two table-based views (see the docstring on - _select_view and CLAUDE.md's issue #3/#13 section) — a phrase with - genuinely zero rows never reaches the download step at all, it dies - inside _select_view's retry loop with InterfaceChangedError. So by the + TOP_POPULAR/TOP_RELATED/DYNAMICS are checked — every view for which + _select_view hard-gates TABLE_ROW_SELECTOR.length > 0 on the DOM before + the "Скачать" click is ever issued (see the docstring on _select_view + and CLAUDE.md's issue #3/#13 section). REGIONS is the only view exempt + from that gate (it is a map with no table rows in its DOM), so it is the + only view exempt here too — this set must stay in lockstep with + _select_view's `if view != WordstatView.REGIONS:` condition, not be + picked per-view by hand. + + A phrase that clears the pre-download gate has already had the code + itself prove TABLE_ROW_SELECTOR.length > 0 moments earlier; genuinely + zero rows never reaches the download step at all — it dies inside + _select_view's retry loop with InterfaceChangedError instead. So by the time a dataset for one of these views is parsed here, an empty dataset.rows already contradicts a state the code itself proved moments - earlier; there is no code path left where that emptiness is legitimate. + earlier; there is no code path left where that emptiness is legitimate, + for any of the three table-based views alike. This used to be conditioned on a second, post-download DOM read (`rendered_rows > 0`) — but re-querying the DOM after the download's @@ -77,13 +85,18 @@ def _is_untrustworthy_empty_export(view: WordstatView, dataset: CsvDataset) -> b proof enough; no second opinion from a later DOM read is needed or trustworthy. See tests/test_collector_view.py. - DYNAMICS is deliberately excluded: issue #11's live-CDP data showed it - consistently populated (24 rows across all three runs) while - TOP_POPULAR/TOP_RELATED alone were empty, and the problem is specific to - those two views, not a general "any table view can be empty" case. - REGIONS has no table in its DOM at all and is unrelated. + DYNAMICS was previously excluded on the strength of issue #11's live-CDP + data (24 rows across three runs, never empty) — but that is evidence the + gate rarely fires for DYNAMICS, not evidence that omitting it is safe. + The gate's premise is structural (the DOM proved rows>0 immediately + before the click), and that premise holds for DYNAMICS exactly as it + does for TOP_POPULAR/TOP_RELATED; selecting views by observed frequency + of emptiness rather than by the structural gate they share was the bug. """ - return view in (WordstatView.TOP_POPULAR, WordstatView.TOP_RELATED) and not dataset.rows + return ( + view in (WordstatView.TOP_POPULAR, WordstatView.TOP_RELATED, WordstatView.DYNAMICS) + and not dataset.rows + ) def _without_traceback(error: Exception) -> Exception: diff --git a/tests/test_collector_view.py b/tests/test_collector_view.py index 3934793..26ca7dc 100644 --- a/tests/test_collector_view.py +++ b/tests/test_collector_view.py @@ -152,31 +152,28 @@ def _dataset(view: WordstatView, rows: list[dict[str, str]]) -> CsvDataset: return CsvDataset(view=view, headers=["query", "count"], rows=rows) -@pytest.mark.parametrize("view", [WordstatView.TOP_POPULAR, WordstatView.TOP_RELATED]) -def test_empty_export_is_untrustworthy_for_top_views_regardless_of_dom_state(view): - # Regression guard for issue #11: _select_view already hard-gates - # TABLE_ROW_SELECTOR.length > 0 on the DOM before "Скачать" is ever - # clicked for these two views, so an empty CSV reaching this point is - # already a contradiction — it must be rejected unconditionally, with no +@pytest.mark.parametrize( + "view", [WordstatView.TOP_POPULAR, WordstatView.TOP_RELATED, WordstatView.DYNAMICS] +) +def test_empty_export_is_untrustworthy_for_table_views_regardless_of_dom_state(view): + # Regression guard for issue #11 (and its cycle-2 follow-up): _select_view + # already hard-gates TABLE_ROW_SELECTOR.length > 0 on the DOM before + # "Скачать" is ever clicked for every view but REGIONS, so an empty CSV + # reaching this point is already a contradiction for all three of these + # table-based views — it must be rejected unconditionally, with no # second, later DOM read able to wave it through as "legitimately empty" # (that re-read can observe a table that has since emptied and silently # accept a corrupted export — the exact bug this predicate replaces). assert _is_untrustworthy_empty_export(view, _dataset(view, [])) is True -@pytest.mark.parametrize("view", [WordstatView.TOP_POPULAR, WordstatView.TOP_RELATED]) -def test_non_empty_export_is_trusted_for_top_views(view): +@pytest.mark.parametrize( + "view", [WordstatView.TOP_POPULAR, WordstatView.TOP_RELATED, WordstatView.DYNAMICS] +) +def test_non_empty_export_is_trusted_for_table_views(view): assert _is_untrustworthy_empty_export(view, _dataset(view, [{"query": "a", "count": "1"}])) is False -def test_empty_dynamics_export_is_not_flagged_by_this_gate(): - # Deliberately out of scope: issue #11's live-CDP data showed dynamics - # consistently populated (24 rows across all three runs) while - # top_popular/top_related alone were empty; the problem is specific to - # those two views. See _is_untrustworthy_empty_export's docstring. - assert _is_untrustworthy_empty_export(WordstatView.DYNAMICS, _dataset(WordstatView.DYNAMICS, [])) is False - - def test_empty_regions_export_is_not_flagged_by_this_gate(): # regions (map view) has no table in its DOM at all. assert _is_untrustworthy_empty_export(WordstatView.REGIONS, _dataset(WordstatView.REGIONS, [])) is False