Skip to content

Keep good output on a failed write, escape CSV formulas, fail the canary without a secret - #3

Merged
jehrr merged 1 commit into
mainfrom
fix/audit-2026-10-14
Oct 15, 2026
Merged

jehrr merged 1 commit into
mainfrom
fix/audit-2026-10-14

Conversation

@jehrr

@jehrr jehrr commented Oct 15, 2026

Copy link
Copy Markdown
Collaborator

Problem

An audit of main at 47789df found:

  • Output files were written by truncation. If json.dump failed partway, a good 994-byte out.json and its out.meta.json were left as 16 bytes of invalid JSON. The previous good run was destroyed by the attempt to replace it.
  • CSV cells were not formula-escaped. =HYPERLINK(...), +SP, @me and -cmd came back from the CSV unchanged. These columns hold third-party text: titles, attributes, optionals, dealer names.
  • A copied .env.example broke the README examples. It shipped an active WEBMOTORS_URL, so after cp .env.example .env, --make volkswagen --model gol exited 2 in all three engines.
  • The Scraping Browser 401 hint said credentials "last about a day". README, TROUBLESHOOTING and .env.example repeated it, and the canary used it to justify having no secret. That figure is not a constant.
  • The canary had never contacted the site. It had no secret, so it skipped and ended green every day.
  • Smaller issues:
    • The bug-report template's example used flags this scraper does not have.
    • The banned-wording scan read virtualenvs.
    • Negative --delay, --retry-delay, --proxy-block-retries and --concurrency values, and --retries in the Scraper API client, were accepted or crashed later.
    • Comments cited private notes by section number.

Resulting behaviour

  • Atomic writes. output_writer._atomic writes a temporary file in the target's directory, then flushes, fsyncs and os.replaces it. On any error it unlinks the temporary file. A new file gets 0o666 & ~umask, and an existing file keeps its mode. JSON, CSV (newline=""), the sidecar and diff_runs.py --out all go through it. The three files are replaced one at a time, not as one transaction.

  • CSV escaping. A string cell starting with = + - @ \t \r \n gets a leading ', applied after list joining. Numbers are left alone and JSON is unchanged. csv_cells_escaped goes into the sidecar, underneath any keys the caller passes in extra.

  • WEBMOTORS_URL. .env.example ships the line commented out. Each engine records typed_url before env_config.apply. An environment URL then gives way to typed search flags, to --ads-file, or to a --mode that differs from it. A typed --url together with filter flags is still refused.

  • CLI validation. These are now argparse errors (exit 2) that name the flag:

    Where Flag Lowest accepted value
    All three engines --delay, --retry-delay, --proxy-block-retries 0
    All three engines --concurrency, --pages 1
    scraper_api_client.py --retries, --delay, --retry-delay 0
    scraper_api_client.py --pages 1

    --retries 0 is still legal in the Scraper API client because there it counts extra attempts.

  • The 401 hint now says the credentials were refused (expired, revoked or mistyped). It no longer states a lifetime.

  • Canary. With neither the WEBMOTORS_PROXY nor the WEBMOTORS_CDP_ENDPOINT secret, every job fails with ::error:: naming both. Either secret enables the run, and both are mapped into env:. The car-search job also reads its first 3 adverts in ad mode, which has its own assertions. Exit 3, 4, 5 or 6, or zero rows, makes the job red.

  • Smaller fixes:

    • The bug template now shows a Gol search.
    • The banned-wording scan skips directories that contain pyvenv.cfg.
    • Comments state each rule in place. A new suite check, check_no_private_citations_in_shipped_files, fails if such a citation comes back. Its tokens are assembled from pieces so the check does not match itself.

Evidence

  • Offline suite, core-only venv: 445 passed, 0 failed (388 before). The 6 skipped groups are absent engine libraries.

  • One venv per engine (requirements.txt plus requirements-<engine>.txt, no browser download, pip check clean):

    Engine Passed Failed
    playwright 459 0
    selenium 447 0
    puppeteer 447 0
  • ci_checks.py: --help-check, --sample-check and --secret-check (45 files) are all clean.

  • Controls. Each was run with PYTHONDONTWRITEBYTECODE=1 and __pycache__ removed. Every one turned the suite red on the named check, and the restored tree was green again:

    • non-atomic json, csv and meta
    • temporary-file cleanup removed
    • flat 0o644
    • no chmod (0600)
    • existing mode ignored
    • no CSV escape
    • escape before list joining
    • numbers escaped
    • housekeeping merged over extra
    • environment URL treated as typed
    • active WEBMOTORS_URL in .env.example
    • --delay unvalidated in playwright
    • --concurrency unvalidated in selenium
    • Scraper API --retries unvalidated
    • canary step gated on the secret again
    • canary ::notice:: skip
    • 401 "about a day" restored
    • planted citation in a .py file, a section sign in CHANGELOG and one in a workflow
    • banned phrase inside a .venv (green with pyvenv.cfg, red without it)
  • Probe: parse_args for all three engines, with driver modules stubbed, env -i, and a copied .env:

    • The README's --make volkswagen --model gol --pages 3 and --category motos --make honda --sort year-desc parse, both with the new example and with WEBMOTORS_URL active.
    • --url ... --make fiat still exits 2.
    • Negative or zero values exit 2 naming the flag.
  • No paid calls, credentials, proxies or live requests.

Not changed

  • Captcha solve budget: this repository has no paid solve path.
  • Credential-scan path handling and the no-proxy paths: already correct.
  • Exit codes: already match the contract.
  • Repository metadata: already correct.
  • Needs an owner decision, not changed: whether inert flags (--proxy/--concurrency/--fingerprint with --cdp-endpoint, --solve-captcha always, --captcha-api, --min-score) should error instead of warn.
  • Version bump, tag and release.
  • Release notes: v0.1.0 and v0.1.1 contain no private citations.

The canary stays red until the owner sets WEBMOTORS_PROXY (a residential exit) or WEBMOTORS_CDP_ENDPOINT (a 2Captcha Scraping Browser API endpoint, country-br).

🤖 Generated with Claude Code

…ags win over WEBMOTORS_URL

- JSON, CSV, the sidecar and diff_runs --out are written atomically
  (temp file beside the target, fsync, rename; new files 0o666 & ~umask,
  existing files keep their mode).
- CSV string cells starting with = + - @ tab CR LF get a leading
  apostrophe; numbers and JSON untouched; csv_cells_escaped in the sidecar
  under the caller's extra.
- .env.example ships WEBMOTORS_URL commented out, and an environment URL
  gives way to typed search flags, --ads-file or a different --mode.
- Negative delays/counts and --concurrency/--pages below 1 are usage
  errors naming the flag, in every engine and the Scraper API client.
- The CDP 401 hint no longer claims credentials last about a day.
- The canary fails by name without WEBMOTORS_PROXY/WEBMOTORS_CDP_ENDPOINT
  instead of skipping green, accepts either, and covers ad mode.
- Bug template example uses this scraper's flags; banned-wording scan
  skips virtualenvs; comments state rules instead of citing private notes,
  with a suite guard against their return.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@jehrr
jehrr marked this pull request as ready for review October 15, 2026 22:19
@jehrr
jehrr merged commit 212df8f into main Oct 15, 2026
7 of 9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant