diff --git a/docs/superpowers/plans/2026-05-09-sp19-20-wr-first.md b/docs/superpowers/plans/2026-05-09-sp19-20-wr-first.md index 314f485ca..f983639d4 100644 --- a/docs/superpowers/plans/2026-05-09-sp19-20-wr-first.md +++ b/docs/superpowers/plans/2026-05-09-sp19-20-wr-first.md @@ -1919,3 +1919,171 @@ Two execution options: **2. Inline Execution** — execute tasks in this session using `superpowers:executing-plans`, batch execution with checkpoints for human review. Best if you want to stay close to the implementation as it lands. Which approach? + +--- + +## Plan Accuracy Errata (appended 2026-05-09) + +The following deviations from the original Phase 2 plan are documented +here for future implementers. Each entry captures the gap, the +decision made, and the rationale. The audit-doc entries +(`docs/dqn-wire-up-audit.md`) carry the implementation-level details; +this errata is the plan-level "what was actually built vs what was +specified". + +### Gap 1: Task 2.0 (label-at-open infrastructure) was NEW, not in plan + +The original plan jumped from Phase 1.4 (Path C aggregation) to Task +2.1 (`sp20_compute_event_reward` device function) and then Task 2.2 +(replace SP12 v3 block in experience_kernels.cu). Task 2.2's call site +needed `label_at_open_per_env[i]` as input to `sp20_compute_event_ +reward(..., label_at_open_sign, ...)`, but no buffer / kernel arg / write +site was specified. + +**Decision**: insert a new Task 2.0 BEFORE Task 2.1 to land the buffer ++ alloc + reset registry + kernel arg + write site at `entering_trade` +in a single atomic commit. Producer-only this commit; the consumer +ships in Task 2.2 atomically with the SP12 v3 reward block deletion. + +**Rationale**: per `feedback_no_partial_refactor`, the contract change +("trade-close reward needs trade-open label") needs every consumer to +migrate atomically. Task 2.2 IS the consumer migration; Task 2.0 ships +the producer infrastructure first so the consumer has a buffer to read +from. The split is defensible because Task 2.0's buffer is +independently FoldReset-tested + the kernel arg pair is NULL-tolerant +(test scaffolds without aux-head wiring continue to compile and pass). + +Sign convention mapping is non-trivial: the upstream +`aux_sign_label_per_step_kernel.cu:77` emits `{0, 1, -1}` for +`{not-up, up, skip-window}`, but the SP20 spec §4.1 uses the +`{-1, 0, +1}` sign convention for `sign(SP19_blended_label)`. The Task +2.0 producer site does the mapping `1 → +1`, `0 → -1`, `-1 → 0`. + +### Gap 2: Per-env alpha plumbing was NOT in Task 2.2 scope + +The plan's Task 2.2 description specified replacing the SP12 v3 reward +block with `sp20_compute_event_reward`, but did NOT specify how the +resulting `alpha = R_event - hold_baseline` would feed +`sp20_aggregate_inputs_kernel`'s `EmaInputs.alpha` field (which Phase +1.4 emitted as a hardcoded `0.0f` placeholder). + +**Decision**: build the `alpha_per_env[N]` plumbing IN Task 2.2 (NOT +defer to Phase 4). Phase 4's n-step distributor is about per-bar +credit decay over time; per-env alpha aggregation is orthogonal and +belongs with the producer that emits alpha. Task 2.2 atomically: + - adds `alpha_per_env: CudaSlice` to the collector; + - threads it as a kernel-arg output of `experience_env_step`; + - adds it as a kernel-arg input of `sp20_aggregate_inputs_kernel`; + - adds the 5th shmem stripe + tree-reduce + close-gated mean + computation in the aggregation kernel; + - updates the launcher signature in `sp20_aggregate_inputs.rs`; + - updates the 2 placeholder pin tests + (`sp20_aggregate_inputs_test::alpha_and_per_bar_hold_reward_are_ + phase_2_and_3_2_placeholders` → + `null_alpha_producer_keeps_phase_1_4_placeholder_contract`; + `sp20_phase1_4_wireup_test::alpha_and_hold_reward_emas_stay_at_ + zero_phase_2_3_2_placeholders` → `..._with_null_producers`). + +**Rationale**: per `feedback_no_partial_refactor`, "when a contract +changes, every consumer migrates atomically" — the contract IS that +the aggregation kernel's `alpha` field is now load-bearing (was a +forward-reference placeholder). All 5 modifications above migrate that +contract in one commit. `hold_baseline` stays at `0.0f` until Phase +3.2's Component 2 lands the real producer, which is a SEPARATE +contract (the Hold opp-cost dual emission). + +### Gap 3: Per-bar SP18 D-leg sites — KEEP for Phase 2 + +The plan's Task 2.2 said "Replace SP18 D-leg + SP12 v3 reward block in +experience_kernels.cu" and "DELETE `compute_sp18_hold_opportunity_cost` +and `compute_sp12_reward_with_cost` from the file" without +distinguishing the trade-close site from the per-bar sites. + +**Decision**: per `feedback_no_stubs.md` ("wire it for real or delete; +no return-zero stubs, no dead params"), do NOT delete a working signal +until the replacement lands. Component 2 (Phase 3.2) owns per-bar Hold +opportunity-cost replacement. Phase 2 deletes ONLY the trade-close +site call to `compute_sp18_hold_opportunity_cost`. The function +definition in `trade_physics.cuh:655` is RETAINED — still called by 2 +sites at `experience_kernels.cu:3722` and `:3833` (per-bar D-leg). + +`compute_asymmetric_capped_pnl` and `compute_min_hold_penalty` device +functions in `trade_physics.cuh` are also RETAINED — both are still +called by `compute_sp18_hold_opportunity_cost` (line 680) AND by +`sp12_reward_math_test_kernel.cu` (the SP12 v3 oracle test surface). + +The plan's wording "DELETE [these helpers]" was overbroad. Errata +clarifies: only the trade-close-site call is deleted; the helpers +themselves stay. The phantom `compute_sp12_reward_with_cost` mentioned +in the plan does NOT exist as a function — the SP12 v3 reward is the +inlined block at experience_kernels.cu:3216-3380 (pre-Task-2.2 line +numbers). That inlined block is what gets replaced. + +### Gap 4: `sp20_compute_event_reward` placement — `sp20_reward.cuh` header + +The plan specified placing the device function "in +`experience_kernels.cu`". The post-review revision said "place it near +the other SP20 helpers in that file, not in trade_physics.cuh". + +**Decision**: put it in a new dedicated header `sp20_reward.cuh` +included by both `experience_kernels.cu` (Task 2.2 caller) and the +test wrapper `sp20_event_reward_test_kernel.cu` (Task 2.1 GPU oracle +tests). + +**Rationale**: `feedback_no_cpu_test_fallbacks` requires GPU oracle +tests share the production function bit-for-bit. If the function lived +inside `experience_kernels.cu` only, the test wrapper couldn't reach +it without code duplication (rejected by `feedback_no_quickfixes`). +Putting it in `trade_physics.cuh` was rejected by the reviewer because +that file is for trade physics, not reward math. A dedicated +`sp20_reward.cuh` is the cleanest fit and mirrors the pattern of +`compute_asymmetric_capped_pnl` / `compute_min_hold_penalty` / +`compute_lump_sum_opp_cost` in `trade_physics.cuh` — header-only +`__device__ __forceinline__` functions shared between production and +test kernel. + +### Gap 5: Task 2.3 was subsumed by Task 2.2 + +The plan listed Task 2.3 separately ("Wire wr_ema and alpha_ema +producer-call into trade-close path"). The aggregation kernel + EMA +kernel chain was already wired in Phase 1.4; Task 2.2's +`alpha_per_env` plumbing is what actually makes `ISV[ALPHA_EMA_INDEX]` +non-zero post-trade-close. There is no additional `sp20_emas_compute` +producer call to wire — the existing Path C chain consumes the new +`alpha` field automatically. + +**Decision**: Task 2.3 is subsumed by Task 2.2's atomic commit; no +separate Task 2.3 commit is needed. + +### Gap 6: `min_hold_*` kernel-arg trio cleanup deferred to Task 2.4 + +After deleting the SP12 v3 inlined reward block at the trade-close +site, three kernel args of `experience_env_step` (`min_hold_target`, +`min_hold_penalty_max`, `min_hold_temperature`) became dead. The plan +did NOT address the cleanup of these args or their upstream producer +chain (`min_hold_temperature_update_kernel`, +`ISV[MIN_HOLD_TEMPERATURE_ADAPTIVE_INDEX=460]`, +`read_min_hold_temperature_from_isv` helper, `config.min_hold_*` +fields). + +**Decision**: per `feedback_no_hiding`, DELETE the 3 kernel args (+ +their `.arg(...)` calls at the launch site) atomically with the +trade-close-site replacement in Task 2.2. DEFER the upstream producer +chain cleanup to a follow-up commit ("Task 2.4: remove orphaned SP12 +v3 min_hold_* producer chain"). + +**Rationale**: the kernel-arg removal is mechanical and tied directly +to the kernel-body change (same site). The producer-chain removal +touches the SP14 ISV slot registry + StateResetRegistry + an ISV +layout fingerprint bump and is independently testable. Per +`feedback_no_partial_refactor` "consumer migrates with contract +change", the consumer IS what migrated in Task 2.2 — the producer can +keep emitting into ISV[460] in the meantime as a documented orphan +without breaking any contract. Task 2.4 is tracked in the audit-doc +Task 2.2 entry under "Deleted: the `min_hold_*` kernel-arg trio". + +--- + +End of Phase 2 errata. Phase 3 / 3.2 / 4 / 5 / 6 / 7 of the plan +remain as specified; future tasks may add additional errata sections +as deviations are made during implementation.