diff --git a/docs/plans/0067-r-sweep-followups.md b/docs/plans/0067-r-sweep-followups.md new file mode 100644 index 0000000..8fb2255 --- /dev/null +++ b/docs/plans/0067-r-sweep-followups.md @@ -0,0 +1,551 @@ +# R-sweep follow-ups (SQN100 + stage1-r --trace) — Implementation Plan + +> **Parent spec:** `docs/specs/0067-r-sweep-followups.md` +> +> **For agentic workers:** REQUIRED SUB-SKILL: use the `implement` skill to run +> this plan. Steps use `- [ ]` checkboxes for tracking. + +**Goal:** Ship two settled, additive follow-ups to the cycle-0065/0066 R-sweep +surface — Part A (#130) a turnover-robust `sqn_normalized` ranking metric, Part B +(#135) per-member `--trace` for the stage1-r sweep. + +**Architecture:** Part A threads one `f64` field through `RMetrics` / +`summarize_r` / the registry rank vocabulary, all additively (raw `sqn` and the +default ranker unchanged; the new field carries `#[serde(default)]` for C18 +back-compat). Part B makes `stage1_r_sweep_family` honour `--trace` exactly as +`momentum_sweep_family` does (reusing the existing `persist_traces_r`) and removes +the interim refusal guard in `run_sweep`. + +**Tech Stack:** `aura-engine` (`report.rs`: `RMetrics`, `summarize_r`), +`aura-registry` (`lib.rs`: `Metric`, `metric_cmp`), `aura-cli` (`main.rs`: +`stage1_r_sweep_family`, `run_sweep`; `tests/cli_run.rs`). + +**Files this plan creates or modifies:** + +- Modify: `crates/aura-engine/src/report.rs` — `RMetrics.sqn_normalized` field + + `SQN_CAP` const + `summarize_r` compute + 3 in-file struct literals + 2 new / + 2 extended unit tests. +- Modify: `crates/aura-registry/src/lib.rs` — `Metric::SqnNormalized` + name arm + + comparator arm + `UnknownMetric` Display + doc-comment + `report_with_r` field + + 1 new / 1 extended unit test. +- Modify: `crates/aura-cli/tests/cli_run.rs` — re-baseline the + `stage1_r_single_run_output_golden` pinned string; invert the + `sweep_strategy_stage1_r_trace_is_refused_name_works` test. +- Modify: `crates/aura-cli/src/main.rs` — `stage1_r_sweep_family` honours `trace` + + doc; `run_sweep` refusal guard removed + doc. + +**Sequencing (load-bearing):** Task 1 (the new `RMetrics` field) is a non-`Default` +struct-field addition that hard-breaks **four** struct literals — it threads all +of them so the workspace compiles at task end (compile-gate). Task 3 (golden +re-pin) and Task 5 (full unfiltered suite) must run **after** Task 1, since the +field changes the serialized `r:` block and the golden pins it byte-exact. Tasks +2, 3, 4 are mutually independent given Task 1. Task 5 is last. + +--- + +### Task 1: `RMetrics.sqn_normalized` field + `summarize_r` compute (Part A engine) + +**Files:** +- Modify: `crates/aura-engine/src/report.rs` (struct ~45-57; `SQN_CAP` ~73; + compute ~147-154; empty arm ~111-123; populated return ~183-195; in-file test + literal ~951-963; tests ~686-749) +- Modify: `crates/aura-registry/src/lib.rs` (test helper `report_with_r` ~248-264) + — threaded here ONLY to keep the workspace compiling (the registry *behaviour* + is Task 2) + +> **RED note for the reviewer:** a brand-new struct field cannot have a +> *compiling* RED test — a test that reads `m.sqn_normalized` does not compile +> until the field exists. So the RED here is the **compile failure** (Step 2), and +> the field + its tests necessarily land in one task. This is the expected shape +> for adding a field, not a spec ambiguity. + +- [ ] **Step 1: Write the failing tests** (append two new tests after + `summarize_r_sqn_zero_when_under_two_trades_or_zero_variance`, ~line 749, and + extend the empty-ledger test): + +```rust + #[test] + fn summarize_r_sqn_normalized_caps_trade_count_at_100() { + // 144 closed trades alternating R = 1.0 / 2.0 (nonzero variance, sd > 0). + // raw sqn = √144·q, SQN100 = √(min(144,100))·q = √100·q for the SAME ratio + // q = mean/sd, so sqn / sqn_normalized = √(144/100) = 1.2, independent of q. + let rec: Vec<_> = (0..144) + .map(|i| r_row(true, if i % 2 == 0 { 1.0 } else { 2.0 }, false, 0.0)) + .collect(); + let m = summarize_r(&rec, 0.0); + assert_eq!(m.n_trades, 144); + assert!(m.sqn_normalized > 0.0, "sqn_normalized nonzero; got {}", m.sqn_normalized); + assert!(m.sqn_normalized < m.sqn, "capped SQN100 < raw sqn for n > 100"); + assert!( + (m.sqn / m.sqn_normalized - 1.2).abs() < 1e-9, + "sqn / sqn_normalized = √(144/100) = 1.2; got {}", + m.sqn / m.sqn_normalized + ); + } + + #[test] + fn summarize_r_sqn_normalized_equals_raw_sqn_below_cap() { + // n = 4 <= 100 -> cap inactive -> SQN100 == raw sqn (same ledger as + // summarize_r_sqn_is_sqrt_n_mean_over_stdev, where sqn = 1.0). + let rec = vec![ + r_row(true, 1.0, false, 0.0), + r_row(true, 1.0, false, 0.0), + r_row(true, 1.0, false, 0.0), + r_row(true, -1.0, false, 0.0), + ]; + let m = summarize_r(&rec, 0.0); + assert!((m.sqn_normalized - m.sqn).abs() < 1e-12, "below cap, SQN100 == sqn"); + assert!((m.sqn_normalized - 1.0).abs() < 1e-9, "sqn_normalized = 1.0 here"); + } +``` + + And add one assertion to the existing `summarize_r_is_zero_on_empty` test (after + the `m.sqn` assertion, ~line 692): + +```rust + assert_eq!(m.sqn_normalized, 0.0); +``` + + And add a serde back-compat test (C18 — the load-bearing `serde(default)` claim), + after `runmetrics_with_r_block_round_trips` (~line 969): + +```rust + #[test] + fn rmetrics_deserializes_without_sqn_normalized_field_to_zero() { + // C18 back-compat: a pre-0067 `r:` block (serialized before sqn_normalized + // existed) lacks the field; serde(default) must fill it with 0.0. + let legacy = r#"{"expectancy_r":1.0,"n_trades":4,"win_rate":0.5,"avg_win_r":1.5,"avg_loss_r":-0.5,"profit_factor":3.0,"max_r_drawdown":0.5,"n_open_at_end":0,"sqn":1.0,"net_expectancy_r":0.4,"conviction_terciles_r":[0.0,0.0,0.0]}"#; + let m: RMetrics = serde_json::from_str(legacy).expect("legacy RMetrics deserializes"); + assert_eq!(m.sqn_normalized, 0.0); + assert_eq!(m.sqn, 1.0); + } +``` + +- [ ] **Step 2: Run to verify it fails** + +Run: `cargo test -p aura-engine` +Expected: FAIL — compile error `no field `sqn_normalized` on type `RMetrics`` (the +field does not exist yet; the new tests reference it). This is the compile-RED. + +- [ ] **Step 3: Add the field, the constant, and the compute** + +In `crates/aura-engine/src/report.rs`, add the field to `RMetrics` between `sqn` +(~54) and `net_expectancy_r` (~55): + +```rust + /// (mean_R / sample-stdev_R) · √(min(n, SQN_CAP)): the n-normalized SQN + /// ("SQN score", Van Tharp), turnover-robust vs. the raw `sqn`. n<2 or + /// zero-variance -> 0.0. + #[serde(default)] + pub sqn_normalized: f64, +``` + +Add the module constant just after `mod r_col { ... }` (~line 73, before the +`summarize_r` doc-comment): + +```rust +/// Van Tharp's conventional trade-count cap for the n-normalized SQN ("SQN +/// score"): capping n stops the single-number objective rewarding sheer turnover. +const SQN_CAP: u64 = 100; +``` + +Replace the `sqn` compute block (current ~147-154) with a combined block that +factors the shared sample-stdev and computes both keys: + +```rust + // SQN = √n · mean / sample-stdev (raw, Van Tharp). SQN100 = √(min(n,SQN_CAP)) · + // mean / sd — the same dispersion ratio with the trade count capped + // (turnover-robust). n < 2 or zero variance -> 0.0 for both (dispersion undefined). + let (sqn, sqn_normalized) = if n < 2 { + (0.0, 0.0) + } else { + let var = rs.iter().map(|&r| (r - mean).powi(2)).sum::() / (n as f64 - 1.0); + let sd = var.sqrt(); + if sd > 0.0 { + let ratio = mean / sd; + ((n as f64).sqrt() * ratio, (n.min(SQN_CAP) as f64).sqrt() * ratio) + } else { + (0.0, 0.0) + } + }; +``` + +Add `sqn_normalized: 0.0,` to the empty-ledger early-return arm, after `sqn: 0.0,` +(~line 120): + +```rust + sqn: 0.0, + sqn_normalized: 0.0, + net_expectancy_r: 0.0, +``` + +Add `sqn_normalized,` to the populated return, after `sqn,` (~line 192): + +```rust + sqn, + sqn_normalized, + net_expectancy_r, +``` + +Thread the in-file test literal `runmetrics_with_r_block_round_trips` (~line 960), +after `sqn: 1.0,`: + +```rust + sqn: 1.0, + sqn_normalized: 1.0, + net_expectancy_r: 0.4, +``` + +Thread the registry test helper `report_with_r` in +`crates/aura-registry/src/lib.rs` (~line 259), after `sqn,` — set it to mirror +`sqn` so the rank-key wiring test (Task 2) can reuse this helper without a new +parameter: + +```rust + sqn, + sqn_normalized: sqn, // mirror sqn: only the rank-key wiring is under test here + net_expectancy_r, +``` + +Also update `report_with_r`'s doc-comment (~246) to mention the mirror: + +```rust + /// A family member carrying an R block — mirrors `report_with` but sets + /// `metrics.r = Some(..)`. Only sqn / expectancy_r / net_expectancy_r vary per + /// call (the rank keys); `sqn_normalized` mirrors `sqn`; the rest are fixed. + fn report_with_r(sqn: f64, expectancy_r: f64, net_expectancy_r: f64) -> RunReport { +``` + +- [ ] **Step 4: Run to verify the engine tests pass** + +Run: `cargo test -p aura-engine` +Expected: PASS — all `aura-engine` unit tests, including the two new `summarize_r` +tests, the extended empty-ledger test, and the serde back-compat test. (0 failed.) + +- [ ] **Step 5: Confirm the whole workspace still compiles (compile-gate)** + +Run: `cargo build --workspace` +Expected: 0 errors (all four `RMetrics` struct literals threaded — the two in +`report.rs`, the in-file test literal, and `report_with_r` in the registry). + +--- + +### Task 2: `Metric::SqnNormalized` rank key (Part A registry) + +**Files:** +- Modify: `crates/aura-registry/src/lib.rs` (enum ~100-107; doc ~111; name arm + ~132-135; comparator ~152; Display ~211; tests ~266-302) + +- [ ] **Step 1: Write the failing / extended tests** + +Add a new test after `rank_by_sqn_orders_members_descending` (~line 276) — it +reuses `report_with_r`, where `sqn_normalized` mirrors `sqn`: + +```rust + #[test] + fn rank_by_sqn_normalized_orders_members_descending() { + // report_with_r sets sqn_normalized == sqn, so ranking by the new key + // orders these members highest-first by their sqn value. + let reports = vec![ + report_with_r(0.5, 0.1, 0.1), + report_with_r(2.0, 0.4, 0.4), + report_with_r(1.0, 0.2, 0.2), + ]; + let ranked = rank_by(reports, "sqn_normalized").expect("rank sqn_normalized"); + let vals: Vec = ranked + .iter() + .map(|r| r.metrics.r.as_ref().unwrap().sqn_normalized) + .collect(); + assert_eq!(vals, vec![2.0, 1.0, 0.5], "sqn_normalized must rank highest-first"); + } +``` + +Extend the existing `unknown_metric_message_lists_r_metrics` test (~line 297) with +one assertion: + +```rust + assert!(msg.contains("sqn_normalized"), "msg: {msg}"); +``` + +- [ ] **Step 2: Run to verify it fails** + +Run: `cargo test -p aura-registry sqn_normalized` +Expected: FAIL — `rank_by("sqn_normalized")` returns `Err(UnknownMetric)` (the name +is not yet resolved), and the `unknown_metric_message` assertion fails (the Display +string does not yet list `sqn_normalized`). + +- [ ] **Step 3: Add the enum variant, the two match arms, and the Display entry** + +Add the variant to `enum Metric` after `Sqn` (~line 104): + +```rust + Sqn, + SqnNormalized, + ExpectancyR, +``` + +Add the name-resolution arm after `"sqn" => Metric::Sqn,` (~line 132): + +```rust + "sqn" => Metric::Sqn, + "sqn_normalized" => Metric::SqnNormalized, +``` + +Add the comparator arm after the `Metric::Sqn` arm (~line 152): + +```rust + Metric::Sqn => r_get(b, |r| r.sqn).total_cmp(&r_get(a, |r| r.sqn)), + Metric::SqnNormalized => { + r_get(b, |r| r.sqn_normalized).total_cmp(&r_get(a, |r| r.sqn_normalized)) + } +``` + +Update the `UnknownMetric` Display string (~line 211) — insert `sqn_normalized, ` +after `sqn, `: + +```rust + "unknown metric '{m}' (known: total_pips, max_drawdown, bias_sign_flips, sqn, sqn_normalized, expectancy_r, net_expectancy_r)" +``` + +Update the `metric_cmp` doc-comment (~line 111) — "the three R keys" → "the four R +keys" and list the new key: + +```rust +/// the four R keys (`sqn`, `sqn_normalized`, `expectancy_r`, `net_expectancy_r`) higher-is-better; +``` + +- [ ] **Step 4: Run to verify the registry tests pass** + +Run: `cargo test -p aura-registry` +Expected: PASS — all registry unit tests, including the new +`rank_by_sqn_normalized_orders_members_descending` and the extended +`unknown_metric_message_lists_r_metrics`. (0 failed.) + +--- + +### Task 3: Re-baseline the single-run golden (Part A) + +**Files:** +- Modify: `crates/aura-cli/tests/cli_run.rs` (`stage1_r_single_run_output_golden` + pinned string ~1618) + +> Depends on Task 1 (the serialized `r:` block now carries `sqn_normalized`). For +> the golden's run `n_trades = 3` (≤ 100), so the cap is inactive and +> `sqn_normalized == sqn == 3.141496526818299`; serde emits it between `"sqn"` and +> `"net_expectancy_r"` (struct declaration order). + +- [ ] **Step 1: Confirm the golden now drifts** + +Run: `cargo test -p aura-cli --test cli_run stage1_r_single_run_output_golden` +Expected: FAIL — the actual output now contains `"sqn_normalized":...` which the +pinned string lacks. + +- [ ] **Step 2: Re-pin the expected string** + +Replace the pinned `r#"..."#` string literal (~line 1618) with the re-baselined +value (the only change is the inserted `,"sqn_normalized":3.141496526818299` after +`"sqn":3.141496526818299`): + +```rust + r#""metrics":{"total_pips":0.34185000000002036,"max_drawdown":0.11139999999998655,"bias_sign_flips":2,"r":{"expectancy_r":1.2710005136982836,"n_trades":3,"win_rate":1.0,"avg_win_r":1.2710005136982836,"avg_loss_r":0.0,"profit_factor":0.0,"max_r_drawdown":0.0,"n_open_at_end":1,"sqn":3.141496526818299,"sqn_normalized":3.141496526818299,"net_expectancy_r":1.2710005136982836,"conviction_terciles_r":[0.9285858482198718,2.0771328641652427,0.8072828287097363]}}}"#, +``` + +> If Step 3 still fails on a byte mismatch, do NOT hand-tweak digits: capture the +> real tail with `cargo run -q -p aura-cli -- run --harness stage1-r` and paste the +> exact `"metrics":...` suffix it prints. + +- [ ] **Step 3: Run to verify it passes** + +Run: `cargo test -p aura-cli --test cli_run stage1_r_single_run_output_golden` +Expected: PASS. + +--- + +### Task 4: Wire `--trace` for the stage1-r sweep (Part B) + +**Files:** +- Modify: `crates/aura-cli/tests/cli_run.rs` + (`sweep_strategy_stage1_r_trace_is_refused_name_works` ~1688-1715 — inverted) +- Modify: `crates/aura-cli/src/main.rs` (`stage1_r_sweep_family` ~1107-1159; + `run_sweep` guard ~1287-1296 + doc ~1281-1285) + +- [ ] **Step 1: Invert the refusal test into a persistence test** + +Replace the whole `sweep_strategy_stage1_r_trace_is_refused_name_works` test +(~1688-1715), including its doc-comment, with: + +```rust +/// Property (#135): `aura sweep --strategy stage1-r --trace ` now persists one +/// member dir per grid point (it was refused with exit 2 before). The 2×2 fast×slow +/// grid yields 4 members; each carries the THIRD `r_equity` tap (the stage1-r member +/// has the R curve, unlike the two-tap sma/momentum members), and a member is +/// chartable via `chart / --tap r_equity`. `--name` (record the rankable +/// family, no per-member streams) still works. +#[test] +fn sweep_strategy_stage1_r_trace_persists_member_dirs_with_r_equity() { + let cwd = temp_cwd("sweep-stage1-r-trace"); + let traced = Command::new(BIN) + .args(["sweep", "--strategy", "stage1-r", "--trace", "t1"]) + .current_dir(&cwd) + .output() + .expect("spawn aura sweep --strategy stage1-r --trace"); + assert!( + traced.status.success(), + "stage1-r --trace must persist members (exit 0): {:?}; stderr: {}", + traced.status, + String::from_utf8_lossy(&traced.stderr) + ); + let base = cwd.join("runs/traces/t1"); + let members: Vec = std::fs::read_dir(&base) + .expect("read t1 trace dir") + .map(|e| e.expect("dir entry").file_name().to_string_lossy().into_owned()) + .collect(); + assert_eq!(members.len(), 4, "2x2 fast×slow stage1-r grid = 4 member dirs; got {members:?}"); + for m in &members { + assert!( + m.chars().all(|c| c.is_ascii_alphanumeric() || matches!(c, '.' | '_' | '-')), + "non-portable member dir name: {m}" + ); + assert!( + base.join(m).join("r_equity.json").exists(), + "member {m} must carry the r_equity tap" + ); + } + // one member is chartable via chart / --tap r_equity. + let one = &members[0]; + let chart = Command::new(BIN) + .args(["chart", &format!("t1/{one}"), "--tap", "r_equity"]) + .current_dir(&cwd) + .output() + .expect("spawn aura chart t1/ --tap r_equity"); + assert!(chart.status.success(), "chart exit: {:?}", chart.status); + assert!( + String::from_utf8_lossy(&chart.stdout).contains("r_equity"), + "chart must render the r_equity series" + ); + // --name records the rankable family (no per-member streams) — still succeeds. + let named = Command::new(BIN) + .args(["sweep", "--strategy", "stage1-r", "--name", "n1"]) + .current_dir(&cwd) + .output() + .expect("spawn aura sweep --strategy stage1-r --name"); + assert!( + named.status.success(), + "stage1-r --name must succeed: {:?}; stderr: {}", + named.status, + String::from_utf8_lossy(&named.stderr) + ); + let _ = std::fs::remove_dir_all(&cwd); +} +``` + +- [ ] **Step 2: Run to verify it fails** + +Run: `cargo test -p aura-cli --test cli_run sweep_strategy_stage1_r_trace_persists` +Expected: FAIL — `--trace` is still refused (exit 2), so `traced.status.success()` +is false. + +- [ ] **Step 3: Make `stage1_r_sweep_family` honour `trace`** + +In `crates/aura-cli/src/main.rs`, change the signature (`_trace` → `trace`, ~1116) +and capture the binder + varying axes + drain `req_rows` + key + persist, mirroring +`momentum_sweep_family`. Replace the `bp.axis(...).axis(...).sweep(...)` body +(~1129-1158) with: + +```rust + let binder = bp.axis("fast.length", [2, 3]).axis("slow.length", [6, 12]); + let varying: HashSet = binder.varying_axes().into_iter().collect(); + binder + .sweep(|point| { + let (tx_eq, rx_eq) = mpsc::channel(); + let (tx_ex, rx_ex) = mpsc::channel(); + let (tx_r, rx_r) = mpsc::channel(); + let (tx_req, rx_req) = mpsc::channel(); + let mut h = stage1_r_graph(tx_eq, tx_ex, tx_r, tx_req, None, None) + .bootstrap_with_cells(point) + .expect("stage1-r grid points are kind-checked against param_space"); + h.run(data.run_sources()); + let eq_rows: Vec<(Timestamp, Vec)> = rx_eq.try_iter().collect(); + let ex_rows: Vec<(Timestamp, Vec)> = rx_ex.try_iter().collect(); + let r_rows: Vec<(Timestamp, Vec)> = rx_r.try_iter().collect(); + let req_rows: Vec<(Timestamp, Vec)> = rx_req.try_iter().collect(); + let mut named = zip_params(&space, point); + named.push(("bias_scale".to_string(), Scalar::f64(0.5))); + named.push(("stop_length".to_string(), Scalar::i64(STAGE1_R_STOP_LENGTH))); + named.push(("stop_k".to_string(), Scalar::f64(STAGE1_R_STOP_K))); + let key = member_key(&named, &varying); + let mut manifest = sim_optimal_manifest(named, window, 0, pip); + manifest.broker = stage1_r_broker_label(pip); + if let Some(name) = trace { + persist_traces_r(&format!("{name}/{key}"), &manifest, &eq_rows, &ex_rows, &req_rows); + } + let mut metrics = summarize(&f64_field(&eq_rows, 0), &f64_field(&ex_rows, 0)); + metrics.r = Some(summarize_r(&r_rows, 0.0)); + RunReport { manifest, metrics } + }) + .expect("the stage1-r named grid matches the stage1-r param-space") +``` + +Change the signature line (~1116): + +```rust +fn stage1_r_sweep_family(trace: Option<&str>, data: &DataSource) -> SweepFamily { +``` + +Replace the stale tail of the `stage1_r_sweep_family` doc-comment (~1112-1115, +from "Per-member tracing is a follow-up:") with: + +```rust +/// summarize_r, so its RunReport carries `r: Some(..)` and the family is rankable +/// by sqn / sqn_normalized / expectancy_r / net_expectancy_r. With `--trace`, each +/// member's equity / exposure / r_equity streams are persisted under +/// `runs/traces///` via `persist_traces_r` (mirroring +/// `momentum_sweep_family`), so a swept member is chartable. +``` + +- [ ] **Step 4: Drop the `run_sweep` refusal guard + update its doc** + +Delete the refusal-guard block in `run_sweep` (~1287-1296, the comment plus the +`if persist && strategy == Strategy::Stage1R { ... std::process::exit(2); }` +block). The `ensure_name_free` check below it and the +`Strategy::Stage1R => stage1_r_sweep_family(persist.then_some(name), &data)` +dispatch stay unchanged (the persist flag now flows through). + +Replace the stale `run_sweep` doc tail (~1281-1285) with: + +```rust +/// carrying the assigned id. With `--trace`, every strategy +/// (`sma`/`momentum`/`stage1-r`) persists each member's streams under +/// `runs/traces///` (opt-in); the `stage1-r` member also carries +/// the `r_equity` tap (via `persist_traces_r`). +``` + +- [ ] **Step 5: Run to verify it passes** + +Run: `cargo test -p aura-cli --test cli_run sweep_strategy_stage1_r_trace_persists` +Expected: PASS — 4 member dirs written under `runs/traces/t1/`, each with +`r_equity.json`, the member charts, and `--name` still succeeds. + +--- + +### Task 5: Full-workspace verification + +**Files:** none (verification only). + +> Must run LAST — after Task 3 re-pins the golden and Task 4 lands Part B. + +- [ ] **Step 1: Build the whole workspace** + +Run: `cargo build --workspace` +Expected: 0 errors. + +- [ ] **Step 2: Run the full test suite (unfiltered)** + +Run: `cargo test --workspace` +Expected: PASS — 0 failed. The pass count is the cycle-0066 baseline plus the net +new tests (two engine, one registry; Part B inverts an existing test in place). + +- [ ] **Step 3: Lint** + +Run: `cargo clippy --workspace --all-targets -- -D warnings` +Expected: 0 warnings.