150 lines
8.7 KiB
Markdown
150 lines
8.7 KiB
Markdown
---
|
||
phase: 01-design-tokens-rams-foundation
|
||
fixed_at: 2026-10-22T02:05:00Z
|
||
review_path: .planning/phases/01-design-tokens-rams-foundation/01-REVIEW.md
|
||
iteration: 1
|
||
findings_in_scope: 6
|
||
fixed: 6
|
||
skipped: 0
|
||
status: all_fixed
|
||
---
|
||
|
||
# Phase 1: Code Review Fix Report
|
||
|
||
**Fixed at:** 2026-10-22T02:05:00Z
|
||
**Source review:** `.planning/phases/01-design-tokens-rams-foundation/01-REVIEW.md`
|
||
**Iteration:** 1
|
||
|
||
**Summary:**
|
||
- Findings in scope: 6 (BLOCKER CR-01 + WR-01..WR-05; INFO findings out of scope per orchestrator)
|
||
- Fixed: 6
|
||
- Skipped: 0
|
||
|
||
**Note on provenance:** a prior fixer run applied all six fixes as atomic commits (61d15c2…43b7d99)
|
||
but terminated before writing this report. This pass *independently re-verified* each committed fix
|
||
(diffs audited against the review's prescribed mechanism, all four verification gates re-run
|
||
empirically, see Verification section) — no fixes were re-applied; master was already clean.
|
||
|
||
## Fixed Issues
|
||
|
||
### CR-01: Dead `--radius-DEFAULT` token — bare `rounded` compiles to `0.25rem` (BLOCKER)
|
||
|
||
**Commit:** `61d15c2`
|
||
**Files modified:** `theme/src/tokens.css`, `theme/scripts/audit.mjs`, `theme/scripts/audit.test.mjs`
|
||
**Applied fix:** Replaced the inert v3-style `--radius-DEFAULT: 0px` with the deprecated bare
|
||
`--radius: 0px` — the key Tailwind v4.3.3 actually drives the bare `rounded` utility from
|
||
(`node_modules/tailwindcss/theme.css` "Deprecated" block). Block comment corrected (bare
|
||
`rounded` is now var-driven; `rounded-none`/`rounded-full` are literal built-ins; the DEFAULT
|
||
sub-key must not reappear). Audit radius drift guard extended to `/--radius(?:-[A-Za-z0-9-]*)?…/`
|
||
so a future non-zero bare key trips exit 2. Regression tests T9 added: bare-key presence,
|
||
inert-DEFAULT absence, non-zero bare key → exit 2, and a controlled build asserting
|
||
`.rounded` compiles to `var(--radius)` with `--radius: 0px` emitted by `@theme static`.
|
||
|
||
### WR-01: Prose candidate leakage compiles Rams-forbidden `.shadow`/`.blur` into the shipped stylesheet
|
||
|
||
**Commit:** `689bc9f`
|
||
**Files modified:** `theme/src/theme.css`, `theme/src/tokens.css`, `theme/scripts/audit.test.mjs`
|
||
**Applied fix:** (1) `@source` narrowed from the whole mockup tree to the four mockup markup
|
||
files, plus `@source not "../scripts"` / `@source not "../basemap"` so toolchain prose can never
|
||
mint utility classes. (2) Defense in depth at the key: the deprecated bare `--shadow`,
|
||
`--shadow-inner`, `--drop-shadow`, `--blur` namespaces are set to `initial` in `@theme static`,
|
||
so bare shadow/blur utilities can never compile from any candidate source (scale keys remain —
|
||
mockup-worn `shadow-*` classes still compile and stay under adjudication 1). T5 regression
|
||
checks added: no `.shadow{` / `.blur{` in `dist/rams.css`, no `border-radius:.25rem` anywhere
|
||
in dist.
|
||
|
||
### WR-02: `familiesFromShorthand` logic bug — off-token families in `font:` shorthands pass unflagged
|
||
|
||
**Commit:** `5e95741`
|
||
**Files modified:** `theme/scripts/audit.mjs`, `theme/scripts/audit.test.mjs`
|
||
**Applied fix:** Two-layers, contract-true: (1) any literal (non-`var()`) `font:` shorthand is
|
||
flagged outright with the convention message (`font: var(--type-*)`), naming the extracted
|
||
families; `var()` shorthands pass. (2) `familiesFromShorthand` itself repaired — iteratively
|
||
strips the leading style/variant/weight keyword run (any CSS-grammar order, incl. oblique
|
||
angles), then the size[/line-height] component (incl. `calc()`), per comma group, so the family
|
||
is genuinely extracted for the message instead of being silently dropped by the old
|
||
single-strip + numeric-filter bug. T10 regression tests: literal shorthand → exit 1, Arial
|
||
extracted, sanctioned-family shorthand still flagged, `var()` shorthand passes.
|
||
|
||
### WR-03: Color-evasion gaps — named keywords, modern color functions, `text-shadow` all pass
|
||
|
||
**Commit:** `ee48e76`
|
||
**Files modified:** `theme/scripts/audit.mjs`, `theme/scripts/audit.test.mjs`
|
||
**Applied fix:** (1) Full CSS Color keyword list (`NAMED_COLORS`, ~148 entries) flagged on the
|
||
color-bearing property set (`COLORISH_PROPS` incl. shorthands, fill/stroke, borders,
|
||
text-decoration, accent-color); `currentcolor`/`transparent` and css-wide keywords stay
|
||
sanctioned. (2) Modern color functions — `oklch()/oklab()/lab()/lch()/hwb()/color()/color-mix()`
|
||
— flagged on any property unless the value is `var()`-bearing (Tailwind's own `color-mix`
|
||
emissions are var-driven and pass). (3) `text-shadow` added to `shadowViolations`: any
|
||
non-none value forbidden (Rams Shapes). T11 regression tests: `color: red`, `oklch(...)`,
|
||
`color(display-p3 ...)`, non-hex `text-shadow` all exit 1 and are listed; sanctioned keywords
|
||
pass. Audit header (§5) updated to document the extended surface.
|
||
|
||
### WR-04: D-13 contract shortfall — spacing and typography numerics not audited
|
||
|
||
**Commit:** `5e254cb`
|
||
**Files modified:** `theme/scripts/audit.mjs`, `theme/gallery.html`
|
||
**Applied fix:** Implemented the missing checks rather than amending the claim. (1) Spacing:
|
||
`padding`/`margin`/`gap` families (logical variants included) — literals must equal a
|
||
`--spacing-*` token value, be zero, or `auto`; dimensional geometry (width/height, offsets,
|
||
border widths, the 18×18 checkbox) is spec-per-components-header and out of scope
|
||
(adjudication 6). (2) Typography: `font-size`/`font-weight`/`line-height`/`letter-spacing`
|
||
validated against the `--type-*` scale, parsed from the compound token grammar (`weight
|
||
size/line-height family…` + `-tracking` companions); `normal`/`bold` keyword equivalents
|
||
sanctioned; `@font-face` blocks exempt from numeric scale as asset metadata (D-16) but still
|
||
color/family-checked (adjudication 7). `npm run audit` output now reports
|
||
"6 spacing values / 10 type sizes / 9 trackings". Gallery D-13 evidence wording updated to
|
||
"colors, spacing, typography, radius, shadows, blur". T12 regression tests: off-scale
|
||
numerics (`padding: 13px`, `font-size: 17px`, weight 650, tracking) → exit 1 and listed;
|
||
token-value literals (1rem, 0.25rem, 14px, 700, 20px, -0.04em), zero/auto, and font-face
|
||
metadata pass.
|
||
|
||
### WR-05: Static server path guard uses prefix match without separator
|
||
|
||
**Commit:** `43b7d99`
|
||
**Files modified:** `theme/scripts/screenshot.mjs`
|
||
**Applied fix:** `sep` imported from `node:path`; guard changed to
|
||
`if (target !== THEME && !target.startsWith(THEME + sep))` — the docroot prefix must end at a
|
||
path separator, so a decoded `..%2FthemeX` (which joins to a `themeX`-sibling path passing the
|
||
old bare `startsWith(THEME)`) now 403s.
|
||
|
||
## Skipped Issues
|
||
|
||
None — all six in-scope findings fixed and verified.
|
||
|
||
## Verification (all four gates, empirically re-run in this pass)
|
||
|
||
1. **Build radius gate** — `npm run build --workspace theme`; `grep -o 'border-radius:[^;}]*' theme/dist/rams.css | sort -u` →
|
||
`border-radius:0`, `border-radius:var(--radius)`, `border-radius:3.40282e38px`
|
||
(the sanctioned `rounded-full` ≥1e30px literal clamp). **No `.25rem` / non-token radius forms.**
|
||
`.rounded` compiles to `border-radius:var(--radius)` with `--radius:0px` emitted by `@theme static`. ✅
|
||
2. **Audit gate** — `npm run audit` (repo root) →
|
||
`AUDIT CLEAN — 354 declarations checked … — 0 off-token values. token sheet: 55 colors / 9 radius
|
||
tokens (all zero) / 7 sanctioned font families / 6 spacing values / 10 type sizes / 9 trackings`. ✅
|
||
3. **Test suite** — `node theme/scripts/audit.test.mjs` → **38 passed, 0 failed**
|
||
(was 16/16 pre-fix; now includes T9 CR-01, T10 WR-02, T11 WR-03, T12 WR-04, extended T5 WR-01). ✅
|
||
4. **Screenshot server guard (live probe)** — server started under the real capture run
|
||
(`node theme/scripts/screenshot.mjs`, listener located by PID), probed with `curl --path-as-is`:
|
||
- `/..%2FthemeX/secret` → **403 forbidden** (the exact WR-05 exploit shape: a `themeX` sibling
|
||
that the old bare prefix check passed)
|
||
- `/..%2FthemeX/gallery.html` → **403**
|
||
- `/..%2Fstitch_minimalist_route_navigator/…` → **403**
|
||
- `/src/sheet.html` → 200 (correct content — no over-blocking)
|
||
- `/gallery.html` → 200 — ✅
|
||
(Method note: a first probe port grab had hit Chromium's own devtools listener — PID-matched
|
||
listener selection fixed that; the guard result above is from the real capture server.)
|
||
Bonus: full capture run completed `CAPTURE OK — 6/6` non-blank captures.
|
||
|
||
Also verified byte-for-byte: deep diff audit of all six fix commits against the REVIEW.md
|
||
prescribed fix shapes (`git show 61d15c2 689bc9f 5e95741 ee48e76 5e254cb 43b7d99`) — mechanisms
|
||
are real (key-level token disarmament, regex/grammar repair, parser extension, guard change),
|
||
not cosmetic suppressions; per CR-01 guidance the elimination of the banned utility is the
|
||
intended goal, so post-fix compiled output genuinely lacks `.rounded`/`.shadow`/`.blur`
|
||
non-zero/off-token forms.
|
||
|
||
---
|
||
|
||
_Fixed: 2026-10-22T02:05:00Z_
|
||
_Fixer: OpenCode (gsd-code-fixer)_
|
||
_Iteration: 1_
|