Skip to content

Commit 24a69df

Browse files
Bill LeoutsakosBill Leoutsakos
authored andcommitted
feat(design): add design conformance checks
1 parent b2b9eed commit 24a69df

48 files changed

Lines changed: 15122 additions & 78 deletions

Some content is hidden

Large Commits have some content hidden by default. Use the searchbox below for content that may be hidden.

.agents/skills/babysit/SKILL.md

Lines changed: 16 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -57,6 +57,12 @@ All three must hold:
5757
reported yet, and treating "not failing" as "passing" reports the PR clean before CI has
5858
had its say. Wait for it — the step-10 stop condition covers a check that never settles.
5959

60+
A passing design-conformance CI step can still contain warnings. Read its latest report and
61+
triage findings using `/ship`'s [committed design check](../ship/SKILL.md#committed-design-check).
62+
Intentional system changes and justified exceptions may remain once explained in the PR;
63+
they do not prevent a clean review or require another fix loop. Honor decisions already made
64+
in this session. Operational failures must be resolved before reporting the PR clean.
65+
6066
Do not stop early on "no new comments this round" alone — a thread can be open from an earlier
6167
round, and cubic often lands its first threads a round after Greptile's. Always check all three
6268
conditions freshly after every push.
@@ -85,11 +91,14 @@ conditions freshly after every push.
8591
If `mergeable` is `CONFLICTING`, fix that first (step 2). If a check is failing, fix that too
8692
— treat it exactly like a review finding. If a check is still `pending`, do not evaluate
8793
"clean" at all: go to step 9 and wait for it. Otherwise, if Greptile is 5/5, every thread
88-
across all pages has `isResolved: true`, and every check has finished and passed, stop —
94+
across all pages has `isResolved: true`, every check has finished and passed, and any design
95+
warnings have been triaged as above, stop —
8996
report the outcome (see "Reporting" below) and skip the rest of this list.
9097

9198
2. **If the PR has a merge conflict**, merge `origin/staging`, resolve the conflicts, run the
92-
usual pre-push checks, push, and go to step 8 to re-trigger review.
99+
usual pre-commit checks and commit the resolution. Run `/ship`'s
100+
[committed design check](../ship/SKILL.md#committed-design-check) against the resulting HEAD
101+
before pushing, then go to step 8 to re-trigger review.
93102

94103
3. **If no review has run yet** (fresh PR, no bot comments): both run automatically on PR open —
95104
confirm via `gh pr checks <n>` (look for `Greptile Review` and `cubic · AI code reviewer`) and
@@ -131,7 +140,11 @@ conditions freshly after every push.
131140
gates from `/ship` steps 4 and 5. A review-fix round is still a code change and can trip
132141
either gate just as easily as the original commit did.
133142

134-
7. **Commit and push** the round's fixes as one commit — `--force-with-lease` whenever step 6's
143+
7. **Commit, check and push** the round's fixes as one commit. After committing and before
144+
every push, follow `/ship`'s [committed design check](../ship/SKILL.md#committed-design-check),
145+
including warning triage and committing/rechecking any resulting fixes. Push only the
146+
checked HEAD; rerun after a rebase or any other change to the comparison.
147+
Use `--force-with-lease` whenever step 6's
135148
sync check rewrote history, which includes a plain `git rebase origin/staging` that completed
136149
with no conflicts, not only the cherry-pick rebuild path; both rewrite commits already
137150
published to the remote, so a plain `git push` can be rejected either way — then run `/ship`

.agents/skills/emcn-design-review/SKILL.md

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,14 @@ User arguments: $ARGUMENTS
1616

1717
This codebase uses **emcn**, a custom component library built on Radix UI primitives with CVA variants and CSS variable design tokens. All UI must use emcn components and tokens.
1818

19+
## Deterministic conformance and fixes
20+
21+
For committed product UI changes, run `bun run check:design --base <actual-target-branch> --head HEAD`. This checks central derivation, component ownership and system changes; it does not judge whether an approved red variant or monospace font is a good product choice. The style review advice below may inform suggestions, but must not be presented as additional deterministic linter rules.
22+
23+
With fixes enabled, correct straightforward local violations using the cited central token, recipe or component prop. For a small gray adjustment, choose the approved token that fits its role (body, muted, icon or surface); do not hardcode the nearest hex. Ask the engineer when a correction has broad impact or ambiguous intent, especially before changing shared definitions or adding variants. An intentional change or justified artwork exception can proceed with an explanation in the PR; retain the warning. New shared standards should involve the designer. Do not enlarge this task into a whole-codebase cleanup or invent a central token just to pass.
24+
25+
Untouched debt stays quiet. Bringing noncompliant styling into a new file or component is an opportunity to fix it, even when its value is unchanged. Product artwork should use the registered central libraries; central asset changes notify. Landing pages, user media and sanctioned provider branding are separate scope. Unknown HTML illustrations may still receive normal styling warnings. Unresolved inputs are not proof of conformance. Only committed blobs are checked, and exit 2 means an operational failure; CI findings currently warn without blocking. See `scripts/design-conformance/README.md` for exact coverage.
26+
1927
## Steps
2028

2129
1. Read the emcn public barrel at `packages/emcn/src/index.ts` (re-exports components, Calendar, Table*, and icons) to know what's available; for the full icon set read `packages/emcn/src/icons/index.ts`

.agents/skills/ship/SKILL.md

Lines changed: 28 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,6 @@
11
---
22
name: ship
3-
description: Commit, push, and open a PR to staging in one shot — runs the cleanup pass and, when migrations changed, the db-migrate safety review first
3+
description: Commit, check design conformance, push, and open a PR to staging — runs cleanup and the applicable migration safety review first
44
argument-hint: "[optional context or scope notes]"
55
---
66

@@ -72,7 +72,7 @@ When the user runs `/ship`:
7272
bun run docs-manifest:check || { echo "❌ docs manifest out of sync — do not ship"; exit 1; }
7373
```
7474
If Phase A regenerated a file, its matching `:check` in Phase B now passes trivially — that parity is the point. Do not ship with any generator or audit failing; fix the cause (never silence it) and re-run. `check:migrations` and `type-check` are covered by steps 5 and CI respectively and are not repeated here.
75-
7. **Stage and commit** the changes with the generated message — including any files Phase A regenerated in step 6
75+
7. **Stage and commit** the changes with the generated message — including any files Phase A regenerated in step 6. Then run the [committed design check](#committed-design-check) below and resolve or explain its findings before step 8.
7676
8. **Push to origin** using the current branch name — `--force-with-lease` if step 2's sync
7777
check did any history rewrite (a clean rebase or a cherry-pick rebuild) on a branch that had
7878
already been pushed once; a plain push would be rejected in exactly the polluted-remote case
@@ -88,7 +88,31 @@ When the user runs `/ship`:
8888
positional/line-by-line comparison against the PR's oldest-first list can spuriously fail on
8989
any multi-commit branch. These two lists must describe the same commits in the same order
9090
(same subjects, the last one being the commit from step 7). If they don't match, the branch
91-
still has a problem — redo step 2's fix and `git push --force-with-lease`.
91+
still has a problem — redo step 2's fix, repeat the committed design check for the resulting HEAD, and `git push --force-with-lease`.
92+
93+
## Committed design check
94+
95+
After committing and before **every push**, run this from the repository root with Bun 1.4.1:
96+
97+
```bash
98+
bun run check:design --base origin/staging --head HEAD
99+
```
100+
101+
Run it for every `/ship`; let the checker apply its own scope. A `.tsx`-only condition would miss CSS, Tailwind configuration, artwork and contract-registry changes. `check:audits` deliberately excludes this base-dependent command. It reads committed merge-base → HEAD blobs, so a run before committing cannot validate the pending changes.
102+
103+
Interpret both the exit status and the report:
104+
105+
- **0 with a completed report:** no findings; continue to push.
106+
- **1 with a completed report:** read the usage violations and central-system notifications. Triage them before pushing; findings are warnings, not an automatic shipping failure.
107+
- **2, unexpected termination, or no completed report:** the check did not complete. Fix the operational problem and rerun before pushing. A startup failure with exit 1 is not a findings report. Do not hide failures with `|| true` or treat missing output as a pass.
108+
109+
Fix straightforward usage violations through the cited central component, prop, recipe or token. For a small local gray correction, choose the approved token appropriate to its role. Do not invent a new token or loosen a contract just to remove the warning. Keep fixes within the work being shipped; unchanged debt elsewhere can wait for its own cleanup.
110+
111+
When a change has broad shared impact or ambiguous intent, explain the finding and ask the engineer how to proceed. Intentional central-system changes and justified exceptions can proceed with an explanation in the PR; involve the designer for new standards or ambiguous broad changes. Honor decisions already given in this session. Retain the warning rather than weakening the linter or requiring every intended system change to produce a clean report.
112+
113+
If this review produces edits, rerun the affected generation/lint/audit checks from step 6, commit the fixes, then repeat the design check against the new HEAD. Also rerun after a rebase, conflict resolution or other change to the comparison. Push only the checked commit; uncommitted fixes are not covered by an earlier result.
114+
115+
In the PR's **Testing** section, record the design-check outcome and explain any retained warnings. Leave **No new warnings introduced** unchecked when warnings remain. This review is about central design-system conformance; approved component variants, colours and fonts remain available for the engineer's product decisions. See `scripts/design-conformance/README.md` for scope and known unchecked inputs.
92116

93117
## Commit Message Format
94118

@@ -137,7 +161,7 @@ Tested manually (or describe testing)
137161
- [x] Code follows project style guidelines
138162
- [x] Self-reviewed my changes
139163
- [ ] Tests added/updated and passing
140-
- [x] No new warnings introduced
164+
- [ ] No new warnings introduced
141165
- [x] I confirm that I have read and agree to the terms outlined in the [Contributor License Agreement (CLA)](./CONTRIBUTING.md#contributor-license-agreement-cla)
142166
```
143167

.github/workflows/test-build.yml

Lines changed: 13 additions & 39 deletions
Original file line numberDiff line numberDiff line change
@@ -201,18 +201,11 @@ jobs:
201201
timeout-minutes: 15
202202

203203
steps:
204-
# The diff-based audits below need a base commit to read, and the default
205-
# depth of 1 clones a single commit with no parent. They normally fetch
206-
# their base by SHA (see "Resolve base ref"), so this depth only covers the
207-
# `HEAD~1` fallback — but without it that fallback resolves to nothing.
208-
#
209-
# Worth stating because the failure was invisible for so long: the migration
210-
# audit read the resulting `git diff` failure as "no migrations changed" and
211-
# exited 0, so it had never actually run on a push build.
204+
# Design conformance compares immutable merge-base sources and requires full history.
212205
- name: Checkout code
213206
uses: actions/checkout@df4cb1c069e1874edd31b4311f1884172cec0e10 # v6
214207
with:
215-
fetch-depth: 2
208+
fetch-depth: 0
216209

217210
- name: Setup Bun
218211
uses: oven-sh/setup-bun@0c5077e51419868618aeaa5fe8019c62421857d6 # v2
@@ -301,37 +294,11 @@ jobs:
301294
302295
echo "✅ All env flags are properly configured"
303296
304-
# One fetch for both base-ref audits, and no `|| true`: a swallowed fetch leaves
305-
# the base ref absent, which neither audit can tell apart from a branch that
306-
# changed nothing. The block-registry check at least degrades to a visible
307-
# `⚠ … skipping` line; the migration audit printed `✓ No new migrations to
308-
# check` and exited 0, clearing the only guard on production DDL.
309-
#
310-
# Depth stays at 1 — without a merge-base the migration audit diffs the two
311-
# tips, which under `--diff-filter=AM` is exactly the migrations new here.
312-
# Resolved once for both diff-based audits, and never with `|| true`: a
313-
# swallowed fetch leaves the base absent, which neither audit can tell apart
314-
# from a branch that changed nothing.
315-
#
316-
# On push the base is `github.event.before`, the tip the branch had before
317-
# this push — not `HEAD~1`, which names only the last commit and would let a
318-
# multi-commit push slip every earlier commit's migrations past the audit.
319-
# It is fetched by SHA at depth 1; the audits diff two tips and need no
320-
# common ancestry. An all-zero `before` means the branch is new and has no
321-
# predecessor to diff, so `HEAD~1` remains the fallback there.
297+
# Resolve immutable event revisions without making the checkout shallow again.
298+
# The same base covers the complete push for migration and block-registry checks.
322299
- name: Resolve base ref for diff-based audits
323300
id: audit_base
324-
run: |
325-
if [ "${{ github.event_name }}" = "pull_request" ]; then
326-
git fetch --depth=1 origin "${{ github.base_ref }}"
327-
echo "ref=origin/${{ github.base_ref }}" >> "$GITHUB_OUTPUT"
328-
elif [ -n "${{ github.event.before }}" ] &&
329-
[ "${{ github.event.before }}" != "0000000000000000000000000000000000000000" ]; then
330-
git fetch --depth=1 origin "${{ github.event.before }}"
331-
echo "ref=${{ github.event.before }}" >> "$GITHUB_OUTPUT"
332-
else
333-
echo "ref=HEAD~1" >> "$GITHUB_OUTPUT"
334-
fi
301+
run: bun --no-env-file scripts/design-conformance/ci.ts --resolve-refs
335302

336303
- name: Check block registry invariants
337304
run: bun run apps/sim/scripts/check-block-registry.ts "${{ steps.audit_base.outputs.ref }}"
@@ -348,6 +315,13 @@ jobs:
348315
- name: Verify docs manifest is in sync
349316
run: bun run docs-manifest:check
350317

318+
# Only findings (exit 1) are non-blocking; operational errors still fail CI.
319+
- name: Design conformance (warnings)
320+
env:
321+
DESIGN_BASE: ${{ steps.audit_base.outputs.ref }}
322+
DESIGN_HEAD: ${{ steps.audit_base.outputs.head }}
323+
run: bun --no-env-file scripts/design-conformance/ci.ts --base "$DESIGN_BASE" --head "$DESIGN_HEAD"
324+
351325
- name: Migration safety (zero-downtime) audit
352326
run: bun run check:migrations "${{ steps.audit_base.outputs.ref }}"
353327

@@ -358,7 +332,7 @@ jobs:
358332
# excludes *.test.ts(x), and including them today surfaces ~2.2k errors,
359333
# so that is its own cleanup rather than a gate to switch on here.
360334
- name: Type-check all workspaces
361-
run: bunx turbo run type-check
335+
run: bun run type-check
362336

363337
# cloud-review-tools.test.ts runs the real helper on the runner, which shells
364338
# out to rg. Blacksmith's image ships it, GitHub's doesn't.

CLAUDE.md

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -4,11 +4,12 @@ You are a professional software engineer. All code must follow best practices: a
44

55
## Global Standards
66

7+
- **Design conformance**: Before pushing committed product UI changes, run `bun run check:design --base <target-branch-ref> --head HEAD` (for example, `origin/staging`). This prevents drift from central EMCN components, tokens and approved artwork; approved combinations remain the engineer's choice. Fix routine violations through the cited central mechanism, including choosing a suitable approved gray for a small local correction. Ask the engineer when intent is unclear or a fix changes shared tokens, recipes or variants. Intentional system changes and justified exceptions may proceed with an explanation in the PR; keep the warning visible and involve the designer for new standards or broad ambiguous changes. Added, changed or extracted noncompliant styling needs correction; untouched debt can wait for the separate cleanup pass. CI findings are warnings; exit 2 is an operational failure. Only committed blobs are checked; see `scripts/design-conformance/README.md` for scope and unchecked inputs.
78
- **Linting / Audit**: `bun run check:api-validation` must pass on PRs. Do not introduce route-local boundary Zod schemas, direct route Zod imports, or ad-hoc client wire types — see "API Contracts" and "API Route Pattern" below
89
- **Logging**: Import `createLogger` from `@sim/logger`. Use `logger.info`, `logger.warn`, `logger.error` instead of `console.log`. Inside API routes wrapped with `withRouteHandler`, loggers automatically include the request ID — no manual `withMetadata({ requestId })` needed
910
- **API Route Handlers**: All API route handlers (`GET`, `POST`, `PUT`, `DELETE`, `PATCH`) must run inside `withRouteHandler`. Ordinary internal and v2 handlers use the shared JSON/binary route builders, which already apply it; never double-wrap a builder. Use raw `withRouteHandler` only for documented protocol or lifecycle exceptions. See "API Route Pattern" below
1011
- **Comments**: Use TSDoc for documentation. No `====` separators. No non-TSDoc comments
11-
- **Styling**: Never update global styles. Keep all styling local to components
12+
- **Styling**: Derive product styling from central tokens, component props and recipes. Keep product composition local. Change central definitions only when intentionally changing the shared standard, and explain that change; never add a global token or override solely to hide a local conformance warning. Landing-page presentation is exempt from the product conformance policy.
1213
- **ID Generation**: Never use `crypto.randomUUID()`, `nanoid`, or `uuid` package. Use `generateId()` (UUID v4) or `generateShortId()` (compact) from `@sim/utils/id`
1314
- **Common Utilities**: Use shared helpers from `@sim/utils` instead of inline implementations:
1415
- `sleep(ms)` from `@sim/utils/helpers` — never `new Promise(resolve => setTimeout(resolve, ms))`

biome.json

Lines changed: 20 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,10 @@
11
{
22
"$schema": "https://biomejs.dev/schemas/2.0.6/schema.json",
3-
"vcs": { "enabled": true, "clientKind": "git", "useIgnoreFile": false },
3+
"vcs": {
4+
"enabled": true,
5+
"clientKind": "git",
6+
"useIgnoreFile": false
7+
},
48
"files": {
59
"ignoreUnknown": false,
610
"includes": [
@@ -143,8 +147,12 @@
143147
"overrides": [
144148
{
145149
"includes": ["apps/sim/app/_styles/tailwind.css"],
146-
"linter": { "enabled": false },
147-
"formatter": { "enabled": false }
150+
"linter": {
151+
"enabled": false
152+
},
153+
"formatter": {
154+
"enabled": false
155+
}
148156
},
149157
{
150158
"includes": ["packages/**"],
@@ -156,6 +164,15 @@
156164
}
157165
}
158166
}
167+
},
168+
{
169+
"includes": [
170+
"scripts/design-conformance/catalogue.json",
171+
"scripts/design-conformance/contracts.json"
172+
],
173+
"formatter": {
174+
"enabled": false
175+
}
159176
}
160177
],
161178
"javascript": {

0 commit comments

Comments
 (0)