CORE-2736: Add the CSS colour audit engine - #149
Conversation
Pure CSS parsing that answers one question: given stylesheet text, which
colour literals are in it and where. Knows nothing about ui-components, the
palette, or the theme — those live in the layer above, added in CORE-2720.
stripNoise removes comments, strings and url() payloads; a brace/semicolon
scanner pulls {property, value} pairs at any nesting depth, so @media is
covered and selectors and @Keyframes percentages are not mistaken for values;
each value is walked for colour literals in any syntax — hex, the functional
notations, and bare named colours against the full CSS named-colour table.
Two rules worth knowing. Functions that merely contain colours (var(),
color-mix(), the gradients) are descended into rather than treated as
literals. And a bare identifier is only read as a colour where one can go —
a custom property, anything with "color" in the name, the border family, or a
colour-bearing shorthand — so animation-name: red is not a finding, while
hex and the colour functions stay in scope everywhere.
Anything unresolvable to channels (hsl(), oklch(), color()) is reported
rather than passing silently, so the escape hatch is explicit.
The 28 cases in cssColors.spec.ts are the contract: what it must flag and
what it must leave alone, including the cases both this repo's and REX's
reviews turned up. Reviewable without any theme context — nothing here
depends on the token file it was built to enforce.
Also points the module field at dist/esm/index.js. "module": "index.js"
named a file that does not exist at the package root.
Split out of #143 so it can be reviewed on its own merits.
Co-authored-by: Roy Johnson <roy.e.johnson@rice.edu>
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
- url() ends at a structural `)`, not one inside a quoted or escaped payload.
`url("icon).svg")` closed early, and the stray quote left behind blanked every
declaration after it, so `color: red` dropped out of the audit entirely.
- custom property names keep their case. CSS matches them case-sensitively, so
lower-casing merged `--Brand` and `--brand` into one entry.
- whitespace in a selector is collapsed only outside strings and escapes, so
`[data-label="a b"]` and `[data-label="a b"]` stay distinct contexts — which
is the whole reason the context keeps string contents.
- channels and alpha validate the CSS `<number>` grammar, shared between them.
`[\d.]+` matched `.` and `1..2`, which parseFloat turned into NaN and a
truncated 1 and which were then handed back as resolved channels.
- a colour-bearing container (the gradients, color-mix, light-dark, cross-fade)
opens the named-colour gate for its arguments, so
`list-style-image: linear-gradient(red, blue)` is found. `var()` is excluded
and keeps the enclosing property's gate.
Vendor-prefixed functions were not read as calls at all, since the call pattern
required a leading letter; `-webkit-linear-gradient(...)` had its stops walked
as loose identifiers.
Ten new cases cover the five defects, plus the blanked-copy length invariant for
the new url() paths.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Escapes were not handled at all, which two of these findings are the same
root cause of, in two scanners that both have to know about them:
- `blankNoise` read the quote in `.foo\"bar` as a string opener, so the rest
of the stylesheet was blanked and every declaration after it vanished. An
escape is now taken before any delimiter test, and both characters are
copied through rather than blanked: an escape in a value is part of an
identifier, and `\red` really is the colour `red`.
- `declarations` reads the blanked copy, where escapes survive, so it needed
the rule again. `.foo\{bar` opened a block that never closed and corrupted
the context stack; `.foo\;bar` truncated the selector to `bar`.
The other two are properties the classifier had no business claiming, both
because a bare identifier there is a name the author chose:
- `list-style` is dropped from the shorthands. It has no <color> component —
its identifier is a <counter-style>, and after `@counter-style red { ... }`
the declaration `list-style: red` means that counter. Gradients written
there are still found, since a gradient opens the gate for its own stops.
- `color-scheme` is excluded from the `includes('color')` heuristic, its value
being an author-defined <custom-ident>. The rest of the colour-named family
that holds no colour is excluded alongside it; those take fixed keywords, so
they change no result today, but the substring should not claim them.
Fifteen new cases. The blanked-copy length invariant still holds under the new
escape path — checked over 30k random metacharacter strings.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
- `url` is only noise as a whole function name. `--x: myurl(#fff)` had its
payload blanked, so the colour was never audited; an unknown container is
value text to descend into.
- a `{}` block inside a custom property is a component value, not a nested
rule. `--x: { red }` was pushing the block as selector context and losing
the value; brace depth is now tracked once such a declaration has started,
and real nesting still reads as nesting.
- the rgb() syntaxes are parsed separately instead of by normalising the
separators away. `rgb(0 0 0 0.5)` and `rgb(0 0 0 // 0.5)` were resolving to
channels, and mixed channel units were accepted.
- CSS escapes are decoded before the named-colour lookup. `r\65 d`, `re\64`,
`\red` and `\72 ed` are all `red` to CSS, so the audit was bypassable; the
`literal` stays the source span, which is what a consumer has to rewrite.
Checked against Chromium rather than read off the grammar, which corrected one
of these: mixed channel units are invalid in *both* rgb() syntaxes, not only
the legacy comma one, since each production takes three channels of one type.
The escape cases are pinned the same way — `\72ed` is U+72ED, not `red`,
because hex digits are consumed greedily up to six, and Chromium rejects it.
Several findings in review turned on what CSS actually means rather than on what this code does, and I got one of them wrong in the fix before last by reading the grammar instead of checking: `rgb(255 50% 0)` looks legal, since the modern syntax is usually described as three channels of number-or- percentage, but rgb() splits into an all-number and an all-percentage production and browsers reject the mixture. `\72ed` looks like `red` and is U+72ED, because hex escapes take up to six digits. So the expectations now come from an engine. scripts/verify-css-colors.mjs puts 59 values through Chromium and prints its verdict on each; the table is checked in and the suite asserts agreement, with no browser needed at test time (there is none in CI, and @playwright/test was an unused devDependency). The assertion is one-directional where it has to be, since a verdict is only that build's — this one has no oklch(). The audit may always decline to resolve a value, because hsl() and the rest are reported as unresolvable by design. It may never resolve one to different channels than the browser, and never resolve one the browser throws away. Alpha is compared at 8-bit precision, which is all a browser keeps: Chromium serialises #ff000080 as alpha 0.5, not as 128/255. That is deliberately a narrower comparison than colorKey makes, which holds alpha decimal so two allowlist entries cannot collide.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
|
@bethshook I have split what was one PR into three for hopefully more coherent reviewing. In the process, Copilot found some more things to fix, which we did some of. Because edge cases are still likely to emerge, and the code here isn't run-critical, I've had Claude create a new ticket to perfect the verifier so these three can get merged, which is a roadblock for everything else. |
Establishes one place a theme value is written and referenced from CSS, with
a test that fails if the two disagree. The sweep of the already-migrated
stylesheets onto it follows separately.
src/theme/theme.css holds a single :root block. Colour tokens are the
kebab-case palette key (palette.neutralLighter -> --ox-color-neutral-lighter)
plus --ox-color-link, --ox-color-link-hover, --ox-z-index-* and
--ox-padding-navbar-*. The --ox- prefix avoids collisions with a consuming
app's own variables.
The file is generated, not hand-written. themeCss.ts owns the projection and
npm run generate:theme-css writes it; build.bash runs it as its first step,
before either tsc pass and before the rsync, and publish.bash inherits that
via build:clean, so a published package cannot ship a stale file. It is
committed as well as generated because jest and ladle read src/ directly and
CI runs lint/test rather than build. Deliberately not hooked into pretest —
regenerating before the suite would make the freshness check pass vacuously.
Adds the four button variant colours to the palette, which theme/buttons.ts
had been holding as bare string literals with nothing recording that they
are hover/active variants of orange and darkGray. Purely additive.
Enforcement, in tokens.spec.ts, on top of the CORE-2736 engine:
1. The committed theme.css is exactly what the generator produces. One
equality, so a missing token, an orphan token and a stale value all
fail the same way.
2. No component stylesheet writes a colour literal that duplicates a
theme value.
3. No component stylesheet introduces a colour that is neither a theme
value nor on the KNOWN_OFF_PALETTE allowlist, each entry with a reason.
4. No component stylesheet reads an --ox-* token that does not exist,
which would otherwise fall through to its fallback silently.
Check 2 cannot pass yet — 16 stylesheets migrated before the tokens existed
still carry hand-copied literals. PENDING_SWEEP names them, and is asserted
to be exactly the failing set so it cannot rot in either direction: dropping
a name without sweeping the file fails, and sweeping a file without dropping
its name fails too. The list reaches empty in the sweep PR and goes away
with the assertion.
No CSS @import: build.bash rsyncs CSS 1:1 with no bundler, so an @import
would depend on each consumer's resolver. Component .tsx files import
theme.css alongside their own stylesheet instead. Consumers need do nothing.
Split out of #143. Stacked on #149.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Two review points. The README claimed the check "covers every syntax a color can be written in" and reads bare names "wherever they appear". Neither is true: a bare name is read only where the property could take a color, which is deliberate and is what keeps `animation-name: red` and `font-family: white` quiet, and escaped spellings of property and function names are missed altogether (CORE-2885). Replaced with what is actually covered, what is excluded on purpose, and what is a known gap. Each claim checked against the checker rather than read off the source. Spelling swept to color throughout the files this PR owns, per review. The engine in #149 still reads colour; that PR is being merged as-is, so the sweep for cssColors.ts and its spec belongs in #143 rather than reopening it. theme.css is regenerated, not hand-edited — the wording lives in themeCss.ts. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
🟡 Changes recommended
The parser misses image() fallback colours, and its exported raw-value helper can report colours inside strings and URLs.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 4/4 changed files
- Comments generated: 2
- Review effort level: Balanced
| * way. It has no default: defaulting it to `true` would quietly restore the over-eager | ||
| * behaviour for any caller that forgot it. | ||
| */ | ||
| export const findColors = (value: string, named: boolean): FoundColor[] => { |
There was a problem hiding this comment.
Agreed, and the framing is the useful part — the doc comment and the code disagreed about the precondition, and only stylesheetColors was on the right side of it by accident of getting blanked values from declarations. Both of your reproductions are exact: findColors('url(#fff)', true) returned #fff and findColors('"red"', true) returned red.
Fixed in aed00c6, taking your first option. The walk is now an internal scanColors whose precondition is explicit — already-blanked value — and which is also how it recurses into its own arguments, where re-blanking would be wasted work. The exported findColors is scanColors(stripNoise(value), named), so the documented contract is the real one. stylesheetColors calls scanColors directly and so does not pay for it twice.
I preferred sanitizing over making it internal because "what colours are in this declaration value" is a reasonable thing for a consumer to want on its own, and the engine is published precisely so consumers do not hand-roll it. Making it internal would have left the next caller writing the stripNoise themselves — which is the shape of the duplication this PR exists to remove.
Worth noting the fix is free of the aliasing risk it looks like it might have. stripNoise blanks to spaces at the same length rather than deleting, so a literal found in the blanked copy has the same characters at the same indices as the source, and literal still comes back as written. A literal cannot straddle a blanked region either: #ff"aa" blanks to #ff plus spaces, which fails the {3,8} hex length, and re"d" blanks to re — both of which are what CSS tokenizes them as anyway.
Covered in both directions: five rows asserting a raw url fragment, a quoted string, a string in a shorthand, a comment and a data: URI all yield nothing, and three asserting that blanking the noise does not blank the colours with it (#fff, "x" red, url(a.svg) red). All five of the first group fail with the stripNoise call reverted.
This comment was marked as resolved.
This comment was marked as resolved.
RoyEJohnson
left a comment
RoyEJohnson
left a comment
There was a problem hiding this comment.
Address Copilot's comments.
… boundary Two findings from Copilot's latest pass. image() takes a bare <color> after its image, so `image(url(marker.svg), red)` is a colour written in the stylesheet wherever the outer property sits. It was not in COLOR_CONTAINERS, so the property gate closed over it and the stop was missed under any property that is not itself colour-valued. image-set() is deliberately still absent: it holds images and resolutions, never a colour. findColors documented its input as a declaration value but required one whose noise had already been blanked, which only stylesheetColors knew because it gets that from declarations. A direct caller got `url(#fff)` reported as white and `"red"` reported as red. The walk is now the internal scanColors, which keeps the already-blanked precondition and recurses without re-blanking, and the exported findColors blanks first so its contract is the documented one. Neither is adjudicable by scripts/verify-css-colors.mjs: the noise boundary is an API contract rather than a question about CSS, and Chromium 105 implements no image() at all -- it rejects even `image(red)` -- so that claim rests on CSS Images 4. The checked-in table still reproduces exactly against a fresh run. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Both of Copilot's comments are fixed in aed00c6 — replies on each thread with the test that pins it.
The second is the one with a consumer consequence. Ten new cases, 230 in On the browser checkLast round I said anything further Copilot turned up should go through The I did use the browser for what it can answer. Chromium confirms Still open, unchangedThe five findings on CORE-2885 are untouched by this. Neither of today's fixes is in that lexing class — One thing I owe you an explanation for: yesterday's comment recorded four findings from the REX copy's review without pushing anything, on the reasoning that the PR was approved and I did not want to invalidate that for non-urgent items. Three of those four are CORE-2885 findings and stay there. That left you with a comment and no commit, which is not a useful state to hand back — sorry for the round trip. @RoyEJohnson ready for another look. |
Jira: CORE-2736 (parent: CORE-2720)
1 of 3 — first of the split of #143, which Roy asked to break up for review.
main→ #149 → #150 → #143.What this is
Pure CSS parsing that answers one question: given stylesheet text, which colour literals are in it and where. It knows nothing about ui-components, the palette or the theme — that layer lands in #150.
stripNoiseremoves comments, strings andurl()payloads.{property, value}pairs at any nesting depth, so@mediais covered while selectors and@keyframespercentages are not mistaken for values.Two rules worth a reviewer's attention:
var(),color-mix(), the gradients) are descended into rather than treated as literals, so building a value out of tokens stays clean.color-named property, or a colour-bearing shorthand. Soanimation-name: redis not a finding, while hex and the colour functions stay in scope everywhere (pinned by a case assertinganimation-name: #d5d5d5is still flagged). Properties whose identifier is a name the author chose are excluded even when they look colour-adjacent:list-style: redcan be a@counter-styleandcolor-scheme: redcan be a scheme.Anything unresolvable to channels (
hsl(),oklch(),color()) is reported rather than passing silently, so the escape hatch is explicit.Why it's worth reviewing separately
It was 53% of #143's insertions and it is the part with the least to do with this ticket — you can review it without knowing anything about theme tokens. The cases in
cssColors.spec.tsare the contract: what it must flag and what it must leave alone.Also fixes
"module": "index.js", which named a file that does not exist at the package root.Verification
mainwith nothing else applied — green, so it genuinely stands alone.scripts/verify-css-colors.mjsputs 59 colour values through Chromium and prints its verdict on each. The table is checked in and the suite asserts agreement, so no browser is needed at test time. Run it by hand to extend or re-verify:aed00c6c— all 59 rows identical.declarationsdepends on is fuzzed over 30k random metacharacter strings while working on that scanner; it is a manual check rather than a committed test.Follow-up
Roy's call on this review was to land the engine as-is and track further verifier work separately. That is CORE-2885, which carries the five findings still open against the parser and the tokenizer decision. All five are lexing — the scanner recognises only the literal ASCII spelling of a token, so escaped identifiers (
c\6f lor,linear-gr\61 dient,u\72 l() and comment-separated compound selectors (.a/**/.b) are misread. Two are false positives. None is triggered by any stylesheet we ship, which is why they do not block this.Provenance
Folds in Roy's
8e5386d6from #143 (treat unresolvablevar()arguments inrgb()/rgba()as errors rather than skipping the check), credited via trailer. It cannot be a separate commit here because the file it edited did not exist yet at that point in the original history.🤖 Generated with Claude Code