Conversation
Six custom properties were referenced by the Dashboard stylesheets but never defined anywhere in the repository. A bare `var(--x)` with no fallback makes the whole declaration invalid at computed-value time, so the browser dropped it and the element silently inherited the body face. Nothing logged and no test failed. Verified in Chromium against the built stylesheet: before this change `.delivery-acceptance-content code` computed to the body font; after, it computes to the Geist Mono stack. - `--font-sans` / `--font-mono` are now declared in `styles.css`. The body face resolves through `--font-sans`, and the stylesheets that referenced `--font-mono` now compute instead of dropping. - `--pw-font-mono` was a misspelled reference; it now reads `--font-mono`. - `--pw-danger`, `--pw-border`, `--pw-canvas`, and `--pw-surface` had no definition, so the Goal delete hover colour, the refresh error colour, and the operator-credential panel border, background, and input border were all being discarded. They now read the existing `--pw-red`, `--pw-line-strong`, `--pw-bg`, and `--pw-card` tokens that the three theme blocks already define, rather than adding a fourth set of names. - The three `var(--font-mono, monospace)` fallbacks in the Goal LoopX mode stylesheets named a bare `monospace` keyword, so those blocks rendered a different face from every other code surface whenever the token was missing. They now name the same family as the token. `var(--pw-surface, #fff)` in `.personal-manager-team-result` is left alone: it supplies a fallback and is a legitimate optional token. Signed-off-by: song <22676124+songoow@users.noreply.github.com> Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Add `scripts/check-css-custom-properties.mjs` plus a focused contract test, so the class of defect fixed in the previous commit cannot return silently. The check fails on any bare `var(--token)` that no stylesheet — or inline style — defines. `var(--token, fallback)` is allowed, because a fallback is an explicit statement that the token is optional and the declaration still computes. This distinction matters: `--pw-surface` is legitimately optional in one place while its bare use elsewhere was a real bug. Two guards keep the check from passing vacuously, since a non-zero exit is its only signal. It fails when the scope yields no stylesheets, and it fails when the definition scan finds fewer than 30 tokens — a floor asserted from the tree rather than from the scan, so a broken definition regex cannot report a clean result. Both were validated by mutation: reintroducing an undefined reference, deleting a real token, and breaking the definition scan each produce exit 1. The check runs in the existing `dashboard-acceptance` job, after dependency install and before the coverage run, and is available locally as `npm run check:css-custom-properties`. `font-token.test.mjs` pins the specific decisions — what the two font tokens are defined as, that the regressed call sites resolve through them, and that the four dead private names stay gone. It joins `smoke:personal-workspace`. Scope is the Dashboard surface only. A scope is added when a real surface needs it; the marketing site defines its own tokens and `--terminal-delay` there is set from `App.tsx`, which the definition scan reads. Signed-off-by: song <22676124+songoow@users.noreply.github.com> Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
The typography section listed fallback stacks that no surface used, and did not say where the tokens live or what happens when one is missing. State the rule the fix depends on — reference the token, never repeat the stack, and define the token on the surface that uses it — and give the real declarations for both surfaces, since each bundles its own font files and the two differ. Note the check that enforces it and the `var(--token, fallback)` exception. Signed-off-by: song <22676124+songoow@users.noreply.github.com> Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
cocolord
left a comment
There was a problem hiding this comment.
动机
这个 PR 修复了真实且可观察的 Dashboard 回归:bare var(--token) 在可见级联中没有定义时,浏览器会丢弃整条声明,代码文本、凭据状态、边框、背景和错误色因而静默回退。exact head 9b05fee62a76526738afc122c46476e5931e8c94 把旧 base 上检测到的 9 处未定义引用修到现有 token;具体 CSS 修复有明确 before/after 收益。
改动思路
styles.css 在 Dashboard 根层声明 --font-sans / --font-mono,personal workspace 把不存在的私有名称收敛到三套主题已有的 token;新脚本扫描 Dashboard CSS 的 bare var() 与 CSS/TSX 定义,并在 dashboard-acceptance 中阻断未定义引用。正向路径是扫描、求差集、空集放行;负向路径应是新增未定义 bare 引用后 exit 1。
具体改动
- workflow/package script 接入 required check,并把
font-token.test.mjs纳入 workspace smoke。 - 三个 CSS 文件恢复字体、错误色、边框和背景;design.md 记录 per-surface font token 规则。
- 新增 231 行 scanner 与 98 行 focused contract test。
关键代码讲解
collectDefinitions(scripts/check-css-custom-properties.mjs:105)构造唯一的“已定义 token”集合。collectBareReferences(:139)只收集无 fallback 的var(--token)。main(:151)执行 scope/反空检查、求差集并以 exit code 驱动 CI。:rootfont tokens(styles.css:20)为 Dashboard 两个入口提供可继承字体族。
对主干的风险
[P1] collectDefinitions 会把任意字符串对象键误判成 inline-style 定义。 第 127 行的 quoted-key 正则没有确认键位于 JSX style / CSSProperties 写入。用 exact-head 函数执行 { "--review-ghost": "not an inline style" } 与 .probe { color: var(--review-ghost); },结果把该 token 放进 definitions,undefinedReferences 为空,门禁错误通过。主题元数据、文档示例或 token catalog 都可能出现这种键;同名 CSS 拼写错误就会再次静默进入主干。最低修复是将 TS/TSX 识别限制为真实 style 写入(或 AST),保留 setProperty 分支,并提交“非样式对象键不能满足 CSS 引用”的负向 fixture。
exact head 的 scanner、font-token、workspace-theme、personal-workspace contract、TypeScript/Vite build 和 GitHub dashboard-acceptance 均通过;本机无可绑定 Chrome,未重复作者的 computed-style 截图。CI shard 3/4、聚合 pytest 与 merge-gate 虽红,但 immutable base 与 head 都复现为相同的 generated-twin / prompt-upgrade-hook 三个断言,且本 PR 不触及这些路径,属于单独的 merge-readiness hold。
我的整体评价
CSS 修复明确正向,复用现有主题 token 也合适;但这个 PR 把通用 scanner 设成 required CI,而它在核心负向路径上可被普通非样式对象键绕过,仓库又没有提交 scanner 自身的负向测试。“防止同类静默回归”是新增 231 行机制的主要收益,目前 whole-PR 证据不足以 APPROVE。修复分类并补 fixture 后,请重跑 checker、focused contracts、Dashboard build/acceptance。
English verdict: REQUEST_CHANGES for exact head 9b05fee62a76526738afc122c46476e5931e8c94. The CSS fix has clear value, but the required guard treats any quoted "--token": key in TS/TSX as an inline-style definition, so unrelated data can mask a genuinely undefined bare CSS variable. Narrow the classifier and add a committed negative fixture. Dashboard validation passes; unrelated Python shard failures reproduce unchanged on base and head.
| for (const match of text.matchAll(/setProperty\(\s*["'`](--[A-Za-z0-9_-]+)["'`]/g)) { | ||
| add(match[1], `setProperty in ${rel}`); | ||
| } | ||
| for (const match of text.matchAll(/["'`](--[A-Za-z0-9_-]+)["'`]\s*:/g)) { |
There was a problem hiding this comment.
[P1] Restrict this match to actual inline-style definitions. This regex scans every TS/TSX object, so unrelated data such as const metadata = { "--review-ghost": "not a style" } is added to definitions; a CSS .probe { color: var(--review-ghost); } is then removed from undefinedReferences and the required check exits successfully. That recreates the silent failure this gate is meant to prevent. Please use a style-aware/AST-bounded match (keeping setProperty separately) and commit a negative collision fixture plus a positive real-inline-style fixture.
Review found a P1 in the guard added by the previous commit: the inline-style
branch matched any quoted `"--token":` key in TS/TSX, so unrelated data could
satisfy a CSS reference and the required check would exit 0.
Reproduced on the exact head before fixing. With
export const themeMetadata = { "--review-ghost": "not an inline style" };
in a `.tsx` file and `.probe { color: var(--review-ghost) }` in CSS, the check
reported "No undefined references" and exited 0 — recreating the silent failure
the gate exists to prevent.
The classifier is now anchored to a style sink rather than to key syntax:
style={{ ... }} JSX style attribute
style: { ... } style object property
const s: CSSProperties = {...} annotated style constant
{ ... } as CSSProperties trailing cast (the Dashboard's own form)
A quoted key only counts inside one of those bodies. The trailing-cast case is
anchored on `as CSSProperties` rather than on `return {`, because matching a bare
`return {` would accept any function returning an object with such a key —
the same defect in a different shape. Object bodies are delimited by brace depth
with strings skipped, so a nested object cannot truncate the match early.
Why a bounded match and not an AST: this check runs under a bare `node` in the
dashboard-acceptance job, and `typescript` is not resolvable from the repository
root there (`MODULE_NOT_FOUND`). An AST would make the gate depend on a package
it does not currently need.
Committed fixtures, as the review asked, under
`scripts/fixtures/css-custom-properties/`: `sinks.tsx` + `positive.css` for the
five sink forms this must accept, and `negative/dataKeys.ts` + `negative.css` for
the look-alike key this must reject. The test drives them through the shipped
`collectDefinitions`, so it cannot drift from the implementation.
The test was mutation-checked in both directions: reinstating the over-broad
match fails on the data-key assertion, and dropping cast support fails on the
`--fixture-cast` assertion. The classifier contract runs before the scan step in
CI so a broken classifier cannot report a clean tree.
`--goal-hue` in `goal-activity-view.tsx` — the only inline custom property in the
Dashboard — stays recognised, and the dashboard scope still reports no undefined
references.
Signed-off-by: song <liusongstep@gmail.com>
Re-review request — exact head
|
| Form | Result |
|---|---|
style={{ "--x": v }} |
defined |
style: { "--x": v } |
defined |
const s: CSSProperties = { ... } |
defined |
{ ... } as CSSProperties |
defined |
el.style.setProperty("--x", v) |
defined |
selector { --x: v } |
defined (stylesheet) |
{ "--x": "data" } |
not defined |
The trailing-cast form is anchored on as CSSProperties, not on return {: matching a bare return { would accept any function returning an object with such a key, which is the same defect in a different shape. Object bodies are delimited by brace depth with string contents skipped, so a nested object cannot truncate a match early.
Why a bounded match and not an AST. This check runs under a bare node in dashboard-acceptance, where typescript is not resolvable from the repository root (MODULE_NOT_FOUND). An AST would make a required gate depend on a package it does not currently need. If you would rather pay that dependency, say so and I will move it.
Committed negative fixture, as you asked
scripts/fixtures/css-custom-properties/ now holds reviewable inputs rather than strings built in the test:
sinks.tsx+positive.css— the five sink forms that must be accepted;negative/dataKeys.ts+negative.css— the look-alike data key that must be rejected.
check-css-custom-properties.test.mjs drives them through the shipped collectDefinitions, so the test cannot drift from the implementation. Mutation-checked in both directions: reinstating the over-broad match makes the negative assertion fail, and dropping the sink match makes the positive one fail. The dashboard scope still reports the same 34 defined tokens and exits 0.
Verification
npm run check:css-custom-properties clean; classifier contract test passes; font-token, workspace-theme and personal-workspace contracts pass; Dashboard tsc/vite build passes. The unrelated Python shard failures reproduce unchanged on base and head.
English verdict request: the required guard is now bounded to real style sinks with a committed negative fixture proving the bypass is closed. Please re-review 51424c608.
cocolord
left a comment
There was a problem hiding this comment.
动机
这个 PR 的产品目标是必要且可观察的:Dashboard 中 9 处 bare CSS custom property 没有定义,浏览器会在 computed-value 阶段丢弃字体、错误色、边框和背景声明,却不给构建或运行时错误。exact head 51424c608f4c8f9c8342e8a2f29e4caf29a22b31 保留了正确的 token 替换,并试图用 required CI 阻止同类静默回归。直接 CSS 修复已经有明确 before/after 收益;本轮复审重点是新增的 377 行 classifier/fixture 修复是否真的封住上次 blocker。
改动思路
styles.css 在 Dashboard 根层声明 --font-sans / --font-mono,personal workspace 将不存在的私有 token 收敛到现有主题 token;Node checker 扫描 CSS 的 bare var(--x),并从 CSS、setProperty 与 TS/TSX style-shaped 对象收集 definitions,再在 dashboard-acceptance 中对差集设非零退出。新 head 用 STYLE_SINK_OPENERS、前向/后向括号扫描和 committed fixtures 取代旧的“所有 quoted key 都算定义”,方向正确,但 source classifier 仍没有形成 JavaScript 的词法边界。
具体改动
相对 exact base 3ec049e138917a8cce4f84197ba196d26445b2b0,整份差异为 13 文件、+744/-13:3 个 CSS 文件修复字体、错误色、边框与背景;styles.css 增加两套 canonical font tokens;一个 361 行 scanner、134 行 classifier contract、98 行 font contract 和 4 个 fixtures 承担防回归;package/workflow 把命令接入默认 smoke 与 required dashboard-acceptance;design.md 记录 per-surface token 规则。
关键代码讲解
stripComments只屏蔽/* ... */,TS/TSX 的//注释和普通字符串仍原样交给分类器。STYLE_SINK_OPENERS识别 JSX style、style: {}与CSSProperties对象;它按文本匹配,不能证明命中位于可执行代码或真实 DOM sink。objectLiteralBodyBefore为 trailing cast 反向配对括号,但没有像前向版本一样跳过字符串/注释,合法值中的 brace 会把 body 扩到相邻数据。main以 definitions 与 944 个 bare references 的差集决定 required check;一旦 false definition 入集,后面没有二次验证。- Dashboard 根 token 与 personal-workspace 的现有 token 替换本身合理,当前 9 个 stylesheet 扫描与 font contract 均通过。
对主干的风险
[P1,阻断] 已注释或字符串中的伪 style 仍能让 required CSS guard 错误放行。 exact-head 的完整公开命令中,我加入一条已注释掉的 style: { "--review-comment-ghost": "not emitted" } 和一个 bare var(--review-comment-ghost);脚本仍以 0 退出并报告 10 stylesheets, 35 defined tokens, 945 bare references ... No undefined references。路径是:stripComments 不移除行注释 → opener regex 命中 dead text → objectLiteralBody 取出 quoted key → main 从 undefined 集合删除真实引用。普通字符串、未绑定 DOM 的 style: {} 数据也会入集;trailing-cast 的反向 parser 还会被字符串内 } 扩大范围,我的独立 collector probe 一次读出了 4 个 ghost token。
这不是缺少格式覆盖,而是门禁唯一失败信号仍可在实际 source path 上变成 false negative,和原始静默回归同方向。请先用 JavaScript lexer/AST 或等价的 string/comment-aware tokenizer 建立一个分类 owner,只让可执行的 JSX/CSSProperties/setProperty 写入满足引用;后向括号处理也需共享同一词法规则。把 comment、string、untyped style: data 与 brace-bearing CSSProperties 做成 committed fixtures,并让负例通过完整 checker 命令断言 exit 1,而不只检查导出的 definitions set。
提交的 classifier contract、Dashboard 当前树扫描、font-token contract、diff hygiene 与 dashboard-acceptance 均通过;它们证明现有 token 修复有效,但没有覆盖上述 dead-source 旁路。远端 shard 1/2、聚合 pytest 与 merge-gate 的 3 个红项,我又用同一命令在 exact base/head 复现为相同的 generated-twin 与两个 prompt-upgrade 断言,且本 PR 不改这些 owner,因此属于独立 merge-readiness hold,不是本次 blocker 的因果依据。
语义与 CI 对齐
设计文档说 checker 会拒绝“没有 stylesheet 或 inline style 定义”的 bare 引用,workflow 又把它设为机器门禁;因此绿色退出是 obligation,不是 guidance。当前 defined 分类把 dead source 和未证明的 style-shaped data 纳入,公开语义宽于实现。最低修复后应同时跑 clean tree、上述全命令 negatives、真实 JSX/CSSProperties/setProperty positives、font/workspace contracts、Dashboard build/acceptance 与完整 required CI。
我的整体评价
REQUEST_CHANGES。CSS 修复的用户体验收益明确,上次 plain metadata-key blocker 也确实被当前 fixture 覆盖;但 whole-PR 的长期价值主要来自新增 required guard,而新的简单 end-to-end 负例仍让它静默放行。361 行 partial lexer 加 232 行测试/fixtures 的维护成本,只有在负向语义闭合后才算相称;当前 long-horizon 是 not_yet_proven,user experience 是 improved,change proportionality 仍是 not_yet_proven。建议保留小而正向的 CSS/token 改动,把 scanner 收敛到一个可验证的 lexical owner 后再按新 exact head 复审。
English verdict: REQUEST_CHANGES — exact head 51424c608f4c8f9c8342e8a2f29e4caf29a22b31. The prior plain-data-key case is fixed, but the required guard still treats commented or stringified style: { "--token": ... } text as a real definition; a full-command probe therefore exits 0 for a genuinely undefined bare CSS variable. Make classification lexical/AST-aware and run committed negative fixtures through the shipped command. Dashboard validation passes; the three red Python assertions reproduce unchanged on exact base/head and are unrelated.
Goal
Six CSS custom properties were referenced by the Dashboard stylesheets but never
defined anywhere in the repository. A bare
var(--x)with no fallback makes thewhole declaration invalid at computed-value time: the browser drops it and
the element silently inherits the body face. Nothing logs, no test fails, and the
component renders slightly wrong in production indefinitely.
This PR defines the missing tokens and adds a check so the class of defect cannot
return silently.
Observable result
Verified in Chromium against the built stylesheet, before and after:
.delivery-acceptance-content code.personal-collaboration small.personal-operator-credential-status.personal-operator-credentialborder + background1px solid/rgb(250 250 250).personal-operator-credential inputborder1px solid.personal-goal-delete:hovercolourrgb(217 0 0).personal-refresh-control.is-error smallrgb(217 0 0)What changed
styles.cssdeclares--font-sansand--font-mono; the body face resolvesthrough
--font-sans.--pw-font-monowas a misspelled reference and now reads--font-mono.personal-workspace.cssused four private names with no definition at all(
--pw-danger,--pw-border,--pw-canvas,--pw-surface). These now read theexisting
--pw-red,--pw-line-strong,--pw-bg, and--pw-cardtokens thatall three theme blocks already define. I did not add a fourth set of alias names —
four parallel token families is the condition this change is trying to reduce.
var(--pw-surface, #fff)elsewhere is left alone: it supplies a fallback and is alegitimate optional token.
goal-loopx-mode.csshad threevar(--font-mono, monospace)fallbacks naminga bare
monospacekeyword, so those blocks rendered a different face from everyother code surface whenever the token was missing. They now name the same family
as the token.
New check
scripts/check-css-custom-properties.mjsfails on any barevar(--token)that no stylesheet or inline style defines.var(--token, fallback)is allowed, because a fallback is an explicit statement that the token is
optional and the declaration still computes. Scope is the Dashboard surface.
New test
font-token.test.mjspins the specific decisions: what the two fonttokens are defined as, that the regressed call sites resolve through them, and
that the four dead private names stay gone.
docs/development/design.mdlisted fallback stacks no surface used. It nownames the real per-surface declarations, the rule they depend on, and the check.
Validation
check-css-custom-propertiespasses on every commit in this branch (the fixlands before the check, so no commit leaves the gate red).
reintroducing an undefined reference, deleting a real token, and breaking the
definition scan each produce exit 1. Two anti-vacuity guards (empty scope, and a
30-token floor asserted from the tree rather than from the scan) were each
triggered deliberately.
--terminal-delayis set fromApp.tsx:546and is correctly accepted, not reported.font-token,workspace-theme, andpersonal-workspace-contracttests, plustsc --noEmit, pass.smoke:personal-workspace-packagedbrowser smoke passes — all 25 scenarios.dashboard-acceptancejob, after dependencyinstall and before the coverage run; also available as
npm run check:css-custom-properties.Boundary
Control-plane and product surface change: this alters rendered Dashboard
behaviour, so it is proposed for review and left for the maintainer to merge.
Not in this PR, deliberately: the wider token work (moving
--color-*into a@themeblock, collapsing the 25 type sizes and 34 border radii, containerqueries) and the
brutal/paper/loopxtheme overlap. Those need owner decisionsI should not make unilaterally, and they are safer on top of a working token layer
than underneath it.