feat(producer): plan-time validator — reject system fonts#774
Conversation
a1f7e50 to
b2306ea
Compare
8d068be to
b13acb6
Compare
miguel-heygen
left a comment
There was a problem hiding this comment.
Good refactor — extracting iterateFontFamilyDeclarations and parseFontFamilyValue from the existing extractRequestedFontFamilies keeps the regex surfaces in sync between the @font-face injector and the plan-time validator. The "primary family only" check is the right design: font-family: "Inter", -apple-system, sans-serif is the canonical fallback chain and should pass.
The deliberate copy of the banned-families set (noted in the PR description as separate concerns that overlap today) is a reasonable call — keeping the two decoupled avoids a false coupling that would slow down future divergence.
Test coverage is thorough: CSS property, data-attribute, case-insensitivity, generic-as-fallback, and the clean-html happy path.
LGTM.
— Magi
vanceingalls
left a comment
There was a problem hiding this comment.
Strengths:
- Extraction of
GENERIC_FAMILIES+iterateFontFamilyDeclarations+parseFontFamilyValuefromdeterministicFonts.ts(deterministicFonts.ts:18-78) gives the validator and the @font-face injector a single source of truth for what counts as a generic family + how to scan declarations. RefactorsextractRequestedFontFamilies(deterministicFonts.ts:225-235) onto the shared iterator — that's the right shape for Rule 2 contract consistency. PR description claims the validator "inlines" the banned set (deliberate copy) — actual code is better than the description: it imports from the single source. Worth updating the description. - "First entry wins" semantics (
planValidation.ts:104) correctly captures CSS font-family fallback semantics — the primary slot is what the browser uses; subsequent entries are fallbacks. Pinned by thefont-family: "Inter", -apple-system, sans-serifPASS test and the invertedfont-family: -apple-system, BlinkMacSystemFont, "Segoe UI"FAIL test (planValidation.test.ts:130,:165). - Case-insensitive matching (
planValidation.ts:105.toLowerCase()) pinned bySYSTEM-UI→ flagged test (planValidation.test.ts:184). - Tests cover ALL three surfaces (
font-familyin<style>,font-familyinlinestyle=,data-font-family=attribute) — the iterator yields all three, so the validator catches all three.
Findings:
important: deterministicFonts.ts:65-67 iterateFontFamilyDeclarations regex set:
[/font-family\s*:\s*([^;}{]+)[;}]?/gi, "font-family"],
[/data-font-family=["']([^"']+)["']/gi, "data-font-family"],
- The
font-familyregex matches in both<style>blocks AND inlinestyle="font-family: …"attributes (the regex is HTML-context-agnostic). Good. - But the
data-font-familyregex requires["']quoting. HTML5 allows unquoted attribute values (<h1 data-font-family=Outfit>). Compositions emitted by our own studio always quote, but external HTML pasted into a composition might not. The validator would then silently pass adata-font-family=system-ui— false negative. Not blocking if the studio's compiled HTML is the only input, but worth a comment naming that assumption + an explicit test asserting unquoted attribute values are either rejected or matched.
nit: deterministicFonts.ts:74 for (const match of html.matchAll(regex)) — regex g flag is consumed by matchAll. The two-element sources array runs sequentially so no state leak, but if the array grows, ordering may matter (e.g. an iframe.srcdoc regex that overlaps with font-family). Worth a comment noting the iterator emits in source-array order.
nit: planValidation.ts:111 error message says Distributed chunk workers render in a Linux container and cannot produce byte-identical output for fonts that resolve to host system installations. — but the validator only inspects the primary family. The error message language might confuse a user whose CSS is font-family: -apple-system, "Inter" — they'll wonder why "Inter" isn't being used. Recommend the error message explicitly note "primary (first) family in the declaration is …; subsequent entries are fallbacks and unused unless the primary fails to load."
Verdict: APPROVE
Reasoning: Validator is correctly shaped, shared GENERIC_FAMILIES source is the right call, three-surface coverage solid. The unquoted-attribute false-negative is the strongest concern and is a small follow-up.
— Vai
Part of Phase 2 of the distributed rendering plan (determinism hardening).
See DISTRIBUTED-RENDERING-PLAN.md §5.3 (Banned in distributed mode) and
§9.3 (typed non-retryable failures).
Extends packages/producer/src/services/render/planValidation.ts with:
- validateNoSystemFonts(compiledHtml) — scans `font-family:` declarations
and `data-font-family=…` attributes. If the PRIMARY family (first
entry in the comma-separated list) resolves to a host-OS / CSS-generic
family, throws PlanValidationError with code SYSTEM_FONT_USED.
- parseFontFamilyValue(value) — pure helper that splits a font-family
declaration value, stripping whitespace + quotes.
Banned primary families: sans-serif, serif, monospace, cursive, fantasy,
system-ui, ui-sans-serif, ui-serif, ui-monospace, emoji, math, fangsong,
-apple-system, BlinkMacSystemFont. Mirrors the GENERIC_FAMILIES list in
deterministicFonts.ts (deliberately a separate copy — they're two
different concerns that happen to overlap today).
Generic families remain acceptable as CSS fallbacks; only the primary
slot is rejected. `font-family: "Inter", -apple-system, sans-serif` is
fine; `font-family: -apple-system, BlinkMacSystemFont` is rejected.
No caller invokes the validator yet. Phase 3's `plan()` will run it on
the compiled HTML before freezing the plan, so chunk workers (Linux
containers without macOS / Windows system fonts) never see compositions
that would render differently between the controller and the workers.
In-process behavior is unchanged.
14 unit tests added to packages/producer/src/services/render/
planValidation.test.ts cover: clean compositions, missing font-family,
each banned primary family, data-font-family= surface, case-insensitive
matching, fallback acceptance, and parser edge cases.
This is part of a stack of 10 PRs; this is PR 9 of 10.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
b13acb6 to
a9f574d
Compare
b2306ea to
146ff0f
Compare
vanceingalls
left a comment
There was a problem hiding this comment.
Re-review of the new HEAD (a9f574d9) after prior approval at b13acb60 was dismissed. PR diff vs. base is the three-file shape: refactor deterministicFonts.ts to extract GENERIC_FAMILIES + parseFontFamilyValue + iterateFontFamilyDeclarations (shared with the validator), then add validateNoSystemFonts in planValidation.ts plus 122 lines of test.
Strengths
deterministicFonts.ts:48—iterateFontFamilyDeclarationsis the right shared-surface abstraction. BothextractRequestedFontFamilies(existing injector) andvalidateNoSystemFonts(new) consume the same iterator, so the validator can't drift to a different surface coverage than the injector. The "treats data-font-family= as a valid surface" test (planValidation.test.ts:209) pins that both surfaces participate.planValidation.ts:104— validates ONLY the FIRST family in each declaration. The "accepts generic families when used only as fallbacks" test (planValidation.test.ts:234) pins that"Inter", -apple-system, sans-serifpasses — exactly the canonical fallback chain that would otherwise false-positive.- Case-insensitivity test at
:222(SYSTEM-UIvssystem-ui) — pins the lowercase-before-lookup contract.
Findings
nit — deterministicFonts.ts:51 — iterateFontFamilyDeclarations regex font-family\s*:\s*([^;}{]+)[;}]? will also match font-family: declarations inside @font-face blocks. If a composition ever names an @font-face with primary system-ui (@font-face { font-family: "system-ui"; src: url(...) }), the validator would erroneously reject it. Pathological/theoretical — no real composition does this — but worth a code comment noting @font-face declarations are scanned too. Marking nit.
Notes (not blocking)
- All required CI green.
— Vai
Verdict: APPROVE
Reasoning: Validator shares the surface iterator with the injector (no drift), and the fallback-chain + case-insensitivity branches are pinned by tests. CI green.
The base branch was changed.

What
Extends
packages/producer/src/services/render/planValidation.tswith:validateNoSystemFonts(compiledHtml)— scansfont-family:declarations anddata-font-family="…"attributes; throwsPlanValidationError({ code: "SYSTEM_FONT_USED" })when the PRIMARY family (first entry in the comma-separated list) resolves to a host-OS / CSS-generic family.parseFontFamilyValue(value)— pure helper that splits a font-family value, strips quotes + whitespace.Banned primary families:
sans-serif,serif,monospace,cursive,fantasy,system-ui,ui-sans-serif,ui-serif,ui-monospace,emoji,math,fangsong,-apple-system,BlinkMacSystemFont.Why
Part of Phase 2, §5.3 (Banned in distributed mode) and §9.3 (typed non-retryable failures).
Distributed chunk workers run in a Linux container without macOS / Windows system fonts. A composition that declares
-apple-systemorsystem-uias its primary family renders differently between the controller and the worker — distributed retries aren't byte-identical and the workflow is broken. Generic families remain acceptable as CSS fallbacks; only the primary slot is rejected.How
deterministicFonts.extractRequestedFontFamilies(inlinestyles,<style>blocks, anddata-font-family=attributes) so the two surfaces stay in sync.deterministicFonts.ts— the two are different concerns that happen to overlap today; if they diverge in the future, that's a design signal).SYSTEM-UI→ flagged).Test plan
planValidation.test.tsgains 14 cases forvalidateNoSystemFonts:"Inter", -apple-system, sans-serif(primary isInter) → passes.font-familydeclarations → passes.-apple-system,system-ui,sans-serif,ui-monospace(viadata-font-family) → throws.SYSTEM-UI).parseFontFamilyValueedge handling.plan()will run it on the compiled HTML before freezing the plan.bun run --cwd packages/producer typecheckclean.bunx oxlint+bunx oxfmt --checkclean.This is part of a stack of 10 PRs; this is PR 9 of 10. Stacked on top of #773.
🤖 Generated with Claude Code