Review 5529
bhamodi · open, request changes drafted · reviewed at 8d870ee3bb2 · view on GitHub
Verdict: request changes — six of seven assembly paths are closed, and the changeset says all of them are
The review asks for the prose rules to route through the same check, because generateProseRules interpolates 11 theme token values straight into rule text and joinDeclarations never reaches them — so the guarantee the changeset ships is not true yet. It did not merge because a typed public token still escapes its declaration and paints the page, and because the build is red on the PR's own new test.
generateThemeRules assembles CSS by string concatenation, so a theme value carrying ; or } closes its declaration and whatever follows it is parsed as CSS in the injected <style>. The person this reaches is an app that builds its theme from stored input — the PR names brand colours and white-labelling — where a tenant-supplied string becomes a rule that paints the whole page instead of one property.
This is the honest "defect nobody has hit yet" exception. The reachable state is named and it is real; nothing in the repo demonstrates a consumer doing it, and the PR does not claim one. That is fine for a hardening fix — but it means the guarantee is the deliverable, and a guarantee that is 90% true is the thing to check hardest.
(2 decisions · ~40 runtime lines of 132)
- Property names must match a name grammar and values must not contain
;,{,},/*or a control character — the fix. - A violation is dropped with a
console.warnrather than thrown — the failure mode.
Before, the theme system built its stylesheet by gluing strings together, and a value containing the characters CSS uses to end one rule and start another could smuggle extra rules in. Now, before a name: value pair is glued in, the pair is checked: the name has to look like a property name, and the value must not contain the characters that end a declaration or a rule. Anything that fails is thrown away with a console warning rather than written out. The responsibility sits in one small helper the assembly sites call, so the stylesheet only ever contains declarations somebody meant to emit — except that only six of the assembly sites call it.
Both decisions trace to the stated problem and nothing is piggybacked. Decision 2 has no sentence in the body saying why silence beats a hard failure; it is a defensible default for a guard, so it is not a finding, but it is unrecorded and it is what makes the false-positive case below invisible.
The unit is right and the placement is wrong at the radius that matters. joinDeclarations is exactly the shape this wants — one pure function, no public surface, cheap to extend — but the file has more assembly sites than it is called from. Driven, all output real from the generator running in the review worktree: the token block, the component base rule, the component pseudo rule and all three of their on-media equivalents are guarded, with the value dropped and warned; the prose element rules are not, and the token value is emitted raw.
Counted: 6 sites route through the helper; 11 val() interpolations in generateProseRules (:545-591) do not. The mechanism is identical and the input is the same input — val at :384 returns the theme's own token string, and generateProseRules(val, parts) at :400 drops it straight into a template literal at :564. Run with a type-scale token carrying a payload, the prose block emits it verbatim: :where(p) closes on the value's } and body { … } becomes a rule of its own, while the same theme object has its token block correctly dropped and warned in the same call. The guard fires and the escape still ships, in a different block of the same stylesheet.
The token used is not contrived. It typechecks against the public API — an ordinary member of the token union — and it plus the ten sibling type-scale tokens are exactly what a white-label config sets. Two weaker instances of the same shape sit on the selector side, also driven: the theme name reaches the @scope prelude and a component key reaches the class selector unescaped. Both need control of the theme's shape rather than one value, so they are the same parent rather than separate asks, and they are why the ask is "put the check where the CSS is emitted" rather than "add a seventh call site".
The next place the same failure occurs is 164 lines below the fix, in the same file, reached by the same input type — not an edge left over, but the original defect, unfixed. Where an unevadable fix lives is at the point a rule is emitted: one emitRule(selector, entries) every generator path goes through, so a new rule site cannot be added without the check. The cheap version that makes the changeset true today uses the fallback val already has, returning var(<key>) when the value fails the predicate.
End users of any app on a theme in this repo: nothing. No shipped theme renders differently, measured byte for byte.
Builders — two of them, and they get opposite things.
- The builder the PR is for, piping tenant input into
defineTheme, gets six of seven assembly paths closed and a changeset telling them the job is done. They stop looking. That is the impact that decides this review: an incorrect safety claim is worse than no claim, because it retires the question. Today nobody believes the generator is safe; after this lands, the changelog says it is, and the prose path is still open. - The theme author who writes a value containing
;— adata:URI is the realistic one — loses that declaration. Their brand mark stops rendering and only a console warning says why.
What landing it newly exposes: nothing in this repo. But it moves the prose gap from "one of many holes in an unguarded generator" to "the one hole in a generator advertised as guarded", which is a worse place for it to sit.
No API change. Nothing added to the barrel, no prop, no export, no type, no default moved; the helper and its two regex constants are module-local, confirmed against the barrel.
No new theme targets, no target renamed, no target's element moved, no token added or removed. The diff writes no CSS of its own — it validates CSS the theme wrote — and the style grep hits only the new docblock's prose and the pre-existing generator, with nothing added as styling. The one theming-shaped consequence is under Breaking: a theme whose value fails the new predicate loses that declaration. Zero across every theme in this repo, and unmeasurable outside it.
joinDeclarations is a new internal helper beside a generator that had none. Is there a sibling already doing this job? A grep for escaping and sanitising across core and the CLI returns 10 files, and the only structural one guards filesystem paths rather than CSS — no existing mechanism reaches this, so a new helper is right and the shape is right. Nothing is frozen either way: it can be renamed or absorbed into an emitter tomorrow at zero cost.
@astryxdesign/core is published and not private, and that governs every axis below — "how many themes do this?" is a question about a population outside this repo, so each answer says which population it is answering for.
- API — no, for everyone. Nothing exported changed signature or meaning; the three new identifiers are module-local.
-
Visual — no for any theme in this repo; yes for a consumer theme whose value contains a
;. Both halves measured. In-repo:generateThemeCSS()for all seven shipped themes at the PR head, then the generator reverted toorigin/mainin the same worktree, same install, same session, and regenerated — 130,285 bytes both times, 0 declarations dropped, and the text diff identical. The PR's byte-identical claim holds for everything that ships here, and this is also the honest answer for a new enforcement: it catches zero violations on this repo's themes today, and the only hits producible were constructed. A rule that fires zero times is normally unfalsifiable; here it is the correct result, because it guards future input rather than migrating existing themes. Consumer half: adata:URI carries;in its mime prefix, so a theme using one loses the declaration, driven end to end in both builds with a frame pair. -
Theme — no in this repo. Every existing target, token and override resolves identically; the byte diff is the proof. Outside it, the
data:case is the exception. -
Behaviour — a theme with zero tokens is unchanged, a passing value is emitted byte-identically, and a failing value or property name reaches a new state: the declaration is dropped, a warning is logged, and the rule may now be empty —
.astryx-button.primary { }, driven, that exact string. Every other state is unreachable, since the diff adds no prop, no default and no early return in a component. The new state is the only behaviour change, and it is the intended one.
The far side of the bound is a legitimate value that happens to contain a listed character. Counted: 0 of the 7 shipped themes carry a value containing ;, and 0 of the 8 data:image hits in this repo are in a theme — all 8 are CLI block templates and unit tests. Not counted: themes in consumers' own apps, a population that exists and cannot be measured from here. Not accepted on anyone's behalf — it is raised in the ask, marked non-blocking, so the author decides.
Zero effects: no useEffect, no useLayoutEffect, no hook, no component — a pure module-scope function, and nothing renders. No listeners or observers added; no getComputedStyle, offsetWidth or getBoundingClientRect anywhere in the diff, so nothing reads layout and nothing can force a reflow; and the diff changes no hook's return value, so nothing re-memoizes.
The cost of the check itself, measured rather than asserted: two regexes per declaration, run once at theme-CSS generation rather than per render or per component — 100,000 checks in 24.2 ms, which is 0.484 ms per 2,000-declaration theme. The worst realistic case in the repo is well under that: all seven shipped themes together generate 130,285 bytes, so the figure overstates the real cost, and it is paid once, off the render path. The bundle gains three module-local identifiers, two regex literals and one loop — tens of bytes, no new dependency.
Two frames, captured in real Chromium and opened with the read tool, plus a text comparison of the generated CSS. The frame pair shows a themed data: URI background: the pink brand mark renders behind the button on main and is absent at this head, with the computed background image going from the data URI to none. The text comparison covers all seven shipped themes, main against head, in the same worktree, install and session: identical, 130,285 bytes each. The Playwright probe was banked in the review kit, reusable for any "does this theme value survive generation?" claim, since the generator's output is CSS text and there is no Storybook story to drive.
| difference | verdict | source |
|---|---|---|
the brand mark renders on main and is absent at this head |
unintentional | the body says "No serialization change for anything legitimate: gradients, calc(), light-dark(), color-mix(), quoted font stacks, and every shipped theme emit byte-identical CSS." A data: URI is legitimate and is not on that list. Nothing in the PR asks for this difference |
| every shipped theme's generated CSS | no difference | byte-identical, both builds |
No rationale is being supplied that the PR did not offer: the body claims legitimate values are unaffected, and this is a legitimate value that is affected. That gap is the finding. Whoever posts publishes the frames to the assets branch on the fork, never to facebook/astryx, and embeds them beside the third finding, because the comment makes a visual claim and that claim owes a frame.
Structural only — this diff renders nothing, adds no element, no role, no ARIA, no user-facing string. Both greps return 0.
Nothing in the rendered-surface set is reachable: no interactive element, no focus, no announcement, no painted state, and CI's a11y job skips on this PR, correctly, because it touches no component.
One line is reachable and was checked: the new console.warn string is not routed through the translator hook, and that is correct rather than a violation. The rule governs user-visible and AT-visible strings, and a developer console warning is neither — the repo has 7 non-test console.warn call sites in core, including one already in this same file at :358, none translated, and the dev-warning hook that does exist is a component hook that does not apply to a pure generator. No finding. Direction: no logical or physical properties written, nothing to mirror.
request changes. The goal is partly met: six of seven assembly paths are genuinely closed, driven — the token block drops the payload and warns — and the seventh is not, driven — the same payload is emitted raw into the prose rules in the same call. The uncovered member is the 11 interpolations in generateProseRules, and it cannot follow separately, because the changeset ships the guarantee now: a follow-up closes the code gap a week later while the changelog has said for a week that it is already closed.
1. generateProseRules interpolates 11 theme token values straight into
rule text; joinDeclarations never reaches them
→ an app that feeds stored input into a type-scale token still gets a
rule that escapes :where(p) and paints the page, while the changeset
tells them the generator no longer allows it — so they stop checking
· generateThemeRules.ts:400, :564 (val at :384)
2. a token name used in the new test is not a member of the token union
→ the build is red; `test` passes because vitest does not typecheck,
so the PR's own test-plan numbers cannot see it
· generateThemeRules.test.ts:749
3. [not blocking] a data: URI value contains ';' and is dropped
→ a theme author's brand mark stops rendering and only a console line
says why; 0 themes in this repo do it, consumer themes unknown
· generateThemeRules.ts:341
The code gap and the false guarantee are the same defect from two ends, and each carries its own weight, so neither depends on the other to reach the verdict. Three notes do compound and they resolve to one thing — failing silently on a value we do not like — describing a theme author whose declaration disappears with only a console line. That is survivable, it is in the ask in its own words, and it does not add up to a block: the harm is a lost background image, not a lost exit.
The worst outcome is the prose escape itself: a stylesheet that still extends beyond its declarations, from a value that arrived through the typed public API, on a PR whose changeset says that cannot happen. That cannot coexist with approve. No design call is involved — this is a defect against the PR's own stated contract, verified by running it.
Each block was confirmed a second way. The first was found by reading the diff's six call sites against the file's assembly sites, and confirmed by running the generator with a typed token; the test is not that the six converted sites still work, but that the unconverted path emits the payload, and it does. The second was found in CI's log and confirmed by reproducing locally: one type error, at that line. The diff was grepped for both fixes and touches neither, so the author has not already done this. The ask costs no extra round trip: CI is already red — the storybook build fails on the type error while the test job passes, because vitest does not typecheck — so the PR was being re-pushed either way.
Two things found and not spent. The selector-side instances ride the "at the emitter" ask rather than being listed separately. And the author has nine other open PRs, all security hardening, several theme-adjacent, none touching this file — so there is no set to review together, and three of the six commits that landed on main during this run are his own PRs landing cleanly, which is worth saying because the block here is about this diff and not about the contributor.
Not verified: how many published consumers set a theme value containing ;, which is why the third finding is raised rather than accepted; whether any real consumer assembles a theme from stored input at all, which the PR asserts and this repo cannot demonstrate — the blocking finding does not depend on it either way; and whether the storybook build fails for a second reason after the type error, since the typechecker stops there.
Thanks for this — the hard part is picking which of the generator's many concatenation sites to guard, and six of them are right.
The prose rules still interpolate token values straight in, though, so the changeset's guarantee isn't true yet:
defineTheme({name: 'brand', tokens: {'--text-body-size': '1rem; } body { background: url(x) '}})The token block drops that and warns;
:where(p)emits it and the rule closes early.--text-body-sizeis an ordinary typed token, so a white-label config feeding the type scale gets there. Elevenval()calls have the same shape.
--color-heroisn't aTokenNameeither — that's the red build.Not blocking: a
data:URI value carries a;, so a themedbackground-imagegets dropped too.Can you route
valthrough the check as well, or is the prose block different for a reason I'm missing? If you'd rather talk it through with someone, we're in Discord.
Inline: packages/core/src/theme/generateThemeRules.ts:564 — Token value goes in raw here. joinDeclarations doesn't reach the prose block. · :384 — val hands back the theme's raw token string. Could route through the same check — the var() fallback is already here. · generateThemeRules.test.ts:749 — --color-hero isn't a TokenName. This is what's failing the build.
One review, three gate passes.
-
Gate 1 — failed on two, both in the private record while the comment was already clean. The impact slot's body described builders shipping a live escape because the changelog told them it was closed, and its own verdict line called that a note; the outcome only survived because another slot happened to carry the weight. And the
data:URI finding was accepted on a class count taken inside the repo, while the same draft had established two paragraphs above that the package is published and its consumers are outside it — the count was measured against the wrong population. - Gate 2 — two more. The blocking and non-blocking findings were flattened into one "two smaller things" clause in the comment, so the author could not tell which was required. And the visual breaking axis was still answered against the in-repo population while the evidence slot admitted one case renders differently — the same scoping error the draft had just fixed one paragraph up, and the reason the frames now exist at all.
- Gate 3 — clean. All four prior fixes confirmed in substance, with both frames opened by the critic and confirmed to show what the slot claims. One practice was adopted mid-run rather than argued: every verdict line now carries its own harm sentence, because round 1's mislabelled slot would not have survived being written next to the sentence its own body contained.
This first-round draft was subsequently posted as changes requested. Its prose-path, invalid-token, data-URI, and rebase asks are the baseline for the current R1e pass below.
Two brief changes this run earned and did not write, because it was read-only: a harm sentence on every verdict line, and a check that reads the posted comment beside the numbered findings item for item, so a blocking finding cannot arrive in the comment looking optional.
#5529 fix(theme): a theme declaration always stays one declaration by bhamodi (bucket: contributor)
019c3fbe45ea597a92059244017b2b98cc490a43
LOOP VERSION: 1.5.0 AUDIT RUBRIC: 1.13
LANE: full WHY: this is a security/trust boundary with our own unresolved changes-requested review; fast lane is ineligible.
WHY 1: A stored theme value can end its intended CSS declaration and add another declaration or rule. WHY 2: A builder using stored brand or white-label input cannot trust the theme generator to contain that input to the property they selected. WHY 3: The theme system exists to turn theme data into bounded styles; if one value can reshape the stylesheet, an app user can receive styling the builder never authored.
USER-FACING PROBLEM: Preventative hardening — a user of an app whose theme comes from stored input can receive unintended page styling because one theme value extends beyond its declaration. PROBLEM SEVERITY: broken task — the generated stylesheet can change unrelated presentation rather than safely applying the selected theme value.
VERDICT: clear — the preventative failure and affected builder/user path are explicit.
The generator checks each property/value pair before writing it into CSS. Structural characters outside strings or URLs are rejected, while valid punctuation inside strings and URLs is preserved. The responsibility remains in the shared generator used by runtime injection and theme builds; the current string scanner does not understand escaped quotes, so it can erase the wrong span before checking.
SOLUTION (2 decisions · ~67 changed runtime lines of 97)
- Route declaration assembly and prose token interpolation through one safety predicate — closes the previously reported prose path.
- Ignore quoted strings and
url()bodies while searching for top-level structure — preserves the previously reported data-URI case, but the chosen regex does not model CSS escapes.
BURDEN: medium — no state, Effects, listeners, public surface, or new dependency; the maintenance burden is correctly parsing enough CSS string/URL syntax to enforce a security boundary in both output paths. BURDEN MATCH: proportionate — a shared parser check is justified by a broken containment guarantee, but it must cover CSS escapes before it can carry that guarantee.
VERDICT: BLOCKS — the second decision still permits a top-level declaration break after an escaped quote.
OWNER: the shared theme CSS generator
TIER 1: runtime <Theme> injection and astryx theme build share this generator
TIER 2: none
SEAMS: token declarations, component base/pseudo declarations, on-media declarations, prose token interpolation
BEHAVIOR UNIT: pure utility — the predicate and declaration joiner are deterministic and focused-testable without component DOM.
| seam | driven result |
|---|---|
| component declaration generation | escaped-quote payload emits a second background declaration |
| runtime browser consumption | exact-head Storybook Chromium applies the emitted second declaration |
| prose interpolation | author regression now rejects the previously reported --text-body-size payload |
| data URI | author regression now preserves a semicolon inside url()
|
Placement is correct: the shared generator owns the boundary, and both runtime/build consumers inherit it. The defect is correctness inside that unit, not a new owner or extension mechanism.
VERDICT: clear — correct owner and test boundary; the blocking parser defect is recorded in SOLUTION/THEMING.
A builder can supply a CSS value containing an escaped quote followed by a semicolon; the scanner accepts it and the generated stylesheet contains the following declaration. In the exact-head Button story, the payload's second background declaration is applied with no page or Storybook error, so an app user receives styling outside the property value the builder intended to accept.
VERDICT: BLOCKS — the original containment failure remains reachable through CSS escape syntax.
No API change. No export, type, prop, default, token, or theme target is added; the new helpers remain module-local.
OSSIFICATION: none — internal implementation can be changed without consumer migration.
VERDICT: clear.
No theme target or token changes. The author's focused tests preserve ordinary gradients, calc(), light-dark(), quoted font stacks, and a data URI, while rejecting the previously reported raw top-level separators. An escaped quote still hides a top-level semicolon from OPAQUE_SPANS, so a theme value can emit another declaration.
VERDICT: BLOCKS — the theme-value boundary still does not match CSS string escape semantics.
BEHAVIOR: the PR improves the prior prose and data-URI cases, but the core unsafe behavior is unchanged for escaped quotes; main and the exact head emit/apply the same second declaration. API: no — no signature, type, export, or default changes. VISUAL: no intended change for legitimate themes; the adversarial before/head frames are byte-identical because both accept the same second declaration. THEME: no shipped target/token changes; the security guarantee remains incomplete rather than introducing a migration.
VERDICT: BLOCKS — the titled behavior is still false for a valid CSS escape sequence.
EFFECTS: zero. RENDER: no React work; validation runs while generating theme CSS. LISTENERS/OBSERVERS: none. LAYOUT: no DOM reads/writes in the changed generator. BUNDLE: no dependency; one module-local predicate and regex scanner.
VERDICT: clear — no added render/resource lifecycle; no performance finding.
VISUAL CHECK: manual frames required WHY: the visible acceptance case is an adversarial theme value not represented by the stable visual job's named stories; exact-head local Storybook is required to show whether the emitted declaration reaches Chromium.
| Warm main | Exact PR head |
|---|---|
 |
 |
Both 900×400 frames are byte-identical (3,823 bytes) and show the same red primary Button after the banked payload is injected. That expected red state was derived before capture from the payload's literal background: rgb(255, 0, 0) and the focused generator output—not copied from the observed pixels.
SENSOR RECEIPT: main d80c7889649b30cb8cecf59f3f65e929491b7c07; head 019c3fbe45ea597a92059244017b2b98cc490a43; story core-button--primary; globals astryxTheme=neutral;colorMode=light;direction=ltr; rendered theme neutral; mode light; direction ltr; viewport 900×400@1; forced colors/reduced motion/coarse pointer false, hover true; semantic state Primary Button, background rgb(255, 0, 0); one visible target at 123.03125×32; fonts loaded; zero page/Storybook errors. Receipts match on every sensor except Build. main.diff and head.diff bank the exact injected style arm; no source files changed.
VERDICT: BLOCKS — the exact head renders the same unintended second declaration as main.
Changed paths add no DOM, role, ARIA, focus, keyboard, direction, locale, or user/AT string. The console warning is developer-facing. CI's component a11y/RTL jobs were skipped because no component surface changed; the Chromium evidence retained the existing native Button and reported no page errors.
VERDICT: clear — no reachable a11y or i18n change.
| slot | verdict |
|---|---|
| PROBLEM | clear |
| SOLUTION | BLOCKS — escaped quotes bypass the structural scan |
| ARCHITECTURE | clear |
| IMPACT | BLOCKS — a second declaration remains reachable |
| API | clear |
| THEMING | BLOCKS — scanner does not match CSS escape semantics |
| BREAKING | BLOCKS — titled containment behavior remains false |
| PERFORMANCE | clear |
| VISUAL | BLOCKS — exact head matches vulnerable main |
| A11Y & I18N | clear |
GOAL: partly met — the prose path, valid TokenNames, data URI, and main rebase from our prior review are addressed; the generated declaration boundary is still bypassable with an escaped quote. DISPOSITION: escaped-quote bypass → blocks now; it is the same unsatisfied boundary from our prior review, not a new R1e nit. ADVICE: bounded direction — account for CSS escape semantics and add the banked regression; no parser implementation is prescribed. AUTHOR CAN PROCEED: yes — a value with escaped quotes must not hide top-level declaration/rule separators, while valid separators inside closed strings and URLs remain accepted. WORST OUTCOME: “an app user receives styling outside the property value the builder intended to accept” → request changes.
JUDGEMENT NEEDED: none — this is a proved correctness defect against the PR's stated contract.
request changes
-
OPAQUE_SPANStreats an escaped quote as the end of a CSS string → a builder's stored theme value passes validation and emits a second declaration that exact-head Chromium applies ·generateThemeRules.ts:348
Thanks — our prior review asked for top-level declaration breaks to stay rejected while valid semicolons inside strings and URLs remain valid.
OPAQUE_SPANS still treats an escaped quote as the end of a quoted string:
backgroundColor: '"x\\"safe"; background: red; "y"'That value passes the check and the generated CSS contains the second background declaration. The original boundary is still bypassable despite the new prose and data-URI coverage.
Could the scanner account for CSS escapes before this lands? If you'd rather talk it through with someone, we're in Discord.
[Reviewed by Robohands]
-
packages/core/src/theme/generateThemeRules.ts:348—OPAQUE_SPANSstops at an escaped quote, so the following semicolon passes as quoted.
- The author's focused file passes 60/60; the banked escaped-quote regression fails on the intended generated-CSS assertion.
- Remote test, build, lint, theme-layer, Storybook, sandbox, and stable visual jobs ran green on this exact head; they do not include this adversarial value.
- Current main has not semantically invalidated the touched generator since the author's rebase; the PR remains mergeable.
TIME total 75m (phase wall clocks overlap where work ran concurrently) setup/rules 6m full brief + critic + harness + versions; dedicated worktree install/build/server 5m fast-install twice after sweep; APFS build-dist guard passed; no build; warm main reused: yes visual exploration 3m generated-CSS probe plus main/head Chromium comparison screenshot capture 5m one synchronized main/head pair with sensor receipts; 0 re-measures after capture browser/a11y 1m exact-head story state, errors, and unchanged surface check focused tests 2m author file 60/60; one banked failing regression code/history 6m prior review reconciliation, current-head delta, main invalidation and CI checks critique + wiki 4m draft, critic pass, immutable gates, public record and serialized push CI wait 0m checks were already settled; no waiting suspended 56m session pause between evidence setup and browser execution waste 5m two module-format probe retries, stale-main receipt retry, swept-worktree recreation
- The correct implementation choice for CSS tokenization; the review intentionally gives outcome-based acceptance criteria instead of speculative parser advice.
- The unrelated Vercel deployment failure; the focused and repository CI evidence needed for this finding had already run.
Not posted: this was a read-only re-review. The public review block above is the exact candidate returned to Cindy.