fix: Address MII+ review feedback - hash position, bounds checks, test coverage#15
Conversation
…e, etc. Co-authored-by: Steake <530040+Steake@users.noreply.github.com>
Co-authored-by: Steake <530040+Steake@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This PR addresses review feedback on the MII+ (Mutual Information Index Plus) tournament system, implementing critical bug fixes, code quality improvements, and comprehensive test coverage enhancements. The changes focus on ensuring deterministic behavior, preventing edge-case failures, and cleaning up technical debt.
Key Changes:
- Fixed lexicographic tiebreaker to include glider position in hash calculation and handle hash equality gracefully
- Added bounds checking for spawn jitter to prevent array access panics
- Prevented noise cells from overwriting glider cells during battle initialization
- Enhanced test coverage for jitter ranges, energy fluctuations, noise placement, and tiebreaker determinism
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| docs/MII+.md | Added note documenting that phenotype feature is deferred to future PR |
| crates/bitcell-simulation/src/lib.rs | Extracted derive_position_from_pubkey() helper to reduce duplication across miner agents |
| crates/bitcell-simulation/Cargo.toml | Removed unused dependencies bitcell-ebsl and thiserror |
| crates/bitcell-consensus/src/orchestrator.rs | Removed verbose defensive comments about borrow checker |
| crates/bitcell-ca/src/battle.rs | Core fixes: position in hash, bounds check for jitter, noise skip logic, visibility changes to pub(crate), and comprehensive test suite additions |
| crates/bitcell-admin/src/api/blocks.rs | Added guard against height == 0 to prevent underflow in block detail calculation |
| Cargo.toml | Formatting improvement - split workspace members onto separate lines |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| } else { | ||
| assert_eq!(outcome, BattleOutcome::BWins); |
There was a problem hiding this comment.
The test logic doesn't handle the case where hash_a == hash_b. When hashes are equal, lexicographic_break() returns AWins, but the test's else branch (line 824) assumes hash_a > hash_b and expects BWins, which would cause a false test failure. The condition should be } else if hash_a > hash_b { to match the implementation.
| } else { | |
| assert_eq!(outcome, BattleOutcome::BWins); | |
| } else if hash_a > hash_b { | |
| assert_eq!(outcome, BattleOutcome::BWins); | |
| } else { | |
| // If hashes are equal, lexicographic_break returns AWins | |
| assert_eq!(outcome, BattleOutcome::AWins); |
|
@copilot apply changes based on the comments in this thread |
Co-authored-by: Steake <530040+Steake@users.noreply.github.com>
Fixed in |
No changes - review fixes already merged in PR #15
💬 We'd love your input! Share your thoughts on Copilot coding agent in our 2 minute survey.