docs: Issue #6 phase 1 live granularity research - #20
Merged
Merged
Conversation
axisrow
force-pushed
the
ao/wordstat-11/issue6-phase1-research
branch
from
August 21, 2026 05:46
13bf0e7 to
fe285a8
Compare
axisrow
marked this pull request as ready for review
August 21, 2026 06:17
This was referenced Aug 21, 2026
…, dead code removed Adds phase 2 findings to the phase 1 live-measurement document: - dtypes.py guards confirmed updated to real 'август 2024'/'22.06.2026' values; the _NUMERIC regex itself was not touched. - Weekly dynamics with an explicit period is not a partial UI glitch: Wordstat ignores the requested window entirely and returns its own default ~2-year range (107 full weeks) regardless of what was asked. validate_period() now rejects the combination outright. - Weekly dynamics without an explicit period was confirmed live to collect predictably (107 rows, factual period from CSV rows). - The now-unreachable weekly date-picker branch in collector.py was removed rather than left dead, collapsing a duplicated month_label computation into one shared function for month/day popups. - Backfill remains explicitly unestablished: the only comparison done was same-day, so it neither confirms nor rules out later revision of already-exported values. Flagged as an open question for a future time-separated remeasurement. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01JCyJr8vFFDW1eSMyCQyNNk
…ill open, dead code removed" This reverts commit bc62b8b.
… the platform The earlier PR #21 revert removed a section claiming Wordstat ignores an explicit weekly period. That claim was wrong: it came from a live prompt that never actually passed date_from/date_to (requested_period was null in that run's own log), so the default window it observed was default because no period was requested, not because Wordstat rejected one. A user-provided live screenshot of the UI (24.12.2018 -- 13.01.2019, weekly, applied by hand) disproved the claim directly. The real defect was in our own _wait_for_table_granularity: it checked only that the first cell's format matched the granularity, which the table's pre-existing (stale) content already satisfied before a period change had actually been applied -- a silent wrong-period export, not a platform limitation. A first fix attempt had its own bug: comparing the table's genitive month text ("24 декабря") against the nominative RUSSIAN_MONTHS list used for calendar clicks ("декабрь") never matched. Documents, with live verification: - weekly period applies exactly (24.12.2018..07.01.2019 for a requested 24.12.2018..13.01.2019 -- 07.01 is the start of the last full week ending 13.01, not truncation) - daily period applies with the same known variable trailing tail already documented above in this file (29/30 rows, not a new defect) - the live-check method (collect_many + --resume-dir, dynamics-only, since top_popular's fail-closed still blocks a full CLI run) - the lesson: DOM control state and actual table/export content can lag each other on live Wordstat (same class of issue as #3, for period instead of view switching) Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01JCyJr8vFFDW1eSMyCQyNNk
Owner
Author
🔍 Local review (cycle 1) — round 9ba8b46e-3b7a-4178-9ff0-8d84e2ea8658Reviewed locally (
Totals: 0 FIX, 0 SKIP, 0 UNVERIFIED. Clean on the first round. |
Owner
Author
📋 Review summary — all cycles
Totals: 0 FIX, 0 SKIP, 0 UNVERIFIED. |
axisrow
added a commit
that referenced
this pull request
Aug 21, 2026
Codex review (cycle-review round 1) found that _wait_for_period_applied only confirms the requested window's start (date_from) before downloading; a table that has repainted its first row but not yet its last row could still export and be marked complete with a stale end boundary. Add a containment check in _assert_contiguous_dynamics_rows: exported rows must fall inside the requested window (shorter is legitimate -- the documented variable daily trailing tail -- but a row outside either boundary is not). Weekly's lower bound compares against the aligned Monday of date_from's week, not the raw date, matching Wordstat's own snapping behavior. Separately, a live CDP measurement (this cycle's investigation of a review finding) confirmed issue #6 step 3's "daily granularity outside the 60-day window ... reject with a domain error" is only half implemented: validate_period bounds window length but not how far date_from can be from today. The daily date-range picker's year-select popup only ever offers the current year, so a request like --date-from 2019-01-01 passed pre-flight validation and failed deep inside calendar-click code with an opaque InterfaceChangedError. Add the missing pre-flight bound, expressed relative to `today` (not "only the current year" -- that would wrongly reject legal windows in January) and consistent with the existing 60-day window-length check: PR #20's live-confirmed window 22.06.2026-20.08.2026 remains accepted. Tests: uv run pytest -q (147 passed), uv run ruff check (clean).
axisrow
added a commit
that referenced
this pull request
Aug 21, 2026
Cycle-review round 2 findings against dc9b16b (round 1's fixes). Codex found the round-1 containment check (2a9689e) sat behind the same len(rows) < 2 early return as the pairwise contiguity loop, but containment only needs dates[0]/dates[-1] -- the same row, for a single-row export -- not a pair. A genuine 1-row daily/weekly export whose single date lay entirely outside the requested window passed _is_untrustworthy_empty_export (which only rejects 0 rows) and was written to Parquet and marked complete, unguarded. Fixed by hoisting the containment check above the row-count early return; the pairwise contiguity loop stays gated at >= 2 rows, since it genuinely needs a pair. Monthly stays exempt from both checks -- its period field is a nominative month name, not a parseable date, so validating its end boundary here would need a separate parser this PR does not add. /review flagged the monthly calendar path (.datepicker-month__years_select / .react-datepicker__month-text) as never live-confirmed, unlike daily/weekly. Live CDP verification against the authenticated Wordstat UI found two real bugs in that path, both now fixed: - month_label.capitalize() ("Январь") never matched the popup's actual lowercase nominative text ("январь"), failing every explicit --granularity monthly --date-from/--date-to request with InterfaceChangedError deep inside the click. - the monthly popup is a single shared range-picker, not two independent day/week-style popups: the intermediate DATE_RANGE_SELECTOR click between picking date_from and date_to closed it instead of reopening it. Skipping that click for the monthly path (day/week keep it -- their two-click flow is live-confirmed working) lets the still-open popup accept date_to directly. Live-verified end to end after both fixes: an explicit --granularity monthly --date-from 2024-01-01 --date-to 2024-06-30 request now collects 6 rows (январь-июнь 2024) with actual_period exactly matching requested_period. /review also re-raised (as RR1) the same weekly-CSV-format claim round 1 correctly SKIPped as R2 -- it recurred only because the doc that disproves it (docs/ISSUE6_PHASE1_RESEARCH.md, PR #20) isn't on this branch. Added an inline comment at the parse site citing that doc's confirmed byte-for-byte weekly CSV format, so a future review pass can check the comment instead of re-raising the same false positive against code alone. Tests: uv run pytest -q (151 passed), uv run ruff check (clean).
axisrow
added a commit
that referenced
this pull request
Aug 21, 2026
* feat: support dynamics granularity and requested periods * fix: validate contiguous dynamics rows without assuming full window * fix: wait for selected dynamics table before export * fix: inspect first dynamics cell for selected granularity * fix: use weekly date picker for weekly dynamics * fix: reject explicit weekly periods (Wordstat ignores the requested window) Live CDP measurements confirm Wordstat's weekly dynamics view silently ignores an explicit --date-from/--date-to and returns its own default ~2-year window instead (107 full weeks, unrelated to what was requested). Since a manifest recording a fabricated 'actual_period' that does not match the caller's intent would be the worst possible outcome, an explicit weekly period is now rejected up front by validate_period() with InvalidPeriodError, rather than collected and silently wrong. Weekly dynamics without an explicit period (Wordstat's default window) remains supported and was confirmed live to behave predictably. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01JCyJr8vFFDW1eSMyCQyNNk * refactor: drop the unreachable weekly date-picker path validate_period() now rejects an explicit weekly period before the browser is ever touched, so _set_period()'s 'week' popup_type and the matching branch in _select_calendar_date() are unreachable dead code. Removing them also collapses the duplicated month_label computation in _select_calendar_date() into one place, leaving a single function that drives both real remaining cases (month, day) instead of three parallel near-identical branches. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01JCyJr8vFFDW1eSMyCQyNNk * Revert "refactor: drop the unreachable weekly date-picker path" This reverts commit 6cdcc89. * Revert "fix: reject explicit weekly periods (Wordstat ignores the requested window)" This reverts commit 498ecff. * perf: make _collect_one's fixed settling/retry pauses test-injectable _collect_one always sleeps once per non-REGIONS view (settling for the export blob to catch up with a table repaint) and again on an empty-export retry — both are unconditional real-time pauses against the live UI, not _wait_for-style condition polling with an early return. Tests exercise _collect_one for real against a fake, instantly-responding page, so those sleeps were paid in full: 140 tests took 26.9s instead of well under 1s (root-caused by wordstat-1 from pytest --durations on PR #21). Both pauses are now constructor parameters (settling_seconds=1.0, empty_export_retry_seconds=2.0, matching prior hardcoded behavior) instead of hardcoded literals, so the CLI's real collector keeps its live-tested delays while tests can set them to 0. Full suite: 140 passed in 0.77s. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01JCyJr8vFFDW1eSMyCQyNNk * fix: wait for the dynamics table's first row to match the requested period _wait_for_table_granularity only checked the first cell's *format* (e.g. "looks like a week range"), which the table's pre-existing content already satisfied before a period change was applied -- the date-range button showed the newly picked dates while the table itself still held stale rows, and the exported CSV silently carried Wordstat's default window instead of the requested one. This was the actual root cause behind an earlier, incorrect conclusion that Wordstat ignores explicit weekly periods (it doesn't: a live screenshot of the UI showed a 2018-12-24 explicit weekly period applied correctly by hand); that conclusion has been reverted. _set_period now waits for the first cell to actually start at the expected date via a new _wait_for_period_applied, turning a silently wrong-period export into a loud InterfaceChangedError if the picker never catches up. Two related fixes surfaced during live verification: - The dynamics table's first-cell month name is genitive for daily/weekly ("22 июня", "24 декабря" -- "of June"/"of December") but nominative for monthly ("август 2024"). Matching daily/weekly's genitive text against the existing nominative RUSSIAN_MONTHS list (used for clicking the calendar popups) never matched, so a first attempt at this wait ran out its full timeout on every weekly/daily request. Added RUSSIAN_MONTHS_GENITIVE and match the exact expected month, not just any month word. - Weekly's first cell is the Monday of the requested date's week, not the date itself (confirmed live) -- not a bug, Wordstat's own alignment. Live-verified against the real Wordstat UI: - Weekly, requested 2018-12-24..2019-01-13: actual_period 24.12.2018..07.01.2019 (07.01 is the start of the last full week ending 13.01 -- the period applies exactly, 3 full weeks, no truncation). - Daily, requested 2026-07-22..2026-08-20: actual_period 22.07.2026..19.08.2026 (29 of 30 rows -- the same variable trailing-tail behavior already documented for daily dynamics in phase 1, not a new defect). 143 passed in 0.68s, ruff clean. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01JCyJr8vFFDW1eSMyCQyNNk * docs: clarify settling_seconds is preventive, unverified, and has a cost Per wordstat-1's decision: keep the default (1.0s, unchanged) but stop the comment from reading like a description of a caught bug. State plainly: introduced without a live timing measurement, no confirmed repro for this specific race, +1s/view (+3s/phrase, +50s on a 50-phrase batch), and why removing it can't currently be verified safe (issue #22 blocks a full CLI run from ever reaching DYNAMICS). Separately note that empty_export_retry_seconds is unlike it: it only fires after a concrete, already-observed empty-export trigger, not preventively. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01JCyJr8vFFDW1eSMyCQyNNk * fix: guard the dynamics period's end boundary and daily lower bound Codex review (cycle-review round 1) found that _wait_for_period_applied only confirms the requested window's start (date_from) before downloading; a table that has repainted its first row but not yet its last row could still export and be marked complete with a stale end boundary. Add a containment check in _assert_contiguous_dynamics_rows: exported rows must fall inside the requested window (shorter is legitimate -- the documented variable daily trailing tail -- but a row outside either boundary is not). Weekly's lower bound compares against the aligned Monday of date_from's week, not the raw date, matching Wordstat's own snapping behavior. Separately, a live CDP measurement (this cycle's investigation of a review finding) confirmed issue #6 step 3's "daily granularity outside the 60-day window ... reject with a domain error" is only half implemented: validate_period bounds window length but not how far date_from can be from today. The daily date-range picker's year-select popup only ever offers the current year, so a request like --date-from 2019-01-01 passed pre-flight validation and failed deep inside calendar-click code with an opaque InterfaceChangedError. Add the missing pre-flight bound, expressed relative to `today` (not "only the current year" -- that would wrongly reject legal windows in January) and consistent with the existing 60-day window-length check: PR #20's live-confirmed window 22.06.2026-20.08.2026 remains accepted. Tests: uv run pytest -q (147 passed), uv run ruff check (clean). * fix: strip a UTF-8 BOM from --phrases-file instead of leaking it Pre-existing bug, unrelated to issue #6 (introduced in 94b1a66, batch mode). /review flagged that _PHRASES_FILE_ENCODINGS tried plain "utf-8" before "cp1251", and a phrases file saved with a UTF-8 byte-order mark decodes successfully under plain "utf-8" too -- just with the BOM character left attached to the first line. Neither resolve_phrases' line.strip() nor the collector's own phrase.strip() removes it (''.isspace() is False), so the first phrase silently carried an invisible leading character into the typed Wordstat search, the run-directory slug, and manifest.json. Fix: try "utf-8-sig" before "cp1251" (it strips a BOM if present and decodes identically to "utf-8" otherwise). Tests: uv run pytest -q (147 passed), uv run ruff check (clean). * fix: guard single-row containment and fix the monthly calendar path Cycle-review round 2 findings against dc9b16b (round 1's fixes). Codex found the round-1 containment check (2a9689e) sat behind the same len(rows) < 2 early return as the pairwise contiguity loop, but containment only needs dates[0]/dates[-1] -- the same row, for a single-row export -- not a pair. A genuine 1-row daily/weekly export whose single date lay entirely outside the requested window passed _is_untrustworthy_empty_export (which only rejects 0 rows) and was written to Parquet and marked complete, unguarded. Fixed by hoisting the containment check above the row-count early return; the pairwise contiguity loop stays gated at >= 2 rows, since it genuinely needs a pair. Monthly stays exempt from both checks -- its period field is a nominative month name, not a parseable date, so validating its end boundary here would need a separate parser this PR does not add. /review flagged the monthly calendar path (.datepicker-month__years_select / .react-datepicker__month-text) as never live-confirmed, unlike daily/weekly. Live CDP verification against the authenticated Wordstat UI found two real bugs in that path, both now fixed: - month_label.capitalize() ("Январь") never matched the popup's actual lowercase nominative text ("январь"), failing every explicit --granularity monthly --date-from/--date-to request with InterfaceChangedError deep inside the click. - the monthly popup is a single shared range-picker, not two independent day/week-style popups: the intermediate DATE_RANGE_SELECTOR click between picking date_from and date_to closed it instead of reopening it. Skipping that click for the monthly path (day/week keep it -- their two-click flow is live-confirmed working) lets the still-open popup accept date_to directly. Live-verified end to end after both fixes: an explicit --granularity monthly --date-from 2024-01-01 --date-to 2024-06-30 request now collects 6 rows (январь-июнь 2024) with actual_period exactly matching requested_period. /review also re-raised (as RR1) the same weekly-CSV-format claim round 1 correctly SKIPped as R2 -- it recurred only because the doc that disproves it (docs/ISSUE6_PHASE1_RESEARCH.md, PR #20) isn't on this branch. Added an inline comment at the parse site citing that doc's confirmed byte-for-byte weekly CSV format, so a future review pass can check the comment instead of re-raising the same false positive against code alone. Tests: uv run pytest -q (151 passed), uv run ruff check (clean). --------- Co-authored-by: axisrow <axisrow@users.noreply.github.com> Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
axisrow
added a commit
that referenced
this pull request
Aug 21, 2026
…bs (#25) Issue #22: _is_untrustworthy_empty_export previously fail-closed on any empty TOP_POPULAR/TOP_RELATED/DYNAMICS export, on the premise that _select_view's pre-download row gate makes an empty CSV structurally impossible for any of the three. Issue #11 disproves that premise for TOP_POPULAR/TOP_RELATED: Wordstat returns them empty every single time, confirmed live, independent of the DOM state before the download click. Since TOP_POPULAR is the first view collected, every full collect() run failed on the very first view and never reached DYNAMICS/REGIONS. The predicate now only fail-closes DYNAMICS, whose emptiness is genuinely anomalous (24 rows across three live runs in #11, corroborated by PR #20). TOP_POPULAR/TOP_RELATED are recorded as normal zero-row exports instead. To keep issue #16's concern intact (a manifest must not read as an unqualified success when its data is empty), CollectionManifest gained a computed empty_views field, and status is now "incomplete" if either missing_views or empty_views is non-empty — so a manifest with two zero-row top views is visibly incomplete in manifest.json, not silently marked complete. Issue #9: collect_many created one CDP tab per CLI invocation via new_page() and never closed it; keep_alive=True correctly protects the user's own Chrome from being closed by session.stop(), but left our own tab orphaned every run. The tab is now closed via session.close_page() in the same finally block, before session.stop() (the CDP handle may be unusable after the session stops), guarded the same way session.stop() already is so a failed close doesn't discard collected results/failures. Verified live against CDP :9223 (not the default :9222): - single-phrase run reaches DYNAMICS/REGIONS; manifest shows status: incomplete, empty_views: [top_popular, top_related], dynamics: 24 rows, regions: 788 rows - --granularity daily collects dynamics_daily.parquet (59 rows) - two-phrase batch: one tab for the whole batch, 4 downloads total - failure path (bad region) after tab creation: tab count unchanged, failure surfaced to the CLI, exit code 1, no traceback - tab count via /json/list: 13 before and after every run above - pytest: 151 passed in ~0.7-0.9s (was 143 passed in 0.68s on main) Claude-Session: https://claude.ai/code/session_01JYQKwtowJVXEkZxmsma3Wm Co-authored-by: axisrow <axisrow@users.noreply.github.com> Co-authored-by: Claude Sonnet 5 <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
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.
Closes #6
Summary
ПериодformatsTests
uv run pytest -q(112 passed)uv run ruff check .Scope
collector.pyandwordstat.tomlunchangedRisk / follow-up