fix(#639): the fillet family reports declined edges instead of only skipping them

[gsdali]

What & why

BRepFilletAPI_MakeFillet::Add silently does nothing for an edge it cannot fillet, most commonly a free-boundary edge of an open shell. filleted(edges:radius:), filleted(edges:startRadius:endRadius:) and filletEvolving(_:) have always skipped such an edge rather than rejecting the whole call, correctly, but had no way to tell a caller which edges those were, or how many.

The Cluster B census (Scripts/repro/cluster-b-fillet-edge-contract/) measured this concretely: filleting all 12 edges of an open shell (a box with one face dropped, sewn) accepts 8 and silently declines 4 ([6, 9, 10, 11]).

Closes #639

The decision

Report, do not reject. Converging every declining entry point onto rejecting a batch with any declined edge would change filleted(edges:radius:), filleted(edges:startRadius:endRadius:) and blendedEdges(_:) on every open shell: a behaviour change wider than this issue, and the same mistake an earlier draft of #633 made and withdrew. Skip stays the answer; the fix is observability, following #482's FillingSurface.refusedConstraintCount precedent.

Contour(edge) == 0, populated by Add() and not Build(), is the only signal OCCT itself exposes for a declined edge. Add returns nothing, and NbFaultyContours()/BadShape()/StripeStatus() describe a contour that failed during Build(), which an edge OCCT never added to any contour never reaches. So this reports which edges were declined (a list of indices, not just a count), and not why: no reason is reachable from this API.

What changed

Three genuinely new entry points, for the three members that had no side channel at all:

• filletedWithReport(edges:radius:)
• filletedWithReport(edges:startRadius:endRadius:)
• filletEvolvingWithReport(_:)

Each returns a new Shape.FilletResult { shape, declinedEdgeIndices }. The bridge computes the report with a new shared helper (occtFilletDeclinedIndices/occtFilletWriteDeclined) that re-checks Contour(edge) for each requested index after Add() and before Build(). The three underlying bridge functions gained two nullable trailing out-parameters; existing non-reporting call sites pass nil and see no change in cost or behaviour.

Two of the issue's own named members needed no new code, only documentation, measured rather than assumed:

• filletedWithFullHistory(radius:edges:)'s ShapeHistoryRef already distinguishes a declined edge via !record.isDeleted && record.generated.isEmpty. My first hypothesis for this recipe was wrong, and measuring it caught that: a declined edge is not necessarily modified.isEmpty too. On the shell fixture every declined edge shows one modified entry, a different edge instance with a shorter length (10.0 to 8.0), because an accepted neighbour's fillet trims the declined edge's shared endpoint. modified alone is not the signal; generated/isDeleted are.
• FilletBuilder.contour(for:) already answers the identical Contour(edge) == 0 question directly, readable right after addEdge with no build() required. This also corrects a Cluster B census finding: its claim that the class API has "no per-edge signal beyond addEdge's own Bool return" was wrong, it simply never tried this query. Measured directly: a foreign edge from a different shape gives addEdge returns true, contour(for:) returns 0, the identical signal a genuine same-shape decline gives.

blendedEdges(_:) (#633's own site) does not adopt this mechanism here, and is out of scope for this PR per the task boundaries. Recommendation for #633: adopt the same FilletResult-shaped report, extended to also carry which duplicate indices were overwritten.

Docs

• docs/SEMVER.md: recorded as additive (not one of the twelve "recorded exceptions", since nothing existing changed signature or behaviour; checked and confirmed the counter arithmetic is unaffected).
• docs/CHANGELOG.md, docs/API_REFERENCE.md (+3 to the operation count, 4301 to 4304), README.md headline count.
• docs/reference/Shape-Features.md, Shape-Measurement.md, Shape-Healing.md: new entries plus the declined-edge recipe for filletedWithFullHistory.
• Scripts/repro/cluster-b-fillet-edge-contract/README.md and ClusterB.swift: the measured grid values did not move (still SKIP/REJECT the same way), only the annotations on the now-fixed rows, plus the FilletBuilder class-API row's correction. swift run Censuses cluster-b still emits 16 rows, cluster-a still 45.

Test plan

• New test: Tests/OCCTModelingTests/Issue639FilletDeclinedEdgeReportTests.swift, 7 cases covering all 5 members named in the issue plus 2 negative controls, following okf/policies/prove-the-test-fails.md. Injection matrix:

Injection	Result
occtFilletWriteDeclined reports empty	The 3 WithReport "names declined set" tests fail; the 2 "empty on closed solid" negative controls correctly stay green
OCCTFilletBuilderContour always returns 1	The FilletBuilder recipe test fails (both its assertions)
OCCTBooleanHistoryIsDeleted always returns true	The history recipe test fails

Each injection restored and confirmed green again afterward.

• Clean swift build, 0 errors, no new warnings, no OCCTSWIFT_LOCAL.
• Full swift test, no env overrides: 5353 tests passing (baseline 5346 + 7 new), 2 consecutive clean runs, no crash flake.
• swift run Censuses cluster-a still 45 rows, cluster-b still 16 rows.
• check-bridge-index.py, check-null-handle-guards.py, check-docs-defaults.py each pass with and without --self-test; count-operations.py (no --self-test) passes, README/API_REFERENCE totals now match derived (4304).
• Zero em-dashes in code, docs, and this PR body.

Notes for the reviewer

• The real gap, once measured, was narrower than the issue's own list of five: only 3 of 5 named members needed new bridge code. The other 2 already carried the answer; nobody had connected it to The fillet family cannot tell a caller that OCCT declined some of the edges it named #639 before.
• FilletBuilder's contour(for:) gives you 0 for both "never added" and "added but declined". Documented explicitly; a caller who wants to tell these apart has to track its own added-edges list, which it already does since it called addEdge itself.

[gsdali]

Independent verification

Report rather than reject is the right call, and the reasoning is the part that matters:
converging on reject would change filleted(edges:radius:), its evolving sibling and
blendedEdges(_:) on every open shell. Adding a side channel changes nothing for existing
callers. Naming that an earlier #633 draft made and withdrew that exact mistake is the kind of
institutional memory that stops it being made a third time.

Measured the decline set myself, building the census's open-shell fixture independently:

PROBE shell edges = 12
PROBE declined    = [6, 9, 10, 11]

Matches the census and the PR exactly.

Then checked the correction to the census, which is the most useful thing in this report. The
census recorded "no per-edge signal beyond addEdge's Bool return". That is wrong, and I confirmed
it:

via FilletBuilder.contour(for:) after addEdge : [6, 9, 10, 11]
via filletedWithReport                        : [6, 9, 10, 11]

The query already existed and nobody had tried it. Worth noting my first attempt at that probe was my
own error, querying a fresh builder before adding any edge, which returns all twelve. The signal only
means anything after addEdge.

Injection, mine rather than the matrix in the PR: making occtFilletWriteDeclined a no-op fails
exactly the three WithReport tests and leaves the negative controls green.

✘ filletedWithReport(edges:radius:) names exactly the census's declined set
✘ filletedWithReport(edges:startRadius:endRadius:) names the same declined set
✘ filletEvolvingWithReport(_:) names the same declined set
   Set(report.declinedEdgeIndices) → []  ==  [6, 9, 10, 11]

Verified: all three CI checks green including the full macOS suite, four gate scripts and their
three --self-tests, Cluster B census still 16 rows, zero em-dashes.

The self-correction worth keeping

The report says its own first hypothesis for the history recipe was wrong: a declined edge is not
modified.isEmpty in general, because an accepted neighbour's fillet can trim a declined edge's
shared endpoint, measured as a 10.0 to 8.0 length change. The reliable signal is
generated/isDeleted. That is a false positive that would have shipped as a "working" recipe on
any fixture where no accepted edge happens to neighbour a declined one, which describes most simple
fixtures.

Merge order, since three PRs are open into this branch

#706 and #709 both touch docs/SEMVER.md and docs/CHANGELOG.md, which is exactly what conflicted on
#698. #708 touches neither.

Suggested: #706, then #708, then #709. #706 before #708 because the bridge guard must land before
the patch that is inert without it, and #709 last so it absorbs the SEMVER churn once rather than
twice. Whichever goes second will want its recorded-exception counters re-checked; that arithmetic
has gone stale twice already.

[secondmouseAU-bot]

Code review (independent)

Scope: the PR diff (+737/-30, 16 files), cross-checked against OCCT's refman contract for Contour, the bridge's resolution helpers, and this repo's doc/counting conventions.

Correctness -- verified, sound

• The OCCT signal is real and correctly used. OCCT refman confirms int Contour(const TopoDS_Edge &E) const: returns 0 when the edge belongs to no contour, and contours are "generated using the Add function" -- so the two load-bearing claims (0 = declined; populated by Add(), not Build()) hold. The const-qualification also confirms the helper's const BRepFilletAPI_MakeFillet& parameter compiles (CI corroborates).
• Check timing is right. Querying after all Add() calls and before Build() correctly handles OCCT merging a later edge into an earlier tangent contour -- a per-edge check right after each Add() could false-positive. The comment explicitly names this.
• Resolution consistency holds. occtFilletAddEdges resolves via occtUseSubShapesByIndex, which internally uses the same occtMapSubShapes + occtMappedSubShapeAt pair the new helper calls directly on the same shape. So Contour() is queried with the exact edge objects Add() received, and the helper's "skip unresolvable index" is safe because the add path already rejects the whole batch in that case (template returns nullptr before the write).
• Buffer contract is safe. Swift allocates exactly edgeCount capacity; at most edgeCount declines can be reported even with duplicate indices; *outDeclinedCount is zero-initialized on every path, so early failures leave no garbage. The Swift side discards the report whenever the shape is nil.
• Behavior preservation is genuine. Non-reporting call sites pass nil, nil; occtFilletWriteDeclined returns immediately when both are null -- zero added cost, zero behavior change -- and the test asserting report.shape area equals the plain sibling's area pins the "same shape, more information" contract.
• The history-recipe self-correction is right and well-guarded: modified can be non-empty on a declined edge (neighbour trimming), so !isDeleted && generated.isEmpty is the correct signal, and the test asserts at least one declined edge has non-empty modified on the fixture -- ensuring the warning is exercised, not just written.

Conventions

Follows the repo's patterns closely: WithReport sibling naming, FilletResult echoing BooleanResult, #482's report-don't-reject precedent, #664's SEMVER-discipline entry (explicitly additive, not a thirteenth exception), CHANGELOG entry under the Pass 1b cluster, census annotations updated without moving measured grid values, -- not em-dashes. API counts are internally consistent: +3 everywhere (README 4301 to 4304, API_REFERENCE Modifications 33 to 36 and Total), with count-operations.py reported green and gate scripts green in CI.

Test coverage

Strong. 7 cases cover all five issue-named members plus two closed-solid negative controls; the declined set [6, 9, 10, 11] is reused from the census rather than re-derived; the injection matrix (plus the independent injection in the comment above) proves each test fails on its own mechanism. The contour(for:) test reads the signal both before and after build(), pinning the "populated by Add" timing. One gap worth noting, not blocking: no case exercises a duplicate requested edge through a WithReport method.

Performance / security

• Performance: zero-cost on the existing path (null params -> immediate return). Reporting path adds one TopExp::MapShapes walk plus one Contour query per requested edge -- trivial next to Build().
• Security: nothing new -- index inputs go through the same validation/resolution as the existing siblings.

Nits (non-blocking)

1. Duplicate requested index that is declined appears twice in declinedEdgeIndices (once per occurrence; the "mirrors the request list" semantics make this defensible and the capacity bound still holds), but it is not spelled out anywhere. One sentence in the FilletResult.declinedEdgeIndices doc would close the loop.
2. outDeclinedCount header contract wording: "or 0 if the call fails before OCCT is touched" does not cover the Build()-failure case, where the count is already written but the function returns NULL. Swift never reads it then, so it is safe in practice -- but the contract would be tighter as "read only when the returned shape is non-NULL."
3. Inherited truncated doc line: the new methods' - Returns: ends "including an edge that is not this shape's" with no noun -- copied from the pre-existing siblings, which read identically, so it is consistent; completing it in all four would be a nice polish.
4. Merge order: per the comment above, fix(#705): reject a repeated edge pair in chamfer2D instead of crashing #706 -> chore(#705): carry the upstream ChFi2d_Builder::AddChamfer kernel patch #708 -> fix(#639): the fillet family reports declined edges instead of only skipping them #709, with this PR rebasing last to absorb the SEMVER.md/CHANGELOG.md churn once. chore(#705): carry the upstream ChFi2d_Builder::AddChamfer kernel patch #708 touches neither file, so only fix(#705): reject a repeated edge pair in chamfer2D instead of crashing #706 is a real conflict source.

Verdict

Approve. The design choice (observe, don't reject) is the right one and argued with institutional memory; the mechanism rests on a verified OCCT contract (Contour(E) == 0, Add-populated); the bridge implementation is timing-correct, resolution-consistent, buffer-safe, and zero-cost for existing callers; tests are mechanism-proven with proper negative controls; docs are exhaustive including an honest correction of both the author's first hypothesis and the census's. Only the four nits above, all cosmetic. Merge last of the three open PRs on this branch.

[gsdali]

All four nits addressed, plus the test gap you noted alongside them.

Nit 1 and the test gap close together. declinedEdgeIndices mirrors the request list rather than deduplicating, so a declined edge named twice is reported twice. Now documented, and pinned by a test asserting count == 2 and Set == [declined]. Deduplicating inside occtFilletWriteDeclined fails it, which is the plausible-but-wrong implementation the test exists to rule out.

Worth recording that my first version of that test was wrong: it requested only the declined edge, twice. That leaves nothing to fillet, so Build() fails and the call returns nil with no report to read. Different behaviour, not the one under test. The fixture needs an accepted edge alongside.

Nit 2 correct: the contract did not cover a Build() failure, where the count is written and NULL returned. Now says read it only when the returned shape is non-NULL, and why.

Nit 3 done on all four, including the two pre-existing siblings the truncation was copied from.

Nit 4, merge order: rebased onto the merged base. #706 and #708 are both in. Two conflicts, both taking each side: the census README wanted base's chamfer2D row (now REJECT, fixed by #705) and this PR's contour(for:) correction; SEMVER.md wanted both new sections.

And the SEMVER arithmetic had gone stale again, exactly as predicted. #705 added an exception, so the counters were one behind: three plus ten is thirteen sections, against a headline saying twelve. Fixed to thirteen, "other ten", "a fourteenth was not taken", and this PR's own note about not moving the count. That is the third time those counters have drifted; worth a gate script eventually.

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

Thanks for verifying the Contour contract against the OCCT refman independently, and for catching that querying after all Add() calls rather than per-edge is what makes tangent-contour merging safe. That timing is the part most likely to be got wrong by someone changing this later.