Read a comment in a tenant theme as a comment, not as something the theme defines - #473
Merged
Merged
Conversation
…heme defines Every rule `validateThemeCss` applies is about what a theme may define, and a CSS comment defines nothing. They were applied to the raw file anyway, so a comment above the blocks read as a second selector and one inside a block read as a variable name. A tenant package is loaded at start-up, so the deployment did not come up, over a comment, saying nothing about comments. Comments are removed before the text is read as definitions. That is stricter than leaving them in: a comment wedged into the middle of `url(` now reaches the rule that refuses it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
kevin9327
requested review from
MikeRyanDev,
davidmckayv,
guidovizoso and
tylerslaton
as code owners
September 9, 2026 23:01
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
# Conflicts: # CHANGELOG.md
davidmckayv
approved these changes
Sep 10, 2026
davidmckayv
left a comment
Contributor
There was a problem hiding this comment.
Deep-reviewed against live code (correctness, governance, no vendor/secret/scale issues). Composed build+tests green. CHANGELOG/format rebase on CI-validated substance.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Put the line a hand-written stylesheet opens with at the top of a tenant package's
theme.css:and the deployment does not start.
loadTenantPackageis a top-levelawaitinserver/src/index.ts, so the throw comes out before anything is listening. What it says is:(verbatim, from calling
validateThemeCssonorigin/main.)None of those sentences mentions a comment, so the thing to remove is not in the message. The middle
one quotes the comment back as the name of a variable.
Why
docs/configuration.mdstates the rule as: "Theme CSS may define only:rootand.darkblocks,approved theme variables, and no
@importorurl()." Every clause of that is about what a themedefines. A comment defines nothing — but
validateThemeCssapplies all three checks to the rawfile, so a comment is read as a definition:
css.replace(/(:root|\.dark)\s*\{[^{}]*\}/g, "")and landsin
remaining, which is the test for a second selector.body.split(";")hands it to the variable loop as a declaration, and it isrefused under whatever the comment happens to say.
The fix
One line, before the three checks: take the comments out, then read what is left as definitions.
Removing them first is stricter than leaving them, not weaker, which is the part worth checking:
urlmakes something a browser does not tokenise as a URL, andthe
url\s*\(test did not read it as one either. With the comment gone, both do, and it isrefused.
and is still refused.
browser would act on.
No doc change: the sentence in
docs/configuration.mdwas already true, and is still true.Measured
bun test server/tests/tenant-package.test.tsorigin/mainwith only the new tests applied: 30 pass, 19 fail.accepts a comment above the blocks,accepts a comment inside a block,accepts a comment between the blocks.describes for packagesynchronisation talk to Postgres, and there is none here. They fail identically on unmodified
main(29 pass, 16 fail before any of my tests were added).The fourth new test,
still refuses what a comment is wrapped around, is the guard againstover-correcting and passes both before and after: a
url()in a value is still refused, a commentfollowed by
body { }is still refused as a selector, and an unterminated comment is still refused.So do the two theme tests that were already there.
Also run, all green:
bun run --filter server typecheck— exit 0bunx biome checkon both changed files — cleanNote on the CHANGELOG
A deployment that would not start now starts, so there is an entry. It goes at the top of
## Unreleased, the one line every entry goes at, so it will conflict with any other PR open againstthat anchor. Happy to rebase whenever it suits you.