fix(mcp): subtract surface holes from measured area

[rootsbymenda]

What does this PR do?

measure with fromId === toId on a slab or ceiling reports the gross polygon area, ignoring
the node's holes. A 4 m × 4 m slab with a 2 m × 2 m opening measures as 16 m², not 12 m².

Repro (at 42ac4be): create a slab with polygon 4×4 and one 2×2 hole, then call
measure { fromId: <slabId>, toId: <slabId> } → areaSqMeters: 16.

Cause: the self-measurement branch calls shoelaceArea(n.polygon) and never reads n.holes
(packages/mcp/src/tools/measure.ts:108-116). Both schemas carry holes — SlabNode.holes
("cutouts in the slab", packages/core/src/schema/nodes/slab.ts:22-33) and CeilingNode.holes
(packages/core/src/schema/nodes/ceiling.ts:19).

Elsewhere the codebase treats surface area as hole-subtracted:

• wiki/architecture/measurements.md: "Slab reports expose hole-subtracted surface";
• core's build validation computes slab floor area as the outer polygon minus every declared hole,
  clamped at zero (packages/core/src/validation/validate-build-json.ts:257-268);
• deriveZoneQuantityReport subtracts holes from proven floor surface
  (packages/core/src/lib/zone-quantities.ts:441-460).

The MCP tool is the outlier — and it's the surface agents use for takeoff-style checks, so a
penetration or void silently inflates the reported area.

Fix (6 lines): in the self-measurement branch, subtract the summed shoelace area of the node's
declared holes from the outer polygon, clamped at zero — mirroring the build-validation convention
at validate-build-json.ts:257-268. Zones are unchanged (ZoneNode has no holes field), and the
distance path (fromId !== toId) is untouched. The tool description now says "net area" so the
contract states what the number is. One nuance flagged rather than hidden: deriveZoneQuantityReport
additionally classifies non-contained holes as unavailable before subtracting; measure here mirrors
the simpler build-validation form (subtract all declared holes, clamp at zero). If you'd rather
measure apply the containment filter too, happy to adjust.

How to test

1. bun test --cwd packages/mcp — new case in packages/mcp/src/tools/measure.test.ts:
   a solid 4×4 slab still measures 16; the same polygon with a 2×2 hole measures 12.
2. Manual repro: run the MCP server, create the holed slab, measure it self-to-self → 12.
3. The exact CI commands pass locally: bun test --cwd packages/mcp (298 pass / 0 fail) and the
   workflow's Biome check (141 files, no issues).

Screenshots / screen recording

N/A — non-visual change (headless MCP tool output).

Checklist

• I've tested this locally (exact MCP CI gates: bun test --cwd packages/mcp + the workflow's
  Biome command; plus a live headless MCP repro — bun dev UI not exercised, this change has no
  UI surface)
• My code follows the existing code style (bun check on the touched package is clean)
• I've updated relevant documentation (the tool's own description string; the wiki already
  documents the convention this aligns to)
• This PR targets the main branch

---

Note

Low Risk
Small behavioral fix in the MCP measure tool with tests; no auth, persistence, or UI surface.

Overview
The MCP measure tool now reports net polygon area for slabs and ceilings when fromId === toId, subtracting each declared hole’s shoelace area from the outer polygon and clamping at zero. Zones are unchanged (no holes field). The tool description now says “net area” instead of gross polygon area.

Tests cover a holed vs solid 4×4 slab (16 vs 12 m²) and legacy JSON loaded without a holes property (still uses the outer polygon only).

^{Reviewed by Cursor Bugbot for commit 6030a1e. Bugbot is set up for automated code reviews on this repo. Configure here.}

[cursor]

Cursor Bugbot has reviewed your changes using high effort and found 1 potential issue.

^{❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.}

Want reviews to match your repository better? Bugbot Learning can learn team-specific rules from PR activity. A team admin can enable Learning in the Cursor dashboard.

^{Reviewed by Cursor Bugbot for commit fe7bc5d. Configure here.}

[rootsbymenda]

Good catch — the schema defaults holes to [], but documents loaded without
running schema defaults can indeed carry no array, and core's own guard at
validate-build-json.ts:257-268 handles exactly that with Array.isArray. Fixed in
2bba13d mirroring that guard, plus a regression test (holed slab with its holes
array stripped post-load → falls back to the outer polygon, no crash). 299 pass,
Biome clean.

[Aymericr]

This is a good PR. The bug is real, the fix is four lines, and it matches the convention the rest of the codebase already uses — validate-build-json.ts and polygonSurfaceArea (behind the slab quick-measure HUD) both sum holes and clamp at zero, so this brings MCP into agreement with what the editor displays for the same surface. I reverted just the source change against your tests and confirmed the holed case fails at 16 instead of 12, so the test genuinely pins the fix rather than passing either way.

Also worth saying: your PR description is accurate, which is rarer than it should be. Every line reference checked out, the "ZoneNode has no holes field" claim is correct, and you volunteered the deriveZoneQuantityReport divergence instead of leaving me to find it. That is the right instinct and it made this fast to review.

One change before merging, pushed to your branch. The second test fabricated its condition with delete (bridge.getNodes()[slab.id] as {...}).holes. getNodes() is documented as a live reference into the store with "do NOT mutate" on it, so the test was reaching past the contract to build a state.

The state it was testing is real, though — I checked. loadJSON casts its nodes straight into setScene with no schema parse, so a document authored before holes existed arrives with the field genuinely absent and the zod default never runs. So I rewrote the test to load such a document, which exercises the actual path instead of simulating its end state. Same assertion, and it now fails for the right reason if the guard is removed.

That also settles the Array.isArray question in your favor — since loadJSON does not validate, holes could arrive as a non-array, and ?? [] would not catch that. Array.isArray is the correct read here even though sibling call sites in core use ?? [], because those sit behind a parse and this does not.

bun test packages/mcp/src  → 301 pass, 0 fail
bun run check-types        → 9 successful, 9 total
bun run check              → 1583 files, no fixes applied

Merging once CI confirms. Thanks for this.

[rootsbymenda]

This is a good PR. The bug is real, the fix is four lines, and it matches the convention the rest of the codebase already uses — validate-build-json.ts and polygonSurfaceArea (behind the slab quick-measure HUD) both sum holes and clamp at zero, so this brings MCP into agreement with what the editor displays for the same surface. I reverted just the source change against your tests and confirmed the holed case fails at 16 instead of 12, so the test genuinely pins the fix rather than passing either way.

Also worth saying: your PR description is accurate, which is rarer than it should be. Every line reference checked out, the "ZoneNode has no holes field" claim is correct, and you volunteered the deriveZoneQuantityReport divergence instead of leaving me to find it. That is the right instinct and it made this fast to review.

One change before merging, pushed to your branch. The second test fabricated its condition with delete (bridge.getNodes()[slab.id] as {...}).holes. getNodes() is documented as a live reference into the store with "do NOT mutate" on it, so the test was reaching past the contract to build a state.

The state it was testing is real, though — I checked. loadJSON casts its nodes straight into setScene with no schema parse, so a document authored before holes existed arrives with the field genuinely absent and the zod default never runs. So I rewrote the test to load such a document, which exercises the actual path instead of simulating its end state. Same assertion, and it now fails for the right reason if the guard is removed.

That also settles the Array.isArray question in your favor — since loadJSON does not validate, holes could arrive as a non-array, and ?? [] would not catch that. Array.isArray is the correct read here even though sibling call sites in core use ?? [], because those sit behind a parse and this does not.

bun test packages/mcp/src  → 301 pass, 0 fail
bun run check-types        → 9 successful, 9 total
bun run check              → 1583 files, no fixes applied

Merging once CI confirms. Thanks for this.

Glad this landed, and I appreciated the thorough review — the loadJSON test rewrite was a genuinely better construction.
One thing for the record: the branch rewrite before merge replaced the commit author, so the fix now shows as authored by you rather than by me (the Co-Authored-By trailer from my commit was kept, my author field was not). Was that intentional? You wrote that volunteering a divergence instead of leaving the reviewer to find it is the right instinct — same instinct here: for future contributions I'd want the author field preserved, or a Co-Authored-By credit on the rewritten commit.