You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
Retires the UpstreamFixes flag extent3d_contains_both_segment_endpoints (WR-3). A bounded rectilinear volume's line-in-domain check (Volume::is_line_in_param_domain_simple) now always needs both segment ends inside the natural parameter box. A box is convex, so that holds exactly when the whole segment is inside. Upstream's ContainsLineSeg3d accepted a segment with either end inside. Removed: the field, its preset line in crates/smlib/src/core.rs, its fixes.json and issue_2260 list entries, and the flag-off branch. The WR-3 test's flag-off half goes. is_line_in_param_domain_simple no longer takes the kernel; its 5 goldens call sites drop the argument. The addendum plan/upstream-issues/WR-3--addendum-2026-11-14-flag-removed-extent3d-contains-both-segment-endpoints.md records the removal.
Part of #14246 (B50). The batch's other flag, twist_simple_duw_sign, stays as it is on main; see Available follow-ups.
Changes a default: no
SolidCraft's preset already had the flag on, so the app's results don't change. solidcore's Kernel::default() does change: a segment with one end outside the box is no longer in the domain. The goldens run on that default, so they move (below).
Test change: the WR-3 test's flag-off half tested the retired upstream path and is removed; the on-half expectations are unchanged. 119 golden rows move, each proven below.
Moved goldens (119 rows, one expectations commit)
Measured on main a7468ec (all goldens pass again after the rebase onto c6adb44) with the flag retired: 283 units, 281 pass, 2 move. Before and after values were taken on one machine. Rows were taken one by one from a bless run; no other row of any unit changed.
unit
rows
before -> after
label
why the new value is right
surface.b_spline_volume (preds)
96
in_line 1 -> 0
PROVEN (closed form, asserted in the harness)
One segment end lies outside the volume's natural parameter box (first to last knot per direction) and the other end inside. So the segment is not in the box; the old 1 came from the either-end test.
surface.volume_p2 (line_in_in_space)
23
result 1 -> 0
PROVEN (closed form, asserted in the harness)
The first map is a B-spline with no orient map, so parameter points equal in-space points. One end is outside its knot box, so the first map's test is 0, and the AND over the chain is 0.
The smallest outside margin is 1e-7 (r2_line_*_5: z = 1.0000001 against a box ending at 1). The comparisons are exact float comparisons. Per-row table (case, before, after, the outside end, the box and the margin), harness and data: https://gist.github.com/brett19/5b91eb84e93bb1f2908d16c00d54d0f1 (extent3d-moved-rows-proof.md, proof-extent3d.py, before-b50.jsonl, after-extent3d.jsonl).
Certificate checklist
Bounded, not sampled: the decision is both endpoint memberships in a closed box, and a box is convex. No sampling.
Rounding: the comparisons are exact (contains_point with tolerance 0), with no arithmetic beyond comparing.
NaN: not fixed here. Extent3d::contains_point, which this PR leaves unchanged, is written as a negated OR of </> tests, so a NaN coordinate counts as inside, both before and after. This PR only replaces the OR of the two endpoint tests with an AND, so a NaN end can no longer make up for a finite end that is outside. Making contains_point refuse NaN changes other callers, so it is filed as solidcore: Extent3d::contains_point treats NaN coordinates as inside (§7.7) #16459.
No tolerance widened; no expectation loosened.
5-6. No refusal size and no loops involved.
Every moved row is explained above.
A real-defect case: wr3_segment_box_containment keeps segments fully inside (accepted) and fully outside (refused) beside the one-end-in cases.
Overlap check (all open PRs, any author, drafts included)
Method: every open PR's file list (GraphQL, 100 files per PR; PRs over 100 files checked by diff), intersected with the flag's read-site files; diffs of every overlapping PR grepped for the flag name in changed lines; gh pr list --search for the name, WR-3 and the read-site files.
Retire twist_simple_duw_sign (WR-46). The fix is right in closed form: Duw = (-per sin a, per cos a, 0) is the mixed partial of P = (u cos a - v sin a, u sin a + v cos a, w) with a = per w. Retiring it moves 127 rows:
surface.twist_volume eval_simple, 80 rows: PROVEN by the twist closed form (only Duw.y moves; every partial of order up to n matches).
surface.srf_in_volume eval and eval_simple, 46 rows: surface partials up to 3rd order through chains of twist, bend, unbend and transform maps. Approach: an independent certified check. Run forward-mode AD up to 3rd order in interval arithmetic through the whole chain, using the base surface jets, and require each new value to lie inside its AD interval (and each old value outside where it moved). Caveat: 3rd-order entries go through smvol_LiftThirdDerivative, which WR-45 (volume_lift_third_derivative_terms, off in goldens) says is wrong. Those entries may stay wrong after this fix, so retire WR-45 first or together, or enclose them against the WR-45-corrected lift.
surface.volume_p2 gen0106 (bbox_simple), 1 row: needs a maintainer decision. The unbend pseudo box (12 sampled spans of edges mapped through a twist) moves by up to 1.07e-6 in interval 3 and about 1e-10 in its basis. It is not an enclosure before or after: 200k samples per edge put mapped edge points up to 3.3e-4 outside it (interval 1 min -2.16664 against the box's -2.16631). Neither value can be shown right; the box construction itself is the defect.
Data and diagnostic (b50-moved-rows-proof.md, b50_diag.rs) are in the gist above.
Status
The whole-diff review passed at 801df0e. Since then: the WR-3 addendum, body edits, and the published proof script pointed at after-extent3d.jsonl. Full gate (sc-gate.sh --changed, core plus craft, kernel rig and all goldens) on b912308, rebased onto c6adb44: all 24 steps PASS. core-tests 696 s, core-rig 144 s, core-goldens 283 units 275 s, craft-clippy, craft-wasm, craft-smlib-core.
Line checks against bounded surface parameter regions now require both segment endpoints to be inside the region. Segments with only one endpoint inside are rejected; checks for unbounded regions remain unchanged.
Updated expected results reflect the stricter endpoint-containment behavior across surface evaluation cases.
Auto incremental reviews are disabled on this repository.
Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.
You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.
Use the checkbox below for a quick retry:
Trigger review
No actionable comments were generated in the recent review.
Walkthrough
The bounded rectilinear parameter-domain check now requires both segment endpoints to lie inside the box. The kernel fix flag and the check’s kernel parameter are removed. Callers, tests, and expected golden results are updated.
The fix flag and kernel parameter are removed. Bounded rectilinear domains require both endpoints inside the box; unbounded domains remain accepted. The addendum records the change.
Callers and containment tests solidcore/crates/goldens/src/units/*volume*.rs, solidcore/crates/solidcore/tests/*
Callers use the revised predicate signature. Tests remove the fix-flag mode and check that segments with one endpoint outside are rejected.
Updated surface golden results solidcore/crates/goldens/expected/surface.b_spline_volume.jsonl, solidcore/crates/goldens/expected/surface.volume_p2.jsonl
Expected result flags change for affected generated and predicate cases. Other recorded values remain unchanged in the affected outputs.
This retires a compatibility flag so the bounded parameter-domain check always requires both segment endpoints inside the box. SolidCraft's behavior is reported as unchanged. Only default-kernel results for segments with one endpoint outside change, and the matching golden outputs were updated. No merge-blocking risk was found.
Security architecture risk: Minimal · up to b9123
The change makes bounded-volume containment stricter and removes its compatibility switch. Inspected callers preserve rejection of out-of-domain inputs; no introduced security or failure-containment risk was established.
Retained concerns
No architecture-level concerns identified.
Security review details
Security Blast Radius
inferred — For the changed bounded branch, head acceptance is a subset of base acceptance: requiring both endpoint tests cannot accept a segment rejected by their either-endpoint combination. The demonstrated impact is stricter geometric validation, not expanded input acceptance.
Trust Boundaries and Controls
observed — Compounded validation still runs when the initial check succeeds and compounding is requested, and its result is ANDed with the initial result. Removing the flag does not remove that downstream check for accepted segments.
Pre-merge checks: 5
Check name
Status
Explanation
Title check
The title names the change: retiring the extent3d_contains_both_segment_endpoints upstream flag, with the behavior change (segment ends must lie inside the knot box). The changeset removes the flag fr…
Description check
The description follows the repository template closely. It has a Change section explaining the flag removal and resulting behavior, though it leans on internal identifiers (WR-3, UpstreamFixes, B50) …
Linked Issues check
Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check
Check skipped because no linked issues were found for this pull request.
Existing Defaults Unchanged
The check fails only for a changed UX default, such as a default setting, preference, command behaviour, UI layout, or shortcut. Kernel or geometry results under solidcore/ are explicitly not UX defau…
Full details: Title check
Explanation
The title names the change: retiring the extent3d_contains_both_segment_endpoints upstream flag, with the behavior change (segment ends must lie inside the knot box). The changeset removes the flag from core.rs, kernel.rs, fixes.json, the issue_2260 test, the volume p1.rs check, and the golden call sites, so the title matches the main change. It is somewhat technical but specific and accurate.
Full details: Description check
Explanation
The description follows the repository template closely. It has a Change section explaining the flag removal and resulting behavior, though it leans on internal identifiers (WR-3, UpstreamFixes, B50) and does not define them for a newcomer. It includes the required 'Changes a default: no' line with an explanation that the kernel default changes, and it gives a 'Test change:' reason for the modified WR-3 test and golden rows. Validation is covered with the gate run, per-row proof, and overlap check. The template's screenshot section is not applicable here because the change has no observable UI or app behavior, and the description does not say so explicitly. The Integration status section is partly addressed through the Status section, which records the rebase and gate run, but it does not give a clear upstream commit and PR status check list. Overall the description is mostly complete.
Full details: Existing Defaults Unchanged
Explanation
The check fails only for a changed UX default, such as a default setting, preference, command behaviour, UI layout, or shortcut. Kernel or geometry results under solidcore/ are explicitly not UX defaults and pass. Evidence gathered: - The PR changes 13 files. Most are under solidcore/ (kernel.rs, surface/volume/p1.rs, tests, goldens, plan addendum). The changed kernel-level behaviour is Volume::is_line_in_param_domain_simple, which now requires both segment ends inside the bounded parameter box. The PR description states that Kernel::default() results change for segments with one end outside the box. That is a geometry or kernel result change, which the check excludes. - The only file outside solidcore/ and the goldens is crates/smlib/src/core.rs. Its diff removes one line, f.extent3d_contains_both_segment_endpoints = true;, from the fixes() preset. The PR description and the objectives state that the app's preset already enabled this flag, so the app's results do not change. This is a kernel configuration line, not a user-facing default setting, preference, or UI behaviour. - The description says Changes a default: no. That statement does not conflict with the code. The only default that changes is Kernel::default() inside solidcore, which the check excludes. No UX default is changed. No changed user-visible default was found, so the check passes.
Finishing Touches
Generate unit tests (beta)
Create a new PR
Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 7 remain after this review.
brett19
changed the title
solidcore: retire the surface-volume flags twist_simple_duw_sign and extent3d_contains_both_segment_endpoints
solidcore: retire extent3d_contains_both_segment_endpoints (segment ends outside the knot box)
Nov 14, 2026
CI on b912308 (run 43552494377): the only REGRESSION line in Native Linux tests is solidcraft-ui-egui::viewport::appearance_tests::theme_switches_refresh_cached_viewport_colours_without_remeshing, which is the open flake #16468. It passes on this branch rebased onto d20e582 (engine, CLI and ui-egui suites run locally). The fillet and naming-corpus failures are on tests/KNOWN_FAILING_TESTS. main's own CI on d20e582 (run 43558293196) fails Native Linux tests with solidcraft-engine::tests::an_older_pattern_of_several_bodies_opens_with_the_names_it_was_saved_with. That test fails the same way here and does not touch solidcore.
Written by Claude Code (Anthropic) on behalf of @brett19.
A bounded rectilinear volume's line-in-domain check needs both segment
ends in the parameter box (tangency-labs#14246 batch B50, WR-3). A box is convex, so
that is exactly the segment inside it; upstream's ContainsLineSeg3d
accepted a segment with either end in. is_line_in_param_domain_simple no
longer needs the kernel.
SolidCraft's preset and the rig's fixes.json had it on. The test's
flag-off half goes; the on half stays.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01SdcgEtwSrdwZxqTQp5MfnU
…main
With extent3d_contains_both_segment_endpoints retired, 119 rows move,
each a line-in-domain answer 1 -> 0; nothing else in any unit moves:
- surface.b_spline_volume preds, 96 rows: in_line 1 -> 0.
- surface.volume_p2 line_in_in_space, 23 rows: result 1 -> 0 (first map
a B-spline with no orient map, so parameter = in-space points).
Proof per row (closed form, checked in the PR's proof harness): one end
of the segment lies outside the volume's natural parameter box (first to
last knot per direction) and the other inside, so the segment is not in
the box; the old 1 came from upstream's either-end test. The smallest
margin outside is 1e-7 (z = 1.0000001 against a box ending at 1).
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01SdcgEtwSrdwZxqTQp5MfnU
… partials) (tangency-labs#16482)
* solidcore: claim retiring the surface-volume flags twist_simple_duw_sign and extent3d_contains_both_segment_endpoints
Part of tangency-labs#14246 (batch B50).
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01SdcgEtwSrdwZxqTQp5MfnU
* solidcore: retire extent3d_contains_both_segment_endpoints
A bounded rectilinear volume's line-in-domain check needs both segment
ends in the parameter box (tangency-labs#14246 batch B50, WR-3). A box is convex, so
that is exactly the segment inside it; upstream's ContainsLineSeg3d
accepted a segment with either end in. is_line_in_param_domain_simple no
longer needs the kernel.
SolidCraft's preset and the rig's fixes.json had it on. The test's
flag-off half goes; the on half stays.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01SdcgEtwSrdwZxqTQp5MfnU
* goldens: segments with one end outside the knot box are not in the domain
With extent3d_contains_both_segment_endpoints retired, 119 rows move,
each a line-in-domain answer 1 -> 0; nothing else in any unit moves:
- surface.b_spline_volume preds, 96 rows: in_line 1 -> 0.
- surface.volume_p2 line_in_in_space, 23 rows: result 1 -> 0 (first map
a B-spline with no orient map, so parameter = in-space points).
Proof per row (closed form, checked in the PR's proof harness): one end
of the segment lies outside the volume's natural parameter box (first to
last knot per direction) and the other inside, so the segment is not in
the box; the old 1 came from upstream's either-end test. The smallest
margin outside is 1e-7 (z = 1.0000001 against a box ending at 1).
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01SdcgEtwSrdwZxqTQp5MfnU
* solidcore: claim retiring twist_simple_duw_sign (WR-46)
Stacked on tangency-labs#16442. Retires the twist Duw sign flag; the moved
srf_in_volume rows get an interval AD certificate.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01SdcgEtwSrdwZxqTQp5MfnU
* solidcore: retire twist_simple_duw_sign
A twist volume's simple evaluation always returns Duw = (-per sin a,
per cos a, 0), the w derivative of its first partial Du = (cos a,
sin a, 0) (tangency-labs#14246 batch B50, WR-46). Upstream's EvaluateSimple writes
-per cos a for the y component; the twist's own Evaluate already used
the right sign.
SolidCraft's preset and the rig's fixes.json had it on. The WR-46
test's flag-off halves go; the simple evaluation is now checked against
the twist's closed form for every partial up to third order.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01SdcgEtwSrdwZxqTQp5MfnU
* goldens: a twist's simple Duw is the w derivative of its Du
Retiring twist_simple_duw_sign moves 122 golden lines (WR-46):
surface.twist_volume eval_simple (76, only Duw.y flips, every partial
matches the twist's closed form) and surface.srf_in_volume eval and
eval_simple (46, second and third partials through twist, bend, unbend
and transform chains, each entry inside an interval forward-mode AD
enclosure; third order against the pinned WR-45 lift).
surface.volume_p2 gen0106 moves to known_failing.json: its unbend pseudo
box is not an enclosure before or after the change, so which box is
right is left to a maintainer.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01SdcgEtwSrdwZxqTQp5MfnU
---------
Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
approved-for-ciReviewed: run the full PR CI (build, tests, goldens, Clippy)ready to mergereviewed, every ask addressed; merge once CI is green
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
brett19
changed the title
bherbruck
added
approved-for-ci
Retires the
UpstreamFixesflagextent3d_contains_both_segment_endpoints(WR-3). A bounded rectilinear volume's line-in-domain check (Volume::is_line_in_param_domain_simple) now always needs both segment ends inside the natural parameter box. A box is convex, so that holds exactly when the whole segment is inside. Upstream'sContainsLineSeg3daccepted a segment with either end inside. Removed: the field, its preset line incrates/smlib/src/core.rs, itsfixes.jsonandissue_2260list entries, and the flag-off branch. The WR-3 test's flag-off half goes.is_line_in_param_domain_simpleno longer takes the kernel; its 5 goldens call sites drop the argument. The addendumplan/upstream-issues/WR-3--addendum-2026-11-14-flag-removed-extent3d-contains-both-segment-endpoints.mdrecords the removal.Part of #14246 (B50). The batch's other flag,
twist_simple_duw_sign, stays as it is on main; see Available follow-ups.Changes a default: no
SolidCraft's preset already had the flag on, so the app's results don't change. solidcore's
Kernel::default()does change: a segment with one end outside the box is no longer in the domain. The goldens run on that default, so they move (below).Test change: the WR-3 test's flag-off half tested the retired upstream path and is removed; the on-half expectations are unchanged. 119 golden rows move, each proven below.
Moved goldens (119 rows, one expectations commit)
Measured on main a7468ec (all goldens pass again after the rebase onto c6adb44) with the flag retired: 283 units, 281 pass, 2 move. Before and after values were taken on one machine. Rows were taken one by one from a bless run; no other row of any unit changed.
surface.b_spline_volume(preds)in_line1 -> 0surface.volume_p2(line_in_in_space)The smallest outside margin is 1e-7 (
r2_line_*_5: z = 1.0000001 against a box ending at 1). The comparisons are exact float comparisons. Per-row table (case, before, after, the outside end, the box and the margin), harness and data: https://gist.github.com/brett19/5b91eb84e93bb1f2908d16c00d54d0f1 (extent3d-moved-rows-proof.md,proof-extent3d.py,before-b50.jsonl,after-extent3d.jsonl).Certificate checklist
contains_pointwith tolerance 0), with no arithmetic beyond comparing.Extent3d::contains_point, which this PR leaves unchanged, is written as a negated OR of</>tests, so a NaN coordinate counts as inside, both before and after. This PR only replaces the OR of the two endpoint tests with an AND, so a NaN end can no longer make up for a finite end that is outside. Makingcontains_pointrefuse NaN changes other callers, so it is filed as solidcore: Extent3d::contains_point treats NaN coordinates as inside (§7.7) #16459.5-6. No refusal size and no loops involved.
wr3_segment_box_containmentkeeps segments fully inside (accepted) and fully outside (refused) beside the one-end-in cases.Overlap check (all open PRs, any author, drafts included)
Method: every open PR's file list (GraphQL, 100 files per PR; PRs over 100 files checked by diff), intersected with the flag's read-site files; diffs of every overlapping PR grepped for the flag name in changed lines;
gh pr list --searchfor the name, WR-3 and the read-site files.surface/volume/p1.rsorwr3_segment_box_containment.rs.closed_smooth_pole_tangent_limit(#14246, WR-725) #15602, Retire revolve_analytic_profile_faces: revolved lines and arcs are analytic by default #15713, solidcore: retire three surface UpstreamFixes flags (#14246, batch A13b) #15901, solidcore: retire revolve_holed_profile_void (#14246, batch A15b) #15963, solidcore: the sweeps' passed-on statuses become the only path (#14246 batch S5) #16283, solidcore: the Setback beside curved faces is the default; retire fillet_setback_beside_curved_faces (#13832 step 4) #16328 and solidcore: STEP export writes inside-out tori with the right same_sense on the default kernel (#16250) #16332 edit the shared lists (core.rspreset,fixes.json,issue_2260_gn_pipe_refined_range_end.rs) at other lines. solidcore: retire three surface UpstreamFixes flags (#14246, batch A13b) #15901 shows the name only as context. Whichever PR lands second rebases.Available follow-ups
twist_simple_duw_sign(WR-46). The fix is right in closed form:Duw = (-per sin a, per cos a, 0)is the mixed partial ofP = (u cos a - v sin a, u sin a + v cos a, w)witha = per w. Retiring it moves 127 rows:surface.twist_volumeeval_simple, 80 rows: PROVEN by the twist closed form (onlyDuw.ymoves; every partial of order up to n matches).surface.srf_in_volumeeval and eval_simple, 46 rows: surface partials up to 3rd order through chains of twist, bend, unbend and transform maps. Approach: an independent certified check. Run forward-mode AD up to 3rd order in interval arithmetic through the whole chain, using the base surface jets, and require each new value to lie inside its AD interval (and each old value outside where it moved). Caveat: 3rd-order entries go throughsmvol_LiftThirdDerivative, which WR-45 (volume_lift_third_derivative_terms, off in goldens) says is wrong. Those entries may stay wrong after this fix, so retire WR-45 first or together, or enclose them against the WR-45-corrected lift.surface.volume_p2gen0106 (bbox_simple), 1 row: needs a maintainer decision. The unbend pseudo box (12 sampled spans of edges mapped through a twist) moves by up to 1.07e-6 in interval 3 and about 1e-10 in its basis. It is not an enclosure before or after: 200k samples per edge put mapped edge points up to 3.3e-4 outside it (interval 1 min -2.16664 against the box's -2.16631). Neither value can be shown right; the box construction itself is the defect.Data and diagnostic (
b50-moved-rows-proof.md,b50_diag.rs) are in the gist above.Status
The whole-diff review passed at 801df0e. Since then: the WR-3 addendum, body edits, and the published proof script pointed at
after-extent3d.jsonl. Full gate (sc-gate.sh --changed, core plus craft, kernel rig and all goldens) on b912308, rebased onto c6adb44: all 24 steps PASS. core-tests 696 s, core-rig 144 s, core-goldens 283 units 275 s, craft-clippy, craft-wasm, craft-smlib-core.🤖 Generated with Claude Code
https://claude.ai/code/session_01SdcgEtwSrdwZxqTQp5MfnU
Summary by CodeRabbit