docs(openup): migrate WI-1 artifacts from unversioned project-top docs/
work-items.md (project-management/), design-wi-1-injectable-rng.md (analysis-design/), test-report-wi-1-injectable-rng.md (test/) move into the canonical OpenUP home per the 2026-10-03 decision reversal. Verbatim, no content changes; project-top docs/ husk removed.
This commit is contained in:
parent
4798f55660
commit
2343d5e154
3 changed files with 141 additions and 0 deletions
94
docs/openup/analysis-design/design-wi-1-injectable-rng.md
Normal file
94
docs/openup/analysis-design/design-wi-1-injectable-rng.md
Normal file
|
|
@ -0,0 +1,94 @@
|
|||
# WI-1 Design — Injectable RNG (skara-brae-server#1, PSP1)
|
||||
|
||||
Revision 2 — after 1B Design Review (defects D1–D5, D7; D1 blocker: ThreadRng !Send across .await).
|
||||
|
||||
## Requirements (source: Forgejo issue #1)
|
||||
|
||||
1. No direct `rand::rng()` in non-test code outside a single construction point.
|
||||
2. `create_character()` and `resolve_combat_full()` accept an injectable RNG.
|
||||
3. All existing tests green; `cargo fmt --check`, `clippy --all-targets -D warnings`, `cargo test` clean.
|
||||
|
||||
Out of scope: route-handler refactor, typed responses, magic numbers.
|
||||
|
||||
Note: `resolve_combat_full` already accepts `rng: &mut impl Rng` (combat.rs:174–183) —
|
||||
only the call-site argument changes. The issue's phrasing overstates; its
|
||||
`train_stat()` precedent reference is also inaccurate (train_stat takes no RNG).
|
||||
Actual precedents: `pick_enemy`, `roll_stats`, `resolve_attack`, `open_ended_roll`.
|
||||
|
||||
## Design
|
||||
|
||||
Pattern: domain functions take `rng: &mut impl Rng`; request handlers obtain
|
||||
their RNG from a **single shared constructor** in sb-api:
|
||||
|
||||
```rust
|
||||
fn request_rng() -> StdRng { StdRng::from_os_rng() }
|
||||
```
|
||||
|
||||
`StdRng` (not `rand::rng()`/ThreadRng) is required because the handler futures
|
||||
must stay `Send` for axum: a ThreadRng binding held across the handlers'
|
||||
`.await` points makes the future `!Send` and fails to compile (D1/D2).
|
||||
`StdRng` is `Send + Sync` and matches the seeded `StdRng` used in domain tests.
|
||||
|
||||
Changes:
|
||||
|
||||
1. **`sb-core/src/character.rs` — `create_character`**
|
||||
Signature: `create_character(rng: &mut impl Rng, user_id: u32, name: String, race: Race, class: Class) -> Character`.
|
||||
Drops the internal `let mut rng = rand::rng();`, forwards `rng` to
|
||||
`roll_stats` (which already takes `&mut impl Rng`). No other body change.
|
||||
|
||||
2. **`sb-api/src/routes/mod.rs` — `request_rng()` helper + create-character
|
||||
handler (~line 288)**
|
||||
Add `request_rng()` (single construction point in the codebase, satisfying
|
||||
the acceptance criterion literally). Create handler:
|
||||
`let mut rng = request_rng();` then `create_character(&mut rng, ...)`.
|
||||
|
||||
3. **`sb-api/src/routes/mod.rs` — `run_adventure` handler**
|
||||
Replace the three `rand::rng()` constructions (lines 442, 497, 504) with
|
||||
one `let mut rng = request_rng();` before the first guarded block; thread
|
||||
`&mut rng` into `domain::pick_enemy`, `combat::resolve_combat_full`
|
||||
(replacing the inline `&mut rand::rng()` argument), and the loot-roll
|
||||
`rng.random_bool(...)` loop. Hoist the block-scoped `use rand::Rng;` (line
|
||||
503) so `random_bool` resolves at the consolidated call sites. Being
|
||||
`Send`, the rng may cross `.await` points legally.
|
||||
|
||||
4. **`sb-core/src/character.rs` — new test (new object, 18 LOC planned)**
|
||||
`create_character_is_deterministic_under_seeded_rng`: two
|
||||
`StdRng::seed_from_u64(42)` runs produce identical characters compared via
|
||||
`format!("{:?}")` (Character derives no PartialEq — D5); covers
|
||||
stats/HP/energy/nerve/spells and id/gold. No different-seed inequality
|
||||
assert (probabilistic flake, review nit — dropped).
|
||||
|
||||
## Coverage & exclusions (acceptance audit)
|
||||
|
||||
All four non-test `rand::rng()` sites covered (character.rs:130;
|
||||
routes/mod.rs:442, 497, 504). Workspace audit found exactly one other direct
|
||||
randomness construction: `sb-auth/src/hash.rs:8` uses `getrandom::fill` for
|
||||
password-salt entropy — infrastructure entropy, not domain randomness;
|
||||
deliberately excluded (out of scope).
|
||||
|
||||
## 2B Code Review record (rev 2.1)
|
||||
|
||||
Implementation deviations from the letter of rev 2 (both accepted, recorded per review nit D11):
|
||||
- `use rand::Rng;` remains block-scoped in the loot section (its only use site; matches file style) instead of hoisted.
|
||||
- The create-handler rng is wrapped in a scoping block (`let mut char = { let mut rng = request_rng(); ... }`) instead of a plain `let mut rng = request_rng();` — tighter drop scope before the following awaits.
|
||||
|
||||
Fmt findings D9/D10 fixed on the touched lines; repo-wide pre-existing fmt debt (HEAD fails `cargo fmt --check` unrelated to this WI) was NOT touched — no drive-by reformat.
|
||||
|
||||
## Alternatives considered
|
||||
|
||||
- `AppState`-held shared RNG: rejected — single-writer contention, no benefit.
|
||||
- ThreadRng with tight scoping/drop before awaits: rejected — fragile
|
||||
(lexical drop scopes + await regions), `StdRng` construction is equivalent
|
||||
and robust (D1 correction).
|
||||
- Generic `R: Rng` type parameter instead of `impl Trait`: equivalent;
|
||||
`&mut impl Rng` matches the existing `services.rs` style.
|
||||
|
||||
## Size plan (from PROBE, method C)
|
||||
|
||||
| Part | B | D | M | A |
|
||||
|---|---|---|---|---|
|
||||
| character.rs | 316 | 0 | 3 | 0 |
|
||||
| routes/mod.rs | 1049 | 2 | 12 | 2 + 4 (request_rng, review rework) |
|
||||
| new test | — | — | — | 18 |
|
||||
|
||||
Plan N = 35 (original) → 39 after review rework; predicted N = 42 (70% PI 37–73) — rework keeps the plan inside the interval. Time 105 min (75–135).
|
||||
Loading…
Add table
Add a link
Reference in a new issue