fix(cli): reject blocked and degraded captures - #2931
Conversation
vanceingalls
left a comment
There was a problem hiding this comment.
Structured block detection + PII-clean provider errors + stalled-body timeout fix. APPROVE as stack tip.
Notable things I checked and liked:
- Stalled-response-body timeout fix — this is a subtle but real correctness change. #2928's
runBoundedVisionRequestonly wrapped thefetch()call itself, so oncefetchresolved with a Response object, the timeout was cleared (finally clause) andawait res.text()/await res.json()could hang forever if the upstream sent headers then stalled the body stream. #2931 moves the entire block (fetch +res.text()+res.json()) INSIDErunBoundedVisionRequest, tying the body-read to the same abort controller. The streaming-body test atcontentExtractor.test.ts:96-141proves this —ReadableStreamthat enqueues once and never closes, race against 250ms wrapper, asserts capture terminates cleanly. Good. - PII/credential redaction in provider error paths:
- OpenRouter:
throw new Error("OpenRouter " + res.status + " " + res.statusText + ": " + detail.slice(0, 200))→throw new Error("OpenRouter request failed with HTTP " + res.status). Body still drained (await res.text()) but discarded — socket cleanup without leaking upstream response text. - Outer catch:
warnings.push("... failed: ${err}")→warnings.push("...captioning failed; captions omitted."). No${err}interpolation. - Regression test at line 152-183 ("reports a rejected Gemini request without leaking its error detail") asserts
warnings.join(" ")doesn't contain the fake key"gemini-secret-key". Locks the invariant.
- OpenRouter:
detectBlockedPageregex is anchored^...$so titles like "Forbidden Fruit Photography", "Attention Required: A Photo Essay", and "Why websites say Access Denied" don't false-positive. Test coverage atpageBlockDetection.test.ts:56-89walks every trap I could think of.- AND-combined evidence:
isMinimalDom AND (challenge || blockedStatus || blockedTitle). Low text alone stays a warning-continue (image-led portfolios survive), only structural minimal-DOM + at least one block signal fails fast. Correct discrimination. detectBlockedPageruns BEFOREphase("navigation", "completed")— so EF seesnavigation startedbut no completion when we throw. Informative: "we attempted navigation but it was rejected as blocked."VisionCaptionOutcome.failedRequests+provider-errorreason — new discriminator between "timed out" and "rejected". Priority inresolveVisionPhaseCompletionis correct:budget-exhausted>request-timeout>provider-error>completed. Actionable-info-first.- Lottie
previewfield now optional — old code wrotepreview: "assets/lottie/previews/foo-preview.png"unconditionally in the manifest, even when the screenshot was skipped for budget reasons. New code only setspreviewwhen the screenshot actually landed. Fixes a real "manifest points at nonexistent file" bug.
Nits (none blocking):
- Outer-catch swallows specific error info (line 380):
} catch { failedRequestCount = Math.max(1, failedRequestCount); warnings.push("... captioning failed; captions omitted."); }. Trade-off between PII safety and debuggability — if a TypeError from a shape change fires here, we see "captioning failed" with zero context. Considerconsole.error(err)ordiag.debug(err)for a non-user-visible channel, keeping the warning sanitized. - HTTP 404 not in
hasBlockedStatus— a 404 with minimal DOM and no blocked-title regex will slip through to warning-continue. Probably fine because 404 is user error (bad URL) not "the site is protecting itself," and the downstream check will fail on empty tokens. Just noting that the taxonomy is "blocked" specifically, not "unusable". _BLOCKED_TITLEregex is ~500 chars of one line — a follow-up refactor to extractconst BLOCKED_STATUSES = [401, 403, 429]andconst BLOCKED_TITLE_PATTERNS = [...]would help maintainability. Not blocking.
CI passing at e983fc3c. Regression tests + Fuji-shape unit coverage are strong.
— Via
james-russo-rames-d-jusso
left a comment
There was a problem hiding this comment.
Reviewed at e983fc3c.
Solid finish to the STUDIO-5419 stack. Verified the biggest carry-over from #2930: res.text() and res.json() are now inside the runBoundedVisionRequest callback (contentExtractor.ts:294-327 area), so Promise.race([request(signal), timeout]) bounds the entire fetch+body-read+parse flow. The two new "body never ends" tests exercise both the success and failure branches — that finding from my #2930 review is closed here.
Also verified the PR-body invariants:
- HTTP 403 evidence:
pageBlockDetection.ts:15-17checks status 401/403/429. - Narrow blocked-title matching: previously an unanchored substring (
/just a moment|attention required|access denied/i), now anchored^…$. - Structurally minimal captures:
isMinimalDom = bodyChildCount <= 5 && textLength < 500gates all classifications; image-led sites with rich DOM pass through. - Lottie preview omission:
previewon the manifest tuple is optional (preview?: string); only assigned after successfulscreenshot(), conditionally spread via...(preview ? { preview } : {}). - CapturePhaseProgress
reasonunion: widened to includeprovider-error— backward-compatible for consumers (no exhaustive switch found on that union outside an unrelated FpsParseResult).
3 concerns flagged inline. Notable non-inline items:
- Manifest tuple
preview?: stringis technically breaking for JSON consumers. Downstream code that readslottie-manifest.jsonand assumespreviewis always present will break on skipped previews. Worth a docs pass on the manifest schema. - Questions I couldn't answer from the diff alone:
- Was there a real production page that hit the old over-broad regex? A regression test with that page's title would harden the narrowing.
- Should the block-detection throw carry an
err.code = 'CAPTURE_BLOCKED'socommands/capture.ts:257can distinguish blocked from generic errors without regex-matchingerrMsg? - Does AbortSignal actually release the underlying HTTPS socket in undici when body read is in-progress? The synthetic ReadableStream test bounds the promise but doesn't prove socket cleanup.
What I didn't verify
- End-to-end behavior through the orchestrator against a real 403 / real Cloudflare interstitial (no integration test exists).
- Locale variants of protection-page titles (non-English Cloudflare interstitials, etc.).
- Whether any downstream tool consumes
lottie-manifest.jsonand assumespreviewis always present.
ce7ffce to
d360760
Compare
60b6547 to
9ae0007
Compare
What
Reject structurally recognized 403/protection-page captures, report bounded provider failures as degraded output, narrow blocked-title matching, and omit Lottie previews skipped after budget exhaustion.
Stack 4/4 for STUDIO-5419. Base:
fix/studio-5419-capture-budget-propagation.Why
Fuji returned a protection page with no usable structure. HyperFrames exited 0, so EF's frame builder surfaced the misleading
could not resolve surfaceerror instead of the HTTP 403 capture failure.How
Classify captures from HTTP status plus minimal-content/structure signals, fail fast on confirmed protection pages, and preserve degraded optional-stage diagnostics. Changed LOC: 503. The stack tip is tree-identical to superseded PR #2927.
Test plan