fix(autofix): stop the write path mutating existing content (follow-on to #13)

[floodwayprintco]

Follow-on to #13 / v1.2.4. That release made saves of existing content non-destructive on the
page path; three mutations were still running elsewhere. All three were surfaced only as
Auto-fixed N issue(s), which reads like a courtesy rather than a rewrite. Found on a live site
when a read-back returned content nobody had sent.

1. bricks_update_template still normalized

tools/templates.js called autofix(content) with no mode, so it defaulted to 'normalize' and
Pass 5 rewrote el.name 'block' → 'container' on any element carrying a flex property. That's
a structural change to a layout a human authored. Observed on one template: three blocks silently
promoted to containers.

06d34cd fixed exactly this for pages (tools/pages.js passes mode:'preserve') and the template
writer was left normalizing. Now it passes mode:'preserve' too.

bricks_create_template still normalizes, deliberately — content the tool generates itself is
what the aggressive repairs are for.

2. stripPxValues rewrote human-authored units on preserve saves

PX_SAFE_KEYS doesn't cover nested keys such as icon.height, so a preserve save turned "20px"
into "20". Bricks itself writes "20px" there — every icon in a stock header template stores it
that way — so on existing content the strip is a deviation, not a fix.

Pass 1 is now normalize-only. Generated content is unaffected.

3. BUILTIN_CSS_FIXES injected classes that may not exist on the target site

These add design-system global classes (ds-section-md, ds-gap-md) plus !important CSS to
content the caller never asked to restyle. On a site built with that design system they're a good
default. On any other site the classes resolve to nothing, and because the only report is
Auto-fixed N issue(s), nobody notices until it shows up in the browser.

Now behind BRICKS_MCP_BUILTIN_FIXES=1. Flagged rather than deleted, so it's one env var away.

⚠️ Point 3 changes a default, and it's the one worth arguing about

Points 1 and 2 only affect saves of existing content and leave generated content normalized
exactly as before, so I'd expect those to be uncontroversial. Point 3 flips a default for
everyone.

Happy to reshape it however you'd rather have it:

• invert to an opt-out (BRICKS_MCP_BUILTIN_FIXES=0), keeping today's default
• gate it on whether the classes actually exist on the target site, which is the real
  precondition and would make it safe on by default
• split it into its own PR so points 1 and 2 can land without waiting on the discussion

Say which and I'll push it.

Verification

Found and confirmed against a live Bricks site: a template save that reported Auto-fixed 3 issue(s) and returned three block elements as container on read-back, and an icon whose
"20px" came back "20". Both stop after the change; a read-back returns exactly what was sent.

node --check passes on both changed files. I didn't see an automated test suite to extend —
tests/test-suite.json looks like inspector fixtures rather than a runner. Point me at one if
there is.

Branched off main, 44 insertions across two files.

[developer2013]

Thanks for these — the measurements made all three PRs fast to verify. #11, #15 and #14 are merged; this one I'd like reshaped before it lands.

Point 1 is right and lands as-is. It's exactly what 06d34cd should have covered when it fixed the page writer; the template writer was simply left behind.

Point 2 I've confirmed, but the fix is broader than the bug. The icon.height recursion is real: icon is in PX_SAFE_KEYS, but stripPxValues passes the immediate key down, so for settings.icon = { height: "20px" } the walker sees currentKey = 'height' and the allowlist never applies. Nested values were never protected.

The problem is what disabling Pass 1 on preserve costs. preserve is used at exactly one call site — tools/pages.js:155, bricks_update_page — and that path is not only for saving content that already exists. It's also the primary push path for freshly generated element arrays (a full page build is a single PUT there). Bricks stores _padding / _margin unitless and treats the bare number as px, so a generated _padding: { top: "60px" } used to be corrected silently and would now go through raw. That trades a rare over-reach for a common footgun.

Preferred shape, either of:

• make the allowlist path-aware (icon.height, icon.width, …), or
• invert Pass 1 to an allowlist of the keys that actually need stripping (the spacing controls),

and keep it running in preserve mode either way.

Point 3: taking the opt-in, as you proposed it. For a tool that runs on arbitrary sites, injecting ds-section-md / ds-gap-md plus !important into content the caller never asked to restyle isn't defensible as a default, and Auto-fixed N issue(s) doesn't tell anyone it happened. BRICKS_MCP_BUILTIN_FIXES=1 is the right shape — no need for the existence check or the opt-out inversion.

So: please split. Points 1 + 3 in one PR and I'll merge it; point 2 as its own with the path-aware (or inverted) allowlist.

On your two side notes:

• There's no test runner — you read tests/test-suite.json correctly, those are inspector fixtures. node --check plus the manual verification you did is the current bar. Adding a real runner is on me, not on you.
• The bricks_patch_page reporting bug on header/footer templates without content_area (reports success against zero matched ids): agreed that's separate, and worth its own issue if you want to file it.