gitadmin/foxhunt
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

docs(sp20): plan accuracy errata for Phase 2 (Tasks 2.0/2.1/2.2/2.3/2.4)

Browse Source 
Append a "Plan Accuracy Errata" section to the SP19+20 plan
documenting the 6 deviations from the original Phase 2 plan that
emerged during implementation. Each entry captures the gap, the
decision made, and the rationale, so future implementers see what
was actually built vs what was specified.

Gaps documented:

  1. Task 2.0 (label-at-open infrastructure) was NEW — split out from
     Task 2.2 to land buffer + alloc + reset + write site atomically
     before Task 2.2's consumer ships. Sign convention mapping detail
     captured (kernel emits {0, 1, -1}, SP20 spec uses {-1, 0, +1}).

  2. Per-env alpha plumbing was not in Task 2.2 plan scope — built in
     Task 2.2 (NOT deferred to Phase 4) to preserve the
     `feedback_no_partial_refactor` contract atomicity. Touches 5
     files in one commit.

  3. Per-bar SP18 D-leg sites — KEEP for Phase 2 per `feedback_no_stubs`.
     Plan's wording "DELETE [these helpers]" was overbroad; only the
     trade-close-site call is deleted. The phantom
     `compute_sp12_reward_with_cost` doesn't exist as a function (the
     SP12 v3 reward is the inlined block).

  4. `sp20_compute_event_reward` placement — new dedicated
     `sp20_reward.cuh` header (NOT inside `experience_kernels.cu`,
     NOT inside `trade_physics.cuh`). Mirrors the
     compute_asymmetric_capped_pnl / compute_min_hold_penalty
     header-only pattern; needed for GPU oracle test wrapper to share
     the function bit-for-bit per `feedback_no_cpu_test_fallbacks`.

  5. Task 2.3 was subsumed by Task 2.2 — the existing Path C chain
     consumes the new `alpha` field automatically once `alpha_per_env`
     is wired; no separate `sp20_emas_compute` producer call needed.

  6. `min_hold_*` kernel-arg trio cleanup deferred to Task 2.4 — the 3
     kernel args were deleted in Task 2.2 (per `feedback_no_hiding`)
     but the upstream producer chain (`min_hold_temperature_update_kernel`,
     ISV[460], `read_min_hold_temperature_from_isv`, `config.min_hold_*`)
     deferred to Task 2.4 because it touches SP14 ISV slot registry +
     StateResetRegistry + ISV layout fingerprint bump.

Implementation-level details remain in `docs/dqn-wire-up-audit.md`
Task 2.0 / 2.1 / 2.2 entries; this errata is the plan-level
"what was actually built vs what was specified".

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This commit is contained in:
jgrusewski
2026-05-10 00:30:33 +02:00
parent 3f9b34030c
commit e1aef53373
1 changed files with 168 additions and 0 deletions
Download Patch File Download Diff File Expand all files Collapse all files

168
docs/superpowers/plans/2026-05-09-sp19-20-wr-first.md
View File

@@ -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<f32>` 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.