fix: contain downloads to their own directory, keep partial results (#27) - #28
Merged
Merged
Conversation
) Two defects, both from live evidence (CDP :9223, daily/weekly/monthly, single-phrase and 2-phrase batch runs — see PR body for manifests): 1. session.downloaded_files (browser-use) can report a download at a path outside the collector's own downloads_path — observed live under the real ~/Downloads. finalize_raw then tried to replace()/unlink() that path, either crashing on macOS's TCC-protected ~/Downloads (Errno 1) or, worse, silently deleting a file it doesn't own. _resolved_file_snapshot now only admits downloaded_files entries that resolve inside downloads_path; _download_current_view detects an escaped path and raises a new DownloadEscapedError instead of ever touching it. finalize_raw gets a second, independent containment check (belt and suspenders) against run_directory/output_root. Cross-filesystem moves (finalize_raw, and _collect_one's own CSV rescue block) now use shutil.move instead of Path.replace (bare os.rename), which raises OSError across filesystems. Root-caused live: DYNAMICS and REGIONS use structurally identical blob:/target=_self export links, and several full 4-view live runs (in-repo and outside --output-dir, single phrase and batch) all completed cleanly — the escape is an intermittent Chrome-side race, not tied to a specific view or to how downloads_path is configured. Treating it as a loud, contained failure is the correct fix, not a workaround for something fixable upstream. 2. A single failing view (e.g. the above race on regions) used to fail the whole phrase, discarding views already collected and printing "Собрано 0 из 1" even when 3 of 4 were on disk with a valid manifest. _collect_one now catches a per-view failure (except AuthenticationRequiredError, which still aborts the whole batch — the session itself is gone) and returns a partial CollectionResult instead of raising, with a new view_errors field recording why each missing view is missing (including "не пробовался" for views skipped after an earlier one failed, not just the one that actually errored). A phrase where nothing at all was collected still raises, so the CLI can't count a fully failed phrase as a result. CLI output/exit code now key off missing_views + view_errors, not manifest.status (status is "incomplete" whenever top_popular/top_related are empty, which is every live run — see issue #22/#25 — so keying on it would fail every successful run again). Tests: 159 passed (was 151 on main), ~0.5s warm — no regression (#23). ruff clean. Manual mutation checks on finalize_raw's containment guard and _collect_one's view_errors guards/break path (mutmut 3.x would not resolve this project's src-layout package correctly in this sandbox — each mutant was applied and reverted by hand instead, confirmed killed by the relevant test each time). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VTU6JGjjt2cZmH5ojqR9w4
Owner
Author
🔍 Local review (cycle 1) — round 709c8db2-a74d-4202-9aee-13a7d01afba7Reviewed locally (
Codex companion: verdict |
Cycle-review follow-up to PR #28 (issue #27): 1. _download_current_view now checks new_escaped on every poll tick, including the one that finds the legitimate CSV, not only inside the "nothing found yet" branch. session.downloaded_files is session-lifetime and append-only, so a stray escaped path masked by a same-tick success was never merely deferred to the next call — it was permanently absorbed into that call's before_escaped baseline and never surfaced again. The view's own successful download still returns normally (its data is fine); the escape is now reported as a non-fatal warning (CollectionResult.escaped_download_warnings) instead of being silently lost. 2. cli.py's partial-report gate now unions manifest.missing_views with view_errors instead of keying off missing_views alone. missing_views is a computed field over manifest.exports only; under --resume-dir a view can retain a stale export entry (its parquet manually deleted, its re-collection attempted and failed) — view_errors gets an entry for it but missing_views stays empty, so the CLI was reporting a run with a missing parquet as a full success at exit 0. This is the same "врёт о фактическом результате" failure mode issue #27 was about, from the opposite direction. Both were found and verified during this PR's local cycle-review (built-in /review + Codex companion, verdicts posted to the PR). Codex: approve, 0 findings. /review flagged a third, unreachable-in-practice TOCTOU gap in finalize_raw's containment guard — left as SKIP (documented in the PR comment), not worth the complexity for a path that cannot occur in the current call graph. Tests: 163 passed (was 159 before this commit) — mutation-verified by manually reverting each fix and confirming the new guard test fails for the right reason. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VTU6JGjjt2cZmH5ojqR9w4
Owner
Author
🔍 Local review (cycle 2) — round f47c0909-c3bc-4397-b194-8ce840a13cf6Reviewed locally (
|
Cycle-review round 2 follow-up (Codex companion, high confidence): views_to_collect() re-attempts a view whose <view>.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 failed too, the stale export entry was never removed — missing_views (a computed field over exports only) fell back to falsely reporting nothing missing for that view, letting a programmatic manifest consumer trust a "this view is fine" signal for a file that doesn't exist. Root-fixed by pruning any stale export entries for pending_views up front, before the retry loop runs — not only after a failed retry — so a crash/Ctrl-C mid-retry also leaves an honest manifest, consistent with this method's existing "write after every step" invariant. Other round-2 findings triaged as SKIP/IRRELEVANT/HALLUCINATION (posted to the PR): a cosmetic duplicate string build, a defensive-only rescue-block concern with no live bug, and a false claim about shutil.move vs Path.replace overwrite semantics (both silently replace an existing destination on POSIX — verified empirically). Tests: 164 passed (was 163) — mutation-verified by reverting the prune and confirming the new resume test fails for the right reason (status stayed "complete" with an empty missing_views despite the deleted parquet). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VTU6JGjjt2cZmH5ojqR9w4
Owner
Author
🔍 Local review (cycle 3, final) — round 9d343843-6b5d-476b-9181-28078ea3a597Reviewed locally (
Итог: 0 FIX в 3-м (финальном) цикле — дальнейшие циклы не требуются. 164 теста зелёных, |
Owner
Author
📋 Review summary — all cyclesЛокальное ревью (
Итого: 2 FIX (оба устранены), 7 SKIP, 1 IRRELEVANT, 2 HALLUCINATION, 1 APPROVE (Codex round 1). 164/164 теста зелёных, |
Owner
Author
✅ Живая верификация (CDP :9223)После завершения 3 циклов ревью проведена обязательная живая проверка всех грануляций против исправленного кода (
Все Прогон №4 — целевая проверка round-2 фикса ( 164/164 теста зелёных, |
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.
Fixes #27.
Дефект 1 — скачанный CSV мог уйти мимо
downloads_pathsession.downloaded_files(browser-use) — журнал загрузок за всю CDP-сессию,не ограниченный
downloads_path._resolved_file_snapshotраньше объединялего вслепую с содержимым каталога, так что "сбежавший" путь (например под
реальным
~/Downloads) детектировался как легитимная новая загрузка ипередавался в
finalize_raw, гдеPath.replace/unlinkлибо падал(
Errno 1 Operation not permitted, TCC на macOS), либо (без--keep-raw)тихо удалил бы чужой файл.
Правки:
_resolved_file_snapshotтеперь допускает только пути, реальнорезолвящиеся внутрь
downloads_path._download_current_viewдетектирует "сбежавший" путь и кидает новыйDownloadEscapedError, ничего не трогая на диске.finalize_rawполучил независимую вторую проверку (belt-and-suspenders)против
run_directory/output_root.finalize_rawи rescue-блок в_collect_oneиспользуютshutil.moveвместоPath.replace(
os.renameпадает между файловыми системами).Root cause (живая диагностика, CDP :9223): экспортные ссылки
dynamicsи
regionsструктурно идентичны (blob:+target=_self), и несколькополных 4-видовых живых прогонов (одна фраза и батч из двух,
--output-dirи внутри репозитория, и в scratchpad вне его) прошли чисто — эскейп не
воспроизвёлся детерминированно. Это нестабильная гонка на стороне Chrome,
не привязанная к конкретному виду и не следствие того, как настроен
downloads_pathв этом коде — поэтому фикс не пытается "починить" самугонку, а делает код устойчивым к её последствиям.
Дефект 2 — сбой одного вида обнулял всю фразу
_collect_oneтеперь ловит ошибку одного вида (кромеAuthenticationRequiredError— та по-прежнему прерывает весь батч, сессияцеликом мертва) и возвращает частичный
CollectionResultвместо исключения.Новое поле
view_errorsобъясняет причину каждого пропущенного вида,включая явное
"не пробовался: сбой на виде X"для видов, следующих за тем,что упал (цикл видов останавливается — состояние DOM после сбоя
непредсказуемо). Фраза, где не собрано вообще ничего, по-прежнему кидает
исключение — иначе CLI засчитал бы полностью провалившуюся фразу как
результат.
CLI больше не завязан на
manifest.status(он"incomplete"всегда, когдаempty_viewsнепуст — аtop_popular/top_relatedпустые на каждом живомпрогоне, issue #22/#25) — критерий теперь
missing_views+view_errors.Тесты
pytest: 159 passed, ~0.5s (было 151 на main) — без регрессии (#23).ruff check .: чисто.Mutmut 3.x не смог корректно резолвить src-layout пакет в этой песочнице
(конфликт с editable-инсталлом соседнего чекаута) — вместо автоматического
прогона мутации внесены и откачены вручную для ключевых веток
(
finalize_raw's containment guard,_collect_one's"ничего не собрано → raise" guard,
AuthenticationRequiredErrorre-raise) —каждая убита релевантным тестом.
Живая верификация (CDP :9223, порт 9222 не трогался)
--granularity daily(--date-from 2026-06-23 --date-to 2026-08-20),--keep-raw,--output-dirвне репозитория — все 4 вида собраны,regions.parquet: 934 строки.--output-dir ./wordstat-output(внутри репозитория, как висходном issue) — все 4 вида.
--granularity weekly, без--keep-raw— все 4 вида, только.parquetна диске (никаких временных
.csv).--granularity monthly(по умолчанию),--keep-raw— все 4 вида.--granularity daily,--keep-raw) — оба manifest.jsonс
missing_views: [].~/Downloadsне трогался ни разу (не удалялся и не переносился) — во всехпрогонах не появилось ни одного эскейпнутого пути; сам список
~/Downloadsпроверить
lsне удалось (TCC блокирует доступ из этой сессии), поэтомуутверждение ограничено «код ни разу не получил путь вне
downloads_pathи, соответственно,
finalize_rawне тронул ничего постороннее».Эскейп-сценарий (файл вне
downloads_path) воспроизведён и проверен толькоюнит-тестом с фейковой session — живой прогон его не поймал (см. root cause
выше про нестабильность гонки).
PR не мержится.