fix(#702): analyze()'s free-edge detection was dead code; isValidSolid already answers the demotion question

[gsdali]

What & why

Shape.healed() and Shape.fixSolid() can reach isValid == true by demoting a solid to a
shell (ShapeFix_Solid hands back the shell unpromoted whenever it cannot close it, already
documented for fixSolid() since #442; a shell has no closure requirement of its own, so the
demoted result is genuinely valid). #702 reported this as invisible to every diagnostic: isValid
true, volume correct, analyze() clean.

The issue's own ThruSections/bevel-gear reproducer does not reproduce on this branch (OCCT
8.0.1 + patches vs the issue's v1.17.0). Three independent, faithful reconstructions, including a
port of OCCTSwiftScripts/recipes/04-spur-gear's involute math matching the issue's own
1152-points-per-wire figure exactly, across 400+ parameter combinations, plus source-reading
ShapeFix_Solid, all confirm nothing is broken there today. No manufactured fix for a defect that
is not reproducing.

What is real and fixed: Shape.analyze(tolerance:)'s free-edge/free-face detection was dead
code. OCCTShapeAnalyze called ShapeAnalysis_Shell::LoadShells(), which only does bookkeeping
and runs no edge analysis, instead of CheckOrientedShells(), which actually populates the
free-edge set (the sibling entry point OCCTShapeAnalyzeShell already calls it correctly).
freeEdgeCount was hardcoded to 0 for every shape, and freeFaceCount's counter variable was
never incremented at all. Fixed; both now report accurately.

Shape.isValidSolid already answered the issue's actual question, unaffected by either bug
(added for #206/#208, unrelated): it checks shapeType == .solid before BRepCheck_Analyzer, so
it reads false on a demoted shell where plain isValid reads true. It had no test for that
branch; added. healed() and fixSolid()'s docs now cross-reference it.

Closes #702

Checklist

• New/changed behavior is covered by a unit test in the same PR:
  Tests/OCCTShapeHealingTests/Issue702SolidDemotionTests.swift, 10 cases.

Notes for the reviewer

Reproduction status, in detail. The issue's reproducer was measured on OCCT v1.17.0. Tried on
this branch's kernel (8.0.1 + eleven carried patches):

• Sharp-cornered gear-tooth polygons (3-72 teeth), tapered and twisted, up to 6 sections.
• A dense sinusoidal profile at various tooth counts, point densities and amplitudes.
• Deliberately self-intersecting star prisms twisted up to 179 degrees (ruled and smoothed).
• A faithful port of OCCTSwiftScripts/recipes/04-spur-gear's involute tooth math, scaled per
  section for a bevel taper (with and without a spiral-twist variant), matching the issue's own
  1152-points-per-wire figure exactly at flankSamples = 14.

Over 400 parameter combinations across these four families, every raw ThruSectionsBuilder
result was BRepCheck_Analyzer-valid, so nothing ever reached healed()/fixSolid() in a state
that could demote. Separately: BRepCheck_Analyzer does not flag 3D self-intersection between
non-adjacent faces at all (the deliberately self-intersecting twisted prisms above all report
isValid == true), which matches analyze()'s own selfIntersectionCount field being a
permanent, undocumented-until-now 0. Source-reading ShapeFix_Solid::Perform(),
CreateSolids(), and CollectSolids() (all three demotion code paths, single-shell and
multi-shell) found every one gated on a genuine closure check (BRep_Tool::IsClosed/
ShapeAnalysis_FreeBounds), matching #442's already-documented and already-tested contract. I
could not construct any case where a topologically closed shell gets wrongly demoted.

Whose bug is the free-edge/free-face defect? Ours, in the bridge, not OCCT. OCCTShapeAnalyze
called the wrong ShapeAnalysis_Shell method (LoadShells() instead of CheckOrientedShells());
OCCT's own API works correctly when called right, which the sibling OCCTShapeAnalyzeShell
already demonstrates. No kernel patch, no upstream filing.

The contract chosen for healed()/fixSolid() demotion itself: preserve, not report or
reject, because it already was, correctly, since #442; this PR doesn't revisit that decision.
Refusing (returning nil/the original invalid solid) would reverse a deliberate #442 decision
("no body is ever dropped to make [isValid] true") for no new reason. Reporting (a new
WithReport-style API, matching #639/#709's precedent) would be redundant: isValidSolid already
exists, already answers the question correctly (verified directly, unaffected by the
analyze() bug), and just needed cross-referencing and a test.

Injection matrix (Tests/OCCTShapeHealingTests/Issue702SolidDemotionTests.swift, all 10
cases, bug reintroduced by reverting OCCTShapeAnalyze to shellAnalysis.LoadShells(shell)):

test	bug present	fixed
analyze reports the open shell's free edges, matching analyzeShell	FAIL	PASS
analyze reports the demoted fixSolid shell's free edges too	FAIL	PASS
analyze counts free faces per shell on a compound of two open shells	FAIL	PASS
analyze reports zero free edges/faces on a genuinely closed solid	PASS	PASS
the fake solid is BRepCheck-invalid before healing	PASS	PASS
fixSolid demotes the fake solid to a shell	PASS	PASS
healed demotes the fake solid to a shell	PASS	PASS
isValid alone does not distinguish a demoted shell from a healthy solid	PASS	PASS
isValidSolid is false on a demoted shell, true on a genuine solid	PASS	PASS
isValidSolid is false on non-solid shape types generally	PASS	PASS

The three tests targeting the actual bug fail precisely when it's present; the other seven
(testing the already-correct demotion mechanism and the already-correct isValidSolid) stay
green throughout, confirming the new tests are isolated to the bug they exist to catch.

docs/SEMVER.md: no entry added. This is a bug fix to the values analyze(tolerance:)
returns, not a new field, removed field, or signature change; per the file's own quick-reference
table this is the PATCH case ("a wrong value repaired"), matching the precedent of the
#605/#609/#583/#595 fabricated-zero fixes, none of which are recorded exceptions either. Rechecked
the "thirteen recorded exceptions" counters at the top of the file: still accurate (3 compile-break

• 10 behavior-only), unchanged by this PR.

What the issue got wrong (its own numbers are from v1.17.0, expected to have moved): the
specific bevel-gear reproducer no longer produces an invalid raw solid on this branch, so the
premise "the builder does produce a solid [that's invalid]" no longer holds here. The issue's
"analyze() ... no self-intersections" evidence was inadvertently reporting a permanently-stubbed
field, not a measured absence, both before and after this fix (now documented rather than silent).

Verified: clean swift build, 0 errors, no new warnings, no OCCTSWIFT_LOCAL. Full swift test: 5356 baseline + 10 new = 5366, 0 failures. Censuses cluster-a 45 rows, cluster-b 16
rows (both unaffected by this change, confirmed unchanged). Four gate scripts and their three
--self-tests all green. Zero em-dashes in the diff.

[secondmouseAU-bot]

Automated code review (xhigh effort)

11 findings verified, collapsing to 7 distinct defects. Most severe: a real correctness bug at the core of the #702 fix itself — OCCTShapeAnalyze's new CheckOrientedShells call omits checkinternaledges=true (unlike its sibling OCCTShapeAnalyzeShell), so analyze()/analyzeShell() can silently disagree, contradicting this PR's own "must agree" test invariant. Verified directly against OCCT's ShapeAnalysis_Shell.cxx source.

---

1. Sources/OCCTBridge/src/OCCTBridge_Healing.mm:181 — correctness (CONFIRMED)

analyze() omits checkinternaledges that analyzeShell() passes for the same check.
The #702 fix calls CheckOrientedShells(shell, /*alsofree*/ true), leaving checkinternaledges at its default false, while the sibling OCCTShapeAnalyzeShell passes CheckOrientedShells(shape->shape, true, true). For a shell containing a TopAbs_INTERNAL-oriented edge, analyze().freeEdgeCount/freeFaceCount can silently disagree with analyzeShell()'s numbers — directly contradicting the new test's own assertion that the two "must agree." The test fixture (a box missing one face) has no internal-orientation edges, so it can't catch this.

2. Sources/OCCTSwift/ShapeAnalysisResult.swift:40 — correctness (CONFIRMED)

totalProblems now double-counts a single open-shell defect (once per free edge via freeEdgeCount, again as +1 via freeFaceCount) now that #702 makes both fields live simultaneously for the first time. For the PR's own fixture, analyze() returns freeEdgeCount=4/freeFaceCount=1 for one defect, so totalProblems=5. The new tests never assert totalProblems's magnitude, so this newly-live double-counting went unverified.

3. docs/guides/cookbook/healing-and-validity.md:83 — correctness (CONFIRMED)

The #702 solid-to-shell demotion warning was added only to healed()'s bullet, not to fixed(tolerance:...)'s or upgraded(tolerance:)'s, even though both are confirmed to run the identical ShapeFix_Shape mechanism that causes the demotion. A caller reading only those docs gets no warning that a solid input can come back as a shell reading isValid == true — the exact bug #702 was filed over.

4. Sources/OCCTBridge/src/OCCTBridge_Healing.mm:182 — correctness (PLAUSIBLE)

Replacing the near-no-op LoadShells(shell) with computational CheckOrientedShells adds a new exception path with no per-shell try/catch, so one OCCT exception now discards the whole analysis (all counts reset to 0, isValid=false) instead of just the free-edge count, for any shell in a multi-shell shape that raises Standard_Failure.

5. Sources/OCCTBridge/src/OCCTBridge_Healing.mm:182 — cleanup (CONFIRMED)

The free-edge/free-face scan is hand-duplicated between OCCTShapeAnalyzeShell and this loop in OCCTShapeAnalyze instead of factored into one shared helper — which is exactly how finding #1's parameter divergence happened (one copy was updated, the other wasn't).

6. Sources/OCCTBridge/include/OCCTBridge_Healing.h:26 — cleanup (CONFIRMED)

New field comments use -- as a sentence dash three times (lines 26, 32, 35; also OCCTBridge_Healing.mm:172 and Issue702SolidDemotionTests.swift:111), violating the repo's no-em-dash writing-style policy — contradicting the PR's own claim of "zero em-dashes in the diff" (true only for the literal — character, not the banned -- variant).

7. Tests/OCCTShapeHealingTests/Issue702SolidDemotionTests.swift:37 — cleanup (CONFIRMED)

The new @Suite display name reads "solid demotion is honestly reported", using the banned word "honestly" per the writing-style policy's hedge-language ban — appears verbatim in every CI test-report run.

---

🤖 Generated by an automated xhigh-effort multi-agent code review (Claude Code)

[secondmouseAU-bot]

Corrected automated code review (xhigh effort)

⚠️ This corrects the review posted above. That review's content did not match the findings verified by this run's actual review pipeline — only 2 of its 7 points overlap with what's below (the checkinternaledges omission and the totalProblems double-count); the rest (a docs gap on fixed/upgraded, a missing per-shell try/catch, duplicated scan logic, and two writing-style nits) were not part of the verified output and should not be treated as this run's findings. The 7 findings below are the actual verified output.

Most severe: a real correctness bug at the core of the #702 fix itself.

---

1. Sources/OCCTBridge/src/OCCTBridge_Healing.mm:182 — correctness (CONFIRMED)

New CheckOrientedShells call omits checkinternaledges=true.
The #702 fix's new call omits the third checkinternaledges argument that the sibling OCCTShapeAnalyzeShell passes explicitly as true, so for a shape whose shell contains a TopAbs_INTERNAL/EXTERNAL-oriented edge, Shape.analyze(tolerance:) can silently disagree with Shape.analyzeShell() on free-edge counts — directly contradicting the invariant the PR's own new regression test's name asserts ("analyze() must agree with analyzeShell()"). That test only passes because its fixture happens to contain no internal-oriented edges.

2. OCCTBridge_Healing.mm:182 — correctness (CONFIRMED)

CheckOrientedShells' bad-orientation return value is discarded.
The new call's boolean return value, which reports whether the shell has badly-oriented edges, is discarded entirely, so a real per-shell orientation defect the scan now computes is silently thrown away instead of surfaced in OCCTShapeAnalysisResult. A shape with a non-manifold shell but zero free edges could report isHealthy == true despite ShapeAnalysis_Shell itself having just determined it has bad-oriented edges.

3. Sources/OCCTSwift/ShapeAnalysisResult.swift:40 — correctness (CONFIRMED)

totalProblems double-counts free edges and free faces.
totalProblems sums freeEdgeCount and freeFaceCount together, so a single open shell's defect is now counted twice, now that #702 makes these fields non-zero for the first time. For the PR's own test fixture (4 free edges, 1 free face from one open shell), totalProblems == 5 for what is really one defect.

4. Sources/OCCTSwift/Shape.swift:2035 — correctness (CONFIRMED)

isValidSolid doc comment missing the required fenced Swift snippet.
isValidSolid's doc, including the new #702 demotion paragraph appended in this PR, has no fenced swift snippet anywhere — the same Documentation Standards violation already present on isValid and healed() two properties earlier.

5. Tests/OCCTShapeHealingTests/Issue702SolidDemotionTests.swift:42 — cleanup (CONFIRMED)

New open-shell fixture duplicates an existing one.
openShellMissingOneFace() re-implements the "drop one face, sew" open-shell fixture that Issue442FixSolidMultiBodyTests.swift already builds inline in the same test target, instead of reusing/extracting it.

6. Shape.swift:754 — cleanup (CONFIRMED)

isValid doc expanded but missing a fenced Swift snippet.
isValid's doc comment is rewritten from a one-liner into a multi-sentence explanation of the #702 demotion hazard with no fenced swift snippet, against the repo's Documentation Standards rule.

7. Shape.swift:770 — cleanup (CONFIRMED)

healed() doc's new Warning block missing a Swift snippet.
healed() gains a new multi-sentence Warning block about solid-to-shell demotion but still carries no fenced swift snippet, against the same rule — despite being exactly the high-traffic API the rule calls out for an in-source snippet.

---

🤖 Generated by an automated xhigh-effort multi-agent code review (Claude Code) — corrected pass

[gsdali]

Addressed all 7 findings from the automated review (verified each independently against ShapeAnalysis_Shell.hxx/.cxx and TopoDS_Builder.hxx in Libraries/occt-src/, not just applied as described).

1. checkinternaledges divergence (most severe, confirmed). ShapeAnalysis_Shell.cxx: with checkinternaledges false, a FORWARD/REVERSED edge with no opposite-orientation partner is unconditionally free; with it true, an edge that also occurs TopAbs_INTERNAL elsewhere in the same shell (matched by IsSame: same TShape and Location, ignoring orientation) is read as connected through that occurrence instead. OCCTShapeAnalyzeShell already passed true; OCCTShapeAnalyze's per-shell loop now does too.

New fixture, since the original box-missing-one-face fixture has no INTERNAL edges and can't catch this: a single square face wrapped directly as a shell (TopoDS_Builder::MakeShell/Add, no sewing), with an .internal-oriented duplicate of one of its own boundary edges embedded back into the face before the face joins the shell. Two things needed measuring rather than assuming, both confirmed empirically before landing:

• TopoDS_Builder::Add's own contract ("Only WIRE and VERTEX can be added in a FACE") rejects a bare edge added straight into a face or shell; the duplicate has to go in via a one-edge wire.
• A plain box face, already owned by its box solid, refuses a second builderAdd (TopoDS_FrozenShape) the same way a shell face refused it after sewing, which is why the fixture builds its own face from scratch rather than reusing a box face or a post-sew shell face.

2. totalProblems double-count. Decision: freeFaceCount is excluded from the sum. It's a derived summary of the same scan freeEdgeCount already counts (never nonzero unless freeEdgeCount is), not an independent defect category, so including both counts one open shell's boundary gap twice. It stays a public field for callers who want the shell-level breakdown. Also caught while writing the fixture: the box-missing-one-face fixture measures a nonzero gapCount (5), a pre-existing, unrelated characteristic, so the new assertions recompute the expected sum from the actual fields rather than hardcoding a number that would have been wrong for a reason unrelated to this bug.

3. Docs gap (confirmed). Read OCCTShapeFixDetailed and OCCTShapeUpgrade in the bridge directly: both end in ShapeFix_Shape fixer(...); fixer.Perform();, the identical mechanism healed() uses. Cookbook now carries the demotion warning on fixed(tolerance:) and upgraded(tolerance:) too, not just healed().

4. Missing per-shell try/catch. Added: CheckOrientedShells is a real computation now, unlike the LoadShells() it replaced, so a Standard_Failure from one malformed shell no longer escapes to the function's outer catch and discards every other shell's count plus the small-edge/small-face/gap counts computed afterward. This is a defensive fix only, not a reproduced bug: I could not construct a shape that makes CheckOrientedShells/CheckEdges actually throw. Every path in CheckEdges is pure topology (shape-map hashing and orientation comparison, no geometric evaluation), and TopoDS_Builder::Add's own type-checking already prevents building the kind of malformed structure that might trigger a downcast failure. Left in as cheap, correct insurance matching every other bridge function's own convention, not because I found a live case.

5. Shared helper. occtAnalyzeShellOrientation (returns checkResult/hasFreeEdges/hasBadEdges/hasConnectedEdges/freeEdgeCount) is now the only place that constructs a ShapeAnalysis_Shell and calls CheckOrientedShells. Both OCCTShapeAnalyze's per-shell loop and OCCTShapeAnalyzeShell call it; the argument can't drift between them again by construction. Picked up a latent nullptr guard on OCCTShapeAnalyzeShell while rewriting it (it dereferenced shape->shape unconditionally before; every sibling function in the file already guards).

6/7. Writing style. Swept the whole diff programmatically (not just the lines the review named) for the banned sentence dash and the hedge word the policy bans. Fixed all instances, including two the review did not name: one in OCCTBridge_Healing.mm at line 177 (pre-existing, in a comment I was already touching) and one in docs/reference/Shape-Features.md's prose describing the free-edge count (the review only flagged the @Suite string, not this second use of the same word). Renamed the suite to "Issue 702: solid demotion is reported accurately".

Injection matrix (prove-the-test-fails):

Test	Injection	Result
analyzeAgreesWithAnalyzeShellOnInternalDuplicate (new)	Reverted OCCTShapeAnalyze's per-shell loop to a raw CheckOrientedShells(shell, /*alsofree*/ true) call (the pre-fix, checkinternaledges-omitted shape), bypassing the shared helper	Failed: analysis.freeEdgeCount → 4 vs shellAnalysis.freeEdgeCount → 3
analyzeAgreesWithAnalyzeShellOnInternalDuplicate (new)	Restored	Passed
analyzeReportsFreeEdgesOnOpenShell, analyzeReportsFreeEdgesOnDemotedShell, analyzeCountsFreeFacesPerShell (existing, new assertions added)	Reinstated + freeFaceCount in totalProblems's formula	All 3 failed (e.g. totalProblems → 10 == expected → 9, and the != check literally equal)
Same 3 tests	Restored	All passed

Full swift test: 5374 tests pass. All 4 static gate scripts (check-bridge-index, check-null-handle-guards, check-docs-defaults, derive-bridge-header-split --verify) and their --self-test variants pass. count-operations.py unaffected (no public API surface added or removed).

Not touched: no docs/SEMVER.md entry (unreleased, ships in v2.0.0). Scripts/tsan-stress.sh not run: nothing here touches concurrency or shared/global state.

[gsdali]

Verified independently. All 7 addressed, and I reproduced the decisive injection myself.

I re-ran finding 1's injection rather than taking the matrix on trust. Reverting
OCCTBridge_Healing.mm:173 to the omitted-argument form:

✘ "analyze() agrees with analyzeShell() even with an INTERNAL-oriented duplicate free edge"
   Expectation failed: (shellAnalysis.freeEdgeCount → 4) == 3

Restored, 11/11 green. So the fixture genuinely distinguishes the two argument values, with exactly
the 4-against-3 the report claims. That is the part that mattered: the previous fixture (a box
missing one face) could not catch this, so a test asserting the two entry points "must agree" was
passing without exercising the thing it names.

The two empirical constraints found while building it, that TopoDS_Builder::Add rejects a bare
edge into a face or shell and that a shape already owned by a parent is frozen against further
Add, are worth having written down in the fixture's comment. That is the kind of detail the next
person building an INTERNAL-orientation fixture would otherwise rediscover.

Also confirmed:

• Finding 5's shared helper is real. occtAnalyzeShellOrientation now backs both entry points,
  so the divergence that caused finding 1 cannot recur by one copy being updated and the other not.
  Fixing the parameter without factoring the duplication would have left the mechanism in place.
• Finding 2's decision is justified rather than re-pinned. Excluding freeFaceCount from
  totalProblems on the grounds that it is a derived summary of the same scan, and cannot be
  nonzero unless freeEdgeCount is, is the right call. Noticing that the fixture also carries a
  pre-existing nonzero gapCount, and computing the expected sum from the real fields instead of
  hardcoding, is what stops the test from pinning a coincidence.
• Finding 4 is correctly labelled defensive-only. Saying plainly that no throwing repro could be
  constructed, because every path in CheckEdges is pure topology, is better than a fabricated one.
  The guard is still worth having; the report just does not claim more for it than it has.
• Findings 6 and 7: the suite display name no longer carries the banned word, and sweeping the
  diff programmatically caught two instances the review had not named.

An independent finding in the diff worth calling out

The new documentation on selfIntersectionCount records that the field has never been computed
and is always 0, and that this is an unimplemented absence rather than a measured one. That was
not in the review and is not part of #702. It is a field that has been silently returning a
reassuring number, which is the same species of defect as #702 itself. Pointing callers at
Shape.isSelfIntersecting(timeout:) is the right disposition for this PR, but that field being
permanently 0 while looking like a measurement probably deserves either removal or its own issue
before v2.0.0 ships. Flagging rather than filing, since it is your call whether it goes.

CI

gate-scripts and iOS smoke green; macOS still running as of this comment.

[gsdali]

Addressed the second (corrected) review's 4 remaining findings, in 10ea759. Findings 1-3 (checkinternaledges, discarded bool return, totalProblems double-count) were already fixed in bbb0c60 before this pass.

Finding 4/6/7 (missing fenced Swift snippets on isValidSolid, isValid, healed()): verified independently before touching anything. isValid and healed() had zero fenced snippets, confirmed. For isValidSolid, I had a prior note suggesting it already had one; git diff against the pre-#702 base showed it never did, before or after that PR's changes, so the review's claim was correct on all three. Added a runnable snippet to each, all illustrating the actual demotion hazard the surrounding prose describes (box missing one face -> sewn -> solidFromShells -> healed()/fixSolid() demotes to a shell that reads isValid == true but isValidSolid == false). Ran each snippet's literal code as a throwaway test against the real build (3/3 passed) before writing it into the doc comment, then deleted the throwaway test (not committed).

Finding 5 (openShellMissingOneFace() fixture duplication): confirmed real. Issue442FixSolidMultiBodyTests.swift's openShellDoesNotPerturbParity() test built the identical "drop one face from a box, compound the remaining five, sew" recipe inline. Extracted to a new shared file, Tests/OCCTShapeHealingTests/ShapeHealingTestFixtures.swift (sewnBoxMissingOneFace(_:tolerance:)), following the existing precedent of Tests/OCCTStressTests/StressTestFixtures.swift for sharing fixtures within one test target. Parameterized on the box itself so each suite keeps its own box size/origin (10x10x10 at origin for #702, a 40x40x40 "bigBox" for #442) rather than forcing one shape on both. Both suites re-run and pass unchanged after the refactor.

No new tests were added (doc snippets + a fixture refactor), so prove-the-test-fails injection doesn't apply here; instead both affected suites and the full suite were re-run to confirm the refactor is behavior-preserving.

Verification:

• swift build (full, env -u OCCTSWIFT_LOCAL): clean
• swift test --filter Issue442FixSolidMultiBody: 16/16 pass
• swift test --filter Issue702SolidDemotion: 11/11 pass
• Full swift test (env -u OCCTSWIFT_LOCAL): 5374/5374 pass (unchanged count from before this commit)
• All 5 static gate scripts + their --self-test variants: clean

CHANGELOG updated under the existing #702 entry with a note on this second, corrected review pass.

No docs/SEMVER.md exception needed or added; nothing here is a public API break, only doc comment additions and a test-only refactor.