fix(#905): carry the BRepOffsetAPI_ThruSections capping-guard kernel patch - #909
Merged
Conversation
gsdali
added a commit
that referenced
this pull request
Aug 14, 2026
Real findings from /code-review, addressed: - MakeSolid()'s new throw could leave face1/face2 (aliased to myFirst/ myLast, exposed via FirstShape()/LastShape()) holding a real, never-added-to-the-shell face when one wire's PerformPlan() succeeded before the other's failed. Nullify both before throwing. Re-verified via override-link: same 5/5 GTest pass, same FuseConeLoftWithBox_DegeneratedEdge pass. Pushed the corrected commit to the same gsdali/OCCT fork branch backing Open-Cascade-SAS/OCCT#1462 (force-push, not a new PR) -- CI re-running there. - Scripts/patches/README.md's "Pin consequence" paragraph was self- contradictory: said 0026 was outside the pin "same as 0022-0025" while also listing the pin as including 0014-0025 (which includes them). Package.swift's own census says all fifteen, through 0025, are baked into the v2.0.0 release asset. Corrected. - Documented, not fixed: why GetStatus() stays _Done on this failure path (Build()'s generic catch doesn't touch myStatus for any exception source; fixing it needs a call-site change the whole point of this fix was to avoid, for a value nothing in this tree reads), and why this patch throws rather than following 0025's (#597) surface-a-diagnostic-number pattern (different problem shape: wrong pass/fail vs. wrong diagnostic). - Added Scripts/repro/905-thrusections-capping-guard/, matching every other carried patch's convention (a standalone ground-truth probe + real before/after transcripts) -- missing from the first commit. - CLAUDE.md: added a pointer to the existing FillingSurface/BRepFill_Filling composable workaround for a caller who hits this on a real non-planar loft, since ThruSections' own capping stays plane-only. Filed separately rather than folded in here: a pre-existing, unrelated bridge gap the reviewer found (ThruSectionsBuilder.generatedFace(from:) doesn't check IsDone(), can return stale data after a failed reused build() -- not introduced or worsened by this fix, same shape already existed for every other exception this file can throw). #910. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Collaborator
Author
|
Pushed a response to
|
…patch ThruSectionsBuilder(isSolid: true) silently omits both end-cap faces for a closed section wire with k >= 2 periods of out-of-plane variation around the loop. MakeSolid()'s local `B` already tracks whether capping succeeded and discards it, unconditionally marking the result Closed(true) regardless. Carry Scripts/patches/0026-BRepOffsetAPI_ThruSections-capping-guard-905.patch: throw StdFail_NotDone when B is still false after the capping block, matching the exception this same function already throws for a null shell. No signature or call-site change. Distinguishes a genuine capping failure from a degenerate (point) end needing no cap, which an earlier null-face-based attempt could not (it regressed BOPAlgo_PaveFillerTest.FuseConeLoftWithBox_DegeneratedEdge). Validated via override-link (compile the changed source standalone, link ahead of the prebuilt OCCT archive, no full rebuild): the new upstream GTest fails against the unpatched source and passes against the patched one, and the actual FuseConeLoftWithBox_DegeneratedEdge GTest passes unmodified against the patched source. Filed upstream as Open-Cascade-SAS/OCCT#1462, CI green. Documents the process correction: an earlier session had opened a same-repo staging PR on the gsdali/OCCT fork (gsdali/OCCT#1, that fork's first PR) to CI-validate a first attempt before submitting upstream. That isn't how any other carried patch here was validated, and the first attempt's null-face guard is what regressed the GTest above. gsdali/OCCT#1 is closed as erroneous; this commit's Scripts/patches/README.md entry says so. Not yet in a rebuilt xcframework -- Package.swift's pin is unchanged. That is a separate release-engineering step, tracked the same way patches 0022-0025 already are. Closes #905 Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Real findings from /code-review, addressed: - MakeSolid()'s new throw could leave face1/face2 (aliased to myFirst/ myLast, exposed via FirstShape()/LastShape()) holding a real, never-added-to-the-shell face when one wire's PerformPlan() succeeded before the other's failed. Nullify both before throwing. Re-verified via override-link: same 5/5 GTest pass, same FuseConeLoftWithBox_DegeneratedEdge pass. Pushed the corrected commit to the same gsdali/OCCT fork branch backing Open-Cascade-SAS/OCCT#1462 (force-push, not a new PR) -- CI re-running there. - Scripts/patches/README.md's "Pin consequence" paragraph was self- contradictory: said 0026 was outside the pin "same as 0022-0025" while also listing the pin as including 0014-0025 (which includes them). Package.swift's own census says all fifteen, through 0025, are baked into the v2.0.0 release asset. Corrected. - Documented, not fixed: why GetStatus() stays _Done on this failure path (Build()'s generic catch doesn't touch myStatus for any exception source; fixing it needs a call-site change the whole point of this fix was to avoid, for a value nothing in this tree reads), and why this patch throws rather than following 0025's (#597) surface-a-diagnostic-number pattern (different problem shape: wrong pass/fail vs. wrong diagnostic). - Added Scripts/repro/905-thrusections-capping-guard/, matching every other carried patch's convention (a standalone ground-truth probe + real before/after transcripts) -- missing from the first commit. - CLAUDE.md: added a pointer to the existing FillingSurface/BRepFill_Filling composable workaround for a caller who hits this on a real non-planar loft, since ThruSections' own capping stays plane-only. Filed separately rather than folded in here: a pre-existing, unrelated bridge gap the reviewer found (ThruSectionsBuilder.generatedFace(from:) doesn't check IsDone(), can return stale data after a failed reused build() -- not introduced or worsened by this fix, same shape already existed for every other exception this file can throw). #910. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Per okf/policies/changelog-on-merge.md: added as the last commit on the branch before merging. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
gsdali
force-pushed
the
fix/905-thrusections-capping-guard
branch
from
August 14, 2026 13:43
bd69e40 to
9ca09ab
Compare
This was referenced Aug 15, 2026
gsdali
added a commit
that referenced
this pull request
Aug 15, 2026
12 findings, all addressed or explicitly documented. Finding 1 (critical, confirmed live via this PR's own first CI run, which crashed swift build + test (macOS) with exactly the predicted SIGSEGV): mismatchedSectionEdgeCountWithoutCheckFailsCleanly needs patch 0027, which isn't in Package.swift's pinned kernel asset. SwiftPM runs every test target in one process, so an unguarded crash aborts the whole suite for every future PR until the pin moves -- the #585 failure shape. Gated on @test(.enabled(if: ProcessInfo.processInfo.environment["OCCTSWIFT_LOCAL"] == "1")), matching #905/PR #909's precedent of adding no Swift test at all for the identical reason. kernel-integration.yml sets OCCTSWIFT_LOCAL=1 when it builds Scripts/patches/ from source, so the test still runs (and is verified) there. Finding 3 (real, verified empirically before documenting): the guard's inequality test already rejected fewer-edge sections, not just more -- previously silent, invalid-but-"successful" results, not just crashes. Documented accurately everywhere (patch header, README, CLAUDE.md, this PR's own SemVer note, which previously undersold this direction). Finding 4: the punctual-section exemption now also verifies a section has at least one edge before exempting it (w1Point/w2Point are vacuously true for a genuinely empty wire). Attempted to reproduce a live crash for this via both a fresh and reused builder and could not -- documented as a hardening, not a proven fix; the GTest written for it could not be made to fail without the check and was removed rather than kept as unproven coverage. Finding 5: added punctualApexWithMatchingSectionsStillSucceedsUnderCreateSmoothed -- nothing previously pinned a legitimate cone-apex loft succeeding once #913's guard reaches CreateSmoothed (3+ sections). Doesn't depend on patch 0027 (the exemption itself is unmodified pre-existing behavior), so it isn't gated; verified passing against both the pinned and the patched kernel. Finding 6: added a CLAUDE.md Known OCCT Bugs entry, matching every other carried patch. Finding 7: committed a reproducer at Scripts/repro/913-thrusections-createsmoothed-section-edge-count-guard/, matching #905's own convention (README + standalone .mm + stock.txt/patched.txt). Finding 8: converted guard-let-else-return setup to try #require(...). Finding 9: hoisted the punctual-section predicate into a lambda shared with the pre-existing fill loop, in both the local patch and the already-submitted upstream OCCT#1466 PR (new commit pushed there). Finding 11: fixed the factually wrong "(zero) edge count" comment -- AddVertex() creates a wire with exactly one degenerate edge, not zero. Finding 12: SIGSEGV and SIGBUS were both genuinely observed, in different binaries (a custom-handler standalone reproducer vs. a GTest binary with OS default signal handling) -- not a contradiction to resolve to one signal, exactly what heap corruption looks like. Documented both, with which binary showed which. Findings 2, 10: no action needed / documented rationale for leaving the check's placement as-is (see the patch's own updated writeup). Verified against a full local kernel rebuild with all 17 patches (patch 0027 v2 correctly recognized as "already applied", confirming the regenerated patch file matches): full swift test 5534/5534 passing, all 6 static gates clean. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What & why
ThruSectionsBuilder(isSolid: true)silently omits both end-cap faces for a closed section wirewith two or more periods of out-of-plane variation around the loop (a genuinely non-planar closed
curve) —
build()returnstrue, butcheckResult.isValidisfalsewith no localized error,and it's the root cause behind #702's demoted-solid symptom. Root-caused in
BRepOffsetAPI_ThruSections.cxx'sMakeSolid(): it already tracks whether capping succeeded in alocal flag and discards it. This PR carries the fix as
Scripts/patches/0026-*.patch, files itupstream as OCCT#1462 (CI green), and
corrects a process error along the way — see "Notes for the reviewer".
Closes #905
CHANGELOG entry
ThruSectionsBuilder(isSolid: true)no longer silently reports success on an uncapped loft (#905)ThruSectionsBuilder(isSolid: true)used to silently omit both end-cap faces for a closed sectionwire with two or more periods of out-of-plane variation around the loop — a genuinely non-planar
closed curve, not just one with nonzero Z spread.
build()returnedtrue, butshape.checkResult.isValidwasfalsewitherrorCount == 0and no localized error, and asubsequent
.healed()call silently demoted the result from a solid to a shell rather thanrepairing it (root cause of #702).
MakeSolid()'s capping helper (PerformPlan(), which only fitsa plane or a surface already attached to a wire's edges) already tracked whether capping actually
succeeded in a local flag and discarded it, unconditionally marking the result
Closed(true)regardless. Fixed in
Scripts/patches/0026-*: throw instead, matching the pattern this samefunction already uses for a null shell. Filed upstream as
OCCT#1462, validated via override-link.
Not yet shipped: the fix is carried, not baked into this release's xcframework —
ThruSectionsBuilder(isSolid: true).build()for this input still reports the old, wrongtrueuntil the kernel is next rebuilt and re-pinned.
SemVer impact
NONE for this PR. It carries a kernel patch and documentation only;
Package.swift's pin isunchanged, so no consumer sees any behavior difference from this diff by itself. The fix takes
effect only once the kernel is rebuilt and re-pinned (a separate release-engineering step, same as
patches
0022-0025), at which point it will be a PATCH-level fix: no API signature change,ThruSectionsBuilder(isSolid: true).build()correctly reports failure instead of a wrong successfor a non-planar closed section wire.
Checklist
this diff has no OCCTSwift-observable behavior change (the kernel isn't rebuilt), so there is
nothing for a Swift test to assert yet. The fix's test coverage is the new upstream GTests
(
NonPlanarClosedWireCappingFails,DegenerateVertexEndStillSucceeds) inOCCT#1462. A Swift-level regression test
is deferred to whichever PR next rebuilds/re-pins the kernel, matching how
0022-0025wereeach handled.
--self-testcase was run once with its subject broken — donefor the upstream GTest, not an OCCTSwift one: override-linked the unpatched
.cxxandconfirmed
NonPlanarClosedWireCappingFailsfails exactly as the issue describes (IsDone()reads
true); override-linked the patched.cxxand confirmed it passes, along withDegenerateVertexEndStillSucceedsand all pre-existing tests in the file. Also compiled andran the actual
BOPAlgo_PaveFillerTest.FuseConeLoftWithBox_DegeneratedEdge, unmodified,against both: fails against the first (null-face) attempt's guard, passes against this one.
docs/CHANGELOG.mdis not in this diff.docs/SEMVER.mdis not in this diff.Notes for the reviewer
Process correction, separate from the fix itself. Issue #905's comment 1 (an earlier session)
proposed the guard against a same-repo staging PR on the
gsdali/OCCTfork —gsdali/OCCT#1, that fork's first PR ever (verified via
gh pr list --repo gsdali/OCCT --state all, one result). That isn't how any other patch inScripts/patches/README.mdwas validated: every prior one went straight from local override-linktesting to the real PR against
Open-Cascade-SAS/OCCT. Comment 2 on #905 is CI on that fork PRcatching a real regression in the first attempt's null-face guard
(
BOPAlgo_PaveFillerTest.FuseConeLoftWithBox_DegeneratedEdge). This PR's fix is a different,correct approach (see the CHANGELOG entry and the
Scripts/patches/README.mdwrite-up), andgsdali/OCCT#1has been closed as erroneous — its own last comment explains why and points here.Why the fix isn't a null-face check.
PerformPlan()leaves the cap face null in two differentcases: capping genuinely failed, or the wire is degenerate (a point section from
AddVertex(),e.g. a cone's apex) and needs no cap at all. A null-face check can't tell these apart; the local
bool BthatMakeSolid()already computes can, sincePerformPlan()'s own degenerate-wireshortcut leaves it
true. Full mechanism inScripts/patches/README.md's0026entry.Deliberately not done here, matching the precedent for
0022-0025: noOCCT.xcframeworkrebuild, no
Package.swiftre-pin, no Swift-level regression test (would fail against thecurrently-pinned kernel). All three are the release-engineering step comment 1 on #905 already
flagged as separate.