fix(universe): close a live G2 blocklist gap — REAL_WORDS missed 43 real element names - #32
Open
MrReasonable wants to merge 2 commits into
Open
fix(universe): close a live G2 blocklist gap — REAL_WORDS missed 43 real element names#32MrReasonable wants to merge 2 commits into
MrReasonable wants to merge 2 commits into
Conversation
…eal element names
REAL_WORDS jumped from cerium (Z=58) straight to tungsten (Z=74),
silently dropping the whole lanthanide run plus Z=75-77, Z=91 and
everything from Z=95 onward. The shipped generator could therefore
mint a fictional element named after a real one and have is_real()
report it clean — reproduced at 43 of the first 20,000 seeds, first
at seed 1631 ("Tu" / "thulium").
Neither pinned universe digest can see this class of regression:
both sweep seeds 0..64 only, and this collision doesn't occur until
seed 1631. `symbol`/`name` are mixed into the digest, so it correctly
reports "unchanged" while a much wider seed range ships a falsely-real
name.
Two new tests, not one — the empirical seed-sweep alone would have
shipped vacuous. A first draft checked minted names against is_real()
itself, and passed cleanly with the bug still present: mint() already
filters every candidate through is_real() before returning it, so
nothing mint() ever returns can fail an is_real()-based check. Fixed
to check against an independent reference list built from atomic
number, never from REAL_WORDS.
xtask keeps its own deliberately-duplicated copy of the blocklist
(so deleting the real one fails the build rather than the gate) —
updated in the same commit, in the same order, or its own
consistency test fails.
MrReasonable <4990954+MrReasonable@users.noreply.github.com>
…ness check, honest framing
Four specialists plus CodeRabbit reviewed the previous commit. Real,
corroborated findings, applied here:
- The completeness test only checked REAL_ELEMENT_NAMES.len(), never
that its 118 entries are distinct. A duplicated line hiding a dropped
one would pass silently — reproduced by hand, now caught by a
BTreeSet distinctness assertion.
- The 20,000-seed sweep was probabilistic and ~1000x more expensive
than necessary. Four reviewers independently enumerated the
generator's full grammar (22,880 productions) and found only 6 of
the 118 real names are producible at all — thulium was the entire
live mint-time exposure; the other 42 matter for a different reason
(xtask's literal-content scan, unconstrained by the grammar). Added
an exhaustive test that checks the full grammar directly against
is_real() — deterministic, complete, sub-millisecond — and reduced
the seed sweep to 2,000 (measured: identical discriminating power to
20,000, since the grammar is what bounds reachable collisions).
- The commit message and doc comments overstated the live scope
("43 of 20,000 seeds") when only one name was ever reachable, and
had an internal inconsistency (78 vs 43 vs 118 don't reconcile —
the correct pre-fix coverage figure is 75). Corrected throughout.
- The sweep's examined counter tracked every mint attempt rather than
Provenance::Generated specifically, so it couldn't detect the
fallback path (which never reaches is_real at all) going quiet.
Fixed to count generated draws, matching the project's own
"probe the guard's bookkeeping" discipline.
- Dropped an unneeded to_ascii_lowercase() allocation — mint's
generated path is lowercase by construction.
Both new mutations probed directly: deleting thulium from REAL_WORDS
alone is caught by the exhaustive test; deleting it from both
REAL_WORDS and the oracle (hidden behind a duplicate, keeping the
oracle's length at 118) is caught by the new distinctness check.
CodeRabbit flagged the new REAL_ELEMENT_NAMES test array as a G1
breach; the emergence-auditor's independent analysis found existing
project precedent (test fixtures may legitimately spell out real
element names, since a name is an identifier, not property data) and
the same reasoning already documented in this file's own header for
the pre-existing, analogous REAL_WORDS/REAL_ELEMENT_SYMBOLS constants.
CodeRabbit's factual claim that 47 names were missing (not 43) was
independently verified wrong: exact diff of the old and new REAL_WORDS
gives 43.
MrReasonable <4990954+MrReasonable@users.noreply.github.com>
📝 WalkthroughWalkthroughThe runtime and copied element-name blocklists now include all 118 elements. Tests validate completeness, uniqueness, grammar rejection, and generated names across 2,000 seeds. ChangesElement-name exclusion coverage
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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: 1
🤖 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 `@crates/borbax-universe/src/naming.rs`:
- Around line 88-106: Remove all added real element names from REAL_WORDS in
crates/borbax-universe/src/naming.rs at lines 88-106 and 120-147, and delete the
hardcoded real-element oracle from the test module at lines 333-474. Ensure the
crate contains no real chemistry data while preserving the remaining naming
behavior and tests.
🪄 Autofix
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: 40407538-38e3-405d-9159-4654c44f7b5b
📒 Files selected for processing (2)
crates/borbax-universe/src/naming.rsxtask/src/main.rs
Comment on lines
+88
to
+106
| "praseodymium", | ||
| "neodymium", | ||
| "promethium", | ||
| "samarium", | ||
| "europium", | ||
| "gadolinium", | ||
| "terbium", | ||
| "dysprosium", | ||
| "holmium", | ||
| "erbium", | ||
| "thulium", | ||
| "ytterbium", | ||
| "lutetium", | ||
| "hafnium", | ||
| "tantalum", | ||
| "tungsten", | ||
| "rhenium", | ||
| "osmium", | ||
| "iridium", |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | 🏗️ Heavy lift
Remove real chemistry data from crates/borbax-universe.
The runtime blocklist and test oracle share one root cause: both hardcode real element data in a prohibited crate.
crates/borbax-universe/src/naming.rs#L88-L106: remove the added real element names fromREAL_WORDS.crates/borbax-universe/src/naming.rs#L120-L147: remove the added real element names fromREAL_WORDS.crates/borbax-universe/src/naming.rs#L333-L474: remove the hardcoded real-element oracle from the test module.
As per coding guidelines, crates/{borbax-universe,borbax-molecule,borbax-reaction}/**/* must not include “real chemistry data, element tables, reaction databases, molecular structures, sequence data, or real-world mapping/calibration data.”
📍 Affects 1 file
crates/borbax-universe/src/naming.rs#L88-L106(this comment)crates/borbax-universe/src/naming.rs#L120-L147crates/borbax-universe/src/naming.rs#L333-L474
🤖 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-universe/src/naming.rs` around lines 88 - 106, Remove all added
real element names from REAL_WORDS in crates/borbax-universe/src/naming.rs at
lines 88-106 and 120-147, and delete the hardcoded real-element oracle from the
test module at lines 333-474. Ensure the crate contains no real chemistry data
while preserving the remaining naming behavior and tests.
Source: Coding guidelines
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.
Summary
REAL_WORDSincrates/borbax-universe/src/naming.rsjumped fromcerium(Z=58) straight to
tungsten(Z=74), silently dropping the wholelanthanide run plus Z=75-77, Z=91, and everything from Z=95 onward — 43
real element names missing out of 118. The shipped generator could
therefore mint a fictional element and have
is_real()report it clean.Live on
maintoday, independent of anything else in flight: reproducedat seed 1631,
("Tu", "thulium").Exhaustive enumeration of the naming grammar (four specialist reviewers,
independently) shows only 6 of the 118 real names are even producible by
the current grammar —
thuliumwas the entire live exposure. The other 42still belong in the blocklist:
REAL_WORDShas a second consumer,xtask's literal-content scan, which has no grammar constraint and wouldflag any of the 118 hardcoded as a label or comment regardless of
reachability.
What changed
REAL_WORDS(andxtask's deliberatelyduplicated copy, which has its own consistency test).
every_name_the_grammar_can_produce_is_checked_against_the_blocklist)that checks the full grammar (22,880 productions) against
is_real()directly — deterministic, complete, sub-millisecond.
the_blocklist_names_every_real_element)against an independent 118-entry reference array, with a distinctness
check — a first draft only checked the array's length, which a
duplicated-entry-hiding-a-dropped-one mutation passes silently.
Demonstrated and fixed.
identical discriminating power at ~10x less cost, since the grammar —
checked exhaustively above — is what bounds reachable collisions, not
the seed count), and fixed its
examinedcounter to trackProvenance::Generatedspecifically so it can detect the fallback path(
Q{n}/quorium{n}, which never reachesis_realat all) going quiet.Two rounds of
/review-pr(4 specialists + CodeRabbit each round). Everymutation this PR's own tests exist to catch was planted and confirmed
caught before landing:
thuliumfromREAL_WORDSalone → caught by the exhaustive test.thuliumfrom bothREAL_WORDSand the reference oracle,hidden behind a duplicated neighbour (keeping the oracle's length at
118) → caught by the new distinctness check.
CodeRabbit findings, investigated and not applied
REAL_WORDSbefore/after (138 − 95 = 43, cross-checked withcomm) gives43. CodeRabbit's range-based approximation (
Z=59..=77+Z=91..=118)double-counts
tungstenand the three actinides that were alreadypresent within those ranges.
test-only reference array as a G1 breach) — investigated by a dedicated
specialist pass, which found existing project precedent that test
fixtures may legitimately spell out real element names (a name is an
identifier, not physics data), matching this file's own header
justification for the pre-existing, analogous
REAL_ELEMENT_SYMBOLS/REAL_WORDSconstants, which are the canonical G2 enforcement mechanism.Test plan
cargo fmt --all --checkcargo clippy --locked --workspace --all-targets -- -D warningscargo test --locked --workspacecargo test --locked --workspace --releaseRUSTDOCFLAGS=-Dwarnings cargo doc --locked --workspace --no-deps --document-private-itemscargo run --locked -p xtask/review-prrounds (determinism, emergence, rust-developer-expert, no-brief lane, CodeRabbit)Summary by CodeRabbit