feat(design-system): gate the ungated prohibitions and close gate 4 - #1770
Merged
Conversation
Closes #264 and the gate-4 half of #265. Measured against origin/main 8db1e53 rather than scoped from the handover, which mattered: four of #264's six prohibitions were already gated while GATES.md §3 read `planned`, and that understatement is what deferred this task twice. Tighten two ratchets carrying stale slack to their measured values: edgeOwnershipConflicts 28 -> 27 and legacyShadowAliases 231 -> 224. Seven files had paid debt down without a baseline refresh, so up to seven new violations would have passed. Every other metric and debtByPath entry was asserted unchanged first. The remaining 224 aliases are #262's cleanup. Add three checks to check:design-system-contract: - statusColouredNumerals (ratcheted 2) — a text-* status hue on an element whose children are all figures. - colourOnlyStatusIndicators (ratcheted 4) — gate 4's repository-wide enumeration. A status hue on a box with no children, no accessible name on it or any ancestor, no sibling text, and not a StatusMark. Also flags shared swatch recipes, since the analyzer is per-file and cannot follow an imported statusDotReady to its call sites. - imageInversions (pinned at zero) — CSS filter/backdrop-filter and the Tailwind invert/hue-rotate utilities. All three are mutation-verified, as is the tightening: reintroducing one var(--shadow-tight) now fails with `legacyShadowAliases increased from 224 to 225` plus the per-path assertion, where the same edit passed at 231. The two status metrics were re-measured three times before the baseline was written. The first draft found 19 colour-only indicators and 3 numerals; 15 were false positives — a name test loose enough that `size` read as a numeral, an ancestor walk checking for JsxOpeningElement when ancestors are JsxElement, and no sibling-text rule, which condemned the ordinary legend pattern. All six recorded entries were read in source and confirmed real. Gate 2 is NOT closed. A rendered-interactive tap enumeration was written, shown to find genuine defects, and reverted: six runs against one production build returned 6, 5, 4, 3, 3 and 9 distinct sub-floor shapes because the audit races the async render, and networkidle plus shape deduplication did not settle it. ui-style-contract.spec.ts runs in the required Production UI job, so an intermittent version would have blocked every merge. Recorded with the defect it found as #289. Correct GATES.md throughout, including the gate 2 row's claim that test:e2e:style-contract needs wiring into verify:cheap — the spec already runs in required CI via productionSpecPattern and playwright-pr-shards, and adding it to verify:cheap:internal would trip check:gate-manifest, which requires every gate in that chain to also run in the browser-less static-pr job. Close #277, verified already satisfied on main. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
This pull request has been ignored for the connected project Preview Branches by Supabase. |
Contributor
📝 WalkthroughWalkthroughThe design-system contract analyzer now checks status-colour usage and image inversion. The contract gate enforces image-inversion findings, updates debt baselines, and documents current gate evidence and outstanding issues. ChangesDesign-system contract enforcement
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant ContractChecker
participant ContractUtils
participant Baseline
ContractChecker->>ContractUtils: Analyze JSX and CSS contracts
ContractUtils-->>ContractChecker: Return status-colour and image-inversion findings
ContractChecker->>Baseline: Compare debt metrics
ContractChecker-->>ContractChecker: Fail when image inversions are found
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 2 | ❌ 3❌ Failed checks (3 warnings)
✅ Passed checks (2 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@scripts/design-system-contract-utils.mjs`:
- Around line 732-747: Update isNumeralExpression to recognize only expressions
that produce numeric values: remove broad binary || recursion and explicitly
accept only numeric-producing operators, while rejecting string concatenation,
logical, comparison, and boolean operators including prefix !. Add fixtures
covering "Rank " + count, count && 1, count === 1, and !count, while preserving
valid numeric expressions.
- Around line 779-783: Restrict rendersVisibleText to JSX expressions that
produce readable text rather than treating every non-empty expression as visible
text. Update its ts.isJsxExpression branch to accept only statically known
text-producing values, while returning false for identifiers, JSX elements,
null, and conditional/non-text expressions unless they have an explicit
accessible name; preserve the existing JSX text and child traversal behavior.
- Around line 808-819: Update hasAccessibleNameInScope to validate
accessible-name attribute values before returning true: reject empty labels and
require aria-labelledby references to resolve to existing elements with usable
text. Preserve the ancestor traversal and only exempt colourOnlyStatusIndicators
when the name is genuinely accessible, and add fixtures covering empty labels
and unresolved references.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 8ad028f6-47e5-4e5a-99c5-f651df2c05af
📒 Files selected for processing (5)
docs/design-system/GATES.mddocs/outstanding-issues.mdscripts/check-design-system-contract.mjsscripts/design-system-contract-baseline.jsonscripts/design-system-contract-utils.mjs
…ocking # Conflicts: # docs/outstanding-issues.md
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8ed66a0570
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Adds design-system contract enforcement for status-colour misuse and image inversion while tightening existing debt ratchets.
Changes:
- Adds AST/CSS checks for colour-only indicators, coloured numerals, and inversion filters.
- Refreshes contract baselines and gate documentation.
- Updates issue and review ledgers with completed and remaining work.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
scripts/design-system-contract-utils.mjs |
Implements new contract analyzers. |
scripts/design-system-contract-baseline.json |
Records new metrics and tighter limits. |
scripts/check-design-system-contract.mjs |
Integrates and reports new checks. |
docs/outstanding-issues.md |
Archives completed work and tracks follow-up defects. |
docs/design-system/GATES.md |
Updates gate status and evidence. |
docs/branch-review-ledger.md |
Records review and verification history. |
Suppressed comments (1)
scripts/design-system-contract-utils.mjs:781
- Every non-empty JSX expression AST node is treated as visible text, even expressions that render nothing or only an icon. For example, a bare status swatch followed by
{false},{null}, or{condition && <Icon />}is exempted byhasSiblingTextChannel, so a new colour-only indicator can bypass the gate. Inspect expression kinds/branches and only return true when the expression can render an actual textual channel.
if (ts.isJsxExpression(node)) return Boolean(node.expression);
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Co-authored-by: BigSimmo <87357024+BigSimmo@users.noreply.github.com>
All seven review findings reproduced against a probe before any change, and
every one was real. Two mattered:
- Neither status pattern accepted Tailwind's `/NN` opacity modifier, and 83
status-token utilities in `src` carry one. The anchored patterns rejected
every one before the semantic checks ran, so a colour-only indicator written
`bg-[color:var(--danger)]/90` walked past a ratchet described as
repository-wide.
- `NUMERAL_TEXT` rejected every letter while its own comment claimed units were
covered, so a dose painted in a status colour — the case the rule exists for
— passed silently. Replaced with `isNumeralTextFragment`, which accepts an
explicit unit list and still rejects prose. It carries no digit requirement,
because `{dose} mg` splits into an expression and a unit-only text child.
Also:
- `isNumeralExpression` now requires an arithmetic operator, rejects any
string/template operand anywhere in the tree, and follows only `+`/`-` prefix
unaries, so `{count + " errors"}` is no longer a numeral. The previous comment
claiming a concatenation "would have a string literal rather than a numeric
one" was wrong: one numeric side was enough.
- `INVERSION_FUNCTION` matches the `invert(`/`hue-rotate(` call itself, covering
`filter-[invert(1)]`, `[filter:invert(1)]` and both `backdrop-` forms.
- `IMAGE_INVERSION_UTILITY` excludes `-0`; `invert-0` disables inversion and a
hard-zero gate must not reject the reset.
- `rendersVisibleText` no longer counts `{null}`, `{false}`, `{undefined}` or a
bare self-closing element as a text channel, and judges markup-building
expressions by that markup.
- `hasNonEmptyAccessibleName` replaces the attribute-presence test, so
`aria-label=""` no longer exempts an indicator.
- The Gate 9 row said a layout-property lint was "planned" while
`layoutTransitionExceptions` already ships and ratchets per path — the exact
contradiction this branch exists to remove.
Fixing these surfaced two false positives of my own, both caught by
re-verifying every baseline entry in source rather than trusting the count: the
sibling walk stopped at the first non-JSX parent, so a conditional badge with a
label was reported colour-only; correcting that then let a sibling
`{started ? <div/> : null}` — another coloured div — count as text.
Baseline unchanged at 4 colour-only / 2 numerals / 0 inversions, and unchanged
by the unit widening, so nothing in the repo paints a dose in a status colour.
`legacyShadowAliases` 224 -> 220 from the main merge. Every new form is
mutation-verified.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
docs/outstanding-issues.md (2)
493-494: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftDisambiguate issue IDs before archiving these objectives.
docs/outstanding-issues.mdassigns#264to DS Track A5 and#277to the handover issue.docs/branch-review-ledger.mdalso uses#264for the M2 design-system scope. IfforceEmbeddingand anonymous/api/setup-statususe a separate namespace, document it and add stable linked references. Otherwise, correct these records and do not treat rows 493–494 as those objectives.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/outstanding-issues.md` around lines 493 - 494, Disambiguate the conflicting issue ID `#264` references: update docs/outstanding-issues.md lines 493-494 and docs/branch-review-ledger.md line 831 so the DS Track A5, handover, and M2 scope records use stable, unambiguous linked references. First verify whether forceEmbedding and anonymous /api/setup-status have a separate issue namespace; if so, document that namespace and link the records, otherwise correct the duplicated IDs and ensure rows 493–494 are not treated as the ledger’s M2 objectives.
35-37: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winLimit the “never closed” statement to open rows.
Archived rows are absent from this queue by design. The current wording can make a closed row appear merely deprioritised. State that absence means deprioritised only for an open row, and direct readers to the detailed tables for closure status.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/outstanding-issues.md` around lines 35 - 37, Update the queue description in the “This queue” section so “absence means deprioritised” applies only to rows that are currently open. Clarify that archived or closed rows may be absent by design, and direct readers to the detailed tables to determine closure status.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@docs/outstanding-issues.md`:
- Around line 493-494: Disambiguate the conflicting issue ID `#264` references:
update docs/outstanding-issues.md lines 493-494 and docs/branch-review-ledger.md
line 831 so the DS Track A5, handover, and M2 scope records use stable,
unambiguous linked references. First verify whether forceEmbedding and anonymous
/api/setup-status have a separate issue namespace; if so, document that
namespace and link the records, otherwise correct the duplicated IDs and ensure
rows 493–494 are not treated as the ledger’s M2 objectives.
- Around line 35-37: Update the queue description in the “This queue” section so
“absence means deprioritised” applies only to rows that are currently open.
Clarify that archived or closed rows may be absent by design, and direct readers
to the detailed tables to determine closure status.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: bf8c3129-b00f-4ca8-8b0d-d6258fb82fb1
📒 Files selected for processing (4)
docs/branch-review-ledger.mddocs/design-system/GATES.mddocs/outstanding-issues.mdscripts/design-system-contract-utils.mjs
🚧 Files skipped from review as they are similar to previous changes (2)
- docs/design-system/GATES.md
- scripts/design-system-contract-utils.mjs
The SWE agent pushed two commits to this branch fixing the same two findings
I was fixing. Resolved by keeping the better half of each rather than
force-pushing over its work.
Took its `STATUS_ALPHA_MODIFIER` over mine: `(?:/(?:\d+(?:\.\d+)?|\[[^\]]+\]))?`
also covers `/50.5` and `/[0.35]`, which my `(?:\/\d{1,3})?` did not.
Kept `isNumeralTextFragment` over its `NUMERAL_TEXT` regex. Measured, that
regex classifies "12 errors" and "3 overdue" as numerals, because it allows any
trailing alphabetic run — the exact false-positive class Codex flagged on the
expression path in the same review round, where the visible word IS the
required non-colour channel. The explicit unit list rejects both while still
accepting "1.5 mg", "40-60%", "10 mg/day" and the unit-only text child left
beside a `{dose}` expression.
Verified on all eleven cases the reviewers raised. Baseline unchanged at 4
colour-only / 2 numerals / 0 inversions.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Owner
Author
|
Re the out-of-diff finding on Those two lines are the
The No change made. There was a real id collision in this PR earlier — 🤖 Addressed by Claude Code |
…ocking # Conflicts: # docs/outstanding-issues.md
…erge Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
BigSimmo
enabled auto-merge (squash)
August 9, 2026 04:50
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/outstanding-issues.md`:
- Line 334: Fix the Markdown formatting in the issue `#291` row by closing the
code span around eslint-rules/require-button-wiring.mjs and removing the stray
quote, so subsequent text renders normally and satisfies MD038.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 395bbeac-755c-4f7f-a460-b942e6d48a29
📒 Files selected for processing (5)
docs/branch-review-ledger.mddocs/design-system/GATES.mddocs/outstanding-issues.mdscripts/design-system-contract-baseline.jsonscripts/design-system-contract-utils.mjs
🚧 Files skipped from review as they are similar to previous changes (3)
- docs/design-system/GATES.md
- scripts/design-system-contract-utils.mjs
- docs/branch-review-ledger.md
Copilot stopped work on behalf of
BigSimmo due to an error
August 12, 2026 11:03
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Closes
#264and the gate-4 half of#265; closes#277. Files a new#291(renumbered from #289 after main claimed that id).Measured against
origin/main8db1e53937rather than scoped from the handover, whichmattered: four of
#264's six prohibitions were already gated whileGATES.md§3 readplanned. That understatement is what deferred this task twice.edgeOwnershipConflicts28 → 27,legacyShadowAliases231 → 224. Seven files had paid debt down without a baseline refresh,so up to seven new violations would have passed. Every other metric and
debtByPathentrywas asserted unchanged first. The remaining 224 aliases are
#262's cleanup, not this PR.statusColouredNumerals(new, ratcheted at 2) — atext-*status hue on an elementwhose children are all figures.
colourOnlyStatusIndicators(new, ratcheted at 4) — gate 4's repository-wideenumeration. A status hue on a box with no children, no accessible name on it or any
ancestor, no sibling text, and not a
StatusMark. Also flags shared swatch recipes, sincethe analyzer is per-file and cannot follow an imported
statusDotReadyto its call sites.imageInversions(new, pinned at zero) — CSSfilter/backdrop-filterplus theTailwind
invert/hue-rotateutilities.GATES.mdcorrected throughout, including the gate 2 row's claim thattest:e2e:style-contractneeds wiring intoverify:cheap.#277closed as already satisfied — verified on main, not inferred.Gate 2 is deliberately not closed
A rendered-interactive tap enumeration was written, shown to find genuine defects, and then
reverted rather than landed. Six runs against one production build returned 6, 5, 4, 3, 3
and 9 distinct sub-floor shapes, largely disjoint, because the audit races the async render;
waitForLoadState("networkidle")plus deduplication to distinct shapes did not settle it, andtwo consecutive agreeing runs turned out to be coincidence.
tests/ui-style-contract.spec.tsruns in the required Production UI job, so an intermittent version would have blocked every
merge in the repo.
What it established in every run is a live defect, now
#291: controls that carrymin-h-tapcomputemin-height: 0pxand render at 16–36px. The existing declared-carrieraudit cannot report them by construction — it only measures elements already computing at or
above the floor, so a floor overridden downward is skipped.
Related correction, recorded in
GATES.md:test:e2e:style-contractis only an alias forrunning that one spec. The spec already runs in required CI via
productionSpecPatternandscripts/playwright-pr-shards.mjs. It must not be added toverify:cheap:internal,because
check:gate-manifestthen demands a matching step in the browser-lessstatic-prjob.Testing
check:design-system-contract— pass:Status-colour boundary: colour-only status indicators 4; status-coloured numerals 2; image inversions 0.statusColouredNumerals increased from 2 to 3,colourOnlyStatusIndicators increased from 4 to 5, andinversion filters would recolour clinical imagery, each with its per-path line.For the tightening, one added
var(--shadow-tight)fails withlegacyShadowAliases increased from 224 to 225— the same edit passed at the old 231 ceiling.lintexit 0 ·tsc -p tsconfig.typecheck.json --noEmitexit 0, zero errorscheck:icon-scale,check:type-scale,check:outstanding-issues,docs:check-links,docs:check-index— all pass.format:check—All matched files use Prettier code style!Test Files 3 passed (3),Tests 126 passed (126)verify:cheap—Test Files 5 failed | 536 passed (541),Tests 10 failed | 5777 passed | 14 skipped.The failures are pre-existing and not from this diff, and that is measured rather than
asserted: running the five failing files against a pristine
origin/maintree gives4 failed | 1 passed (5),9 failed | 23 passed, and re-applying this diff reproducesexactly the same
4 failed | 9 failed.worker-observabilitypasses in both batches — itsfull-run failure is order-dependent.
Baseline honesty
The two status metrics were re-measured three times before the baseline was written. The first
draft found 19 colour-only indicators and 3 numerals; 15 were false positives in three
shapes — a name test loose enough that
sizeread as a numeral, an ancestor walk checking forJsxOpeningElementwhen ancestors areJsxElement, and no sibling-text rule, which condemnedthe ordinary legend pattern of a coloured disc beside its label. All six recorded entries were
read in source and confirmed real.
Classifier reports
clinicalRisk: false,operationalRisk: false,ragRanking: false, so nogovernance preflight or RAG-impact line applies. No auto-merge: this PR changes gates.
🤖 Generated with Claude Code
Summary by CodeRabbit
Documentation
Quality Improvements