fix(cli): capture the best declared favicon, not the first one

[miguel-heygen]

The problem

hyperframes capture reads the page's icon <link> tags, keeps only {rel, href}, and downloads them in DOM order, stopping at the first one that fetches successfully.

Plenty of sites declare a legacy .ico first and the good asset after it, so the .ico wins:

• linear.app declares .ico sizes="any", then an SVG, then a 180x180 apple-touch icon.
• notion.com declares .ico, then an unsized apple-touch PNG.
• stripe.com declares an SVG, a 96x96 PNG, a shortcut .ico, and a 180x180 apple-touch PNG.

The sizes and type attributes are the only evidence of which candidate is best, and they were discarded at the point of collection. page.html on disk does not keep the <link> tags, and only the winner's bytes are ever fetched, so nothing downstream can recover the choice.

The change

• The page evaluate now keeps sizes and type alongside rel and href. Same single evaluate, no second fetch of the page.
• New pure rankIconCandidates() orders candidates best-first: SVG, then largest declared size (an apple-touch-icon with no sizes counts as its spec size of 180), then .ico, then a mask-icon last. Stable within a tier, so DOM order breaks ties. sizes parsing handles 32x32, any (no declared pixel size), and multi-size lists like 180x180 167x167 (max wins).
• The download loop iterates the ranked order and still falls through to the next candidate when one fails, so ranking changes which icon wins, never whether one lands.

mask-icon, from review

The selector link[rel*="icon"] also matches rel="mask-icon", Safari's pinned-tab asset. That is a single-colour silhouette drawn in a browser-chosen tint, not the site mark, and it is served as an SVG. Ranking on format alone therefore promoted the outline above the page's real colour favicon on any site that declares one.

mask-icon now gets its own lowest tier. It is ranked last rather than dropped, so a page that declares nothing else still lands an icon instead of none. Two fixtures cover it: a mask SVG declared first loses to both a normal SVG and a 32x32 PNG, and a mask-icon alone is still returned.

Removing the mask tier reproduces the reported regression:

- Expected
+ Received
  [
+   "https://x.test/pinned.svg",
    "https://x.test/favicon.svg",
    "https://x.test/favicon.png",
-   "https://x.test/pinned.svg",
  ]

Verification

bun run --filter '@hyperframes/cli' typecheck and the full CLI suite pass (2889 passed, 3 skipped). The ranking tests are non-vacuous: replacing the comparator with DOM order fails all three site-shape cases, e.g.

- Expected
+ Received
  [
+   "https://linear.app/favicon.ico",
    "https://linear.app/favicon.svg",
    "https://linear.app/apple-touch-icon.png",
-   "https://linear.app/favicon.ico",
  ]

Ran the built CLI against two live sites; both now land the SVG instead of the .ico:

Site	File on disk	Dimensions
linear.app	assets/favicon.svg	16x16 viewBox, scalable vector
stripe.com	assets/favicon.svg	512x512 viewBox, scalable vector

Fixtures are transcribed from the sites' declared <link> tags, not from CLI output.

[james-russo-rames-d-jusso]

:large_green_circle: LGTM from my side — leaving as a comment.

Independent trace at a3f1f63ec44a57c0452f99766a007e659ae661e6 on heygen-com/hyperframes#3606. Read the four changed files at HEAD via gh api /contents?ref=<sha> (not the diff), verified the ranker's tier + tie-break logic against the site fixtures, and confirmed the download loop preserves fall-through discipline. Clean, well-scoped, well-tested fix.

Scope at HEAD

Four files, +209 / -5:

• packages/cli/src/capture/faviconRanker.ts — new pure module (parseSizes, isSvg, isIco, declaredSize, rankIconCandidates).
• packages/cli/src/capture/faviconRanker.test.ts — new; 3 site-shape witnesses + 5 unit witnesses.
• packages/cli/src/capture/index.ts — page evaluate now keeps sizes + type alongside rel + href; typed as IconCandidate[].
• packages/cli/src/capture/assetDownloader.ts — passes faviconLinks through rankIconCandidates() before iterating; parameter widened from Array<{ rel; href }> to IconCandidate[].

Nothing else in the diff. No coupling to other capture stages.

rankIconCandidates — pure, stable, tier logic verified

The function is a straight filter → map → sort → map. No fetch, no I/O, no globals mutated. Array.prototype.sort is stable per ES2019+, and the comparator additionally breaks ties by the original DOM index a.i - b.i, so tie order is deterministic across engines regardless of the language-level guarantee — belt-and-suspenders, correct.

• SVG tier (0): type === "image/svg+xml" OR pathname ends .svg. So a .svg href with no type still ranks as SVG (the LINEAR fixture exercises exactly this — favicon.svg with type: null).
• .ico tier (2): type === "image/x-icon" OR type === "image/vnd.microsoft.icon" OR pathname ends .ico. Both standard MIME spellings covered.
• Middle tier (1) — largest declared size wins: declaredSize() returns parseSizes(c.sizes) when > 0; otherwise falls back to 180 if rel includes the token apple-touch-icon, else 0. Comparator sorts b.size - a.size inside a tier, so larger wins.
• parseSizes: splits on whitespace, matches /^(\d+)x(\d+)$/i, takes the max edge. "any" and malformed tokens fall through and return 0. parseSizes("180x180 167x167") → 180 (verified by unit witness).
• URL parse: pathnameOf uses new URL(href).pathname.toLowerCase(); falls back to href.split(/[#?]/)[0] on parse failure. In practice the DOM l.href reflection returns absolute URLs so the fallback is defensive-only, but it protects against pathological inputs. Query strings and fragments do not contaminate the extension check.

Read shape verified: an SVG-declared candidate wins over any sized PNG, and any sized PNG wins over an .ico.

Page evaluate — sizes + type preserved

captureWebsite in packages/cli/src/capture/index.ts extends the existing page1.evaluate — same single evaluate, no second page fetch, no second navigation. The selector still catches link[rel*="icon"] AND link[rel="apple-touch-icon"]. The IIFE now returns { rel, href, sizes: l.getAttribute('sizes'), type: l.getAttribute('type') }. Using getAttribute rather than the reflected properties is the right call for sizes (the reflected property is a DOMSettableTokenList; the string attribute is what the ranker expects).

Download loop — fall-through preserved

In assetDownloader.ts, the favicon block is now:

for (const icon of rankIconCandidates(faviconLinks || [])) {
  ...
  try {
    ...
    const buffer = await fetchBuffer(icon.href, ...);
    if (buffer) { writeFileSync(...); assets.push(...); break; }
  } catch { /* skip */ }
}

Fall-through discipline intact: fetch failure or exception continues to the next-ranked candidate; only a successful buffer-write breaks. The remainingMs <= 0 guard is preserved. If every candidate fails, the loop exits without pushing to assets — same silent-empty behavior as before, no regression. Ranking changes which icon wins, never whether one lands. Matches the PR body's claim.

Test witnesses — the three site fixtures

Each fixture transcribes the real declared attributes:

• LINEAR (.ico sizes="any" → SVG → 180 apple-touch): expects [svg, apple-touch, .ico]. Under the ranker: SVG is tier 0; apple-touch is tier 1 size 180; .ico is tier 2. ✓
• NOTION (.ico unsized → unsized apple-touch PNG): expects [apple-touch, .ico]. Under the ranker: apple-touch is tier 1 with declaredSize falling back to APPLE_TOUCH_DEFAULT_PX = 180; .ico is tier 2. ✓
• STRIPE (SVG → 96 PNG → shortcut .ico → 180 apple-touch PNG): expects [svg, 180 apple-touch, 96 png, .ico]. Under the ranker: SVG tier 0; apple-touch tier 1 size 180; PNG tier 1 size 96; shortcut .ico tier 2. ✓ Also note this fixture correctly exercises the .ico extension-detection path even though the shortcut carries no type — matches how Stripe actually declares it.

The two edge witnesses (drop empty href, keep DOM order within tier) close the two other correctness invariants.

The parseSizes block covers the three interesting input shapes: single, multi-size list, any, and null/undefined.

Break-on-purpose non-vacuousness

The author demonstrates one witness (comparator → DOM order). I traced two more mentally:

• Deleting the APPLE_TOUCH_DEFAULT_PX fallback in declaredSize → NOTION regresses (.ico and unsized apple-touch both become size 0 in tier 1 vs tier 2, apple-touch still wins by tier — actually still passes). Swapping the apple-touch fallback to 0 doesn't break NOTION because it's a tier crossing. So the 180 default is only load-bearing when a .ico and an unsized apple-touch are both tier-comparable AND another sized candidate exists — LINEAR fixture would still land SVG first regardless of the fallback. Not a gap in the test set, but a note that the 180-default itself is a soft constant — its value only matters against other sized non-SVG candidates. Non-blocking observation.
• Flipping pathnameOf(...).endsWith(".svg") to startsWith(".svg") → LINEAR SVG demotes to tier 1 with size 0, apple-touch (tier 1 size 180) beats it, output becomes [apple-touch, svg, .ico], LINEAR test fails. ✓ Caught.

Reasonable coverage for a bounded, pure module.

Non-blocking notes

None of these gate merge. All are follow-up-worthy at author discretion.

1. apple-touch-icon-precomposed: the rel token check in declaredSize is .split(/\s+/).includes("apple-touch-icon"), which does NOT match apple-touch-icon-precomposed. Sites still shipping the legacy variant (rare on modern stacks, but present on some older marketing sites) with no explicit sizes will score 0 instead of 180. If the intent is to treat precomposed as the same asset class, rel.includes("apple-touch-icon") (substring, not token) — or an explicit includes("apple-touch-icon-precomposed") — closes it. Low-priority; the site-shape fixtures don't hit this today.

2. <link rel="manifest"> icons: not touched. A site whose best asset is only declared inside a web-app-manifest JSON (icons: [{ src, sizes, type }, ...]) still loses. Explicit follow-up scope if you want capture to keep improving on this axis.

3. data: URI hrefs: pathnameOf on a base64 data URI won't hit .svg/.ico extension checks, and safeFetch rejects non-http(s) protocols outright. So a data-URI-declared icon fails silently in the download loop. Pre-existing behavior, but worth naming if the intent of this fix is to widen the "we land the right icon" surface — data URIs are the pathological case that no ranking can save without a separate decode path.

4. Type-vs-bytes mismatch: the ranker trusts the declared type / extension. A site that declares type="image/svg+xml" but serves .ico bytes still lands garbage. Same shape as before this PR — not a regression. Post-fetch content-type sniff on the winner would harden but sits outside this PR's scope.

5. Middle-tier PNG vs. unsized apple-touch: an unsized apple-touch-icon scores 180, a PNG declared sizes="192x192" scores 192 → PNG wins. Correct per spec; just noting the ranker collapses <link rel="icon" type="image/png" sizes="192x192"> and <link rel="apple-touch-icon"> into one comparable tier. Matches the STRIPE fixture's intent.

Peer state at HEAD

GET /repos/heygen-com/hyperframes/pulls/3606/reviews returns [] at a3f1f63e. Requested reviewers: jrusso1020 + jerrai-bot-heygen. reviewDecision: REVIEW_REQUIRED, mergeable: MERGEABLE. No prior reviews to reconcile against.

CI state at HEAD

Preflight, Lint, Format, Typecheck, Build, Test: runtime contract, Producer unit + integration, SDK unit+contract+smoke, Studio load smoke, Smoke: global install, CLI smoke (required), CLI npx shim on ubuntu/macos/windows, Preview parity, CodeQL, Analyze (actions/python/javascript-typescript) — all green at HEAD.

Still in progress at time of review: Test, Tests on windows-latest: studio-engine-cli, Tests on windows-latest: studio-core. No failures. The change is CLI-only (no studio-engine touch), so I'd expect windows lanes to land green.

Coordination note

Miguel calls out this is the upstream half — after release, a one-line pin bump lands in the brand-cpu worker's Dockerfile downstream. That coord is outside this PR; flagging it here so the release-cut is scheduled with that in mind.

Routing

OSS heygen-com/hyperframes protection matrix: require_last_push_approval=true, dismiss_stale=false. The human stamp must be at the exact HEAD SHA post-last-push. Cut Room convention → routes to jrusso1020. I'm leaving as COMMENTED; the merge stamp is the human owner's call once the windows lanes finish.

— Review by Rames

[jerrai-bot-heygen]

Changes requested at exact head a3f1f63ec44a57c0452f99766a007e659ae661e6.

link[rel*="icon"] admits rel="mask-icon". The new ranker unconditionally promotes every SVG to tier 0, so a Safari monochrome pinned-tab asset can now beat the page’s actual color favicon/site mark. This is a behavior regression for the stated goal of capturing an accurate site mark: on pages that declare a mask-icon before a normal favicon, capture selects the silhouette.

Please exclude mask-icon candidates from this favicon flow (or explicitly rank them below normal favicon candidates) and add a fixture that proves a mask SVG does not beat a normal SVG/PNG favicon. The pure tiering, sizes/type preservation, fallback download loop, and current tests otherwise look good.

No merge/queue action taken.

Review by Jerrai

[miguel-heygen]

Good catch on mask-icon — fixed in c6f4b57: it now gets its own lowest tier (ranked last rather than dropped, so a page declaring nothing else still lands an icon), with fixtures proving a mask SVG loses to both a normal SVG and a 32x32 PNG, and that a mask-icon alone is still returned. Branch is also rebased onto current main to clear a conflict with the new asset-drop accounting.

[jerrai-bot-heygen]

Reviewed at exact head c6f4b57cc04607a9c002f7360aaf38a23c7dbedf.

The repair gives mask-icon a lowest tier before SVG classification, so a Safari pinned-tab silhouette cannot outrank a normal SVG/PNG favicon. It still remains a final fallback when it is the sole declared option; the existing downloader preserves ranked fall-through on fetch failure. The mask-versus-normal and mask-only fixtures are discriminating against the prior SVG-first behavior.

Current completed checks are green; remaining jobs are pending, which does not block a code-quality approval.

— Jerrai

[jrusso1020]

Approving at c6f4b57cc04607a9c002f7360aaf38a23c7dbedf.

The mask-icon tier is right, and the fixture that covers it discriminates twice rather than once. A silhouette losing only to the real SVG would be satisfied by any rule that breaks the format tie; the fixture also puts it behind a 32x32 PNG, which is the assertion that pins mask-icon below tier 1 rather than merely below tier 0. The mask-only case then proves it is ranked last and not filtered, which is the difference between a site with one pinned-tab declaration landing an icon and landing none.

I checked the body's load-bearing claim - "ranking changes which icon wins, never whether one lands" - at the loop rather than taking it. downloadAssets still walks every candidate, still breaks on the first success, and still counts a failure into drops.unavailable before continuing, so a ranked-first candidate that 404s falls through exactly as DOM-order did. The written filename comes from extname(new URL(href).pathname), not from the loop index, so reordering does not rename anything on disk. index survives only in the budget-exhausted count, which is a count and not an identity.

Two details that are easy to get wrong and are not wrong here. parseSizes scoring any as 0 rather than Infinity is the right call and the reason is in the fixtures: linear.app declares sizes="any" on its .ico, so an Infinity reading would have made the legacy file unbeatable inside its own tier. And pathnameOf going through new URL().pathname is what keeps Stripe's favicon.png?w=180&h=180 from being read as an unknown extension - the same call the downloader makes when it names the file.

One non-blocking gap, and it is narrow: the Apple spec-size default does not apply to the legacy spelling. declaredSize matches the token apple-touch-icon exactly, so rel="apple-touch-icon-precomposed" - one token, not two - scores 0 instead of 180. The selector does collect it, since rel*="icon" matches the longer string. I ran the two shapes side by side:

A) rel="apple-touch-icon-precomposed" (no sizes) vs a 32x32 png
     precomposed -> tier=1 size=0        32x32 -> tier=1 size=32
     ranked: small-32.png  >  touch-180.png

B) rel="apple-touch-icon" (no sizes), same page otherwise
     apple-touch -> tier=1 size=180      32x32 -> tier=1 size=32
     ranked: touch-180.png  >  small-32.png

Same page, one spelling apart, opposite winner - and the losing arm is the one the 180 default exists to protect. It only mis-orders inside tier 1, never drops a candidate and never beats the .ico tier, so it is a follow-up rather than a hold. startsWith("apple-touch-icon") on the token closes it; exact-token matching is right for mask-icon, where a longer rel really would be a different thing, and that is presumably where the idiom came from.

Every check is SUCCESS at this head, CLI smoke (required) included, with the skipped lanes all path-filtered. Approving on my side; I am not merging or queueing, and this repo requires an approval on the last push, so a further push needs a fresh one.

Review by Rames