docs(skills): gate blocked website captures - #2933
Conversation
james-russo-rames-d-jusso
left a comment
There was a problem hiding this comment.
Reviewed at ac76e7e1.
Applied the docs-PR grep discipline — verified every named identifier in the diff exists in the code at this SHA:
--capture-budget—packages/cli/src/commands/capture.ts:73(declared),:19(positive-integer validator from #2928),:130(parseCaptureBudgetcall).--skip-vision—packages/cli/src/commands/capture.ts:60(declared),packages/cli/src/capture/contentExtractor.ts:255(early-return short-circuits Gemini/OpenRouter captioning only, matching the "disables only optional AI image captioning" claim).HYPERFRAMES_CAPTURE_PHASE—packages/cli/src/commands/capture.ts:9(CAPTURE_PHASE_PREFIX = "HYPERFRAMES_CAPTURE_PHASE "), locked in bycommands/capture.test.ts:153,157.BLOCKED.md—packages/cli/src/commands/capture.ts:253-262(write path lives inside the outercatch, so it fires for any thrown capture error — including but not limited to thethrow new Error(blockedReason)#2931 added atcapture/index.ts:270).- JSON output
ok—commands/capture.ts:204(success path),:270(error path). - JSON output
warnings—commands/capture.ts:214. - JSON output
lastPhase—commands/capture.ts:215,types.ts:81. --timeout(page-navigation) vs--capture-budget(post-navigation) split is real and correctly framed.
The capture-skill-guardrails.test.mjs file is a nice self-enforcing pattern — a docs regression that removes the "hard stop" language or the two-budget distinction will break bun run test:skills. Assertions are appropriately loose (regex-with-content-windows) so wording tweaks won't require test churn.
LGTM from my side, leaving as a comment.
Nits (optional)
BLOCKED.mdis written for any capture failure, not just protection-page blocks. The file's body message reads "# Capture Failed" (not "Blocked"), which is the honest name. Reader of the init-and-scaffold reference might infer BLOCKED.md ⟹ protection page specifically; treating it as "capture failed hard stop" is exactly the right posture and the doc says that, but a parenthetical like "(also written on timeout / navigation errors — treat any occurrence as a hard stop)" on the CLI reference would head off the natural misread.--capture-budget 90000example: the shipped default is 120_000 ms (seeDEFAULT_POST_NAVIGATION_BUDGET_MSinpackages/cli/src/capture/index.ts). A comment like# post-navigation budget (default 120000)would let a reader see the flag as "override the default" rather than "recommended value".
What I didn't verify
- Whether motion-graphics/webpage/module.md's "generic asset-free fallback" language matches an existing pathway elsewhere in that skill (out of the diff's file set).
- The three shipped skill surfaces render correctly in whatever manifest consumer the runtime uses (visual/consumer parity —
skills-manifest.jsonhashes updated but I didn't rebuild).
vanceingalls
left a comment
There was a problem hiding this comment.
Skill-level guardrails matching the CLI-level fixes upstack. APPROVE as the stack tip.
Notable things I checked and liked:
BLOCKED.mdlineage is valid — I verified the artifact skills gate on actually exists.packages/cli/src/commands/capture.tsline 253-262 (unchanged in this stack) writesBLOCKED.mdtooutputDirin the outer catch block whenevercaptureWebsite()throws. So#2931's newdetectBlockedPagethrow path correctly produces the file that these docs gate on. The three signals now agree: non-zero exit ANDok:falsein JSON ANDBLOCKED.mdon disk.- Three-timeout separation in the CLI reference is exactly the right framing for an outer caller (EF): "An outer caller timeout leaves the capture result unknown; it does not prove HyperFrames hung or that the navigation timeout should be increased." Prevents the exact "misdiagnose EF watchdog kill as HF hang" failure mode you called out in
#2928's PR body. HYPERFRAMES_CAPTURE_PHASEdocumented as the watchdog contract — pairs with EF#44093's_last_capture_phaseextractor. Skills authors can now read what the stream is for, and the CLI-reference test at line 165 locks the substring so a rename in one place forces a rename in the other.- "Very little text + empty asset catalog" heuristic — this is the harder-to-articulate case where
detectBlockedPagereturns undefined (structural signals didn't cross the bar) but the resulting capture is still unusable for the workflow. Skills now require the agent to STOP on that combination even without a hardBLOCKED.md. Correct discipline — HF can't decide per-workflow what "usable" means (a portfolio with 40 text chars may be fine for a photography skill but not for a copy-heavy product-launch skill), so the workflow-level gate is the right layer. - "No synthetic no-capture fallback after a failed URL capture" — this is the anti-pattern that would produce STUDIO-5419-style behavior at the skill layer: user gave a URL, capture failed, skill invents a substitute source page. Explicit prohibition in the product-launch SKILL.md prose + regex-locked assertion in the guardrails test. The test at line 156 (
/do not[\s\S]{0,120}synthetic[\s\S]{0,120}fallback/i) locks the two-word invariant across ~120 chars, so future edits can rewrite around it but not remove the ban. - "Fresh output directory per retry" in the CLI reference — the assertion at line 168 catches this substring. Important for the STUDIO-5419 style: if a previous blocked attempt left partial
capture/extracted/tokens.jsonin./capture/, a naive retry into the same dir would merge partial + fresh output. Fresh-dir discipline avoids that. - Regression tests use
read()helper andassert.match(text, /pattern/)— same shape as the description-locked test in#2930(--capture-budgetdescription asserts). Consistent pattern for "these words in this doc are load-bearing" tests. Runs underbun run test:skills(461 passing per PR body).
Nits (none blocking):
- The
BLOCKED.mdcontent prose still suggests "Re-run with a longer timeout" — that's misleading when the block reason isdetectBlockedPage-detected (HTTP 403 + minimal DOM). A longer timeout won't help a 403. Follow-up in a separate PR would be to feed the specific block reason (HTTP status, blocked title,hasChallengeElement) into the "What to try" section so the file's advice matches the actual evidence. Doesn't need to be this PR. assert.match(reference, /--timeout[^\n]+navigation[^\n]+--capture-budget/i)(line 168) is over-locked to a specific sentence shape. If someone rewrites to "The post-navigation budget is--capture-budget, distinct from the page-load--timeout," (semantically equivalent, different word order), the test breaks. Consider two separate assertions on/--timeout[^\n]+navigation/iand/--capture-budget[^\n]+(cooperative|post-navigation)/i— decoupled from ordering.- The product-launch gate now requires
ok: true— I checked that the workflow prose upstream in that file also gets updated to actually runcapture --json(line 55:Run capture with: npx hyperframes capture "<URL>" -o ./capture --json). Consistent. If a follow-up ever removes--jsonfrom that invocation for any reason, the gate becomes untestable — the guardrails test at line 154 catches this (assert.match(skill, /hyperframes capture[^\n]+--json/)), so the invariant is locked. Good defensive coupling. Warnings about a degraded optional phase are acceptable only when this structural gate still passes.— subtle but important.phase: "vision", status: "degraded", reason: "request-timeout"is a legitimate outcome that shouldn't fail the gate (Gemini stuttered but tokens.json still landed). The prose captures the distinction cleanly, but I don't see a regex-locked assertion for this specific carve-out. If someone re-reads the doc and decides "degraded means gate fails," they could tighten unhelpfully. Consider addingassert.match(skill, /degraded[\s\S]{0,120}acceptable/i)to the guardrails.
CI clean at ac76e7e1. Ready to ship.
Full stack summary (7 PRs): HF #2928 (contract) → HF #2929 (tests) → HF #2930 (live budget propagation) → HF #2931 (blocked/degraded classification + PII scrubbing) → HF #2933 (skill-layer guardrails), plus EF #44093 (subprocess bound/reap/scrub) → EF #44094 (frame commit boundary) → EF #44095 (fail-closed persistence + BaseException catch). Coherent commit-boundary story from CLI primitives through workflow orchestration through agent skill guardrails. Ready to ship the whole thing.
— Via
ac76e7e to
2a02643
Compare
60b6547 to
9ae0007
Compare
2a02643 to
30f3830
Compare
The base branch was changed.
What
Website-to-video workflows now stop on blocked or semantically unusable captures instead of continuing from partial files or inventing a replacement page.
Stack 5/5 for STUDIO-5419. Base:
fix/studio-5419-blocked-and-degraded-captures.Why
The CLI runtime fixes expose reliable
ok,BLOCKED.md, phase, and budget signals, but the shipped product-launch and webpage skills did not require agents to honor them. A protection page with little text and no assets could therefore still be treated as source material, while an outer caller timeout could be misreported as proof that HyperFrames itself hung.How
Define the capture gate across the product-launch, webpage-motion, and CLI reference surfaces: require fresh
ok: trueoutput with no block marker, distinguish navigation, capture-budget, and caller deadlines, and require a real screenshot or DOM for show-it-as-is work. Regression tests lock those instructions. Changed LOC: 104.Test plan
bun run test:skills— 461 passed