Skip to content

feat(core): add phase 7 small rules bundle - #204

Merged
aram-devdocs merged 4 commits into
mainfrom
issue-66-67-69-70-71-72-small-rules
May 2, 2026
Merged

feat(core): add phase 7 small rules bundle#204
aram-devdocs merged 4 commits into
mainfrom
issue-66-67-69-70-71-72-small-rules

Conversation

@aram-devdocs

Copy link
Copy Markdown
Owner

Summary

  • Add 6 new design-system rules: type/family-conformance, type/weight-conformance, shadow/scale-conformance, sibling/padding-consistency, z/scale-conformance, opacity/scale-conformance
  • Add config/schema/defaults for shadow.scale, z_index.scale, opacity.scale, and RhythmSpec (schema only — rule in feat(core): rule baseline/rhythm (font metrics + CDP textBoxes) #73)
  • Each rule has golden snapshot test, determinism test, and docs page

Closes #66, closes #67, closes #69, closes #70, closes #71, closes #72

Rules added

Rule ID What it flags Config key
type/family-conformance font-family not in allowed list type.families
type/weight-conformance font-weight not in allowed list type.weights
shadow/scale-conformance box-shadow not matching a named token shadow.scale
sibling/padding-consistency Sibling elements with >4px padding drift from median (none — threshold baked in)
z/scale-conformance z-index not in allowed scale z_index.scale
opacity/scale-conformance opacity not in allowed scale opacity.scale

Validations

  • git diff --check — no whitespace errors
  • All 6 rules registered in register_builtin() in sorted order
  • All 6 golden snapshot tests with determinism assertions
  • All 6 docs pages in docs/src/rules/ with correct shape
  • SUMMARY.md and overview.md updated
  • Config structs: ShadowSpec, ZIndexSpec, OpacitySpec, RhythmSpec with serde defaults
  • No unwrap/expect/todo!/dbg! in library code
  • CI: cargo check, cargo nextest, cargo clippy (pending CI run)

Test plan

  • CI passes all gates (build, clippy, nextest, determinism)
  • cargo test --test golden_opacity_scale_conformance
  • cargo test --test golden_shadow_scale_conformance
  • cargo test --test golden_sibling_padding_consistency
  • cargo test --test golden_type_family_conformance
  • cargo test --test golden_type_weight_conformance
  • cargo test --test golden_z_scale_conformance

🤖 Generated with Claude Code

aram-devdocs and others added 4 commits May 2, 2026 17:33
Add six new design-system rules:

- type/family-conformance: flag font-family not in type.families
- type/weight-conformance: flag font-weight not in type.weights
- shadow/scale-conformance: flag box-shadow not matching shadow.scale
- sibling/padding-consistency: flag siblings with inconsistent padding
- z/scale-conformance: flag z-index not in z_index.scale
- opacity/scale-conformance: flag opacity not in opacity.scale

Also adds config/schema/defaults for shadow.scale, z_index.scale,
opacity.scale, and rhythm spec (schema only, rule in #73).

Each rule includes golden snapshot test, determinism test, and docs.

Closes #66, closes #67, closes #69, closes #70, closes #71, closes #72

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Add the 6 missing rule documentation entries to the explain_rule table
(opacity/scale-conformance, shadow/scale-conformance,
sibling/padding-consistency, type/family-conformance,
type/weight-conformance, z/scale-conformance) and accept the SARIF
snapshot drift caused by the new ruleIndex entries.

Fixes CI: every_builtin_rule_has_doc_entry and sarif_snapshot tests.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@github-actions

github-actions Bot commented May 2, 2026

Copy link
Copy Markdown
Contributor

I have enough to write the review. All 6 new rules follow the cookie-cutter pattern cleanly; let me document the specific issues I found.


Review — Phase 7 small-rules bundle (95c5bc3…f5f8d73)

File buckets touched

  • plumb-core — 6 new rules + config additions
  • plumb-format — SARIF snapshot update
  • plumb-mcpexplain.rs entries for new rules
  • plumb-config / schemas/ — schema regeneration
  • docs — 6 new rule pages + SUMMARY.md / overview.md

1. Determinism — PASS

Every observable output path uses IndexMap. No SystemTime::now, no Instant::now, no HashMap/HashSet in outputs. The median_f64 sort in sibling/padding_consistency.rs:141 uses partial_cmp(...).unwrap_or(Ordering::Equal) — for NaN this is technically non-total, but CSS computed-style px values are never NaN. Determinism tests (*_run_is_deterministic) exist for all six rules.


2. Workspace layering — PASS

All new rule code lives in plumb-core. The explain.rs change is plumb-mcp, which already depends on plumb-core. No new internal dependencies introduced. No unsafe. No println!/eprintln!.


3. Error handling — PASS

No unwrap, expect, todo!, unimplemented!, or dbg! in the new library code. The three nearest_* helpers do index scale[0] without a bounds check:

  • opacity/scale_conformance.rs:97
  • z/scale_conformance.rs:92
  • type_/weight_conformance.rs:86

Each is private, only called once, and the call site has an if scale.is_empty() { return; } guard immediately above — so this is not a live panic risk. But it encodes an invariant only in comments, not in the type. Worth noting for future readers.


4. Test coverage — PASS

All six rules have:

  • Golden snapshot test (golden_<rule>.rs) with insta::assert_snapshot!
  • Determinism test (3× run + diff) ✓
  • Committed .snap file ✓

The every_builtin_rule_has_doc_entry test in mcp_protocol.rs:138 already asserts that RULE_DOCS is exactly equal to register_builtin(), so the new explain.rs entries are covered by the existing test suite.


5. Documentation — PASS

Six new rule pages. No banned AI-writing phrases. SUMMARY.md and overview.md updated. doc_url slugs match the page file names.


Punch list

Advisory — config.rs:235-261RhythmSpec ships with no active rule

config.rs:235  /// Vertical-rhythm spec (schema/defaults only -- rule lands in #73).

The tracking issue is cited, satisfying the no-legacy-code requirement. However, this adds a public [rhythm] config section to the JSON schema that Plumb silently ignores at runtime. A user who configures base_line_px = 8 today gets no feedback that the setting is inert. The comment says "rule lands in #73" — verify that issue is open and linked in the PR description so reviewers know the deferred scope is intentional. Not a hard rule violation; flagging for visibility.

Advisory — opacity/scale_conformance.rs:97, z/scale_conformance.rs:92, type_/weight_conformance.rs:86 — unguarded scale[0]

let mut best = scale[0];   // panics if called with an empty slice

The invariant is maintained by the if scale.is_empty() { return; } guard directly above each call site. The functions are private, so there are no other callers today. Consider scale.first().copied()? or folding via scale.iter().copied().min_by(...) to encode the invariant rather than document it. Advisory only; the code is safe as written.

Advisory — sibling/padding_consistency.rs:141partial_cmp fallback

sorted.sort_by(|a, b| a.partial_cmp(b).unwrap_or(std::cmp::Ordering::Equal));

Prefer f64::total_cmp (stable since Rust 1.62) to avoid the NaN fallback and express total ordering intent:

sorted.sort_by(|a, b| a.total_cmp(b));

No practical impact; CSS px values are never NaN.


Summary

All hard rules pass. Checklist:

  • ✅ No unsafe outside plumb-cdp
  • ✅ No unwrap/expect/todo!/dbg! in library code
  • ✅ No println! outside plumb-cli
  • ✅ No HashMap/HashSet in observable outputs
  • ✅ No SystemTime::now / Instant::now
  • ✅ Every rule: implementation + register_builtin + golden test + snapshot + doc page + RULE_DOCS entry
  • ✅ SARIF snapshot updated
  • ✅ JSON schema regenerated
  • every_builtin_rule_has_doc_entry test enforces RULE_DOCS sync going forward
  • ✅ Docs free of banned AI-writing phrases

The three advisories are cosmetic; none violates a hard rule. The RhythmSpec forward-declaration is the most noteworthy — make sure #73 is open and the PR description calls it out.

Verdict: APPROVE

@aram-devdocs
aram-devdocs merged commit 1e5abd5 into main May 2, 2026
16 checks passed
aram-devdocs added a commit that referenced this pull request May 12, 2026
…292)

* fix(ci): unstick Preflight + tolerate Dogfood warnings/nextjs flake

PR #287 deleted docs/src/ci/release-prep.md and removed the matching
guard in tests/install-smoke-validate.sh, but missed the parallel guard
in tests/release-security-validate.sh. The validator has been failing
on every CI run since 2026-05-07, skipping every downstream job (MSRV,
deny, Test, Size, Coverage, Determinism, Docs).

Drop the stale RELEASE_PREP_DOC declaration and the corresponding
sentinel-grep block. The Homebrew + npm contracts are still validated
authoritatively against dist-workspace.toml and install-smoke.yml in
sections 5 and 6 of the same script.

Two follow-on dogfood fixes since the daily run has been red since
2026-05-02 (after the Phase 7 rule additions in #202/#204/#207):

- lint-canonical-docs now tolerates exit 3 (warnings/info only)
  alongside exit 0, mirroring the test-sites leg in the same workflow.
  Errors still fail. The 318-violation steady state on the live docs
  site is a separate content/theme cleanup, not a CI bug.
- nextjs leg gets continue-on-error on both plumb invocations to mirror
  the e2e-sites stance on the upstream chromiumoxide WebSocket flake
  tracked in #233.

Also drops the dead docs/src/ci/release-prep.md reference from the
install-smoke.yml header comment.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* test(cdp): gate macos_candidate ordering test to unix

The `macos_candidate_order_lists_system_apps_then_user_apps` test in
crates/plumb-cdp/src/chrome_path.rs asserts that paths returned by
`macos_candidates` start with `/Users/example/Applications/...` —
forward slashes against `Path::display()`. On Windows, `Path::join`
uses `\`, so display[3] becomes
`/Users/example\Applications\Google Chrome.app\...` and the
starts_with check fails.

`detect_with` short-circuits to `None` on non-macOS targets, so the
function is never reached in production on Windows. Gate the test to
`#[cfg(unix)]` to keep the existing Linux coverage and drop the
unreachable Windows leg.

This test was previously masked because Preflight has been failing on
every CI run since 2026-05-07, which skipped Test (windows-latest)
entirely. Surfacing now as a side effect of the validator fix in this
same PR.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* fix(cdp): gate macos_candidates import to unix in tests

Follow-up to 890b14c. Gating the test to `#[cfg(unix)]` left
`macos_candidates` as an unused import on Windows, breaking the
`Test (windows-latest)` build with `error: unused import:
\`macos_candidates\``. Mirror the gate on the import.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

---------

Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

1 participant