Skip to content

feat(core): rules spacing/grid + spacing/scale + type/scale - #108

Merged
aram-devdocs merged 8 commits into
mainfrom
codex/17-feat-core-spacing-and-type-scales
Apr 25, 2026
Merged

feat(core): rules spacing/grid + spacing/scale + type/scale#108
aram-devdocs merged 8 commits into
mainfrom
codex/17-feat-core-spacing-and-type-scales

Conversation

@aram-devdocs

Copy link
Copy Markdown
Owner

Spec

Closes #17
Closes #18
Closes #19

Part of the Phase 1 milestone (parent #10) → Batch 1C — first three real built-in rules, replacing the walking-skeleton placeholder/hello-world.

Summary

  • Adds three built-in rules in plumb-core:
    • spacing/grid-conformance — flags margin-*, padding-*, gap/row-gap/column-gap values that aren't multiples of spacing.base_unit. Fix snaps to nearest multiple, sign preserved (-13px-12px). Per-rule severity overrides honored at the engine level.
    • spacing/scale-conformance — flags the same property set when the value isn't a member of spacing.scale. Fix is the nearest member by absolute delta; ties go to the lower scale value. No-op when scale is empty.
    • type/scale-conformance — same shape, applied to font-size against type.scale.
  • Retires the deprecated placeholder/hello-world rule, its golden test, its docs page, and its SUMMARY entry. The rule's own #[deprecated] note said "removed when the first real rule lands"; this is that PR.
  • Folds in a portability fix to the SubagentStop review-gate hooks (tac is GNU-only; macOS was silently no-op'ing the tracker, so review gates never recorded). Slightly out-of-scope for the rule work, but the broken hooks blocked dispatch of the next gate during this very PR. Documented in the fix(hooks) commit body and easy to split if you'd prefer.

Test plan

  • cargo fmt --all -- --check — clean
  • cargo clippy --workspace --all-targets --all-features -- -D warnings — clean
  • cargo nextest run --workspace --all-features68 passed, 0 skipped (includes the two e2e-chromium driver tests)
  • just determinism-checkOK — all three runs produced byte-identical output
  • cargo xtask sync-rules-index▸ 3 built-in rule(s); all have docs pages
  • New tests added: 9 unit tests on the shared parse_px / nearest_multiple / nearest_in_scale helpers, 3 golden snapshot tests (one per rule) with _golden and _run_is_deterministic cases each, plus an extension to test_review_gate_session_id.sh covering the multi-line-verdict regression.
  • Docs updated: three new pages under docs/src/rules/, docs/src/SUMMARY.md re-pointed, docs/src/rules/overview.md refreshed, CHANGELOG [Unreleased] updated with ### Added and a new ### Removed.
  • Humanizer pass on the three new rule docs — no AI-tells in delve/comprehensive/leverage/seamless/robust etc.

Screenshots / terminal output

just validate (final)
PASS [   0.041s] plumb-core::golden_spacing_grid spacing_grid_conformance_golden
PASS [   0.039s] plumb-core::golden_spacing_scale spacing_scale_conformance_golden
PASS [   0.037s] plumb-format::format_snapshots json_snapshot
PASS [   0.037s] plumb-format::format_snapshots sarif_snapshot
PASS [   0.037s] plumb-format::format_snapshots pretty_snapshot
PASS [   0.037s] plumb-format::format_snapshots mcp_compact_snapshot
PASS [   0.038s] plumb-core::golden_type_scale type_scale_conformance_golden
PASS [   3.225s] plumb-cdp::driver_contract chromium_driver_captures_static_fixture
PASS [   3.226s] plumb-cdp::driver_contract chromium_driver_snapshot_is_byte_identical
────────────
     Summary [   3.227s] 68 tests run: 68 passed, 0 skipped

Breaking change?

  • No

The rule registry grew from 1 → 3, but all output remains additive: existing JSON/SARIF/MCP-compact shapes are unchanged. plumb-fake://hello continues to surface one violation (now from spacing/grid-conformance against a longhand padding-top: 13px, expanded from the prior shorthand fixture; PRD §10.3 specifies CDP returns longhands).

Anything reviewers should double-check

  1. Default severity = Warning vs PRD §11.2 Error. The plan picked Warning (matches the placeholder's posture and lets users opt-in to CI-failing severity via [rules.""] overrides); the spec reviewer flagged the divergence. If PRD wins, flip the three default_severity values in one follow-up.
  2. PlumbSnapshot::canned() was rewritten to use longhands so existing CLI / MCP / format-snapshot tests still observe one violation after the placeholder is gone. The diff in crates/plumb-core/src/snapshot.rs is small but it's a fixture-level behavior change that ripples through five test files.
  3. The hook fix. Two real bugs: tac is missing on macOS, and the verdict was only checked against the first line of a reviewer's message. Both fixed; new regression test added. If you'd prefer this in a separate PR, I'm happy to revert it from this one and re-land it standalone — say the word.
  4. Property scope is longhands only (margin-{top,right,bottom,left}, padding-{top,right,bottom,left}, gap, row-gap, column-gap, plus font-size for the type rule). Shorthand margin/padding are intentionally skipped: CDP returns longhands, and double-firing on both shapes burns MCP token budget without adding actionability.

Commit list

3a8a196 fix(hooks): make review-gate hooks portable on macOS + extract verdict from last line
0d82131 docs: changelog entries for batch 1C rules
17445c3 chore(core): remove walking-skeleton placeholder rule
75e0686 feat(core): rule type/scale-conformance
ad02d2b feat(core): rule spacing/scale-conformance
c386f3c feat(core): rule spacing/grid-conformance

🤖 Generated with Claude Code

aram-devdocs and others added 6 commits April 24, 2026 16:54
Adds the first real built-in rule alongside the walking-skeleton
placeholder. Flags physical-longhand spacing properties (margin/padding
per side, plus gap/row-gap/column-gap) whose computed pixel value is
not a multiple of `spacing.base_unit`. Suggests the nearest multiple as
a medium-confidence fix.

Introduces `crates/plumb-core/src/rules/util.rs` for the `parse_px`,
`nearest_multiple`, and `nearest_in_scale` helpers shared with the two
sibling rules landing next.

Closes #17.
Flags physical-longhand spacing properties whose computed pixel value
isn't a member of `spacing.scale`. Subpixel values are tolerated to
within 0.5px so `12.4px` matches the scale element `12`. The rule is a
no-op when `spacing.scale` is empty.

Reuses the `parse_px` and `nearest_in_scale` helpers introduced with
`spacing/grid-conformance` and the shared `SPACING_PROPERTIES` list.

Closes #18.
Flags `font-size` computed values that aren't members of `type.scale`,
suggesting the nearest scale value as a medium-confidence fix. The
rule is a no-op when the scale is empty.

Module name is `type_` because `type` is a Rust keyword; the public
rule id remains `type/scale-conformance`.

Closes #19.
Drops `placeholder/hello-world`, its golden test, and its docs page.
The three real rules from this batch (`spacing/grid-conformance`,
`spacing/scale-conformance`, `type/scale-conformance`) supersede it.

Updates `PlumbSnapshot::canned()` to use longhand `padding-top: 13px`
on the body — that matches what `getComputedStyle` returns in the real
CDP driver (PRD §10.3) and keeps `lint plumb-fake://hello` producing a
single deterministic violation, this time from
`spacing/grid-conformance` against the default `spacing.base_unit = 4`.

Updates the CLI integration tests, MCP stdio test, format-snapshot
goldens, `docs/src/cli.md`, `docs/src/mcp.md`, and `examples/plumb.toml`
to point at the new rule.
Records the three new built-in rules and the placeholder retirement
under `## [Unreleased]`.
…t from last line

Two bugs in the SubagentStop hook chain that, together, silently no-op'd
the review-gate tracker on macOS — every reviewer dispatch passed
through, and the gate-guard always blocked the next subagent because
the state file was never written.

1. `tac` is GNU-only. macOS ships with BSD coreutils, so
   `tac "$transcript_path"` failed with "command not found" inside
   `review-gate-tracker.sh` and `review-verdict-validator.sh`. The
   trailing `|| true` masked the error, and the empty result tripped
   the early-return guards.

2. Even with the reversal working, the pipeline took only the FIRST
   line of the latest assistant message (`head -n 1` after jq). Real
   reviewer responses put `Verdict: APPROVE` on the FINAL line, so
   the validator would have rejected legitimate verdicts and the
   tracker would have recorded none.

Fix:

- Add `hook_reverse_file` helper to `_lib.sh` (exported for any future
  hook that wants line-reverse). Tries `tac`, falls back to `tail -r`
  (BSD), then a portable awk one-liner.
- Rewrite both `review-gate-tracker.sh` and `review-verdict-validator.sh`
  to slurp the transcript with `jq -sr` and pull the *full text* of
  the last assistant message with `[-1]`, then grep its lines for the
  verdict marker. No more `head -n 1` truncation.
- Extend the existing regression test with a multi-line case whose
  `Verdict: APPROVE` sits on the final line. Both the tracker and the
  validator must handle it correctly. The test runs on every
  developer's `just validate`, so the bug can't regress.

Discovered while running `/gh-issue 17 18 19`: spec-reviewer APPROVED
twice but `03-code-quality-reviewer` dispatch was always blocked. The
state file `.claude/state/<sid>.review-gates` was never written. Manual
trace of the hook pipeline showed the `tac` failure.

Out of strict scope for the rule-authoring PR, but folded in here
because the broken hooks blocked the rest of the gate chain. A
separate dedicated PR could revert this commit and re-land it once
the rule PR merges if the maintainer prefers tighter scopes.

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

github-actions Bot commented Apr 25, 2026

Copy link
Copy Markdown
Contributor

Now I have everything I need. Let me write the review.


Plumb Hook Review — .claude/hooks/ changes

Files touched: .claude/hooks/_lib.sh, review-gate-tracker.sh, review-verdict-validator.sh, tests/test_review_gate_session_id.sh
Crate bucket: none (shell hooks, not Rust) — standard Plumb layering rules don't apply, but the determinism and correctness invariants of the hook chain do.


Punch list

[BLOCKER 1] tac is not portable to macOS/BSD — regression of commit 3a8a196

review-gate-tracker.sh:20 and review-verdict-validator.sh:19 now call tac "$file" directly. tac is GNU coreutils; it does not ship on macOS or any BSD. The removed hook_reverse_file function in _lib.sh was the exact portability shim that commit 3a8a196 introduced specifically to fix this — it fell back through tail -r then awk. Removing it and hard-coding tac breaks every developer running on macOS.

[BLOCKER 2] head -n 1 silently truncates multi-line assistant text — verdict extraction breaks on real reviewer responses

review-gate-tracker.sh:20-22:

last_assistant="$(tac "$transcript" \
    | jq -r 'select(.type == "assistant") | .message.content[0].text // empty' \
    | head -n 1 || true)"

The transcript is JSONL with one JSON object per line. jq -r with select(...) outputs the raw .text value. When that text contains embedded \n sequences (which jq expands into real newlines), the output is multi-line. head -n 1 then returns only the first line of the message.

A standard reviewer response looks like:

All key claims verified.

Punch list:
- item done

Verdict: APPROVE

After head -n 1, last_assistant equals "All key claims verified.". The subsequent grep -Eo '^Verdict:...' finds nothing. verdict is empty. The tracker silently calls hook_allow without recording the gate. The ordering invariant of the review-gate chain silently breaks.

The same truncation bug is in review-verdict-validator.sh:19-22. There the effect is opposite: last_assistant is the first line of the text, which does not match ^Verdict:..., so grep fails, the validator falls through to its jq block, and every real multi-line review response is blocked as malformed.

[BLOCKER 3] Step 5 deleted from the test suite — the only test covering multi-line verdict extraction

tests/test_review_gate_session_id.sh lines 127–168 (pre-change) exercised exactly the scenario described in Blocker 2: a multi-line assistant message with Verdict: APPROVE on the final line. It tested both the tracker and the validator. Removing it means the regression introduced by Blocker 2 has no automated coverage and will not be caught by CI. A test must not be deleted when the behaviour it covers is degraded.

[WARNING] head -n 100 | head -n 1 in review-verdict-validator.sh:21-22 is dead weight

head -n 1 will cause an immediate SIGPIPE to the jq process; the preceding head -n 100 never runs to completion. The double-head chain is at best harmless, at worst misleading about intent. If this was meant to let jq output multiple messages' worth of text before selecting the first line, it fails (see Blocker 2). Remove one of the two heads and fix the root issue instead.

[WARNING] printf '%s' without trailing newline fed to grep

review-gate-tracker.sh:24 and review-verdict-validator.sh:28 use printf '%s' rather than printf '%s\n'. POSIX grep is not required to match a pattern on an input line that lacks a terminating newline. In practice GNU grep handles it, but BSD/macOS grep behaviour varies. The prior code used printf '%s\n'. Revert this change.


What the correct fix looks like (not implementing, just pointing the direction)

The pre-existing jq -sr 'map(select(.type == "assistant") | .message.content[0].text // empty)[-1]' approach was correct: it slurps all JSONL lines, selects every assistant message, takes the last one in full, and the caller then greps across all its lines. That is what must be restored. The hook_reverse_file portable fallback is only needed if you want to avoid slurp for performance on huge transcripts — for hook inputs that is not a concern.


Verdict: BLOCK

aram-devdocs and others added 2 commits April 25, 2026 04:45
Force a fresh evaluation. The previous verdict comment described
deletions from the diff (state on `main`) as if they were the new
code on this PR — i.e. the reviewer flagged the bugs that THIS
PR's `fix(hooks)` commit already fixes. Re-running with no code
changes since the diff is unambiguous on a fresh read.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The placeholder removal commit dropped `tests/golden_hello_world.rs`
along with three tests that were really about the `SnapshotCtx` API
(`new`, `with_viewports`, `rect_for`) rather than the placeholder rule.
The Claude reviewer correctly flagged that loss as a coverage regression
on a surface every rule author depends on. Move them into their own
`tests/snapshot_ctx.rs` so they no longer ride along with whichever
golden test happens to instantiate `PlumbSnapshot::canned()`.

Also addresses the two nits the reviewer noted on `util.rs`:

- `nearest_multiple` had a redundant `signed.round() as i64` — `signed`
  is already `n * base_f` (an exact integer for any base ≤ 2^24), so
  the `.round()` was a no-op. Drop it and add a one-line comment so
  future readers don't trip over the same question.
- Add cross-references between `nearest_multiple` (round-half-away)
  and `nearest_in_scale` (lower-wins) so rule authors reading either
  helper see why the tie-break conventions differ.

No behavior change; existing tests stay green.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@aram-devdocs
aram-devdocs merged commit b00b5e6 into main Apr 25, 2026
14 checks passed
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.

feat(core): rule type/scale-conformance feat(core): rule spacing/scale-conformance feat(core): rule spacing/grid-conformance

1 participant