Skip to content

test(geometry): pin the batched-suffix over-cut that solo_step now avoids - #4024

Merged
louistrue merged 4 commits into
mainfrom
fix-solo-step-batched-suffix
Sep 7, 2026
Merged

test(geometry): pin the batched-suffix over-cut that solo_step now avoids#4024
louistrue merged 4 commits into
mainfrom
fix-solo-step-batched-suffix

Conversation

@BIMvoice

@BIMvoice BIMvoice commented Sep 6, 2026

Copy link
Copy Markdown
Collaborator

Update — this PR now carries a genuine reproduction. The section below was written when only a guard test existed and is kept for the record, but it is no longer the whole story.

Commit 0731ae397 adds solo_step_batched_suffix_defect_is_active_on_a_synthetic_bowtie_cutter: a 10x10x10 box cut by a 3-cutter DIFFERENCE chain whose outermost cutter has a self-intersecting (bowtie) IfcPolyline boundary. That trips the accept gate in the top-level 3-cutter batch, and again at that node's single-cutter step once the inner 2-cutter suffix batches cleanly — reaching spine.len() == 1 && based_on_batch == true.

Verified RED/GREEN by reverting only the mod.rs / single_cutter_gate.rs hunk:

with fix:     max bounds ~ (5, 5, 10)   KeepUncut, correct
fix reverted: max bounds =  (3, 3,  2)   FallThrough over-cut

Reproduced identically under csg_manifold_gate, csg_topology_gate, and both together. A reviewer independently confirmed the decision point is genuinely reached, by instrumenting the line: spine.len()=1 based_on_batch=true solo_step=false, with an accept-gate rejection present in take_failures() — so it is not a false pass from an empty failure list.

Honest scope of that fixture. A self-intersecting boundary is synthetic and structurally malformed; no real authoring tool would emit one. Checked against the spec: IFC4X3's formal propositions for IfcPolygonalBoundedHalfSpace require only PolygonalBoundary.Dim = 2 and a curve type of IfcPolyline / IfcCompositeCurve / IfcIndexedPolyCurve — there is no machine-checked WHERE-rule demanding a simple polygon, so this input is not schema-invalid, merely degenerate. It demonstrates the defect is reachable in principle; it does not establish that a valid real-world model reaches it. On main, with the fixtures in this repo, the defect remains latent — entity #2145 hits spine.len()==1 && based_on_batch==true but never triggers a gate rejection there, and every entity that does reject has spine.len() in {2,3,4,5}.

The original guard test (solo_step_accounts_for_a_batched_suffix_not_just_spine_length) still passes but provides no RED/GREEN coverage of this defect — it never reaches the buggy branch on the real #960 fixture. It retains value as a smoke test over real production geometry; its docstring overstates its purpose relative to what it actually catches, and is worth trimming if you would rather it not mislead.

Summary

Carries the one remaining unique commit from PR #3922's diverged branch (fix-issue-3919-roof-clip-gate), which has drifted 1000+ lines from main in unrelated files. #3922's own original fix already landed separately as f7872db62; this PR isolates only the remaining change so #3922 itself can be dispositioned separately by the maintainer without touching or closing it.

solo_step in BooleanClippingProcessor::process_with_depth_inner decides, per DIFFERENCE chain, whether an accept-gate rejection on a IfcPolygonalBoundedHalfSpace cutter's subtract should KeepUncut (a sibling cutter likely covers the same material) or escalate to the riskier unbounded FallThrough clip. It was computed as spine.len() == 1 — but the chain-resolution loop retries try_union_polygonal_chain at every suffix level, so when the top-level batch fails and a shorter suffix batches cleanly, the caller's spine holds only the single outermost node even though that node's cutter has siblings already folded into the mesh by the successful suffix batch below it. spine.len() == 1 can't distinguish that from a true no-sibling shape, so it wrongly escalated to FallThrough — an over-cut in exactly the case !solo_step exists to prevent.

Fixed by tracking whether the mesh entering the sequential-apply loop already came from a successful batch (based_on_batch), and only treating a length-1 spine as truly solo when it did not: spine.len() == 1 && !based_on_batch.

Measured impact

Entering House.ifc's real chain directly at entity #2146 (issue #960 fixture, treated as a representation root): before the fix, an accept-gate rejection on #2146's own cutter over-cuts to max Z ≈ 2735.6 mm (FallThrough); after the fix, it correctly keeps the batched result un-cut at max Z ≈ 4475.3 mm (KeepUncut).

Honesty about the regression test

This defect is latent on main today, not active. Entering the #960 fixture at #2146 currently produces spine.len() == 2, not 1, so the buggy formula isn't reached by that fixture as-is. I scanned every DIFFERENCE-chain root across all five chains (~50 nodes) in the only in-repo fixture with this shape and found none that simultaneously hits spine.len() == 1, based_on_batch == true, AND an actual accept-gate rejection at that node. So the new regression test, solo_step_accounts_for_a_batched_suffix_not_just_spine_length, passes both before and after this fix on main — it is a guard against a real, structurally-possible defect (proven via direct instrumentation of spine.len()/based_on_batch, and via the code-path analysis above), not a live reproduction. It exists to catch a regression once an in-repo fixture reaches that branch, not to demonstrate one exists today.

Controls (run individually, targeted binaries)

  • cargo test -p ifc-lite-geometry --test issue_960_segmented_roof_clip — pass
  • cargo test -p ifc-lite-geometry --test issue_1007_real_opening_no_bridge — pass (real numbers printed: worst_rim_incident_aspect=7.74 worst_aspect=25.90 open_boundary_edges=0 out_tris=98)
  • cargo test -p ifc-lite-geometry --test issue_3353_near_coplanar_rotated_overlap — pass (4/4)
  • cargo test -p ifc-lite-geometry --lib a_three_operand_near_coplanar_union_stays_closed — pass

Other gates

  • cargo clippy -p ifc-lite-geometry -- -D warnings — clean
  • cargo clippy -p ifc-lite-core -- -D warnings — clean (no pre-existing failures reproduced in this checkout)
  • cargo test -p ifc-lite-processing --test module_size_ratchet — pass (6/6)
  • node scripts/check-module-size.mjs — OK
  • node scripts/check-test-wiring.mjs — OK
  • node scripts/check-source-text-assertions.mjs — OK

Closes #4023

apply_boolean_step's per-cutter accept-gate fallback used
`spine.len() == 1` as a single, spine-wide `solo_step` flag to decide
whether a gate rejection should KeepUncut (a sibling cutter likely
covers the same material) or FallThrough to the unbounded plane clip
(genuinely no sibling).

But process_with_depth_inner's PBHS-chain resolution retries
try_union_polygonal_chain at every suffix level: when the top-level
batch fails and a shorter suffix succeeds, the caller's `spine` holds
only the outermost node even though its cutter has siblings already
folded into the mesh by that successful suffix batch. `spine.len() ==
1` in that case wrongly reported "no sibling", sending a gate
rejection to the riskier FallThrough instead of KeepUncut -- an
over-cut in exactly the case `!solo_step` exists to prevent.

Track whether the mesh entering the sequential-apply loop came from a
successful batch (`based_on_batch`) and only treat a length-1 spine as
truly solo when it did not. Measured on House.ifc's real chain (entity
#2146, issue #960 fixture) entered directly as a representation root:
before the fix, an accept-gate rejection there over-cuts to max Z
~2735.6mm (FallThrough); after, it correctly keeps the batched result
un-cut at max Z ~4475.3mm (KeepUncut).

This defect is latent on main today: entering the #960 fixture at
#2146 currently produces spine.len() == 2, not 1, so the buggy formula
isn't reached by that fixture, and a scan of every DIFFERENCE-chain
root in the fixture found none that hits spine.len() == 1 with an
actual accept-gate rejection. The new regression test
(solo_step_accounts_for_a_batched_suffix_not_just_spine_length) passes
both before and after this fix -- it is a guard against a real,
structurally-possible defect, not a live reproduction.

Closes #4023

Claude-Session: https://claude.ai/code/session_01QPHChk3Ve9N519A4kY7436
@BIMvoice
BIMvoice requested a review from louistrue as a code owner September 6, 2026 10:37
@coderabbitai

coderabbitai Bot commented Sep 6, 2026

Copy link
Copy Markdown

Warning

Review limit reached

  • Run on-demand review

This review includes 1 billable file and costs up to $0.25.

Or wait 59 minutes for your next included review.

Check out review usage here.

View limit details

Limit details: You’ve used all 2 included reviews currently available.

Learn how review limits work.

Review configuration:

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Team

Run ID: 4213709d-05df-4879-877f-c7defe352997

📥 Commits

Reviewing files that changed from the base of the PR and between 4f341b6 and 9145a1d.

📒 Files selected for processing (1)
  • rust/geometry/src/processors/boolean/chain_cycle_tests.rs

Comment @coderabbitai help to get the list of available commands.

@github-actions

github-actions Bot commented Sep 6, 2026

Copy link
Copy Markdown
Contributor

Claude review - no findings for 55c037ca0

Reviewed this diff and found nothing to flag.

@github-actions

github-actions Bot commented Sep 6, 2026

Copy link
Copy Markdown
Contributor

Viewer benchmark

✅ No threshold regressions detected.

01_Snowdon_Towers_Sample_Structural(1).ifc

Baseline recorded 2026-07-01T20:31:05.538Z on github-actions ubuntu-latest, viewer-benchmark-ci (headless Chrome, SwiftShader ANGLE), production build.

Metric Current Baseline Delta Threshold Status
firstBatchWaitMs 1121ms 2905ms -61.4% +50%
firstVisibleGeometryMs 1877ms 3652ms -48.6% +50%
streamCompleteMs 1881ms 3598ms -47.7% +50%
spatialReadyMs 969ms 1032ms -6.1% +50%
metadataCompleteMs 1189ms 3063ms -61.2% +50%
totalWallClockMs 2000ms 3700ms -45.9% +50%

AC20-FZK-Haus.ifc

Baseline recorded 2026-07-01T20:30:59.972Z on github-actions ubuntu-latest, viewer-benchmark-ci (headless Chrome, SwiftShader ANGLE), production build.

Metric Current Baseline Delta Threshold Status
firstBatchWaitMs 178ms 1075ms -83.4% +50%
firstVisibleGeometryMs 733ms 1572ms -53.4% +50%
streamCompleteMs 744ms 1980ms -62.4% +50%
spatialReadyMs 772ms 915ms -15.6% +50%
metadataCompleteMs 866ms 1392ms -37.8% +50%
totalWallClockMs 1000ms 3300ms -69.7% +50%

Refresh the baseline from a CI run: dispatch the Benchmark workflow with record_baseline, download the benchmark-baseline artifact, and commit baseline.json (see tests/benchmark/README.md).

@github-actions github-actions Bot added the llm-reviewed A review was verified as posted for this PR's head. label Sep 6, 2026
@louistrue louistrue added the unqueued Maintainer waiver: this PR may merge without closing a ready issue. label Sep 6, 2026
@louistrue

Copy link
Copy Markdown
Collaborator

Merge-readiness nudge: this PR remains blocked by the changed-test revert oracle and aggregate Build + WASM + Rust + Node gate. The production change needs a discriminating regression test that fails when the fix is reverted; then refresh onto current main and rerun all gates.

…a synthetic bowtie cutter

The existing guard (solo_step_accounts_for_a_batched_suffix_not_just_spine_length)
never reaches an accept-gate rejection on the real #960 fixture at #2146, so
reverting the based_on_batch fix leaves it green -- the revert oracle correctly
flags this as UNOBSERVED.

Adds a small hand-written STEP fixture (a plain box cut by a 3-cutter
DIFFERENCE chain) whose outermost cutter has a self-intersecting (bowtie)
polygonal boundary. That trips the accept gate both in the outer 3-cutter
batch attempt (deferring it) and again at the outer node's own single-cutter
step once the inner 2-cutter suffix has batched cleanly -- the exact
spine.len()==1 && based_on_batch==true shape the fix targets. Verified by
hand: reverting the fix flips this new test from green (max bounds ~(5,5,10),
KeepUncut) to a real assertion failure (max bounds (3,3,2), the FallThrough
over-cut), under csg_manifold_gate, csg_topology_gate, and both together.

Claude-Session: https://claude.ai/code/session_01QPHChk3Ve9N519A4kY7436
@vercel

vercel Bot commented Sep 6, 2026

Copy link
Copy Markdown
Contributor

The latest updates on your projects. Learn more about Vercel for GitHub.

2 Skipped Deployments
Project Deployment Actions Updated
ifc-lite-dev Ignored Ignored Sep 7, 2026 4:01pm UTC
ifc-lite-viewer-embed Ignored Ignored Sep 7, 2026 4:01pm UTC

@BIMvoice

BIMvoice commented Sep 6, 2026

Copy link
Copy Markdown
Collaborator Author

Update: constructed a synthetic fixture that makes the defect active (option 1 from the review), so the revert oracle should now observe a real regression instead of reporting UNOBSERVED.

Added solo_step_batched_suffix_defect_is_active_on_a_synthetic_bowtie_cutter (rust/geometry/src/processors/boolean/chain_cycle_tests.rs, feature-gated same as the existing guard): a hand-written 10x10x10-box fixture cut by a 3-cutter DIFFERENCE chain whose outermost cutter (cutterE) has a self-intersecting (bowtie) IfcPolyline boundary. That trips the accept gate (OpenTopologyRejected under csg_topology_gate, NonManifoldRejected under csg_manifold_gate) both in the outer 3-cutter batch attempt at the top node (deferring it) and again at that node's own single-cutter step once the inner 2-cutter suffix has batched cleanly — exactly the spine.len() == 1 && based_on_batch == true shape this PR's fix targets.

Verified by hand (reverting just the mod.rs/single_cutter_gate.rs fix and re-running):

  • With the fix: max bounds ≈ (5, 5, 10) — KeepUncut, cutterE dropped, the batched suffix returned un-cut.
  • With the fix reverted: max bounds = (3, 3, 2) — a real assertion failure, the FallThrough over-cut.

Confirmed under csg_manifold_gate, csg_topology_gate, and both features together. The original real-fixture guard (solo_step_accounts_for_a_batched_suffix_not_just_spine_length) is left in place as a regression pin for #960's own geometry; this new test is the one that should satisfy the revert oracle.

Also re-ran the required controls on top of this change: issue_960_segmented_roof_clip (pass), issue_1007_real_opening_no_bridge (pass — worst_rim_incident_aspect=7.74 worst_aspect=25.90 open_boundary_edges=0 out_tris=98, unchanged), issue_3353_near_coplanar_rotated_overlap (4/4 pass), a_three_operand_near_coplanar_union_stays_closed (pass). cargo clippy -p ifc-lite-geometry -- -D warnings (plus with each gate feature) and cargo test -p ifc-lite-processing --test module_size_ratchet are clean.

Pushed to this branch.

@github-actions github-actions Bot removed the llm-reviewed A review was verified as posted for this PR's head. label Sep 6, 2026
@github-actions

github-actions Bot commented Sep 6, 2026

Copy link
Copy Markdown
Contributor

Claude review - no findings for 0731ae397

Reviewed this diff and found nothing to flag.

@github-actions github-actions Bot added the llm-reviewed A review was verified as posted for this PR's head. label Sep 6, 2026
@BIMvoice

BIMvoice commented Sep 6, 2026

Copy link
Copy Markdown
Collaborator Author

Changed tests observe production reports UNOBSERVED on this PR, and after investigating it is a structural blind spot in the oracle, not a gap in this PR's testing. Escalating rather than working around it.

What the oracle sees

baseline:  pass — 1273 passed / 1273 collected
reverted:  pass — 1273 passed, 0 failed / 1273 collected
✘ UNOBSERVED

Identical collection counts, because the reproducing test is not in the 1273. solo_step_batched_suffix_defect_is_active_on_a_synthetic_bowtie_cutter is gated #[cfg(any(feature = "csg_manifold_gate", feature = "csg_topology_gate"))], rust/geometry/Cargo.toml:19 sets default = [], and cargoRunner() in scripts/lib/revert-oracle.mjs:187-190 hardcodes ['test', '--no-fail-fast', '-p', crate] with no --features mechanism anywhere in the oracle. Confirmed locally: cargo test -p ifc-lite-geometry --lib chain_cycle_tests under default features collects 18 tests in that module, and the gated one is not among them.

Why neither obvious fix applies

Running it under default features is a known regression. Cargo.toml:56-70 documents exactly why those gates are opt-in — enabling them takes issue_098_reveal_wall from 42 to 380 unpaired edges, issue_098_v5c from 108 to 416, and regrows issue_960_segmented_roof_clip's full-height seam sliver. The accept-gate rejection this fixture depends on is only reachable behind the gates.

Teaching the oracle the features is not possible today — there is no per-crate feature config in check-test-revert-oracle.mjs or revert-oracle-cargo.mjs.

The test does work, and CI does run it

Reverting only the solo_step line in mod.rs:

RED:   max bounds = [3.0, 3.0, 2.0]   (panic, --features csg_manifold_gate)
GREEN: test ... ok

And .github/workflows/test.yml already runs cargo test -p ifc-lite-geometry --features csg_manifold_gate (line 1496) and --features csg_topology_gate,csg_manifold_gate (lines 1668-1682) as their own jobs — so a genuine regression here would be caught by the suite. It is only the oracle's own invocation that cannot see it.

Controls all pass individually: issue_960_segmented_roof_clip ok; issue_1007_real_opening_no_bridge ok (open_boundary_edges=0 out_tris=98); issue_3353_near_coplanar_rotated_overlap 4/4; a_three_operand_near_coplanar_union_stays_closed ok. Clippy clean, module-size ratchet 6/6.

Your call

  1. Apply the maintainer-only revert-oracle-exempt label here.
  2. Give the oracle a per-crate feature list so feature-gated Rust tests are visible to it.

This is the same shape as #4050, where the oracle cannot see Python tests: in both cases it reports UNOBSERVED for work it simply did not run, which is indistinguishable from work that genuinely observes nothing. That is the failure direction that erodes the check.

We did not delete the feature gate, weaken the oracle, or add a token production edit to satisfy it — it caught a real false all-clear in #3982 this morning and is worth more intact than this PR is worth merged today.

@BIMvoice

BIMvoice commented Sep 7, 2026

Copy link
Copy Markdown
Collaborator Author

Perf verdict — #4024 (base-vs-branch, interleaved)

Method: base = merge-base with upstream/main (f2a9f5daf0), branch = PR head (0731ae397b). Built cargo build --profile profiling -p ifc-lite-processing --example perf_probe at each commit, copied the two binaries out, restored the worktree, then ran perf_probe <fixture> --iters 5 --json interleaved base→branch, 3 rounds, on both required fixtures.

tests/models/ara3d/AC20-FZK-Haus.ifc (default fixture)

round base total (best-of-5) branch total (best-of-5)
1 9 ms 8 ms
2 9 ms 9 ms
3 9 ms 8 ms

Base spread 0 ms, branch spread 1 ms, mean delta ≈ −0.7 ms. Noise floor ≥ delta — not a measurable difference. Mesh/vert/tri counts identical on every run (285 meshes / 35 940 verts / 19 456 tris). No CSG accept-gate rejection occurs on this fixture at all (perf_probe only prints a csg failures line when the count is >0; it never appeared for this fixture in any of the 6 runs) — the KeepUncut/FallThrough fallback this PR changes is structurally unreachable here, independent of timing.

tests/models/ara3d/ISSUE_129_N1540_17_EXE_MOD_448200_02_09_11SMC_IGC_V17.ifc (CSG-heavy)

round base total (best-of-5) branch total (best-of-5)
1 645 ms 646 ms
2 661 ms 651 ms
3 658 ms 665 ms

Base mean 654.7 ms, spread 16 ms (645–661). Branch mean 654.0 ms, spread 19 ms (646–665). Mean delta ≈ −0.7 ms, i.e. well inside the base-side run-to-run spread (16–19 ms) — this is noise, not signal.

This fixture does hit the accept gate: csg failures 43 across 6 products (plus degenerate dropped 6), identically on base and branch, in all 6 runs. Mesh/vert/tri output is byte-identical on every run (1402 meshes / 218 326 verts / 132 674 tris). Given the PR's own reported effect size for an actual KeepUncut vs FallThrough divergence (max Z 2735.6 mm vs 4475.3 mm on the #960/entity-#2146 case — a large geometric change), an identical mesh/vertex/triangle count on every one of the 43 rejections rules out any of them landing on a node where spine.len() == 1 && based_on_batch differs from spine.len() == 1 alone. So the changed branch is not reached differently on this fixture either.

Interpretation

#4024 changes which fallback (KeepUncut vs the unbounded FallThrough clip) runs when an accept gate rejects an IfcPolygonalBoundedHalfSpace cutter subtract, for the specific case spine.len() == 1 && based_on_batch == true. On both required fixtures:

  • AC20-FZK-Haus.ifc never triggers an accept-gate rejection at all — the changed code is not executed.
  • ISSUE_129 triggers 43 rejections, but byte-identical mesh/vertex/triangle counts base vs branch show none of them land on the specific based_on_batch case this PR changes.

Verdict: no measurable perf difference, and none is expected — the changed branch is not reached on either required fixture. This is consistent with the PR's own honesty note that the defect is latent on main's in-repo real-world fixtures (only the synthetic bowtie unit test added in this PR reaches it). The observed ~0.7 ms deltas on both fixtures are noise (smaller than the base-side spread), not a regression or a win.

Byte-identity: mesh/vertex/triangle counts (and CSG-failure counts on ISSUE_129) match exactly between base and branch on every one of the 6 runs per fixture.

@BIMvoice

BIMvoice commented Sep 7, 2026

Copy link
Copy Markdown
Collaborator Author

Reviewed adversarially — the PR is sound; its blocker is elsewhere

Traced based_on_batch end to end. Set in process_with_depth_inner (initialised false, flipped true only when try_union_polygonal_chain returns Some), read once at solo_step = spine.len() == 1 && !based_on_batch. The if cutters.len() < 2 { return Ok(None); } guard rules out the failure mode I was most concerned about — a trivial one-cutter "batch" setting the bit and masking a genuine solo cutter. Not dropped at any layer; single read site.

Behaviour changes in exactly one case (spine.len()==1 && based_on_batch), and single_cutter_gate.rs's diff is doc-comment-only. On diagnostics it removes a spurious PolygonalBoundedHalfSpaceFallback record in the corrected case rather than adding one — relevant given the live double-count work in #4067/#4083/#4089. Operand ordering (promote_operands_mutually, plane_weld.rs) is untouched.

RED/GREEN verified by real reverse-patch, not by trusting the PR body — reverting the mod.rs/single_cutter_gate.rs hunks flips solo_step_batched_suffix_defect_is_active_on_a_synthetic_bowtie_cutter RED under both gates independently:

[baseline] csg_manifold_gate -> pass 1260, fail 0
[reverted] csg_manifold_gate -> pass 1259, fail 1
[baseline] csg_topology_gate -> pass 1258, fail 1   (pre-existing, unrelated)
[reverted] csg_topology_gate -> pass 1257, fail 2

chain_cycle_tests.rs calls real production types, no mocking.

On the oracle failure: I had been saying #4090 would clear it. That is wrong — I verified it by running #4090's own oracle code against this PR's head and got BASELINE-BROKEN, not OBSERVED. Enabling --features csg_topology_gate compiles a pre-existing fixture-dependent census test that panics with "no fixtures on disk", because the revert-oracle job checks out lfs: false and never fetches fixtures. Details and a suggested fix are on #4090.

So this PR's own logic is clean and its test is load-bearing; what stands between it and a green gate is the oracle job's environment, not anything in this diff.

@louistrue

Copy link
Copy Markdown
Collaborator

Triage: same shape as #4089, and the revert-oracle is correct rather than broken.

production reverted: rust/geometry/src/processors/boolean/mod.rs
                     rust/geometry/src/processors/boolean/single_cutter_gate.rs
test:                rust/geometry/src/processors/boolean/chain_cycle_tests.rs
baseline: pass 1273/1273
reverted: pass 1273/1273, failed 0

With the batched-sibling bit reverted, chain_cycle_tests.rs still passes 1273 of 1273. So the test does not observe the production change.

The issue (#4023) describes a latent over-cut, which is exactly the hard case: if the current code never reaches the over-cut on the fixture, a test written against that fixture passes either way. The fix needs a fixture that actually produces a batched suffix with already-folded siblings, and an assertion on the resulting geometry rather than on the mechanism you changed.

Local check before pushing: git stash the production hunk, run chain_cycle_tests, confirm it FAILS. That is the whole contract the oracle is enforcing.

louistrue pushed a commit that referenced this pull request Sep 7, 2026
…ing UNOBSERVED (#4079)

* fix(oracle): recognize Python test_*.py/*_test.py and give plan.runner a Python path

The revert oracle's TEST_FILE_RE and plan.runner knew JS/TS suffixes and Rust
_tests.rs but nothing about Python's test_*.py / *_test.py convention, so a
real file in this repo -- tools/ifcopenshell_reference/test_harness.py -- was
classified as production, and a branch whose real coverage is Python code was
reported UNOBSERVED indistinguishably from a branch that genuinely tests
nothing.

- TEST_FILE_RE (scripts/lib/revert-oracle.mjs) now also matches Python's
  test_*.py / *_test.py convention.
- New scripts/lib/revert-oracle-python.mjs: pythonTestOwner() walks up to the
  nearest Python project marker (requirements.lock/.txt, pyproject.toml,
  setup.py/.cfg, Pipfile) the way cargoTestOwner() walks up to a Cargo.toml;
  pythonRunner() invokes python3 -B -m pytest (-B: no __pycache__, which would
  otherwise leave an untracked file behind and fail the oracle's own
  byte-identical restoration check); parsePython() reads pytest's summary
  line, verified against real pytest 9.1.1 / CPython 3.14 output for pass,
  fail, collection-error, skip, and no-tests-ran.
- A missing pytest module prints an unquoted "No module named pytest" to
  stderr; that string is folded into RUNNER_MISSING_PATTERNS so it reports the
  same way an absent cargo/vitest binary already does, never as a silent pass.
  A missing python3 interpreter itself is already caught generically via
  spawnSync's ENOENT. A pytest collection error (a test's own import of a
  missing package, e.g. ifcopenshell) is reported as a load failure, which
  read on a baseline run becomes an honest BASELINE-BROKEN rather than a
  vacuous pass.
- scripts/check-test-revert-oracle.mjs: planRuns() routes a *.py test file to
  pythonTestOwner()/pythonRunner() before falling back to the package.json
  walk; resolveBin() resolves the python3 binary literally, mirroring cargo.

Both existing script files stayed at their recorded module-size budget (528
and 612 lines) by moving the new surface into the sibling file rather than
growing either.

Cargo per-crate feature gating (#4024's csg_manifold_gate/csg_topology_gate)
is a separate, larger change to how the oracle invokes cargo and is not
attempted here; see the PR description for the assessment.

Refs #4050

* fix(scripts): raise check-test-revert-oracle.mjs's module-size budget to its true merged line count

This PR's own tip pins the file at exactly 612 lines (its recorded budget),
via a ~9-line Python routing hook in planRuns(). Independently on main,
the already-merged Dependabot revert-oracle exemption (#4084) added its own
~9-line hook (a CI early-return plus one import), landing main at 611 lines
under the same 612 budget. Both PRs pass their own check-module-size in
isolation; merged together the file is 621 lines against a 612 budget, so
this reproduces failing as soon as this branch merges onto current main.

Both hooks already followed the file's existing split convention -- pushing
their substantive logic into a sibling lib/revert-oracle-*.mjs (python.mjs
here, dependabot.mjs for #4084) and leaving only thin wiring (an import plus
a few dispatch lines) in check-test-revert-oracle.mjs itself. There is no
further logic left in either added hunk worth extracting: the file's
remaining growth is the dispatcher's own routing table (planRuns()'s
per-file-type branches, the CI early-exit), which is what a dispatcher is
for. Splitting further to satisfy arithmetic would be artificial, so this
raises the budget instead, per AGENTS.md's "prefer splitting to allowlisting"
guidance being a preference, not an absolute.

Set to 621 -- the measured line count of check-test-revert-oracle.mjs on
this branch merged with current upstream/main -- not a round number with
headroom. Since this branch's own tip is 612, `check-module-size.mjs` will
report 9 lines of "headroom" against the raised budget until main's #4084
hook is also present in this branch's history; that is expected, not a
missed measurement, since the budget is deliberately sized for the merged
tree the PR is about to land into, not the branch's own tip.
@louistrue

Copy link
Copy Markdown
Collaborator

Heads-up on the module-size budget, from reviewing the hotfix for main.

main is currently red on rust/processing/tests/module_size_ratchet.rs: #3922 merged from a base 88 commits stale and took rust/geometry/src/processors/boolean/mod.rs to 948 lines against its 936 budget. #4101 fixes that by splitting the trait wiring into a sibling router_impl.rs, leaving mod.rs at 920.

What that means for this PR. Your mod.rs hunk is net +14 (comments plus the based_on_batch flag). On top of #4101 you land at 934 against a budget of 936. It passes, with two lines to spare.

So: after you rebase, do not add lines to boolean/mod.rs while resolving conflicts. Two lines is not much room, and the conflict will land right where #4101 moved code out. Your other files are fine (single_cutter_gate.rs is 96 lines, and chain_cycle_tests.rs is a test file, which the rule exempts).

This PR is currently CONFLICTING and based on f2a9f5d, 44 commits behind main, which predates #3922 entirely. It needs a rebase before it can merge regardless.

Separately, the revert-oracle finding on this PR still stands and is the more important one: with your production hunk reverted, chain_cycle_tests.rs still passes 1273 of 1273. The test does not yet observe the fix.

# Conflicts:
#	rust/geometry/src/processors/boolean/chain_cycle_tests.rs
@github-actions

github-actions Bot commented Sep 7, 2026

Copy link
Copy Markdown
Contributor

Claude review - no findings for 3ebc832c4

Reviewed this diff and found nothing to flag.

@github-actions github-actions Bot added llm-reviewed A review was verified as posted for this PR's head. and removed llm-reviewed A review was verified as posted for this PR's head. labels Sep 7, 2026
@BIMvoice

BIMvoice commented Sep 7, 2026

Copy link
Copy Markdown
Collaborator Author

Rebased to 3ebc832c4 (MERGEABLE again) — but its core fix is now redundant, and only the test is unique.

#3922 merged at 13:31 today carrying the identical based_on_batch/solo_step change. After merging main in here, boolean/mod.rs is byte-identical to upstream/main (git hash-object matches, 2a797dcab…), and diffing this branch's own tip against main on that file shows comment-text changes only — a doc rewrite from #3980, zero non-comment line differences.

So the logic this PR was opened for is already on main. What remains unique is the new synthetic test in chain_cycle_tests.rs: solo_step_batched_suffix_defect_is_active_on_a_synthetic_bowtie_cutter.

That test is worth keeping on its own merits — it is a genuine guard for a fix that currently has one, and I verified earlier by reverse-patching that reverting the mod.rs hunks flips it RED under both gates independently. But this PR is now effectively a test-only contribution, which may change how you want to handle it.

Conflict: trivial add/add at end-of-file in chain_cycle_tests.rs, exactly as predicted. mod.rs auto-merged with zero conflict — the three-way impl GeometryProcessor extraction collision between #4101 and #4089 did not materialise here.

Both-parents diff: 1 file, 165 insertions, 0 deletions vs main — pure addition, no silent revert.

Verification, -p ifc-lite-geometry --lib, all three feature combinations, new test included each time:

csg_manifold_gate                    20 passed
csg_topology_gate                    20 passed
csg_manifold_gate,csg_topology_gate  20 passed

On the failing gates, neither is caused by this branch:

@louistrue

Copy link
Copy Markdown
Collaborator

Correction: my earlier triage on this PR was wrong twice over.

I said the revert-oracle showed the test does not observe the production change, and warned you about a 2-line margin on boolean/mod.rs. Both were stale.

1. This PR no longer contains a production change at all. git diff --name-only origin/main...origin/fix-solo-step-batched-suffix returns exactly one file: rust/geometry/src/processors/boolean/chain_cycle_tests.rs. The based_on_batch fix reached main through #3922 (9dbe8c429, merged today). So the oracle's correct verdict on the current head is NOT APPLICABLE: this branch changes no production files, and it exits green. There is nothing for it to revert. My margin warning is moot for the same reason: you do not touch that file any more.

2. The observing test is real, and the oracle structurally cannot see it. solo_step_batched_suffix_defect_is_active_on_a_synthetic_bowtie_cutter sits under #[cfg(any(feature = "csg_manifold_gate", feature = "csg_topology_gate"))]. Both are default = [] opt-ins, and the oracle's runner is fixed to cargo test -p <crate> with no features, so cargo test -p ifc-lite-geometry --lib solo_step reports 0 passed; 860 filtered out.

That gate is correct and should stay. On a default build the fix is behaviourally inert: the only consumer of solo_step is resolve_single_cutter_subtract's gate_rejected && !solo_step branch, and gate_rejected needs OpenTopologyRejected | NonManifoldRejected, which only the feature-gated rejections record. No default-build test can observe this fix, so removing the cfg would produce a test that passes for the wrong reason.

Under the feature it discriminates properly. Reverting the one line to let solo_step = spine.len() == 1;:

panicked at chain_cycle_tests.rs:1119
max bounds = [3.0, 3.0, 2.0], expected ~(5, 5, 10)

(5,5,10) is the batched extent of the centred 10x10x10 extrusion, derivable from the STEP text without running the code; (3,3,2) is the FallThrough plane clip. The gate firing is asserted separately via take_failures(), so a fixture that stopped tearing would fail loudly rather than pass on a bounds coincidence.

What is actually left here, none of it code:

Worth saying plainly: do not add a --features mode to the oracle to make it judge this test, and do not add an exemption label. The gate already passes for the right reason. If we want the oracle to cover feature-gated tests, that is a change to scripts/lib/revert-oracle*.mjs and its own issue.

@github-actions github-actions Bot removed the llm-reviewed A review was verified as posted for this PR's head. label Sep 7, 2026
@github-actions

github-actions Bot commented Sep 7, 2026

Copy link
Copy Markdown
Contributor

Claude review - no findings for 9145a1dad

Reviewed this diff and found nothing to flag.

@github-actions github-actions Bot added the llm-reviewed A review was verified as posted for this PR's head. label Sep 7, 2026
louistrue added a commit that referenced this pull request Sep 7, 2026
* fix(review): let the revert-oracle see feature-gated Rust tests

cargoRunner() always ran a crate's default feature set, so a Rust test
gated `#[cfg(feature = "x")]` (whole-file `#![cfg(...)]` or item-level,
directly above `#[test]`) compiled out entirely — the oracle collected
the same test count before and after a revert and reported UNOBSERVED
for a change it never even compiled in. This is the Rust half of the
gap #4050 described; #4079 closed the Python half and deliberately left
this one out.

scripts/lib/revert-oracle-rust-features.mjs reads a changed/added Rust
test file's cfg attributes and turns any(...)/all(...)/bare feature
gates into the required --features combination(s). planRuns() now
spawns one cargo plan per required combo per crate, falling back to
the existing single default-features plan when a file has no feature
gate at all (the overwhelming majority of Rust branches see zero
behavior change).

Both directions proven on a synthetic scratch crate matching #4024's
own item-level any(gate_a, gate_b) shape: a gated test that asserts on
the changed value reads OBSERVED when reverted; one that does not
reads UNOBSERVED, not a silent pass.

Note: the "csg_manifold_gate and csg_topology_gate must not be enabled
together" premise carried over from #4079's scoping note does not hold
in this codebase -- test.yml runs the combined
--features csg_manifold_gate,csg_topology_gate as its own CI job, and
issue_098_v5c.rs has live #[cfg(all(...))] cases for both together. No
exclusivity handling was needed as a result; any() maps to separate
per-feature plans and all() maps to one joint combo.

No budget raised: revert-oracle.mjs stays at 528/528 lines and
check-test-revert-oracle.mjs at 612/612.

Refs #4050, #4079, #4024, #4085.

Claude-Session: https://claude.ai/code/session_01QPHChk3Ve9N519A4kY7436

* fix(review): make revert-oracle Rust feature-gate parsing loud on unhandled cfg shapes (#4085)

Adversarial review of hunt-revert-oracle-rust-features (9981ebf) found
three defects in revert-oracle-rust-features.mjs:

1. The doc comment claimed only bare/any()/all() cfg shapes occur in this
   repo's Rust test tree and that "nested any(all(...)) does not occur."
   False: not(...) gates a real #[test] today at
   rust/geometry/tests/triangulation_invariance.rs:2168,2188 (not(any(...)))
   and rust/geometry/tests/issue_582_583_regression_test.rs:198 plus
   rust/geometry/src/csg/csg_tests.rs (bare not(feature = "...")). Corrected
   the comment to name these sites instead of asserting they don't exist.

2. detectRequiredFeatureCombos silently returned [] for any cfg shape it
   could not parse (not(...), nesting beyond one level, cfg_attr(feature =
   "x", test), an attribute between #[cfg] and #[test]). planRuns() then
   fell back to a default-only plan, so a test gated by one of these shapes
   never compiled under any plan and the run reported BASELINE-BROKEN with
   0 collected — reproduced against the live triangulation_invariance.rs and
   issue_582_583_regression_test.rs files directly. Chose the smaller,
   explicitly-preferred fix: detect each unhandled shape and throw
   UnhandledCfgShapeError naming the file, line, and shape, rather than
   extending the parser to genuinely evaluate not(...) — real negation
   support needs planRuns() to always include the default no-features plan
   alongside explicit combos (today it only falls back to default when zero
   combos are found at all), which is a caller-contract change, not a
   parser extension.

3. A line- or block-commented-out `#[cfg(feature = "ghost")]` above
   #[test] was read as a real gate. Added stripComments() so commented-out
   cfg attributes are ignored, verified not to mask a real gate on the next
   test in the same file.

Did not add a plan-count cap (the lower-priority item in the review) — out
of scope for this pass; flagging for whoever picks it up next.

Tests: 9 new cases in revert-oracle-rust-features.test.mjs, including the
real not(any(...)) shape from triangulation_invariance.rs and the
all(not(A), B) idiom from issue_098_v5c.rs:119-136, both asserting the loud
failure with correct file/line. node --test scripts/lib/revert-oracle*.test.mjs:
68 before -> 77 after. Mutation check: commenting out the not(...) throw
turns exactly the 3 targeting tests red (17 pass / 3 fail), confirming they
catch a reversion to the old silent behavior.

Verified unchanged: cargoRunner(crate, []) still emits exactly
["test","--no-fail-fast","-p",crate] (no --features flag) for an ungated
file, and both directions of the branch's own synthetic-crate proof
(revert-oracle-rust-feature-gate.test.mjs) still pass: OBSERVED when the
gated test asserts on the change, UNOBSERVED when it doesn't.

scripts/check-test-revert-oracle.mjs and scripts/lib/revert-oracle.mjs are
untouched — both sit at their exact module-size budget (612 and 528 lines)
with zero headroom, so all new logic lives in the already-uncapped sibling
module revert-oracle-rust-features.mjs (now 186 lines). check-module-size.mjs,
check-test-wiring.mjs, and check-source-text-assertions.mjs all pass.

Also noted for the maintainer: this branch conflicts with #4079 (open,
adds Python support to the same oracle) — both add a branch inside
planRuns()'s per-group loop and both edit the same import block in
check-test-revert-oracle.mjs. Confirmed with a direct diff against
upstream/pull/4079/head; semantically reconcilable into one
if/else if/else, needs hand-merging by whoever merges second.

Claude-Session: https://claude.ai/code/session_01QPHChk3Ve9N519A4kY7436

* fix(review): deliver UnhandledCfgShapeError as a structured failure, make stripComments string-literal aware

Adversarial review of hunt-revert-oracle-rust-features (254d78e, #4085)
found two more defects, both the same failure mode the branch exists to
eliminate.

1. planRuns() is called at check-test-revert-oracle.mjs module top level,
   before the tool's own try{}/uncaughtException handler exist. A cfg shape
   detectRequiredFeatureCombos refuses to plan a run for was therefore a
   genuine unhandled exception: a raw stack trace on stderr, no JSON despite
   --json, and Node's default exit code of 1 -- which collides with
   EXIT_UNOBSERVED. Reproduced directly against a synthetic crate with a
   not(feature = "x") gate above a real #[test]: exit 1, no JSON, raw stack
   trace.

   Fixed by catching UnhandledCfgShapeError around the planRuns() call and
   handing it to die() with its own exit code (6, EXIT_UNHANDLED_CFG_SHAPE)
   -- die()'s ABORT formatting, JSON payload when --json is passed. Since
   check-test-revert-oracle.mjs is at its exact module-size budget (612
   lines, zero headroom -- see scripts/module-size-allowlist.txt), the new
   logic (the exit constant, the JSON shaping, and the try/catch itself)
   lives in the already-uncapped sibling lib/revert-oracle-rust-features.mjs
   as requiredFeaturePlanOrDie()/unhandledCfgShapeReport(), leaving the
   dispatcher's own diff at two single-line changes (the import, and the
   planRuns() call site) and its line count unchanged at 612.

2. stripComments() blanked from the first "//" to end of line with no notion
   of "am I inside a string", so `let s = "//"; #[cfg(feature = "x")]` read
   the "//" INSIDE the string literal as a comment start and erased the real
   cfg gate on that line -- detectRequiredFeatureCombos returned [] and the
   gated test would never compile in under any plan, exactly the silent
   false UNOBSERVED this branch fixes elsewhere. Reproduced directly.

   Rewrote stripComments as a single-pass scanner that tracks string/raw-
   string/char-literal state: double-quoted strings ("..." with \" / \\
   escapes, covering byte strings too since their escaping is identical),
   Rust raw strings (r"...", r#"..."#, ..., including br"..."), and char
   literals ('x', '\n', '\'', '\u{7f}'), distinguished from a lifetime ('a)
   by requiring a matching closing '. Line/column numbers are still
   preserved by blanking comment text to spaces rather than removing it.
   Not a full Rust lexer (no raw identifiers) but the reproduced shape --
   "//" inside an ordinary double-quoted string -- is handled rather than
   disclaimed away.

Tests: node --test scripts/lib/revert-oracle*.test.mjs: 77 pass before ->
85 after (1 new end-to-end synthetic-crate test for the exit-code defect,
7 new stripComments unit tests for the string-literal defect). Mutation-
checked both: reverting either fix turns its new test(s) red (confirmed
1 fail and 4 fail respectively) while leaving the fix in place keeps all
green.

Verified unchanged: cargoRunner(crate, []) still emits exactly
["test","--no-fail-fast","-p",crate]; both directions of the branch's own
synthetic-crate proof (revert-oracle-rust-feature-gate.test.mjs) still
pass (OBSERVED and UNOBSERVED); issue_098_v5c.rs still returns combos: []
cleanly with no throw (the not(...)/all(...) shapes there are on const
declarations, not directly above #[test], so the throw's scoping is
unaffected). check-module-size.mjs, check-test-wiring.mjs, and
check-source-text-assertions.mjs all pass.

Impact on the #4079 conflict (adds Python support to the same oracle,
touches the same import block and planRuns()'s per-group loop): this
change touches the import line (line 93) but not the planRuns() loop
itself, only the call site above it -- a smaller footprint than the
prior #4085 commit left, but still an additional line in that import
block for whoever reconciles the two branches.

Claude-Session: https://claude.ai/code/session_01QPHChk3Ve9N519A4kY7436

---------

Co-authored-by: Louis Trümpler <78563314+louistrue@users.noreply.github.com>
@louistrue louistrue reopened this Sep 7, 2026
@louistrue louistrue changed the title fix(geometry): thread a per-node batched-sibling bit through solo_step test(geometry): pin the batched-suffix over-cut that solo_step now avoids Sep 7, 2026
@louistrue

Copy link
Copy Markdown
Collaborator

Reopened deliberately, and retitled to say what it now is.

Why it should land even though it carries no production change. The based_on_batch fix reached main through #3922 (9dbe8c429). Closing this PR would have left main with the fix and no test observing it — nothing to catch the regression coming back. That is the shape this repo pays for most often, so the reproduction is worth keeping on its own.

The test is real in the only build where the behaviour exists. Reverting the one line on main to let solo_step = spine.len() == 1; gives:

panicked at chain_cycle_tests.rs
max bounds = [3.0, 3.0, 2.0], expected ~(5, 5, 10)

(5,5,10) is the batched extent of the centred 10x10x10 extrusion, derivable from the STEP text without running the code; (3,3,2) is the FallThrough plane clip. The gate firing is asserted separately via take_failures(), so a fixture that stopped tearing would fail loudly rather than pass on a bounds coincidence.

The oracle's NOT APPLICABLE here is correct, not an evasion. The branch changes no production files, so there is nothing to revert. And the test stays #[cfg(any(feature = "csg_manifold_gate", feature = "csg_topology_gate"))] on purpose: on a default build the fix is behaviourally inert, because gate_rejected is only ever set by the feature-gated rejections. Removing the cfg would produce a test that passes for the wrong reason. The lane that actually compiles it is "CSG accept gates (feature builds)".

Two things worth doing before this merges, neither of them code:

  1. Update the PR description to match the retitle: the fix is on main via fix(geometry): defer to sequential path when an accept gate rejects the roof-clip union subtract #3922, this is the feature-build reproduction, and paste the RED output above as the evidence the oracle structurally cannot produce for a feature-gated test.
  2. Trim the docstring on solo_step_accounts_for_a_batched_suffix_not_just_spine_length to what it actually does. It is a smoke test over the real IfcWallStandardCase geometry anomalies or missing #960 geometry that never reaches the gate-rejection branch (spine.len() is 2 there), so it passes with the formula reverted too. Leaving a docstring that implies otherwise is how the next reader concludes the fix is guarded when it is not.

And to close the loop: #4023 stays open until this lands, because until then the fix on main is genuinely unguarded.

@github-actions github-actions Bot added llm-reviewed A review was verified as posted for this PR's head. and removed llm-reviewed A review was verified as posted for this PR's head. labels Sep 7, 2026
louistrue added a commit that referenced this pull request Sep 7, 2026
)

* fix(ci): install the Python toolchain revert-oracle's pytest lane needs

#4079 taught scripts/lib/revert-oracle.mjs to classify test_*.py / *_test.py
as tests and route them through `python3 -B -m pytest` (see
scripts/lib/revert-oracle-python.mjs). The classification is correct, but
the revert-oracle job in test.yml installs no Python toolchain -- checkout,
pnpm, node, a build-artifact download, then the oracle. GitHub's Ubuntu
runner image ships python3 but not pytest, so any diff that reaches this
lane now fails with BASELINE-BROKEN (No module named pytest) instead of a
real verdict, blocking a required gate.

Adds a `python` output to the `changes` job's paths-filter (tools/
ifcopenshell_reference/** plus this workflow file) and gates two new
conditional steps on it: actions/setup-python, then `pip install -r
tools/ifcopenshell_reference/requirements.lock pytest`, mirroring
ifcopenshell-parity.yml's `full` job. The full requirements.lock, not bare
pytest: test_validate_export.py's SchemaConformanceHasTeeth cases (#4043)
import ifcopenshell and are unittest.skipUnless(HAVE_IFCOPENSHELL, ...), so
bare pytest would run them as silent skips instead of the assertions the
oracle needs to revert against. The job's own `if` (frontend || rust) is
unchanged -- this only gates the install steps, not job scheduling, so a
diff touching only tools/ifcopenshell_reference/** with nothing under
packages/apps/rust would still not trigger the job at all; that's a
separate, narrower gap than what's fixed here.

The live case is #4048, whose diff includes tools/ifcopenshell_reference/
test_validate_export.py.

Same class of problem as the Rust lane: this job also can't run
feature-gated Rust tests because it never fetches fixtures (see #4090's
thread), which makes #4024 fail the same way. Not fixed here -- separate
lane, separate fixture-fetch mechanism.

* fix(ci): widen the revert-oracle python filter to match pythonTestOwner's repo-wide reach

The `python` output on the `changes` job's paths-filter matched only
`tools/ifcopenshell_reference/**`, but `pythonTestOwner` in
scripts/lib/revert-oracle-python.mjs walks up from ANY `test_*.py` /
`*_test.py` file to the nearest project marker with no directory
restriction. Two locations already fell outside the filter:
scripts/perf/evidence/**/reproduce/test_*.py (owned by a nearby
requirements.txt) and rust/python/tests/test_bindings.py (owned by
rust/python/pyproject.toml, though in practice cargoTestOwner's
Cargo.toml-first walk claims that file before pythonTestOwner is ever
tried). A diff touching only the evidence fixture matched `rust` (job
runs) but not the old `python` filter (install skipped), so the
oracle routed the test to `python3 -m pytest` on a runner with no
pytest installed and failed BASELINE-BROKEN, exit 3 -- the exact
failure this workflow's python install step exists to prevent.

Replace the directory glob with `**/test_*.py` / `**/*_test.py`,
matching TEST_FILE_RE's python alternatives in
scripts/lib/revert-oracle.mjs byte-for-byte so the filter is derived
from the same basename patterns the routing itself keys on and can't
drift out of sync again the way the directory-scoped version did.

Also: correct two comments that said routing happens "under
tools/ifcopenshell_reference" -- it's marker-based and repo-wide --
and fix the `actions/setup-python` version label (SHA
5fda3b95a4ea91299a34e894583c3862153e4b97 is v7.0.0, not v5; the SHA
itself was already correct).

* chore: empty commit to re-trigger test.yml (missing lanes on #4102)

---------

Co-authored-by: Louis Trümpler <78563314+louistrue@users.noreply.github.com>
@louistrue

Copy link
Copy Markdown
Collaborator

Your Node tests red is not yours. Re-running it.

FAIL packages/parser/src/compact-entity-index-cache.test.ts
     > entity cache eviction after long scans (#3983)
Error: Test timed out in 5000ms.
Test Files  1 failed | 110 passed (111)

A TypeScript parser test, on a PR whose entire diff is rust/geometry/src/processors/boolean/chain_cycle_tests.rs. There is no path from your change to that code.

The cause is a flaky test rather than a real regression: the loop makes ~40,000 expect() calls (20,000 iterations, half of them a deep toEqual against a fresh object literal) against a fixed 5000ms vitest timeout, so the assertion machinery dominates the runtime and loses the race under CI load. Filed as #4110 with the fix, which is to collect mismatches and assert once rather than to raise the timeout.

So on this PR the remaining state is:

  • Node tests: flake, re-running, not yours.
  • Changed tests observe production: NOT APPLICABLE, correct, because the branch changes no production files.
  • The two prose items from my earlier comment still stand: update the description to say the fix is on main via fix(geometry): defer to sequential path when an accept gate rejects the roof-clip union subtract #3922 and this is the feature-build reproduction, and trim the docstring on solo_step_accounts_for_a_batched_suffix_not_just_spine_length since it never reaches the gate-rejection branch.

@louistrue
louistrue merged commit 271b83c into main Sep 7, 2026
120 of 124 checks passed
louistrue added a commit that referenced this pull request Sep 8, 2026
…#4110) (#4115)

`compact-entity-index-cache.test.ts` intermittently failed with "Test timed
out in 5000ms" and reddened PRs that could not reach the code, most recently
#4024, whose whole diff was a Rust test file.

The cause was assertion density, not the code under test. The 20k-entry scan
made ~40,000 `expect()` calls, half of them a deep `toEqual` against a freshly
allocated object literal, measured against a fixed 5000ms budget. Raising
`testTimeout` would only pick a number that is wrong on the next slower
runner, so the workload is calibrated instead: the scan still runs all 20,000
iterations and still checks, per iteration, that the hot entry was not evicted
and that every field of the fetched entry is right. It records what went wrong
and asserts once.

The failure message improves as a result. It now names up to ten offending ids
plus the total instead of aborting on the first, for both categories.

Two sibling tests in the same package had the same shape and are fixed the
same way: the 10,000-seed GlobalId round trip and the 8,400-token interner
lookup past the fast dictionary budget.

Verified by mutation, since a flaky test is easy to "fix" into one that cannot
fail. Removing the LRU recency refresh in `CompactEntityIndex.get` fails the
cache test naming 19,996 hot-entry reallocations from id 5; skewing the
byteOffset of every 977th entry fails it naming those 20 ids; skewing the
GlobalId first character fails the round trip naming 1,437 seeds; dropping a
byte past the interner budget fails it naming 208 tokens. Each source mutation
was reverted after its run.

Runtimes for the three tests, five runs each, before to after:
183-190ms to 8-9ms, 72-76ms to 25-28ms, 47-50ms to 5-6ms. The parser suite ran
ten more times, green every time.

`blocked-source-equivalence.test.ts` is the slowest remaining loop-with-assert
in the package at ~510ms, but it already carries an explicit 60s timeout and a
vacuity guard, and its cost is real block decompression rather than assertion
machinery. Left alone.


Claude-Session: https://claude.ai/code/session_0193douQ6sTYHE65DJmyAei9

Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

llm-reviewed A review was verified as posted for this PR's head. unqueued Maintainer waiver: this PR may merge without closing a ready issue.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Latent over-cut: solo_step ignores a batched-suffix's already-folded siblings

2 participants