diff --git a/docs/openup/analysis-design/design-wi-1-injectable-rng.md b/docs/openup/analysis-design/design-wi-1-injectable-rng.md new file mode 100644 index 0000000..30e5641 --- /dev/null +++ b/docs/openup/analysis-design/design-wi-1-injectable-rng.md @@ -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). diff --git a/docs/openup/project-management/work-items.md b/docs/openup/project-management/work-items.md new file mode 100644 index 0000000..4b5cb41 --- /dev/null +++ b/docs/openup/project-management/work-items.md @@ -0,0 +1,11 @@ +# Skara Brae — Work Items List + +| # | Issue | Work Item | Iteration | PSP level | Est. N (LOC) | Act. N (LOC) | Est. time (min) | Act. time (min) | Status | +|---|-------|-----------|-----------|-----------|--------------|--------------|-----------------|-----------------|--------| +| 1 | skara-brae-server#1 | Injectable RNG (plan item #8) | I1 | PSP1.0 | 35 (pred. 42, PI 37–73) | **41** | 105 (PI 75–135) | **90** | **Done** — both actuals inside 70% PI (size −2.4%, time −14%) | +| 2 | skara-brae-server#2 | Architecture cleanup + OpenUP work set (plan item #7) | I1 | PSP1.0 | 660 (pred. 951, PI 607–1295) | — | 620 (PI 455–785) | — | Open — typed responses, run_adventure extraction, constants, unwrap, fmt baseline; full OpenUP work set (Vision, UC, Suppl. Specs, A&D, Test Cases) | + +Notes: +- Estimates filled during PLN (PROBE); actuals during PM postmortem. +- Iteration I1 = first Skara Brae iteration (starts with this WI). +- OpenUP work products live at the project top folder (`docs/openup/`), not per-repo. diff --git a/docs/openup/test/test-report-wi-1-injectable-rng.md b/docs/openup/test/test-report-wi-1-injectable-rng.md new file mode 100644 index 0000000..ff2cea1 --- /dev/null +++ b/docs/openup/test/test-report-wi-1-injectable-rng.md @@ -0,0 +1,36 @@ +# WI-1 Test Report — Injectable RNG (skara-brae-server#1, PSP1) + +Date: 2026-10-02 · Phase: TST · Executor: agent (PM session, Kilo/glm-5.3) + +## Scope under test + +Branch `wi-1-injectable-rng` (uncommitted working tree vs base `64e081a`): +`sb-core/src/character.rs` (create_character signature + new determinism test), +`sb-api/src/routes/mod.rs` (request_rng helper, create handler, run_adventure +consolidation). + +## Test cases + +| # | Test | Purpose | Result | +|---|---|---|---| +| 1 | `create_character_is_deterministic_under_seeded_rng` (NEW) | Two `StdRng::seed_from_u64(42)` runs yield identical stats (Debug-string) and equal hp/max_hp/energy/nerve — proves RNG injectability end-to-end | **pass** | +| 2 | Existing sb-core suite (44 tests: character 20, combat 8, services 17 -1 overlap after adding #1) | No regression from signature change / rng threading | **pass** | +| 3 | Acceptance audit: `grep -rn "rand::rng()" crates/` | Zero occurrences in the whole repo (incl. tests) — acceptance criterion | **pass** (only remaining entropy: `sb-auth/src/hash.rs:8` `getrandom::fill`, excluded by design) | +| 4 | `cargo clippy --all-targets -- -D warnings` | Zero-warning standard | **pass** (clean) | +| 5 | `cargo build --workspace` | Compile integrity | **pass** | +| 6 | `rustfmt --check` (touched lines) | Coding standard on new/rewritten code | **pass** (repo-wide pre-existing fmt debt documented, not touched — see CMP log) | + +## Evidence + +- `cargo test --workspace`: **45 passed, 0 failed** (sb-core lib target; remaining targets empty) +- Determinism test present and green: `test character::tests::create_character_is_deterministic_under_seeded_rng ... ok` +- Full output captured in session log (PSP time log, TST phase, 2026-10-02 22:17) + +## Defects found in test phase + +None. (All defects D1–D11 were found in Design Review / Code / Code Review phases; see defect log.) + +## Notes + +- The new test replaced a whole-struct `Debug` comparison after D8 (HashMap iteration order instability) — comparison now covers the random-dependent state exactly. +- No behavioral change expected from ThreadRng→StdRng: same distributions, one OS-entropy seeding per request (design rev 2, D1 correction).