fix: face identity is IsSame, and the stale half of the shared-face test

[gsdali]

What & why

Face identity keys on OCCT's IsSame (same TShape, same Location, orientation may differ), per the decision recorded in the issue. The production code was already correct under that decision. The stale half was the test's premise, which asserted a faces()-versus-subShapes(ofType:) divergence that OCCTSwift #541/#613 erased by routing both through the same TopTools_IndexedMapOfShape.

Closes #1

This unblocks #2 (the resolver consolidation) and the rest of ecosystem#43, which were all waiting on this call being made and encoded.

Verified, not assumed

The issue asks explicitly for the alternative to be ruled out before concluding it is test-only: shellB's copy of the shared face could have been silently dropped during tessellation, which would be a real render-path bug.

It is not. Measured on the fixture, body.faceIndices carries 14 triangles over 6 ordinals. The shared face's ordinal owns 4 of them, in two vertex-disjoint groups (mesh vertices 0..3 and 12..15) sitting at identical positions, separated in the triangle stream by shellA's other two faces. The mesher walks face occurrences (occtForEachOrientedFace) and stamps each with the deduplicated map index, so both shells' copies reach the GPU under one ordinal. That is the intended behaviour.

Corroborating: compound.orientedFaces() reports 7 occurrences, with occurrence 0 and occurrence 3 both carrying index=0, which is the shared face's two owners.

What changed in the test, and why each change is a strengthening

The contract survives verbatim in intent: picks landing on either shell's copy of the shared face must resolve to the same durable identity.

• The 7-versus-6 premise is gone because that divergence is gone. Both deduplicated enumerations now answer 6. The occurrence count moved to orientedFaces() (7), which the test now asserts, so the distinction the fixture exists to exercise is still pinned down rather than deleted.
• The shared face is located by isSame(as:), not by hardcoded ordinals 0 and 3. Under dedup, ordinal 3 is simply a different face, so the old assertion was testing a relationship the model no longer has. isSame is the decision's own primitive and the assertion now survives a reordering of the fixture.
• New, and the actual regression: both shells' copies must be tessellated. Under dedup, "the two ordinals agree" is true by construction and proves nothing. What can still break silently is a shell's copy never reaching the mesh. The test groups the shared ordinal's triangles into connected components by shared mesh vertex, requires exactly two, and requires them to cover the same geometry. That geometric check reads the mesh rather than the identity table, so it is not circular with the uid assertions and it also catches a neighbour being mis-stamped with the shared ordinal.
• Every triangle of the shared face, from both copies, now goes through handlePick, rather than one triangle per ordinal.

This case was covered nowhere in the repo before. FaceIdentityTableTests.t_sharedFaceBetweenShellsResolvesToOneGraphUID asserts the count of distinct face indices, which is 6 whether or not shellB's copy exists. So the rewrite adds coverage rather than trading it away.

Also in this PR

• The decision is written down where FaceIdentityTable can be read against it (issue checkbox 1), as a doc comment on the type: the IsSame semantic, why orientedFaces() is deliberately not an identity, why two triangles sharing an ordinal can be different geometry from different shells, and why the table exists at all (it is the GPU-side stand-in for OCCT attaching a TopoDS_Shape to a sensitive entity, which we cannot do with a triangle buffer). Comment only, and OCCTSwiftTools gains no UI framework import.
• The "5 expected failures" baseline is cleared from CLAUDE.md, README.md, okf/index.md and .github/workflows/tests.yml. Leaving it would mislead the next reader exactly as it misled the last one, and tests.yml is a blocking gate that documented a red baseline.
• AIS audited against OCCTSwift v2.0.0's break table (issue checkbox 3, never done for this code). Its only contact with the table is Shape.subShape(type:index:) at three sites in InteractiveContext.swift, each a fallback behind an identity table, and #541/#613 moved both sides of that fallback onto the same enumeration so they now agree instead of diverging. #568 (an unresolvable index refuses the call) is already handled by the existing guard let / ?? chains. No use anywhere in the target of wires/shells/solids (#502), buildCurves3d (#498), AAG (#642/#699), chamfer2D (#705), the mass-property surface (#609), or any symbol removed by #784.

Checklist

• New or changed behavior is covered by a unit test in the same PR (not just manual verification), see SecondMouseAU/OCCTReconstruct#397 for the ecosystem-wide test-coverage standard this is piloting.

Notes for the reviewer

Test results. Baseline was 314 tests across 27 suites with exactly 5 failures, all in t_sharedFaceBetweenShells_everyPickResolvesToCorrectSubShape. Now:

✔ Test t_sharedFaceBetweenShells_everyPickResolvesToCorrectSubShape() passed after 0.666 seconds.
✔ Suite "InteractiveContext mutation lifecycle" passed after 0.669 seconds.
✔ Test run with 314 tests in 27 suites passed after 0.812 seconds.

Same 314 across the same 27 suites, so no test was deleted or added to reach green. Run as OCCT_SERIAL=1 swift test --parallel --num-workers 1. swift build, swift build --build-tests, swift-format lint --strict and swiftlint lint --strict are all clean.

One judgment call worth checking. The two-copies assertion groups triangles by shared mesh vertex rather than by contiguous run in faceIndices. Runs would have been shorter, and the mesher does emit one contiguous block per occurrence, but run-adjacency is an emission-order detail: if a future fixture ever put the two occurrences back to back they would merge into one run and the test would fail for the wrong reason. Vertex connectivity states the thing being claimed (two independent triangulations exist) and is order-independent.

Left alone deliberately. Remap.remapRef falls back to the graph's raw node index as an ordinal when a uid is not in the identity table (Remap.swift:109). That is pre-existing, already flagged in faceIdentityTable(for:)'s own doc comment, and belongs to the resolver consolidation in #2 rather than here.

One gap worth filing separately, as the issue notes: Shape exposes isSame(as:) and isPartner(with:) but no isEqual(as:), though the kernel has all three. Not a blocker under this decision. Not filed by this PR.

🤖 Generated with Claude Code

[kilo-code-bot]

Code Review Summary

Status: No Issues Found | Recommendation: Merge

Files Reviewed (6 files)

• .github/workflows/tests.yml - Updated baseline comment
• CLAUDE.md - Updated baseline and added face identity documentation
• README.md - Updated known issues to face identity section
• Sources/OCCTSwiftTools/FaceIdentityTable.swift - Added comprehensive doc comment explaining IsSame identity decision
• Tests/OCCTSwiftAISTests/InteractiveContextMutationTests.swift - Rewrote shared face test with stronger assertions
• okf/index.md - Updated policy note about face identity

---

_{Reviewed by nemotron-3-ultra-550b-a55b:free · Input: 109.7K · Output: 7.7K · Cached: 90.7K}