chore(#705): carry the upstream ChFi2d_Builder::AddChamfer kernel patch

[gsdali]

Summary

Second PR in this workstream, after #706 (the bridge-side crash fix). Root-caused the
chamfer2D SIGSEGV (#705) upstream: it is a genuine OCCT defect, not a bridge bug, and this PR
carries the kernel patch and files it upstream, following this repo's established
bridge-mitigation-then-kernel-patch pattern (#298, #341, #344, #349).

Root cause

ChFi2d_Builder::AddChamfer(const TopoDS_Edge& E1, const TopoDS_Edge& E2, double D1, double D2)
(ChFi2d_Builder_0.cxx) calls ChFi2d::FindConnectedEdges to look up the pair's shared vertex and
dereferences the two edges it returns without checking the returned status first:

TopoDS_Edge EE1, EE2;
status = ChFi2d::FindConnectedEdges(newFace, commonVertex, EE1, EE2);
if (EE1.IsSame(E2))   // no status check first

FindConnectedEdges leaves both edges null on every failure path, and a pair's second call fails
it, because the pair's shared vertex was already consumed chamfering the pair the first time (the
first call's own BuildNewWire rebuilds the face's wire, replacing that vertex). The sibling
overload, AddChamfer(edge, vertex, distance, angle), calls the identical FindConnectedEdges and
checks the status correctly.

The two checks requested before writing the patch

1. How the five guarded call sites return. Every guarded FindConnectedEdges call site in
   this file (the sibling AddChamfer overload, both ModifyChamfer overloads, and both call
   sites inside RemoveChamfer) declares a default-constructed null value early and returns it
   verbatim on ChFi2d_ConnexionError. AddChamfer(E1, E2, D1, D2) already has such a variable,
   chamfer, declared on entry and already used as this function's own "declined" signal on three
   earlier refusal paths in the same function. The patch returns that existing value rather than
   inventing a new one.
2. Whether OCCT's own tooling can reach the unguarded path. Yes.
   BRepTest_Fillet2DCommands.cxx's DRAW chfi2d command loops over edge-name pairs read from the
   command line and calls this same two-edge overload once per pair, so
   chfi2d result face e1 e2 CD 1 1 e1 e2 CD 1 1 (naming the same two edges twice in one
   invocation) reaches the identical crash through OCCT's own tooling, not just this bridge.

The patch

Scripts/patches/0022-ChFi2d_Builder-AddChamfer-connexion-error-check-705.patch: four lines,
adding the same status check immediately after FindConnectedEdges, matching the sibling
overload's idiom line for line.

Verification (override-link, no full rebuild)

Per the override-link technique documented in Scripts/patches/README.md's #0001 entry, which
the new 0022 entry cites. (docs/guides/building-occt.md mentions override-link only as a
contamination hazard and mandates the final proof with no override-linked TUs, so it is not the
source for the technique.) The single patched .cxx compiled
standalone (-O0 -DNDEBUG -DNo_Exception -DOCC_CONVERT_SIGNALS, matching the production kernel's
own defines) and linked before -lOCCT-macos, so the linker resolves this TU's symbols from the
override rather than the stock archive member.

	stock	patched (override-linked)
first AddChamfer call	IsNull=0, status=5 (ChFi2d_IsDone)	IsNull=0, status=5, unchanged
second AddChamfer call, same pair	SIGSEGV, exit 139	IsNull=1, status=7 (ChFi2d_ConnexionError), exit 0

clang-format --dry-run --Werror on the patched file reports only pre-existing, unrelated
violations elsewhere in the file (shifted by 4 lines, same count and content before and after).

Reproducer: Scripts/repro/705-chamfer2d-duplicate-pair/.

What this PR does NOT do

• Does not rebuild the xcframework or bump the pin. The patch is carried and inert against the
  pinned v2.0.0-kernel.1 until a rebuild ships it. The supporting text for patch-carried being a
  separate event from patch-shipped is docs/guides/building-occt.md's "Shipping a rebuild" intro,
  not its "Mid-release" section: that section's actual directive is to publish a
  vX.Y.Z-kernel.N pre-release promptly rather than wait, and it says the divergence is fine
  "for a day", not for a release. Deferring is still the right call here, for two reasons the
  section does not cover: this PR adds no Swift regression test, so ci.yml stays green on the
  pinned asset with nothing masked, and fix(#705): reject a repeated edge pair in chamfer2D instead of crashing #706's guard already protects every caller. But the
  deferral is a decision with an expiry, not a default, so patch 0022 is added to Rebuild OCCT.xcframework with patch 0017 (#484 null-context guard) and ship a patch release #512's rebuild
  scope to stop it drifting past the next release.
• Does not remove the bridge guard from fix(#705): reject a repeated edge pair in chamfer2D instead of crashing #706. A caller on the currently-pinned kernel still
  needs it; it becomes redundant, not wrong, once a rebuild ships this patch.
• Does not touch Shape+Geom2d.swift or OCCTBridge_Modeling.mm. This PR is patches/repro
  files only, no Swift or bridge changes, no rebuild artifacts.

Filed upstream

• Issue (reproducer): Open-Cascade-SAS/OCCT#1431
• PR (fix): Open-Cascade-SAS/OCCT#1432

Verify

• git apply --check -p1 and --reverse --check -p1 both clean against the pristine V8_0_1 tag.
• clang-format --dry-run --Werror: no new violations.
• swift build: unaffected (no source files changed), clean.
• Zero em-dashes in this diff.

Refs #705

[gsdali]

Independent verification

The patch applies cleanly to Libraries/occt-src at V8_0_1, checked with git apply --check.

It is the right five lines. I proposed this shape before the work started and the result matches
the sibling overload verbatim, which is the whole argument: ChFi2d_Builder_0.cxx calls
FindConnectedEdges in six places, five check the returned status, and the sixth is the one that
crashes. Returning the already-default-constructed chamfer reuses the function's existing "declined"
signal rather than inventing one.

Upstream filed properly: issue OCCT#1431
with the reproducer, PR OCCT#1432 with the fix,
one file, +4/-0. The PR body states the mechanism and then leads on the sibling overload as
precedent, which is the framing that got #1377, #1382 and #1392 merged: an argument from OCCT's own
code rather than from our preference.

Gates green, zero em-dashes. No Swift source changes, so the suite is unaffected.

The one thing to be deliberate about on merge

Patch 0022 is carried but not shipped. It is inert against the pinned v2.0.0-kernel.1 until a
rebuild, which is correct for this PR and is exactly the case docs/guides/building-occt.md step 4
covers. Until then #706's bridge guard is what protects callers, which is the established PR1 then
PR2 pattern (#298, #341, #344, #349).

So the sequencing matters: #706 should merge first. Merging this one alone would leave a patch in
the tree that fixes nothing yet and no bridge guard, and the crash would still be live.

Whether to cut v2.0.0-kernel.2 and bump the pin is a separate call. It is not urgent while the
bridge guard holds, and it costs a 75-minute rebuild plus a published asset, so it is worth batching
with the next kernel patch rather than doing for this one alone.

[secondmouseAU-bot]

Code review (independent)

Scope: the PR diff (+267/-0, 4 files), cross-checked against upstream OCCT at V8_0_1 and this repo's own conventions.

Independent verification (all confirmed)

• Patch context vs upstream V8_0_1: fetched ChFi2d_Builder_0.cxx at the V8_0_1 tag. Function signature at line 71; the three earlier refusal paths return chamfer; at lines 83/89/95 exactly as the README entry claims; FindConnectedEdges call at line 101; hunk context (@@ -99,6 +99,10 @@) matches verbatim. The sibling AddChamfer(E, V, D, Ang) overload contains the identical status-check idiom the patch copies.
• Status enum numerics: ChFi2d_ConstructionError at V8_0_1 gives ChFi2d_IsDone = 5 and ChFi2d_ConnexionError = 7, exactly matching the reproducer's reported status=5 / status=7.
• All failure paths are covered: the verbatim FindConnectedEdges returns ChFi2d_ConnexionError on every failure branch (vertex not in map, empty list, one incident edge, three-plus incident edges), so the status check guards all of them, not just the reproducer's path.
• DRAW reachability claim: BRepTest_Fillet2DCommands.cxx (src/Draw/TKTopTest/BRepTest/, at V8_0_1) loops over command-line edge pairs and calls the two-edge MF.AddChamfer(E1, E2, p1, p2) once per pair, so a duplicated pair crashes through OCCT's own tooling. Claim holds.
• Conventions: numbering is correct (0022 follows 0021; gaps like the missing 0013 are intentional per the README's permanent-id policy); entry format matches 0021 (heading, bold lead, Fix:, Validation, Retire); it is the last active entry before "Retired patches"; repro dir follows the <issue>-<slug> convention; no index table needs updating.
• CI: green, including swift test against patched kernel (macOS) (kernel-integration.yml): a full from-source rebuild with the carried patches applied and the entire Swift suite run against that binary. This is end-to-end proof the patch applies via build-occt.sh, compiles at production flags, and regresses nothing -- stronger evidence than the override-link probe.

Correctness

The fix is right and minimal. Returning the already-declared null chamfer reuses the function's existing refusal signal with Status() == ChFi2d_ConnexionError, semantically identical to the sibling overload. No behavior change on the success path (CI confirms).

Nits (documentation accuracy only, non-blocking)

1. "Leaves both edges null on every failure path" is overstated. Repeated in the PR body, patches README, and repro README. The verbatim source shows the exactly-one-incident-edge path assigns E1 before returning ConnexionError, and the three-plus path assigns both. That phrasing is accurate only for the reproducer's path (vertex absent from the rebuilt face's map). The patch does not depend on nullness -- it depends on the status, which is uniformly ConnexionError -- so the fix stands; tightening the wording to "returns ChFi2d_ConnexionError on every failure path" would make the documented rationale precisely correct.
2. Attribution of the override-link technique. The PR body cites docs/guides/building-occt.md for it, but that guide does not document the technique -- it only mentions override-link probes as a contamination hazard (rebuild step 1) and mandates the final proof run with no override-linked TUs (step 3). The technique is actually documented in the #0001 entry of Scripts/patches/README.md, which the new 0022 README entry correctly cites. Minor mis-citation in the PR body.
3. The "Mid-release" citation cuts the other way. The PR frames carrying-an-inert-patch as "per building-occt.md's Mid-release guidance", but that section's actual directive is to publish a vX.Y.Z-kernel.N pre-release and bump the pin promptly ("do not wait for the release commit"); the supporting quote for patch-merged != patch-shipped is the "Shipping a rebuild" intro. Deferring is defensible here (no new Swift regression tests means ci.yml stays green on the pinned asset, and fix(#705): reject a repeated edge pair in chamfer2D instead of crashing #706's guard protects callers), but the guide's condition is that the divergence is fine "for a day", not "for a whole release" -- so the rebuild/pin-bump follow-up should be tracked so it does not drift past the next release. The existing comment already treats v2.0.0-kernel.2 as a deliberate batch-with-next-patch decision, which is the right framing.

Risks

• Sequencing is the one real hazard, already flagged in the PR's own comment: fix(#705): reject a repeated edge pair in chamfer2D instead of crashing #706 (bridge guard) must merge first. This PR alone would carry a patch that fixes nothing on the pinned kernel while leaving callers unguarded.
• Retirement obligation: once upstream OCCT#1432 lands and the pin moves past it, 0022 must be retired per the README's convention -- the entry already says so.
• No performance, security, or API surface impact: four lines of C++ in a carried patch file, inert until rebuild.

Test coverage

Deliberately no new Swift tests: any test asserting the fix would be red against the pinned asset (the exact trap the build guide warns about). Coverage sits where this repo's pattern puts it: the deterministic standalone reproducer with before/after evidence, and the CI kernel-rebuild run of the full suite against the patched kernel. A Swift-level regression test could not drive this path through the public API anyway once #706's duplicate-pair guard is in place, so the Scripts/repro/ harness is the appropriate home for it.

Verdict

Approve. The root-cause analysis checks out against upstream V8_0_1 source line for line, the fix copies the sibling overload's own idiom, enum numerics and the DRAW-reachability claim both verify, conventions are followed, and CI validated the patch in a real full rebuild. The three documentation nits above are wording/citation fixes only and can be addressed in place or left for the follow-up rebuild PR. Merge after #706, and keep the kernel-rebuild/pin-bump tracked so the inert patch does not outlive the next release.

[gsdali]

All three addressed, and the first one corrects me as much as the PR.

Nit 1 is right and I repeated the error in my own review. I wrote that FindConnectedEdges "leaves E1/E2 null on all three of its failure returns". There are four, and only two leave both unassigned:

failure	E1	E2
vertex absent from the map (this repro)	unassigned	unassigned
vertex present, zero incident edges	unassigned	unassigned
exactly one incident edge	assigned	unassigned
three or more incident edges	assigned	assigned

The patch is unaffected, since it keys on the status, which is uniformly ChFi2d_ConnexionError. But the wrong rationale is the argument for guarding on nullness, and that guard would have missed half the failure paths. Both READMEs now carry the table and say why the status is the right thing to check.

Nit 2 correct. building-occt.md mentions override-link only as a contamination hazard and mandates the final proof without it. The technique is in Scripts/patches/README.md's #0001 entry, which the 0022 entry already cited; only the PR body was wrong. Fixed.

Nit 3 is the one with teeth, and I have given it an owner. You are right that "Mid-release" says publish a vX.Y.Z-kernel.N pre-release promptly and calls the divergence fine "for a day", so citing it as cover for deferring was backwards. The supporting text is the "Shipping a rebuild" intro.

Deferring is still right here for two reasons that section does not cover: this PR adds no Swift regression test, so ci.yml stays honest on the pinned asset with nothing masked, and #706's guard already protects every caller. But that is a deferral with an expiry, so patch 0022 is now in #512's rebuild scope, with the reasoning recorded there. 0022 is the first patch carried since the pin moved to v2.0.0-kernel.1, so it is also the trigger for whether a kernel.2 is worth cutting or whether it batches with the next one.

Thanks for verifying the enum numerics and the DRAW reachability against V8_0_1 independently. ChFi2d_IsDone = 5 / ChFi2d_ConnexionError = 7 matching the reproducer's output is the check that rules out the repro reporting a status it never saw.