core(math): canonicalize -0.0 in rotations; add MulAssign for Mat4 #13
Conversation
…t4; add tests (mul_assign, no -0.0)
|
Warning Rate limit exceeded@flyingrobots has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 25 minutes and 33 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (10)
Summary by CodeRabbitRelease Notes
WalkthroughAdds MulAssign for Mat4, canonicalizes -0.0 in trig-based Mat4 rotation constructors, and introduces tests plus documentation and CI/githook updates for formatting, rustdoc, and security auditing. Changes
Sequence Diagram(s)sequenceDiagram
participant Caller as Caller
participant LHS as Mat4 (self)
participant RHS as Mat4 (rhs)
Caller->>LHS: LHS *= RHS
activate LHS
LHS->>LHS: multiply(&RHS)
note right of LHS #eef2ff: existing multiply computes product\nand returns a new Mat4
LHS-->>LHS: assign result to self
LHS-->>Caller: mutated LHS
deactivate LHS
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
…and decision log
…eep 2025-10-29 geom fat-AABB intent and 2025-10-28 math PR #13 entry
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
…e partial-staging semantics; instruct user to review and restage
…n rustfmt changes files; preserve partial staging semantics
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
crates/rmg-core/tests/mat4_mul_tests.rs(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
crates/rmg-core/tests/mat4_mul_tests.rs (1)
crates/rmg-core/src/math/mat4.rs (6)
scale(50-56)identity(19-25)translation(35-41)rotation_x(63-71)rotation_y(78-86)rotation_z(93-101)
…ial-staging workflow; add tips and env toggles
…i/wasm); devcontainer installs hooks; add spec-merkle-commit doc + links; pin rust-version in rmg-geom
…hs; verify in-place result differs from original and matches operator path
…s and add composite-LHS checks
…ngle choice and add FRAC_PI_6/FRAC_PI_3 sanity checks
…ures; update plan/log notes
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (2)
CONTRIBUTING.mdis excluded by!*.mdREADME.mdis excluded by!*.md
📒 Files selected for processing (10)
.devcontainer/post-create.sh(1 hunks).githooks/pre-commit(1 hunks).githooks/pre-push(1 hunks).github/workflows/ci.yml(1 hunks)crates/rmg-core/src/snapshot.rs(1 hunks)crates/rmg-core/tests/mat4_mul_tests.rs(1 hunks)crates/rmg-geom/Cargo.toml(1 hunks)docs/decision-log.md(2 hunks)docs/execution-plan.md(1 hunks)docs/spec-merkle-commit.md(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
crates/rmg-core/tests/mat4_mul_tests.rs (1)
crates/rmg-core/src/math/mat4.rs (7)
rotation_x(63-71)scale(50-56)to_array(145-147)rotation_y(78-86)translation(35-41)rotation_z(93-101)new(140-142)
🪛 LanguageTool
docs/spec-merkle-commit.md
[style] ~43-~43: Consider replacing this word to strengthen your wording.
Context: ...ths, or endianness is a breaking change and invalidates previous hashes. - The comm...
(AND_THAT)
🪛 markdownlint-cli2 (0.18.1)
docs/decision-log.md
21-21: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Above
(MD022, blanks-around-headings)
27-27: Trailing spaces
Expected: 0 or 2; Actual: 1
(MD009, no-trailing-spaces)
🔇 Additional comments (8)
.devcontainer/post-create.sh (2)
20-23: ---Makefile verification PASSED—no action required.
The
hookstarget exists in the Makefile. The devcontainer post-create script is good to merge.
From: Linus Torvalds torvalds@linux-foundation.org
To: {PR_AUTHOR_NAME} <{PR_AUTHOR_EMAIL}>
Cc: code-rabbit@vger.kernel.orgLook, this was straightforward. The grep found it. The target is there. Stop second-guessing yourself and let this ship. This is exactly the kind of thing that passes verification without drama. Your
.devcontainer/post-create.shworks. Move on.BunBun
20-23: ---From: Linus Torvalds torvalds@linux-foundation.org
To: {PR_AUTHOR_NAME} <{PR_AUTHOR_EMAIL}>
Cc: code-rabbit@vger.kernel.org
The review comment fundamentally misunderstands the codebase and should be ignored.
Look, the reviewer is freaking out over error handling that doesn't actually exist as a problem here. Let me cut through the noise with actual facts:
The
hookstarget EXISTS. I grepped the Makefile—line 7, right there. "No hooks target" is not a concern in this codebase.The
|| truepattern is INTENTIONAL and CONSISTENT. This entire script uses it for optional setup:
- Line 13:
rustup component add ... || true- Line 14:
rustup target add ... || true- Line 17:
cargo fetch || true- Line 23:
make hooks || trueThis isn't a bug. It's a design decision: optional setup steps gracefully degrade. The devcontainer still boots even if some nice-to-haves fail. That's not inconsistent with
set -euo pipefail—that's orthogonal. Strict mode handles the mandatory steps;|| truehandles the optional ones.The PR explicitly integrates hook installation as optional devcontainer setup. Git history shows this across multiple commits:
devcontainer installs hooks, not "devcontainer must install hooks."The reviewer is suggesting validation of
makeavailability and target existence. In a devcontainer context where these are bootstrapping dependencies, that's security theater. Ifmakeisn't available or the Makefile is broken, the whole project is broken—no amount of validation in post-create.sh fixes that.The code is fine. The pattern is consistent. The target exists. Move on.
BunBun
Likely an incorrect or invalid review comment.
crates/rmg-core/tests/mat4_mul_tests.rs (2)
69-84: Finally actionable failure outputFrom: Linus Torvalds torvalds@linux-foundation.org
To: flyingrobots flyingrobots@users.noreply.github.com
Cc: code-rabbit@vger.kernel.orgI tore into you before for hiding the evidence; now the failure message actually names the axis, slot, and offending angle, and you even tossed in intermediate angles so I can't accuse you of cherry-picking the quadrants. Good. Keep it that way or I'm back with a blowtorch.
BunBun
91-189: Randomized hammer worthy of the nameFrom: Linus Torvalds torvalds@linux-foundation.org
To: flyingrobots flyingrobots@users.noreply.github.com
Cc: code-rabbit@vger.kernel.orgDeterministic xorshift stress that walks the owned/borrowed/composed paths in a loop of 64? That's the kind of mean-spirited coverage that actually flushes out in-place math bugs instead of petting them. Keep that seed brutal and don't let it rot.
BunBun
docs/execution-plan.md (1)
57-60: Math Polish Entry — Good.The entry correctly identifies the canonicalization of -0.0 and MulAssign support as the core changes in PR #13. The "gather feedback before merge" note is prudent. No issues here.
docs/decision-log.md (3)
102-107: Pre-commit auto-format policy entry — LGTM.The entry accurately documents the decision to preserve index integrity by running fmt then aborting, then requiring restage. Rationale and consequence are both sound. No content issues.
108-113: CI + Security hardening entry — Solid.Captures the cargo audit/deny additions and rustdoc gates. The note about
rustsec/audit-check@v1is pragmatic and saves future maintainers from the tag-pinning gotcha. Good call.
115-118: Snapshot commit spec entry — Correct.Accurately describes the decision to add
docs/spec-merkle-commit.mdand the rationale for making provenance explicit. Links are mentioned as added. No issues.
… to eliminate wildcard warnings
…pace MSRV at 1.71.1; update plan/log
…g; docs: strengthen Merkle spec wording; fix decision-log heading spacing
… RUSTUP_TOOLCHAIN=1.75.0; docs note
- TxId: repr(transparent) + const from_raw/value for FFI/WASM; add tests for tx lifecycle. - Scheduler: crate-private, doc ordering invariant; add unit test to prove drain order (scope_hash, rule_id). - Engine: duplicate rule detection already enforced; add integration tests for DuplicateRuleName/DuplicateRuleId. - Payload: wire-format rustdoc already present; round-trip + length guards in tests. - Graph: fields crate-private; edges_from ordering docs. - Record: add invariants to rustdoc for NodeRecord/EdgeRecord. - Math: clamp uses manual impl by design; silence clippy::manual_clamp locally. Crate allows module_name_repetitions to keep public API names stable. All rmg-core tests pass; clippy clean for rmg-core.
- Mat4::multiply: remove unresolved link. - Engine docs: avoid linking to private . - Footprint: qualify link; re-export at crate root. - Ensure passes.
- rust-toolchain.toml -> stable (rustfmt, clippy). - CI: remove MSRV job; all jobs run with dtolnay/rust-toolchain@stable. - Security Audit: run latest cargo-audit on stable (no pin). - Docs: record decision and execution plan update.
…napshot doc backticks)
…t to satisfy clippy -D missing_docs
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
docs/decision-log.md (1)
24-126: Format inconsistency still not resolved.From: Linus Torvalds torvalds@linux-foundation.org
To: flyingrobots flyingrobots@echo.dev
Cc: code-rabbit@vger.kernel.orgYou fixed the blank line before the heading (line 22), good. But the previous feedback about format inconsistency still stands:
Lines 5-21 use a markdown table for decision entries. Lines 24 onward use markdown headings with bullet lists. You've created two different formats in the same document with no explanation.
Either:
- Migrate lines 24-126 into the table format (annoying but consistent), OR
- Add a clear section break at line 23 like:
## Recent Decisions (2025-10-28 onwards) The following entries use heading + bullet format for detailed notes.Pick one. Don't leave this half-table, half-heading mess for someone else to untangle later.
BunBun.
docs/execution-plan.md (1)
41-134: What the hell is this PR's actual scope?Your PR is titled "core(math): canonicalize -0.0 in rotations; add MulAssign for Mat4" but these execution plan entries document:
- Pre-commit formatting policy changes (line 41-44)
- CI security hardening with audit/deny (line 45-52)
- Snapshot commit spec additions (line 54-57)
- PR #13 math polish (line 59-62) ← THE ONLY MENTION OF ACTUAL MATH
- Hooks formatting gates (line 64-68)
- rmg-core determinism tests (line 120-123)
- CI toolchain policy switches (line 125-128)
- Rustdoc lint cleanups (line 130-133)
This is either:
- Massive scope creep where you've bundled 8 different changes into one "math" PR
- You've selected the wrong files for review and the actual math changes are elsewhere
If this is scope creep, STOP. One PR, one logical change. Split this up. If you're trying to sneak infrastructure changes past review by hiding them in a "math" PR, that's even worse.
Line 59-62 mentions the math changes but WHERE IS THE ACTUAL CODE?
♻️ Duplicate comments (1)
crates/rmg-wasm/Cargo.toml (1)
1-27: Same workspace migration as rmg-ffi.
rmg-coremoved to workspace dependency. Same comment as for rmg-ffi—standard refactor, assumes root Cargo.toml is correct.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (20)
.githooks/pre-push(1 hunks).github/workflows/ci.yml(4 hunks).github/workflows/security-audit.yml(1 hunks)Cargo.toml(1 hunks)crates/rmg-core/src/footprint.rs(1 hunks)crates/rmg-core/src/lib.rs(1 hunks)crates/rmg-core/src/math/mat4.rs(5 hunks)crates/rmg-core/src/math/mod.rs(1 hunks)crates/rmg-core/src/record.rs(2 hunks)crates/rmg-core/src/scheduler.rs(1 hunks)crates/rmg-core/src/snapshot.rs(2 hunks)crates/rmg-core/tests/duplicate_rule_registration_tests.rs(1 hunks)crates/rmg-ffi/Cargo.toml(1 hunks)crates/rmg-geom/Cargo.toml(2 hunks)crates/rmg-wasm/Cargo.toml(1 hunks)deny.toml(1 hunks)docs/decision-log.md(2 hunks)docs/execution-plan.md(2 hunks)docs/spec-merkle-commit.md(1 hunks)rust-toolchain.toml(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
crates/rmg-core/src/math/mat4.rs (1)
crates/rmg-core/src/math/vec3.rs (1)
mul_assign(234-236)
crates/rmg-core/tests/duplicate_rule_registration_tests.rs (2)
crates/rmg-core/src/ident.rs (2)
make_node_id(35-40)make_type_id(27-32)crates/rmg-core/src/demo/motion.rs (1)
motion_rule(66-78)
crates/rmg-core/src/scheduler.rs (3)
crates/rmg-core/src/ident.rs (1)
make_node_id(35-40)crates/rmg-core/src/tx.rs (1)
from_raw(30-32)crates/rmg-core/src/engine_impl.rs (1)
new(71-84)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Security Audit
- GitHub Check: Cargo Audit (stable)
- GitHub Check: Cargo Audit (stable)
🔇 Additional comments (18)
crates/rmg-core/src/footprint.rs (1)
21-22: Documentation link is correct, but this file is orthogonal to the PR objectives.Look, the rustdoc inline link syntax is fine—
[pack_port_key](crate::footprint::pack_port_key)resolves correctly to the function at line 149. Documentation gets better. But here's the problem: your PR title is "canonicalize -0.0 in rotations; add MulAssign for Mat4," and I'm staring at footprint scheduling logic. This file has nothing to do with trigonometric canonicalization or matrix multiplication.Either:
- You accidentally included this unrelated documentation cleanup, or
- This is part of a broader workspace documentation consistency pass that wasn't mentioned in the objectives
Either way, it's orthogonal. The change itself is harmless and correct, but the narrative is confused. Did you mean to send a different file, or is this intentional churn?
crates/rmg-geom/Cargo.toml (2)
5-5: LGTM: MSRV documentation.From: Linus Torvalds torvalds@linux-foundation.org
To: flyingrobots flyingrobots@echo.dev
Cc: code-rabbit@vger.kernel.orgAdding
rust-version = "1.71.1"is the right thing to do. It documents the floor, makescargocheck it, and keeps your MSRV validation in the pre-push hook honest.BunBun.
14-14: LGTM: Workspace dependency.From: Linus Torvalds torvalds@linux-foundation.org
To: flyingrobots flyingrobots@echo.dev
Cc: code-rabbit@vger.kernel.orgMoving to
workspace = trueis standard monorepo practice. Centralized version management, no surprises.BunBun.
crates/rmg-core/src/snapshot.rs (2)
3-6: LGTM: External spec reference.From: Linus Torvalds torvalds@linux-foundation.org
To: flyingrobots flyingrobots@echo.dev
Cc: code-rabbit@vger.kernel.orgPointing to
docs/spec-merkle-commit.mdfor the precise definitions is smart. Keeps the code comments terse and avoids duplication drift. As long as that spec file actually exists and says what you claim it does, this is fine.BunBun.
45-45: LGTM: Code formatting fix.From: Linus Torvalds torvalds@linux-foundation.org
To: flyingrobots flyingrobots@echo.dev
Cc: code-rabbit@vger.kernel.orgUsing backticks for
state_rootis correct. Satisfiesclippy::doc_markdownand improves readability.BunBun.
crates/rmg-core/src/scheduler.rs (1)
118-159: LGTM: Deterministic ordering test.From: Linus Torvalds torvalds@linux-foundation.org
To: flyingrobots flyingrobots@echo.dev
Cc: code-rabbit@vger.kernel.orgThe test correctly validates that
drain_for_txreturns rewrites in ascending(scope_hash, rule_id)order. You insert out of order, theBTreeMapmaintains sorted keys,into_values()yields them in key order, and you verify the sequence matches[(1,1), (1,2), (2,1)].This locks down the ordering invariant mentioned in the module docs (line 4-6). Good.
BunBun.
.githooks/pre-push (1)
70-96: Good fix: explicit required vs optional handling.From: Linus Torvalds torvalds@linux-foundation.org
To: flyingrobots flyingrobots@echo.dev
Cc: code-rabbit@vger.kernel.orgYou addressed the previous feedback correctly:
- Required crates (
rmg-core,rmg-geom) must exist; missing ones trigger an error message and setmissing_required=1.- Optional crates (
rmg-ffi,rmg-wasm) log a skip message if absent.- The hook aborts if any required crate is missing (line 93-96).
No more silent skips that could hide typos or stale configuration. This is maintainable.
BunBun.
crates/rmg-core/src/record.rs (2)
10-14: Invariants documented but not enforced.From: Linus Torvalds torvalds@linux-foundation.org
To: flyingrobots flyingrobots@echo.dev
Cc: code-rabbit@vger.kernel.orgYou've documented invariants that
tymust be valid, node id is external, and payload is opaque. That's fine as far as documentation goes.But understand: these are promises, not guarantees. Nothing in this code enforces that
tyis actually valid in any schema, or that the payload makes sense. You're trusting the caller to uphold these contracts.If that's your intended API surface—thin records with invariants enforced elsewhere—then document where that enforcement happens. If you're planning validation layers, mention them. If not, at least you've warned future maintainers what the assumptions are.
BunBun.
24-29: Invariants documented but not enforced.From: Linus Torvalds torvalds@linux-foundation.org
To: flyingrobots flyingrobots@echo.dev
Cc: code-rabbit@vger.kernel.orgSame story as
NodeRecord: you promisefrom/toreference existing nodes,idis stable,tyis valid. The record type itself does nothing to enforce this.If
GraphStoreor the engine layer validates these contracts, mention it. If not, you're relying on convention and discipline. That can work in a controlled codebase, but make sure it's explicit.BunBun.
crates/rmg-core/tests/duplicate_rule_registration_tests.rs (2)
16-37: LGTM: Duplicate name test.From: Linus Torvalds torvalds@linux-foundation.org
To: flyingrobots flyingrobots@echo.dev
Cc: code-rabbit@vger.kernel.orgTest logic is straightforward: register
motion_rule()twice, expectDuplicateRuleName, verify the name matchesMOTION_RULE_NAME. Clean and correct.BunBun.
54-77: Test is correct—hashing scheme is verified.From: Linus Torvalds torvalds@linux-foundation.org
To: flyingrobots flyingrobots@echo.dev
Cc: code-rabbit@vger.kernel.orgYour concern was sound in principle, but the investigation clears the code completely:
The scheme matches.
build.rscomputesMOTION_UPDATE_FAMILY_IDusingblake3("rule:motion/update")at compile time. Your test computesblake3("rule:" + MOTION_RULE_NAME)whereMOTION_RULE_NAME = "motion/update". They're identical.It's not hardcoded.
motion_rule()usesMOTION_RULE_ID, which is assignedMOTION_UPDATE_FAMILY_ID—the generated value frombuild.rs. Not a magic constant buried in the code.Validation already exists. The codebase has a test
motion_rule_id_matches_domain_separated_name()inmotion.rs(lines 134–145) that does exactly what you're asking for. It's already there, already passing, already validating the scheme.Your test won't pass for the wrong reason. It will pass because the implementation is sound.
BunBun.
docs/spec-merkle-commit.md (1)
1-7: Clean spec introduction, minor terminology redundancy.The document clearly defines the two hash types upfront. Line 6 uses "commit hash (commit_id)" which is fine for first reference but just use "commit_id" consistently thereafter since you've already aliased it.
deny.toml (1)
1-36: Deny config is sensible for permissive licensing.License allowlist is comprehensive for common permissive licenses. Denying copyleft is an explicit policy choice—fine, but make sure your CONTRIBUTING docs state this so contributors don't waste time proposing GPL deps.
Line 12 includes ISC which wasn't mentioned in the execution-plan's list (line 51 of execution-plan.md). Not a problem, just noting the discrepancy in documentation.
Line 30:
multiple-versions = "warn"is practical. Going to "deny" would break too many real-world dependency trees. Good choice.Line 31:
wildcards = "deny"is correct. Version wildcards are a supply-chain risk.Lines 34-35: Denying unknown registries/git sources is good security hygiene.
One suggestion: add a comment in this file explaining the policy rationale for copyleft denial, so future maintainers understand the reasoning:
# Disallow copyleft by default; we do not expect GPL-family in the runtime. + # Rationale: Echo is Apache-2.0 and targets embedding in proprietary systems. copyleft = "deny"crates/rmg-ffi/Cargo.toml (1)
1-17: ---Workspace dependency setup confirmed—nothing to fix here.
Your workspace configuration is solid. The root
Cargo.tomlhasrmg-coreproperly defined in[workspace.dependencies]withversion = "0.1.0"andpath = "crates/rmg-core". The migration toworkspace = trueincrates/rmg-ffi/Cargo.tomlis correct. No version drift, no path issues, no problems.The refactor is good. Ship it.
BunBun
crates/rmg-core/src/math/mod.rs (1)
38-42: From: Linus Torvalds torvalds@linux-foundation.org
To: {PR_AUTHOR_NAME} <{PR_AUTHOR_EMAIL}>
Cc: code-rabbit@vger.kernel.org
Subject: Why is there an allow() attribute doing absolutely nothing?Remove
#[allow(clippy::manual_clamp)]—it's dead weight.Your function already calls
.clamp(). The entire point ofclippy::manual_clampis to warn you when you're NOT using.clamp()and instead rolling your own min/max logic. You're using the standard library method. The assertion on line 40 doesn't trigger this lint.There's no git history explaining this. There's no clippy config requiring it. It's just lint suppression cargo-culting—the exact kind of technical debt that makes codebases harder to maintain.
Stop adding
#[allow]attributes as a defense mechanism. That's how you end up with unmaintainable code where nobody knows why a suppression exists or if it's actually needed.Either use clippy's analysis to fix something, or don't suppress it. This serves neither purpose.
BunBun
Likely an incorrect or invalid review comment.
Cargo.toml (1)
11-13: Workspace dependency wiring holds up.Version+path combo here keeps the local crate aligned for the workspace consumers; no red flags.
.github/workflows/ci.yml (1)
94-135: Expanded guardrails are finally worth the cycles.Stable toolchain everywhere, per-crate rustdoc gates, and the dedicated audit/deny jobs mean the pipeline will actually yell before junk ships.
crates/rmg-core/src/math/mat4.rs (1)
64-253: Math layer cleanup is on point.The sin/cos sanitization finally rids us of -0.0 baggage, and the MulAssign impls reuse
multiplywithout aliasing nonsense. That’s the right fix.
- pre-push: set PINNED=stable; remove MSRV lane; run fmt/clippy/tests and rustdoc on stable. - pre-commit: fail if SKIP_HOOKS is set; no bypass. - Messaging updated accordingly.
…e (stable toolchain, no bypass)
…date CONTRIBUTING
…ng cargo-audit each run
…e_count=0, genesis parents, empty digest wording (blake3(b)), mandate v1 decision_digest empty; docs: log intent and decision
…hooks; update CONTRIBUTING with pinned version
…'Recent Decisions' section for heading+bullets format consistency
…l and 'cargo audit --deny warnings' on pinned 1.90.0
This PR canonically normalizes -0.0 in Mat4 trig constructors (rotation_x/y/z) to avoid bitwise drift across platforms, and adds ergonomic in-place multiplication (MulAssign) for Mat4.
CI: formats, clippy (pedantic), rustdoc all green locally.