fix(#910): guard ThruSectionsBuilder.generatedFace(from:) on IsDone()

[gsdali]

What & why

ThruSectionsBuilder.generatedFace(from:) (OCCTThruSectionsGeneratedFace) read OCCT's
GeneratedFace() without checking whether the last build actually succeeded. GeneratedFace() is
a bare lookup into myEdgeFace, which Build() never clears between calls, so a builder reused
after a failed rebuild kept answering from a prior successful build's (or the failed build's own
partial) bindings — silently, no null, no error.

Revised after review round 1 (see the PR's own comment thread): the first commit guarded on
ts->builder->IsDone(), matching OCCTThruSectionsShape's existing pattern — and that guard is
incomplete. BRepOffsetAPI_ThruSections::Build() has two punctual-section validation loops (a
wholly-degenerate MIDDLE section, reachable via the public addVertex() at an interior position)
that return WrongUsage without ever calling OCCT's NotDone(). On a builder that already
built successfully once, IsDone() stays stale-true through that failed rebuild. The shipped fix
is a bridge-side built field on the OCCTThruSections wrapper, set exactly once in
OCCTThruSectionsBuild via IsDone() && GetStatus() == BRepFill_ThruSectionErrorStatus_Done.

Revised again after review round 2: built alone is still not enough. OCCT's myEdgeFace map
is never cleared, only ever added to — so a THIRD build succeeding after an intervening failure
(build ok → add a mismatched section, build fails → checkCompatibility(true) reconciles it,
build ok again) can rebuild every section's edges, not just the newly-added one's, stranding an
earlier build's edge → face binding in the map without ever overwriting it. Measured empirically
before this round's fix: generatedFace(from:) answered non-nil with a face that was provably not
part of the successful third build's own shape. OCCTThruSectionsGeneratedFace now confirms the
face it finds is actually a member of the current Shape() (TopExp_Explorer) before returning
it, instead of trusting the map. Round 2 also found and fixed a related inconsistency: the six
builder-configuration setters (setSmoothing, setMaxDegree, setContinuity,
checkCompatibility, setParType, setCriteriumWeight) didn't invalidate built the way
addWire/addVertex already did, so changing a setting right after a successful build used to
leave .shape silently serving pre-change geometry. All eight mutators now invalidate
consistently, and the doc comments on .shape/generatedFace(from:) were rewritten to describe
that actual contract instead of the narrower "nil if the last build() call failed" one they no
longer matched.

Closes #910

CHANGELOG entry

ThruSectionsBuilder no longer returns stale results after a failed rebuild, or after a build on a changed builder that hasn't been rebuilt (#910)

generatedFace(from:) and shape both read post-build OCCT state without reliably checking
whether the last build() call on that instance actually succeeded, and without accounting for
OCCT's own internal state never resetting between builds or setting changes on a reused builder.
Fixed in two rounds:

• Outcome tracking. A ThruSectionsBuilder reused across multiple build() calls could
  silently keep returning a prior successful build's geometry — through generatedFace(from:),
  shape, and build()'s own return value — after a later build() call on the same instance
  genuinely failed, including failures that OCCT's own IsDone() does not reliably report on a
  reused builder (a punctual middle section added via addVertex()).
• Stale binding after a later success. Even with outcome tracking, a builder that succeeded,
  then failed, then succeeded again could still have generatedFace(from:) answer with a face left
  over from the first success rather than the current one — OCCT's internal edge→face map is
  additive-only. generatedFace(from:) now confirms its answer is actually part of the current
  shape before returning it.
• Setting changes without a rebuild. setSmoothing, setMaxDegree, setContinuity,
  checkCompatibility, setParType, and setCriteriumWeight now invalidate a prior successful
  build the same way addWire/addVertex already did — previously, changing a setting after a
  successful build left .shape/generatedFace(from:) silently serving geometry built under the
  old setting until the caller happened to add a new section too.

Fixed by tracking the real build outcome bridge-side rather than trusting IsDone() alone, and by
verifying generatedFace(from:)'s answer against the current build's own shape rather than
trusting OCCT's internal map.

SemVer impact

PATCH. A caller of ThruSectionsBuilder.build(), .shape, or .generatedFace(from:) who reuses a
builder across build() calls now correctly gets false/nil after any failed rebuild, or after
changing a setting without rebuilding, instead of stale or wrong geometry from an earlier build. No
signature change; the only observable difference is a previously-buggy return value becoming
correct — including for callers who call setSmoothing/setMaxDegree/setContinuity/
checkCompatibility/setParType/setCriteriumWeight between a successful build() and reading
.shape/generatedFace(from:) without an intervening rebuild: that combination previously
returned the pre-change build's geometry and now correctly returns nil until build() is called
again. No migration.

Checklist

• New or changed behavior is covered by a unit test in the same PR (not just manual
  verification): five tests in Tests/OCCTStressTests/StressBuilderLifecycleTests.swift —
  generatedFaceNilAfterFailedRebuild (the original open/closed-mismatch scenario),
  generatedFaceNilAfterWrongUsageOnReusedBuilder (the IsDone()-defeating scenario round 1's
  review found), generatedFaceNilAfterFailedRebuildRuledPath (coverage for CreateRuled()'s
  different myEdgeFace-binding mechanism, now genuinely reached via isRuled: true + 3
  sections rather than the 2-section shortcut round 2's review caught it silently relying on),
  generatedFaceIsMemberOfShapeAfterSuccessFailureSuccessOnReusedBuilder (round 2's finding 1:
  success → failure → success no longer returns a stale binding), and
  shapeNilAfterSettingChangedWithoutRebuild (round 2's finding 2: all six setters invalidate
  a prior build, not just addWire/addVertex).
• Every new test and every new --self-test case was run once with its subject broken, and the
  failure is reported here:
  - Round 1, first commit: reverted the IsDone() guard and re-ran the original test — it
  failed with Expectation failed: (loft.generatedFace(from: edge) → OCCTSwift.Shape) == nil.
  - Round 1, second commit: reverted the bridge back to the IsDone()-only guard and re-ran
  generatedFaceNilAfterWrongUsageOnReusedBuilder — it failed with exactly the three
  symptoms the review predicted (build() == true, shape != nil, generatedFace != nil).
  - Round 2: removed the TopExp_Explorer membership check and re-ran
  generatedFaceIsMemberOfShapeAfterSuccessFailureSuccessOnReusedBuilder — failed with
  Expectation failed: isMember. Removed all six setters' built = false resets and re-ran
  shapeNilAfterSettingChangedWithoutRebuild — failed at both assertions (setContinuity and
  checkCompatibility). Also independently verified — by deleting the addWire/addVertex
  resets and running the full pre-round-2 ThruSectionsBuilder-touching test set (130 tests
  across 6 files) — that no existing test caught that reset's removal, confirming review
  round 2's finding 7 before adding coverage for it.
  - Restored the real fix after each injection; final state: all 5 required static gates clean,
  full OCCTModelingTests (645/645) and OCCTStressTests (365/365, +2 over round 1's 363)
  pass.
• The CHANGELOG entry above is complete, and docs/CHANGELOG.md is not in this diff.
• The SemVer impact above is stated, and docs/SEMVER.md is not in this diff.
• Every static gate script is clean except code-style: check-style-manifest.py fails on
  this head because both touched OCCTBridge_Modeling.* files are still on
  Scripts/style-manifest-bridge.txt. code-style is not a required check (only
  gate-scripts is); bringing a 24k/3.4k-line pair of files into clang-format compliance is
  out of scope for this fix and is tracked as a deferred sub-issue of the format-check work,
  #917 — round 2's review flagged that
  the previous version of this checklist implied full cleanliness inaccurately; this line now
  says what's actually true.

Notes for the reviewer

Found during PR #909's review (the #905 fix). Filed separately per this project's scope-boundary
policy rather than folded into #909.

The deferred SIGSEGV originally noted here (checkCompatibility(false) + mismatched section edge
counts on a reused builder) has since been root-caused, patched, and shipped separately:
#913,
OCCT#1466,
#915 (verified against a full local kernel
rebuild, 5533/5533 tests).

Round 2 review disposition (12 findings, all independently re-verified against the pinned
V8_0_1 OCCT source before acting on any of them, per this project's own practice — several
findings were confirmed by reading BRepOffsetAPI_ThruSections.cxx directly rather than trusting
the review's claim, and one candidate fix (finding 8's original framing) turned out to need its own
separate issue rather than a quick patch once the actual mechanism was traced):

• 1 (blocking, fixed): generatedFace(from:) membership check against the current shape.
• 2 (fixed): all six setters now invalidate built.
• 3 (fixed): doc comments (Swift + bridge header) rewritten to match the real contract.
• 4 (fixed): CHANGELOG entry and SemVer paragraph above now cover the setter-invalidation
  behavior change.
• 5 (fixed): checklist's static-gate line above now states code-style's real, deferred
  status instead of implying it's clean.
• 6 (fixed): struct comment no longer cites OCCTSectionBuilder as a working precedent — it
  has the identical unfixed bug, tracked at OCCTSectionBuilder's built flag never resets to false on a failed rebuild (same class as #910) #916.
• 7 (fixed): added shapeNilAfterSettingChangedWithoutRebuild; independently confirmed via
  injection that no prior test caught either the setter gap or the original addWire/addVertex
  reset's removal.
• 8 (filed as a follow-up, out of scope here):
  #919 — traced to
  BRepOffsetAPI_ThruSections::SetCriteriumWeight itself silently no-oping on a negative weight
  (verified directly in the OCCT source, myStatus = Failed but myCritWeights left unchanged),
  compounded by Build() unconditionally resetting myStatus = Done on entry so the failure is
  unobservable even via GetStatus() by the time a caller checks. Unrelated to generatedFace(from:)
  staleness; needs its own bridge-signature change (setCriteriumWeight would need to return
  Bool) rather than fitting this PR's shape.
• 9 (fixed): added an in-source swift snippet to ThruSectionsBuilder's type doc, since none
  of its doc comments had one before.
• 10 (fixed): generatedFaceNilAfterFailedRebuildRuledPath now uses isRuled: true with 3
  sections instead of silently relying on the 2-section CreateRuled() dispatch shortcut.
• 11 (not actionable here): ThruSectionsBuilder's unsynchronized built field on an
  @unchecked Sendable type matches every sibling builder class in this codebase (none guard their
  internal mutable state with a per-instance lock) — this project's documented thread-safety model
  is caller-side serialization via OCCTSerial.withLock, not per-field locking
  (docs/thread-safety.md), and the class-wide question is already tracked project-wide by Bridge-level thread-handling contract: per-call safety classification, scoped/controllable internal parallelism, encapsulated global state — safety AND throughput #342.
  Adding a mutex to just this one field would be inconsistent with every other builder and
  wouldn't actually make concurrent same-instance use safe (the underlying OCCT builder pointer
  itself isn't reentrant either).
• 12 (fixed): trimmed the struct's built comment and the AddWire/GeneratedFace comments
  that had grown to restate the same three facts multiple times across this PR's two rounds.

[secondmouseAU-bot]

xhigh-effort review of 268e94f. The --comment flag was silently dropped by the review tooling again (a known issue), so these are being posted directly. Every OCCT-behaviour claim below was re-verified against the actual OCCT V8_0_0 source before posting — file/line citations are to src/ModelingAlgorithms/TKOffset/BRepOffsetAPI/BRepOffsetAPI_ThruSections.{hxx,cxx} at that tag.

The diagnosis in the PR body is right: GeneratedFace() is a bare myEdgeFace lookup and Build() genuinely never clears it (confirmed — myEdgeFace is a plain NCollection_DataMap member, and no path in Build() touches it before CreateRuled()/CreateSmoothed() re-Bind() into it). But IsDone() is the wrong gate, and the guard as written still lets the #910 defect through.

1. IsDone() does not mean "this build succeeded" — the guard is incomplete (Sources/OCCTBridge/src/OCCTBridge_Modeling.mm:8770)

BRepOffsetAPI_ThruSections::Build() opens with two punctual-section validation loops, and both return without calling NotDone():

// BRepOffsetAPI_ThruSections.cxx:341
myStatus = BRepFill_ThruSectionErrorStatus_Done;
...
for (i = 2; i <= myWires.Length() - 1; i++) {          // :346
  ...
  if (wdeg) {
    myStatus = BRepFill_ThruSectionErrorStatus_WrongUsage;   // :356
    return;                                                  // :357  <-- no NotDone()
  }
}
if (myWires.Length() <= 2) {                            // :360
  ...
  if (wdeg) {
    myStatus = BRepFill_ThruSectionErrorStatus_WrongUsage;   // :373
    return;                                                  // :374  <-- no NotDone()
  }
}

The terminal if (myStatus != Done) { NotDone(); return; } at :531-535 is never reached on those two paths. So on a reused builder whose earlier build succeeded (Done() at :598/:604/:1053 set myDone = true), myDone stays true through a failed rebuild.

This is reachable straight through the public Swift API, because AddVertex appends a wire made of a single degenerated edge — exactly what the first loop scans for:

// BRepOffsetAPI_ThruSections.cxx:311-328
void BRepOffsetAPI_ThruSections::AddVertex(const TopoDS_Vertex& aVertex) {
  ...
  BB.Degenerated(DegEdge, true);
  ...
  myWires.Append(DegWire);

and ThruSectionsBuilder.addVertex(_:) is public. Concrete sequence:

loft.addWire(c1); loft.addWire(c2)
#expect(loft.build())                    // true, myEdgeFace populated
loft.addVertex(v); loft.addWire(c3)      // myWires = [c1, c2, degWire, c3]
#expect(!loft.build())                   // FAILS — build() returns true
loft.generatedFace(from: edge)           // still build #1's face

The first loop hits degWire at i == 3, sets WrongUsage, returns with myDone untouched. OCCTThruSectionsBuild returns IsDone() == true for a build that never ran, the new guard passes, and generatedFace(from:) hands back the previous build's face — plus shape hands back the previous build's solid, since it is fooled identically. That is #910, on the same accessor, still open.

Fix: OCCT 8.0 exposes the real outcome — BRepFill_ThruSectionErrorStatus GetStatus() const { return myStatus; } (.hxx, public, inline). Gate on IsDone() && GetStatus() == BRepFill_ThruSectionErrorStatus_Done.

2. Altitude: record the outcome once on the wrapper instead of probing OCCT per accessor (OCCTBridge_Modeling.mm:8711)

struct OCCTThruSections already carries bridge-side state (sectionCount), and there are now three ways OCCTThruSectionsBuild can report failure while leaving OCCT's myDone untouched: the sectionCount < 2 early return (:8715, never calls NotDone()), the catch (...) (:8719), and OCCT's own WrongUsage returns above. Every accessor that probes IsDone() independently gets all three wrong.

One field covers all of them:

ts->built = false;
ts->builder->Build();
ts->built = ts->builder->IsDone()
         && ts->builder->GetStatus() == BRepFill_ThruSectionErrorStatus_Done;

then gate both OCCTThruSectionsShape (:8726) and OCCTThruSectionsGeneratedFace on ts->built. Wires(), FirstShape(), LastShape() and Generated() are all still unwrapped; as written, the next one wrapped will copy the same incomplete pattern.

3. The regression test passes silently if its setup yields nil (Tests/OCCTStressTests/StressBuilderLifecycleTests.swift:454)

guard let openWire = Wire.polygon3D([...], closed: false),
      let openShape = Shape.fromWire(openWire) else { return }

sits immediately before the only two assertions that test #910. Wire.polygon3D returns nil on points.count < 2 and on any bridge failure; Shape.fromWire is likewise fallible. If either ever starts returning nil — an OCCT bump, a polygon3D regression, a change to open-wire handling — the test returns early and reports green while asserting nothing about the guard it exists to protect. Same hazard at :447 and :441.

This undercuts okf/policies/prove-the-test-fails.md ("Adding a self-test is not the rule, watching it fail is"): the failure was proved today, but the structure lets it stop proving anything silently. try #require(...) on the setup turns a nil into a failure rather than a skip.

4. Only the smoothed path is covered; the ruled path is the more exposed one (:442)

The test builds with isRuled: false, so only CreateSmoothed() runs. OCCT documents two different contracts for the same accessor ("if Ruled Returns the Face generated by each edge except the last wire; if smoothed Returns the Face generated by each edge of the first wire"), and the two implementations bind myEdgeFace from different sources. Since Build() does myWires = WorkingSections; (.cxx:466) on every successful compatibility pass, the ruled path — whose keys come from myWires — is the one most exposed to cross-build aliasing, and it gets zero coverage. An isRuled: true variant is ~5 lines.

5. The positive assertion passes incidentally and is fragile across OCCT bumps (:448)

generatedFace(from: edge) != nil uses an edge from the caller's own input wire, but myEdgeFace is keyed by post-compatibility edges: Build() reassigns myWires = WorkingSections (.cxx:466) and CreateSmoothed() binds from those working sections, while the originals are kept separately in myInputWires (.hxx). The test asks for s1.subShapes(ofType: .edge).first and resolves only because two same-topology closed circles need no re-splitting, so the TShape survives unchanged. The test also enables checkCompatibility(true) — the very flag gating that rewrite. If a future OCCT re-creates that edge, :448 fails for a reason unrelated to #910. Derive the edge from loft.shape, or assert the mapping via a path that does not depend on CompatibleWires leaving input edges untouched.

6. Public behaviour changed; neither doc surface moved (docs/reference/Document-Completions.md:905)

The reference entry still reads "Returns: The generated face shape, or nil if not found", and Sources/OCCTSwift/ThruSectionsBuilder.swift's comment is still the bare "/// Get the face generated from an edge after building." (confirmed at this PR's head). Neither mentions the new "nil if the last build did not succeed" contract — which the PR body itself calls out as the only observable change. This is the CLAUDE.md Documentation Standards rule ("when wrapping or changing a public API, give it a /// summary + parameter docs + at least one fenced snippet", and "update the relevant doc in the same commit"). Cost: a caller reading either doc still believes nil means "edge not found".

7. The parity the PR claims is only half-asserted (:459)

The bridge comment and PR body both justify the change as "matching the existing guard on ThruSectionsBuilder.shape", but the test pins only the new half. One extra line — #expect(loft.shape == nil) — locks in the sibling invariant on the same fixture, and would have surfaced finding 1, where both accessors go stale together.

8. The new test walks toward the uncharacterised SIGSEGV reported in the same PR body (:458)

The "Notes for the reviewer" report a SIGSEGV from checkCompatibility(false) + mismatched section edge counts on a reused builder, attributed to CreateSmoothed()'s per-edge lockstep walk, and defer filing it. This test is the same shape (reused builder, mismatched section topology) with compatibility checking left on. The only thing keeping it on the safe side is if (myWCheck) (.cxx:380) routing into BRepFill_CompatibleWires' NotSameTopology rejection — which does call NotDone() (.cxx:461-463), so the test's own failure path is sound — before CreateSmoothed() ever sees the mismatch. CLAUDE.md is explicit that "OS signals raised inside OCCT are still uncatchable in-process", and ThruSections has prior form here (#176/#178). If a kernel bump shifts which branch handles this input, the failure lands in OCCTStressTests as a whole-process abort with no test name, indistinguishable from the known #344/#345 parallel-run crashes. Worth characterising the deferred defect before landing a test that walks toward it.

9. loft.checkCompatibility(true) is a no-op presented as load-bearing (:443)

Both the constructor and Init() already set myWCheck = true (.cxx:268, :288), so the call changes nothing. The neighbouring comment reads as though this line is what routes the build into the NotSameTopology path, so a future reader will treat a dead line as essential. Either drop it, or keep it and say in the comment that it restates OCCT's default rather than changing it.

---

Bottom line: the direction is right and the stale-myEdgeFace analysis holds up, but IsDone() is the wrong gate — a reused builder can still report build() == true and serve both stale shape and stale generatedFace(from:). The robust form is a bridge-side outcome flag built on GetStatus(), gating both accessors (findings 1 + 2), with try #require on the test setup (finding 3).

[gsdali]

Pushed a response to the review's findings (commit 1ac2129):

• Fixed, verified empirically before and after (finding 1, the load-bearing one): IsDone()
  alone is the wrong gate. Reproduced the exact AddVertex()-at-an-interior-position scenario the
  review predicted — Build()'s "wholly-degenerate middle section" check returns WrongUsage
  without calling NotDone(), so a reused builder's IsDone() stays stale-true through a failed
  rebuild. Confirmed live: build() returned true, shape/generatedFace(from:) both still
  served the prior build's stale geometry.
• Fixed (finding 2): OCCTThruSections now carries a built field, set once in
  OCCTThruSectionsBuild via IsDone() && GetStatus() == BRepFill_ThruSectionErrorStatus_Done.
  OCCTThruSectionsShape and OCCTThruSectionsGeneratedFace both gate on that instead of
  re-deriving from OCCT state, closing the gap for all three ways Build()'s return value could
  diverge from what the accessors saw.
• Fixed (findings 3, 7, 9): the existing test is now throws + try #require(...) instead of
  silent-skip guard let ... else { return }; added the sibling loft.shape == nil assertion;
  dropped the no-op checkCompatibility(true) call (already OCCT's own default).
• Fixed (finding 4): added a ruled-path variant (isRuled: true) — CreateRuled() binds
  myEdgeFace via BRepFill_Generator, a different mechanism from CreateSmoothed()'s own loop.
• Fixed (finding 6): /// doc comments on both generatedFace(from:) and shape, plus
  docs/reference/Document-Completions.md, now state the "nil if the last build did not succeed"
  contract.
• Added: a dedicated regression test for finding 1's own mechanism
  (generatedFaceNilAfterWrongUsageOnReusedBuilder) — proved it fails against the pre-fix
  IsDone()-only guard with exactly the three symptoms predicted (build() == true,
  shape != nil, generatedFace != nil), then passes after the built-flag fix. Full
  StressThruSectionsBuilderLifecycleTests (8/8), OCCTModelingTests (645/645), OCCTStressTests
  (363/363) all green.
• Noted, not changed (finding 5): the positive assertion's edge identity does depend on
  BRepFill_CompatibleWires leaving already-matching-topology input edges untouched — true today,
  not a documented guarantee. Left it in place but called out the dependency inline: the suggested
  alternative (deriving the edge from loft.shape) exercises different, equally-untested
  key-derivation semantics of GeneratedFace(), and the assertion this test actually exists to
  protect (the negative one, after the failed rebuild) doesn't depend on this identity holding
  across OCCT versions either way — it's checking for nil, not a specific face.
• De-risked, not part of this PR (finding 8): the deferred SIGSEGV this PR's own body flagged
  (checkCompatibility(false) + mismatched section edge counts on a reused builder) has since been
  root-caused, fixed, and filed separately: #913,
  upstream OCCT#1466, OCCTSwift
  #915 — verified against a full local
  kernel rebuild, 5533/5533 tests passing.

[gsdali]

Ran my own review pass before the formal one, including dispatching /code-review 912 high as a
second, independent check. Combined findings, pushed a response (commit 7b0abee):

• Fixed (code-review finding, real CI blocker): code-style was failing — this PR's 3 touched
  files were still listed in Scripts/style-manifest-{swift,bridge}.txt. Brought
  ThruSectionsBuilder.swift into full compliance (one import-order fix) and removed it from the
  manifest. OCCTBridge_Modeling.{h,mm} are not addressed: measured a full clang-format pass
  at 3,392 and 24,133 diff lines respectively — disproportionate for this PR, matching
  okf/policies/code-style.md's own #877 precedent for exactly this situation. code-style isn't
  a required status check, so this doesn't block merging; flagging for a call on whether to defer
  via a tracked follow-up or absorb here.
• Fixed (code-review finding 4, real): all three new tests' successful build used exactly 2
  sections, and Build() dispatches any 2-section call to CreateRuled() regardless of isRuled
  — so CreateSmoothed()'s own myEdgeFace binding was never exercised, despite two tests'
  comments claiming otherwise. Redesigned the two "smoothed path" tests to use 3 sections; re-ran
  the revert-and-confirm-it-fails cycle against the redesigned fixture.
• Fixed (code-review finding 5): corrected a test comment that overclaimed coverage of finding
  1's specific mechanism.
• Fixed (code-review finding 6, real, pre-existing): AddWire/AddVertex never reset built,
  so a section added after a successful build without a following build() call left
  shape/generatedFace(from:) silently stale. Fixed; no existing test relied on the old
  behavior.
• Fixed (code-review finding 7): the new doc comment on generatedFace(from:) was
  self-contradictory on a literal read. Reworded.
• Documented, not changed (code-review finding 3): added the missing rationale for fixing this
  bridge-side rather than via a kernel patch, directly on the built field.
• Filed separately, not folded in: OCCTSectionBuilder (same file) has the identical staleness
  bug this PR fixes for OCCTThruSections — built is only ever set true, never reset on a
  failed rebuild. #916.

Verified: full StressThruSectionsBuilderLifecycleTests (8/8), OCCTModelingTests (645/645),
OCCTStressTests (363/363), and every static gate script clean.

[secondmouseAU-bot]

xhigh-effort review of 7b0abee (second round, after commits 1ac2129 + 7b0abee). Every OCCT claim below was re-verified against Open-Cascade-SAS/OCCT at V8_0_1 — the tag this repo is currently pinned to — not the V8_0_0 the first round cited; line numbers have moved. I also read Scripts/patches/README.md's 0026 entry, since patch 0026's MakeSolid() throw interacts with the new gate.

The core fix holds up. I independently confirmed the two load-bearing claims:

• Build() sets myStatus = BRepFill_ThruSectionErrorStatus_Done at BRepOffsetAPI_ThruSections.cxx:341, on entry, so there is no stale-status false-negative — IsDone() && GetStatus() == Done cannot wrongly reject a genuinely successful build.
• Both punctual-section loops (:346-359, :360-375) return with myStatus = WrongUsage and no NotDone(), so GetStatus() is genuinely the piece IsDone() was missing.
• And the AND-form is correct against patch 0026 specifically: a capping failure throws through Build()'s catch → NotDone() while GetStatus() stays Done (as Scripts/patches/README.md documents deliberately), so IsDone() carries that case and GetStatus() carries the WrongUsage case. Neither alone is sufficient; the conjunction is.

Nothing from round 1 is re-raised. Findings 1-9 are addressed or consciously deferred with reasons I accept. The 12 comments below are new, and they cluster into three groups:

Correctness — the fix is one layer short (1, 2). built gates the read, but myEdgeFace is still never cleared, so the moment a later build succeeds the flag flips back to true and generatedFace(from:) can hand back an earlier build's — or a failed build's partial — face. Build A ✅ → build B ❌ → build C ✅ is enough to reproduce, and none of the three new tests reaches a second successful build. Separately, six configuration setters on the same struct don't invalidate built while the two input mutators now do, so setContinuity(3) after a successful build leaves .shape serving the C2 solid while addWire one line away returns nil.

Contract drift introduced by this round's own fix (3, 4, 7). 7b0abee's built = false in AddWire/AddVertex is a good change, but it makes all three doc surfaces literally false ("nil if the most recent build() did not succeed" — the most recent build() did succeed), it isn't in the CHANGELOG entry or the SemVer paragraph even though it's the one change here that can break working code, it has no test (delete both lines and all 8 tests still pass), and the sibling accessor's header doc wasn't updated alongside the one that was.

Process/CI (5, 6, 10). code-style is failing on this head — check-style-manifest.py rule 2, because both touched OCCTBridge_Modeling.* files are still on style-manifest-bridge.txt. Deferring is defensible; deferring in a PR comment rather than in the tree isn't, and the checklist's "every static gate script clean" isn't accurate as written. The new struct comment also cites OCCTSectionBuilder as an existing identical precedent when it's actually the same bug unfixed (your own #916) — inverted, that comment is useful; as written it will get copied. And the isRuled: true variant added for round 1's finding 4 was removed again in 7b0abee, leaving CreateRuled()-via-myIsRuled untested under a test named RuledPath.

Also noted, lower stakes: no fenced snippet on either changed public member (9, CLAUDE.md's context7 rule), an unsynchronised bool reached by every accessor on an @unchecked Sendable type (11), two silent-discard paths for criterium weights that this PR's new "GetStatus() is the authority" framing can't see (8), and ~25 comment lines restating three facts four times (12).

Bottom line: the built + GetStatus() mechanism is the right answer to #910 and I'd land it, but (1) means #910 is still reachable through the same accessor on a three-build sequence, and (3)/(4) mean this round's own behaviour change ships undocumented and unproven. I'd fix 1-4 before merge and take 5-12 as follow-ups.

[secondmouseAU-bot]

1. built only gates the read immediately after a failed build — myEdgeFace is still never cleared, so a later successful build re-opens #910.

The comment three lines up states the real defect precisely: Build() never clears myEdgeFace, so it accumulates "the prior successful build's (or the failed build's own partial) bindings". The fix then guards only the case where built == false. But built goes back to true on the next successful build, and the contaminated map is still there.

Concrete sequence, all through the public Swift API:

let loft = ThruSectionsBuilder(isSolid: true, isRuled: false)
loft.addWire(a); loft.addWire(b); loft.addWire(c)
#expect(loft.build())                       // build A: myEdgeFace ← A's bindings

loft.addVertex(v); loft.addWire(d)          // punctual middle section
#expect(!loft.build())                      // build B: WrongUsage — built == false ✅ guard works

// caller removes the bad section by constructing nothing new — just rebuilds after
// a valid mutation, or (equivalently) any third build that succeeds:
#expect(loft.build())                       // build C succeeds → built == true
loft.generatedFace(from: edgeFromA)         // ← returns build A's face, guard passes

CreateSmoothed()/CreateRuled() only ever Bind() into myEdgeFace (BRepOffsetAPI_ThruSections.cxx:664, :676, :1012 at V8_0_1); nothing calls Clear(), and Build() (:339) doesn't either. So after C, the map holds A's bindings, B's partial bindings (CreateSmoothed() binds per-edge as it goes, then bails at :816 with myStatus = Failed), and C's — all indistinguishable to GeneratedFace(), which is a bare IsBound/lookup (:1638-1647).

The same thing happens without any failed build at all whenever a rebuild re-splits sections: Build() does myWires = WorkingSections (:466), so build A's working edges become build B's input, and if B's new section forces BRepFill_CompatibleWires to re-split them, A's edges keep their A-era face bindings while B binds new TShapes.

This directly contradicts the new doc contract added in this PR (ThruSectionsBuilder.swift:83): "Returns nil if edge isn't a profile edge the build used" — for a stale edge it returns a face, not nil.

The bridge-side flag is the right shape; it just needs to also invalidate the map, not only the read. E.g. record a monotonically increasing buildGeneration and keep the bridge's own edge → (generation, face) map populated from GeneratedFace() results, or (cheaper, and closer to what OCCT's own contract implies) re-derive the answer only from the current build by checking the returned face is actually a sub-shape of builder->Shape() before handing it back. Either way, please add a test for "build A succeeds → build B fails → build C succeeds → generatedFace(from: edgeFromA)", since nothing in the three new tests reaches a second successful build.

[secondmouseAU-bot]

2. Only the two input mutators invalidate built; the six configuration mutators on the same struct don't — same staleness, no signal.

The rationale you wrote here — "a section added after a successful build belongs to a build that hasn't happened yet" — applies verbatim to every setter that changes what the next Build() would produce, and none of them resets the flag:

function	line	changes the next build?	resets built?
OCCTThruSectionsAddWire	8690	yes	✅ (this PR)
OCCTThruSectionsAddVertex	8703	yes	✅ (this PR)
OCCTThruSectionsSetSmoothing	8713	yes	❌
OCCTThruSectionsSetMaxDegree	8719	yes	❌
OCCTThruSectionsSetContinuity	8725	yes	❌
OCCTThruSectionsCheckCompatibility	8762	yes	❌
OCCTThruSectionsSetParType	8768	yes	❌
OCCTThruSectionsSetCriteriumWeight	8782	yes	❌

Concrete failure:

loft.addWire(a); loft.addWire(b); loft.addWire(c)
#expect(loft.build())          // C2 loft
loft.setContinuity(3)          // asks for C3
loft.shape                     // ← still the C2 solid, no nil, no error

vs. the sibling mutator one line away:

loft.addWire(d)
loft.shape                     // ← nil, because addWire resets `built`

Two mutators on the same object, the same "your cached result no longer matches the configuration" situation, opposite answers. Whichever contract you pick is defensible; picking different ones per mutator is what will confuse callers (and is what the next person wrapping Wires()/FirstShape()/Generated() will have to guess at). Suggest resetting built in all eight, behind a one-line static inline void invalidate(OCCTThruSections* ts) { ts->built = false; } so the set can't drift again.

[secondmouseAU-bot]

3. The documented contract is now false on all three doc surfaces, because of this PR's own addWire/addVertex change.

This line promises nil only when "the most recent build() call did not succeed". Commit 7b0abee added a second nil condition that has nothing to do with a failed build:

loft.addWire(a); loft.addWire(b)
#expect(loft.build())     // succeeds
loft.addWire(c)           // OCCTThruSectionsAddWire sets built = false
loft.shape                // ← nil, even though the most recent build() DID succeed

Same wrong claim in two more places added by this PR:

• ThruSectionsBuilder.swift:83-84 — "Returns nil if edge isn't a profile edge the build used, or if the most recent build() call on this instance did not succeed." Neither disjunct covers the add-then-read case.
• docs/reference/Document-Completions.md:903-904 — "or if the builder's most recent build() call did not succeed (including on a reused builder, after a later failed rebuild)". The parenthetical actively narrows it to the failed-rebuild case.

It's also an observable behaviour change the PR body doesn't declare. The CHANGELOG entry describes only failed-rebuild staleness, and the SemVer section says "the only observable difference is a previously-buggy return value becoming correct" — but a caller who has never had a failed build, and who reads .shape after appending a section, goes from getting the last build's solid to getting nil. That deserves its own sentence in the CHANGELOG entry, since it's the one change here that can break working code.

Suggested wording for all three: "nil if no successful build() has happened for the builder's current set of sections".

[secondmouseAU-bot]

4. The built = false-on-add behaviour added in 7b0abee has zero test coverage.

Three new tests, all the same shape: mutate → build() → assert. Every one of them calls build() between the mutation and the assertions, so built is false because the rebuild failed, not because addWire/addVertex reset it. Delete ts->built = false; from OCCTThruSectionsAddWire (OCCTBridge_Modeling.mm:8699) and OCCTThruSectionsAddVertex (:8709) and all 8 tests in this suite still pass.

That's the repo's own checklist item — "New or changed behavior is covered by a unit test in the same PR (not just manual verification)" — and CLAUDE.md's Prove the test fails policy ("Every new test, and every new --self-test case, is run once with its subject broken … Adding a self-test is not the rule, watching it fail is", okf/policies/prove-the-test-fails.md). The PR body's revert-and-confirm log covers finding 1's GetStatus() gate and the original IsDone() guard, but not this.

Four lines closes it:

@Test func accessorsInvalidatedByAddingASectionWithoutRebuilding() throws {
    let w1 = try #require(Wire.circle(origin: SIMD3(0, 0, 0), normal: SIMD3(0, 0, 1), radius: 5))
    let w2 = try #require(Wire.circle(origin: SIMD3(0, 0, 10), normal: SIMD3(0, 0, 1), radius: 3))
    let w3 = try #require(Wire.circle(origin: SIMD3(0, 0, 20), normal: SIMD3(0, 0, 1), radius: 2))
    let s1 = try #require(Shape.fromWire(w1))
    let s2 = try #require(Shape.fromWire(w2))
    let s3 = try #require(Shape.fromWire(w3))
    let loft = ThruSectionsBuilder(isSolid: true, isRuled: false)
    loft.addWire(s1)
    loft.addWire(s2)
    #expect(loft.build())
    let edge = try #require(s1.subShapes(ofType: .edge).first)
    #expect(loft.shape != nil)
    loft.addWire(s3)                       // no build() — the prior result is now stale input
    #expect(loft.shape == nil)
    #expect(loft.generatedFace(from: edge) == nil)
}

[secondmouseAU-bot]

5. This sentence cites a known-broken sibling as precedent — OCCTSectionBuilder's built is not "for the same reason", it's the same bug unfixed.

OCCTSectionBuilder (below in this same file) already carries an identical built field for the same reason, predating this PR.

OCCTSectionBuilder (:9877) has a field spelled the same way, but it is only ever written true, at :9959:

OCCTShapeRef OCCTSectionBuilderBuild(OCCTSectionBuilderRef builder) {
    ...
    if (!builder->section.IsDone()) return nullptr;
    builder->built = true;                 // never set back to false, anywhere in the file

Nothing resets it — not a failed rebuild, not Init1/Init2 (:9901-:9937), which are exactly the "input changed after a successful build" mutators this PR just taught OCCTThruSections to handle. AncestorFaceOn1/On2 (:9964, :9975) then gate on a permanently-true flag. That's precisely the #910 defect, which is why you filed it as #916 in the same session as this commit.

So the comment is claiming an established, working pattern where the only other instance is a live bug — and the whole point of putting the rationale in a struct comment is that the next person wrapping a builder copies it. Please either drop the sentence or invert it, e.g. "OCCTSectionBuilder (:9877) has the same field but never resets it — see #916; do not use it as the model."

Altitude note while you're here: this is now the second wrapper in this file that needs "did the last build() on this instance succeed", and both hand-roll it. The generalizable form is a tiny shared mixin (struct BuildOutcome { bool built = false; void invalidate(); }) that every builder wrapper embeds, so the invariant is stated once and #916 becomes a one-line adoption rather than a second bespoke fix.

[secondmouseAU-bot]

8. GetStatus() is not the outcome oracle this comment implies — Build() resets myStatus on entry, so a whole class of Failed statuses is unobservable by construction.

The gate itself is sound (I re-checked V8_0_1: Build() sets myStatus = BRepFill_ThruSectionErrorStatus_Done at BRepOffsetAPI_ThruSections.cxx:341, so there is no stale-status false-negative, and the two WrongUsage early returns at :356/:373 are caught exactly as described — nice catch). Two adjacent consequences of that reset are worth recording, because the comment reads as "GetStatus() reports what Build() decided" full stop:

(a) SetCriteriumWeight sets myStatus = Failed and this bridge can never see it.

// BRepOffsetAPI_ThruSections.cxx:1665-1676
void BRepOffsetAPI_ThruSections::SetCriteriumWeight(const double W1, const double W2, const double W3)
{
  if (W1 < 0 || W2 < 0 || W3 < 0)
  {
    myStatus = BRepFill_ThruSectionErrorStatus_Failed;
    return;                        // <-- weights NOT applied
  }
  myCritWeights[0] = W1; ...

OCCTThruSectionsSetCriteriumWeight (:8782) returns void and swallows nothing (no throw is raised), so from Swift:

loft.setCriteriumWeight(w1: -1, w2: 1, w3: 1)   // silently ignored
#expect(loft.build())                            // true — Build() reset myStatus to Done first

the caller gets a loft approximated with the previous weights, build() returns true, and the new GetStatus() check cannot report it because Build() cleared the flag before it ran. If GetStatus() is now the bridge's notion of truth, this is the one caller-visible place it silently isn't.

(b) OCCTThruSectionsSetContinuity (:8728) clobbers whatever weights the caller set:

ts->builder->SetCriteriumWeight(1.0, 1.0, 1.0); // ensure defaults
ts->builder->SetContinuity(occtGeomAbsFromParametricContinuity(continuity));

so loft.setCriteriumWeight(w1: 2, w2: 1, w3: 3); loft.setContinuity(2) throws the weights away with no diagnostic. Both are pre-existing and neither is in this diff — flagging because this PR makes GetStatus() the bridge's authority on ThruSections outcomes, and these are the two places that authority is silently empty. A follow-up issue is a fine home for them.

[secondmouseAU-bot]

9. Public API changed; the /// block still has no fenced snippet (CLAUDE.md Documentation Standards).

The prose half of last round's finding 6 is fixed — thanks. The rule it cites has a second half that isn't:

Document with a runnable Swift snippet so context7 indexes it. … So when wrapping or changing a public API, give it a /// summary + parameter docs + at least one fenced ```swift snippet … Snippets are what context7 harvests — terse one-line summaries don't surface in answers.
— CLAUDE.md, Documentation Standards → Rules

Both members whose contract this PR changes ship prose only: generatedFace(from:) here (also missing - Parameter edge: / - Returns:, which docs/reference/Document-Completions.md:902-904 does carry) and shape at :57. The new contract is precisely the kind that a snippet communicates better than a paragraph, because the interesting part is a sequence:

/// - Parameter edge: A profile edge from one of the input wires.
/// - Returns: The generated face, or `nil` if `edge` isn't a profile edge of the current
///   build, or if no successful ``build()`` has happened for the builder's current sections.
///
/// ```swift
/// let loft = ThruSectionsBuilder(isSolid: true)
/// loft.addWire(bottom)
/// loft.addWire(top)
/// guard loft.build(), let edge = bottom.subShapes(ofType: .edge).first else { return }
/// let face = loft.generatedFace(from: edge)   // the side face swept from `edge`
///
/// loft.addWire(mismatchedSection)
/// _ = loft.build()                            // false
/// loft.generatedFace(from: edge)              // nil — not the previous build's face
/// ```

[secondmouseAU-bot]

10. The isRuled: true coverage added for last round's finding 4 was removed again in 7b0abee — this test is named RuledPath but never sets isRuled.

Commit 1ac2129's reply says "Fixed (finding 4): added a ruled-path variant (isRuled: true)". 7b0abee replaced it with isRuled: false + 2 sections. The comment above justifies that correctly — myWires.Length() == 2 || myIsRuled (BRepOffsetAPI_ThruSections.cxx:504) does route 2 sections to CreateRuled() — but the two dispatch arms are not equivalent inside CreateRuled():

• nbSects == 2: the history loop runs once, and myEdgeFace gets exactly the first wire's edges.
• myIsRuled with nbSects >= 3: the loop runs nbSects - 1 times (for (i = 1; i <= nbSects - 1; i++)), binding edges from every wire but the last — which is the behaviour OCCT's own doc for GeneratedFace describes ("if Ruled Returns the Face generated by each edge except the last wire"), and the arm where cross-build rebinding of the same edge actually happens.

So after this change no test reaches CreateRuled() via myIsRuled at all, and the parameter a user who wants a ruled loft actually passes is untested. Either restore the isRuled: true variant alongside this one, or rename this test (...TwoSectionPath) and update the comment so the gap is visible rather than looking covered.

Worth noting for the same reason: the #expect(loft.generatedFace(from: edge) != nil) positive assertion at :534 inherits the fragility called out in last round's finding 5, which you documented inline in the first test (:462-464) but not here or in generatedFaceNilAfterWrongUsageOnReusedBuilder (:505). Same dependency on BRepFill_CompatibleWires leaving matching-topology input edges untouched, same one-line caveat needed.

[secondmouseAU-bot]

11. @unchecked Sendable + a new unsynchronised bool read on every accessor.

built is a plain non-atomic bool written by OCCTThruSectionsBuild/AddWire/AddVertex and now read by OCCTThruSectionsShape and OCCTThruSectionsGeneratedFace. ThruSectionsBuilder declares @unchecked Sendable, so the compiler permits:

let loft = ThruSectionsBuilder(isSolid: true)
// ... sections added ...
Task { _ = loft.build() }
Task { _ = loft.shape }      // torn read of `built`; TSan data race

Before this PR the accessors read OCCT's myDone instead, which is equally unsynchronised, so this isn't a regression — but the PR is explicitly introducing bridge-owned state as the correctness mechanism, which is the moment to say what its concurrency contract is. docs/thread-safety.md and OCCTSerial already exist for exactly this. Minimum: a /// note on the class that instances are not safe to use from multiple threads without external serialisation; better: route the flag through the same serialisation the rest of the bridge uses, so @unchecked is actually earned.

[secondmouseAU-bot]

12. Simplification: the same three facts are re-explained in four places, 25 comment lines for 11 lines of code.

"IsDone() isn't a reliable last-build signal", "Build()'s WrongUsage paths skip NotDone()", and "Build() never clears myEdgeFace" are each stated more than once:

• :8658-8672 — the struct comment (16 lines), states all three.
• :8696-8698 — AddWire (3 lines), the invalidation rationale.
• :8739-8741 — Build (3 lines), re-states "IsDone() alone is not enough" + the WrongUsage returns, then says "see the struct's own comment".
• :8792-8796 — GeneratedFace (5 lines), re-states the myEdgeFace mechanism and points at both the struct comment and OCCTThruSectionsShape.

The last two both cross-reference the first, which is the right instinct — they just also duplicate what they're referencing. Collapsing them to one line each (// gate on ts->built — see the struct's comment) keeps every pointer intact and drops ~15 lines. Concrete cost of leaving it: four copies of a claim about OCCT internals that will drift apart at the next kernel pin (three of them already cite V8_0_0 line-number reasoning that V8_0_1 has moved), and the file is already at the top of comment-ratio-check.py's report.

Worth keeping in full: the "bridge-side, not a kernel patch" paragraph (:8666-8672) — that's the decision record, it isn't duplicated anywhere, and it's the part a future reader can't reconstruct. (Minus its last sentence, see the separate comment on :8672.)

[gsdali]

Pushed round 2 fixes (115f17a). Independently re-verified every finding against the pinned V8_0_1 OCCT source (not just applying the review at face value) before acting on any of them — full disposition, including where I diverged (finding 8 became its own issue rather than a quick patch once traced, finding 11 rejected as inconsistent with the rest of the codebase) is in the updated PR body. Summary:

• 1 (blocking): confirmed live via a throwaway probe before fixing — generatedFace(from:) after a success→failure→success sequence returned a face that was provably not part of the third build's own shape. Fixed with a membership check against the current Shape().
• 2, 3, 6, 7, 10, 12: fixed as described.
• 4, 5: addressed in the PR body itself (CHANGELOG/SemVer now cover the setter-invalidation change; checklist now states code-style's real status instead of implying cleanliness).
• 8: real, but a separate defect (SetCriteriumWeight itself no-ops on a negative weight, and Build() overwrites the only status that would report it) — filed as BRepOffsetAPI_ThruSections::SetCriteriumWeight silently no-ops on a negative weight, and Build() erases the only status that would have reported it #919, sub-issue of ThruSectionsBuilder.generatedFace(from:) doesn't check IsDone(), can return stale data after a failed reused build() #910.
• 9: added an in-source snippet.
• 11: not actionable in this PR — matches the existing unsynchronized-state pattern on every sibling builder class; project-wide question tracked by Bridge-level thread-handling contract: per-call safety classification, scoped/controllable internal parallelism, encapsulated global state — safety AND throughput #342.

All 5 required static gates clean, full OCCTModelingTests (645/645) and OCCTStressTests (365/365, +2) pass. code-style still fails as documented (deferred to #917, not a required check).

[gsdali]

swift build + test (macOS) investigation, resolved. This job failed 3/3 reruns on 115f17a (round 2's commit) with a double-SIGSEGV crash. Investigated thoroughly before concluding anything:

• Round 1's commit (7b0abee) passed this same job cleanly.
• Reran main's CI twice (same day, same runner pool, unrelated code) as a control — 2/2 clean.
• Ran the full local suite 7 times on this exact commit — 7/7 clean, 5537/5537 tests each.

Net: a real, CI-runner-specific signal (not general infra flakiness — the control rules that out; not a deterministic bug — 7 clean local runs rule that out), not reproducible locally, root cause not established. Filed as #920 with the full evidence rather than dismissed or left silent. swift build + test (macOS) is not a required check (gate-scripts is, and it's green), so not treating this as blocking, but flagging it plainly rather than quietly rerunning until green.

[gsdali]

Went back through this round's 12 inline comments individually rather than relying on the summary body I'd worked from — the inline comments carried more precise detail, and cross-checking each one against the actual diff (f3efeb8) turned up six real gaps between what the review asked for and what I'd actually done:

• Finding 3: only fixed 2 of the 3 doc locations it named, and missed a 4th (docs/reference/Shape-Completions.md's shape entry) with the same stale wording. Fixed both.
• Finding 6: fixed the checklist text but not what was actually asked — an inline # deferred, see #917 annotation in style-manifest-bridge.txt itself, and (separately) verified its prediction that swift-format lint --strict would fail once ThruSectionsBuilder.swift left the Swift manifest. It was right — confirmed directly and fixed.
• Finding 9: added a class-level snippet, but the ask was specifically - Parameter/- Returns + a fenced snippet on generatedFace(from:). Added.
• Finding 10: fixed the test's isRuled gap but missed its second half — the fragility caveat on the positive != nil assertion, present on the original test but not the two I touched. Added to both.
• Finding 11: responded in the PR body but didn't do the "minimum" ask — an in-source /// note that instances aren't safe for concurrent use. Added.
• Finding 12: asked to keep the "bridge-side, not a kernel patch" paragraph in full while trimming the rest — my trim dropped it entirely. Restored.

Also caught, while re-reading finding 8 closely: a second silent-discard I'd missed when filing #919 (setContinuity unconditionally resets criterium weights to 1,1,1, discarding a caller's prior setCriteriumWeight with no diagnostic) — added as a follow-up comment on #919 rather than a new issue, since it's the same investigation.

Finding 5's shared-mixin suggestion is still consciously not done (real refactor, touches #916's in-flight fix, out of proportion here) — flagging that explicitly rather than letting it look overlooked.

All 5 required gates clean (re-verified check-style-manifest.py against the actual merge-base, not the since-moved origin/main tip — local runs against a live branch ref can show spurious noise from unrelated merges). Full OCCTModelingTests/OCCTStressTests pass.