fix: select recipe 01's inside-corner fillet edge geometrically (#105)

[gsdali]

What & why

Recipe 01's inside-corner fillet has never applied. prism.filleted(edges: prism.concaveEdges(), radius: filletRadius) returns nil at the configured filletRadius = 8, and a ?? prism fallback hid it, so the bracket shipped with a sharp inside corner
while its header, README, and the top-level recipes README all advertised a
concaveEdges()-driven fillet.

Closes #105

Diagnosed mechanism: concaveEdges() returns the wrong two edges

The L-profile has exactly one reentrant vertex, at (thickness, thickness). Extruded
along Z, that vertex produces exactly one concave edge: a line parallel to the extrusion
axis, running its full 40 mm length. prism.concaveEdges() does not return that edge. It
returns two different edges instead, each 45 mm (legLength - thickness), lying in the
end-cap plane (z = width):

edge[7]  (the true reentrant edge)   len=40, x:[5,5] y:[5,5] z:[0,40]   -> classified CONVEX
edge[9]  (concaveEdges() result #1)  len=45, x:[5,50] y:[5,5] z:[40,40] -> classified CONCAVE
edge[12] (concaveEdges() result #2)  len=45, x:[5,5] y:[5,50] z:[40,40] -> classified CONCAVE

edge[7] sits exactly where the profile's reentrant vertex predicts a concave edge
should be. It is classified convex. The two edges concaveEdges() calls concave are
boundary segments of the top cap, at the far end of the part from the reentrant corner.

Those two wrong edges are bounded by the 5 mm leg thickness (they sit on the boundary
between the end cap and a wall that is only 5 mm across), so a fillet there fails above
roughly that radius: reproduced r=8.0 -> nil, r=5.0 -> nil, r=4.9 -> ok exactly as
measured in the issue. The true edge (edge[7]) is bounded only by
legLength - thickness (45 mm), since it runs the full unconstrained length of each
wall: it fillets cleanly all the way from r=0.1 up to r=44.9, and only fails at
r=45.0 (the wall's own length).

Both edges should not be filleted. The two concaveEdges() picks are the top cap's
ordinary perimeter edges (like any other box edge), not the bracket's structural inside
corner; filleting them would round a cosmetic top-face edge, not the corner that matters,
and they are excluded from the fix entirely.

A concave fillet adds material, not removes it

Filleting the true reentrant edge increases volume: it fills part of the sharp inside
corner with a rounded blend. r^2 * (1 - pi/4) * length is the magnitude either way, but
the sign flips depending on which edge you hand it:

edges filleted	r	volume change	analytic prediction
concaveEdges() (wrong, 2 edges, L=45 each)	3.0	-176.42 mm3 (removed)	matches issue's own measurement
edges(where:) selector (correct, 1 edge, L=40)	3.0	+77.26 mm3 (added)	3^2*(1-pi/4)*40 = 77.26
edges(where:) selector (correct, 1 edge, L=40)	8.0 (configured)	+549.38 mm3 (added)	8^2*(1-pi/4)*40 = 549.38

The configured filletRadius = 8 needed no change at all, once pointed at the right
edge. It was never too large for the part; it was only ever being measured against the
wrong edge's much tighter limit.

Fix

Select the inside-corner edge geometrically with Shape.edges(where:): a line parallel
to the extrusion axis, positioned at (thickness, thickness), the same approach recipe
03 uses for the pipe flange (#103). Force-unwrap the fillet result (!), not ?? prism:
a future regression now crashes loudly instead of silently shipping an un-filleted
bracket.

Regenerated output.brep and output.png. The header comment, README.md, and the
top-level recipes/README.md line all described concaveEdges() as the selection
mechanism; updated all three to describe the geometric selector and explain why
concaveEdges() was wrong on this shape, plus a Gotchas note that a concave fillet adds
volume rather than removing it.

Verified end to end

• Ran the actual corrected recipe through occtkit run + occtkit metrics (not just an
  in-process diagnostic): solidCount = 1, bounding box unchanged ((0,0,0) to
  (50,50,40), since the fillet fills part of the existing envelope rather than growing
  it).
• Final drilled bracket: 18779.69 mm3, against the previous un-filleted reference of
  18230.31 mm3. Delta is +549.38 mm3, exactly the fillet's analytic prediction; the four
  holes remove the same volume in both cases since they sit well clear of the corner.
• Rendered output.png before/after and diffed pixel-by-pixel: the sharp V at the inside
  corner is now a visible rounded arc at that exact edge; everything else in the render
  is unchanged bar 1px anti-aliasing jitter.

Audit: other ?? fallbacks on geometry operations in recipes/

Grepped every recipes/*/main.swift for ?? . Found one other instance of the same
shape, recipes/06-fan-blade/main.swift:72: blade = blade.union(hub) ?? blade.
Verified it has never actually fired: volume, bounding box, and solidCount are
byte-identical before and after removing the fallback (blade.union(hub)!), so this is a
dormant instance, not a live bug, but it should fail loudly if it ever does. Fixed
alongside this PR since it's a one-line, zero-behavior-change fix of the exact same
pattern.

Every other ?? in recipes/*/main.swift is .volume ?? 0 inside a print statement,
a display default for an already-emitted shape's optional volume. That does not affect
what geometry ships (only what number gets printed if volume itself returns nil), so
it is not the same defect class; left alone.

Also checked Sources/occtkit/Commands/Heal.swift:109
(let healed = fixer.shape ?? input), which is structurally similar but out of scope
(not in recipes/, and it already surfaces a warning plus before/after snapshots when
didChange is false, so the caller can detect a no-op heal, unlike the silent recipe
pattern). Not filing a follow-up issue; flagging here in case it's useful.

Recorded a new decision,
concave-edge-classifier-can-select-wrong-edges,
since this is now the second independent recipe (after the flange chamfer, #103/#104)
where concaveEdges() / convexEdges() disagreed with a shape's actual geometry, no
longer a single-shape quirk worth only a footnote.

Checklist

• New behavior is covered by Scripts/recipe-check.sh in the same PR: the
  regenerated output.brep reference asserts solidCount >= 1 and the volume via
  occtkit metrics, which is this repo's established test mechanism for recipes (no
  unit test framework exists here; see CLAUDE.md).

Notes for the reviewer

• All three gates pass: Scripts/recipe-check.sh (all 7 recipes), Scripts/policy-check.sh,
  Scripts/verb-check.sh.
• Do not merge. Per the task, a human reviews every PR here.

[gsdali]

The upstream defect is 1.x only, and is already fixed in 2.0.0

Pushed 39383ba, which changes no code but scopes the workaround.

The concaveEdges() defect this PR works around is fixed in the 2.0.0 line. It was too involved to backport to 1.x, so it rides the 2.0.0 refactor. Verified against the published v2.0.0-kernel.1 prerelease with the same repro:

                 L-prism            T-prism            inside corner
1.17.0           concave=2  (exp 1) concave=3  (exp 2) inConcave=false   MISMATCH
2.0.0-kernel.1   concave=1  (exp 1) concave=2  (exp 2) inConcave=true    OK

A box, having no reentrant edges, is correct on both.

The geometric selection stays, because this package is pinned to 1.17.0 and the defect is real there. What changed is that the workaround is now recorded as temporary rather than permanent, in both the OKF entry and the recipe header, so whoever migrates to 2.0.0 knows this can be simplified back to concaveEdges() and knows to re-run the repro rather than assume the migration carried the fix.

Volume unchanged and re-verified after the edit: 18779.69, all 7 recipes pass, policy-check clean.

Correction to the upstream report

I also corrected OCCTSwift#695. It claimed the bug was "verified at v1.17.0 and at v2.0.0-kernel.1", but only the 1.17.0 run was the classifier repro; the 2.0.0-kernel.1 run I had done was a different probe. At 2.0.0-kernel.1 the case passes. Left it for the maintainer to close or relabel as "fixed in 2.0.0, not backported", since that is a release-plan call.

[secondmouseAU-bot]

Review: fix: select recipe 01's inside-corner fillet edge geometrically (#105)

Overview

Recipe 01's L-bracket has never actually applied its inside-corner fillet: prism.filleted(edges: prism.concaveEdges(), radius: filletRadius) returned nil at filletRadius = 8, and ?? prism silently swallowed the failure. The PR root-causes this precisely: concaveEdges() on this L-profile picks two different edges (top-cap boundary segments, each bounded by the 5 mm leg thickness) instead of the one true reentrant edge (bounded by the full 45 mm unconstrained wall length), so a perfectly reasonable 8 mm radius reads as infeasible against the wrong edge's tighter limit.

The fix:

• Selects the inside-corner edge geometrically via Shape.edges(where:) (a line parallel to Z at (thickness, thickness)), mirroring recipe 03's existing pattern for the pipe flange.
• Replaces the ?? prism fallback with a force-unwrap, so a future regression crashes instead of shipping silently-wrong geometry.
• Regenerates output.brep/output.png, updates the recipe's header comment, README.md, and the top-level recipes/README.md.
• Fixes a second, dormant instance of the identical ?? fallback pattern in recipe 06 (blade.union(hub) ?? blade → blade.union(hub)!), found via a grep audit the PR body documents.
• Records a new OKF decision (concave-edge-classifier-can-select-wrong-edges.md), cross-linked from the index and from the related revolve-seams-cannot-be-chamfered.md entry, and scopes the defect explicitly to OCCTSwift 1.x (verified fixed on v2.0.0-kernel.1, filed upstream as OCCTSwift#695).

Correctness — verified

I checked the core claims against the OCCTSwift 1.16.1 docs (Edge.isLine, Edge.bounds, Shape.filleted(edges:radius:)) and they match the API used exactly. The selector logic is sound:

let insideCorner = prism.edges { edge in
    guard edge.isLine else { return false }
    let b = edge.bounds
    let runsFullWidth = abs((b.max.z - b.min.z) - width) < 1e-6
        && abs(b.max.x - b.min.x) < 1e-6 && abs(b.max.y - b.min.y) < 1e-6
    guard runsFullWidth else { return false }
    return abs(b.min.x - thickness) < 1e-6 && abs(b.min.y - thickness) < 1e-6
}

This is robust against the false-positive case one might worry about (the top-cap boundary edges accidentally matching): those lie in a constant-z plane, so their Z-extent is 0, not width, and they're excluded by runsFullWidth regardless of any length coincidence with legLength/thickness.

The volume-delta arithmetic in the PR body also checks out independently: r²·(1−π/4)·length for r=8, length=40 → 549.38 mm³, and for r=3, length=40 → 77.26 mm³ — both match what's claimed to two decimal places. The regenerated output.brep is consistent with this too: Surfaces count goes 11→12 with a new radius-8 cylindrical surface added at the corner's coordinates, and TShapes 71→83, exactly what adding one fillet blend should produce.

Code quality / conventions

• Matches the repo's established geometric-selector idiom (recipe 03) rather than inventing something new — good adherence to the search-before-building policy.
• Docs (header comment, recipe README, top-level recipes table, OKF entry) all update in the same PR — compliant with docs-current.
• No em-dashes in any of the new prose; consistent with writing-style.
• No dependency floor bump needed — edges(where:) has been available since OCCTSwift v1.2.1, well under the existing v1.17.0 floor.
• Minor naming nit: insideCorner holds a [Edge] (it's fed straight into filleted(edges:)), but the name reads singular. Recipe 03's equivalent is named chamferTargets (plural, and clearly a collection). Something like insideCornerEdges would be more consistent and avoid a reader momentarily expecting a single Edge.

Suggestion (non-blocking)

The new code trusts that prism.filleted(edges:radius:) returns nil if the edge selection is bad, and relies on that for the "fail loudly" guarantee. But if insideCorner ever comes back empty (e.g., a future parameter change or an upstream classifier tweak breaks the predicate silently), it's not certain filleted(edges: [], radius:) returns nil rather than a no-op success — OCCT's fillet builder may just hand back the shape unchanged for zero edges. If that's the case, the force-unwrap wouldn't crash, and the recipe would silently regress to exactly the un-filleted-bracket bug this PR fixes, just without a nil to catch it. The main safety net today is Scripts/recipe-check.sh's volume/bbox drift check against the reference output.brep, which would catch this — but only as long as nobody blindly regenerates the reference from a broken build.

Consider adding an explicit guard for defense in depth:

precondition(!insideCorner.isEmpty, "inside-corner edge selector matched nothing")

This makes the failure mode self-diagnosing at the point of the bug rather than only detectable via a reference-drift test elsewhere.

Test coverage

Reasonable given this repo's testing model (no unit-test framework; Scripts/recipe-check.sh + occtkit metrics is the established mechanism, per CLAUDE.md). The PR goes further than the minimum: it re-derives the expected volume delta analytically and cross-checks it against the actual run, and diffs the before/after render pixel-by-pixel. That's a solid verification story for a change that's otherwise easy to "fix" in a way that looks plausible but fillets the wrong edge again.

Security / performance

No concerns — this is offline CAD geometry generation over hardcoded parameters, no external input, and the edge-selection closure scans on the order of ~18 edges (trivial cost).

Risks

Low. The change is scoped to one recipe's edge selection plus a one-line dormant-fallback fix elsewhere, both backed by concrete before/after measurements. The 1.x-only framing is explicit and testable (the OKF entry gives the exact repro to re-run before reverting to concaveEdges() on a future 2.0.0 migration), so this isn't leaving a silent trap for later.

Bottom line: correct, well-verified, and well-documented fix. The only actionable suggestion is the defensive empty-selector guard; everything else is a nit.

---

🤖 Generated with Claude Code

[gsdali]

Both addressed in 490b958. The suggestion rested on an empirical question, so I measured it rather than assuming either way.

The empty-selector concern does not materialise, but the guard is still worth having

You flagged that it was "not certain" whether filleted(edges: [], radius:) returns nil or hands back the shape unchanged. It returns nil, on both versions that matter:

1.17.0          filleted(edges: [], r: 8) -> nil
2.0.0-kernel.1  filleted(edges: [], r: 8) -> nil

So the existing force-unwrap would already have caught an empty match, and the silent-regression scenario you described could not have happened as written.

I added the precondition anyway, for two reasons the measurement does not remove:

1. It names the actual fault. A bare force-unwrap crash says nothing about which of the two things went wrong, the selector or the fillet.
2. It stops the recipe depending on undocumented nil-on-empty behaviour. That is currently an accident of the wrapper rather than a stated contract, and a 2.0.0 migration is already on the horizon.

The code comment says exactly this, so the next reader knows the guard is defence in depth rather than a fix for an observed failure.

Proved the guard fires

Otherwise it would be another check that cannot fail, which is the pattern this whole run of PRs has been about. Breaking the predicate so it matches nothing:

Script/main.swift:73: Precondition failed: inside-corner edge selector matched nothing
Error: Script exited with code 5

and 18779.69 once restored.

Naming nit

Taken: insideCorner to insideCornerEdges, matching recipe 03's chamferTargets. You were right that it read singular while holding a collection.

Verification

Volume unchanged at 18779.69, all 7 recipes pass, policy-check and verb-check clean, no em-dashes.

Thanks for the review, particularly the Surfaces 11 to 12 and TShapes 71 to 83 cross-check on the regenerated BREP. That is a stronger confirmation that exactly one blend was added than the volume delta alone.

[secondmouseAU-bot]

Review (pass 2)

What changed since the last review

Two commits landed since my first review, both responsive to it:

1. Earlier commit (already reflected in what I reviewed the first time): scoped the concaveEdges() defect explicitly to OCCTSwift 1.x with a cross-version repro table, and corrected the upstream issue (OCCTSwift#695) where the author had mis-stated which OCCTSwift version they'd actually tested.
2. 490b958 (new): addresses my two comments directly —
   • insideCorner → insideCornerEdges (the naming nit)
   • Added precondition(!insideCornerEdges.isEmpty, "inside-corner edge selector matched nothing") before the fillet call (the empty-selector guard)

Assessment of the new commit

The guard addition is done well, not just bolted on:

• The author didn't take my "not certain whether filleted(edges: [], radius:) returns nil" at face value — they measured it on both 1.17.0 and 2.0.0-kernel.1 and confirmed it does return nil today. So the specific silent-regression scenario I raised turns out not to be reachable as the code stood. Good catch that my hedge was checkable, and they checked it rather than just implementing the suggestion blind.
• They kept the precondition anyway, for a sharper reason than mine: it makes the crash name the actual fault (selector vs. fillet) instead of an anonymous force-unwrap trap, and it stops the recipe depending on filleted's empty-array behavior as an unstated contract — a real concern given the 2.0.0 migration is explicitly on the horizon per the OKF entry.
• They proved the guard actually fires (deliberately broke the predicate, confirmed Precondition failed: inside-corner edge selector matched nothing / exit code 5, then restored and re-verified 18779.69). That's the right level of rigor for a check whose entire value is firing correctly when needed.

One residual, minor nit

precondition(...) is a slight style departure from this file's own convention: recipe 01's neighbors use guard ... else { fatalError(...) } for "this should never happen" cases (e.g. recipe 06's guard var blade = Shape.loft(...) else { fatalError("loft failed") }). Functionally near-identical under this repo's normal build (both trap; precondition only gets stripped under -Ounchecked, which I see no evidence this repo uses), so this isn't a correctness concern — just a small inconsistency in which "crash loudly" idiom gets used where. Not worth a follow-up commit on its own.

Bottom line

Both prior points are resolved correctly and verified empirically rather than assumed. No new issues introduced by the delta. I'd approve this as-is (modulo the repo's own "a human reviews every PR here" policy, which still applies regardless of what I say).

---

🤖 Generated with Claude Code