diff --git a/src/wordstat/cli.py b/src/wordstat/cli.py index 5939f51..d6ee7e1 100644 --- a/src/wordstat/cli.py +++ b/src/wordstat/cli.py @@ -168,8 +168,40 @@ def collect( except WordstatError as error: raise click.ClickException(str(error)) from error + partial_count = 0 for result in batch.results: click.echo(result.manifest_path) + # issue #27: a result with missing_views is one where at least one + # view failed (but at least one other succeeded — a phrase where + # nothing at all was collected never reaches batch.results, see + # _collect_one's own guard) — surface that per view instead of + # letting "Собрано N из M" imply every phrase in it is complete. + # Deliberately NOT keyed off manifest.status: status is + # "incomplete" whenever empty_views is non-empty too, and + # top_popular/top_related are legitimately empty on every live + # Wordstat run (issue #22/#25) — keying success on status would + # make ordinary complete runs exit 1 again. + # + # Union with view_errors, not manifest.missing_views alone (cycle-review + # follow-up to #27): missing_views is a computed field over + # manifest.exports only, which under --resume-dir can still list a view + # as "exported" from a *prior* run even though that view's parquet is + # gone from disk and this run's re-collection attempt failed for it + # (views_to_collect re-attempts a view whose export entry survives but + # whose file doesn't — see storage.py). In that case view_errors has an + # entry for the view but missing_views is empty, so keying only off + # missing_views silently reported a run with a missing parquet as a + # full, error-free success at exit 0 — the exact "врёт о фактическом + # результате" failure mode issue #27 was about, just from the opposite + # direction (a stale manifest entry instead of a fresh gap). + reported_views = set(result.manifest.missing_views) | set(result.view_errors) + if reported_views: + partial_count += 1 + for view in sorted(reported_views, key=lambda v: v.value): + reason = result.view_errors.get(view, "не собран") + click.echo(f" {result.manifest.phrase} [{view.value}]: {reason}", err=True) + for warning in result.escaped_download_warnings: + click.echo(f" {result.manifest.phrase}: {warning}", err=True) for failure in batch.failures: click.echo(f"{failure.phrase}: {failure.error}", err=True) @@ -179,6 +211,7 @@ def collect( # attempted, so "Собрано N из M" alone would misleadingly read as "all the # rest failed". suffix = f" (батч прерван, {skipped} фраз(ы) не пробовались)" if skipped else "" - click.echo(f"Собрано {len(batch.results)} из {batch.total}{suffix}", err=True) + partial_suffix = f", из них частично {partial_count}" if partial_count else "" + click.echo(f"Собрано {len(batch.results)} из {batch.total}{partial_suffix}{suffix}", err=True) if batch.failures: ctx.exit(1) diff --git a/src/wordstat/collector.py b/src/wordstat/collector.py index b16e9e4..57d9ab0 100644 --- a/src/wordstat/collector.py +++ b/src/wordstat/collector.py @@ -3,6 +3,7 @@ import asyncio import json import re +import shutil import tempfile import time from collections.abc import Callable @@ -15,6 +16,7 @@ from wordstat.dataset_io import write_dataset from wordstat.errors import ( AuthenticationRequiredError, + DownloadEscapedError, DownloadTimeoutError, InterfaceChangedError, InvalidRequestError, @@ -189,7 +191,20 @@ def __init__( async def collect( self, phrase: str, region: str = "Россия", resume_directory: Path | None = None ) -> CollectionResult: - """Collect popular, related, dynamics and regional reports for one phrase.""" + """Collect popular, related, dynamics and regional reports for one phrase. + + Contract change (issue #27): a phrase that collected at least one + view but not all four (e.g. regions failed after top_popular/ + top_related/dynamics already succeeded) no longer raises here. Per + _collect_one, that case now comes back from collect_many as a + result, not a failure — this method's ``if batch.failures`` branch + therefore only fires when *zero* views were collected for the + phrase (see _collect_one's own guard) or the session's + authentication was lost. A caller that needs to know whether every + view was collected must inspect the returned result's + ``manifest.missing_views``/``view_errors``, not rely on this method + raising for a partial run the way it used to. + """ batch = await self.collect_many([phrase], region=region, resume_directory=resume_directory) if batch.failures: @@ -402,6 +417,33 @@ async def _collect_one( manifest_path=manifest_path, manifest=manifest, ) + # Cycle-review follow-up to issue #27: views_to_collect() treats a + # view as pending when its .parquet is missing from disk, + # even if manifest.exports still has an ExportSummary for it (the + # file was manually deleted after a prior run, or a previous + # resume's write_manifest happened but the file write that should + # have preceded it did not — see write_dataset/finalize_raw + # ordering). Until this view is re-collected below, its stale + # export entry must not stay in manifest.exports: missing_views is + # a computed field derived only from exports (see + # CollectionManifest), so an unpruned stale entry makes + # missing_views silently omit a view whose data file does not + # exist. Pruned *before* the loop attempts to re-collect it (not + # only on a failed retry) so a crash/Ctrl-C mid-retry still leaves + # an honest manifest, consistent with this method's existing + # "write after every step, never lie about what's on disk" + # invariant. A successful re-collection below overwrites this via + # merge_export as usual; nothing here disturbs a view that is not + # in pending_views. + stale_pending = {view for view in pending_views if any(e.view == view for e in manifest.exports)} + if stale_pending: + manifest = manifest.model_copy( + update={ + "exports": [e for e in manifest.exports if e.view not in stale_pending], + "updated_at": datetime.now(UTC), + } + ) + write_manifest(manifest_path, manifest) else: run_directory = create_run_directory(self.output_root, phrase) manifest = None @@ -476,84 +518,148 @@ async def _collect_one( manifest = manifest.model_copy(update={"source_url": await page.get_url(), "updated_at": datetime.now(UTC)}) write_manifest(manifest_path, manifest) - for view in pending_views: - selector = VIEW_SELECTORS[view] - await self._select_view(page, selector, view) - # The live UI can expose the first table row before the export - # blob has been rebuilt for the selected phrase/view. A short - # settling interval prevents a header-only CSV from racing the - # table repaint; the structural row gate above remains required. - if view is not WordstatView.REGIONS and self.settling_seconds > 0: - await asyncio.sleep(self.settling_seconds) - if view is WordstatView.DYNAMICS and ( - granularity is not Granularity.MONTHLY or date_from is not None - ): - await self._set_granularity(page, granularity) - if date_from is not None: - await self._set_period(page, granularity, date_from, date_to) - source = await self._download_current_view(page, session, downloads_path) + # issue #27: a failure on one view (e.g. the live escaped-download + # race on regions, or any other InterfaceChangedError/parse failure) + # used to propagate straight out of _collect_one, which collect_many + # then recorded as a whole-phrase PhraseFailure — discarding the + # other views this same phrase had already collected and written to + # disk. view_errors accumulates a message per failed view; the loop + # then `break`s instead of continuing to the next view, because a + # view that failed mid-select/download/parse can leave the page in + # an unpredictable state (stuck popup, half-applied granularity, + # ...) that later views should not be attempted against blindly. + # AuthenticationRequiredError is the one exception NOT caught here: + # it means the whole session is gone, not just this view, and must + # keep propagating so collect_many's per-phrase try/except still + # sees it and breaks the batch (see that method's own comment on + # why it must be checked before the generic Exception branch). + view_errors: dict[WordstatView, str] = {} + escaped_download_warnings: list[str] = [] + last_view_error: Exception | None = None + for view_index, view in enumerate(pending_views): try: - # Convert before disposing of the download, so a parse or - # write failure leaves the raw CSV on disk to inspect. The - # live export blob can lag the table repaint once; retry an - # empty table export once before failing closed. - dataset = parse_wordstat_csv(source, view) - if _is_untrustworthy_empty_export(view, dataset): - if self.empty_export_retry_seconds > 0: - await asyncio.sleep(self.empty_export_retry_seconds) - source = await self._download_current_view(page, session, downloads_path) + selector = VIEW_SELECTORS[view] + await self._select_view(page, selector, view) + # The live UI can expose the first table row before the export + # blob has been rebuilt for the selected phrase/view. A short + # settling interval prevents a header-only CSV from racing the + # table repaint; the structural row gate above remains required. + if view is not WordstatView.REGIONS and self.settling_seconds > 0: + await asyncio.sleep(self.settling_seconds) + if view is WordstatView.DYNAMICS and ( + granularity is not Granularity.MONTHLY or date_from is not None + ): + await self._set_granularity(page, granularity) + if date_from is not None: + await self._set_period(page, granularity, date_from, date_to) + source, escape_warning = await self._download_current_view(page, session, downloads_path) + if escape_warning is not None: + escaped_download_warnings.append(f"[{view.value}] {escape_warning}") + try: + # Convert before disposing of the download, so a parse or + # write failure leaves the raw CSV on disk to inspect. The + # live export blob can lag the table repaint once; retry an + # empty table export once before failing closed. dataset = parse_wordstat_csv(source, view) - if _is_untrustworthy_empty_export(view, dataset): - raise InterfaceChangedError( - f"Wordstat returned an empty {view.value} CSV after a retry, but the page had rendered " - "at least one table row before the download was triggered; export is not trustworthy" + if _is_untrustworthy_empty_export(view, dataset): + if self.empty_export_retry_seconds > 0: + await asyncio.sleep(self.empty_export_retry_seconds) + source, escape_warning = await self._download_current_view(page, session, downloads_path) + if escape_warning is not None: + escaped_download_warnings.append(f"[{view.value}] {escape_warning}") + dataset = parse_wordstat_csv(source, view) + if _is_untrustworthy_empty_export(view, dataset): + raise InterfaceChangedError( + f"Wordstat returned an empty {view.value} CSV after a retry, but the page had " + "rendered at least one table row before the download was triggered; export is " + "not trustworthy" + ) + if view is WordstatView.DYNAMICS: + self._assert_contiguous_dynamics_rows( + dataset, granularity, date_from=date_from, date_to=date_to + ) + file_name = self._dynamics_file_name(granularity) if view is WordstatView.DYNAMICS else None + if file_name is None: + data_path, dtypes = write_dataset(dataset, run_directory) + else: + data_path, dtypes = write_dataset(dataset, run_directory, file_name=file_name) + raw_path = finalize_raw( + source, run_directory, view, self.keep_raw, output_root=self.output_root ) + except Exception: # noqa: BLE001 + # source lives in the batch's shared, temporary downloads + # directory; it would otherwise vanish with that directory + # once the batch finishes. Rescue it into this phrase's own + # run directory for any failure past this point (parsing, + # dtype inference, the parquet write itself), not just + # CsvFormatError, so "the CSV stays on disk to inspect" holds + # regardless of which step failed. + # shutil.move (not Path.replace/os.rename) so this survives + # source and run_directory sitting on different filesystems — + # same reasoning as finalize_raw's own move. source here is + # always the file _download_current_view already confirmed + # lives inside downloads_path (an escaped path raises + # DownloadEscapedError before source is ever bound in this + # scope), so no containment check is needed on this path. + if source.exists(): + shutil.move(str(source), str(run_directory / source.name)) + raise + if view != WordstatView.REGIONS: + self._previous_table_snapshot = await self._table_snapshot(page) + export = ExportSummary( + view=view, + file=data_path.name, + raw_file=raw_path.name if raw_path else None, + row_count=len(dataset.rows), + dtypes=dtypes, + ) + # Rewritten after every view, not just once at the end: an + # interruption between views must leave a manifest that honestly + # reflects the views collected so far (see write_manifest and + # CollectionManifest.status), not a stale one still claiming + # every view is missing. + manifest = merge_export(manifest, export) if view is WordstatView.DYNAMICS: - self._assert_contiguous_dynamics_rows( - dataset, granularity, date_from=date_from, date_to=date_to + manifest = manifest.model_copy( + update={"actual_period": self._actual_period(dataset)} ) - file_name = self._dynamics_file_name(granularity) if view is WordstatView.DYNAMICS else None - if file_name is None: - data_path, dtypes = write_dataset(dataset, run_directory) - else: - data_path, dtypes = write_dataset(dataset, run_directory, file_name=file_name) - raw_path = finalize_raw(source, run_directory, view, self.keep_raw) - except Exception: # noqa: BLE001 - # source lives in the batch's shared, temporary downloads - # directory; it would otherwise vanish with that directory - # once the batch finishes. Rescue it into this phrase's own - # run directory for any failure past this point (parsing, - # dtype inference, the parquet write itself), not just - # CsvFormatError, so "the CSV stays on disk to inspect" holds - # regardless of which step failed. - if source.exists(): - source.replace(run_directory / source.name) + write_manifest(manifest_path, manifest) + except AuthenticationRequiredError: raise - if view != WordstatView.REGIONS: - self._previous_table_snapshot = await self._table_snapshot(page) - export = ExportSummary( - view=view, - file=data_path.name, - raw_file=raw_path.name if raw_path else None, - row_count=len(dataset.rows), - dtypes=dtypes, - ) - # Rewritten after every view, not just once at the end: an - # interruption between views must leave a manifest that honestly - # reflects the views collected so far (see write_manifest and - # CollectionManifest.status), not a stale one still claiming - # every view is missing. - manifest = merge_export(manifest, export) - if view is WordstatView.DYNAMICS: - manifest = manifest.model_copy( - update={"actual_period": self._actual_period(dataset)} - ) - write_manifest(manifest_path, manifest) + except Exception as error: # noqa: BLE001 - per-view isolation is intentional, see comment above + view_errors[view] = f"{type(error).__name__}: {error}" + last_view_error = error + # Every view after this one was never attempted at all (the + # loop breaks, see the comment above this loop) — record + # that explicitly instead of leaving them silently absent + # from both exports and view_errors. Without this, the CLI + # (which falls back to "не собран" for any view in + # missing_views with no view_errors entry) would print the + # same message for "we tried and it genuinely failed" and + # "we never even attempted this view", which reads as a + # false claim that every view was tried. + for skipped_view in pending_views[view_index + 1 :]: + view_errors[skipped_view] = f"не пробовался: сбой на виде {view.value}" + break + + if last_view_error is not None and not manifest.exports: + # Nothing at all was collected for this phrase (the very first + # attempted view already failed, or a --resume-dir run whose + # only remaining view just failed too) — this must still surface + # as a failure, not a "partial success" CollectionResult with + # zero exports. Without this guard, collect_many would count a + # completely failed phrase in batch.results, and the CLI would + # report it as collected. Re-raises the original exception type/ + # message (not a generic wrapper) so collect_many's per-phrase + # except still records the real cause in PhraseFailure.error. + raise last_view_error return CollectionResult( run_directory=run_directory, manifest_path=manifest_path, manifest=manifest, + view_errors=view_errors, + escaped_download_warnings=escaped_download_warnings, ) @staticmethod @@ -1014,13 +1120,33 @@ async def _table_snapshot(self, page) -> str | None: f"() => document.querySelector({json.dumps(TABLE_ROW_SELECTOR)})?.textContent ?? null" ) - async def _download_current_view(self, page, session: BrowserSession, downloads_path: Path) -> Path: + async def _download_current_view( + self, page, session: BrowserSession, downloads_path: Path + ) -> tuple[Path, str | None]: + """Download the CSV for the currently selected view. + + Returns ``(path, escape_warning)``. ``escape_warning`` is ``None`` on + the common path; it carries a message when a stray escaped download + (see ``DownloadEscapedError`` below) was observed in the very same + poll tick as the legitimate CSV (issue #27 follow-up). That case + must not fail the view — the view's own CSV is fine and already on + disk — but the operator still needs to know Chrome dropped a file + outside ``downloads_path`` somewhere. Checking ``new_escaped`` only + applies when no legitimate CSV was found in this tick would silently + lose that signal forever: ``session.downloaded_files`` is + session-lifetime and append-only, so the same path is already inside + next call's ``before_escaped`` baseline and would never show up in a + future ``new_escaped`` diff either — this is not a "check it next + time" gap, the escape is gone from view for good once masked by a + same-tick success. + """ # macOS resolves /tmp to /private/tmp; the downloads directory and # session.downloaded_files can report the same physical file under # different unresolved paths, which would otherwise look like two # distinct downloads. Compare resolved paths, keep the original for # return. before = self._resolved_file_snapshot(downloads_path, session) + before_escaped = self._escaped_download_paths(downloads_path, session) # "Скачать" now opens a format menu (CSV / XLSX) instead of downloading # directly; a second click on the CSV entry is required. await self._click(page, DOWNLOAD_SELECTOR) @@ -1033,10 +1159,64 @@ async def _download_current_view(self, page, session: BrowserSession, downloads_ current = self._resolved_file_snapshot(downloads_path, session) new_resolved = set(current) - set(before) csv_files = [current[resolved] for resolved in new_resolved if resolved.suffix.lower() == ".csv"] + # Evaluated every tick, including the one that finds the + # legitimate CSV: a stray escaped path can land in the exact + # same tick as a good download, and (per the docstring above) + # that escape would never be detected on any later call either + # once masked here — this is the only tick in which it is ever + # observable at all. + new_escaped = self._escaped_download_paths(downloads_path, session) - before_escaped + escape_warning = ( + "Chrome reported a download outside the run's downloads directory " + f"({downloads_path}): {sorted(str(path) for path in new_escaped)}. " + "The file was left untouched; it is not safe to move or delete " + "automatically. Move it manually if it belongs to this run." + if new_escaped + else None + ) if len(csv_files) == 1 and csv_files[0].stat().st_size > 0: - return csv_files[0] + # The view's own download succeeded — a stray escape seen in + # this same tick is reported as a warning, not a failure: the + # data this view needed is safely on disk, and raising here + # would discard it for no reason (see the docstring above for + # why this is the only chance to report the escape at all). + return csv_files[0], escape_warning if len(csv_files) > 1: raise DownloadTimeoutError("Wordstat produced more than one new CSV for a single export") + # Chrome can report a download at a path outside downloads_path + # despite Browser.setDownloadBehavior having been configured for + # this session (issue #27 — observed live on the fourth view of a + # phrase, landing under the real ~/Downloads). Root-cause + # investigated live (CDP :9223, issue #27 fix): every table + # view's export link is `a[download]` with an `href="blob:..."` + # and `target="_self"` — DYNAMICS and REGIONS are structurally + # identical on this point (dumped both live), so a per-target + # blob/`_self` explanation was ruled out; browser-use's own + # Browser.setDownloadBehavior call (downloads_watchdog.py) is + # also browser-level, not per-target, so it should not degrade + # between views either. Several live full 4-view runs (single + # phrase and a 2-phrase batch, both with --keep-raw, one against + # --output-dir inside the repo and one outside it) all completed + # cleanly with every file landing inside downloads_path — the + # escape did not reproduce on demand, meaning it's an + # intermittent Chrome-side race (not deterministically tied to + # "the fourth view" or any specific view), not a bug in how this + # collector configures downloads_path. So this containment check + # can't be "fixed away" upstream; treating the intermittent + # escape as an honest, loud failure instead of a silent + # mistargeted move/delete is the correct and sufficient fix. This + # is never treated as "the" download for this view — that file + # is not ours to move or delete (it may not even be from this + # run) — but it must fail loudly and specifically instead of a + # generic DownloadTimeoutError that leaves the operator guessing + # whether Wordstat ever produced anything at all. + if new_escaped: + raise DownloadEscapedError( + "Chrome reported a download outside the run's downloads directory " + f"({downloads_path}): {sorted(str(path) for path in new_escaped)}. " + "The file was left untouched; it is not safe to move or delete " + "automatically. Move it manually if it belongs to this run." + ) await asyncio.sleep(0.25) raise DownloadTimeoutError("Wordstat did not produce a CSV before the download timeout") @@ -1102,7 +1282,42 @@ def _resolved_file_snapshot(directory: Path, session: BrowserSession) -> dict[Pa file that appears under two unresolved forms (e.g. /tmp vs /private/tmp on macOS) between downloads-directory globbing and session.downloaded_files. + + session.downloaded_files (issue #27) is browser-use's own + session-lifetime log of every CDP downloadWillBegin/downloadProgress + event, not a list scoped to this collector's downloads_path — it can + legitimately (or by a Chrome quirk not yet root-caused, see + _download_current_view's docstring) name a path outside directory + entirely, or a path inside a downloads directory from an earlier, + already-cleaned-up phrase in the same batch. Filtering it down to + paths resolving under directory keeps the escaped-path case fully + out of this snapshot instead of letting set difference "detect" it + as a legitimate new download — the escape is instead surfaced + explicitly by _download_current_view below. """ + resolved_directory = directory.resolve() paths = {path for path in directory.glob("*") if path.is_file()} - paths |= {Path(path) for path in session.downloaded_files} + for reported in session.downloaded_files: + candidate = Path(reported) + if candidate.exists() and candidate.resolve().is_relative_to(resolved_directory): + paths.add(candidate) return {path.resolve(): path for path in paths if path.exists()} + + @staticmethod + def _escaped_download_paths(directory: Path, session: BrowserSession) -> set[Path]: + """Paths session.downloaded_files reports that fall outside directory. + + A companion to _resolved_file_snapshot: that method silently drops + these paths from its result so they are never mistaken for our own + download, but _download_current_view still needs to know they exist + in order to fail loudly (DownloadEscapedError) instead of just timing + out with a confusing "no CSV appeared" when Chrome in fact downloaded + something, just not where it was told to. + """ + resolved_directory = directory.resolve() + escaped = set() + for reported in session.downloaded_files: + candidate = Path(reported) + if candidate.exists() and not candidate.resolve().is_relative_to(resolved_directory): + escaped.add(candidate) + return escaped diff --git a/src/wordstat/errors.py b/src/wordstat/errors.py index 7681db0..df500ff 100644 --- a/src/wordstat/errors.py +++ b/src/wordstat/errors.py @@ -29,6 +29,21 @@ class DownloadTimeoutError(WordstatError): """The UI accepted an export request but did not produce a CSV file.""" +class DownloadEscapedError(WordstatError): + """Chrome reported a download outside the run's own downloads directory. + + session.downloaded_files (browser-use) accumulates every path Chrome has + ever reported as downloaded across the whole CDP session, regardless of + where it actually landed — see issue #27, where the fourth view of a + phrase (regions) was reported at an absolute path under the user's real + ~/Downloads instead of the collector's own temporary downloads_path. That + path must never be moved or deleted (it may be a file the user cares + about, and may not even belong to this tool's run at all): this error is + raised instead, so the file is left untouched and the failure is loud + rather than a silent Errno 1/2 from finalize_raw trying to relocate it. + """ + + class CsvFormatError(WordstatError): """A downloaded file cannot be decoded as a headed CSV report.""" diff --git a/src/wordstat/models.py b/src/wordstat/models.py index 85ea4c6..5e1d72d 100644 --- a/src/wordstat/models.py +++ b/src/wordstat/models.py @@ -152,11 +152,39 @@ class CollectionResult(BaseModel): The parsed rows are deliberately not carried here: they are already on disk as Parquet, and the manifest describes every export. + + ``view_errors`` (issue #27) records why a view present in + ``manifest.missing_views`` did not make it into ``manifest.exports`` — + the manifest itself only carries a view's absence, not the reason + (``DownloadTimeoutError``, ``InterfaceChangedError``, a parse failure, + ...). A phrase where at least one view failed but at least one other + succeeded still comes back as a ``CollectionResult`` (not a batch + failure — see ``collector._collect_one``), so the operator needs + somewhere to read *why* a view is missing without re-running with more + logging. Empty when every requested view was collected successfully. + Keyed by :class:`WordstatView`, not the raw exception object: the error + is only ever read back as text (CLI output), and keeping the exception + itself here would hold its traceback/frames alive for as long as this + result is (same reasoning as ``PhraseFailure.error`` being stripped of + its traceback in ``collector._without_traceback``). + + ``escaped_download_warnings`` (issue #27 follow-up) records a case where + Chrome reported a download outside the run's ``downloads_path`` in the + same polling tick as the legitimate CSV for the current view — the view + itself still succeeded (its own CSV is fine), so this must not fail the + view or the phrase, but the operator still needs to know Chrome dropped + a stray file somewhere it wasn't supposed to (the file itself is never + touched — see ``_escaped_download_paths``/``DownloadEscapedError``). + Plain strings, not keyed by view: a single poll tick can only ever + belong to the view currently being downloaded, so the message names the + view and path together. Empty on every normal run. """ run_directory: Path manifest_path: Path manifest: CollectionManifest + view_errors: dict[WordstatView, str] = Field(default_factory=dict) + escaped_download_warnings: list[str] = Field(default_factory=list) class PhraseFailure(BaseModel): diff --git a/src/wordstat/storage.py b/src/wordstat/storage.py index acf16b0..3b1f599 100644 --- a/src/wordstat/storage.py +++ b/src/wordstat/storage.py @@ -2,11 +2,12 @@ import os import re +import shutil import tempfile from datetime import UTC, datetime from pathlib import Path -from wordstat.errors import ResumeMismatchError +from wordstat.errors import DownloadEscapedError, ResumeMismatchError from wordstat.models import CollectionManifest, ExportSummary, WordstatView @@ -32,19 +33,59 @@ def slugify(value: str) -> str: return compact[:64] or "query" -def finalize_raw(source: Path, run_directory: Path, view: WordstatView, keep_raw: bool) -> Path | None: +def finalize_raw( + source: Path, run_directory: Path, view: WordstatView, keep_raw: bool, output_root: Path | None = None +) -> Path | None: """Dispose of a download once it has been converted to Parquet. By default the raw CSV is removed, leaving only the converted datasets in the run directory. With ``keep_raw`` it is renamed to the view's canonical name so it sits next to its ``.parquet`` counterpart. + + Belt-and-suspenders against issue #27 (a downloaded file that ends up + outside the tool's own downloads_path, e.g. under the user's real + ~/Downloads on macOS): a source that is not inside ``run_directory`` + itself, nor inside ``output_root`` (the batch's shared temporary + downloads directory lives under it — see ``collector.collect_many``), is + refused outright with ``DownloadEscapedError`` instead of being moved or + deleted. ``output_root`` is a caller-supplied parameter rather than + inferred from ``run_directory``'s parents: under ``--resume-dir``, + ``run_directory`` can be an arbitrary user-supplied path that is not + necessarily two levels under ``--output-dir`` at all, and inferring it + would either reject a legitimate download or (worse) widen the allowed + zone unpredictably. Callers that have no ``output_root`` to pass (e.g. + existing tests that only care about the rename/delete behavior) may omit + it, in which case only ``run_directory`` is treated as safe. This is a + second line of defense — collector.py's ``_download_current_view`` + already refuses to hand such a path to this function at all — kept here + in case any future caller reaches this function directly with an + unchecked path. Checked before the ``missing_ok`` unlink path too: a + caller passing ``keep_raw=False`` for a stray path must not silently + delete a file that does not belong to this run. """ + resolved_source = source.resolve() + allowed_roots = [run_directory.resolve()] + if output_root is not None: + allowed_roots.append(output_root.resolve()) + if source.exists() and not any(resolved_source.is_relative_to(root) for root in allowed_roots): + raise DownloadEscapedError( + f"Refusing to move or delete {source} — it is outside the run " + f"directory ({run_directory}) and its output root ({output_root}); " + "it is not safe to assume this tool owns that file." + ) + if not keep_raw: source.unlink(missing_ok=True) return None destination = run_directory / f"{view.value}.csv" - return source.replace(destination) + # shutil.move tries an atomic os.rename first and only falls back to a + # copy+unlink when source/destination sit on different filesystems (e.g. + # --output-dir on a different mount than the batch's shared downloads + # directory) — Path.replace (bare os.rename) raises OSError in that case + # instead of relocating the file. + shutil.move(str(source), str(destination)) + return destination def write_manifest(path: Path, manifest: CollectionManifest) -> None: diff --git a/tests/test_cli.py b/tests/test_cli.py index b925c96..491d00c 100644 --- a/tests/test_cli.py +++ b/tests/test_cli.py @@ -8,7 +8,14 @@ from wordstat import cli from wordstat.cli import main, resolve_phrases from wordstat.errors import PhraseEntryError -from wordstat.models import BatchCollectionResult, CollectionManifest, CollectionResult, PhraseFailure +from wordstat.models import ( + BatchCollectionResult, + CollectionManifest, + CollectionResult, + ExportSummary, + PhraseFailure, + WordstatView, +) def test_empty_phrase_is_reported_without_a_traceback(): @@ -116,6 +123,168 @@ async def fake_collect_many(self, phrases, region="Россия", resume_directo assert "Собрано 1 из 1" in result.output +def _fake_manifest_with_missing_view(phrase: str) -> CollectionManifest: + return CollectionManifest( + phrase=phrase, + region="Россия", + created_at=datetime(2026, 8, 20, 12, 0, tzinfo=UTC), + source_url="https://wordstat.yandex.ru/?words=" + phrase, + exports=[ + ExportSummary( + view=WordstatView.TOP_POPULAR, + file="top_popular.parquet", + raw_file=None, + row_count=0, + dtypes={"Запрос": "string"}, + ), + ExportSummary( + view=WordstatView.TOP_RELATED, + file="top_related.parquet", + raw_file=None, + row_count=0, + dtypes={"Запрос": "string"}, + ), + ExportSummary( + view=WordstatView.DYNAMICS, + file="dynamics.parquet", + raw_file=None, + row_count=12, + dtypes={"Дата": "string"}, + ), + # regions deliberately absent: missing_views == [REGIONS] + ], + ) + + +def test_batch_partial_phrase_exits_zero_and_reports_the_missing_view(monkeypatch, tmp_path: Path): + """Issue #27: a phrase that collected 3 of 4 views must not print + "Собрано 0 из 1" or exit non-zero — it belongs in batch.results (not + batch.failures), same as a fully collected phrase, but the CLI must + still surface which view is missing and why. Deliberately does NOT + assert on manifest.status, which is "incomplete" here for an unrelated + reason (empty_views from the always-empty top_popular/top_related, see + CLAUDE.md/issue #22) — exit code must not be keyed off that field.""" + + async def fake_collect_many(self, phrases, region="Россия", resume_directory=None): + run_directory = tmp_path / "чай" + manifest_path = run_directory / "manifest.json" + return BatchCollectionResult( + total=1, + results=[ + CollectionResult( + run_directory=run_directory, + manifest_path=manifest_path, + manifest=_fake_manifest_with_missing_view("чай"), + view_errors={WordstatView.REGIONS: "DownloadTimeoutError: simulated"}, + ) + ], + failures=[], + ) + + monkeypatch.setattr(cli.WordstatCollector, "collect_many", fake_collect_many) + + result = CliRunner().invoke(main, ["collect", "чай"]) + + assert result.exit_code == 0 + assert "Собрано 1 из 1, из них частично 1" in result.output + assert "чай [regions]: DownloadTimeoutError: simulated" in result.output + + +def _fake_manifest_with_stale_export_for_regions(phrase: str) -> CollectionManifest: + """All four views present in exports, including a *stale* REGIONS entry + (as if a prior run collected it, but the on-disk parquet was later + deleted) — mirrors --resume-dir's views_to_collect re-attempting a view + whose export entry survives but whose file doesn't (see storage.py). With + all four exports present, `missing_views` (computed only from `exports`) + is empty even though this call's re-collection of REGIONS failed.""" + base = _fake_manifest_with_missing_view(phrase) + return base.model_copy( + update={ + "exports": [ + *base.exports, + ExportSummary( + view=WordstatView.REGIONS, + file="regions.parquet", + raw_file=None, + row_count=934, + dtypes={"Регион": "string"}, + ), + ] + } + ) + + +def test_batch_resume_reattempt_failure_is_reported_even_when_missing_views_is_empty( + monkeypatch, tmp_path: Path +): + """Cycle-review follow-up to issue #27: missing_views is a computed field + over manifest.exports only. Under --resume-dir, a view can have a stale + export entry (its parquet was manually deleted, but the manifest still + lists it) — views_to_collect re-attempts such a view, and if that + re-attempt fails, view_errors gets an entry for it but missing_views + stays empty (the stale export entry is still in manifest.exports). Keying + the CLI's partial-report gate on `missing_views` alone would silently + report this run as a full success at exit 0 while the view's parquet is + still absent — the exact "врёт о фактическом результате" failure mode + issue #27 was about. The gate must be the union of missing_views and + view_errors.""" + + async def fake_collect_many(self, phrases, region="Россия", resume_directory=None): + run_directory = tmp_path / "чай" + manifest_path = run_directory / "manifest.json" + return BatchCollectionResult( + total=1, + results=[ + CollectionResult( + run_directory=run_directory, + manifest_path=manifest_path, + manifest=_fake_manifest_with_stale_export_for_regions("чай"), + view_errors={WordstatView.REGIONS: "DownloadTimeoutError: simulated resume re-attempt failure"}, + ) + ], + failures=[], + ) + + monkeypatch.setattr(cli.WordstatCollector, "collect_many", fake_collect_many) + + result = CliRunner().invoke(main, ["collect", "чай"]) + + assert result.exit_code == 0 + assert "Собрано 1 из 1, из них частично 1" in result.output + assert "чай [regions]: DownloadTimeoutError: simulated resume re-attempt failure" in result.output + + +def test_batch_reports_escaped_download_warnings(monkeypatch, tmp_path: Path): + """A view that succeeded despite a same-tick escaped download (cycle- + review follow-up to issue #27) must still surface the warning to the + operator, even though the phrase is otherwise fully collected.""" + + async def fake_collect_many(self, phrases, region="Россия", resume_directory=None): + run_directory = tmp_path / "чай" + manifest_path = run_directory / "manifest.json" + return BatchCollectionResult( + total=1, + results=[ + CollectionResult( + run_directory=run_directory, + manifest_path=manifest_path, + manifest=_fake_manifest_with_stale_export_for_regions("чай"), + escaped_download_warnings=[ + "[regions] Chrome reported a download outside the run's downloads directory: stray.csv" + ], + ) + ], + failures=[], + ) + + monkeypatch.setattr(cli.WordstatCollector, "collect_many", fake_collect_many) + + result = CliRunner().invoke(main, ["collect", "чай"]) + + assert result.exit_code == 0 + assert "чай: [regions] Chrome reported a download outside the run's downloads directory" in result.output + + def test_batch_aborted_early_reports_untried_phrases_distinctly(monkeypatch): async def fake_collect_many(self, phrases, region="Россия", resume_directory=None): # Only the first of 3 phrases was attempted (and failed) before the diff --git a/tests/test_collector_batch.py b/tests/test_collector_batch.py index 544fddd..86ea747 100644 --- a/tests/test_collector_batch.py +++ b/tests/test_collector_batch.py @@ -13,7 +13,13 @@ import wordstat.collector as collector_module from wordstat.collector import WordstatCollector -from wordstat.errors import AuthenticationRequiredError, InvalidRequestError, PhraseEntryError, ResumeMismatchError +from wordstat.errors import ( + AuthenticationRequiredError, + DownloadTimeoutError, + InvalidRequestError, + PhraseEntryError, + ResumeMismatchError, +) from wordstat.models import CollectionManifest, CollectionResult, WordstatView from wordstat.storage import load_manifest @@ -445,7 +451,7 @@ async def download(self, page, session, directory): download_count += 1 source = directory / f"export-{download_count}.csv" _write_view_csv(source, "тест") - return source + return source, None monkeypatch.setattr(WordstatCollector, "_assert_authenticated", noop) monkeypatch.setattr(WordstatCollector, "_set_phrase", noop) @@ -490,7 +496,7 @@ async def fake_download(self, page, session, dl_path): call_count["n"] += 1 source = dl_path / f"export-{call_count['n']}.csv" _write_view_csv(source, "тест") - return source + return source, None def failing_write_dataset(dataset, run_directory): raise RuntimeError("simulated pyarrow.ArrowInvalid") @@ -533,7 +539,7 @@ async def fake_select_view(self, page, selector, view): async def fake_download(self, page, session, dl_path): source = dl_path / "export.csv" _write_view_csv(source, "тест") - return source + return source, None view_calls = {"n": 0} @@ -556,11 +562,191 @@ def flaky_write_dataset(dataset, run_directory): async def run(): page = _FakePage() session = _FakeSession() - with pytest.raises(RuntimeError, match="second view fails"): + return await collector._collect_one(page, session, downloads_path, "тест", "Россия") + + # Issue #27: the first view already succeeded, so this is now a partial + # result (not a raised exception) — the rescue must still not raise a + # FileNotFoundError on top of the second view's original RuntimeError, + # which is what this test guards; that message now surfaces through + # view_errors (the failed view) instead of propagating out of + # _collect_one. The two views after it were never attempted (the loop + # breaks) and get their own distinct "не пробовался" entries. + result = asyncio.run(run()) + assert len(result.view_errors) == 3 + assert "second view fails" in result.view_errors[WordstatView.TOP_RELATED] + for never_attempted in (WordstatView.DYNAMICS, WordstatView.REGIONS): + assert "не пробовался" in result.view_errors[never_attempted] + + +# --- issue #27: a single failing view must not fail the whole phrase ----- + + +def test_collect_one_returns_a_partial_result_when_one_view_fails(monkeypatch, tmp_path): + """3 of 4 views succeed, the 4th (regions) fails with a non-auth error + (DownloadTimeoutError, mirroring issue #27's live symptom). _collect_one + must not raise: it returns a CollectionResult whose manifest honestly + reports the failed view as missing, and whose view_errors carries the + reason — instead of the caller losing 3 successfully collected views and + seeing "Собрано 0 из 1" for a phrase that mostly succeeded.""" + + _patch_common(monkeypatch) + + downloads_path = tmp_path / "downloads" + downloads_path.mkdir() + + async def fake_select_view(self, page, selector, view): + pass + + call_count = {"n": 0} + + async def fake_download(self, page, session, dl_path): + call_count["n"] += 1 + if call_count["n"] == 4: + raise DownloadTimeoutError("simulated: Wordstat never produced a CSV for regions") + source = dl_path / f"export-{call_count['n']}.csv" + _write_view_csv(source, "тест") + return source, None + + monkeypatch.setattr(WordstatCollector, "_select_view", fake_select_view) + monkeypatch.setattr(WordstatCollector, "_download_current_view", fake_download) + + collector = WordstatCollector("cdp", tmp_path, settling_seconds=0, empty_export_retry_seconds=0) + + async def run(): + page = _FakePage() + session = _FakeSession() + return await collector._collect_one(page, session, downloads_path, "тест", "Россия") + + result = asyncio.run(run()) + + assert result.manifest.missing_views == [WordstatView.REGIONS] + assert len(result.manifest.exports) == 3 + assert result.manifest.status == "incomplete" + on_disk = load_manifest(result.manifest_path) + assert on_disk.missing_views == [WordstatView.REGIONS] + assert len(on_disk.exports) == 3 + assert WordstatView.REGIONS in result.view_errors + assert "simulated" in result.view_errors[WordstatView.REGIONS] + + +def test_collect_one_records_untried_views_after_a_non_final_view_fails(monkeypatch, tmp_path): + """When the 2nd of 4 views fails (not the last one), the loop breaks + (see the comment above the view loop) and REGIONS/whichever views come + after are never attempted at all — that must be recorded distinctly + from "this view was tried and it failed", or the CLI's fallback message + for a view with no view_errors entry would misreport an untried view as + a failure with no known cause.""" + + _patch_common(monkeypatch) + + downloads_path = tmp_path / "downloads" + downloads_path.mkdir() + + async def fake_select_view(self, page, selector, view): + pass + + call_count = {"n": 0} + + async def fake_download(self, page, session, dl_path): + call_count["n"] += 1 + if call_count["n"] == 2: + raise DownloadTimeoutError("simulated: top_related never downloaded") + source = dl_path / f"export-{call_count['n']}.csv" + _write_view_csv(source, "тест") + return source, None + + monkeypatch.setattr(WordstatCollector, "_select_view", fake_select_view) + monkeypatch.setattr(WordstatCollector, "_download_current_view", fake_download) + + collector = WordstatCollector("cdp", tmp_path, settling_seconds=0, empty_export_retry_seconds=0) + + async def run(): + page = _FakePage() + session = _FakeSession() + return await collector._collect_one(page, session, downloads_path, "тест", "Россия") + + result = asyncio.run(run()) + + assert len(result.manifest.exports) == 1 # only TOP_POPULAR, the first view + assert result.manifest.missing_views == [ + WordstatView.TOP_RELATED, + WordstatView.DYNAMICS, + WordstatView.REGIONS, + ] + # The view that actually failed carries the real error... + assert "simulated: top_related never downloaded" in result.view_errors[WordstatView.TOP_RELATED] + # ...but DYNAMICS/REGIONS were never even attempted, and must say so + # distinctly rather than reusing the same failure message or being + # silently absent from view_errors altogether. + for never_attempted in (WordstatView.DYNAMICS, WordstatView.REGIONS): + assert "не пробовался" in result.view_errors[never_attempted] + + +def test_collect_one_still_propagates_authentication_loss_mid_phrase(monkeypatch, tmp_path): + """AuthenticationRequiredError must keep propagating out of _collect_one + (not be swallowed into a partial result) — collect_many relies on it to + break the whole batch instead of retrying a dead session phrase by + phrase.""" + + _patch_common(monkeypatch) + + downloads_path = tmp_path / "downloads" + downloads_path.mkdir() + + async def fake_select_view(self, page, selector, view): + pass + + call_count = {"n": 0} + + async def fake_download(self, page, session, dl_path): + call_count["n"] += 1 + if call_count["n"] == 2: + raise AuthenticationRequiredError("simulated: session logged out mid-phrase") + source = dl_path / f"export-{call_count['n']}.csv" + _write_view_csv(source, "тест") + return source, None + + monkeypatch.setattr(WordstatCollector, "_select_view", fake_select_view) + monkeypatch.setattr(WordstatCollector, "_download_current_view", fake_download) + + collector = WordstatCollector("cdp", tmp_path, settling_seconds=0, empty_export_retry_seconds=0) + + async def run(): + page = _FakePage() + session = _FakeSession() + with pytest.raises(AuthenticationRequiredError): + await collector._collect_one(page, session, downloads_path, "тест", "Россия") + + asyncio.run(run()) + + +def test_collect_one_raises_when_every_view_fails(monkeypatch, tmp_path): + """A phrase where nothing at all was collected must not come back as a + "partial success" CollectionResult with zero exports — that would let + the CLI count an entirely failed phrase as collected.""" + + _patch_common(monkeypatch) + + downloads_path = tmp_path / "downloads" + downloads_path.mkdir() + + async def fake_select_view(self, page, selector, view): + pass + + async def fake_download(self, page, session, dl_path): + raise DownloadTimeoutError("simulated: nothing ever downloads") + + monkeypatch.setattr(WordstatCollector, "_select_view", fake_select_view) + monkeypatch.setattr(WordstatCollector, "_download_current_view", fake_download) + + collector = WordstatCollector("cdp", tmp_path, settling_seconds=0, empty_export_retry_seconds=0) + + async def run(): + page = _FakePage() + session = _FakeSession() + with pytest.raises(DownloadTimeoutError): await collector._collect_one(page, session, downloads_path, "тест", "Россия") - # Must raise the original RuntimeError, not a FileNotFoundError from the - # rescue trying to move an already-moved file. asyncio.run(run()) @@ -586,7 +772,7 @@ async def fake_download(self, page, session, dl_path): call_count["n"] += 1 source = dl_path / f"export-{call_count['n']}.csv" _write_view_csv(source, "тест") - return source + return source, None seen_statuses = [] real_write_manifest = collector_module.write_manifest @@ -647,7 +833,7 @@ async def failing_after_first_download(self, page, session, dl_path): raise RuntimeError("simulated interruption") source = dl_path / "export-1.csv" _write_view_csv(source, "тест") - return source + return source, None monkeypatch.setattr(WordstatCollector, "_select_view", fake_select_view) monkeypatch.setattr(WordstatCollector, "_download_current_view", failing_after_first_download) @@ -657,10 +843,14 @@ async def failing_after_first_download(self, page, session, dl_path): async def run_first(): page = _FakePage() session = _FakeSession() - with pytest.raises(RuntimeError, match="simulated interruption"): - await collector._collect_one(page, session, downloads_path, "тест", "Россия") + return await collector._collect_one(page, session, downloads_path, "тест", "Россия") - asyncio.run(run_first()) + # Issue #27: one view succeeding before another fails is now a partial + # result, not a raised exception (see _collect_one's view_errors guard — + # it only re-raises when *no* view was collected at all). + first_result = asyncio.run(run_first()) + assert WordstatView.TOP_RELATED in first_result.view_errors + assert "simulated interruption" in first_result.view_errors[WordstatView.TOP_RELATED] run_directories = [p for p in tmp_path.glob("runs/*") if p.is_dir()] assert len(run_directories) == 1 @@ -677,7 +867,7 @@ async def succeeding_download(self, page, session, dl_path): call_count["n"] += 1 source = dl_path / f"resume-{call_count['n']}.csv" _write_view_csv(source, "тест") - return source + return source, None monkeypatch.setattr(WordstatCollector, "_download_current_view", succeeding_download) @@ -701,6 +891,75 @@ async def run_resume(): assert (run_directory / still_there.file).stat().st_mtime == first_view_mtime +def test_collect_one_resume_prunes_stale_export_when_parquet_is_missing_and_retry_fails(monkeypatch, tmp_path): + """Cycle-review follow-up to issue #27 (round 2): views_to_collect() + re-attempts a view whose .parquet is missing from disk even though + manifest.exports still has a stale ExportSummary for it (e.g. the file + was manually deleted after a prior run). If that re-attempt then fails + too, the stale export entry must have been pruned from manifest.exports + up front — otherwise missing_views (computed from exports only) falsely + reports nothing missing for a view whose data file does not exist, and a + programmatic manifest consumer gets a false "this view is fine" signal.""" + + _patch_common(monkeypatch) + + downloads_path = tmp_path / "downloads" + downloads_path.mkdir() + + async def fake_select_view(self, page, selector, view): + pass + + monkeypatch.setattr(WordstatCollector, "_select_view", fake_select_view) + + async def succeeding_download(self, page, session, dl_path): + source = dl_path / "export.csv" + _write_view_csv(source, "тест") + return source, None + + monkeypatch.setattr(WordstatCollector, "_download_current_view", succeeding_download) + + collector = WordstatCollector("cdp", tmp_path, settling_seconds=0, empty_export_retry_seconds=0) + + async def run_first(): + page = _FakePage() + session = _FakeSession() + return await collector._collect_one(page, session, downloads_path, "тест", "Россия") + + first_result = asyncio.run(run_first()) + assert not first_result.view_errors # all 4 views collected cleanly + run_directory = first_result.run_directory + full_manifest = load_manifest(run_directory / "manifest.json") + assert full_manifest.status == "complete" + + # Simulate a manually-deleted parquet: the manifest still names it in + # exports, but the file itself is gone from disk. + regions_export = next(e for e in full_manifest.exports if e.view == WordstatView.REGIONS) + (run_directory / regions_export.file).unlink() + + # Resume, and this time make the re-attempted view's download fail. + async def failing_download(self, page, session, dl_path): + raise DownloadTimeoutError("simulated: regions re-collection failed on resume") + + monkeypatch.setattr(WordstatCollector, "_download_current_view", failing_download) + + async def run_resume(): + page = _FakePage() + session = _FakeSession() + return await collector._collect_one( + page, session, downloads_path, "тест", "Россия", resume_directory=run_directory + ) + + result = asyncio.run(run_resume()) + + # The stale export entry must be gone: missing_views must name REGIONS, + # not silently omit it because a stale exports entry survived. + assert WordstatView.REGIONS in result.manifest.missing_views + assert WordstatView.REGIONS in result.view_errors + on_disk = load_manifest(run_directory / "manifest.json") + assert WordstatView.REGIONS in on_disk.missing_views + assert not any(e.view == WordstatView.REGIONS for e in on_disk.exports) + + def test_collect_one_resume_directory_rejects_a_different_phrase(monkeypatch, tmp_path): _patch_common(monkeypatch) @@ -713,7 +972,7 @@ async def fake_select_view(self, page, selector, view): async def fake_download(self, page, session, dl_path): source = dl_path / "export.csv" _write_view_csv(source, "чай") - return source + return source, None monkeypatch.setattr(WordstatCollector, "_select_view", fake_select_view) monkeypatch.setattr(WordstatCollector, "_download_current_view", fake_download) @@ -795,7 +1054,7 @@ async def run_first(): async def succeeding_download(self, page, session, dl_path): source = dl_path / "export.csv" _write_view_csv(source, "тест") - return source + return source, None monkeypatch.setattr(WordstatCollector, "_download_current_view", succeeding_download) @@ -843,7 +1102,7 @@ async def fake_select_view(self, page, selector, view): async def fake_download(self, page, session, dl_path): source = dl_path / "export.csv" _write_view_csv(source, "тест") - return source + return source, None monkeypatch.setattr(WordstatCollector, "_select_view", fake_select_view) monkeypatch.setattr(WordstatCollector, "_download_current_view", fake_download) diff --git a/tests/test_download_path_containment.py b/tests/test_download_path_containment.py new file mode 100644 index 0000000..3a8743d --- /dev/null +++ b/tests/test_download_path_containment.py @@ -0,0 +1,174 @@ +"""Issue #27: a download landing outside downloads_path must never be moved +or deleted — it may be a stray path in ~/Downloads, a user's file. + +session.downloaded_files is populated by browser-use from CDP events across +the whole browser session; nothing about it guarantees a path stays inside +the downloads_path this collector actually configured. _resolved_file_snapshot +used to union that list in blindly, so a stray/escaped path was silently +treated as "the new download" and handed to finalize_raw, which then either +crashed trying to replace() across a TCC-protected directory (~/Downloads on +macOS: Errno 1 Operation not permitted) or, worse, without --keep-raw would +have unlink()ed a file the tool has no business touching. +""" + +import asyncio + +import pytest + +from wordstat.collector import WordstatCollector +from wordstat.errors import DownloadEscapedError + + +class _FakePage: + async def evaluate(self, script, *args): + # Both the "click download button" and "wait for CSV menu item" steps + # only need to not raise; the CSV click below is what matters. + return "true" + + +class _FakeSessionWithStrayDownload: + """Starts with no downloads reported; a stray path outside downloads_path + is only added once the CSV menu item is clicked — mirrors the live + issue #27 symptom, where the escaped download only appears after the + export click, not before it.""" + + def __init__(self, stray_path): + self.downloaded_files = [] + self._stray_path = str(stray_path) + + def report_stray_download(self): + self.downloaded_files.append(self._stray_path) + + +def test_download_current_view_refuses_a_file_outside_downloads_path(monkeypatch, tmp_path): + downloads_path = tmp_path / "downloads" + downloads_path.mkdir() + + # Simulates a file Chrome saved to ~/Downloads (or anywhere else outside + # our own downloads_path) instead of honoring downloads_path. + stray_dir = tmp_path / "not-ours" / "Downloads" + stray_dir.mkdir(parents=True) + stray_file = stray_dir / "wordstat_regions.csv" + stray_file.write_text("Регион;Показов\nМосква;100\n", encoding="cp1251") + + collector = WordstatCollector("cdp", tmp_path, timeout_seconds=1, settling_seconds=0) + + session = _FakeSessionWithStrayDownload(stray_file) + + async def click(self, page, selector): + if selector == "button.save-button": + # The second click (on the CSV menu item) is what triggers the + # actual download in the real UI; report the stray path only + # after that click, matching the real event ordering. + return None + session.report_stray_download() + + monkeypatch.setattr(WordstatCollector, "_click", click) + + async def run(): + await collector._download_current_view(_FakePage(), session, downloads_path) + + with pytest.raises(DownloadEscapedError, match=str(stray_file)): + asyncio.run(run()) + + # The stray file must be left exactly where it was — never moved, never + # deleted, regardless of --keep-raw. + assert stray_file.exists() + assert stray_file.read_text(encoding="cp1251") == "Регион;Показов\nМосква;100\n" + + +class _FakeSessionWithSimultaneousStrayDownload: + """Reports the stray path in the SAME tick the legitimate CSV appears — + the click handler drops both files before the first poll iteration runs, + so _resolved_file_snapshot and _escaped_download_paths both see the full + picture on their very first call. Regression coverage for the cycle-review + follow-up to issue #27: session.downloaded_files is session-lifetime and + append-only, so a stray path masked by a same-tick success here would + never surface as new_escaped on any later call either — this is the only + tick in which the escape is observable at all.""" + + def __init__(self, stray_path): + self.downloaded_files = [] + self._stray_path = str(stray_path) + + def report_stray_download(self): + self.downloaded_files.append(self._stray_path) + + +def test_download_current_view_warns_but_succeeds_when_escape_and_legitimate_csv_share_a_poll_tick( + monkeypatch, tmp_path +): + """A stray escaped download that lands in the exact same poll tick as the + view's own legitimate CSV must not fail the view — the view's data is + safely on disk — but the operator must still be told about the stray file + via the returned warning, since this is the only tick in which the escape + is detectable at all (session.downloaded_files never resets, so it would + be silently absorbed into next call's baseline and never surface later).""" + + downloads_path = tmp_path / "downloads" + downloads_path.mkdir() + + stray_dir = tmp_path / "not-ours" / "Downloads" + stray_dir.mkdir(parents=True) + stray_file = stray_dir / "wordstat_regions.csv" + stray_file.write_text("Регион;Показов\nМосква;100\n", encoding="cp1251") + + collector = WordstatCollector("cdp", tmp_path, timeout_seconds=1, settling_seconds=0) + session = _FakeSessionWithSimultaneousStrayDownload(stray_file) + + async def click(self, page, selector): + if selector == "button.save-button": + return None + # Both the legitimate CSV and the stray path land before the first + # poll iteration observes either — same tick, by construction. + legit = downloads_path / "wordstat_top_queries.csv" + legit.write_text("Запрос;Показов\nремонт;1000\n", encoding="cp1251") + session.report_stray_download() + + monkeypatch.setattr(WordstatCollector, "_click", click) + + async def run(): + return await collector._download_current_view(_FakePage(), session, downloads_path) + + path, warning = asyncio.run(run()) + + assert path.name == "wordstat_top_queries.csv" + assert path.read_text(encoding="cp1251") == "Запрос;Показов\nремонт;1000\n" + assert warning is not None + assert str(stray_file) in warning + + # The stray file must still be left exactly where it was. + assert stray_file.exists() + assert stray_file.read_text(encoding="cp1251") == "Регион;Показов\nМосква;100\n" + + +def test_download_current_view_returns_no_warning_on_the_common_path(monkeypatch, tmp_path): + """No escape at all: the warning slot must be None, not an empty string + or some other falsy-but-present sentinel — callers branch on `is not + None`.""" + + downloads_path = tmp_path / "downloads" + downloads_path.mkdir() + + collector = WordstatCollector("cdp", tmp_path, timeout_seconds=1, settling_seconds=0) + + class _FakeSessionNoDownloads: + downloaded_files: list[str] = [] + + session = _FakeSessionNoDownloads() + + async def click(self, page, selector): + if selector != "button.save-button": + (downloads_path / "wordstat_top_queries.csv").write_text( + "Запрос;Показов\nремонт;1000\n", encoding="cp1251" + ) + + monkeypatch.setattr(WordstatCollector, "_click", click) + + async def run(): + return await collector._download_current_view(_FakePage(), session, downloads_path) + + path, warning = asyncio.run(run()) + + assert path.name == "wordstat_top_queries.csv" + assert warning is None diff --git a/tests/test_storage.py b/tests/test_storage.py index f85ecb0..f2811f7 100644 --- a/tests/test_storage.py +++ b/tests/test_storage.py @@ -5,7 +5,7 @@ import pytest -from wordstat.errors import ResumeMismatchError +from wordstat.errors import DownloadEscapedError, ResumeMismatchError from wordstat.models import CollectionManifest, ExportSummary, WordstatView from wordstat.storage import ( create_run_directory, @@ -82,6 +82,68 @@ def test_finalize_raw_tolerates_an_already_missing_download(tmp_path: Path) -> N assert finalize_raw(tmp_path / "gone.csv", tmp_path, WordstatView.DYNAMICS, keep_raw=False) is None +def test_finalize_raw_refuses_a_source_outside_the_run_directory(tmp_path: Path) -> None: + """Issue #27 belt-and-suspenders: even if a caller ever passes a source + path that escaped the collector's own downloads_path (e.g. a stray path + Chrome reported under the user's real ~/Downloads), finalize_raw must + never move or delete it — regardless of keep_raw. The file must be left + exactly where it was and a domain error raised instead of a bare OSError + (Errno 1 on macOS's TCC-protected ~/Downloads) or, worse, a silent + unlink() of a file the tool has no business touching.""" + + output_root = tmp_path / "wordstat-output" + run_directory = output_root / "runs" / "20260821T000000Z-test" + run_directory.mkdir(parents=True) + + # Outside output_root entirely — mirrors a stray download landing under + # the user's real ~/Downloads, unrelated to --output-dir. + outside_dir = tmp_path / "not-ours" + outside_dir.mkdir() + source = outside_dir / "wordstat_regions.csv" + source.write_text("Регион;Показов\n", encoding="cp1251") + + with pytest.raises(DownloadEscapedError, match=str(source)): + finalize_raw(source, run_directory, WordstatView.REGIONS, keep_raw=True) + + assert source.exists() + assert source.read_text(encoding="cp1251") == "Регион;Показов\n" + + # Same guard must hold for keep_raw=False, where the naive behavior + # would have been an outright unlink() of the user's file. + with pytest.raises(DownloadEscapedError, match=str(source)): + finalize_raw(source, run_directory, WordstatView.REGIONS, keep_raw=False) + + assert source.exists() + + +def test_finalize_raw_accepts_a_source_inside_output_root_even_when_run_directory_is_elsewhere( + tmp_path: Path, +) -> None: + """--resume-dir can point at a run_directory that is not nested under + --output-dir at all (see prepare_resume_directory: it never requires + that). A legitimate download sitting inside the batch's shared + downloads directory (under output_root) must still be accepted even + though it is not inside run_directory itself — output_root is an + independently allowed root, not merely a fallback derived from + run_directory's parents.""" + + output_root = tmp_path / "wordstat-output" + downloads_dir = output_root / ".downloads-abc123" + downloads_dir.mkdir(parents=True) + source = downloads_dir / "wordstat_regions.csv" + source.write_text("Регион;Показов\n", encoding="cp1251") + + # An arbitrary resume directory, deliberately NOT under output_root. + run_directory = tmp_path / "elsewhere" / "my-resume-dir" + run_directory.mkdir(parents=True) + + kept = finalize_raw(source, run_directory, WordstatView.REGIONS, keep_raw=True, output_root=output_root) + + assert kept == run_directory / "regions.csv" + assert kept.exists() + assert not source.exists() + + def _manifest( phrase: str = "ремонт квартир", region: str = "Москва",