Skip to content

fix(skills): design-handoff review-finding fixes (CodeRabbit on ponderous-site#31 + lawnomator-site#14) - #86

Merged
evanharmon1 merged 6 commits into
mainfrom
fix/design-handoff-review-findings
Jul 13, 2026
Merged

fix(skills): design-handoff review-finding fixes (CodeRabbit on ponderous-site#31 + lawnomator-site#14)#86
evanharmon1 merged 6 commits into
mainfrom
fix/design-handoff-review-findings

Conversation

@evanharmon1

Copy link
Copy Markdown
Owner

Fixes the CodeRabbit review findings raised against the vendored copy of this skill in
ponderousdev/ponderous-site#31 (plus 3 from
ponderousdev/lawnomator-site#14). Downstream
copies are drift-checked and never edited, so every accepted finding is fixed here in the canonical
source (ai/skills/frontend/design-handoff/); consumers pick it up on the next skill sync.

Verdicts — ponderous-site#31

Finding (comment id) File Verdict Resolution
Tall pages truncated to top slice (3566278381) assets/brand-screenshots.spec.ts Accepted Pages over the capture cap are now captured as stacked -part1/-part2/… clips (fullPage + document-relative clip) covering through the bottom; in-cap pages keep the single fullPage file.
Overflow guard light-theme-only (3566278384) assets/brand-screenshots.spec.ts Accepted Guard now runs per route × theme via the existing setTheme helper.
Alpha dropped before contrast scoring (3566278387) assets/check-contrast.mjs Accepted Alpha preserved for oklch/hex(#rrggbbaa)/rgb[a]; translucent fg composited over the audited bg (gamma sRGB) before scoring; translucent bg reported unsupported → rendered check, never PASS on the opaque value. Verified against an alpha fixture (a 0.35-alpha muted-foreground that passed before now FAILs).
Inline-style bypasses: gradient literals, hsla(), color() (3566278389) assets/check-off-palette.sh Accepted Attribute pattern scans the whole style value (bounded at quote/semicolon), both patterns flag rgba/hsla/hwb/lab/lch/color(); background-image/outline-color/caret-color added. Regression-run: 10 bypass cases flagged, 6 var(--token) negatives clean. (No asset-test harness exists in this repo; cases documented in the commit.)
Bogus ratios over painted backgrounds (3566278394) assets/measure-rendered-contrast.mjs Accepted Samples over a background-image or painted ::before/::after anywhere in the ancestor chain now fail as UNSUPPORTED (fail-closed) with a manual-measurement instruction; overlay siblings are documented as undetectable/manual-only.
Critical: shell interpolation in ingest:design (3566278397) assets/Taskfile.design.yml Accepted BUNDLE/DEST now pass via Task env: (never spliced into shell source) to a new assets/ingest-design.sh, which picks tar vs unzip by validated extension. Tested: BUNDLE='x.tar.gz; touch PWNED' does not execute; paths with spaces work.
Document unsupported painted backgrounds (3566278401) references/accessibility-verification.md Accepted New "What it can't model" bullet: UNSUPPORTED samples and overlay siblings require manual/pixel measurement; no sample accepted without a measured number.
@font-face before @import "tailwindcss" kills the import (3566278404) references/assets-fonts-favicons.md Accepted Example reordered (imports first); the incorrect "@font-face blocks can go anywhere" claim rewritten — any rule before an @import invalidates it.
Email contract contradiction (3566278408) references/brand-page.md Accepted One workflow now: hand-built table-HTML (the zero-dependency pattern) is the documented email path; React Email only if the repo already carries the dependency.
PDFs overclaimed as print-ready (3566278410) references/brand-page.md Accepted (prose fix, not machinery) Recipe outputs labeled digital-proof PDFs (RGB, bleed size, no crop marks); Tier 3 Print states true print-ready output needs a dedicated production export + vendor validation.
Bootstrap truncates existing stylesheet (3566278412) references/greenfield-bootstrap.md Accepted Rule is now create-empty only if absent; existing (token-less) globals.css contents are preserved, with a recoverability note for untracked files.
Zip-slip on bundle extraction (3566278413) references/ingesting-the-bundle.md Accepted Archive treated as untrusted: ingest-design.sh rejects absolute paths, .. traversal, and link entries before extracting (tested against crafted tar/zip archives); manual grep equivalent documented.
Dev-server contradiction (3566278415) references/responsive-and-cross-browser.md Accepted The stray "point them at the dev server" line now says production build && preview, matching the config and the spec header.
sidebar-* claimed theme-constant by convention (3566278418) references/token-reconciliation.md Accepted Corrected: shadcn writes different sidebar values in :root vs .dark; constant chrome requires deleting/aligning the .dark overrides — or dedicated fixed-chrome tokens when a real themed sidebar coexists.
.zip handoff contract mismatch (3566278420) SKILL.md + references/ingesting-the-bundle.md Accepted Both docs now agree: identify the coding handoff by content shape (README + chats/ + project), never extension; the raw-assets "Download as .zip" export lacks that shape and is rejected. Extraction flow (ingest:design) accepts both .tar.gz and .zip after entry validation.

Verdicts — lawnomator-site#14

Finding (comment id) File Verdict Resolution
attribute regex misses rgba(/hsla( (3566249742) assets/check-off-palette.sh Accepted Folded into the shared rgba?\(/hsla?\( color-function alternation used by both patterns.
2>/dev/null masks a bad $root as "clean" (3566249745) assets/check-off-palette.sh Accepted Missing target dir now exits 2 with an error instead of reporting the gate clean.
addInitScript stacking across themes (3566249747) assets/measure-rendered-contrast.mjs Accepted Fresh page/context per theme pass (init scripts persist for a page's lifetime and run in undefined order); page closed after each pass.

Rejected: none — every finding reproduced against the canonical source.

Notes

  • New asset: assets/ingest-design.sh (env-passed args, extension-validated tar/unzip, entry
    validation). SKILL.md/greenfield-bootstrap.md copy-lists updated; Taskfile.design.yml
    ingest:design delegates to it, keeping cmds: trivial per repo convention.
  • Known residual (documented in the script header): check-off-palette.sh can false-positive on an
    SVG url(#id) whose id is 3+ hex chars; flagged lines are human-reviewed by design.
  • Downstream PR threads (ponderous-site#31 / lawnomator-site#14) intentionally not replied
    to/resolved here — a separate agent handles those, citing this PR.

Gates

  • task verify — green (lint incl. shellcheck/shfmt on the new/changed .sh, markdown/yaml/hygiene,
    skills validation, 63 tooling tests)
  • Functional tests: check-contrast alpha fixture; off-palette 10-positive/6-negative regression set;
    ingest-design against good tar/zip, ..-traversal tar, symlink tar, crafted evil zip, injection
    filename, and paths with spaces; ingest:design exercised end-to-end via go-task
  • node --check on both .mjs; tsc parse of the spec template (module-resolution errors only, expected — @playwright/test is a target-repo dependency)

🤖 Generated with Claude Code

evanharmon1 and others added 5 commits July 12, 2026 16:52
CodeRabbit review of the vendored copy (ponderousdev/ponderous-site#31)
flagged ingest:design as a critical shell-interpolation hole: BUNDLE/DEST
were spliced unquoted into shell source, so a value like
'x.tar.gz; rm -rf ~' executed and paths with spaces broke — and the tar/unzip
fallback chain extracted untrusted archives with no entry validation
(zip-slip).

- new assets/ingest-design.sh: takes BUNDLE/DEST from the environment (no
  interpolation), selects tar vs unzip by validated extension, and refuses
  archives containing absolute paths, .. traversal, or link entries before
  extracting
- Taskfile.design.yml ingest:design now passes BUNDLE/DEST via env: and
  delegates to the script (keeps cmds: trivial per repo convention)
- ingesting-the-bundle.md: treat the archive as untrusted — validate entries
  before extraction (manual grep equivalent documented); identify the coding
  handoff by content shape (README + chats/), not extension, since exports
  have shipped as both .tar.gz and .zip

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…orted grounds

Two fail-open holes in the dual contrast gate (CodeRabbit on
ponderous-site#31 / lawnomator-site#14):

- check-contrast.mjs dropped alpha when parsing oklch/rgb/hex, so a
  translucent foreground token could PASS AA on its opaque value. Alpha is
  now preserved end-to-end: translucent foregrounds are composited over the
  audited background (gamma sRGB, as browsers blend) before scoring, and a
  translucent background — impossible to composite statically — is reported
  as unsupported for the rendered check instead of passing.
- measure-rendered-contrast.mjs composited ancestor background-colors only,
  reporting fictional AA ratios for text over gradients, images, or painted
  pseudo-elements. Those samples are now detected and fail as UNSUPPORTED
  until measured manually; undetectable overlay siblings are documented as
  manual-only (accessibility-verification.md).
- measure-rendered-contrast.mjs also re-added its theme init script to a
  shared page each pass (init scripts persist and run in undefined order);
  each theme now gets a fresh page/context.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…n bad root

CodeRabbit (ponderous-site#31 / lawnomator-site#14) showed the attribute
pattern only matched a color literal immediately after the property name, so
style={{ background: "linear-gradient(#111, #222)" }} passed; rgba()/hsla()
and modern color functions also slipped through, and a typo'd target dir
reported a false 'clean' because grep's stderr is suppressed.

- scan the whole style value (bounded at quote/semicolon), catching literals
  buried in gradients; var(--token)-only values still pass
- flag rgba/hsla/hwb/lab/lch/color() in both patterns; add
  background-image/outline-color/caret-color properties
- exit 2 when the target dir doesn't exist instead of falling through to the
  success message

Regression-run against fixtures: 10 bypass cases flagged, 6 token-safe
negatives clean, missing dir fails.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…theme overflow guard

- pages taller than the browser capture cap were silently truncated to the
  top slice; they are now captured as stacked -part1/-part2/… clips covering
  through the document bottom (single fullPage file kept within the cap)
- the horizontal-overflow guard ran light-theme-only; it now asserts per
  route x theme via the existing setTheme helper, catching dark-only overflow
- align the spec header and responsive-and-cross-browser.md with the
  production build-and-preview rule (one stray line still said to point
  baseURL/webServer at the dev server)

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…dance

Doc-level CodeRabbit findings from ponderous-site#31:

- assets-fonts-favicons.md: the manual @font-face example placed the block
  above @import "tailwindcss", which per the CSS spec silently kills the
  import; example reordered and the '@font-face can go anywhere' claim
  corrected (any rule before an @import invalidates it)
- brand-page.md: Tier 3 Email no longer mandates React Email — the
  zero-dependency table-HTML pattern is the documented workflow (React Email
  only if the repo already carries it); Tier 3 Print and the collateral
  recipe now label CSS-generated PDFs as digital proofs, with true
  print-ready output (CMYK/300dpi/crop marks) requiring a production export
  and vendor validation
- greenfield-bootstrap.md: the shadcn-init safety rule created an EMPTY
  globals.css even when one existed, deleting app CSS; now create-only-if-
  absent, preserve existing contents; script-copy lists include
  ingest-design.sh
- token-reconciliation.md: shadcn sidebar-* defaults DIFFER between :root
  and .dark (they are not authored identical by convention) — constant
  chrome requires deleting/aligning the .dark overrides or minting dedicated
  fixed-chrome tokens
- SKILL.md: identify the handoff by content shape (README + chats/), not
  archive extension — the raw-assets .zip export is not the coding handoff;
  bundled-assets list updated for ingest-design.sh and the hardened gates

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@evanharmon1 evanharmon1 changed the title fix(skills): design-handoff review-finding fixes (CodeRabbit on ponderous-site#31) fix(skills): design-handoff review-finding fixes (CodeRabbit on ponderous-site#31 + lawnomator-site#14) Jul 12, 2026
@evanharmon1
evanharmon1 requested a review from Copilot July 13, 2026 00:53

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Updates the canonical ai/skills/frontend/design-handoff/ skill to address downstream CodeRabbit review findings in vendored copies, primarily tightening safety/accuracy of the bundled “design gate” assets and aligning the written guidance with current export/tool behavior.

Changes:

  • Hardened and clarified design-bundle ingestion (new ingest-design.sh, Taskfile wiring, and docs aligned to “content shape” vs extension).
  • Improved correctness of design QA assets (alpha-aware static contrast, fail-closed rendered contrast over painted grounds, broader off-palette detection).
  • Refined Playwright cross-browser sweep guidance (production preview server requirement, tall-page capture chunking, per-theme overflow checks).

Reviewed changes

Copilot reviewed 14 out of 14 changed files in this pull request and generated 3 comments.

Show a summary per file
File Description
ai/skills/frontend/design-handoff/SKILL.md Updates skill contract/docs for bundle identification and asset inventory.
ai/skills/frontend/design-handoff/references/token-reconciliation.md Corrects guidance on sidebar-* tokens and theme behavior.
ai/skills/frontend/design-handoff/references/responsive-and-cross-browser.md Aligns browser verification guidance to production preview server usage.
ai/skills/frontend/design-handoff/references/ingesting-the-bundle.md Documents format drift and emphasizes safe extraction / untrusted archive handling.
ai/skills/frontend/design-handoff/references/greenfield-bootstrap.md Updates bootstrap steps to preserve existing CSS and include new scripts.
ai/skills/frontend/design-handoff/references/brand-page.md Clarifies email/print deliverable expectations (zero-dep email, “digital proof” PDFs).
ai/skills/frontend/design-handoff/references/assets-fonts-favicons.md Fixes and clarifies @import ordering rules relative to @font-face.
ai/skills/frontend/design-handoff/references/accessibility-verification.md Documents rendered-contrast limitations and required manual measurement cases.
ai/skills/frontend/design-handoff/assets/Taskfile.design.yml Routes ingest:design through env-passed vars and the new extraction script.
ai/skills/frontend/design-handoff/assets/measure-rendered-contrast.mjs Uses per-theme fresh pages and fail-closed UNSUPPORTED detection for painted grounds.
ai/skills/frontend/design-handoff/assets/ingest-design.sh Adds safe extraction with entry validation and extension-based extractor selection.
ai/skills/frontend/design-handoff/assets/check-off-palette.sh Broadens detection for modern color functions and improves failure behavior.
ai/skills/frontend/design-handoff/assets/check-contrast.mjs Preserves/composites alpha for foregrounds and fail-closed behavior for translucent backgrounds.
ai/skills/frontend/design-handoff/assets/brand-screenshots.spec.ts Improves screenshot coverage across themes and tall-page chunk capture.

Comment thread ai/skills/frontend/design-handoff/assets/brand-screenshots.spec.ts
Comment thread ai/skills/frontend/design-handoff/assets/brand-screenshots.spec.ts Outdated
Comment thread ai/skills/frontend/design-handoff/assets/ingest-design.sh Outdated
…t() API

- ingest-design.sh: the tar link guard grepped ' -> ', which only matches
  symlinks; GNU/bsd tar render hardlinks as 'name link to target' (no arrow),
  so they slipped through. Match the mode-string type column (^[hl]) to reject
  both symlinks and hardlinks portably.
- brand-screenshots.spec.ts: import expect from @playwright/test and call
  expect(...) instead of test.expect(...), which is not the documented API and
  throws at runtime (expect was never imported).

Both from Copilot review on PR #86.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@evanharmon1
evanharmon1 merged commit 61534e2 into main Jul 13, 2026
22 of 23 checks passed
@evanharmon1
evanharmon1 deleted the fix/design-handoff-review-findings branch July 13, 2026 01:11
This was referenced Jul 13, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants