Skip to content

fix: gate built assets and guard generators against silent drift - #176

Merged
jackgranatowski merged 2 commits into
mainfrom
claude/codebase-audit-vendoring-security-i9592s
Jul 8, 2026
Merged

jackgranatowski merged 2 commits into
mainfrom
claude/codebase-audit-vendoring-security-i9592s

Conversation

@jackgranatowski

@jackgranatowski jackgranatowski commented Jul 8, 2026

Copy link
Copy Markdown
Contributor

Summary

Closes the "green CI, stale/incorrect artifact" gaps found in a full-tree audit. The theme is silent drift: places where a source change (or a missing input) produced a wrong committed artifact without anything failing.

  • CI — drift gate on built assets (ci.yml): assets/admin-app/ and integrations/bricks/assets/editor-app/ are committed, runtime-loaded bundles. CI built them but never diffed against the committed copies, so editing app source (or a framework sync) without a rebuild shipped stale JS silently. Both build jobs now fail via git status --porcelain on the artifact dirs (.map files are gitignored, so no false positives).
  • CI — release rebuilds editor-app (release.yml): the release job rebuilt only admin-app, so a release could ship a stale editor bundle. Added the Build editor-app step.
  • gen-class-hints guard (scripts/gen-class-hints.js): running the writer without a framework checkout silently overwrote classes-hints.json with a truncated (manual/override-only) map. It now exits non-zero instead of clobbering. (--check mode is unchanged — it still reports drift loudly.)
  • Color-math golden tests (tests-php/ColorMathTest.php): the PHP OKLCH↔hex math (swatch previews) had no test and could drift from its JS counterpart. Added golden oklch_to_hex / parse_oklch / hex_to_oklch values plus hex→oklch→hex round-trips.
  • Duplication guards: tests/color-model-copies-identical.test.js asserts the two intentionally-identical color-model.js copies stay byte-identical (only classifyVar behaviour was previously guarded); tests/element-types-mirror.test.js asserts PHP BUILTIN_DEFAULTS mirrors JS ELEMENT_TYPE_LABEL_MAP.
  • Docs/comments: corrected the stale .syncignore description in sync-core.mjs and the classes-hints.json filename in CLAUDE.md; added .nvmrc (node 22, matching CI).

Type

  • fix
  • feat
  • docs
  • chore / tooling

Checklist

  • Conventional Commit messages
  • npm test passes (144/144, incl. 3 new guard tests)
  • npm run lint:php passes (php -l, 34 files)
  • npm run verify passes
  • Generated artifacts not hand-edited
  • No SPA source changed, so no asset rebuild needed (this PR adds the gate that enforces it going forward)

Notes

  • ColorMathTest.php couldn't be executed here (composer install can't reach github.com through the sandbox proxy) — it will run under the CI quality job's composer phpunit. Golden values and round-trips were validated with a standalone PHP harness against the current Slashed_Color_Math.
  • Deliberately not included: flipping phpcs.xml.dist ignore_warnings_on_exit=1 → 0. That would very likely turn CI red on pre-existing WPCS warnings, and phpcs/phpstan can't be run in this environment to see/fix them first. Best done in a follow-up where the tooling runs.

🤖 Generated with Claude Code

https://claude.ai/code/session_01SRP2W4xhP46TZg5Pbnwxob


Generated by Claude Code

Summary by CodeRabbit

  • New Features

    • Added checks that prevent releases when bundled frontend assets are out of date.
    • Release builds now refresh the editor bundle before packaging.
  • Bug Fixes

    • Reduced the risk of shipping stale or partially generated frontend assets.
  • Tests

    • Added validation to keep related color, element-type, and editor bundle mappings in sync.
  • Chores

    • Updated the supported Node.js version and related project documentation.

Closes the "green CI, stale artifact" gaps found in the codebase audit:

- ci: fail the PR when the committed admin-app / editor-app bundles
  (assets/admin-app, integrations/bricks/assets/editor-app) don't match a
  fresh build. These are committed, runtime-loaded artifacts that CI built
  but never diffed, so a source change without a rebuild shipped silently.
- ci(release): rebuild editor-app in the release job (only admin-app was
  rebuilt), so releases can't ship a stale editor bundle.
- fix(gen-class-hints): refuse to overwrite classes-hints.json when the
  framework CSS source is absent, instead of silently writing a truncated
  (manual/override-only) map over the committed file.
- test(color-math): golden OKLCH<->hex + round-trip PHPUnit coverage so the
  PHP swatch math can't drift from its JS counterpart unnoticed.
- test(dupes): assert the two byte-identical color-model.js copies stay
  identical, and that PHP BUILTIN_DEFAULTS mirrors JS ELEMENT_TYPE_LABEL_MAP.
- docs/comments: correct the stale .syncignore description in sync-core.mjs
  and the classes-hints.json filename in CLAUDE.md; add .nvmrc (node 22).

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01SRP2W4xhP46TZg5Pbnwxob
@coderabbitai

coderabbitai Bot commented Jul 8, 2026

Copy link
Copy Markdown

Review Change Stack

Warning

Review limit reached

@jackgranatowski, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 51 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews.

How do review limits work?

CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability.

For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window.

Please refer docs for additional details.

Review details
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: b6541edd-a259-4c5b-a270-419dd5e848db

📥 Commits

Reviewing files that changed from the base of the PR and between bcdd935 and 903c0bd.

📒 Files selected for processing (2)
  • package.json
  • scripts/gen-class-hints.js
📝 Walkthrough

Walkthrough

This PR adds CI/release workflow guards that fail builds when committed editor-app or admin-app bundles are stale, pins Node.js to version 22 via .nvmrc, adds a safeguard against generating truncated classes-hints.json, and introduces PHPUnit and Node tests for color-math conversions and PHP/JS mirror consistency, alongside minor doc updates.

Changes

Build drift guards and mirror tests

Layer / File(s) Summary
CI drift checks for committed bundles
.github/workflows/ci.yml, .github/workflows/release.yml
CI now fails if built editor-app/admin-app bundles differ from committed versions; release workflow rebuilds editor-app before packaging.
Class-hints generation safeguard
scripts/gen-class-hints.js, CLAUDE.md
Adds frameworkSourcePresent() check to prevent overwriting classes-hints.json when framework sources are missing; renames referenced file in docs.
Color math test suite and bootstrap wiring
tests-php/ColorMathTest.php, tests-php/bootstrap.php
Adds golden/round-trip PHPUnit tests for OKLCH↔hex conversion and loads the class-color-math.php dependency in the test bootstrap.
JS/PHP mirror drift tests
tests/color-model-copies-identical.test.js, tests/element-types-mirror.test.js
Adds tests verifying two color-model.js copies are identical and that PHP BUILTIN_DEFAULTS matches JS ELEMENT_TYPE_LABEL_MAP exactly.
Node version pin and sync docs
.nvmrc, SLASHED-for-WP/admin-app/scripts/sync-core.mjs
Pins Node.js to version 22 and clarifies sync-core.mjs documentation on .syncignore and CI drift checks.

Estimated code review effort: 2 (Simple) | ~15 minutes

Possibly related PRs

Suggested labels: codex

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly summarizes the main change: preventing silent drift by gating built assets and generator outputs.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch claude/codebase-audit-vendoring-security-i9592s

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@coderabbitai coderabbitai Bot added the codex label Jul 8, 2026
@qodo-code-review

Copy link
Copy Markdown

PR Summary by Qodo

Gate committed bundles and prevent silent drift in CI/generators

🐞 Bug fix 🧪 Tests ⚙️ Configuration changes 📝 Documentation 🕐 40+ Minutes

Grey Divider

AI Description

• Fail CI if committed admin/editor bundles differ from a fresh build.
• Rebuild editor-app during releases to avoid shipping stale assets.
• Add generator guards and cross-language golden/duplication tests to prevent drift.
Diagram

graph TD
  CI["CI workflow"] --> BUILD{{"Build SPAs"}} --> GATE{{"Drift gate"}} --> ART[("Committed artifacts")]
  REL["Release workflow"] --> BUILD
  FWSRC[("Framework CSS source")] --> GEN{{"gen-class-hints"}} --> ART
  CI --> TESTS["Golden/duplication tests"]
  subgraph Legend
    direction LR
    _wf["Workflow"] ~~~ _step{{"Step/Script"}} ~~~ _art[("Artifact")]
  end
Loading
High-Level Assessment

The following are alternative approaches to this PR:

1. Stop committing built SPA bundles (build at release/install time)
  • ➕ Eliminates “committed artifact drift” class of problems entirely
  • ➕ Reduces repo churn from generated JS/CSS changes
  • ➖ Often infeasible for WP plugin distribution where assets must ship prebuilt
  • ➖ Moves complexity to packaging/install, potentially harming reliability
2. Artifact checksum manifest instead of git-status gating
  • ➕ Explicit list of expected artifact outputs; can be validated without relying on git status semantics
  • ➕ Can enforce coverage of specific files (e.g., app.js/app.css)
  • ➖ Yet another generated artifact that must be maintained
  • ➖ Less direct than diffing the real committed directories after a build
3. Separate “verify-artifacts” job using git diff --exit-code on specific files
  • ➕ More targeted failure surface (only specific files)
  • ➕ Can provide clearer diffs for reviewers
  • ➖ Higher maintenance: must keep file list in sync as build outputs change
  • ➖ Current directory-based check is simpler and already benefits from ignored .map files

Recommendation: Keep the current approach. Given these bundles are intentionally committed and runtime-loaded, building them in CI and failing on a dirty tree is the most direct, low-maintenance way to prevent shipping stale artifacts. The added generator guard complements this by preventing accidental truncation when the upstream framework source is missing, and the new golden/mirror tests cover the remaining “silent drift” vectors that CI builds alone cannot detect.

Files changed (10) +275 / -7

Bug fix (1) +25 / -0
gen-class-hints.jsRefuse to overwrite class hints without framework source present +25/-0

Refuse to overwrite class hints without framework source present

• Adds a framework-source presence check so the write mode exits non-zero when framework CSS files are missing. Prevents silently clobbering the committed 'classes-hints.json' with a truncated manual/override-only map; '--check' mode remains drift-reporting only.

scripts/gen-class-hints.js

Tests (4) +200 / -0
ColorMathTest.phpAdd golden regression tests for OKLCH ⇄ hex color math +96/-0

Add golden regression tests for OKLCH ⇄ hex color math

• Introduces PHPUnit golden-value tests for 'oklch_to_hex', 'hex_to_oklch', and 'parse_oklch', plus hex→oklch→hex round-trip checks. Guards against silent drift between PHP and JS color conversion implementations used for swatch previews.

tests-php/ColorMathTest.php

bootstrap.phpLoad color math class for PHPUnit +1/-0

Load color math class for PHPUnit

• Adds 'class-color-math.php' to the PHPUnit bootstrap includes so the new ColorMath tests can run.

tests-php/bootstrap.php

color-model-copies-identical.test.jsAssert Bricks/Gutenberg color-model.js copies are byte-identical +43/-0

Assert Bricks/Gutenberg color-model.js copies are byte-identical

• Adds a Node test that reads both copies of 'color-model.js' and asserts exact byte equality. Prevents unintentional divergence in duplicated logic beyond the previously behavior-only guard.

tests/color-model-copies-identical.test.js

element-types-mirror.test.jsVerify PHP BUILTIN_DEFAULTS mirrors JS ELEMENT_TYPE_LABEL_MAP +60/-0

Verify PHP BUILTIN_DEFAULTS mirrors JS ELEMENT_TYPE_LABEL_MAP

• Adds a Node test that parses the PHP 'BUILTIN_DEFAULTS' const array and deep-compares it to the JS element-type label map. Turns a “KEEP IN SYNC” comment into an enforced contract.

tests/element-types-mirror.test.js

Documentation (2) +11 / -7
CLAUDE.mdFix generated hints filename references +2/-2

Fix generated hints filename references

• Updates documentation to reference 'classes-hints.json' (plural) instead of the stale 'class-hints.json' path. Keeps developer docs consistent with the actual generated artifact names.

CLAUDE.md

sync-core.mjsCorrect sync/.syncignore guidance in comments +9/-5

Correct sync/.syncignore guidance in comments

• Rewrites the header comment to accurately describe how '.syncignore' preserves the small set of intended divergences and how CI drift checks enforce upstream sync. No functional behavior changes.

SLASHED-for-WP/admin-app/scripts/sync-core.mjs

Other (3) +39 / -0
ci.ymlFail CI when committed SPA bundles are stale after build +31/-0

Fail CI when committed SPA bundles are stale after build

• Adds post-build drift gates for editor-app and admin-app artifact directories. After building, CI checks 'git status --porcelain' for changes under the committed bundle paths and fails with a clear remediation message and diff when outputs are out of date.

.github/workflows/ci.yml

release.ymlRebuild editor-app during release packaging +7/-0

Rebuild editor-app during release packaging

• Ensures the release workflow rebuilds the editor-app bundle in addition to admin-app. Prevents releases from shipping a stale editor bundle when only admin-app was rebuilt previously.

.github/workflows/release.yml

.nvmrcPin Node.js version to 22 +1/-0

Pin Node.js version to 22

• Adds an .nvmrc specifying Node 22 to align local development with CI expectations.

.nvmrc

@qodo-code-review

qodo-code-review Bot commented Jul 8, 2026

Copy link
Copy Markdown

Code Review by Qodo

🐞 Bugs (0) 📘 Rule violations (0) 📜 Skill insights (0)

Context used
✅ Compliance rules (platform): 8 rules

Grey Divider


Action required

1. Release editor-app deps missing ✓ Resolved 🐞 Bug ☼ Reliability
Description
The release workflow runs npm run build:editor-app but never installs the editor-app package’s
dependencies, so the release job can fail due to missing Vite/Svelte toolchain in that subproject.
Root npm ci does not install dependencies for SLASHED-for-WP/integrations/bricks/editor-app
because it’s not configured as an npm workspace.
Code

.github/workflows/release.yml[R55-61]

+      # Rebuild BOTH bundled Svelte apps so the release ships freshly-built
+      # artifacts rather than whatever was last committed. The admin-app build
+      # re-vendors the configurator from the framework (needs GITHUB_TOKEN);
+      # the editor-app build is self-contained.
+      - name: Build editor-app
+        run: npm run build:editor-app
+
Relevance

⭐⭐⭐ High

Team repeatedly hardens release.yml to prevent release-time failures/drift (e.g., preserve/build
assets fixes in PRs 122, 74).

PR-#122
PR-#74
PR-#29

ⓘ Recommendations generated based on similar findings in past PRs

Evidence
Release adds a Build editor-app step, but the only install in the workflow is npm ci at repo
root; the root build:editor-app script doesn’t install editor-app dependencies. The CI editor-app
build job demonstrates the required npm ci --prefix ... step before running the build.

.github/workflows/release.yml[55-65]
package.json[16-27]
.github/workflows/ci.yml[38-57]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
`release.yml` invokes `npm run build:editor-app`, but the root `build:editor-app` script only runs the build inside the subpackage and does not run `npm ci` there. Since the repo root is not an npm workspace, dependencies for `SLASHED-for-WP/integrations/bricks/editor-app` are not installed by the earlier root `npm ci`, and the release build can fail.

### Issue Context
- `npm run build:editor-app` maps to `npm --prefix ... run build` (no install step).
- CI’s dedicated editor-app build job installs deps explicitly, which release should mirror.

### Fix Focus Areas
- .github/workflows/release.yml[43-66]
- package.json[16-27]

### Suggested fix
Add an explicit install step before the editor-app build in `release.yml`, e.g.:
- `npm ci --prefix SLASHED-for-WP/integrations/bricks/editor-app`
then run `npm run build:editor-app`.

(Alternative: change `package.json` `build:editor-app` to perform `npm ci --prefix ...` before the build, aligning it with `build:admin-app`.)

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools



Remediation recommended

2. Partial hints overwrite risk ✓ Resolved 🐞 Bug ≡ Correctness
Description
gen-class-hints.js only checks that at least one framework CSS source file exists before writing,
but missing source files are silently treated as empty, so a partial/mispointed framework checkout
can overwrite classes-hints.json with an incomplete hints map. This preserves the “no-framework”
clobber protection but still allows “incomplete framework” clobbering.
Code

scripts/gen-class-hints.js[R175-186]

+/**
+ * True when at least one of the framework CSS source files is present on disk.
+ * When the framework checkout is missing, parseFile() returns {} for every
+ * source and generate() would produce a hints map containing only the
+ * hardcoded MANUAL_HINTS + OVERRIDE_HINTS — a small fraction of the real set.
+ * Writing that would silently clobber the committed classes-hints.json, so the
+ * writer below refuses to run without a framework source. (--check mode still
+ * runs generate() and reports the resulting drift loudly, which is safe.)
+ */
+function frameworkSourcePresent() {
+  return SOURCE_FILES.some(({ file }) => fs.existsSync(path.join(FRAMEWORK, file)));
+}
Relevance

⭐⭐⭐ High

Repo prefers fail-fast guards against missing/partial inputs for generators (accepted reliability
checks in PRs 175, 50, 49).

PR-#175
PR-#50
PR-#49

ⓘ Recommendations generated based on similar findings in past PRs

Evidence
The generator silently skips missing CSS sources (parseFile returns {}), and the new guard only
asserts at least one source exists (some(...)). Writer mode then proceeds to write the generated
JSON if that weak condition is met, enabling incomplete output to overwrite the committed hints file
in partial-checkout scenarios.

scripts/gen-class-hints.js[121-123]
scripts/gen-class-hints.js[175-186]
scripts/gen-class-hints.js[219-234]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
The writer-mode guard in `scripts/gen-class-hints.js` only verifies that *some* framework source file exists. Because `parseFile()` returns `{}` when a source file is missing, an incomplete framework checkout (or incorrect `SLASHED_FRAMEWORK_DIR` pointing at the wrong folder that happens to contain one expected file) can still produce a truncated output and overwrite the committed `classes-hints.json`.

### Issue Context
- `parseFile()` silently returns `{}` when the source file doesn’t exist.
- `frameworkSourcePresent()` uses `SOURCE_FILES.some(...)`, so it passes even if most inputs are missing.

### Fix Focus Areas
- scripts/gen-class-hints.js[121-123]
- scripts/gen-class-hints.js[175-186]
- scripts/gen-class-hints.js[219-234]

### Suggested fix
Replace the `some(...)` check with an `every(...)` check (or compute and report the list of missing files) and exit non-zero if any required `SOURCE_FILES` entry is absent before writing. Optionally log missing filenames to make misconfiguration obvious.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


Grey Divider

Qodo Logo

Comment thread .github/workflows/release.yml
Comment thread scripts/gen-class-hints.js
…int sources

Addresses two bugs found in review of #176:

- release.yml called `npm run build:editor-app`, but that root script only
  built the subpackage — it never installed its deps, and the repo isn't an
  npm workspace so root `npm ci` doesn't reach it, so the release build could
  fail on a missing Vite/Svelte toolchain. Make `build:editor-app` self-install
  (`cd … && npm ci && npm run build`), mirroring `build:admin-app`, so both
  apps behave identically in release and local full builds.
- gen-class-hints write guard used `SOURCE_FILES.some(...)`, so a partial or
  mispointed framework checkout (only some CSS sources present) still passed
  and could overwrite classes-hints.json with a truncated map. Require EVERY
  source to resolve and list the missing files, matching gen-bricks-inventory's
  fail-on-any-missing stance.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01SRP2W4xhP46TZg5Pbnwxob
@jackgranatowski
jackgranatowski merged commit 5422d25 into main Jul 8, 2026
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants