fix(#705): reject a repeated edge pair in chamfer2D instead of crashing

[gsdali]

Summary

Shape.chamfer2D(edgePairs:distances:) SIGSEGVs, uncatchably, when the same edge pair is named
twice. Found by Cluster B's census (#665, PR #704), which records the crash rather than running it
live, since an in-process OS signal would kill the census itself.

Root cause is an upstream OCCT defect, not this bridge's own:
BRepFilletAPI_MakeFillet2d::AddChamfer(edge1, edge2, ...) calls ChFi2d::FindConnectedEdges to
look up the pair's shared vertex, then dereferences the two edges it returns without checking the
returned status first. That lookup leaves both edges null on every failure path, and a pair's
second call fails it, because the shared vertex was already consumed chamfering the pair the first
time. The sibling overload, AddChamfer(edge, vertex, distance, angle), checks the identical
status correctly, which is the precedent a follow-up kernel patch PR will cite upstream. This PR
carries the immediate bridge-side guard; the kernel patch is tracked separately (matches this
repo's established pattern: bridge mitigation now, kernel patch after, e.g. #298, #341, #344, #349).

Fix: OCCTFace2DChamfer (OCCTBridge_Modeling.mm) now checks each pair against every prior
pair in the same call before invoking AddChamfer, and rejects the whole request (nil) on a
match in either order. Reusing one edge across two different pairs, e.g. chamfering adjacent
corners of a polygon, is unaffected and stays non-nil, confirmed by a positive test that chamfers
every corner of a rectangle.

Contract chosen

Reject the whole call, matching fillet2D(vertexIndices:radii:)'s own contract for a duplicated
vertex on this same BRepFilletAPI_MakeFillet2d builder (#568). fillet2D rejects incidentally,
because calling AddFillet twice on the same vertex fails Build()/IsDone() on its own;
chamfer2D cannot fail the same way, since its duplicate crashes inside OCCT before Build() ever
runs, so an explicit guard was required either way. Given that, reject is the answer already
established one call away, and it sidesteps the "which distance wins" ambiguity a
first-wins/last-wins choice would introduce with no signal to the caller.

Not a vote on #633. #633 is open on the wider fillet/chamfer family's duplicate-index
direction (fillet is last-wins, chamfer is first-wins, per Cluster B's census). This fix removes a
crash, not a contract redesign, and is recorded as a data point for that issue rather than an
attempt to settle it.

Verification (separate-process, before/after)

An in-process crash kills the test runner, so the crash itself is proven in a standalone process
(temporarily repointing Sources/OCCTTest/main.swift, restored afterward, no diff left behind):

Input	Before	After
[(0, 1), (0, 1)]	SIGSEGV, exit 139	nil, exit 0
[(0, 1), (1, 0)] (reversed, same pair)	SIGSEGV, exit 139	nil, exit 0
[(0, 1), (0, 1), (0, 1)] (triple)	SIGSEGV, exit 139	nil, exit 0
[(0, 1), (1, 2)] (one edge, two different pairs)	non-nil, unaffected	non-nil, unaffected
[(0, 1), (1, 2), (2, 3), (3, 0)] (every corner)	non-nil, unaffected	non-nil, unaffected

Injection matrix (prove-the-test-fails)

• The crash itself: proven only by the separate-process exit codes above. An in-process
  @Test cannot assert a SIGSEGV without killing the whole suite, which is exactly the shape of
  defect okf/policies/prove-the-test-fails.md and this repo's own precedent (Uncatchable SIGSEGV in parallel swift test run, right after concurrent OBJ imports — possibly related to #341, unconfirmed #344/Uncatchable SIGABRT in parallel swift test run — cause unknown, very little evidence (see #344 for the companion SIGSEGV) #345) call for
  external evidence, not an in-process red/green cycle.
• chamfer2DRejectsDuplicatePair (new, in-process, asserts the fixed behaviour only):
  before the fix landed, red would mean a crash, not an assertion failure, so this test's own
  red/green cycle is the table above.
• chamfer2DAcceptsSharedEdgeAcrossDifferentPairs (new, in-process, proves the guard keys on
  the pair, not either index alone): injected a plausible wrong fix (reject on any repeated
  single index) and confirmed it turns this test red (2 assertion failures), then restored the
  correct pair-based guard and confirmed green again.

Neither existing assertion in Tests/OCCTModelingTests/Issue568IndexSkipTests.swift was changed;
both new tests are additions after the existing chamfer2DAcceptsResolvablePairs.

Docs

• Sources/OCCTSwift/Shape+Geom2d.swift: /// note + a fenced swift snippet showing the
  refused duplicate.
• docs/reference/Shape-Measurement.md: matching reference-page note.
• docs/SEMVER.md: recorded exception, a call that used to crash now returns nil.
• docs/CHANGELOG.md: full writeup under ## Unreleased.
• Scripts/repro/cluster-b-fillet-edge-contract/README.md and Scripts/repro/censuses/ClusterB.swift:
  the chamfer2D duplicate-pair row is now measured live (safe, since it no longer crashes) instead
  of being noted as unsafe to run; row count unchanged at 16.

What the issue and prior measurement got wrong

• The issue's own root-cause guess ("AddChamfer rebuilds the face incrementally, so..stale edge
  handles") was a reasonable, explicitly-hedged hypothesis, not a confirmed mechanism. The actual
  upstream root cause (above) is different: it is an unchecked failure status on a connectivity
  lookup, not staleness from incremental rebuilding. Every doc/comment in this PR that touches the
  mechanism states the corrected version.
• The census's own static classifier (classify_fillet_sites.py) still reports OCCTFace2DChamfer
  as OTHER for index handling, a known, already-documented blind spot (it does not recognise the
  occtMappedSubShapeAt-based reject idiom). Unaffected by this fix and left as-is per the
  existing README note.

Verify

• Clean swift build, 0 errors, no new warnings, no OCCTSWIFT_LOCAL.
• Full swift test: 5,348 passed (baseline 5,346 plus these 2 new tests), 0 failures.
• swift run Censuses cluster-a unchanged at 45 rows; cluster-b unchanged at 16 rows (one cell
  changed, no rows added/removed).
• Gate scripts: check-bridge-index, check-null-handle-guards, check-docs-defaults each with
  and without --self-test, and count-operations, all exit 0.
• Zero em-dashes in this diff.

Closes #705

[gsdali]

Independent verification

Reproduced the fixed behaviour in a separate process, since the crash cannot be asserted
in-process:

A dup pair      : nil        (I measured SIGSEGV, exit 139, before this)
B swapped dup   : nil        order independent, as claimed
C shared edge   : non-nil    (0,1) and (1,2) share edge 1, still works
D every corner  : non-nil    all four corners of a rectangle
exit: 0

Row C is the one that makes the guard correct rather than merely safe. Keying on the individual
index instead of the pair would have broken chamfering every corner of a rectangle, which is the
ordinary case. Proving that by injecting the wrong fix, rejecting on any repeated single index,
and watching the new test go red is better evidence than asserting the right one passes.

Verified: full suite 5,348 tests, 0 failures, censuses still 45 and 16 rows, four gate scripts
and their three --self-tests green, zero em-dashes.

On the test file I flagged. Issue568IndexSkipTests.swift gains 30 lines and changes none, so
the concern about silently editing a landed test does not apply. The force-unwrapped fixtures match
that file's own dominant idiom, 16 trailing force-unwraps against 7 guard-lets, including in the
pre-existing tests directly above.

The root-cause correction is the part worth keeping

The report notes the issue's stale-handle hypothesis was a hedged guess and root-caused it properly
instead: AddChamfer(E1, E2, ...) dereferences ChFi2d::FindConnectedEdges's output without
checking the status, and that function leaves both edges null on every failure path. That matches
what I found reading the same files, arrived at independently, and the docs and comments that
stated the old hypothesis as fact were corrected rather than left. A plausible-sounding wrong
mechanism in a shipped comment is worse than no comment, because the next reader trusts it.

The reachability check came back positive too: OCCT's own DRAW chfi2d command hits the identical
crash naming the same two edges twice. That is what makes the upstream filing a report about OCCT
rather than about our usage.

[secondmouseAU-bot]

Review

Repo: SecondMouseAU/OCCTSwift · fix/705-chamfer2d-duplicate-pair-crash → refactor/381-pass1b · +205/−21 across 8 files · CI green (build + test macOS, iOS sim smoke, gate-scripts)

Overview

Shape.chamfer2D(edgePairs:distances:) SIGSEGVs uncatchably when an edge pair is named twice (order-independent). Root cause is upstream: BRepFilletAPI_MakeFillet2d::AddChamfer dereferences the edges returned by ChFi2d::FindConnectedEdges without checking its status, and the pair's second call fails that lookup because the shared vertex was already consumed. The fix adds an O(n²) prior-pair scan in OCCTFace2DChamfer (OCCTBridge_Modeling.mm) that rejects the whole call (nullptr) the moment any two entries name the same pair in either order, before AddChamfer ever touches the second one. Docs, the Cluster B census, and two new tests are updated in the same PR.

Code correctness

• The guard is correct: it compares raw input indices (not resolved shapes), canonicalizes order via sameOrder/swappedOrder, and runs before AddChamfer but after e1/e2 are resolved from the original (unmutated) edgeMap — consistent with the stated root cause.
• Legitimate reuse of one edge across two different pairs (e.g. (0,1) then (1,2)) is correctly left untouched — the check keys on the pair, not either index alone.
• The nested j < i loop is O(n²) in edgePairs.count, but that's bounded by face edge count in realistic geometry, so it's a non-issue in practice.

Minor issue — stale/forward-referencing comment

The new comment in OCCTBridge_Modeling.mm says:

A kernel patch is carried separately (Scripts/patches/0022-*); this guard is what protects callers until it ships.

Scripts/patches/ only goes up to 0021-... as of this diff — no 0022-* file exists yet, and the PR description itself frames the kernel patch as future work ("a follow-up kernel patch PR will cite upstream"). Worth softening to "tracked in a follow-up patch, not yet filed" so a future reader doesn't go looking for a file that isn't there.

Test coverage

• chamfer2DRejectsDuplicatePair covers exact-duplicate, reversed-duplicate, and triple-duplicate.
• chamfer2DAcceptsSharedEdgeAcrossDifferentPairs is the important negative-space test — pins that keying on the pair (not either index) doesn't break the ordinary multi-corner case; per the other review comment it was proven to catch an overly-broad "reject on repeated single index" fix by going red first.
• Crash verification is necessarily out-of-process (SIGSEGV can't be asserted in-process), which is reasonable and precedented in this repo.

Style / convention

• Matches the established "bridge mitigation now, kernel patch tracked separately" pattern (#298/#341/#344/#349), and explicitly avoids re-litigating #633 (first-wins vs last-wins) — good scoping discipline.
• Doc updates (Swift ///, Shape-Measurement.md, CHANGELOG.md, SEMVER.md) are thorough; the SEMVER anchor link resolves correctly against the new heading.
• Correcting the earlier (now-known-wrong) "incremental rebuild / stale handle" hypothesis in ClusterB.swift and its README, rather than leaving the superseded theory in place, is good hygiene.

Security / risk

• This closes a real crash/DoS vector: any path that constructs edgePairs from user input, a selection, or a loop (e.g. a NL→CAD pipeline) could previously be handed an uncatchable process kill from a duplicated pair. Converting that to a clean nil, recorded as a SemVer exception, is a meaningful robustness fix.
• No new unsafe memory access or external input parsing — pure integer comparison over already-validated indices.

Verdict: Solid, well-scoped crash fix with correct root-cause analysis, adequate test coverage (including a proven-to-catch-the-wrong-fix negative test), and thorough doc/changelog hygiene. The only loose end is the Scripts/patches/0022-* comment reference, which is cosmetic.

🤖 Generated with Claude Code

[gsdali]

Fixed, though not quite the way you suggested.

The comment did point at Scripts/patches/0022-*, which this PR does not create, so reading this commit alone sends you after a file that is not there. Real finding.

Your suggested wording was "tracked in a follow-up patch, not yet filed". That would be wrong in the other direction: it is filed. OCCT#1431 carries the reproducer and OCCT#1432 the fix, both opened before this review. The comment now cites those instead, which are real and stable regardless of merge order, and states that the patch is inert until the pinned xcframework is rebuilt so the reason this guard exists stays on the record.

Also worth correcting one thing in the overview: the O(n²) scan is bounded by edgePairs.count, the caller's own list, not by the face's edge count. A caller can name the same pair a thousand times. Still a non-issue, since the guard rejects on the first repeat and the list is caller-authored, but the bound is the argument list rather than the geometry.

13 tests in the suite pass, four gate scripts green, zero em-dashes.