M4: House style on-brand by construction (design tokens + skill consumes house style)

[adnaan]

M4 — House style on-brand by construction

Generated UIs are now on-brand by construction (design tokens apply automatically) and by guidance (the generation skill authors against the house style) — delivered by tokens + guidance, no enforcement lint (the re-scoped milestone shape: tinkerdown-only, spine-only, no lint).

Phase 1 — Declarative design tokens (styling.tokens)

A project sets its on-brand page-shell palette once:

styling:
  tokens:
    accent: "#5a67d8"
    card_bg: "#ffffff"

• StylingConfig.Tokens + KnownStyleTokens (14 page-shell tokens) + ValidateStyleTokens() wired into Load — an unknown key fails loudly at config load (names the key, lists the known set); absent → built-in defaults (backward-compatible).
• buildStylingOverrideCSS emits a sorted, CSS-injection-sanitized :root { --token: value } block, after the primary_color block so an explicit tokens.accent wins by source order.
• Scope (advisor-confirmed): page-shell palette IN; the generated client-chrome CSS (search/tabs fallbacks + four undefined tokens) is OUT — internal chrome, never author-facing, and hand-editing a generated/minified artifact is the make build regenerates the committed client bundle from stale node_modules, breaking checkbox e2e tests #295 provenance trap.

Phase 2 — The skill consumes the house style

Closes a declared-but-unconsumed gap: generation.style_guide was a config field nothing read.

• SKILL.md reads styling + generation.style_guide and carries the on-brand lever — write semantic HTML, never hardcode colours, follow the style-guide.
• reference.md gains a § House style (rule + 14-token table); its § Tailwind CSS example, which demonstrated hardcoded colours, is fixed to layout/spacing only.
• Forward drift-guard (KnownStyleTokens ⊆ reference.md, so a new token can't go untaught) + structural + integration tests.
• The PII reference app ships a style-guide.md + styling.tokens, demonstrating a governed, on-brand console.

Verification

• Full root suite green (go test ., all 54 files incl. every !ci E2E) with ./tinkerdown pre-built.
• Four-channel E2E (TestStylingTokensComputedStyle) proves a token override reaches computed style through the real cascade; unconfigured site keeps the default.
• /simplify on both phases; two advisor catches fixed in-commit (Phase 1's four-channel E2E gap; Phase 2's self-contradicting Tailwind counter-example).

Notes

• Token overrides are single-value across both themes (like primary_color); per-theme palettes are a future affordance.
• House style is advisory (guidance + tokens), never enforced — the no-lint shape.
• Plan (docs/plans/2026-07-09-ephemeral-ui-reframe.md) updated per-phase with Learn; Phase 1's stale pre-kickoff block rewritten to shipped scope.

🤖 Generated with Claude Code

https://claude.ai/code/session_018M9pJSPmG6i1D8s6rpEV4h

[claude]

Review

Nicely scoped and well-documented change — the tokens + skill-consumption split is clean, and the test coverage (unit, config-load, structural drift-guard, and a real four-channel E2E through getComputedStyle) is thorough. A few notes:

Code quality / correctness

• Verified CSS cascade ordering is correct. stylingOverrideCSS (built in internal/server/styling.go) is injected at server.go:2754, right after the closing </style> of the built-in :root/[data-theme="dark"] block (server.go:1196-1234). So the new styling.tokens :root block wins over both light and dark defaults by source order, exactly as claimed in the comments and PR description — confirmed by reading the actual fmt.Sprintf call site rather than just the doc comments.
• Minor UX inconsistency (not a bug, but worth flagging): an unknown token key fails loudly at config load (ValidateStyleTokens), but an invalid token value (e.g. a typo like accent: "red;" that trips sanitizeCSSValue's meta-character check) is silently dropped at render with no diagnostic anywhere. Given the PR's stated philosophy ("a typo can't silently skin nothing"), a bad value still can silently skin nothing — it just won't be misinterpreted as a different token. This mirrors pre-existing primary_color behavior though, so it's not a regression, just an inherited rough edge. Might be worth a load-time sanity check (or at least a debug log) in a follow-up.
• KnownStyleTokens is an exported mutable map[string]string var rather than an unexported map with an accessor — fine within internal/config, but worth keeping in mind if this package's surface ever gets re-exported.

Security

• sanitizeCSSValue correctly rejects <>{};" and newlines, which covers the CSS-context breakout the new test (TestBuildStylingOverrideCSS_TokensSanitized) exercises. Since styling.tokens values come from the trusted tinkerdown.yaml (site-owner authored, not end-user input), this is defense-in-depth rather than a hard boundary — appropriately scoped.

Test coverage

• Good breadth: round-trip + unknown-key-rejection + backward-compat at config load, unit tests for CSS emission + sanitization, a forward drift-guard (KnownStyleTokens ⊆ reference.md) so a future token can't go undocumented, and a real browser E2E proving the override reaches computed style (not just emitted markup) with a control case (unconfigured site keeps the default).
• One gap per above: no test asserts behavior when a token value (not key) is invalid — e.g. that Load still succeeds and the token is simply omitted from output. Would be a cheap addition to lock in the current (arguably fine) behavior.

Scope / process

• Good adherence to the "no enforcement lint" milestone shape — this stays advisory (tokens + guidance), which matches tinkerdown's semantic-HTML-first, classless-CSS model instead of fighting it.
• No lvt-* attributes are touched by this PR, so the CLAUDE.md check against livetemplate/client doesn't apply here.

Overall: solid, low-risk, well-tested addition. No blocking issues.

[adnaan]

Thanks for the thorough review. Addressed the test-coverage note in e647f14:

• Invalid token value behavior — added TestLoad_StylingTokensUnsafeValueAccepted, locking that ValidateStyleTokens gates keys (fail-loud at load) but not values: an unsafe value round-trips into config and is dropped at render via sanitizeCSSValue, exactly as primary_color/font behave. This pins the key-vs-value split so it can't silently regress. (The render-side omission was already covered by TestBuildStylingOverrideCSS_TokensSanitized.)

On the other notes, all non-blocking and agreed:

• Load-time value validation — deliberately not added here. primary_color/font have the identical silent-drop behavior, so validating only token values would introduce a new inconsistency; a load-time sanity check across all style values is a separate, out-of-scope consistency pass, better as its own follow-up.
• KnownStyleTokens exported mutable map — fine within internal/config; noted for if the package surface is ever re-exported.
• Cascade ordering / sanitization scope — confirmed correct; the two :root blocks stay separate by design (source-order precedence is load-bearing), and styling.tokens values are site-owner-authored, so sanitization is defense-in-depth.

[claude]

Review: M4 — House style on-brand by construction

Reviewed the full diff (config.go, styling.go, tests, SKILL.md/reference.md, PII example). Overall this is well-scoped, well-tested, and the docs/plan/CHANGELOG stay in lockstep with the code — good discipline throughout.

Code quality / correctness

• Cascade ordering is correct and verified. I traced buildStylingOverrideCSS's output through server.go: the big :root/[data-theme="dark"] block closes its <style> tag at line 2753, and the stylingOverrideCSS string (its own <style>...</style>) is injected as a separate block right after, at line 2754 — not nested inside the first <style> tag. So "same specificity, later source order wins" (the comment in styling.go) holds up, and TestStylingTokensComputedStyle backs it with a real computed-style assertion through chromedp rather than just checking emitted text. Nice four-channel E2E.
• ValidateStyleTokens (config.go:727) does the right thing — fails loudly on an unknown key, defaults to empty/no-op otherwise. The defensive if _, ok := config.KnownStyleTokens[key]; !ok { continue } skip in buildStylingOverrideCSS (styling.go:47-50) is good belt-and-suspenders for the case where a StylingConfig is constructed without going through Load (e.g. directly via NewWithConfig in a test or embedder).
• sanitizeCSSValue reuse is consistent with the existing primary_color/font handling — token values aren't validated at load, only sanitized at render, and TestLoad_StylingTokensUnsafeValueAccepted deliberately pins that behavior rather than silently changing it later. That's a reasonable, explicitly-documented tradeoff (matches an existing pattern rather than introducing a new inconsistency), not a gap.

Minor / non-blocking observations

• ValidateStyleTokens iterates a Go map, so if multiple unknown keys are present in one config, which one gets named in the error is non-deterministic run-to-run. Low impact (the common case is a single typo) but worth a sort.Strings-then-first-mismatch if you ever want a fully deterministic error message.
• The scope cut (page-shell tokens only, client-chrome CSS excluded) is clearly justified in the commit message and plan doc (the four undefined --bg-hover/--badge-bg/etc. tokens live only in the generated/minified client CSS). Good call keeping that out of an author-facing config surface.

Test coverage

Strong across every layer:

• Unit: round-trip, fail-loud typo (name + known-set message), backward-compat default, map↔validator drift guard (config); emission/sanitization (server).
• Structural/doc drift-guard: TestStyleTokensDocumented forward-guards KnownStyleTokens ⊆ reference.md so a token can never go undocumented to the generation skill — nice mechanism, and the write-up for why it's forward-only-by-design (no unique textual signature to scrape, unlike lvt-) is convincing.
• Integration: TestPIIManifestHouseStyleConsumable proves the demo manifest's style_guide resolves to a real file and styling.tokens populates.
• E2E: TestStylingTokensComputedStyle proves the override reaches actual computed style via the real cascade, and that an unconfigured site is unaffected (no cross-site leakage of tokens).

Security

No new attack surface: token values flow through the same sanitizeCSSValue gate already used for primary_color/font, and the config file itself is operator-controlled (same trust boundary as the rest of tinkerdown.yaml). buildCustomCSSLink's separate escaping is untouched by this PR.

CLAUDE.md compliance

No lvt-* client attributes are introduced or touched by this PR (it's a server-side config/render + skill-docs feature), so the "check livetemplate/client before reimplementing an lvt-* attribute" instruction doesn't apply here.

Note on verification

I wasn't able to run go build/go test in this sandbox (shell commands beyond gh/git/read-only tools required interactive approval that wasn't available), so this review is based on careful static reading of the diff plus tracing the template/cascade logic by hand. Everything checked out; nothing here blocks merging as far as I can tell.

🤖 Generated with Claude Code

[adnaan]

Addressed the determinism note in the latest commit: ValidateStyleTokens now collects unknown keys, sorts, and reports the first — so the error message is stable run-to-run instead of depending on map iteration order. TestValidateStyleTokens_MultipleUnknownDeterministic locks it. Thanks again for the careful read.

[claude]

Review

I read through the full diff (config.go, styling.go, the new tests, SKILL.md/reference.md, the PII example, and the plan/CHANGELOG updates) plus the two prior review rounds already on this PR. Both previously-flagged issues are properly addressed:

• The silently-dropped-unsafe-value behavior is now pinned by TestLoad_StylingTokensUnsafeValueAccepted, with a clear rationale (validating only token values would introduce a new inconsistency vs. primary_color/font, which already round-trip-then-drop the same way).
• The non-deterministic "which unknown key gets named" issue is fixed in ValidateStyleTokens by collecting + sorting unknown keys before reporting the first, locked by TestValidateStyleTokens_MultipleUnknownDeterministic.

Went back through the rest of the diff looking for anything new the earlier rounds didn't cover:

Correctness

• Traced the cascade ordering myself: buildStylingOverrideCSS's output is injected as its own <style> block after the built-in :root/[data-theme="dark"] block in server.go, and the tokens block is emitted after the primary_color block within that same string — so tokens.accent correctly wins over both the built-in default and an explicit primary_color at equal specificity via source order. Matches the E2E assertion in TestStylingTokensComputedStyle.
• ValidateStyleTokens's c == nil guard matches the existing idiom used by ValidateActions/ValidateGeneration in this file — consistent, not an oddity.
• Token key matching is case-sensitive (accent vs Accent would be rejected as unknown) — reasonable given the whole config surface is snake_case and this fails loudly rather than silently, so not a real risk.

Security

• sanitizeCSSValue (shared with primary_color/font) blocks <>{};" and newlines, which covers breaking out of both the CSS declaration and the surrounding <style> tag. Values come from operator-authored tinkerdown.yaml, so this is defense-in-depth rather than a hard trust boundary — appropriately scoped, nothing new introduced here.

Test coverage

• Genuinely thorough across all layers: config round-trip/backward-compat/fail-loud-typo/map↔validator-drift, CSS emission + sanitization unit tests, a forward doc drift-guard (TestStyleTokensDocumented), an integration test against the real PII manifest, and a real four-channel E2E through getComputedStyle with a control case (unconfigured site keeps the default). I don't see a coverage gap left after the two follow-up commits.

CLAUDE.md compliance

• No lvt-* client attributes are touched by this PR, so the "check livetemplate/client before reimplementing an lvt-* attribute" instruction doesn't apply here.

Docs/scope

• CHANGELOG, plan doc, SKILL.md, and reference.md all stay in lockstep with the shipped code, and the milestone re-scope (tinkerdown-only, spine-only, no enforcement lint) is well-justified and consistently reflected everywhere it's mentioned.

No blocking issues. This is ready to merge as far as I can tell — nice job closing out the two review rounds with targeted, well-justified fixes rather than over-correcting.

🤖 Generated with Claude Code