From b113bc1062ece62cc8f61a20602f0616ccb58b67 Mon Sep 17 00:00:00 2001 From: jehrr <79864894+jehrr@users.noreply.github.com> Date: Thu, 15 Oct 2026 17:48:53 +0000 Subject: [PATCH 1/2] Report parser failures and blocks honestly, cap paid solves per page, and write output atomically - A rendered listing grid that parses to nothing is a parser failure (exit 5, or 6 with other rows), not an empty search (exit 4); the Scraper API path uses the same page verdicts. - One paid captcha solve per page across attempts, rotations and markers, checked before purchase in all three engines. - JSON/CSV/sidecar written via temp file + os.replace, modes preserved; formula-shaped CSV text cells escaped and counted. - Negative --retries/--delay and zero --pages/--concurrency rejected; --proxy with --cdp-endpoint refused; inert --captcha-api removed. - Credential scan reads .json/.csv/.html/.cfg fixtures and detects venvs by pyvenv.cfg; refuses to report an unscanned tree clean. - Canary fails, naming ZIMMO_PROXY, instead of skipping green. - Public files no longer cite private maintainer notes; a suite check keeps it that way. Co-Authored-By: Claude Opus 5.5 --- .dockerignore | 2 +- .github/workflows/canary.yml | 61 ++-- .github/workflows/tests.yml | 10 +- .gitignore | 6 +- CHANGELOG.md | 73 +++- CONTRIBUTING.md | 5 +- Dockerfile | 14 +- README.md | 59 +-- captcha_solver.py | 39 +- check_no_credentials.py | 70 +++- diff_runs.py | 4 +- env_config.py | 51 ++- fingerprint_client.py | 29 +- output_writer.py | 171 +++++++-- playwright_scraper.py | 64 ++-- product_parser.py | 7 +- proxy_pool.py | 2 +- puppeteer_scraper.py | 67 ++-- pyproject.toml | 4 +- requirements-playwright.txt | 2 +- requirements.txt | 2 +- scraper_api_client.py | 60 +++- selenium_scraper.py | 63 ++-- smoke_test.py | 479 ++++++++++++++++++++++--- tests/test_cdp_connection.py | 2 +- tests/test_concurrency.py | 4 +- tests/test_concurrency_async.py | 2 +- tests/test_family_sync.py | 2 +- tests/test_fingerprint_kwargs.py | 2 +- tests/test_paid_solves_and_verdicts.py | 313 ++++++++++++++++ tests/test_run_contract.py | 2 +- tests/test_smoke.py | 2 +- 32 files changed, 1371 insertions(+), 302 deletions(-) create mode 100644 tests/test_paid_solves_and_verdicts.py diff --git a/.dockerignore b/.dockerignore index 42a23fc..6e33cf5 100644 --- a/.dockerignore +++ b/.dockerignore @@ -14,4 +14,4 @@ __pycache__/ .venv*/ README.md CHANGELOG.md -CLAUDE.md +*.md diff --git a/.github/workflows/canary.yml b/.github/workflows/canary.yml index 6fd4a9b..dc0e28d 100644 --- a/.github/workflows/canary.yml +++ b/.github/workflows/canary.yml @@ -1,10 +1,16 @@ name: canary -# One real run daily against a known-good FILTERED CATEGORY URL -# (CLAUDE.md §11). Requests >=3 pages -- with one page, pagination is -# never exercised, which is exactly how a silent-single-page bug would -# survive undetected. Uploads artefacts on every run so a red canary can -# be diagnosed from the log alone. +# One real run daily against a known-good search URL, >=3 pages, so +# pagination is exercised and not only page 1. Artefacts are uploaded on +# every run, so a red canary can be diagnosed from the run alone. +# +# This canary ends green ONLY when the scraper read real listings from +# zimmo.be. zimmo.be refuses GitHub's datacentre addresses (Cloudflare +# 403, measured 2026-09-23 and 2026-10-13) and serves only a residential +# exit (Belgian or German worked; a US exit was refused) with a headful +# browser, so the run needs the ZIMMO_PROXY repository secret. Without +# it the job FAILS with an error naming the secret: a skipped or +# "refused, as expected" run is not a test of the site. on: schedule: @@ -18,40 +24,22 @@ on: - ".github/workflows/canary.yml" jobs: - # `secrets` is not available in a job-level `if`, so presence is tested - # here and handed on as an output. The canary job below is then SKIPPED - # (shown as skipped, not as a green run) when no ZIMMO_PROXY is set: a - # run from the runner's own address is refused by Cloudflare every time - # (README, "What you need"), so running it would test nothing and - # reporting it as a pass would be a claim nobody measured. - gate: - runs-on: ubuntu-22.04 - env: - HAS_ZIMMO_PROXY: ${{ secrets.ZIMMO_PROXY != '' }} - outputs: - has_proxy: ${{ steps.g.outputs.has_proxy }} - steps: - - id: g - run: | - echo "has_proxy=$HAS_ZIMMO_PROXY" >> "$GITHUB_OUTPUT" - if [ "$HAS_ZIMMO_PROXY" != "true" ]; then - echo "::notice::canary SKIPPED, not passed: no ZIMMO_PROXY secret is set, and zimmo.be refuses this runner's address. The site needs a residential exit plus a headful browser (README, 'What you need'). Add a residential proxy as the ZIMMO_PROXY repository secret to make this canary test the scraper daily." - fi - canary: - needs: gate - if: needs.gate.outputs.has_proxy == 'true' # jammy, pinned, for the same reason tests.yml's engine-smoke job # pins it: playwright==1.44.0 cannot install its system deps on # Ubuntu 24.04, and the Dockerfile's base image is v1.44.0-jammy. runs-on: ubuntu-22.04 - # What zimmo.be lets through, measured 2026-09-23 (README, "What you - # need"): a residential exit AND a visible (headful) browser. Headless - # Chromium is refused even from a Belgian home address, and a - # datacentre address is refused either way. - env: - HAS_ZIMMO_PROXY: "true" steps: + - name: Require the ZIMMO_PROXY secret + env: + ZIMMO_PROXY: ${{ secrets.ZIMMO_PROXY }} + run: | + if [ -z "$ZIMMO_PROXY" ]; then + echo "::error::ZIMMO_PROXY is missing or empty, so this canary cannot reach zimmo.be (it refuses GitHub runners' addresses). Set the ZIMMO_PROXY repository secret to a residential proxy URL with a Belgian or German exit, e.g. http://user:pass@host:port. This run tested nothing." + exit 1 + fi + echo "[canary] ZIMMO_PROXY is set (value masked)." + - uses: actions/checkout@v4 - uses: actions/setup-python@v5 @@ -66,9 +54,8 @@ jobs: - name: Run against a known-good filtered category, >=3 pages (headful, under xvfb) env: - # The proxy goes through the ENVIRONMENT, never a command line - # (CLAUDE.md §3, §11). An unset secret arrives as an empty - # string, which env_config.py treats as unset. + # The proxy goes through the ENVIRONMENT, never a command line, + # so it cannot appear in the process list or the job log. ZIMMO_PROXY: ${{ secrets.ZIMMO_PROXY }} run: | set +e @@ -91,7 +78,7 @@ jobs: code = int(os.environ.get("exit_code", "1")) if code != 0: - print(f"::error::engine exit code {code} (0 ok, 3 blocked, 4 zero rows, 5 content never obtained, 6 partial)") + print(f"::error::engine exit code {code} (0 ok, 3 blocked, 4 zero rows, 5 content never obtained or parser found nothing on a rendered page, 6 partial)") sys.exit(1) meta = json.load(open("canary_run.meta.json")) diff --git a/.github/workflows/tests.yml b/.github/workflows/tests.yml index 8184638..8606114 100644 --- a/.github/workflows/tests.yml +++ b/.github/workflows/tests.yml @@ -1,6 +1,6 @@ name: tests -# Offline only (CLAUDE.md §11) — nothing here touches zimmo.be or a +# Offline only — nothing here touches zimmo.be or a # credential. See canary.yml for the one real live run. on: push: @@ -13,7 +13,7 @@ jobs: strategy: matrix: # Oldest and newest supported Python — claiming 3.9 without - # testing it is how a walrus operator ships (CLAUDE.md §11). + # testing it is how a walrus operator ships. python-version: ["3.9", "3.12"] steps: - uses: actions/checkout@v4 @@ -107,7 +107,7 @@ jobs: python-version: "3.12" # One venv per engine, installed from its OWN requirements file - # (CLAUDE.md §11) — a single shared environment lets pip silently + # — a single shared environment lets pip silently # resolve the family's mutually-unsatisfiable pins by reaching # for whatever it can, which stays green against a version # nobody ships. @@ -141,7 +141,7 @@ jobs: docker-build: runs-on: ubuntu-latest # "Build the Docker image in CI. Nothing else does, which is how - # all three [sibling] repos shipped a broken one." (CLAUDE.md §11) + # all three [sibling] repos shipped a broken one." # Four steps, each catching a DISTINCT failure a manual COPY-list # review cannot: the image builds; its entrypoint actually runs; # Chromium actually launches (not just `playwright install @@ -173,7 +173,7 @@ jobs: run: | docker create --name zimmo-check zimmo-scraper:ci --help FOUND=0 - for forbidden in .env tests sample_output.json sample_output.csv README.md CLAUDE.md; do + for forbidden in .env tests sample_output.json sample_output.csv README.md CHANGELOG.md CONTRIBUTING.md; do if docker cp zimmo-check:/app/$forbidden /tmp/should-not-exist 2>/dev/null; then echo "FORBIDDEN: /app/$forbidden exists inside the image" rm -rf /tmp/should-not-exist diff --git a/.gitignore b/.gitignore index ede79c8..62fb14c 100644 --- a/.gitignore +++ b/.gitignore @@ -1,5 +1,5 @@ # Scraper output — large, machine-specific, and stale by the time anyone -# reads it. Sample files are the one deliberate exception (CLAUDE.md §2). +# reads it. Sample files are the one deliberate exception. *.json *.csv !sample_output.json @@ -11,7 +11,7 @@ zimmo_listings.* # Local diagnostics / live-test working directory — and ANY run directory by # kind, not one chosen name (a sibling committed a raw dump through `live/`), -# plus paged dumps like X.page3 (family template §24, §26). +# plus paged dumps like X.page3. live_results/ live/ out/ @@ -69,6 +69,6 @@ secrets* # Every spelling of a working .env, not just `.env`: a `.env.bak` or # `.env.local` is exactly how a live key gets one `git add -A` from a commit -# (family template §22). The negation keeps the documented example tracked. +#. The negation keeps the documented example tracked. .env* !.env.example diff --git a/CHANGELOG.md b/CHANGELOG.md index 88859fc..c690371 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,75 @@ manage. A patch release means "fixes" — not that every flag is frozen, so a behaviour-changing default landing in a patch is stated plainly here rather than treated as a violation of the format. +## [Unreleased] + +> **Exit codes.** A page whose listing grid rendered but parsed to zero +> rows is now a parser failure: exit 5 when no row was recovered, exit 6 +> (`partial`, `parser_failed_pages` in the sidecar) when other pages had +> rows. It used to be reported as an empty search, exit 4. A run with no +> rows where one page was served empty and another failed is now exit 5, +> not 4, and the Scraper API path now answers 4 for a served empty page +> and 3 for a Cloudflare challenge (both were 5). +> +> **Rejected flag values (exit 2).** `--retries`, `--delay` and +> `--retry-delay` below 0, and `--pages`, `--concurrency` and +> `--proxy-block-retries` below 1, are argparse errors in all three +> engines (`--retries -1` used to run no navigation at all and report exit +> 5; `--retries 0` still means one attempt). `--proxy`/`--proxy-file` +> together with `--cdp-endpoint` (from flags or `.env`) is refused instead +> of the proxy being silently ignored. `--captcha-api` is removed: nothing +> ever read it. +> +> **CSV cells and file modes.** In the CSV only, a text cell starting with +> `=`, `+`, `-`, `@`, a tab, CR or LF gets a leading apostrophe so a +> spreadsheet does not run it as a formula; numbers and the JSON are +> unchanged, and the sidecar counts the escaped cells in +> `csv_cells_escaped`. Output files keep an existing file's mode; a new one +> gets what the umask allows, as before. + +### Fixed + +- Output files (`.json`, `.csv`, `.meta.json`) are written to a temporary + file in the same directory and moved into place only when complete, so a + failed or interrupted write leaves the previous good file intact instead + of truncated. +- One page buys at most one paid captcha solve, however many attempts, + proxy rotations or challenge markers it goes through. The allowance is + checked before each purchase in all three engines; a missing key or an + unsolvable Managed Challenge costs nothing and does not use it up. +- `currency` is null when nothing states one (it defaulted to `EUR` on the + embedded-JSON and JSON-LD paths, which today's pages do not use). +- The credential scan now also reads `.json`, `.csv`, `.html` and `.cfg` + files, recognises a virtualenv by its `pyvenv.cfg` under any name, + judges the `sample_output` exemption by file name rather than by the + whole path, and refuses to report a tree clean when it read no file. +- README: the Scraping Browser credential lifetime is no longer stated as + "about a day"; an HTTP 401 on connect is described as credentials no + longer accepted. The Docker image is documented as needing + `--cdp-endpoint` or a display, and the Dockerfile no longer claims a CI + step that did not exist (the offline suite now checks its COPY list). +- Comments, docs and tests no longer cite private maintainer notes; each + rule is stated in place. The offline suite fails if such a citation + returns. + +### Changed + +- The canary no longer skips green: without a usable `ZIMMO_PROXY` secret + (missing or empty) it fails with an error naming it, because zimmo.be + refuses GitHub's runners and a run that never reached the site tested + nothing. Any nonzero exit (block, zero rows, parser failure, partial) + is red. + +### Not changed + +- Challenge markers (`turnstile`, `cf-turnstile-response`) that a Scraping + Browser session may inject: changing them needs real served and refused + captures over `--cdp-endpoint`, which this change did not take. +- The family flags `--fingerprint`, `--fp-*`, `--locale` and `--mode`, and + whether to keep `fingerprint_client.py`: an owner decision. +- The three per-engine fetch loops (no shared-loop refactor); version, + tag and release of 1.3.0. + ## [1.3.0] — a published row schema, three columns removed, a canary that can say "not run" (2026-10-13) > **Output change.** `rating`, `review_count` and `in_stock` are gone from @@ -411,7 +480,7 @@ fixed, and each has its own regression test in `smoke_test.py`. confirmed live via the proxy provider's own usage dashboard, not just this project's own output. -## [0.3.0] — External audit fixes (codex.md, 2026-09-14) +## [0.3.0] — External audit fixes (2026-09-14) An independent audit ran this project against the live site and its own test suite and found ten confirmed defects, several severe. Every @@ -573,7 +642,7 @@ patched on the audit's word alone — see each item for how. match the family default. JSON-LD parsing now also handles `@graph` and an `image` field shaped as an `ImageObject` dict. `SELECTORS`' key renamed from an ad-hoc `item_link_css_approx` to the family's own - `item_link` convention (CLAUDE.md §5). + `item_link` convention. - `env_config.py`: a value containing `{...}` (a vendor's own braced placeholder, e.g. `ws://{login}-zone-...`) is now treated as unset, not just a fixed literal list — a copied `.env.example` following the diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 62ace10..9738f58 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -2,8 +2,9 @@ Thanks for considering a contribution. This repo is part of the [2scraper](https://github.com/2scraper) family — several single-site -scrapers sharing one architecture (`~/2scraper/CLAUDE.md`, not committed -to this repo, describes it in full for maintainers). +scrapers sharing one architecture: shared modules with no site knowledge +(`output_writer.py`, `env_config.py`, `proxy_pool.py`, `captcha_solver.py`) +and the site itself confined to `product_parser.py`. ## Before you start diff --git a/Dockerfile b/Dockerfile index c88e184..91b31ed 100644 --- a/Dockerfile +++ b/Dockerfile @@ -1,10 +1,12 @@ # zimmo-scraper — Playwright edition, containerised. # -# Explicit COPY list, not `COPY . .` (CLAUDE.md §11): the image should -# carry no test suite, no fixtures and no stray .env. The list below is -# checked against playwright_scraper.py's own import graph by this -# repo's docker-build CI job (via a real `ast` walk, not a manual -# review) -- keep the two in sync by hand. +# Explicit COPY list, not `COPY . .`: the image should +# carry no test suite, no fixtures and no stray .env. smoke_test.py +# checks the list below against playwright_scraper.py's own local import +# graph (an `ast` walk), so a missing module fails the offline suite. +# +# The default run is headless, and zimmo.be refuses headless Chromium: +# use the image with --cdp-endpoint, or give it a display and --headful. FROM mcr.microsoft.com/playwright/python:v1.44.0-jammy @@ -18,7 +20,7 @@ RUN pip install --no-cache-dir -r requirements.txt -r requirements-playwright.tx COPY env_config.py output_writer.py product_parser.py captcha_solver.py \ proxy_pool.py playwright_scraper.py ./ -# The exact invocation a missing module breaks (CLAUDE.md §11) -- CI's +# The exact invocation a missing module breaks -- CI's # docker-build job actually runs this, rather than trusting that the # image "should" work. ENTRYPOINT ["python3", "playwright_scraper.py"] diff --git a/README.md b/README.md index ae5f67c..615ab6a 100644 --- a/README.md +++ b/README.md @@ -126,9 +126,10 @@ ZIMMO_CDP_ENDPOINT=ws://{login}-zone-scraping_browser-country-be-pid-{profileId} python3 playwright_scraper.py --url "https://www.zimmo.be/nl/gent-9000/te-koop/" --pages 3 ``` -A profile's credentials expire (about a day in this family's -experience): an HTTP 401 `deny_no_user` on connect means a stale profile, -not a broken scraper, and the run reports it as exit 5. +A profile's credentials can stop being valid (no fixed lifetime is +promised here): an HTTP 401 `deny_no_user` on connect usually means the +endpoint's credentials are no longer accepted, not a broken scraper, and +the run reports it as exit 5. Check the endpoint before changing code. Reuse the same `pid` across runs rather than minting a new one each time — the Scraping Browser API caps live connections per profile, and a @@ -156,9 +157,9 @@ python3 scraper_api_client.py --url "https://www.zimmo.be/nl/gent-9000/te-koop/" |---|---|---| | `--url` | — (or `ZIMMO_URL`) | A zimmo.be search-results URL | | `--category` | none | Label written into every row | -| `--pages` | 1 | Number of result pages | +| `--pages` | 1 | Number of result pages (1 or more) | | `--delay` | 0.0s | Sleep this long after each successfully-fetched page (rate limiting) | -| `--retries` / `--retry-delay` | 2 / 3.0s | Per-page retry budget | +| `--retries` / `--retry-delay` | 2 / 3.0s | Extra attempts per page (0 = one attempt; negative values are refused) and the wait between them | | `--concurrency` | 1 | Parallel workers for pages 2+ (page 1 always fetched alone; refused together with `--cdp-endpoint`) | | `--format` | both | `json`, `csv`, or `both` | | `--out` | `zimmo_listings` | Output file prefix | @@ -168,7 +169,7 @@ python3 scraper_api_client.py --url "https://www.zimmo.be/nl/gent-9000/te-koop/" | `--twocaptcha-key` | none (or `TWOCAPTCHA_KEY`) | Needed only if Cloudflare/reCAPTCHA actually appears | | `--solve-captcha` | `when-blocked` | `always` solves on every detection; `when-blocked` only when the page has no usable content yet; `never` detects but never solves | | `--min-score` | solver default (0.7) | Minimum reCAPTCHA v3 score to request | -| `--cdp-endpoint` | none (or `ZIMMO_CDP_ENDPOINT`) | Connect to an existing browser instead of launching one | +| `--cdp-endpoint` | none (or `ZIMMO_CDP_ENDPOINT`) | Connect to an existing browser instead of launching one; refused together with `--proxy`/`--proxy-file`, since that browser uses its own exit | | `--allow-empty` | off | Write output even if zero products were found | | `--dump-html` | off | Always save a per-page debug `.html`/`.png`, including on success | | `--headless` / `--headful` | headless | Toggle a visible browser window | @@ -231,14 +232,24 @@ does not serve is HTTP 404 ("Pagina niet gevonden"); 429 and 5xx are retried up to `--retries` times and then count as failed. A page number past the end of a listing is HTTP 200 carrying page 1 again — the run recognises that by its data (no new listing) and stops there, -`complete`. A failed or blocked page lands in `failed_pages`; if it is -page 1, no later page is requested. +`complete`. A page whose listing grid rendered but parsed to zero rows is +a **parser failure** (`parser_failed_pages`), never an empty search. A +failed, blocked or parser-failed page lands in `failed_pages`; if it is +page 1, no later page is requested. One page buys at most one paid +captcha solve, across all its attempts and proxy rotations. Exit codes, identical across all engines: `0` ok · `1` crash · `2` bad usage · `3` blocked · `4` zero products (no `--allow-empty`) · `5` the content was never obtained (a remote API error, a dead proxy, a -load timeout, an HTTP error or 404 on page 1) · `6` partial (some pages -failed or were blocked). +load timeout, an HTTP error or 404 on page 1, or a rendered page the parser +read nothing from) · `6` partial (some pages failed, were blocked or +parsed to nothing). The Scraper API path uses the same page verdicts. + +In the CSV, a text cell starting with `=`, `+`, `-`, `@`, a tab, CR or LF +is prefixed with an apostrophe so a spreadsheet does not run it as a +formula (`csv_cells_escaped` in the sidecar counts them); the JSON keeps +every value as served. Files are replaced atomically, so a failed write +leaves the previous output intact. Compare two runs: `python3 diff_runs.py --old run1.json --new run2.json` — refuses to compare a run whose sidecar isn't `status: complete`, and @@ -268,17 +279,21 @@ CI runs this offline suite on Python 3.9 and 3.12, an `engine-smoke` job that installs each engine into its own clean venv, and a `docker-build` job that actually builds the image, runs its entrypoint, confirms Chromium launches inside it, and confirms no `.env`/tests/ -fixtures ended up in it. A separate `canary` workflow runs live against -a real filtered category once a day: 3 pages, headful under xvfb, with -assertions on the share of rows carrying a price, living area, bedrooms, -energy grade and property type. It needs a residential exit to pass — -measured 2026-09-23, GitHub's runner is refused like any datacentre -address — so it takes one from a `ZIMMO_PROXY` repository secret, and -without that secret the live job is **skipped** (a `gate` job prints a -`::notice::` saying why) instead of running a request that is refused -every time. A skipped run is not a pass: GitHub still draws the badge -green for a workflow whose only live job was skipped, so open the run -and check that the `canary` job ran before reading the badge as "tested". +fixtures ended up in it. The image runs headless by default, and +zimmo.be refuses headless Chromium: use it with `--cdp-endpoint` (a +Scraping Browser API session), or supply a display yourself and pass +`--headful`. + +A separate `canary` workflow runs live against a real filtered category +once a day: 3 pages, headful under xvfb, with assertions on the share of +rows carrying a price, living area, bedrooms, energy grade and property +type. It needs a residential exit — measured 2026-09-23 and 2026-10-13, +GitHub's runner is refused like any datacentre address, and a US +residential exit was refused while Belgian and German ones were served — +so it takes one from the `ZIMMO_PROXY` repository secret. Without that +secret (or with an empty one) the canary **fails** with an error naming +it: a run that never reached the site is not reported as a pass, and +neither is a block (exit 3), zero rows, or a parser failure (exit 5). Besides the share of populated columns it asserts the arithmetic of the data: every row has a unique `sku`, prices fall in a plausible range, no `original_price` sits at or below its `price`, and every URL is on @@ -294,7 +309,7 @@ renamed or removed, and the CHANGELOG says so. ## Repo maintenance notes (not shipped behaviour) -- A per-clone local `CLAUDE.md`, if you keep one, belongs in +- Private per-clone notes, if you keep any, belong in `.git/info/exclude`, not `.gitignore`. - Before making this repo public, scan every blob that ever existed (`git rev-list --objects --all`) for credentials, not just the diff --git a/captcha_solver.py b/captcha_solver.py index c78d762..089f3a7 100644 --- a/captcha_solver.py +++ b/captcha_solver.py @@ -8,7 +8,7 @@ confirmed live on different sites in this scraper family, and both are checked here. -FIXED (external audit, 2026-09-14, codex.md finding #2): the live +FIXED (external audit, 2026-09-14, finding #2): the live zimmo.be page at audit time presented a Cloudflare **Managed Challenge** -- a hidden `cf-turnstile-response` input exists, but there is no `
` element the old @@ -66,6 +66,36 @@ TWOCAPTCHA_RES_URL = "https://2captcha.com/res.php" +# --- Paid-solve allowance --------------------------------------------------- +# +# Every 2captcha.com solve is a purchase. One page gets at most +# SOLVES_PER_PAGE of them, however many attempts, proxy rotations or +# challenge markers that page goes through: an engine creates ONE +# SolveBudget per page before its attempt loop and hands it to every +# solve call, and both solve functions below take it as a required +# argument and spend it immediately before submitting the task -- never +# after a token has been bought. A missing key or an unsolvable Managed +# Challenge raises before the purchase and costs nothing. +SOLVES_PER_PAGE = 1 + + +class SolveBudgetExhausted(RuntimeError): + pass + + +class SolveBudget: + def __init__(self, limit: int = SOLVES_PER_PAGE): + self.limit = limit + self.used = 0 + + def spend(self, what: str) -> None: + if self.used >= self.limit: + raise SolveBudgetExhausted( + f"{what}: this page's paid-solve allowance ({self.limit}) is already " + f"spent -- not buying another solve for it.") + self.used += 1 + + @dataclass class CaptchaChallenge: kind: str @@ -154,12 +184,13 @@ def _solve_with_2captcha(api_key: str, challenge: CaptchaChallenge, def solve_recaptcha_v3(challenge: CaptchaChallenge, twocaptcha_api_key: Optional[str], - min_score: Optional[float] = None) -> str: + budget: SolveBudget, min_score: Optional[float] = None) -> str: if not twocaptcha_api_key: raise RuntimeError( "reCAPTCHA v3 detected but no --twocaptcha-key was provided. " "Pass --twocaptcha-key or set TWOCAPTCHA_KEY in .env." ) + budget.spend("reCAPTCHA v3") kwargs = {} if min_score is None else {"min_score": min_score} return _solve_with_2captcha(twocaptcha_api_key, challenge, **kwargs) @@ -262,7 +293,8 @@ def _solve_turnstile_with_2captcha(api_key: str, challenge: TurnstileChallenge, raise TimeoutError("2captcha.com did not return a Turnstile token in time") -def solve_turnstile(challenge: TurnstileChallenge, twocaptcha_api_key: Optional[str]) -> str: +def solve_turnstile(challenge: TurnstileChallenge, twocaptcha_api_key: Optional[str], + budget: SolveBudget) -> str: """Raises a clear, actionable error for the sitekey=None case (audit finding #2) rather than submitting a nonsensical request to 2captcha's API and getting back a confusing remote error.""" @@ -278,6 +310,7 @@ def solve_turnstile(challenge: TurnstileChallenge, twocaptcha_api_key: Optional[ "Turnstile challenge detected but no --twocaptcha-key was provided. " "Pass --twocaptcha-key or set TWOCAPTCHA_KEY in .env." ) + budget.spend("Turnstile") return _solve_turnstile_with_2captcha(twocaptcha_api_key, challenge) diff --git a/check_no_credentials.py b/check_no_credentials.py index d8b5535..1aa5c11 100644 --- a/check_no_credentials.py +++ b/check_no_credentials.py @@ -3,10 +3,8 @@ check_no_credentials.py ----------------------------- ONE implementation of "no committed credential", invoked from BOTH -tests.yml and the offline test suite (CLAUDE.md §16 -- "one -implementation, invoked from both CI and the offline suite, and a check -asserting that the workflow calls the script rather than reimplementing -it"). An earlier version of this exact check existed only as an inline +tests.yml and the offline test suite, with a check asserting that the +workflow calls the script rather than reimplementing it. An earlier version of this exact check existed only as an inline `git grep` in tests.yml -- a second, independent copy of the same pattern in a different location is exactly the "two sources of truth" trap this family found in three sibling repos at once: one copy matched @@ -41,7 +39,7 @@ from typing import List, Tuple _ALLOWLISTED_SUBSTRINGS = ("sample_output",) -# FIXED (external audit, 2026-09-14, codex.md finding #10): the +# FIXED (external audit, 2026-09-14, finding #10): the # previous allowlist exempted ANY path containing "test_" or "/tests/" # entirely -- confirmed live during audit to make a real secret # committed as e.g. "test_secret.py" invisible to this scanner. This @@ -81,13 +79,17 @@ def _looks_like_real_credential(matched_text: str) -> bool: after_scheme = matched_text.split("://", 1)[-1] creds = after_scheme.rstrip("@") if "{" in creds or "}" in creds: - return False # a vendor's own braced placeholder, per CLAUDE.md §17 + return False # a vendor's own braced placeholder ({login}) parts = re.split(r"[:\-]", creds.lower()) if any(part in _PLACEHOLDER_WORDS for part in parts): return False return True -_SCAN_EXTENSIONS = (".py", ".md", ".yml", ".yaml", ".toml", ".txt", ".env.example") +# .json/.csv/.html/.cfg too: a fixture or capture that holds an API key in +# a JSON field ({"apiKey": "..."}) is as committable as a .py file, and +# until 2026-10-14 a force-tracked fixture.json was never opened. +_SCAN_EXTENSIONS = (".py", ".md", ".yml", ".yaml", ".toml", ".txt", ".env.example", + ".json", ".csv", ".html", ".cfg") _SKIP_DIRS = {".git", "__pycache__", ".pytest_cache", "node_modules", ".venv", "venv"} @@ -95,6 +97,20 @@ def _is_candidate(filename: str) -> bool: return filename.endswith(_SCAN_EXTENSIONS) or filename in (".env.example", ".env") +def _venv_roots(root: str, rel_paths) -> set: + """Relative directories under `root` that are Python virtualenvs, + recognised by their pyvenv.cfg rather than by a directory name (a venv + can be called anything).""" + roots = set() + for rel in rel_paths: + parts = rel.split("/") + for i in range(1, len(parts)): + prefix = "/".join(parts[:i]) + if prefix not in roots and os.path.isfile(os.path.join(root, prefix, "pyvenv.cfg")): + roots.add(prefix) + return roots + + def _git_committable_files(root: str): """Files git could commit under `root` -- tracked, plus untracked and not ignored -- or None when `root` is not inside a git work tree.""" @@ -105,7 +121,10 @@ def _git_committable_files(root: str): ).stdout except (OSError, subprocess.SubprocessError): return None - return [os.path.join(root, rel) for rel in out.decode("utf-8", "replace").split("\0") if rel] + rels = [rel for rel in out.decode("utf-8", "replace").split("\0") if rel] + venvs = _venv_roots(root, rels) + return [os.path.join(root, rel) for rel in rels + if not any(rel == v or rel.startswith(v + "/") for v in venvs)] def _iter_files(root: str): @@ -119,9 +138,10 @@ def _iter_files(root: str): # can be committed, and a real `.env` is the user's own configuration, # so it is never opened. for dirpath, dirnames, filenames in os.walk(root): - dirnames[:] = [d for d in dirnames if d not in _SKIP_DIRS] + dirnames[:] = [d for d in dirnames if d not in _SKIP_DIRS + and not os.path.isfile(os.path.join(dirpath, d, "pyvenv.cfg"))] for filename in filenames: - # FIXED (external audit, 2026-09-14, codex.md finding #10): + # FIXED (external audit, 2026-09-14, finding #10): # `.env` has no extension this glob would ever match, and # only the LITERAL example file was special-cased -- a real # `.env` accidentally committed (a .gitignore miss, a @@ -132,8 +152,12 @@ def _iter_files(root: str): yield os.path.join(dirpath, filename) -def _is_allowlisted(path: str) -> bool: - return any(marker in path for marker in _ALLOWLISTED_SUBSTRINGS) +def _is_allowlisted(path: str, root: str) -> bool: + """Judged on the path RELATIVE to the scanned root, by file name: a + clone that happens to live under a directory called `sample_output...` + must not exempt every file in it.""" + name = os.path.basename(os.path.relpath(path, root)) + return any(name.startswith(marker) for marker in _ALLOWLISTED_SUBSTRINGS) def describe_match(matched_text: str) -> str: @@ -145,17 +169,21 @@ def describe_match(matched_text: str) -> str: return f"{len(matched_text)}-char hex key" -def scan(root: str) -> List[Tuple[str, int, str]]: +def scan(root: str, stats: dict = None) -> List[Tuple[str, int, str]]: """Returns a list of (path, line_number, description) findings. The - description never contains the matched text (see describe_match).""" + description never contains the matched text (see describe_match). + `stats["scanned"]`, when a dict is passed, receives the number of files + actually read.""" findings = [] this_file = os.path.abspath(__file__) + scanned = 0 for path in _iter_files(root): if os.path.abspath(path) == this_file: continue - if _is_allowlisted(path): + if _is_allowlisted(path, root): continue + scanned += 1 try: with open(path, "r", encoding="utf-8", errors="ignore") as f: lines = f.readlines() @@ -172,6 +200,8 @@ def scan(root: str) -> List[Tuple[str, int, str]]: continue findings.append((path, line_num, describe_match(match.group()))) + if stats is not None: + stats["scanned"] = scanned return findings @@ -180,7 +210,13 @@ def main() -> int: p.add_argument("--path", default=".", help="Root directory to scan (default: current directory)") args = p.parse_args() - findings = scan(args.path) + stats = {} + findings = scan(args.path, stats) + if not stats.get("scanned"): + # A scan that read nothing proves nothing: a wrong --path or an + # exclusion that swallowed the tree must not report "clean". + print(f"Scanned 0 files under {args.path!r} -- refusing to report the tree clean.") + return 1 if findings: print(f"Possible committed credential(s) found ({len(findings)}):") for path, line_num, text in findings: @@ -190,7 +226,7 @@ def main() -> int: "in this file -- the one place that list lives.") return 1 - print(f"No committed credentials found (scanned under {args.path!r}).") + print(f"No committed credentials found ({stats['scanned']} files scanned under {args.path!r}).") return 0 diff --git a/diff_runs.py b/diff_runs.py index 84ad2f6..21fb29d 100644 --- a/diff_runs.py +++ b/diff_runs.py @@ -3,7 +3,7 @@ diff_runs.py ----------------- Diff two zimmo-scraper output runs by `sku`. No site knowledge -- shared -verbatim across the 2scraper family (CLAUDE.md §1, §9). +verbatim across the 2scraper family. Refuses to compare two runs unless BOTH sidecars report status "complete": a partial run's un-fetched pages would otherwise read as @@ -198,7 +198,7 @@ def main() -> int: if args.fail_on_change and (result["changed_count"] > 0 or result["added_count"] > 0 or result["removed_count"] > 0): - # FIXED (external audit, 2026-09-14, codex.md): the help text + # FIXED (external audit, 2026-09-14): the help text # says "if anything actually changed" -- a listing appearing or # disappearing between two runs is exactly that, to anyone # reading the flag's own description, even though internally diff --git a/env_config.py b/env_config.py index 55e0cf6..83dc57e 100644 --- a/env_config.py +++ b/env_config.py @@ -2,7 +2,7 @@ env_config.py ----------------- Hand-rolled .env loader for zimmo-scraper, per the 2scraper family -template (CLAUDE.md §3). No new dependency -- defers to python-dotenv +template. No new dependency -- defers to python-dotenv only if the project already has it installed; otherwise parses KEY=VALUE lines itself. ~30 lines of actual logic, as prescribed. @@ -20,12 +20,53 @@ check when a key "isn't working". """ +import argparse import os from pathlib import Path from typing import Dict, Optional + +# --- Numeric flag types ----------------------------------------------------- +# +# argparse turns a ValueError/ArgumentTypeError from these into +# "error: argument --retries: ...", exit 2. --retries counts EXTRA attempts +# (0 = one navigation, no retry), so 0 is legal there; a negative value +# used to run zero navigations and report "content never obtained", exit 5. + +def non_negative_int(text: str) -> int: + value = int(text) + if value < 0: + raise argparse.ArgumentTypeError(f"must be 0 or more, got {value}") + return value + + +def positive_int(text: str) -> int: + value = int(text) + if value < 1: + raise argparse.ArgumentTypeError(f"must be 1 or more, got {value}") + return value + + +def non_negative_float(text: str) -> float: + value = float(text) + if not value >= 0: # also rejects nan + raise argparse.ArgumentTypeError(f"must be 0 or more, got {text}") + return value + + +def refuse_proxy_with_cdp_endpoint(parser: argparse.ArgumentParser, args) -> None: + """A --cdp-endpoint browser is already running with its own exit: a + local proxy cannot be applied to it and was silently ignored. Refuse + the combination (from flags or .env alike) instead of running without + the proxy the caller asked for.""" + if getattr(args, "cdp_endpoint", None) and (getattr(args, "proxy", None) + or getattr(args, "proxy_file", None)): + parser.error("--proxy/--proxy-file cannot be combined with --cdp-endpoint: the remote " + "browser uses its own exit (check ZIMMO_PROXY and ZIMMO_CDP_ENDPOINT in " + ".env too)") + # Explicit map: env var name -> argparse destination it fills. -# CLAUDE.md §3: never map a variable onto a flag that already has a +# Never map a variable onto a flag that already has a # non-empty default (e.g. --out defaults to "zimmo_listings") -- the # loader only fills UNSET values, so such a mapping would be silently # inert and look configurable when it is not. @@ -75,7 +116,7 @@ def _load_dotenv_values(dotenv_path: Optional[str] = None) -> Dict[str, str]: def _is_placeholder(value: Optional[str]) -> bool: - """CLAUDE.md §17: "a copied .env.example read as CONFIGURED" — the + """Guards against a copied .env.example being read as CONFIGURED: the two credentialled URLs the vendor documents (the Scraping Browser endpoint and the 2Captcha proxy URL) are written with `{login}`, `{password}`-style placeholders IN THE VENDOR'S OWN DOCS, and an @@ -130,8 +171,8 @@ def _mask(value: Optional[str]) -> str: if __name__ == "__main__": # Diagnostic entry point -- prints WHERE each value came from, never # the value itself. Run this first when a key "isn't working". Names - # the placeholder reason rather than quoting the value (CLAUDE.md - # §17: the value itself can be a credentialled URL). + # the placeholder reason rather than quoting the value (the value + # itself can be a credentialled URL). dotenv_values = _load_dotenv_values() print("env_config.py diagnostic — source of each known variable:\n") for env_var in ENV_KEYS: diff --git a/fingerprint_client.py b/fingerprint_client.py index 8de03ca..3a479c6 100644 --- a/fingerprint_client.py +++ b/fingerprint_client.py @@ -2,17 +2,16 @@ fingerprint_client.py ----------------- 2Captcha fingerprint fetch + application. No site knowledge -- shared -verbatim across the 2scraper family (CLAUDE.md §1). +verbatim across the 2scraper family. **UNCONFIRMED against a real, current 2Captcha fingerprint API response** -- unlike this family's Scraping Browser API integration -(whose CDP endpoint shape IS confirmed live and documented in -CLAUDE.md §12). The endpoint path and field names below are a +(whose CDP endpoint shape IS confirmed live). The endpoint path and field names below are a best-effort structure, not verified against real API docs. **Confirm `FINGERPRINT_API_URL` and every field name below before depending on this in a live run.** -CLAUDE.md §16 documents SIX real, repeated bugs found in this exact +SIX real, repeated bugs were found in this exact module across four sibling repos, all invisible to import/--help/ compileall/the offline suite/CI, none of them raising -- "the tool does less than it says while reporting success". Addressed here from @@ -24,8 +23,8 @@ the one call it makes, which is lower risk than the family's other known leak points -- but `redact_secret_patterns()` is still applied to every logged exception here, defensively, since "lower - risk" is not "no risk" and this is exactly the bug class CLAUDE.md - §8 names as the family's worst. + risk" is not "no risk" and a leaked key is the worst + bug class this family has had. 2. **`--fingerprint`/the client never actually setting a user agent** (a key the API returns in neither format the code checked for). Addressed by trying multiple plausible field-name shapes @@ -57,8 +56,7 @@ rather than trusting a manual COPY-list review. Captcha solving, the Scraping Browser API, proxies and fingerprints are -four SEPARATELY-BILLED 2Captcha products behind one API key (CLAUDE.md -§12). Never integrate a competitor's fingerprinting product here. +four SEPARATELY-BILLED 2Captcha products behind one API key. Never integrate a competitor's fingerprinting product here. """ import json @@ -100,8 +98,7 @@ class FingerprintProfile: def _first_present(data: dict, *keys: str, default=None): - """Try several plausible field-name shapes in order -- CLAUDE.md - §16's bug #2 was exactly one guessed key name failing silently + """Try several plausible field-name shapes in order -- bug #2 above was exactly one guessed key name failing silently against a real response shaped differently. Trying several is still a guess, but a narrower one, and `raw` is kept on the returned profile so a real response can be inspected directly.""" @@ -130,10 +127,10 @@ def fetch_fingerprint(api_key: str, *, os_name: str = "windows", resp.raise_for_status() data = resp.json() except (requests.RequestException, json.JSONDecodeError) as e: - # CLAUDE.md §16 bug #1: redact defensively even though this + # Bug #1 above: redact defensively even though this # call's own key rides in the JSON body, not a query string -- # "lower risk" is not "no risk", and this is the exact bug class - # CLAUDE.md §8 names as the family's worst. + # a leaked key is the worst bug class this family has had. logger.warning("fingerprint_client: could not fetch a fingerprint (%s) -- " "continuing with the engine's own default fingerprint instead.", redact_secret_patterns(str(e))) @@ -155,7 +152,7 @@ def fetch_fingerprint(api_key: str, *, os_name: str = "windows", user_agent=user_agent, platform=data.get("platform", "Win32"), # Read the API's OWN locale field verbatim -- never construct - # one from a country code (CLAUDE.md §16 bug #3: "en-{country}" + # one from a country code (bug #3 above: "en-{country}" # for a German fingerprint is a malformation this shape cannot # produce, by construction, as long as this stays a straight # dict read). @@ -179,10 +176,10 @@ def playwright_context_options(profile: FingerprintProfile) -> Dict[str, Any]: """Options for playwright.chromium.launch().new_context(**options). Every key here is confirmed against a real, installed Playwright's own Browser.new_context() signature via inspect.signature() -- - see tests/test_fingerprint_kwargs.py (CLAUDE.md §10: "assert that a - paid API's kwargs are ones the driver accepts"). + see tests/test_fingerprint_kwargs.py (a paid API's kwargs must be + ones the driver accepts). - Never call this when connecting via --cdp-endpoint (CLAUDE.md §8): + Never call this when connecting via --cdp-endpoint: the Scraping Browser brings its own fingerprint, and stacking a second on top creates a contradiction rather than better cover.""" return { diff --git a/output_writer.py b/output_writer.py index ccbafb5..eada31a 100644 --- a/output_writer.py +++ b/output_writer.py @@ -4,7 +4,7 @@ Shared listing model, JSON/CSV writers, and the run-status/exit-code contract used by all engines. Site knowledge: ~none. -Fixed per external audit (2026-09-14, codex.md finding #5): a run whose +Fixed per external audit (2026-09-14, finding #5): a run whose `unattempted_pages` were never passed into finish_run() could report `status=complete` and exit 0 even though it deliberately never even tried some of the requested pages (e.g. after page 1 came back blocked). @@ -14,11 +14,15 @@ didn't fail". Which pages count as trusted is `classify_page()`'s call. """ +import contextlib import csv +import io import json +import os +import tempfile from dataclasses import dataclass, asdict, fields, field from datetime import datetime, timezone -from typing import Optional, List, Dict, Any +from typing import Optional, List, Dict, Any, Tuple from urllib.parse import urlparse, urlunparse, parse_qsl, urlencode @@ -31,7 +35,7 @@ class Product: title: Optional[str] = None brand: Optional[str] = None price: Optional[float] = None - currency: Optional[str] = "EUR" + currency: Optional[str] = None original_price: Optional[float] = None discount_pct: Optional[float] = None image_url: Optional[str] = None @@ -45,7 +49,7 @@ class Product: epc_value: Optional[float] = None def dedupe_key(self) -> str: - """CLAUDE.md-family convention extended per audit finding #9: + """The family's dedupe-by-sku convention, extended per audit finding #9: an object with no `sku` at all (a JSON-LD/CSS-fallback result, which sometimes has none) previously bypassed deduplication and diff entirely. Falls back to the listing URL, which is always @@ -102,7 +106,7 @@ def json_schema() -> dict: # The same code, under the name the rest of this family uses for it as of # 2026-09-21: 5 means "the content was never obtained" -- a remote API # error is one way for that to happen, a dead proxy or a load timeout is -# another. See CLAUDE.md §25. +# another. EXIT_FETCH_FAILED = EXIT_REMOTE_API_ERROR EXIT_PARTIAL = 6 @@ -127,6 +131,12 @@ def json_schema() -> dict: PAGE_BLOCKED = "blocked" # 401/403, or a challenge that never cleared PAGE_NOT_FOUND = "not_found" # 404/410 -- retrying will not change it PAGE_FETCH_ERROR = "fetch_error" # 429/5xx/other 4xx -- the content was never obtained +# The page rendered its listing grid (the engine saw more than +# MIN_CARD_MATCHES listing links before parsing) and the parser still +# returned nothing. That is a parser failure, never "the search is empty": +# reporting it as an empty catalogue (exit 4) would tell a pipeline the +# listings are gone when they are on the screen. +PAGE_PARSER_FAILED = "parser_failed" RETRYABLE_STATUSES = frozenset({408, 429, 500, 502, 503, 504}) @@ -140,13 +150,19 @@ def classify_page(products_found: int, http_status: Optional[int], then the only signal, exactly as before this fix.""" if products_found: return PAGE_CONTENT + if rendered_ok: + # Engines only report rendered_ok after the listing grid appeared, + # and never for a status that rules listings out (404, 5xx), so a + # cleared 403 challenge followed by an unparseable grid lands here + # too. + return PAGE_PARSER_FAILED if http_status in (401, 403): return PAGE_BLOCKED if http_status in (404, 410): return PAGE_NOT_FOUND if http_status is not None and http_status >= 400: return PAGE_FETCH_ERROR - if challenge_detected and not rendered_ok: + if challenge_detected: return PAGE_BLOCKED return PAGE_EMPTY @@ -172,6 +188,15 @@ def should_retry_page(outcome: str, http_status: Optional[int]) -> bool: return outcome == PAGE_FETCH_ERROR and http_status in RETRYABLE_STATUSES +def split_page_result(result) -> Tuple[tuple, bool]: + """An engine's _fetch_page returns (products, ok, blocked, + remote_api_error, parser_failed); the page dispatchers only consume + the first four. Returns those four plus the parser-failure flag + (False for a four-element result).""" + result = tuple(result) + return result[:4], bool(result[4]) if len(result) > 4 else False + + def selection_url(url: str) -> str: """The search a run covered, independent of which page was asked for: scheme and host lower-cased, the `page` parameter dropped, the rest of @@ -189,31 +214,101 @@ def _field_names() -> List[str]: return [f.name for f in fields(Product)] +# --- Writing files -------------------------------------------------------- +# +# Every output file is written to a temporary file in the destination's own +# directory and moved over the destination only once it is complete +# (flush + fsync + os.replace). A crash, a full disk or a serialisation +# error mid-write therefore leaves the previous good file byte-identical +# instead of truncated. The final mode is the existing file's own mode, or +# for a new file what open() would have produced under the current umask +# (0o666 & ~umask) -- a temp file starts at 0600, which must not leak into +# the result. + +def _current_umask() -> int: + mask = os.umask(0) + os.umask(mask) + return mask + + +@contextlib.contextmanager +def _atomic_open(path: str, newline: Optional[str] = None): + path = os.path.abspath(path) + directory = os.path.dirname(path) + try: + mode = os.stat(path).st_mode & 0o7777 + except FileNotFoundError: + mode = 0o666 & ~_current_umask() + fd, tmp = tempfile.mkstemp(prefix=f".{os.path.basename(path)}.", suffix=".tmp", dir=directory) + try: + with io.open(fd, "w", encoding="utf-8", newline=newline) as f: + yield f + f.flush() + os.fsync(f.fileno()) + os.chmod(tmp, mode) + os.replace(tmp, path) + except BaseException: + try: + os.unlink(tmp) + except OSError: + pass + raise + + +# A spreadsheet treats a cell starting with one of these as a formula +# (title and location are text the site's advertisers write). In the CSV +# only, such a cell gets a leading apostrophe; numbers are left alone and +# the JSON output keeps the exact value. +_FORMULA_PREFIXES = ("=", "+", "-", "@", "\t", "\r", "\n") + + +def _csv_cell(value: Any) -> Tuple[Any, bool]: + if isinstance(value, (list, dict)): + value = json.dumps(value, ensure_ascii=False) + if isinstance(value, str) and value.startswith(_FORMULA_PREFIXES): + return "'" + value, True + return value, False + + def write_json(products: List[Product], path: str) -> None: - with open(path, "w", encoding="utf-8") as f: + with _atomic_open(path) as f: json.dump([asdict(p) for p in products], f, ensure_ascii=False, indent=2) -def write_csv(products: List[Product], path: str) -> None: +def write_csv(products: List[Product], path: str) -> int: + """Returns how many cells were escaped as formula-shaped.""" fieldnames = _field_names() - with open(path, "w", encoding="utf-8", newline="") as f: + escaped = 0 + with _atomic_open(path, newline="") as f: writer = csv.DictWriter(f, fieldnames=fieldnames) writer.writeheader() for p in products: - writer.writerow(asdict(p)) - - -def save(products: List[Product], out_prefix: str, fmt: str) -> None: + row = {} + for k, v in asdict(p).items(): + row[k], was_escaped = _csv_cell(v) + escaped += was_escaped + writer.writerow(row) + return escaped + + +def save(products: List[Product], out_prefix: str, fmt: str) -> int: + """Writes the requested formats; returns the CSV escape count (0 when + no CSV was written).""" + escaped = 0 if fmt in ("json", "both"): write_json(products, f"{out_prefix}.json") print(f"[+] Saved {len(products)} listings -> {out_prefix}.json") if fmt in ("csv", "both"): - write_csv(products, f"{out_prefix}.csv") + escaped = write_csv(products, f"{out_prefix}.csv") print(f"[+] Saved {len(products)} listings -> {out_prefix}.csv") + if escaped: + print(f"[+] {escaped} CSV cell(s) started with = + - @ or a control character " + f"and were prefixed with an apostrophe (JSON keeps the raw value).") + return escaped def write_meta(meta: Dict[str, Any], out_prefix: str) -> None: - with open(f"{out_prefix}.meta.json", "w", encoding="utf-8") as f: + with _atomic_open(f"{out_prefix}.meta.json") as f: json.dump(meta, f, ensure_ascii=False, indent=2) print(f"[+] Saved run metadata -> {out_prefix}.meta.json") @@ -233,6 +328,7 @@ def finish_run( started_at: Optional[str] = None, exhausted_pages: Optional[List[int]] = None, url: Optional[str] = None, + parser_failed_pages: Optional[List[int]] = None, ) -> int: """The single place that decides status, exit code, and whether/what to write. @@ -246,7 +342,7 @@ def finish_run( those pages were never ATTEMPTED, only skipped) could see `pages_completed < pages_requested` yet still get back `status=complete, exit 0` -- because nothing failed, nothing was - EVER TRIED either. "Data-based termination" (CLAUDE.md §7: a page + EVER TRIED either. "Data-based termination" (a page added no new sku, so later pages were never requested) is the one legitimate reason pages_completed can be less than pages_requested while still being genuinely `complete`. Those pages arrive in @@ -267,6 +363,10 @@ def finish_run( failed_pages = failed_pages or [] unattempted_pages = unattempted_pages or [] exhausted_pages = exhausted_pages or [] + # A parser-failed page is a failed page: its listings were on the + # screen and are not in the output. + parser_failed_pages = sorted(set(parser_failed_pages or [])) + failed_pages = sorted(set(failed_pages) | set(parser_failed_pages)) finished_at = datetime.now(timezone.utc).isoformat() if remote_api_error and not products: @@ -280,17 +380,22 @@ def finish_run( "in place.") return EXIT_BLOCKED - # Pages were attempted and NONE completed: the content was never - # obtained -- a dead proxy, a load timeout -- which is a different fact - # from "we read the listing and it held nothing". Without this it fell - # through to EXIT_ZERO_PRODUCTS and told a pipeline the catalogue was - # empty on a run that never reached the site. Exit 5 has meant "the - # content was never obtained" family-wide since 2026-09-21 (CLAUDE.md - # §25); this repo predates that pass. - if not products and failed_pages and pages_completed == 0: - print(f"[!] None of the {len(failed_pages)} attempted page(s) could be fetched -- " - f"the content was never obtained. Writing nothing, leaving any previous " - f"good output in place.") + if not products and parser_failed_pages: + print(f"[!] Page(s) {parser_failed_pages} rendered a listing grid but the parser " + f"read zero listings from it -- a parser failure, not an empty search. " + f"Writing nothing, leaving any previous good output in place.") + return EXIT_FETCH_FAILED + + # A page was attempted and failed, and no listing was recovered from + # any page: the content was never obtained (a dead proxy, a load + # timeout), which is a different fact from "we read the listing and it + # held nothing". This holds even when another page was served empty: + # an empty page next to a failed one does not prove the search is + # empty, so it must not be reported as exit 4. + if not products and failed_pages: + print(f"[!] {len(failed_pages)} attempted page(s) {failed_pages} could not be fetched " + f"and no listing was recovered -- the content was never obtained. Writing " + f"nothing, leaving any previous good output in place.") return EXIT_FETCH_FAILED if not products and not allow_empty: @@ -299,7 +404,13 @@ def finish_run( f"run. Pass --allow-empty to force writing an empty result.") return EXIT_ZERO_PRODUCTS - if failed_pages: + if parser_failed_pages: + status = "partial" + other = [p for p in failed_pages if p not in parser_failed_pages] + stop_reason = (f"parser found nothing on rendered page(s) {parser_failed_pages}" + + (f"; pages failed: {other}" if other else "")) + exit_code = EXIT_PARTIAL + elif failed_pages: status = "partial" stop_reason = f"pages failed: {failed_pages}" exit_code = EXIT_PARTIAL @@ -325,13 +436,14 @@ def finish_run( stop_reason = "ok" exit_code = EXIT_OK - save(products, out_prefix, fmt) + csv_cells_escaped = save(products, out_prefix, fmt) write_meta({ "status": status, "stop_reason": stop_reason, "pages_requested": pages_requested, "pages_completed": pages_completed, "failed_pages": failed_pages, + "parser_failed_pages": parser_failed_pages, "unattempted_pages": unattempted_pages, "exhausted_pages": exhausted_pages, "listing_exhausted": bool(exhausted_pages), @@ -341,6 +453,7 @@ def finish_run( "started_at": started_at, "finished_at": finished_at, "source": "zimmo.be", + "csv_cells_escaped": csv_cells_escaped, }, out_prefix) return exit_code diff --git a/playwright_scraper.py b/playwright_scraper.py index 857787a..d98faf6 100644 --- a/playwright_scraper.py +++ b/playwright_scraper.py @@ -54,16 +54,19 @@ from captcha_solver import ( detect_recaptcha_v3, solve_recaptcha_v3, detect_cloudflare_challenge, - detect_turnstile_challenge, solve_turnstile, + detect_turnstile_challenge, solve_turnstile, SolveBudget, INJECT_TOKEN_JS, INJECT_TURNSTILE_TOKEN_JS, ) from product_parser import parse_products, SELECTORS from output_writer import ( Product, finish_run, dedupe_by_sku, EXIT_CRASH, classify_page, page_outcome_is_ok, should_retry_page, status_rules_out_listings, - PAGE_CONTENT, PAGE_BLOCKED, + PAGE_CONTENT, PAGE_BLOCKED, PAGE_PARSER_FAILED, split_page_result, +) +from env_config import ( + apply_env_defaults, non_negative_int, positive_int, non_negative_float, + refuse_proxy_with_cdp_endpoint, ) -from env_config import apply_env_defaults from proxy_pool import ( ProxyPool, load_proxy_file, is_proxy_error, warn_if_concurrency_without_pool, refuse_concurrency_with_cdp_endpoint, @@ -103,12 +106,12 @@ def page_url(base_url: str, page_num: int) -> str: return urlunparse(parsed._replace(query=new_query)) -def handle_captcha_if_present(page, args) -> bool: +def handle_captcha_if_present(page, args, budget: SolveBudget) -> bool: """Runs after EVERY navigation. Returns True if a challenge was detected (regardless of whether solving was attempted or succeeded). - FIXED (external audit, 2026-09-14, codex.md finding #3): + FIXED (external audit, 2026-09-14, finding #3): --solve-captcha=never previously did not prevent the solver from being called at all -- detection and solving were unconditional. Now: "never" detects (for logging/blocked-status) but never calls a @@ -147,7 +150,7 @@ def handle_captcha_if_present(page, args) -> bool: logger.warning("Cloudflare Turnstile challenge detected (sitekey=%s) -- " "attempting to solve.", turnstile.sitekey) try: - token = solve_turnstile(turnstile, args.twocaptcha_key) + token = solve_turnstile(turnstile, args.twocaptcha_key, budget) page.evaluate(INJECT_TURNSTILE_TOKEN_JS, token) logger.info("Turnstile token injected. Reloading page to continue.") page.wait_for_timeout(1500) @@ -168,7 +171,8 @@ def handle_captcha_if_present(page, args) -> bool: logger.warning("reCAPTCHA v3 detected (sitekey=%s, action=%s) -- attempting to solve.", challenge.sitekey, challenge.action) try: - token = solve_recaptcha_v3(challenge, args.twocaptcha_key, min_score=args.min_score) + token = solve_recaptcha_v3(challenge, args.twocaptcha_key, budget, + min_score=args.min_score) page.evaluate(INJECT_TOKEN_JS, token) logger.info("Token injected. Reloading page to continue.") page.wait_for_timeout(1500) @@ -273,11 +277,12 @@ def reset(self): def _fetch_page(url: str, page_num: int, args, proxy_pool: Optional[ProxyPool], worker_offset: int, cdp: Optional[CdpConnection] = None - ) -> Tuple[List[Product], bool, bool, bool]: + ) -> Tuple[List[Product], bool, bool, bool, bool]: """Fetch and parse one page. Returns - (products, success, blocked, remote_api_error). + (products, success, blocked, remote_api_error, parser_failed); a + failure path returns the first four only. - CRITICAL FIX (external audit, 2026-09-14, codex.md finding #1): this + CRITICAL FIX (external audit, 2026-09-14, finding #1): this function now creates and tears down its OWN `sync_playwright()` context internally, rather than receiving one shared object created in a different thread. The previous version created a single @@ -312,6 +317,9 @@ def _fetch_page(url: str, page_num: int, args, proxy_pool: Optional[ProxyPool], def _fetch_page_attempts(url, page_num, args, proxy_pool, worker_offset, current_exit, cdp): + # One paid-solve allowance for this page, shared by every attempt and + # every proxy rotation below (captcha_solver.SolveBudget). + budget = SolveBudget() for attempt in range(args.retries + 1): browser = None page = None @@ -340,7 +348,7 @@ def _fetch_page_attempts(url, page_num, args, proxy_pool, worker_offset, current response = page.goto(url, wait_until="domcontentloaded", timeout=60000) http_status = response.status if response else None - detected = handle_captcha_if_present(page, args) + detected = handle_captcha_if_present(page, args, budget) rendered_ok = (False if status_rules_out_listings(http_status) else _wait_for_listing_markers(page)) @@ -393,7 +401,7 @@ def _fetch_page_attempts(url, page_num, args, proxy_pool, worker_offset, current if ok and args.delay > 0: time.sleep(args.delay) - return products, ok, outcome == PAGE_BLOCKED, False + return products, ok, outcome == PAGE_BLOCKED, False, outcome == PAGE_PARSER_FAILED except PWTimeout: _drop_cdp_after_failure(cdp, page) @@ -543,14 +551,15 @@ def _scrape(args, cdp: Optional[CdpConnection]) -> int: # -- see _fetch_page's own docstring for why every call, including # this one in the main thread, manages its own context uniformly). url_1 = page_url(args.url, 1) - products_1, ok_1, blocked_1, remote_1 = _fetch_page(url_1, 1, args, proxy_pool, - worker_offset=0, cdp=cdp) + (products_1, ok_1, blocked_1, remote_1), parser_failed_1 = split_page_result( + _fetch_page(url_1, 1, args, proxy_pool, worker_offset=0, cdp=cdp)) page_results = {1: products_1} failed_pages = [] if ok_1 else [1] any_blocked = blocked_1 any_remote_error = remote_1 pages_completed = 1 if ok_1 else 0 + parser_failed_pages: List[int] = [1] if parser_failed_1 else [] unattempted_pages: List[int] = [] exhausted_pages: List[int] = [] @@ -565,7 +574,11 @@ def _scrape(args, cdp: Optional[CdpConnection]) -> int: if remaining_pages: def fetch_fn(page_num, worker_offset): url = page_url(args.url, page_num) - return _fetch_page(url, page_num, args, proxy_pool, worker_offset, cdp=cdp) + result, parser_failed = split_page_result( + _fetch_page(url, page_num, args, proxy_pool, worker_offset, cdp=cdp)) + if parser_failed: + parser_failed_pages.append(page_num) + return result initial_skus = {p.sku for p in products_1 if p.sku} more_results, more_failed, more_blocked, more_remote, more_unattempted = \ @@ -588,7 +601,7 @@ def fetch_fn(page_num, worker_offset): all_products, args.out, args.format, pages_requested=args.pages, pages_completed=pages_completed, failed_pages=failed_pages, - # FIXED (external audit, 2026-09-14, codex.md finding #5): + # FIXED (external audit, 2026-09-14, finding #5): # unattempted_pages was computed but never passed into # finish_run() -- a run that stopped at page 1 (blocked) with # --pages 5 could report status=complete, exit 0. Now passed @@ -599,6 +612,7 @@ def fetch_fn(page_num, worker_offset): remote_api_error=any_remote_error, allow_empty=args.allow_empty, started_at=started_at, exhausted_pages=exhausted_pages, url=args.url, + parser_failed_pages=parser_failed_pages, ) @@ -607,12 +621,13 @@ def parse_args(): p.add_argument("--url", default=None, help="zimmo.be search-results URL (falls back to ZIMMO_URL from .env)") p.add_argument("--category", default=None, help="Label to tag output rows with (optional)") - p.add_argument("--pages", type=int, default=1, help="Number of result pages to fetch") - p.add_argument("--delay", type=float, default=0.0, + p.add_argument("--pages", type=positive_int, default=1, help="Number of result pages to fetch") + p.add_argument("--delay", type=non_negative_float, default=0.0, help="Seconds to sleep after each successfully-fetched page (rate limiting)") - p.add_argument("--retries", type=int, default=2, help="Retries per page on a transient failure") - p.add_argument("--retry-delay", type=float, default=3.0, help="Seconds to wait between retries") - p.add_argument("--concurrency", type=int, default=1, + p.add_argument("--retries", type=non_negative_int, default=2, + help="Extra attempts per page on a transient failure (0 = one attempt)") + p.add_argument("--retry-delay", type=non_negative_float, default=3.0, help="Seconds to wait between retries") + p.add_argument("--concurrency", type=positive_int, default=1, help="Parallel workers for pages 2+ (page 1 is always fetched alone). " "Not supported together with --cdp-endpoint.") p.add_argument("--format", choices=["json", "csv", "both"], default="both") @@ -629,16 +644,16 @@ def parse_args(): "defaulting to True, which meant it could never actually be turned off " "from the command line -- this flag is the real, working opt-out.") p.add_argument("--proxy-shuffle", action="store_true", help="Shuffle the proxy pool's exit order at startup") - p.add_argument("--proxy-block-retries", type=int, default=3, + p.add_argument("--proxy-block-retries", type=positive_int, default=3, help="Treat a proxy exit as blocked after this many failures") p.add_argument("--twocaptcha-key", default=None, help="2captcha.com API key") - p.add_argument("--captcha-api", default=None, help="Alternate captcha-solving API base URL (advanced)") p.add_argument("--solve-captcha", choices=["always", "when-blocked", "never"], default="when-blocked") p.add_argument("--min-score", type=float, default=None, help="Minimum acceptable reCAPTCHA v3 score to request from the solver") p.add_argument("--cdp-endpoint", default=None, help="Connect to an existing browser over CDP instead of launching Playwright's " - "bundled Chromium. --proxy and --headless/--headful are ignored when set.") + "bundled Chromium. Refused together with --proxy/--proxy-file (the remote " + "browser has its own exit); --headless/--headful are ignored when set.") p.add_argument("--allow-empty", action="store_true", help="Write output even if zero products were found") p.add_argument("--dump-html", action="store_true", @@ -650,6 +665,7 @@ def parse_args(): apply_env_defaults(args) if not args.url: p.error("--url is required (or set ZIMMO_URL in .env)") + refuse_proxy_with_cdp_endpoint(p, args) return args diff --git a/product_parser.py b/product_parser.py index 6dd75db..95f338c 100644 --- a/product_parser.py +++ b/product_parser.py @@ -32,7 +32,7 @@ Each object in `properties` was a complete, well-structured listing record. This primary path was explicitly confirmed sound by an external -audit (2026-09-14, codex.md): "Основной embedded-JSON путь обычно +audit (2026-09-14): "Основной embedded-JSON путь обычно избегает этой ошибки" (the primary embedded-JSON path is generally immune to the price-parsing bug found in the fallback) -- prices here came from `prijs`/`zprijs` as already-numeric JSON fields, never @@ -356,7 +356,8 @@ def _parse_jsonld(html: str, base_url: str) -> List[Product]: sku=node.get("sku") or node.get("productID"), title=node.get("name"), price=_to_float(str(price)) if price is not None else None, - currency=offers.get("priceCurrency", "EUR"), + # No stated currency is null, never an assumed EUR. + currency=offers.get("priceCurrency") or None, image_url=_extract_image_url(node.get("image")), location=location, )) @@ -659,6 +660,8 @@ def _parse_properties_json(html: str, base_url: str, category: Optional[str]) -> title=title, brand=advertiser.get("name") if isinstance(advertiser, dict) else None, price=price, + # A price on zimmo.be's own JSON is in euro; no price, no currency. + currency="EUR" if price is not None else None, original_price=original_price, discount_pct=discount_pct, image_url=item.get("hoofdFoto"), diff --git a/proxy_pool.py b/proxy_pool.py index 5609d4e..ca624f8 100644 --- a/proxy_pool.py +++ b/proxy_pool.py @@ -4,7 +4,7 @@ Proxy pool, rotation policy, credential masking, and argv safety. No site knowledge. -Two fixes per external audit (2026-09-14, codex.md finding #4): +Two fixes per external audit (2026-09-14, finding #4): 1. A failed proxy exit now rotates away IMMEDIATELY on the next attempt, rather than staying a candidate until its failure count reaches --proxy-block-retries. Previously, with the default of 2 diff --git a/puppeteer_scraper.py b/puppeteer_scraper.py index abda0a3..2d61c47 100644 --- a/puppeteer_scraper.py +++ b/puppeteer_scraper.py @@ -6,11 +6,11 @@ Same feature set as playwright_scraper.py -- see its module docstring for the full strategy, pagination, and captcha-handling story. All three engines in this family must agree on exit codes, run status, and -whether a run crashes or spends money (CLAUDE.md §6); this file mirrors +whether a run crashes or spends money; this file mirrors playwright_scraper.py's control flow deliberately, translated into pyppeteer's async API. -pyppeteer is effectively unmaintained (CLAUDE.md §6) -- kept for +pyppeteer is effectively unmaintained -- kept for parity, demoted in priority, not in correctness. Usage @@ -35,16 +35,19 @@ from captcha_solver import ( detect_recaptcha_v3, solve_recaptcha_v3, detect_cloudflare_challenge, - detect_turnstile_challenge, solve_turnstile, + detect_turnstile_challenge, solve_turnstile, SolveBudget, INJECT_TOKEN_JS, INJECT_TURNSTILE_TOKEN_JS, ) from product_parser import parse_products, SELECTORS from output_writer import ( Product, finish_run, dedupe_by_sku, EXIT_CRASH, classify_page, page_outcome_is_ok, should_retry_page, status_rules_out_listings, - PAGE_CONTENT, PAGE_BLOCKED, + PAGE_CONTENT, PAGE_BLOCKED, PAGE_PARSER_FAILED, split_page_result, +) +from env_config import ( + apply_env_defaults, non_negative_int, positive_int, non_negative_float, + refuse_proxy_with_cdp_endpoint, ) -from env_config import apply_env_defaults from proxy_pool import ( ProxyPool, load_proxy_file, is_proxy_error, warn_if_concurrency_without_pool, refuse_concurrency_with_cdp_endpoint, @@ -62,7 +65,7 @@ # their own, and opens its websocket with pings disabled, so a remote # browser that stops answering left the run hanging -- measured: page 1 # done, then the connect for page 2 still waiting after ten minutes. -# Every remote call is bounded (CLAUDE.md §8); a timeout lands in the +# Every remote call is bounded; a timeout lands in the # ordinary retry path below. CDP_CALL_TIMEOUT_S = 60 @@ -113,7 +116,7 @@ def _mask_credentials(url: str) -> str: def page_url(base_url: str, page_num: int) -> str: """See playwright_scraper.py's page_url() -- identical logic, duplicated so this engine has no import-time dependency on - Playwright being installed (CLAUDE.md §10).""" + Playwright being installed.""" if page_num <= 1: return base_url parsed = urlparse(base_url) @@ -123,7 +126,7 @@ def page_url(base_url: str, page_num: int) -> str: return urlunparse(parsed._replace(query=new_query)) -async def handle_captcha_if_present(page, args) -> bool: +async def handle_captcha_if_present(page, args, budget: SolveBudget) -> bool: html = await page.content() detected_anything = False @@ -151,7 +154,7 @@ async def handle_captcha_if_present(page, args) -> bool: logger.warning("Cloudflare Turnstile challenge detected (sitekey=%s) -- " "attempting to solve.", turnstile.sitekey) try: - token = solve_turnstile(turnstile, args.twocaptcha_key) + token = solve_turnstile(turnstile, args.twocaptcha_key, budget) await page.evaluate(f"({INJECT_TURNSTILE_TOKEN_JS})", token) logger.info("Turnstile token injected. Reloading page to continue.") await asyncio.sleep(1.5) @@ -172,7 +175,8 @@ async def handle_captcha_if_present(page, args) -> bool: logger.warning("reCAPTCHA v3 detected (sitekey=%s, action=%s) -- attempting to solve.", challenge.sitekey, challenge.action) try: - token = solve_recaptcha_v3(challenge, args.twocaptcha_key, min_score=args.min_score) + token = solve_recaptcha_v3(challenge, args.twocaptcha_key, budget, + min_score=args.min_score) await page.evaluate(f"({INJECT_TOKEN_JS})", token) logger.info("Token injected. Reloading page to continue.") await asyncio.sleep(1.5) @@ -257,7 +261,7 @@ async def reset(self): async def _fetch_page(url: str, page_num: int, args, proxy_pool: Optional[ProxyPool], worker_offset: int, cdp: Optional[CdpConnection] = None - ) -> Tuple[List[Product], bool, bool, bool]: + ) -> Tuple[List[Product], bool, bool, bool, bool]: """With --cdp-endpoint the page opens in `cdp`, the run's one connection; a caller that passes none gets one for this call only.""" own_cdp = None @@ -273,6 +277,9 @@ async def _fetch_page(url: str, page_num: int, args, proxy_pool: Optional[ProxyP async def _fetch_page_attempts(url, page_num, args, proxy_pool, worker_offset, cdp): current_exit = proxy_pool.get_exit_for_worker(worker_offset) if proxy_pool else None + # One paid-solve allowance for this page, shared by every attempt and + # every proxy rotation below (captcha_solver.SolveBudget). + budget = SolveBudget() for attempt in range(args.retries + 1): browser = None page = None @@ -285,7 +292,7 @@ async def _fetch_page_attempts(url, page_num, args, proxy_pool, worker_offset, c "args": ["--disable-blink-features=AutomationControlled", "--window-size=1366,900"], } if current_exit: - # Credentials never reach argv (CLAUDE.md §8): only + # Credentials never reach argv: only # the credential-free server string goes on the # launch flag. The credential itself goes through # the DevTools Fetch domain below (_authenticate_proxy). @@ -303,7 +310,7 @@ async def _fetch_page_attempts(url, page_num, args, proxy_pool, worker_offset, c response = await page.goto(url, {"waitUntil": "domcontentloaded", "timeout": 60000}) http_status = response.status if response else None - detected = await handle_captcha_if_present(page, args) + detected = await handle_captcha_if_present(page, args, budget) rendered_ok = (False if status_rules_out_listings(http_status) else await _wait_for_listing_markers(page)) @@ -350,7 +357,7 @@ async def _fetch_page_attempts(url, page_num, args, proxy_pool, worker_offset, c if ok and args.delay > 0: await asyncio.sleep(args.delay) - return products, ok, outcome == PAGE_BLOCKED, False + return products, ok, outcome == PAGE_BLOCKED, False, outcome == PAGE_PARSER_FAILED except Exception as e: error_msg = redact_secret_patterns(str(e)) @@ -393,7 +400,7 @@ async def _fetch_page_attempts(url, page_num, args, proxy_pool, worker_offset, c async def _fetch_pages_concurrently(fetch_fn, remaining_pages: List[int], concurrency: int, seen_skus_initial=()) -> Tuple[dict, List[int], bool, bool, List[int]]: """asyncio equivalent of playwright_scraper.py's own - `_fetch_pages_concurrently` -- same contract, same CLAUDE.md §7 + `_fetch_pages_concurrently` -- same contract, same data-based termination, reimplemented as a small worker-pool pulling from a shared index under a lock rather than a ThreadPoolExecutor, since pyppeteer is async-native.""" @@ -468,14 +475,15 @@ async def _scrape_async(args, cdp: Optional[CdpConnection]) -> int: refuse_concurrency_with_cdp_endpoint(args.concurrency, args.cdp_endpoint) url_1 = page_url(args.url, 1) - products_1, ok_1, blocked_1, remote_1 = await _fetch_page(url_1, 1, args, proxy_pool, - worker_offset=0, cdp=cdp) + (products_1, ok_1, blocked_1, remote_1), parser_failed_1 = split_page_result( + await _fetch_page(url_1, 1, args, proxy_pool, worker_offset=0, cdp=cdp)) page_results = {1: products_1} failed_pages = [] if ok_1 else [1] any_blocked = blocked_1 any_remote_error = remote_1 pages_completed = 1 if ok_1 else 0 + parser_failed_pages: List[int] = [1] if parser_failed_1 else [] unattempted_pages: List[int] = [] exhausted_pages: List[int] = [] @@ -488,7 +496,11 @@ async def _scrape_async(args, cdp: Optional[CdpConnection]) -> int: if remaining_pages: async def fetch_fn(page_num, worker_offset): url = page_url(args.url, page_num) - return await _fetch_page(url, page_num, args, proxy_pool, worker_offset, cdp=cdp) + result, parser_failed = split_page_result( + await _fetch_page(url, page_num, args, proxy_pool, worker_offset, cdp=cdp)) + if parser_failed: + parser_failed_pages.append(page_num) + return result initial_skus = {p.sku for p in products_1 if p.sku} more_results, more_failed, more_blocked, more_remote, more_unattempted = \ @@ -512,6 +524,7 @@ async def fetch_fn(page_num, worker_offset): remote_api_error=any_remote_error, allow_empty=args.allow_empty, started_at=started_at, exhausted_pages=exhausted_pages, url=args.url, + parser_failed_pages=parser_failed_pages, ) @@ -520,12 +533,13 @@ def parse_args(): p.add_argument("--url", default=None, help="zimmo.be search-results URL (falls back to ZIMMO_URL from .env)") p.add_argument("--category", default=None, help="Label to tag output rows with (optional)") - p.add_argument("--pages", type=int, default=1, help="Number of result pages to fetch") - p.add_argument("--delay", type=float, default=0.0, + p.add_argument("--pages", type=positive_int, default=1, help="Number of result pages to fetch") + p.add_argument("--delay", type=non_negative_float, default=0.0, help="Seconds to sleep after each successfully-fetched page (rate limiting)") - p.add_argument("--retries", type=int, default=2, help="Retries per page on a transient failure") - p.add_argument("--retry-delay", type=float, default=3.0, help="Seconds to wait between retries") - p.add_argument("--concurrency", type=int, default=1, + p.add_argument("--retries", type=non_negative_int, default=2, + help="Extra attempts per page on a transient failure (0 = one attempt)") + p.add_argument("--retry-delay", type=non_negative_float, default=3.0, help="Seconds to wait between retries") + p.add_argument("--concurrency", type=positive_int, default=1, help="Parallel workers for pages 2+ (page 1 is always fetched alone). " "Not supported together with --cdp-endpoint.") p.add_argument("--format", choices=["json", "csv", "both"], default="both") @@ -539,16 +553,16 @@ def parse_args(): p.add_argument("--no-proxy-rotate", dest="proxy_rotate", action="store_false", help="Disable proxy rotation -- retry the SAME exit on every failure instead") p.add_argument("--proxy-shuffle", action="store_true", help="Shuffle the proxy pool's exit order at startup") - p.add_argument("--proxy-block-retries", type=int, default=3, + p.add_argument("--proxy-block-retries", type=positive_int, default=3, help="Treat a proxy exit as blocked after this many failures") p.add_argument("--twocaptcha-key", default=None, help="2captcha.com API key") - p.add_argument("--captcha-api", default=None, help="Alternate captcha-solving API base URL (advanced)") p.add_argument("--solve-captcha", choices=["always", "when-blocked", "never"], default="when-blocked") p.add_argument("--min-score", type=float, default=None, help="Minimum acceptable reCAPTCHA v3 score to request from the solver") p.add_argument("--cdp-endpoint", default=None, help="Connect to an existing browser over CDP instead of launching pyppeteer's " - "bundled Chromium. --proxy and --headless/--headful are ignored when set.") + "bundled Chromium. Refused together with --proxy/--proxy-file (the remote " + "browser has its own exit); --headless/--headful are ignored when set.") p.add_argument("--allow-empty", action="store_true", help="Write output even if zero products were found") p.add_argument("--dump-html", action="store_true", @@ -560,6 +574,7 @@ def parse_args(): apply_env_defaults(args) if not args.url: p.error("--url is required (or set ZIMMO_URL in .env)") + refuse_proxy_with_cdp_endpoint(p, args) return args diff --git a/pyproject.toml b/pyproject.toml index 74c5607..13a6d21 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -16,7 +16,7 @@ dependencies = [ [project.optional-dependencies] # Mirrors requirements-{engine}.txt exactly — kept in sync by hand -# (CLAUDE.md §6: CI installs only requirements.txt + one of these files +# (CI installs only requirements.txt + one of these files # directly, not through these extras, so drift here would go unnoticed # by CI; check both whenever either changes). playwright = ["playwright>=1.44"] @@ -26,7 +26,7 @@ selenium = ["selenium>=4.20"] [tool.setuptools] # This is a flat collection of top-level scripts, not a package — # setuptools refuses to guess among this many top-level modules, so each -# one is named explicitly (CLAUDE.md §6). +# one is named explicitly. py-modules = [ "env_config", "output_writer", diff --git a/requirements-playwright.txt b/requirements-playwright.txt index 0ecfb37..4cd512c 100644 --- a/requirements-playwright.txt +++ b/requirements-playwright.txt @@ -1,4 +1,4 @@ -# EXACTLY pinned, not >=1.44 (external audit, 2026-09-14, codex.md +# EXACTLY pinned, not >=1.44 (external audit, 2026-09-14, # finding #7): Dockerfile's base image (mcr.microsoft.com/playwright/ # python:v1.44.0-jammy) bundles a SPECIFIC Chromium revision matched to # playwright==1.44.0 exactly. An unpinned `>=1.44` requirement lets pip diff --git a/requirements.txt b/requirements.txt index 3f7f234..c04518c 100644 --- a/requirements.txt +++ b/requirements.txt @@ -2,7 +2,7 @@ # pip install -r requirements.txt --break-system-packages # # Then ONE engine-specific file on top, in its OWN virtualenv if you want -# more than one installed at once (CLAUDE.md §6): playwright and pyppeteer +# more than one installed at once: playwright and pyppeteer # declare mutually unsatisfiable pins (pyee <12 vs >=13), and pyppeteer # and selenium collide on urllib3 (<2.0 vs >=2.6). CI's own engine-smoke # job installs each engine into its own fresh venv for exactly this diff --git a/scraper_api_client.py b/scraper_api_client.py index 14d3ffd..028dabd 100644 --- a/scraper_api_client.py +++ b/scraper_api_client.py @@ -74,12 +74,20 @@ import logging import sys from datetime import datetime, timezone +from typing import Optional, Tuple import requests -from product_parser import parse_products -from output_writer import finish_run, dedupe_by_sku, EXIT_REMOTE_API_ERROR -from env_config import apply_env_defaults +from bs4 import BeautifulSoup + +from captcha_solver import detect_cloudflare_challenge +from product_parser import parse_products, SELECTORS +from output_writer import ( + finish_run, dedupe_by_sku, EXIT_REMOTE_API_ERROR, + classify_page, page_outcome_is_ok, status_rules_out_listings, + PAGE_CONTENT, PAGE_BLOCKED, PAGE_PARSER_FAILED, +) +from env_config import apply_env_defaults, positive_int from proxy_pool import redact_secret_patterns logging.basicConfig(level=logging.INFO, format="%(asctime)s [%(levelname)s] %(message)s") @@ -100,11 +108,15 @@ # not expected to appear on a genuine Cloudflare block page at all. DEFAULT_WAIT_FOR_TEXT = "€" +# Same threshold as the browser engines' readiness wait. +MIN_CARD_MATCHES = 5 + def scrape_via_api(url: str, api_key: str, wait_for_text: str, timeout: int, - cdp_url: str = None, proxy: str = None) -> str: + cdp_url: str = None, proxy: str = None) -> Tuple[str, Optional[int]]: """Calls the Scraper API's synchronous /tasks/sync endpoint and - returns the fetched page's raw HTML. Raises on any failure. + returns (the fetched page's raw HTML, the target's own HTTP status or + None). Raises on any failure of the API call itself. `proxy`: CONFIRMED LIVE 2026-09-16 -- field name and a plain `scheme://user:pass@host:port` string value are accepted, AND the @@ -150,7 +162,7 @@ def scrape_via_api(url: str, api_key: str, wait_for_text: str, timeout: int, except requests.RequestException as e: # The key rides in a header here, not a query string, so this # is lower risk than the family's known leak points -- but - # redact globally regardless (CLAUDE.md §8), and a caller- + # redact globally regardless, and a caller- # supplied --cdp-url IS part of the request body and could in # principle surface in a verbose client-side error. raise RuntimeError(f"Scraper API request failed: {redact_secret_patterns(str(e))}") from None @@ -177,21 +189,22 @@ def scrape_via_api(url: str, api_key: str, wait_for_text: str, timeout: int, # and the TARGET's HTTP code is `http_code`. Comparing the string # `status` with 400 raised TypeError on every successful call. target_status = data.get("http_code") - if isinstance(target_status, int) and target_status >= 400: - logger.warning("Target page itself responded with HTTP %s -- parsing whatever HTML " - "came back anyway.", target_status) + if not isinstance(target_status, int): + target_status = None + elif target_status >= 400: + logger.warning("Target page itself responded with HTTP %s.", target_status) html = data.get("body", "") if not html: raise RuntimeError(f"Scraper API returned no HTML body. Full response: {resp.text[:500]}") - return html + return html, target_status def scrape(args) -> int: started_at = datetime.now(timezone.utc).isoformat() try: - html = scrape_via_api(args.url, args.twocaptcha_key, args.wait_for_text, + html, target_status = scrape_via_api(args.url, args.twocaptcha_key, args.wait_for_text, args.timeout, args.cdp_url, args.proxy) except Exception as e: logger.error("Scraper API request failed: %s", redact_secret_patterns(str(e))) @@ -217,12 +230,31 @@ def scrape(args) -> int: products = dedupe_by_sku(products) + # The same page verdict as the browser engines (output_writer. + # classify_page): a served page with no listings is an empty search + # (exit 4), a Cloudflare challenge or a 403 is a block (exit 3), a + # rendered listing grid that parses to nothing is a parser failure + # (exit 5), and a 5xx/404 means the content was never obtained. + # "Rendered" means the HTML holds more than MIN_CARD_MATCHES listing + # links, the same test the browser engines wait for. + rendered_ok = (not status_rules_out_listings(target_status) + and len(BeautifulSoup(html, "html.parser").select(SELECTORS["item_link"])) + > MIN_CARD_MATCHES) + outcome = classify_page(len(products), target_status, detect_cloudflare_challenge(html), + rendered_ok) + if outcome != PAGE_CONTENT: + logger.warning("Page 1: HTTP %s, classified as %s.", target_status, outcome) + ok = page_outcome_is_ok(outcome) + return finish_run( products, args.out, args.format, - pages_requested=1, pages_completed=1 if products else 0, - failed_pages=[] if products else [1], + pages_requested=1, pages_completed=1 if ok else 0, + failed_pages=[] if ok else [1], + blocked=outcome == PAGE_BLOCKED, + parser_failed_pages=[1] if outcome == PAGE_PARSER_FAILED else [], allow_empty=args.allow_empty, started_at=started_at, + url=args.url, ) @@ -238,7 +270,7 @@ def parse_args(): "and Browser API)") p.add_argument("--wait-for-text", default=DEFAULT_WAIT_FOR_TEXT, help="Text that must appear in the page before the Scraper API returns") - p.add_argument("--timeout", type=int, default=90, + p.add_argument("--timeout", type=positive_int, default=90, help="Max seconds to let the Scraper API's own task run (1-120)") p.add_argument("--cdp-url", default=None, help="Route the Scraper API's fetch through your OWN existing browser " diff --git a/selenium_scraper.py b/selenium_scraper.py index 9cd80b1..b1ddd74 100644 --- a/selenium_scraper.py +++ b/selenium_scraper.py @@ -5,9 +5,9 @@ Same feature set as playwright_scraper.py -- see its module docstring for the full strategy. All three engines must agree on exit codes, run -status, and whether a run crashes or spends money (CLAUDE.md §6). +status, and whether a run crashes or spends money. -TWO CONFIRMED ENGINE LIMITS (CLAUDE.md §6): +TWO CONFIRMED ENGINE LIMITS: 1. Selenium's `debuggerAddress` was designed for a LOCAL, UNAUTHENTICATED debug port. It does NOT forward a `user:pass` @@ -47,16 +47,19 @@ from captcha_solver import ( detect_recaptcha_v3, solve_recaptcha_v3, detect_cloudflare_challenge, - detect_turnstile_challenge, solve_turnstile, + detect_turnstile_challenge, solve_turnstile, SolveBudget, INJECT_TOKEN_JS, INJECT_TURNSTILE_TOKEN_JS, ) from product_parser import parse_products, SELECTORS from output_writer import ( Product, finish_run, dedupe_by_sku, EXIT_CRASH, classify_page, page_outcome_is_ok, should_retry_page, status_rules_out_listings, - PAGE_CONTENT, PAGE_BLOCKED, + PAGE_CONTENT, PAGE_BLOCKED, PAGE_PARSER_FAILED, split_page_result, +) +from env_config import ( + apply_env_defaults, non_negative_int, positive_int, non_negative_float, + refuse_proxy_with_cdp_endpoint, ) -from env_config import apply_env_defaults from proxy_pool import ( ProxyPool, load_proxy_file, is_proxy_error, warn_if_concurrency_without_pool, refuse_concurrency_with_cdp_endpoint, @@ -117,7 +120,7 @@ def build_driver(args, proxy_server_only: Optional[str] = None) -> webdriver.Chr options.add_argument("--headless=new") options.add_argument("--disable-blink-features=AutomationControlled") options.add_argument("--window-size=1366,900") - # UA comes from the browser, not a hardcoded literal (CLAUDE.md §8). + # UA comes from the browser, not a hardcoded literal. if proxy_server_only: options.add_argument(f"--proxy-server={proxy_server_only}") @@ -135,10 +138,10 @@ def build_driver(args, proxy_server_only: Optional[str] = None) -> webdriver.Chr return driver -def handle_captcha_if_present(driver, args) -> bool: +def handle_captcha_if_present(driver, args, budget: SolveBudget) -> bool: """Selenium's execute_script takes a function BODY with an explicit call, not Playwright/pyppeteer's `() => expr` convention - (CLAUDE.md §1 on why no JavaScript crosses an engine boundary).""" + (which is why no JavaScript string crosses an engine boundary).""" html = driver.page_source detected_anything = False @@ -163,7 +166,7 @@ def handle_captcha_if_present(driver, args) -> bool: logger.warning("Cloudflare Turnstile challenge detected (sitekey=%s) -- " "attempting to solve.", turnstile.sitekey) try: - token = solve_turnstile(turnstile, args.twocaptcha_key) + token = solve_turnstile(turnstile, args.twocaptcha_key, budget) driver.execute_script(f"({INJECT_TURNSTILE_TOKEN_JS})(arguments[0]);", token) logger.info("Turnstile token injected. Reloading page to continue.") time.sleep(1.5) @@ -184,7 +187,8 @@ def handle_captcha_if_present(driver, args) -> bool: logger.warning("reCAPTCHA v3 detected (sitekey=%s, action=%s) -- attempting to solve.", challenge.sitekey, challenge.action) try: - token = solve_recaptcha_v3(challenge, args.twocaptcha_key, min_score=args.min_score) + token = solve_recaptcha_v3(challenge, args.twocaptcha_key, budget, + min_score=args.min_score) driver.execute_script(f"({INJECT_TOKEN_JS})(arguments[0]);", token) logger.info("Token injected. Reloading page to continue.") time.sleep(1.5) @@ -225,15 +229,18 @@ def _navigation_status(driver) -> Optional[int]: def _fetch_page(url: str, page_num: int, args, proxy_pool: Optional[ProxyPool], - worker_offset: int) -> Tuple[List[Product], bool, bool, bool]: + worker_offset: int) -> Tuple[List[Product], bool, bool, bool, bool]: current_exit = proxy_pool.get_exit_for_worker(worker_offset) if proxy_pool else None + # One paid-solve allowance for this page, shared by every attempt and + # every proxy rotation below (captcha_solver.SolveBudget). + budget = SolveBudget() for attempt in range(args.retries + 1): driver = None try: if current_exit and current_exit.auth_tuple(): # Selenium's --proxy-server cannot authenticate at all - # (CLAUDE.md §6) -- strip the credential and warn. + # -- strip the credential and warn. logger.warning("Proxy exit %s has credentials, but Selenium's --proxy-server " "cannot authenticate at all -- connecting WITHOUT credentials, " "which will likely fail against an authenticated proxy. Use " @@ -264,7 +271,7 @@ def _fetch_page(url: str, page_num: int, args, proxy_pool: Optional[ProxyPool], return [], False, False, False http_status = _navigation_status(driver) - detected = handle_captcha_if_present(driver, args) + detected = handle_captcha_if_present(driver, args, budget) rendered_ok = (False if status_rules_out_listings(http_status) else _wait_for_listing_markers(driver)) @@ -311,7 +318,7 @@ def _fetch_page(url: str, page_num: int, args, proxy_pool: Optional[ProxyPool], if ok and args.delay > 0: time.sleep(args.delay) - return products, ok, outcome == PAGE_BLOCKED, False + return products, ok, outcome == PAGE_BLOCKED, False, outcome == PAGE_PARSER_FAILED except Exception as e: error_msg = redact_secret_patterns(str(e)) @@ -413,13 +420,15 @@ def scrape(args) -> int: refuse_concurrency_with_cdp_endpoint(args.concurrency, args.cdp_endpoint) url_1 = page_url(args.url, 1) - products_1, ok_1, blocked_1, remote_1 = _fetch_page(url_1, 1, args, proxy_pool, worker_offset=0) + (products_1, ok_1, blocked_1, remote_1), parser_failed_1 = split_page_result( + _fetch_page(url_1, 1, args, proxy_pool, worker_offset=0)) page_results = {1: products_1} failed_pages = [] if ok_1 else [1] any_blocked = blocked_1 any_remote_error = remote_1 pages_completed = 1 if ok_1 else 0 + parser_failed_pages: List[int] = [1] if parser_failed_1 else [] unattempted_pages: List[int] = [] exhausted_pages: List[int] = [] @@ -432,7 +441,11 @@ def scrape(args) -> int: if remaining_pages: def fetch_fn(page_num, worker_offset): url = page_url(args.url, page_num) - return _fetch_page(url, page_num, args, proxy_pool, worker_offset) + result, parser_failed = split_page_result( + _fetch_page(url, page_num, args, proxy_pool, worker_offset)) + if parser_failed: + parser_failed_pages.append(page_num) + return result initial_skus = {p.sku for p in products_1 if p.sku} more_results, more_failed, more_blocked, more_remote, more_unattempted = \ @@ -456,6 +469,7 @@ def fetch_fn(page_num, worker_offset): remote_api_error=any_remote_error, allow_empty=args.allow_empty, started_at=started_at, exhausted_pages=exhausted_pages, url=args.url, + parser_failed_pages=parser_failed_pages, ) @@ -464,29 +478,29 @@ def parse_args(): p.add_argument("--url", default=None, help="zimmo.be search-results URL (falls back to ZIMMO_URL from .env)") p.add_argument("--category", default=None, help="Label to tag output rows with (optional)") - p.add_argument("--pages", type=int, default=1, help="Number of result pages to fetch") - p.add_argument("--delay", type=float, default=0.0, + p.add_argument("--pages", type=positive_int, default=1, help="Number of result pages to fetch") + p.add_argument("--delay", type=non_negative_float, default=0.0, help="Seconds to sleep after each successfully-fetched page (rate limiting)") - p.add_argument("--retries", type=int, default=2, help="Retries per page on a transient failure") - p.add_argument("--retry-delay", type=float, default=3.0, help="Seconds to wait between retries") - p.add_argument("--concurrency", type=int, default=1, + p.add_argument("--retries", type=non_negative_int, default=2, + help="Extra attempts per page on a transient failure (0 = one attempt)") + p.add_argument("--retry-delay", type=non_negative_float, default=3.0, help="Seconds to wait between retries") + p.add_argument("--concurrency", type=positive_int, default=1, help="Parallel workers for pages 2+ (page 1 is always fetched alone). " "Not supported together with --cdp-endpoint.") p.add_argument("--format", choices=["json", "csv", "both"], default="both") p.add_argument("--out", default="zimmo_listings", help="Output file prefix") p.add_argument("--proxy", default=None, help="Proxy URL. Credentials, if present, are STRIPPED with a warning -- " - "Selenium's --proxy-server cannot authenticate at all (CLAUDE.md §6).") + "Selenium's --proxy-server cannot authenticate at all.") p.add_argument("--proxy-file", default=None, help="Path to a file of proxy URLs, one per line") p.add_argument("--proxy-rotate", action="store_true", default=True, help="Rotate to a different pool exit on a dead-proxy error (default: on)") p.add_argument("--no-proxy-rotate", dest="proxy_rotate", action="store_false", help="Disable proxy rotation -- retry the SAME exit on every failure instead") p.add_argument("--proxy-shuffle", action="store_true", help="Shuffle the proxy pool's exit order at startup") - p.add_argument("--proxy-block-retries", type=int, default=3, + p.add_argument("--proxy-block-retries", type=positive_int, default=3, help="Treat a proxy exit as blocked after this many failures") p.add_argument("--twocaptcha-key", default=None, help="2captcha.com API key") - p.add_argument("--captcha-api", default=None, help="Alternate captcha-solving API base URL (advanced)") p.add_argument("--solve-captcha", choices=["always", "when-blocked", "never"], default="when-blocked") p.add_argument("--min-score", type=float, default=None, help="Minimum acceptable reCAPTCHA v3 score to request from the solver") @@ -505,6 +519,7 @@ def parse_args(): apply_env_defaults(args) if not args.url: p.error("--url is required (or set ZIMMO_URL in .env)") + refuse_proxy_with_cdp_endpoint(p, args) return args diff --git a/smoke_test.py b/smoke_test.py index b8ac93c..269eedf 100644 --- a/smoke_test.py +++ b/smoke_test.py @@ -200,7 +200,7 @@ def main() -> int: no_turnstile = detect_turnstile_challenge(SAMPLE_LISTING_HTML, "https://www.zimmo.be/nl/") ok &= check("no false-positive Turnstile detection on clean page", no_turnstile is None) - # --- New in the CLAUDE.md-aligned rewrite: output contract (§9) --- + # --- Output contract --- from output_writer import finish_run, dedupe_by_sku, write_csv, Product, EXIT_ZERO_PRODUCTS, EXIT_OK with tempfile.TemporaryDirectory() as tmp: @@ -225,7 +225,7 @@ def main() -> int: code_never = finish_run([], never_prefix, "both", pages_requested=3, pages_completed=0, failed_pages=[1], unattempted_pages=[2, 3], allow_empty=True) ok &= check("0 rows because no page was ever FETCHED is exit 5, not 4 -- even with " - "--allow-empty, and with nothing written (CLAUDE.md §25)", + "--allow-empty, and with nothing written", code_never == EXIT_FETCH_FAILED == 5 and not os.path.exists(never_prefix + ".json") and not os.path.exists(never_prefix + ".meta.json")) @@ -243,7 +243,7 @@ def main() -> int: ok &= check("dedupe_by_sku keeps the FIRST occurrence (page order, not arrival order)", len(deduped) == 2 and deduped[0].url == "first-seen") - # --- New in the CLAUDE.md-aligned rewrite: JSON-LD edge cases (§4) --- + # --- JSON-LD edge cases --- SAMPLE_JSONLD_GRAPH_HTML = """