Refactor v2.5 - #25
Open
aka-blackboots wants to merge 185 commits into
Open
aka-blackboots wants to merge 185 commits into
aka-blackboots wants to merge 185 commits into
Conversation
Restructure the 2.5 kernel and Three.js SDK to the planned layer tree and enforce the coding standards mechanically, with behaviour proven unchanged: the debug kernel snapshot (461 cases), the SDK snapshot and the API surface are byte-identical to the Phase A baseline, the parity oracle output equals its fixtures, and every gate passes on a clean rebuild. Kernel: one-way layer order, curated public API (776 to 322 paths), every non-test function at most 80 lines and 7 parameters, shared helpers consolidated, tests/source_rules with frozen one-way exception lists, a test-support crate, string errors typed. SDK: split into dto, kernel, world-graph, rendering, marks, bodies, picking, export and runtime modules behind a 63-line facade; strict tsconfig, typed ESLint with layer rules, source, cycle and duplicate checks, a testing entry, and the scripts and browser tests in their planned homes.
npm run check runs every Rust and SDK gate from main-v2.5 in a fixed order, logs each step to .check/<step>.log, keeps going past failures and exits non-zero with the failed step names. check:full adds the Playwright runs on three 0.168 and 0.184. --only and --from pick a subset.
…orted Covers --only, --from, --skip, the empty selection, unknown names and missing values, the test:wasm rename, and the performance and browser flake classifier, before verify.mjs exports them.
Clippy gets its own target dir so cargo re-lints only what changed without touching sources. A rotated20BooleanMs regression is retried twice and the memory disposal browser test is repeated five times before either counts as a failure; any other regressed metric is marked INVESTIGATE. Browser ports, Playwright output and vite caches follow OG_PW_PORT_BASE and the three version, so the 168 and 184 runs overlap. Step selection gains --skip, fails on empty selections and bad arguments with one line, and verify.mjs runs steps only as the entry script so its pure functions are testable.
… STEP Finding O:orc-1: batch subtraction had no oracle. These tests replay a batch-matrix fixture corpus recorded from main; they fail until the oracle writes it. The STEP matrix loop becomes one helper shared by the boolean and batch matrices.
Finding O:orc-1: subtract_planar_cutters had no oracle. The traced copy now records subtract_planar_cutters and its batch handlers, and a batch module rebuilds the snapshot's batch scenes (13 entries, 4 ordered scenes and the staged mixed scene in both orders, 23 rows) with main's builders, writing each row's result, handlers and STEP files to batch-matrix/, plus a serial fallback row for non-mixed coverage gaps. The STEP writer moves to write.rs so matrix and batch share it; matrix output is unchanged. Five helpers repeat the snapshot's across kernels and get allowed duplicate groups.
…ode per context Retires the frozen operations->world_graph upward imports and module cycle, and adds tests that SingularParameterization takes the code of its graph context and that every OperationError keeps its code and message as a GraphError. Both fail until OperationError and ErrorContext exist.
…form The five boolean .tess.json files are never read and no matrix or batch row has a tessellation fixture, so a regression in the ported trimmed-face refinement goes unnoticed. Main numbers refinement midpoints from a HashSet, so the comparison is made over an order-free canonical form built in test-support. The batch fixture listing moves to tests/parity/support.rs so the new test shares it.
The standards review of W-B1 found batch.rs sitting beside batch/, which breaks the mod.rs convention for directory modules, and parts.rs exposing upright, arc and Part's fields wider than their only users in parts.rs need. scene and ordered now sit below their last caller, and four duplicate-group reasons name the snapshot helpers as general fixtures rather than batch-only ones. No output changes.
operations no longer imports world_graph: creating steps return OperationError, which world_graph converts into GraphError with the same code, message and null details. CreatingOperation and ModifyingOperation move into world_graph, and operands.rs keeps its own invalid(). SingularParameterization now reports InvalidGeometry, and every graph entry point rewrites it by ErrorContext (Create/Rebuild to InvalidParameter, Transform to InvalidTransform). The frozen upward imports and the operations<->world_graph cycle are retired; the sweep line_edge duplicate pair is re-keyed and its merge deferred to the debt note.
…sults The oracle now tessellates every boolean fixture and every matrix and batch row whose result holds a BRep with main and with traced, asserts the two agree in an order-free canonical form, and writes it under tessellation/. Main numbers refinement midpoints from a HashSet, so vertex numbering is not stable across runs; the canonical form sorts vertices, triangles and outline segments so the fixtures are. The canonical helpers exist once per kernel, recorded as allowed duplicate groups.
The fixture-reading one-liner existed twice in the parity tests; it now lives once in support.rs beside parity_fixtures, both pub(super) like every other test-crate support module. The oracle's canonical.rs lists its steps below their caller in call order and separates every function with a blank line.
The standards verifier found invalid declared pub(super), which from operations/mod.rs is crate-wide, while its only callers are descendants of operations; private is the narrowest visibility that compiles. The blank line in sweep/mod.rs was left by the removed world_graph import.
86a80b4 removed a blank line in sweep/mod.rs, moving line_edge from line 270 to 269; the allowlist member now matches what check:duplicates reports. Hash, count and reason are unchanged.
The blank line added before canonical_vertices moved the later canonical functions down one line; the allowlist members now name the lines the duplicate check reports.
Pins D6 (a name over 4 KiB is LimitExceeded, empty stays InvalidParameter), D7 (the exportStep node list is exempt from the 64 KiB params cap, capped at 100,000 ids, ids over 1,024 chars are InvalidParameter) and the export of a Body's children in stored order. params.rs moves to params/mod.rs so its tests sit beside it.
Pins typed ErrorDetails on GraphError: every GeometryError variant keeps its code in every context, a failing chained operate names its handlers, tool index and og_ids, a failing multi-body export names the body, and the bindings DTO keeps the error JSON byte-identical. Retires the frozen serde_json entries on world_graph/error.rs.
expand_export now walks every node's children depth-first in stored order, so Solids parented under a Body are exported and Wires or Sheets under it are reported in skipped (S:step-4). A name over 4 KiB is LimitExceeded, separate from the InvalidParameter option faults (D6). exportStep reads its node list through params::nodes, which is exempt from the 64 KiB params cap, caps the list at 100,000 ids without allocating the extra one and rejects ids over 1,024 chars as it reads (D7).
…ar sweep seams stay bitwise watertight The validity test named 20 of the 28 verdict fixtures and skipped arc, circle, cuboid and the five builder-box and builder-sphere-cut pairs. Looping over every verdict file in the folder keeps new fixtures from being skipped. Refinement midpoints on the circular L-path sweep reuse the boundary sample they coincide with in uv; evaluating the surface there instead leaves seam edges that differ in the last bits and break bitwise watertightness. No parity fixture reaches the reuse, so it stays and this test pins it.
GraphError::Code now carries ErrorDetails instead of a serde_json Value, so error.rs leaves the frozen serde_json lists. Operate failures raised by the boolean handlers keep the handlers recorded so far, the failing tool's index and the operand og_ids; multi-body STEP export failures in preflight or emission name the failing body. The bindings build the error JSON through ErrorDto with unchanged bytes, and the snapshot tool records every GraphError as the same code/details/message JSON.
rustfmt wraps the exported_matrix_rows signature. The step_oracle, parity, wasm_core, invariants and validity tests and the golden test pass after the step-1 deletion.
The folder holds plain test inputs and the expected results that remaining tests need, so it takes a neutral name: tests/fixtures/parity becomes tests/fixtures/cases and the tests/parity binary becomes tests/cases, found by its folder now that the Cargo.toml [[test]] entry is gone. The helper parity_fixtures becomes case_files. No test, golden module or golden key is renamed.
CORPUS.md described the recording of the 2.0 answers by a tool that no longer exists. TARGETS.md now names the deferred wasm work as a comparison and says where the boolean matrix rows are read from.
The migration guide's current-version code must stay correct as the API changes, so the README gate now runs its type check and fence check over MIGRATION.md too. Red until the guide exists.
People and coding assistants need one plain place that says how to move existing code to a new version. MIGRATION.md holds the 2.0 to 2.5 entry and the rules for adding the next one; the README points to it once, the gate type-checks its ts blocks, verify.yml runs when it changes, and the release steps ask for an entry when a release breaks existing code.
Some listed changes do not compile unchanged, so the guide no longer says they all still compile.
The review of wave W-I1 found rows that change behaviour without saying so: the sweep profile, the setPlacement pivot and anchor, getReport, the STEP units of exportBrepToStep, getBrepSerialized's return type, and items listed as removed that 2.5 partly replaces. Each row and bullet now says what an applier must do, rows that behave differently carry a mark, and a short "Partly replaced" part maps WorldGraph, the pattern helpers and two AnalyticSolid kinds. developer.md gets one paragraph re-wrapped.
The second review of W-I1 found that the sweep row gave a profile turned about the path differently from 2.0 without saying so, and that the rotation, scale, STEP, getBrep, WorldGraph and linearExtrusion lines held only in unstated cases. The guide now says how xDirection sets the turn, when the profile comes out mirrored, and the conditions under which each row holds.
The README's version note said that npm still installs 2.0, which stops being true the moment 2.5.0 is published, and a release step asked for it to be removed by hand. It now tells the reader how to check the installed line, so it holds on both sides of the release and the manual step is gone. The README no longer lists PDF export among things 2.0 had in the browser build. developer.md now says what the migration guide check covers, that the guide is not shipped, and what the stored expected results under tests/fixtures/cases are and why no command regenerates them. MIGRATION.md takes the last review's wording points: the 2.0 AnalyticSolid cuboid row, the placement arguments of the 2.0 Line and Polyline, where to PLACE a polyline before turning it again, the sweep bullet split in two, what dispose frees, and the missing parts of "Adding an entry".
The cases, STEP oracle, validity and WebAssembly tests still spoke of "source", "ported", "main" or "parity", words from when they compared against the 2.0 kernel. They now compare against stored expected results, so their names say "stored". Two module files follow: graph_parity.rs is graph_stored_step.rs and the golden parity_fixtures.rs is boolean_fixtures.rs. No assertion, input or golden key changes, and the same number of tests run. The golden matrix reader no longer skips ".step." files, since the boolean-matrix folder holds none; its 18-row count check still guards the folder. TARGETS.md now names what the golden test reads from disk, says the five fixture booleans are built in code, and notes that a CI runner on another macOS version can differ from the recorded Mac file.
The W-J1 review found four sentences that said more than the tree supports: the 2.5 quick-start light is there because the block builds its own scene, the tessellation tests read only the twelve builder bodies, there is one golden file per target, and only the math library can differ between macOS versions.
… bullet The close-out audit of W-J1 and W-J2 found that no step sent readers to "Check your result", that the meaning of ? came after the table already used it, and that the sweep row named a bullet that had been split in two.
On CI the golden test must build every scene without comparing or recording, only the aarch64-apple-darwin golden is recorded, and the timing budgets are not checked there. These tests describe that before the code does.
A CI or release run could not pass until someone started the record job by hand, downloaded its artifacts, reviewed them and committed them, once per platform. When CI is true the golden test now builds every scene and compares nothing, the timing budgets are measured but not checked, and nothing is recorded. The Linux golden and the record job go, and the documents say where each check runs.
On CI the kernel tests must stop comparing results computed through the machine's math library digit for digit with results stored from one Mac, and the golden test must not fail on a boolean matrix row whose result differs from its stored one. These tests describe the switch and the golden finding rule before the code does.
The release workflow runs the kernel tests on Linux, whose math library rounds some values differently from the Mac the stored results came from, so the exact comparisons with files under tests/fixtures/cases could never pass there. When CI is true those comparisons are skipped, while every case still runs with its other checks, rows whose stored result is an error are still compared, and the golden test ignores only a matrix row's result disagreement. The developer guide and TARGETS.md say where each comparison runs.
The corpus checks still compare a boolean matrix row's handlers on CI and on a target with no golden file, so 'compares nothing' and 'nothing is compared' overstated it. TARGETS.md also said every key is the hash of record text, but sweep-3d hashes raw outputs.
…gates need The golden file hashes case records, handler listings and extra texts, so 'record text' was too narrow. CI runs on Ubuntu and macOS and compares on neither. The release step said 'locally', but the comparisons it relies on run only with CI unset, and two of them only on a machine with a golden file for its target and a timings block.
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
No description provided.