Skip to content

Review: audit response — the --retries 0 artefact, and the regression it hid - #6

Merged
jehrr merged 1 commit into
review-base/pre-audit-responsefrom
review/audit-response
Oct 13, 2026
Merged

jehrr merged 1 commit into
review-base/pre-audit-responsefrom
review/audit-response

Conversation

@jehrr

@jehrr jehrr commented Oct 13, 2026

Copy link
Copy Markdown
Collaborator

Retrospective, for review only. This content is ALREADY on main as
9dbf278 — I pushed it directly, which was a deviation from this repo's
own practice (PRs #2–#5 all went through review) and I should have asked.
This PR exists so the change can be read as a diff.

Its base is review-base/pre-audit-response, pinned at 0d4831b — the
tree the audit examined. Merging it does nothing to main. Close it
when the review is done, and both branches can go.

Response to the third-party audit of 0d4831b (2026-10-13).

The audit's headline finding was real, and its cause was not

It reported exit 3 on a 39-byte response and concluded availability was
unproven. Its command carried --retries 0. The load loop is
range(1, retries + 1), so zero runs no navigation at all and the engine
then judges about:blank — which is 39 bytes.

One variable changed, which is the rule for this:

command result
--retries 0 (the audit's) exit 3, 39 bytes, four attempts, 37 s
--retries 1 exit 0, 1,395,922 bytes, 50 rows, 100% coverage, 11 s
README command, 3 pages exit 0, 250 rows, complete, 14 s

Two general tells, both cheap: a "refusal" returning in tens of milliseconds
is not a round trip, and 39 bytes is an empty DOM rather than anything a
server sent.

So the bug is ours and is fixed — --retries 0 is now a usage error (exit 2)
— and the site was serving throughout. A corollary the audit could not have
known: this flag is affected in 26 of 45 repos in this family.

Related: its "four requests may have increased load" is not what happened.
Four outer attempts × zero navigations each = zero requests.

What the audit did not find, because it ran one mode

Between 2026-09-17 and 2026-10-13 OpenSea stopped inlining its state on the
ranking and on the activity feed; only a collection page still ships rows.
Both modes fell through to the DOM fallback — an address and a name, every
price null — and reported complete, exit 0:

mode before after
collections 23 rows, dom 161, graphql
activity 24 rows, dom 300, graphql
items 250 250 (unaffected)

data_source read dom on every row — the column working exactly as
designed — and nothing acted on it.

Three changes come out of that, and the third is how it reached a release:

  • page 1's rows are used only when the page actually inlined some, so a
    mode that stops inlining is handled by the path that already existed for a
    request the page does not answer: the feed starts from the endpoint;
  • a run that is nonetheless all-fallback says so and is no longer complete;
  • all three canary jobs assert the route mix. The items job already did
    and would have gone red the next morning; the other two did not.

A claim of mine that rotted, corrected rather than dropped

The price-boundary stop rested on passing a null-sort-key cursor back
answering Something went wrong (measured 2026-09-17). Today that cursor
pages normally — a walk went 1,000 rows deep, pages 4 to 10 carrying zero
priced items. OpenSea fixed their end.

The behaviour is unchanged, because a floor scrape should still stop where
the prices stop, but it is now keyed on a page with no priced row — a
property of the catalogue rather than of the transport. Every copy of the old
sentence in the code and the docs is corrected and dated; a captured response
for both behaviours is pinned in the fixtures.

The rest of the audit's list

  • P1.1, cooldown by response kind — a throttle is no longer a block.
    429/503 get their own budget (separate from --retries), honour
    Retry-After — the in-page fetch had to start returning that header, or
    honouring it was dead code — and wait at the same exit. No throttle
    has ever been seen here
    : x-ratelimit-remaining reads 399 and did not
    move across a burst of 60 requests, so this is implemented, unexercised,
    and driven offline against a fake driver for exactly that reason.
  • P1.2, canary regression signal — the route-mix assertions above.
  • P1.3, endpoint fixtures — four captured responses: an invalid cursor,
    the null-key cursor, an unknown collection, a schema validation error. All
    four come back HTTP 200, so a caller branching on the status alone
    reads every one as success. Captured, not assembled.
  • P2.4, launch errors — a browser that will not start is exit 2 with one
    actionable line; --verbose re-raises the driver's own stack.
  • P2.6, resume — last_cursor in the sidecar, so the one fact a resume
    would need is not discarded. Not the feature itself.
  • Beyond the list — Selenium with an authenticated proxy strips the
    credentials it cannot send, gets an empty document, and used to advise
    raising --delay. It now names what it did, because it knows.

What was NOT changed, and why

  • No competitor comparison in the README. This repo belongs to 2Captcha;
    a README carrying Bright Data's pricing is their advertising at our
    expense. The audit's own comparison is explicitly not a benchmark — it ran
    neither service. The honest half (what this repo does not do
    operationally) is a paragraph I will add on request.
  • The engine matrix stays at three. It is a deliberate design: the three
    keep each other honest, and they did today — Selenium and pyppeteer
    independently reproduced Playwright's numbers. CI already builds a venv per
    engine, requirements are already split, and pyppeteer's unmaintained state
    is already in the README.
  • No shared-loop refactor (P2.5). It is a refactor whose verification is
    a live run of three engines, and it does not belong in the same change as
    edits verified by planted faults.
  • No checkpoint/resume (P2.6) and no schema version marker (P3.9).
    Both are product decisions pending family-wide, not defects here.
  • The urllib3/LibreSSL warning is the auditing machine's environment
    (system Python 3.9 on macOS), not a repo defect; CI tests 3.9 and passes.
    The audit says as much itself.

Verification

  • 11,045 offline assertions pass, in both virtualenvs.
  • Six planted faults. Each edit asserted to have changed the file, each
    run checked for the expected message. One control stayed green — that is
    a hole in the suite rather than a pass, and the check was tightened until
    it caught the fault.
  • All three engines, all three modes, live. Selenium and pyppeteer reproduce
    Playwright's rows.
  • CI green on 9dbf278 (waited on the run whose headSha matched).
  • Canary dispatched and green from a bare GitHub runner with no secrets:
    items 350 / collections 161 / activity 300 — a second network
    independently contradicting the audit's availability conclusion.

🤖 Generated with Claude Code

A third-party audit (2026-10-13) reported this site as refusing us — exit 3,
39 bytes, availability unproven. Its command carried `--retries 0`. The load
loop is `range(1, retries + 1)`, so zero runs NO navigation and the engine
then judges `about:blank`, which is 39 bytes. §28's rule — change ONE
variable from the audit's own command — settles it:

  --retries 0        exit 3, 39 bytes, four attempts, 37 s
  --retries 1        exit 0, 1,395,922 bytes, 50 rows, 100% coverage, 11 s
  defaults, 3 pages  exit 0, 250 rows, complete, 14 s

So the audit was right about the symptom and wrong about the cause, and the
symptom was ours. `--retries 0` is now a usage error.

THE PART THE AUDIT DID NOT FIND, because it ran one mode. Between
2026-09-17 and today OpenSea stopped inlining its state on the ranking and
the activity feed; only a collection page still ships rows. Both modes fell
through to the DOM fallback — an address and a name, every price null — and
reported `complete`, exit 0, with 23 and 24 rows against the 160 and 200 the
endpoint served the same minute. `data_source` said `dom` on every row,
which is that column working; nothing read it.

  page 1's rows are used only when the page actually inlined some, so a mode
  that stops inlining is handled by the path that already existed for a
  request the page does not answer: the feed starts from the endpoint;
  an all-fallback run says so and is no longer `complete`;
  all three canary jobs assert the route mix — the items job already did and
  would have gone red, the other two did not, which is how this shipped.

Re-measured after the fix: items 250, collections 161, activity 300.

A CLAIM OF MINE THAT ROTTED, corrected rather than dropped. The price
boundary stopped a walk because passing a null-sort-key cursor back answered
`Something went wrong` (2026-09-17). Today that cursor pages normally — 1,000
rows deep, pages 4 to 10 carrying zero priced items. OpenSea fixed their end.
The stop survives because a floor scrape should still stop where prices
stop, but it is now keyed on a page with no priced row (§7: the catalogue,
not the transport). Every copy of the old sentence in code and docs is
corrected and dated.

ALSO, from the audit's list:

  a throttle is no longer a block. 429/503 get their own budget, honour
  Retry-After (the in-page fetch had to start returning that header, or
  honouring it was dead code) and wait at the SAME exit. No throttle has
  ever been seen here — `x-ratelimit-remaining` reads 399 and did not move
  across a burst of 60 — so it is implemented, unexercised, and driven
  offline against a fake driver for exactly that reason;
  a browser that will not start is exit 2 with one line, `--verbose` to
  re-raise, instead of a traceback that says "there is a bug here";
  four captured endpoint responses as fixtures — invalid cursor, the
  null-key cursor, an unknown collection, a validation error. All four are
  HTTP 200, so a caller branching on the status reads every one as success;
  `last_cursor` in the sidecar, so the one fact a resume would need is not
  discarded. Not a resume feature; that is a product decision.

And a message fix §28 would call for: Selenium with an authenticated proxy
strips the credentials it cannot send, gets an empty document, and used to
advise raising --delay. It now names what it did, because it knows.

Controls: six planted faults, each asserted to have changed the file, each
run checked for the EXPECTED message. One stayed green and was a hole in the
suite rather than a pass — a text check matched an assignment the fault left
alone — and was tightened until it caught it.

Not done, with reasons in the reply: the competitor table, shrinking the
engine matrix, the shared-loop refactor and full checkpoint/resume.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@jehrr
jehrr merged commit b119262 into review-base/pre-audit-response Oct 13, 2026
15 checks passed
@jehrr
jehrr deleted the review/audit-response branch October 13, 2026 15:22
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