Task 7: geodesic directions and the rotation permutation table - #14
Merged
Merged
Conversation
…able Task 7 is a lift, not a fresh implementation, and the plan's Step 3 code has been deleted rather than executed. `geodesic.rs` was written in `borbax-experiments` months of plan-time early because the G2 locality measurement could not run without a rotation table. The compiler settles whether that was right. Substituting the plan's own Step 3 for `geodesic.rs` and running the gate's clippy leg gives 71 errors — 38 indexing_slicing, 9 as_conversions (two genuine usize->u8 truncations), 9 unwrap_used, 3 disallowed_methods on f64::total_cmp (§13.4) and 2 on f64::min (§13.1), plus needless_range_loop, manual_midpoint and four missing_docs. The plan's code fails leg 2 of the six-leg gate in 71 places, five of them determinism violations, into a crate that has zero. It also had 11 tests against the shipped 16, and lacked `rotation_matrices` as real geometry, the test tying the integer table back to it, a per-element determinant check, and `minus_identity_is_not_in_the_group` — the exact hazard CLAUDE.md names. `experiments` now re-exports rather than keeping a copy. A divergence between the table G2 measured and the table the binding kernel searches would invalidate the measurement with nothing failing. That is an argument for one shared table, not one copy of the construction: `borbax_universe::packing` holds a third, deliberately, as an independent oracle answerable to nothing but Euler. The lift was not verbatim, and that is the part worth reading. Four comments asserted "this lifts into `borbax-molecule`" as a future event. One listed four measured claims and told its reader to re-measure on arrival rather than copy — so they were re-measured, and the third was false: antipodes are exact negations numerically (`a + b == 0.0` at every index, all three levels) but not bitwise, because the zero component of each (0, ±1, ±φ) vertex is stored `+0.0` and negates to `-0.0`. 12, 24 and 48 bit mismatches at D = 12, 42, 162 — exactly the zero-component counts — and zero numeric mismatches. Benign, since `dist2` sees the numeric equality, and untested either way until now. Two tests added for gaps the lift created. `is_identity`'s only coverage was a G2 call site that does not come with the module. And nothing tested the premise licensing the `total_cmp` exemption — that `dirs` holds no `-0.0` and no NaN. That second one is the important one: planting a `-0.0` silently reorders the entire direction set, and 17 of the 18 tests pass straight through it, because every other property is order-invariant. Three tests widened from D=42 to all three levels, including the only one tying the table to real rotations — confining the single geometric check to one D is how a level-dependent indexing error survives, and this project has already recorded the inverse mistake. The test module trades `.unwrap()` for `unwrap_or_else(|| unreachable!(…))`: `clippy.toml` sets no `allow-unwrap-in-tests`, so the workspace deny reaches inside `#[cfg(test)]` and a verbatim lift would have broken this crate's zero-bare-unwrap property. Every guard was run against the mutation it exists to stop. Reordering `perms` against the matrices fails exactly one test, the one whose doc claims to be the only tie to geometry. Building the 60 improper elements fails seven. `is_identity`'s tolerance was probed on both sides of its window. And one ulp on a single coordinate fails the antipode assertion, which is three orders inside the `< 1e-12` it replaced. MrReasonable <4990954+MrReasonable@users.noreply.github.com>
Five specialists plus a CodeRabbit CLI pass. The headline was found twice, independently, by the two lanes deliberately cross-routed onto each other's domains — which is the convergence this project treats as worth more than either lane's confidence. THE CANONICAL DIRECTION ORDERING WAS AN OUTPUT THAT NOTHING PINNED. Every one of the 18 tests asserted a property — group closure, bijectivity, det = +1, the anti involution, -I absent, matrix/table agreement, unit norms — and every one is invariant under relabelling `dirs` with the matching relabelling of `perms` and `anti`, which is exactly what a reordered sort produces. Measured: re-keying the sort (z,y,x) -> (x,y,z) left the whole 325-test workspace green. The realistic mutation is not a deliberate re-key, it is a tidy-up. Rewriting `normalise`'s `scale(a, 1.0 / n)` as componentwise division is mathematically identical, better rounded, unflagged by clippy, and is how `borbax_universe::packing`'s independent copy already spells it — and it moves six of the 162 index positions at D=162, because 121 of the 161 adjacent pairs share a bit-identical `z` and the tie is broken only by rounding. That was also green across the workspace. My own probe was the weaker half of the pair and I had stopped at it: it perturbs a coordinate AFTER the sort, where it breaks antipodal symmetry and is caught. Upstream of the sort, antipodal symmetry survives and nothing saw it. `the_canonical_direction_ordering_is_pinned` is the fix, FNV-1a over dirs, perms and anti at all three levels; the constants were reproduced independently of the lane that proposed them, and are identical in debug and release. Because the three `test` legs run on three platforms this is a working §13.4 golden today rather than at Task 20 — where `goldens --emit` currently exits 0 with "No golden artifacts yet". Corroborated by two reviewers each: - `GeoError::SubdivisionCountMismatch` replaces `UnsupportedResolution` at the `verts.len() != D` guard. `D` is already on the ladder when it fires, so the old variant printed "D must be 12, 42 or 162" when D was one of those — and it is the guard keeping `copy_from_slice` from panicking, so it is the one place the message is read. - `vertex_count` and `level_for` collapsed onto one LADDER table. They were two independent encodings of the same ladder, and the deferral I wrote for the overflow was wrong twice: the first overflowing level is 31, not 32, and the cost to "every call site" was zero — there were none outside its own test. At level 32 release wrapping returned 2. - `Vec3` made public; a `pub fn` signed with a private alias renders as a bare `[f64; 3]` beside a linkable `Mat3`. Claims of mine that were wrong or overstated, withdrawn: - phi/2 as "a constant of the icosahedral group". Max entrywise deviation is not a similarity invariant — conjugating by 0.3 rad gives 0.717 — so it holds only in the (0, ±1, ±φ) frame. Replaced with the basis-free bound 2*sqrt(2)*sin36/3 = 0.554, true in any orientation. - The portability inventory omitted `f64::midpoint`, the one operation here IEEE-754 does not specify. Now named, with the cross-ISA measurement that settles it: identical bit dump on aarch64 and x86-64 including -C target-cpu=x86-64-v3, and zero FMA-class instructions emitted. - Two tolerance comments compared the wrong quantity — a linear spacing against a squared tolerance, and "headroom over zero", which is not a quantity. - "ulp at 1.0" used in both directions 300 lines apart. - That Task 10 needs `is_identity`; its plan section asks for neither it nor `rotation_matrices`. - That a second copy is "the one thing that must never happen"; packing.rs holds a third, correctly. - "Five separate comments" (four) and "the fourth item" (the third), both corrected in the amended message of the previous commit. - experiments/README.md still advertised the file this branch deletes — the claims-land-in-more-places class, inside the commit whose message is about it. Routed forward with discriminators: §22.8's actual claim has no test anywhere in V0 (Task 8/9); Task 10's required probe, with the measurement showing a same-index fixture INVERTS rather than merely failing to discriminate; and packing.rs's six-position divergence against FRONTIER_COEFF (Task 20). MrReasonable <4990954+MrReasonable@users.noreply.github.com>
Two review findings taken together because both are cheap now and a breaking change once Task 10 has consumers. Ian's scope call. FIELDS ARE PRIVATE; `build` IS THE ONLY PRODUCER. Three invariants are established there and were unenforceable afterwards: `rotation_perms` closed under composition, `anti` an involution with `dirs[anti[i]] == -dirs[i]`, and every index `< D`. With the fields public a caller could desynchronise them — `g.dirs.reverse()` is enough — which reopens at runtime exactly the hazard the `-0.0` probe found at construction, and every test but the ordering golden passes through it because the rest are invariant under relabelling. `perms` BECAME `rotation_perms`, AND `contact_perms = anti . rotation_perms` IS PRECOMPOSED. Measured -13.21% at D=42 against `anti[perms[r][i]]`, noise floor 0.24%, five signature-pool sizes from 3 KB to 2.6 MB, two process launches agreeing to the timer quantum. -2.72% at D=162, where the kernel stops being L1 load-throughput bound. The transpose layout was measured and rejected at +0.00%/+0.24%; gather-then-sum at +11.6%/+57.2%. The rename is the load-bearing half, and the performance lane corrected me on this after I asserted the opposite in its brief. Precomposing alone does NOT close the ANTI hazard: a sibling field still called `perms` reads MORE plausible sitting next to the right one, so adding a table does not remove an affordance. And `perms` cannot be deleted — Task 9's `canonicalise` rotates a single signature with no contact involved and needs the raw permutation. Both tables have real consumers, so the hazard closes only through the names. `rotation_perms` inside a function called `affinity` is something a reviewer reads as wrong; `perms` was not. `contact_perms` deliberately gets no golden constant of its own — that would be a second encoding of a derived fact, free to drift from the composition it is supposed to be. Two tests pin it against the tables that do have constants, and both fail when the `anti` step is dropped (probed): `contact_perms_is_anti_composed_with_rotation_perms` and `contact_perms_is_never_the_rotation_table`, the second because the composed table is the improper coset and must never coincide with the rotation group. The change is bit-identical by construction — same values, same order, only the addressing differs — and `the_canonical_direction_ordering_is_pinned` did not move, which is what says so rather than the sentence above. Task 9's and Task 10's plan snippets were updated with it: Task 9 to `rotation_perms(r)`, Task 10's kernel to `contact_perms(r)` with a comment that the antipode is already applied. Nine call sites across the harnesses. 329 tests, six gate legs green. MrReasonable <4990954+MrReasonable@users.noreply.github.com>
CodeRabbit's second pass on the branch, plus a whole-repo sweep for the old
name. Six findings, four applied, two held.
The rename `perms` -> `rotation_perms` reached every call site — the compiler
saw to that — and left five prose references standing, in exactly the class
this project keeps recording: a correction lands where the compiler looks and
not where the reader does.
- `rotation_matrices`' doc still said "`perms` is what the binding kernel
needs". It is `contact_perms` now, and saying otherwise in the doc of the
function that exists to cross-check the tables is the worst place for it.
- The struct doc, the golden test's doc, `is_identity`'s withdrawal note and
the geometry-tie test's doc all named the old field.
- `experiments/src/signature.rs` carried two comments describing indexing
through `perms[r]` above code already reading `rotation_perms(r)`.
- The plan's Interfaces block still published `Geodesic::{dirs, perms, anti}`
as the produced surface, with no mention that the fields are now private.
- `build`'s `# Errors` section never gained `SubdivisionCountMismatch`, added
in the previous commit two screens above it.
One left deliberately: the sentence explaining that `rotation_perms` inside a
function called `affinity` reads as wrong "where `perms` — the name this field
used to carry — read like the obvious choice" is about the old name on purpose.
Also corrected the plan's "there must be exactly one copy", which as written
forbids `borbax_universe::packing`'s independent oracle. The invariant is one
shared *production* table; a validation implementation answerable to nothing
but Euler is the opposite of a problem.
HELD, not applied. CodeRabbit proposed an index tie-break on the canonical
sort, on the grounds that it would make the order independent of sort
stability. Declined: `slice::sort_by` is stable, the comparator is a total
order, and no tie is reachable in the first place — `intern` dedups at 1e-9
while the nearest neighbours are ~0.28 apart at D=162, measured. The suggestion
adds an allocation and a `Vec<(usize, Vec3)>` round-trip to break ties that
cannot occur, and `the_canonical_direction_ordering_is_pinned` already guards
the thing it is worried about. Its second held finding — documenting the
accessors' panic-on-`r >= 60` — is real and is with a reviewer whose brief
asks for the better answer than a doc note.
MrReasonable <4990954+MrReasonable@users.noreply.github.com>
Cross-check round: three specialists pointed at the FIXES rather than the original code, each routed away from the lane that proposed the thing it was checking. The two worst findings are both in commit c93644d — the commit whose whole purpose was closing the ANTI hazard. THE PLAN'S TASK 10 SNIPPET NOW TAUGHT THE DEFECT. c93644d changed the kernel to `let perm = g.contact_perms(r)` and `let j = perm[i]`, then APPENDED the new explanation without deleting the old one. The block read "Indexing `b` by `perm[i]` directly compares A against B's point inversion" immediately above a line that now correctly does exactly that. An implementer following the comment restores an `anti` step, gets `anti[anti[rotation_perms[r][i]]]` — the identity — and lands the 60-improper-element search that CLAUDE.md's ANTI bullet exists to prevent, by obeying the plan. Two casualties of the same rename, both found by the same lane: - CLAUDE.md and the plan both specified the required Task 10 probe as "delete `anti[...]` from `affinity`'s index expression". After the rename there is no `anti[...]` there to delete. The mandated probe was un-executable as written; it is now a substitution of `rotation_perms(r)` for `contact_perms(r)`. - CLAUDE.md still said the kernel indexes through `ANTI[PERM[R][i]]`, never `PERM[R][i]` directly — the opposite of what the shipped API now asks for. `contact_perms`' DOCSTRING NAMED THE WRONG POSE. It said "the direction on a partner rotated by `r`". Measured on a support function at D=42 over all 60x42 pairs: `b[contact_perms(r)[i]]` equals `h_{R_r^T . B}(-u_i)` to 1.8e-15, while the forward reading is off by 2.26. The partner sits at the TRANSPOSE. Costs nothing in `affinity`, which maximises over a group closed under inverse; costs §14.5's renderer everything, since it is handed the winning `r` precisely so it does not re-derive the pose. A plausible, silent, wrong docking picture in a project that ships SVG from day one because a shape-based chemistry cannot be developed blind. The formula on the same line was correct throughout — this is the class of error no table-versus-table assertion can see, so `contact_perms_is_the_antipode_of_the_rotated_direction` now pins it against the matrices directly. `Rotation` newtype. `rotation_perms(r)` and `contact_perms(r)` took `usize` and panicked on `r >= 60` — a library panic on an argument the type permits, which CLAUDE.md treats as a correctness bug and which `clippy::panic` cannot see because it arrives via indexing. Worse, the module-level `#[allow(clippy::indexing_slicing)]` justified itself as "indexed by loop bounds proven against D and N_ROTATIONS", which had silently become false for the two `pub fn` taking caller input — an allow whose reason is wrong is worse than a bare one, because the reason is what the next reader trusts. Rejected the two alternatives on the lane's reasoning: returning the whole array moves the indexing to Task 10's kernel, which then needs its own allow; `Option<&[u8; D]>` in a 60-iteration hot loop invites `let ... else { continue }`, silently dropping a rotation and returning a wrong affinity. Claim-audit cluster, all verified by grep across source, tests, both plan files, spec, README and CLAUDE.md: - `GeoError`'s own doc still said "Both variants"; there are three since a067095 - `build`'s `# Errors` never gained `SubdivisionCountMismatch` - "re-indexes the whole table" contradicted "six of the 162" in the same file - "one ulp at 1.0" survived at two sites after the commit that claims to have disambiguated it — the recorded failure mode reproducing inside its own repair - the plan's Step 4 counts were stale within one commit, twice - `g2.rs`'s user-facing error string told the reader to "check perms" Also: `dirs()` was the one public signature not using `Vec3`, undercutting the reason `Vec3` was made public one commit earlier. `#[derive(Clone)]` dropped — 23.5 KB, zero cloners, and it quietly contradicts "built once and passed by reference"; adding a trait impl later is additive, removing one is breaking. And `packing.rs`'s discriminator tested the wrong thing. Re-running the probe with the reciprocal-multiply form would show the six-index difference, which is already known. The sharper question is why a *packing error* depends on vertex ordering at all, given the recorded 1.1 band shift when the sort was added. 330 tests, six gate legs green, and the ordering golden did not move. MrReasonable <4990954+MrReasonable@users.noreply.github.com>
`is_identity`'s doc withdrew the "Task 10's binding kernel needs this" claim by quoting it back and then denying it, which leaves the sentence on the page for the next reader to half-remember. Rephrased to the timeless form: the binding kernel does not need it, the placement rests on the harness use. Measured while I was in there: the non-test half of this file is 1.50 lines of comment per line of code (the test module is 0.46, which is fine). A reviewer called the top half a changelog and named ~44 lines of past-tense narration to cut. I have taken the one clear case and left the rest for Ian, because his standing preference is generous comments that capture the reasoning and the same reviewer called several of those passages the best documentation in the repository. That is a judgement call about register, not a defect. MrReasonable <4990954+MrReasonable@users.noreply.github.com>
…sured
Final cross-check lane, and it supplied the measurement that turns the previous
commit's finding from an argument into a number.
CLAUDE.md's ANTI bullet used to say the kernel must index through
`ANTI[PERM[R][i]]`. `PERM` was renamed under it. Followed literally against the
new API that reads `anti[contact_perms[r][i]]` — and because `anti` is an
involution and `contact_perms = anti ∘ rotation_perms`, that is exactly
`rotation_perms[r][i]`:
D= 12: 720/720 entries identical
D= 42: 2520/2520 entries identical
D=162: 9720/9720 entries identical
100% at every resolution. **The apparently-defensive second `anti` IS the
improper-coset search.** 8a02321 fixed the "reach for rotation_perms" spelling
and missed this one, which is the spelling the stale prose actively produced.
Both are now named, in CLAUDE.md and in the plan's kernel comment.
ROTATION_MATRICES WERE UNPINNED WHILE THE MODULE DOC CLAIMED OTHERWISE. The doc
says the cross-ISA bit dump covered "every direction, every permutation entry,
`anti`, and all 60 matrices". The golden pinned the first three.
`the_permutation_table_agrees_with_the_rotation_matrices` admits 1e-12, so a
change to `frame`'s arithmetic could move matrix bits with the whole suite
green. Now pinned at 0xe3181547c2169bfd, reproduced independently and identical
in debug and release. Probed: a 1e-15 perturbation to `frame` — three orders
inside that 1e-12 — fails the golden alone, 22 of 23 still passing.
A CORRECTION TO c93644d's COMMIT MESSAGE, which I cannot amend without
rewriting three commits of history. It said the precomposition is bit-identical
"and the ordering golden did not move, which is what says so". That is wrong:
the golden does not hash `contact_perms`, so it is evidence the rename and
privatisation did not perturb construction, not evidence about the
precomposition. The actual evidence is recomposing `anti[rotation_perms(r)[i]]`
at the call site and hashing both — identical at all three D on four builds
spanning two ISAs.
Worth recording from the same lane, because it corrects the reasoning rather
than the code: `contact_perms`' own two tests are **blind to compensating
errors by construction** — they assert exactly the invariant a compensating
substitution preserves. Mutating with σ = `anti` leaves
`contact_perms_is_anti_composed_with_rotation_perms`,
`minus_identity_is_not_in_the_group` and `antipode_commutes_with_every_rotation`
all green while 8 of 22 fail. What closes that gap is the golden hashing `anti`
and `rotation_perms` *separately*. Had it hashed only the composition, the hole
would be real. The decision not to give `contact_perms` its own constant was
right, for a different reason than the one I gave.
Portability is now executed rather than argued: all nine constants reproduce on
real x86-64 Linux, and the musl and gnu opcode streams for this module are
identical at 5234 instructions. Windows is inspected not executed — op-for-op
histogram match against linux-gnu, zero FMA-class instructions — so the first
windows-latest run is what settles it.
330 tests, six gate legs green.
MrReasonable <4990954+MrReasonable@users.noreply.github.com>
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 43 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (4)
📝 WalkthroughSummary by CodeRabbit
WalkthroughThe PR adds a validated geodesic rotation oracle to ChangesGeodesic table migration
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Experiment
participant Geodesic
participant RotationTables
Experiment->>Geodesic: build directions and permutation tables
Geodesic->>RotationTables: obtain 60 rotation matrices
RotationTables-->>Geodesic: return rotation mappings
Geodesic-->>Experiment: expose dirs, rotation_perms, anti, contact_perms
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@CLAUDE.md`:
- Around line 252-262: Correct the classification in the paragraph beginning
“Two spellings break it and both are silent”: state that using rotation_perms(r)
omits the antipode, while applying anti again to contact_perms(r) yields anti ∘
anti ∘ rotation = rotation; identify both as searching the 60 proper rotations,
and describe the intended anti ∘ rotation mapping as orientation-reversing.
In `@crates/borbax-molecule/src/geodesic.rs`:
- Around line 1155-1201: Parameterize every_rotation_is_a_bijection and
permutations_form_a_group with a const-generic check::<const D: usize>() helper,
and derive the seen-array size and composition range from D instead of
hardcoding 42. Invoke the helper for the existing D=42 coverage while preserving
the current assertions and group checks.
- Around line 15-24: Update the module documentation around f64::midpoint to
describe its overflow check and fallback that halves the inputs, replacing the
inaccurate claim that it is simply (a + b) * 0.5. Keep the portability and
bit-identical deduplication discussion consistent with the corrected behavior.
In `@crates/borbax-universe/src/packing.rs`:
- Around line 446-473: Add the governing numbered PRD § reference to the Rust
comment documenting packing-error invariance under vertex ordering, alongside
the existing Task 7 and Task 20 references. If this invariant is not specified
by the PRD, explicitly mark the note as implementation-only instead.
In `@experiments/src/g2.rs`:
- Around line 319-323: Update the documentation line immediately above the error
attribute on the relevant Geodesic variant to reference rotation_perms instead
of perms, aligning it with the diagnostic message and the current symbol name.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: ff77ac3a-4a4e-4237-9575-d4f4fd0f6677
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (14)
CLAUDE.mdcrates/borbax-molecule/src/geodesic.rscrates/borbax-molecule/src/lib.rscrates/borbax-universe/src/packing.rsdocs/superpowers/plans/2026-07-26-borbax-v0-chemistry.mddocs/superpowers/plans/2026-07-26-borbax-v0.mdexperiments/Cargo.tomlexperiments/README.mdexperiments/src/bin/fusion.rsexperiments/src/bin/ptable.rsexperiments/src/g2.rsexperiments/src/geodesic.rsexperiments/src/lib.rsexperiments/src/signature.rs
💤 Files with no reviewable changes (1)
- experiments/src/geodesic.rs
…t caught it CodeRabbit reviewed this PR — its first real round since #10 — and its Major finding is correct on the arithmetic, which makes my sentence wrong as written. I wrote that both ANTI mistakes "convert the search into the 60 improper elements". As **index permutations** that is backwards: `anti` is `-I`, so the correct composition `anti ∘ rotation` has determinant -1 and is the improper coset, while both mistakes land on the 60 *proper* rotations. As **transformations of the partner body** it inverts: the correct kernel searches 60 proper poses of B, and dropping the antipode searches B point-inverted. Both readings are true of different objects, and the bullet asserted the second while every surrounding sentence was about the first — which is how it managed to be simultaneously right and backwards. Now says which is which. The consequence needs neither framing and is provable rather than measured. Since `mirror(B)[i] = B[anti[i]]`, pairing `a[i]` with `b[rotation_perms(r)[i]]` is *exactly* the correct kernel evaluated against `mirror(B)`. The defect does not merely score wrongly — it computes A's affinity with B's enantiomer, so enantiomers become interchangeable and §22.8's homochirality is impossible by construction rather than emergent. Four more, all valid: - The module doc said `f64::midpoint` "is `(a + b) * 0.5`" while the inline comment forty lines down said it is that *guarded*. Both describe the same function; only one was precise. Now states both branches and which is reachable. - `every_rotation_is_a_bijection` and `permutations_form_a_group` hardcoded 42 (`[false; 42]`, `(0..42)`) beside a `geo::<42>()` that already named it. §22.2 locks `D` to one of three values, and retargeting either test would have produced an out-of-bounds index or a silently truncated composition rather than a compile error. Both parameterised; they now run at all three. - `g2.rs`'s `RotationSearchIsBroken` doc still named `perms` one line above the error string where I had already fixed it — the two halves of one diagnostic pointing a debugger at different symbols. And one taken the other way. CodeRabbit asked for a numbered PRD reference on `packing.rs`'s new block, or an explicit note that it is implementation-only. The second is the honest branch: the spec says nothing about vertex visitation order, because "an oracle must not depend on the order it visits its own inputs" is a property of that probe's validity as ground truth, not an obligation the simulation carries. Manufacturing a `§` would be worse than saying there is not one. Written into the comment with the reasoning. 330 tests, six gate legs green. MrReasonable <4990954+MrReasonable@users.noreply.github.com>
This was referenced Jul 31, 2026
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.
Task 7 turned out not to be the task the plan described.
It was a lift, and the plan's Step 3 was deleted rather than executed
geodesic.rsalready existed inborbax-experiments, written months ofplan-time early because the G2 locality measurement could not run without a
rotation table. By the time Task 7 came due it was ahead of the plan's code.
The compiler settles it. Substituting the plan's Step 3 and running the gate's
clippy leg gives 71 errors — 38
indexing_slicing, 9as_conversions(twogenuine
usize → u8truncations), 9unwrap_used, 3disallowed_methodsonf64::total_cmp(§13.4) and 2 onf64::min(§13.1) — into a crate that haszero. It also had 11 tests against the shipped 16, and lacked
minus_identity_is_not_in_the_group, the precise hazard CLAUDE.md names.borbax-experimentsnow re-exports rather than keeping a copy, so the table G2measured and the table the binding kernel will search cannot drift apart.
The review found more than the lift did
Ten reviewers: six specialists on the lift, three cross-checking the fixes, two
CodeRabbit CLI passes. The headline was found independently by the two lanes
cross-routed onto each other's domains.
A canonical ordering is an output that property tests structurally cannot
see. All 18 original tests — group closure, bijectivity, determinant, the
antiinvolution,-Iabsent, matrix agreement, unit norms — are invariantunder relabelling
dirs, so re-keying the sort left the entire 325-testworkspace green. The realistic mutation is not sabotage but a tidy-up:
rewriting
normalise'sscale(a, 1.0/n)as componentwise division ismathematically identical, better rounded, unflagged by clippy, and is how
borbax-universe's own copy already spells it — and it moves six of the 162index positions at D=162.
the_canonical_direction_ordering_is_pinnedis thefix, and because the three
testlegs run on three platforms it is a working§13.4 golden now, rather than at Task 20 where
goldens --emitstill exits0 with "No golden artifacts yet".
The rename re-planted the hazard it was introduced to close.
contact_permsperms→rotation_permsrename (−13.21% at D=42, bit-identical) updatedthe Task 10 kernel snippet's code and appended the new comment without
deleting the superseded one — leaving a paragraph that instructs the reader into
the exact defect. Measured: following the old CLAUDE.md wording against the new
API gives
anti[contact_perms[r][i]], which equalsrotation_perms[r][i]in100% of entries at all three resolutions. The defensive-looking second
antiis the improper-coset search.Verification
fmt,clippy -D warnings,test,test --release,cargo doc,xtask).sides of
is_identity's tolerance window.an independent Python implementation sharing no code. musl and gnu opcode
streams identical at 5234 instructions. Windows inspected — op-for-op
histogram match, zero FMA-class instructions — so the
windows-latestleg iswhat settles it.
Routed forward with discriminators
different species) has no test anywhere in V0. Every
canonicalisetestasserts rotation-invariance; none asserts mirror-difference.
rename removed the
anti[...]its first wording said to delete.packing.rs's independent oracle disagrees on six of 162positions. The sharper question is why a packing error depends on vertex
ordering at all.
Note on review coverage
CodeRabbit's GitHub app has produced nothing on the last three PRs — measured:
#9 got 24 inline comments, #10 got 10, #11–#13 got zero. The repo is private and
the free tier is public-repo only. It is not a required check, so nothing was
gated wrongly. The CLI is authenticated and was run twice here, finding 11 real
issues.