investigate(#597): the two remaining bridge-side sites cannot be fixed by rejecting on error

CLAUDE.md

@@ -269,6 +269,7 @@ suite into these targets (each `Tests/OCCT<Domain>Tests/`, declared in `Package.
 - `ShapeUpgrade_UnifySameDomain::IntUnifyFaces` (backs `UnifySameDomainBuilder.build()`) SIGSEGV'd (Address 0, uncatchable in-process) on a real mesh-sewn solid — found via OCCTReconstruct#194, minimized to a standalone, deterministic OCCTSwift-only reproducer. `IntUnifyFaces` (and its file-local `SplitWire` helper) disambiguate between multiple candidate next-edges at a branching vertex by comparing each candidate's pcurve tangent direction on the current reference face; three call sites fetch that pcurve via `BRep_Tool::CurveOnSurface(edge, refFace, first, last)` and dereference it immediately (`->D1(...)`/`->Value(...)`) with no `IsNull()` check — unlike every other `CurveOnSurface` call site in the same file, which do check. `CurveOnSurface` legitimately returns a null handle when an edge has no pcurve on the given face — routine for a raw mesh-sewn solid at a vertex shared by more than two edges. Confirmed via a debug (`-g -O0`) single-TU override-link + `lldb bt`: resolves precisely to `ShapeUpgrade_UnifySameDomain.cxx:4003` (`aPCurve->D1(...)`), reached via `IntUnifyFaces` → `UnifyFaces` → `Build`. **Fixed in v1.15.8** (`Scripts/patches/0013-*`, xcframework rebuilt): all five unguarded sites (three in `IntUnifyFaces`, two in `SplitWire`) now guard with `IsNull()`, following the file's own established pattern — a missing pcurve on a candidate edge means "skip it, not a rankable direction"; a missing pcurve on the current edge falls back to treating all candidates as equally likely, same as the existing single-candidate shortcut. Reproducer at [`Scripts/repro/348-unify-null-pcurve`](https://github.com/SecondMouseAU/OCCTSwift/tree/main/Scripts/repro/348-unify-null-pcurve); filed upstream as [OCCT#1391](https://github.com/Open-Cascade-SAS/OCCT/issues/1391) (repro) / [OCCT#1392](https://github.com/Open-Cascade-SAS/OCCT/pull/1392) (fix). #348. **Shipped upstream in OCCT 8.0.1** ([OCCT#1392](https://github.com/Open-Cascade-SAS/OCCT/pull/1392)); patch `0013` retired. 8.0.1 also guards three further null-pcurve sites in the same file and makes `RelocatePCurvesToNewUorigin` return `bool`, so unify can now decline a relocation instead of proceeding with a partial one.
 - `GeomTools_Curve2dSet::Add`/`GeomTools_SurfaceSet::Add` accept a null handle and defer the crash to `Write()`, unlike the `GeomTools_CurveSet` sibling: **#643, closed on evidence, no bridge fix needed.** `GeomTools_CurveSet::Add` guards (`return (C.IsNull()) ? 0 : myMap.Add(C);`); the other two don't (`return myMap.Add(S);`), so a null is bound at index 1 and only crashes later, inside `Write()` (`PrintCurve2d`/`PrintSurface` → `->DynamicType()`). `Index()` has the identical divergence one function down: no crash (`FindIndex` never dereferences), but a bogus non-zero index where `CurveSet::Index` correctly answers 0. Cluster C's census (#666, PR #711) found `OCCTGeomToolsCurve2dSetWrite`/`OCCTGeomToolsSurfaceSetWrite` (`OCCTBridge_IO.mm`) already guard every array element before `Add()` (the #618 "array element through a cast" shape) and are the only call sites of either class in the tree; re-verified rather than inherited, both statically (grep confirms one call site each, both guarded) and dynamically (override-linking the real bridge functions against a genuinely null-wrapping struct: refused, not crashed, for a null-only array and a mixed valid+null array). **Upstream asymmetry confirmed still live at the pinned kernel** (`v2.0.0-kernel.1`, re-measured directly, not assumed from the original 8.0.0p1 report) and byte-identical on current upstream `master`. **Fixed upstream, kernel patch carried** (`Scripts/patches/0023-*`, not yet in a rebuilt xcframework, tracked in #512): the same one-line guard `CurveSet::Add`/`Index` already have, applied to `Add`/`Index` on both siblings. Verified by override-linking the two patched files ahead of the unpatched archive: all three classes' `Add()` return 0 for a null handle and `Write()` completes normally; all three classes' `Index()` return 0. Filed upstream as [OCCT#1434](https://github.com/Open-Cascade-SAS/OCCT/issues/1434) (repro) / [OCCT#1435](https://github.com/Open-Cascade-SAS/OCCT/pull/1435) (fix). See [`Scripts/repro/643-geomtools-null-write/`](https://github.com/SecondMouseAU/OCCTSwift/tree/main/Scripts/repro/643-geomtools-null-write) for the full writeup and injection matrix. #643.
 - `GeomFill_Sweep::BuildAll` overwrites the measured C1-conversion error with the requested tolerance: **#597, the kernel half** (the bridge half, #741/#751, closed separately, see below). `SError` is set to the real measured error at `GeomFill_Sweep.cxx:286` (`Approx.MaxErrorOnSurf()`), but when `ForceApproxC1` is set and the swept surface isn't already C1 in V, the class re-approximates through `GeomConvert_ApproxSurface(mySurface, theTol, ...)` (`theTol` a literal `1.e-4`) and, on `HasResult()` (documented as true even for a result "not NECESSARILY within the required tolerance"), finishes with `SError = theTol;` instead of reading `MaxError()`, which reports what the conversion actually achieved and sits two lines above, unread. `BRepFill_Sweep`/`BRepFill_PipeShell`/`BRepOffsetAPI_MakePipeShell::ErrorOnSurface()` all forward `SError` verbatim, so `SetForceApproxC1(true)`, a public, documented API, hands every caller a number describing the request, not the result. **Reaching the branch needs a spine whose tangent discontinuity sits inside one edge**, not at a vertex: `BRepFill_Sweep` splits at spine vertices, so a polyline spine never gets there. The fixture (from #572, pinned by `Issue572SweepApproxTests.swift`) is a single-edge degree-2 B-spline spine with an interior knot of multiplicity 2, swept with a circle profile. **Checked, not assumed, that `MaxError()` is the right quantity**: unlike `GeomPlate_MakeApprox::ApproxError()` (the #571 trap, which measures an *intermediate* object and broke 6/6 real tests when gated on), `GeomConvert_ApproxSurface`'s `Surf` argument here *is* `mySurface`, the exact surface being replaced, confirmed by reconstructing the identical call from outside the kernel and finding its output's degree/pole counts and measured deviations bit-identical to the real forced build's. Also confirmed the number **moves**: patch `0019` (#522) is what makes this possible, since before it every interior truncation error was structurally zero; measured `MaxError() = 2.54714` against the pinned kernel, matching #572's own independent measurement of the same fixture to the printed digit, 25000x the `0.0001` the stock code reports. **Fixed** (`Scripts/patches/0025-*`, override-link validated, not yet in a rebuilt xcframework): `SError = ConvertApprox.MaxError();`, one line; the four `CError` literal `0.` entries a few lines above are left untouched rather than fabricated (see #726). Validation confirms this is diagnostic-only: every geometry value the reproducer prints (degree/pole/knot counts, two independent geometric deviations, same-parameter and nearest-point) is byte-identical before and after, since `mySurface` is already `ConvertApprox.Surface()` two statements earlier; no bridge site gates on this number today (`PipeShellBuilder.errorOnSurface` is info-only, and `OCCTGeomFillSweep`'s own error gate from #741 never sets `ForceApproxC1` so never reaches this branch), so `swift test` is unaffected. See [`Scripts/repro/597-geomfill-sweep-error-overwrite/`](https://github.com/SecondMouseAU/OCCTSwift/tree/main/Scripts/repro/597-geomfill-sweep-error-overwrite) for the full writeup. Upstream PR drafted but not sent (`draft-pr.md` in that directory), per `okf/policies/upstream-occt-style.md`. #597.
+- **`GeomPlate_MakeApprox::ApproxError()` and `BRepOffsetAPI_MakeFilling::G0Error()` look like the obvious gate for "accepted an approximation without reading its error" (Cluster E, #668), and are not: investigated and rejected on measurement, twice, for two different reasons. This is the bridge half of #597** (the kernel half is immediately above). `occtPlateApproxSurface` (`OCCTBridge_Internal.h`, backs `OCCTShapePlatePoints`/`OCCTShapePlateCurves` + 4 more entry points) and `OCCTShapeFillBuildResult` (`OCCTBridge_Healing.mm`, backs `OCCTShapeFill`/`OCCTShapeFillWithSupport`/`OCCTShapeFillConstraints`) both run an approximation and never check the error it reports, the same shape #741 fixed for `OCCTGeomFillSweep`. Both candidate fixes (`if (approx.ApproxError() > tolerance) return null;` / `if (filling.G0Error() > tolerance) return null;`) were built and run against the real bridge, not just reasoned about, and both broke real, already-shipped, already-tested behaviour. `GeomPlate_MakeApprox::ApproxError()`'s own header doc gives away why the plate site is unfixable this way: it measures distance between the **fitted BSpline and the intermediate `GeomPlate_Surface`** the caller never sees, not fidelity to the caller's own input points. On the #571 fixture it exceeds `tolerance` by up to 5.8x while the deviation from the caller's own 25 input points stays inside tolerance throughout, and gating on it failed all 6 `Issue571PlateApproxTests`. `BRepOffsetAPI_MakeFilling::G0Error()` is the right kind of number for the fill site (`BRepFill_Filling::G0Error()`'s own header: "the maximum distance between the result and the constraints", the caller's own boundary, unlike the plate case) but is still unsafe to gate on: `FillingParameters`'s Swift default is `tolerance: Double = 1e-4`, not a fallback for an unset value, so `Tol3d = 1e-4` is the number **every** default `Shape.fill` call is built at, and legitimate, correct, higher-continuity or heavily-constrained fills routinely exceed it. Gating broke 2 of 17 `FillingSupportFaceTests` (`curvatureContinuityIsAccepted`, `internalConstraintIsNotABoundary`), both at the plain default tolerance, both asserting specific, correct, checked geometry. **A PR #751 review caught that the fill site's own supporting fixture initially understated this**: `wavyEdge()` in `occt_597_fill_g0_realistic.mm` built its sine-sampled boundary points as a single-span, full-multiplicity `Geom_BSplineCurve`, i.e. as control poles of a degree-13 Bezier, which damps a high-frequency control polygon severely (62.8% of the intended amplitude gone, measured directly from the Bernstein basis). Rebuilt on `GeomAPI_Interpolate` (which actually interpolates the sampled points, matching `OCCTCurve3DInterpolate`'s existing precedent in `OCCTBridge_Curve3D.mm`), the same boundary's `G0Error` jumps from "within the bridge's default tolerance" to exceeding even that loosest tested tolerance by ~2000x, and the accepted surface's control poles land up to ~530 units from a ~10-unit-scale boundary. `IsDone()` is still true, and `G0Error()` alone (0.2) does not communicate how far the fit has actually diverged. A draft of this entry then claimed no single `G0Error()` threshold could separate that diverged fit from the two regressing tests, without measuring what those two tests' own `G0Error()` actually were; measured directly (temporary debug prints in both `BRepOffsetAPI_MakeFilling::Build()` call sites, reverted after), they are 5.295e-4 (5.3x tolerance) and 1.231e-3 (12.3x tolerance), two to three orders of magnitude below the corrected fixture's ~2000x. So a fixed absolute threshold placed between those values and the diverged case (e.g. 0.01) would in fact separate them; the honest conclusion is narrower, not reversed: gating on the *caller's own requested tolerance* is unsafe and proven so by the two real regressions, while a *fixed* threshold is unjustified rather than impossible, since nobody has measured what it should be and picking one now would be inventing a number with as little basis as the `1e-4` this investigation already discredited, exactly the failure mode #726 exists to catch. No bridge fix shipped from this investigation; two doc comments (`OCCTBridge_Internal.h` on `occtPlateApproxSurface`, `OCCTBridge_Healing.mm` on `OCCTShapeFillBuildResult`) record why, so the same fix is not silently re-attempted after a future file breakdown (#393-#395) moves either comment. `ShapeUpgrade_UnifySameDomain`'s two bridge sites and `ShapeCustom_BSplineRestriction` were also swept and need nothing: the former exposes no error API at all (confirms #741's finding), the latter already self-polices in the kernel (declines a face's conversion outright rather than ever accepting one out of tolerance). See [`Scripts/repro/597-bridge-modeling-healing-approx-error/`](https://github.com/SecondMouseAU/OCCTSwift/tree/main/Scripts/repro/597-bridge-modeling-healing-approx-error) for the full measurement and [`Scripts/repro/572-approx-consumer-sweep/`](https://github.com/SecondMouseAU/OCCTSwift/tree/main/Scripts/repro/572-approx-consumer-sweep) for Cluster E's own census artifact. #597.
 
 ### Carrying OCCT source patches
 

Scripts/repro/572-approx-consumer-sweep/README.md

@@ -223,3 +223,30 @@ harness's fixtures because the fit there is exact, and filed with the above.
 checked against the released pre-`0019` kernel with `OCCTSWIFT_REMOTE=1`, where exactly the two
 pinning tests fail with the numbers above (0.876114 and 0.103785) and the three deliberate
 non-regression controls pass.
+
+## #597: the wider family, beyond `GeomConvert_ApproxSurface`
+
+Cluster E's own one-line definition (`docs/v2.0.0-plan.md`) is "accept an approximation without
+reading its error", not "constructs `GeomConvert_ApproxSurface`" specifically. The sites above are
+the ones that follow #522's blast radius through that one class. #597 swept the rest of the
+cluster's territory: every OCCT class in `OCCTBridge_Modeling.mm`/`OCCTBridge_Healing.mm` capable of
+reporting a fitting/approximation error, regardless of which approximator it wraps. This is that
+enumeration, extending this artifact per the census-once rule rather than duplicating it in #597's
+own directory:
+
+| site | file | error API | reads it? | verdict |
+|---|---|---|---|---|
+| `occtPlateApproxSurface` (backs `OCCTShapePlatePoints`, `OCCTShapePlateCurves`, +4 more in `OCCTBridge_ProjLib_NLPlate.mm`) | Healing.mm (shared helper in ProjLib_NLPlate.mm) | `GeomPlate_MakeApprox::ApproxError()`/`CriterionError()` | no | investigated, not a fixable defect (measures fidelity to an intermediate object the caller never sees, not the caller's own input) |
+| `OCCTShapeFillBuildResult` (backs `OCCTShapeFill`, `OCCTShapeFillWithSupport`, `OCCTShapeFillConstraints`) | Healing.mm | `BRepOffsetAPI_MakeFilling::G0Error()`/`G1Error()`/`G2Error()` | no | investigated, not a fixable defect (the right metric, but the bridge's `Tol3d` default is routinely and legitimately exceeded by correct fills) |
+| `occtUnifySameDomain` / `OCCTUnifySameDomain` builder | Healing.mm / Modeling.mm | none (`ShapeUpgrade_UnifySameDomain` exposes no error getter at all) | n/a | confirms this artifact's own finding above; nothing to read |
+| `PipeShellBuilder` (`OCCTPipeShell*`) | Modeling.mm | `BRepFill_PipeShell::ErrorOnSurface()` | **yes** | not a defect: deliberate manual-builder design, error is an opt-in accessor |
+| `FillingSurface` (`OCCTFilling*`) | Modeling.mm | `BRepOffsetAPI_MakeFilling::G0Error()`/`G1Error()`/`G2Error()` | **yes** | same as above |
+| `ShapeCustom_BSplineRestriction` (`OCCTShapeBSplineRestrictionAdvanced`) | Healing.mm | `SurfaceError()`/`Curve3dError()`/`Curve2dError()`/`MaxErrors()` | no | self-polices in the kernel: declines a face's conversion rather than ever accepting one out of tolerance |
+| `ShapeCustom_ConvertToBSpline`, `ShapeCustom_ConvertToRevolution` | Healing.mm | none | n/a | not an approximation with a reported error |
+| `BRepFill_NSections`, `BRepOffsetAPI_ThruSections`, `BRepOffsetAPI_MakeOffset(Shape)`, `BRepFill_Evolved` | both | none | n/a | no error-reporting API in OCCT for these |
+| `GeomAPI_PointsToBSpline(Surface)`, `GeomAPI_Interpolate` | Modeling.mm | none | n/a | `#include`d only; no construction site in either file |
+
+Full measurement detail (the actual probes, the reverted-fix confirmation against
+`Issue571PlateApproxTests`/`FillingSupportFaceTests`, and why each verdict above holds) lives at
+[`Scripts/repro/597-bridge-modeling-healing-approx-error/`](../597-bridge-modeling-healing-approx-error/README.md),
+which this table indexes rather than duplicates.