Viewer Step 1: a window, a seed, and a universe - #22
Merged
Merged
Conversation
Step 1 of docs/superpowers/plans/2026-08-01-borbax-viewer.md. `borbax-ui`
opens a window, takes a seed, calls `Universe::generate`, and shows that
universe's element count. Deliberately ahead of Task 11: §14.5 puts the
interactive viewer in V1, and Ian's 2026-08-01 instruction was to bring the
visuals forward rather than run ten more tasks with nothing to look at.
The crate is `borbax-ui`, not the plan's `borbax-viewer`. Three reviewed
documents already name it that — PRD §18.3's layout, the V0 plan's File
Structure, and Task 19 Step 3, which is written against `borbax-render` and
`borbax-ui` as a *pair* of backends over one geometry. The plan was the only
dissenter and has been amended. The binary is `borbax`.
The line reads `80 elements · physics v1`. The version is read from the
universe via `u8::from(u.physics)`, never typed, because §6 says a universe
is `(seed, physics)` and a viewer that hardcodes the version becomes a liar
the day V2 ships.
The seam is per-file and greppable rather than a convention to remember:
`state.rs` imports no UI type, `panel.rs` imports `egui` and contains no
`format!`, `main.rs` is the only file naming `eframe`. That is what lets
`egui_kittest` drive the real drawing code headlessly in CI — `egui` lays out
on the CPU and cannot open a window.
24 crate tests, 419 in the workspace. Six-leg gate green.
Four of the tests drive real frames, and they exist for one defect the other
twenty structurally cannot see: an immediate-mode panel that paints the count
*before* reading the seed box shows the previous seed's number for one frame.
Probed by moving `ui.label` above `ui.text_edit_singleline` — both frame tests
fail and all twenty state-level tests stay green. Every other guard was probed
the same way: the `starts_with('-')` shortcut that calls `-abc` negative fails
exactly one test; an unconditional `reload` fails exactly one; a constant
element count fails four.
Two `xtask` guards, both closing holes that predate this crate:
- **G1's data-file check was a hand-kept list of four crate paths** and had
already gone stale — `crates/borbax-ui/assets/elements.json` passed `cargo
xtask` cleanly, in the crate with the largest G2/G4 surface in the project.
It now walks `crates/` and `experiments/` whole, applying Task 10's rule:
invert the default, unknown ⇒ covered. Safe for Task 19 — `.svg` goldens and
`.snap` snapshots are not data extensions, checked rather than assumed.
- **`[lints] workspace = true` was enforced by nothing.** Four manifests carry
a comment about the trap; a comment is not a guard, and deleting the two
lines leaves fmt, clippy, test and doc all green while `unsafe_code =
"forbid"` silently forbids nothing. It has bitten this repository once
already. `check_every_member_inherits_the_lints` now fails closed, and its
bookkeeping counter was probed separately from its assertion.
Three claims in the plan were false against the tree and are corrected in it:
- **`wgpu` was NOT "already in the dependency tree"** — zero manifests, zero
lockfile entries, 13 packages in the whole workspace, and §21 lists GPU
screening as an explicit V1 non-goal. The PRD's §14.0 says the tree needs it
"either way", an argument about the *eventual* tree; the plan flattened
"eventually" into "already". The native decision stands on its other reasons,
but the cost is new and wholly the viewer's: 418 lockfile packages,
~888 s of new cold-build CPU, and a build cache going from 30 MB to ~600 MB
per platform.
- **There is no G2 element-name scan to extend, for any crate** —
`check_blocklist_present` asserts only that an identifier still appears in
`naming.rs`; the blocklist is a generation-time filter. No G4 unit check
exists either. Step 7 writes three checks rather than adding a path, and the
name/unit ones move to Step 2, which is where the surface they guard first
appears.
- **`state.rs` "computes nothing"** was exactly backwards. It computes
everything, because it is the half a test can reach.
`eframe`'s tree duplicates 31 transitive dependencies we neither choose nor
can unify, held by one crate-level `#![expect(clippy::multiple_crate_versions)]`
rather than a workspace allow, so the lint keeps working on the physics crates.
Both the count and the placement were first recorded wrongly — "~34", and
"required in both crate roots" — and it was an *unfulfilled* expectation on
`main.rs` that caught it. Measured: the lint is evaluated once per package.
Two things this does not test, stated rather than implied: no CI leg opens a
window, so window creation, the event loop and GPU init are unverified on every
platform; and rendering the physics version as a literal `1` passes all 24
tests, because `PhysicsVersion` has one variant and every available test
compares 1 against 1. That guard is latent, derives its expectation from
`PhysicsVersion::CURRENT`, and is documented as vacuous today.
MrReasonable <4990954+MrReasonable@users.noreply.github.com>
Claude-Session: https://claude.ai/code/session_015yLN32a5iKSXd6ZibxSFwT
…n numbers Applying the five-lane review of 50d566b. Three of the lanes found the same two defects independently, each by trying a mutation my probe had not. **The lints guard was defeatable, in the check added because "a comment is not a guard".** It read src.contains("[lints]") && src.contains("workspace = true") and the second conjunct is satisfied unconditionally by every manifest in the workspace — `edition.workspace = true` is on line 3 of all seven. So it collapsed to "is the `[lints]` header present", which a comment satisfies and which an **empty** `[lints]` table satisfies. Cargo accepts an empty table and the crate then inherits nothing: reviewers planted `[lints]` with the opt-in line deleted plus an `.unwrap()` in shipped library code and got a clean gate AND a clean `clippy -D warnings`. That is the exact state the guard exists to make impossible, reachable with the guard green. I had probed it by deleting the whole table, which it does catch. Nobody had deleted one line of it. Now section-scoped with whole-line equality, which also rejects both commented forms. **Its bookkeeping was decorative and its comment wrong in both halves.** It said "six members exist" (there are seven) and justified the bar as "deleting the filter does [move it]" — deleting the filter makes the count go *up*. And the scenario its failure message names, a moved layout, was the one case it could not catch: with `crates/` renamed, `experiments` and `xtask` still supply two manifests, `2 < 2` is false, and five members go unchecked. Eight other guards fired on that mutation; this one said nothing. The missing-root case is now loud and by name — which is what its sibling `check_no_data_files`, changed in the same commit, already did twenty lines above it. **G1's widening was case-sensitive.** `elements.json` fires, `elements.JSON` did not — and on macOS and Windows those are the same file to the OS, so the rename that defeats a §5 guard is invisible to whoever makes it. Precedence 1. **`Domain::Universe` for the seed button aliased element generation.** `Stream::new(m, Domain::Universe, 0)` is bit-for-bit `borbax_universe`'s own `stream(seed)` — the stream the shell pattern is drawn from. Two lanes measured the aliasing independently and it was exact. Nothing was wrong on screen, since the universe is keyed on the drawn seed and not on `moment`; what was wrong is that changing element generation's first draw would silently change which universes the button offers, which is what `Domain` exists to prevent. `Domain::Hash` is documented for precisely this — "using a `Stream` as a hash function". **Two guards added, because this commit's own argument demands them.** The crate's leaf-ness and its per-file seam were each asserted in four documents and checked in none — and leaf-ness is what licenses every §13.1 relaxation the crate grants itself. The seam check strips whole-line comments first, because the prose necessarily quotes what it forbids: a raw `grep -rl eframe src/` returns three files and `grep format! panel.rs` returns one, both correctly and both uselessly. CLAUDE.md's "check it by grep" instruction was false as literally written and now says so. The `Universe::generate` single-call-site grep was scheduled for Step 7; the surface it guards exists now. All six new and repaired guards probed by the mutation each exists to stop, run before this commit. **Numbers I got wrong, and the method that made them wrong.** `cargo test` is fail-fast *across test binaries*, so a probe stops counting at the first failing binary. Re-measured with `--no-fail-fast`: - label emitted before the seed box: **3** frame tests fail, not "both" — and all 20 state-level tests stay green, which was the half that mattered and was right. My first attempt at this probe also *added* a label rather than moving one, which is a no-op mutation and reported 0; `procedural_review.md` records that class and I walked into it anyway. - a constant element count: **7** tests, not four. - `starts_with('-')` shortcut: 1. Unconditional `reload`: 1. `randomise` ignoring its argument: 1. Physics version as a literal: still 0, still the documented gap. Standing rule worth keeping: **every mutation probe runs `--no-fail-fast`, and counts named tests rather than grep hits** — my own counter matched the `test result: FAILED` summary line and inflated three of these figures again on the way to fixing them. **Stale figures corrected, in the commit whose subject was stale figures.** The plan said the shipped line reads `87 elements` (it reads 80), 418 lockfile packages (366 — 418 was the pre-`default-features` resolve), 18 tests (24), and 34 clippy errors "in each of the two targets" — the exact wording `lib.rs` records as refuted by measurement, left standing in the plan. **And the `#![expect]` reason recorded a false mechanism for the second time.** "Once per package" is wrong: it is a per-crate pass and lib/bin are separate crates. The binary escapes because clippy resolves the local package by matching the crate name against `cargo metadata`, and this binary is `borbax`, which matches no package. Verified by renaming `[[bin]] name` to `borbax-ui`, at which point the bin lints too and emits the same 31. So renaming the binary, or adding a second bin whose crate name matches the package, brings 31 errors back under `-D warnings` — which the previous explanation said could not happen. `ci.yml`'s cache rationale reasoned from "the workspace has seven third-party packages… so the archive is 30 MB". It has ~353 now. Every cache timing in that file is a pre-`eframe` measurement and the conclusion drawn from them inverts: with a large tree the cache matters more, not less. Marked stale rather than deleted — a number labelled stale is evidence about the past. Renovate: `borbax-ui` gets its own tier so `eframe`/`egui` bumps stop arriving labelled `determinism-review`. `egui` ships monthly and that label's whole value is that it only fires on something that can move a result. Not fixed here, and pre-existing: `renovate.json` emits **4 config errors under `renovate-config-validator --strict` on `main` as well as on this branch** (`customManagers[0].managerFilePatterns`). The custom manager that reads the `.prototools` rustc pin is therefore inert, which means TIER C — the toolchain pin, a determinism control — would never fire. Out of scope for a viewer commit and needs a decision about which Renovate schema to target. 419 workspace tests, six-leg gate green. MrReasonable <4990954+MrReasonable@users.noreply.github.com> Claude-Session: https://claude.ai/code/session_015yLN32a5iKSXd6ZibxSFwT
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 39 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: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (6)
📝 WalkthroughWalkthroughThis PR adds the ChangesBorbax viewer
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant eframe_App as eframe::App
participant panel_draw as panel::draw
participant ViewerState
eframe_App->>panel_draw: forward ViewerState and egui::Ui
panel_draw->>ViewerState: apply seed edit or randomise
ViewerState-->>panel_draw: provide outcome and status line
panel_draw-->>eframe_App: render viewer controls and status
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
Comment |
Owner
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
…d no guard Applying the CodeRabbit CLI round on top of the specialist fleet. Eight findings: three real, one real with the wrong remedy, three declined with reasons, one false. **The seed box rendered the previous seed in the frame the button was clicked.** `randomise` wrote `seed_text` *after* `ui.text_edit_singleline` had already emitted it, so the box showed the old number beside the new universe's count — the mirror of the defect the frame tests were written for, in the other widget. I had documented that as harmless because it self-corrects next frame. What made it worth fixing is the second half of the finding: the test claiming to cover the button asserted on `harness.state().seed_text()` and never on what was painted, so nothing would have noticed it getting worse. The button now runs before the box, both sit on one row, and the test asserts the accessibility value. It failed first, reading `""` against a state of `"145206"`. **§13.1 bans wall-clock and nothing enforced it.** `clippy.toml`'s `disallowed-methods` list is 129 lines of `f64::`/`f32::` entries and names no time type; `xtask`'s `BANNED_CALLS` mirrors it. So the invariant was held by nobody having written one, and viewer Step 1 is the workspace's first read. CodeRabbit's remedy — "retain a `Stream` outside the panel" — was declined: a `Stream` needs a seed, and a deterministic one would hand out the same universes in the same order on every launch, which is worse for a button called "surprise me". Its premise is also wrong. The universe is a pure function of the *drawn seed*, which is displayed and retypable, and `the_random_button_puts_a_seed_in_the_box_that_can_be_typed_back` proves the round-trip; no wall-clock value survives into a result. Both auditors reached that independently with the spec in hand. The finding underneath it is real though, and it is the precedent: a first read with no guard is how a second one lands somewhere that *does* produce results. `check_wall_clock_has_one_home` permits `SystemTime::now`/`Instant::now` in exactly one named file. Adding them to `clippy.toml` instead does not work — a cross-check test forces every entry into `BANNED_CALLS`, whose scan has no path exemption by design, so the one sanctioned call could not be let through. **Declined, with reasons.** Restoring `default-features = true` for Wayland client-side decorations: real, already documented in the manifest, and costs 72 Linux packages for a platform nobody runs this on yet — revisit at the first Linux or shareable build. Two "cite numbered PRD sections" nudges: the comments in question document `egui` frame mechanics, which no PRD section covers. **One finding was false and worth recording as such.** "The workspace contains five `borbax-` crate manifests… update the ci.yml comments that state there are six." `cargo metadata --no-deps` reports **six** `borbax-*` packages — the sixth is `borbax-experiments`, whose directory is `experiments/`, so a count of `crates/borbax-*` misses it. The comment was right. This is the third round in which a CodeRabbit finding cited the tree incorrectly and the second in which another finding in the same run was one nothing else would have caught. Also: MD040 language identifier on the plan's `src/` tree block, and the plan's "grep the viewer for arithmetic and find only projection, layout and colour" — falsified by Step 1's own `wrapping_mul` and `saturating_add`, neither of which reaches the screen. The rule that survives is narrower: no arithmetic on a value that came out of a `borbax-*` call. `check_the_viewer_boundaries_hold` crossed `clippy::too_many_lines` and is split into three functions, which is the right shape anyway — leaf-ness, the import seam and the clock are unrelated invariants sharing a subject. **All six guards re-probed after the split**, since a refactored guard is an unprobed one. 419 workspace tests, six-leg gate green. MrReasonable <4990954+MrReasonable@users.noreply.github.com> Claude-Session: https://claude.ai/code/session_015yLN32a5iKSXd6ZibxSFwT
Owner
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
Owner
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (4)
xtask/src/main.rs (2)
2519-2529: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider matching
[lints]headers that carry a trailing comment.
skip_while(|line| *line != "[lints]")requires an exact line match. A manifest that writes[lints] # inherit workspace lintsopts in correctly, but this check reports it as non-compliant. The direction is fail-closed, so this is not a correctness risk. It does produce a confusing failure for a valid manifest.♻️ Optional: tolerate a trailing comment on the header line
- let has_table = src - .lines() - .map(str::trim) - .skip_while(|line| *line != "[lints]") + let has_table = src + .lines() + .map(str::trim) + .skip_while(|line| line.split('#').next().map(str::trim) != Some("[lints]")) .skip(1) .take_while(|line| !line.starts_with('[')) .any(|line| line == "workspace = true");🤖 Prompt for 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. In `@xtask/src/main.rs` around lines 2519 - 2529, Update the `[lints]` header detection in the `has_table` computation to recognize headers followed by a trailing comment, while still requiring the line to represent the `[lints]` section. Preserve the existing workspace-entry scanning and fail-closed behavior for unrelated headers.
2692-2716: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winExclude test code from the
Universe::generatecall-site count.The current inline
#[cfg(test)]module does not callUniverse::generate, so the check passes today. If a viewer unit test calls it later, the textual count will fail despite unchanged production code. Count only non-test code.🤖 Prompt for 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. In `@xtask/src/main.rs` around lines 2692 - 2716, Update the call-site scan in the loop building call_sites to exclude Rust test code before counting Universe::generate occurrences. Ensure inline #[cfg(test)] modules and other test-only code are ignored while production viewer code remains counted, preserving the existing exactly-one validation and failure reporting.crates/borbax-ui/tests/panel.rs (1)
44-48: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueBind
Universe::generate(7)once instead of calling it twice.This calls
Universe::generate(7)twice to build one expected string.expected_forintests/acceptance.rsbinds the result once and reads both fields from it. Follow the same pattern here for consistency.♻️ Proposed refactor
- let expected = format!( - "{} elements · physics v{}", - Universe::generate(7).table.len(), - u8::from(Universe::generate(7).physics) - ); + let universe = Universe::generate(7); + let expected = format!( + "{} elements · physics v{}", + universe.table.len(), + u8::from(universe.physics) + );🤖 Prompt for 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. In `@crates/borbax-ui/tests/panel.rs` around lines 44 - 48, Update the expected-string setup around Universe::generate(7) to bind the generated Universe once, then read both table.len() and physics from that binding when constructing expected. Follow the existing expected_for pattern in the acceptance tests and preserve the current output.CLAUDE.md (1)
149-159: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winNarrow the string-source claim.
panel.rspaints static strings such as"seed"and"surprise me". Change “every string the window paints comes fromstate.rs” to “the computed status string the window paints comes fromstate.rs.”🤖 Prompt for 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. In `@CLAUDE.md` around lines 149 - 159, Update the prose in the viewer seam documentation to narrow the claim from every painted string to the computed status string painted by the window, while preserving the existing explanation that status-string tests reflect the displayed output.
🤖 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 @.github/workflows/ci.yml:
- Around line 94-116: Update both occurrences of the lockfile-size description
in the workflow comments to accurately describe the 366 package entries: use
“353 additional lockfile package entries” instead of “~353 third-party,” or
explicitly state the 359 external-package count alongside the 7 local/path
entries.
In `@CLAUDE.md`:
- Around line 161-166: Revise the Step 3 hazard in the documentation so it does
not pre-approve native camera trigonometry or describe routing through det_math
as unnecessary. State that every Rust source file must route sin and cos through
borbax-units/src/det_math.rs, and leave the decision open only when the
repository rule and its guard are changed together in the same reviewed task.
In `@docs/superpowers/plans/2026-08-01-borbax-viewer.md`:
- Around line 322-335: Replace the planned grep-based single-generation check
with a semantic guard that detects all supported call forms of
Universe::generate without counting comments or documentation, and explicitly
define/test those source forms. Mutation-probe the guard with a second
generation call, a direct panel::draw call bypassing reload, and incorrect
regenerations bookkeeping; separately verify counter, filtering, and threshold
behavior against their reported diagnostics.
- Around line 67-75: Revise the numeric-display rule in the plan so it applies
only to simulation-derived values, not every number shown by the viewer.
Explicitly exempt user input, seed identity, and layout values, while retaining
the restriction against performing arithmetic on values returned by borbax-*
calls.
In `@renovate.json`:
- Around line 53-60: Restrict the TIER B-prime Renovate rule identified by its
description to UI-only dependencies by adding matchDepNames for eframe and egui.
Preserve the existing labels replacement behavior for those dependencies, while
leaving egui_kittest handled by the separate dev-dependency rule.
In `@xtask/src/main.rs`:
- Around line 2720-2721: Update the documentation around the wall-clock scan to
state that it covers DATA_FREE_ROOTS (“crates” and “experiments”) while
excluding xtask, and accurately describe the two spellings matched by
CLOCK_READS. Either narrow the claim to those exact patterns or expand
CLOCK_READS to include the additional Instant/SystemTime forms identified by the
review.
- Around line 2618-2622: Update the scan-root handling in the function
containing the DATA_FREE_ROOTS loop to fail immediately when a required root is
missing, matching the existing guard behavior in check_no_data_files and
check_every_member_inherits_the_lints. Remove the silent continue for
nonexistent directories and use the established failure mechanism so missing
roots cannot produce a false clean result.
- Around line 2748-2752: Update the DATA_FREE_ROOTS scan loop around scan_root
and dir so a missing required scan root returns the same loud failure used by
the guards near the checks at lines 2384 and 2456, rather than continuing and
allowing a clean result. Preserve normal scanning for existing roots and reuse
the established error/reporting behavior.
---
Nitpick comments:
In `@CLAUDE.md`:
- Around line 149-159: Update the prose in the viewer seam documentation to
narrow the claim from every painted string to the computed status string painted
by the window, while preserving the existing explanation that status-string
tests reflect the displayed output.
In `@crates/borbax-ui/tests/panel.rs`:
- Around line 44-48: Update the expected-string setup around
Universe::generate(7) to bind the generated Universe once, then read both
table.len() and physics from that binding when constructing expected. Follow the
existing expected_for pattern in the acceptance tests and preserve the current
output.
In `@xtask/src/main.rs`:
- Around line 2519-2529: Update the `[lints]` header detection in the
`has_table` computation to recognize headers followed by a trailing comment,
while still requiring the line to represent the `[lints]` section. Preserve the
existing workspace-entry scanning and fail-closed behavior for unrelated
headers.
- Around line 2692-2716: Update the call-site scan in the loop building
call_sites to exclude Rust test code before counting Universe::generate
occurrences. Ensure inline #[cfg(test)] modules and other test-only code are
ignored while production viewer code remains counted, preserving the existing
exactly-one validation and failure reporting.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 98d74b3d-0df2-4dad-b21f-53ab0a499247
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (12)
.github/workflows/ci.ymlCLAUDE.mdcrates/borbax-ui/Cargo.tomlcrates/borbax-ui/src/lib.rscrates/borbax-ui/src/main.rscrates/borbax-ui/src/panel.rscrates/borbax-ui/src/state.rscrates/borbax-ui/tests/acceptance.rscrates/borbax-ui/tests/panel.rsdocs/superpowers/plans/2026-08-01-borbax-viewer.mdrenovate.jsonxtask/src/main.rs
…wo more CodeRabbit's round on PR #22: 8 actionable + 4 nitpicks, **all twelve real**. The hit rate is high because most of this diff is my own prose making checkable claims, which is what it is good at. **The finding worth the whole round.** `check_no_data_files` and `check_every_member_inherits_the_lints` were both changed *in this PR* to fail loudly when a scan root is missing, with a comment saying silence is unaffordable for a §5 guard. I then split `check_the_viewer_boundaries_hold` into three functions twenty lines below them and gave two of the three the silent `continue` I had just removed. Fixing a pattern in the places you can see is not fixing the pattern. Probed: with `experiments/` renamed, four guards now name the missing root where two did before. With `crates/` renamed, three do — the leaf check returns early when the viewer itself is absent, which is correct, and is why the other scenario is the one that verifies it. **Two negative probes added, and they are the half that was missing.** Every guard here was probed for firing on the defect; none had been probed for staying silent on correct code, which is the direction that gets guards deleted. `[lints] # inherit workspace lints` is a valid opt-in and was reported non-compliant — the header match now tolerates a trailing comment. A `Universe::generate` inside `#[cfg(test)]` would have fired the single-call-site check; the scan is now shipped-code-only. **Numbers, again.** "~353 third-party packages" in `ci.yml` was the *delta* (366 − 13) quoted as a count. Measured: 366 entries = 7 workspace + **359 external**. Both copies corrected. **A Renovate rule that would have stripped the label it exists to protect.** TIER B-prime matched `crates/borbax-ui/Cargo.toml` by *file*, and `labels` REPLACES rather than adds — but that manifest also declares `borbax-universe` and `borbax-rng`. So a bump to a physics crate would have arrived tagged `deps-ui` with `determinism-review` stripped, which is precisely inverted. Now scoped by `matchDepNames: ["eframe", "egui"]`. **I pre-approved an exception to a hard invariant in CLAUDE.md.** The Step 3 note said "the viewer produces no results, so routing camera trig through `det_math` is cost with no benefit" — stating as settled a §13.1 exception that has not been reviewed. §13.1 applies to every `.rs` file; an exception is a policy change that lands with the guard implementing it, in one reviewed task. Rewritten as the open decision it is. **Two rules that were false as written.** The plan's "every number on screen must be traceable to a `borbax-*` call" — the typed seed is a number on screen and is user input; now "every *simulation-derived* number", with input, seed identity and layout named as exempt. And CLAUDE.md's "every string the window paints comes from `state.rs`" — `ui.label("seed")` does not; narrowed to the computed status string, which is the one a test can be wrong about. Also: the wall-clock guard's doc claimed the whole workspace and covers `DATA_FREE_ROOTS` only (not `xtask`) over two literal spellings — `Instant as Clock`, `UNIX_EPOCH.elapsed()` and a bare `.elapsed()` all pass. Stated rather than overclaimed. The single-call-site check has the same class of hole for a `Universe as U` alias; recorded in the failure message and routed to Step 7 with the AST-versus-textual trade-off written down. CI on the previous commit was green on all three platforms — **Windows 38m15s and Ubuntu 21m30s, both cold `eframe` builds, and the cross-platform golden job passed.** That was the last unverified risk in Step 1. 419 workspace tests, six-leg gate green, all guards re-probed after the edits. MrReasonable <4990954+MrReasonable@users.noreply.github.com> Claude-Session: https://claude.ai/code/session_015yLN32a5iKSXd6ZibxSFwT
Owner
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
This was referenced Aug 3, 2026
6 tasks
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.
What this is
Step 1 of the viewer —
borbax-uiopens a window, takes a seed, callsUniverse::generate, and shows that universe's element count. A "surprise me" button draws a six-digit seed and types it into the box, so a universe a child likes is one she can write down and come back to.Deliberately ahead of Task 11. §14.5 puts the interactive viewer in V1; the instruction on 2026-08-01 was to bring the visuals forward rather than run ten more tasks with nothing to look at. Task 19's SVG goldens are not replaced — they are how §13.6 proves cross-platform bit-identity, which a screenshot cannot do.
The crate is
borbax-ui, not the plan'sborbax-viewerThree reviewed documents already name it that: PRD §18.3's layout, the V0 plan's File Structure, and Task 19 Step 3, which is written against
borbax-renderandborbax-uias a pair of backends over one geometry — "one projection, two renderers, no duplicated maths".borbax-viewerappeared only in the viewer plan, which has never been through/review-plan. The plan was amended; the spec was not. The binary isborbax.The seam, which is the whole testability story
state.rspanel.rseguiformat!main.rseframeeguilays out on the CPU and cannot open a window, soegui_kittestdrives the realdrawheadlessly in CI. Four of the 24 tests do exactly that, and they exist for one defect the other twenty structurally cannot see: an immediate-mode panel that paints the count before reading the seed box shows the previous seed's number for one frame. Probed by moving the label — 3 frame tests fail, all 20 state-level tests stay green.cargo xtasknow checks this seam, stripping whole-line comments first, because the prose necessarily quotes what it forbids.Four
xtaskguardsTwo close holes that predate this crate, two close claims this crate makes about itself:
crates/borbax-ui/assets/elements.jsonpassed cleanly. Now every crate undercrates/, case-insensitively. Unknown ⇒ covered.[lints] workspace = truewas enforced by nothing. Four manifests carry a comment about the trap; deleting the two lines leaves fmt, clippy, test and doc all green whileunsafe_code = "forbid"forbids nothing. It has bitten this repository once already.borbax-uimust stay a leaf — that is what licenses every §13.1 relaxation the crate grants itself.Universe::generatehas exactly one call site. The plan deferred this to Step 7; the surface exists now.The second commit is the interesting one
A five-lane review found two of the guards I added did not guard, and three lanes found them independently — each by trying a mutation my probe had not.
contains("[lints]") && contains("workspace = true"), and every manifest satisfies the second clause viaedition.workspace = true. It collapsed to "is the header present", which a comment satisfies and which an empty[lints]table satisfies while the crate inherits nothing. Reviewers planted that plus an.unwrap()in shipped code and got a clean gate and a cleanclippy -D warnings. I had probed it by deleting the whole table; nobody had deleted one line of it.examined < 2bookkeeping could not catch the scenario its own failure message named.Domain::Universefor the seed button was bit-for-bit the stream element generation draws its shell pattern from. Harmless on screen; a live coupling for Step 1b. NowDomain::Hash.Plus corrections to numbers I got wrong:
cargo testis fail-fast across test binaries, so a mutation probe stops counting at the first failing binary. Re-measured with--no-fail-fast. My first attempt at the frame-order probe also added a label rather than moving one — a no-op mutation, the classprocedural_review.mdalready records.And the plan's own stale figures, in the commit whose subject was stale figures:
87 elements(it reads 80), 418 lockfile packages (366), 18 tests (24), and the#![expect]placement claim thatlib.rsrecords as refuted — left standing in the plan.Dependency cost — new, and wholly the viewer's
The plan claimed
wgpuwas "already in the dependency tree". It was not: zero manifests, zero lockfile entries, 13 packages in the whole workspace, and §21 lists GPU screening as an explicit V1 non-goal. Corrected in the plan, because a false justification gets reused later to wave through a dependency that has not earned it.Actual: 366 lockfile packages,
default-features = falsekeeping that from being 428. No pre-existing package changed version or feature set —libmstays 0.2.16 with an unchanged{default, arch}feature resolution, verified rather than assumed. No golden moved.Known-open, pre-existing, not fixed here
renovate.jsonemits 4 config errors underrenovate-config-validator --strictonmainas well as this branch (customManagers[0].managerFilePatterns). The custom manager that reads the.prototoolsrustc pin is inert, so TIER C — the toolchain pin, a determinism control — would never fire. Out of scope for a viewer commit and needs a decision about which Renovate schema to target.What this does NOT test, stated rather than implied
eframecompiles on its host. This branch has never had a CI run, so the Linux and Windows legs are genuinely unknown.1passes all 24 tests.PhysicsVersionhas one variant, so every available test compares 1 against 1. That guard is latent, derives its expectation fromPhysicsVersion::CURRENT, and is documented as vacuous today.Gate
Six legs green locally:
fmt,xtask,clippy -D warnings,test,test --release,cargo doc. 419 workspace tests.https://claude.ai/code/session_015yLN32a5iKSXd6ZibxSFwT
Summary by CodeRabbit