Warrior_EA/REFACTOR_NOTES.md

# Refactor + audit — branch `refactor/solid-dry`

Baseline before any change: **0 errors, 0 warnings** (commit `4f28165`).
Every commit on this branch compiles 0/0 independently and was verified before
being committed.

Compiling from `C:\Users\admin\Documents\Workspaces\Warrior_EA` still fails with
`error 313: invalid resource path 'Network.cl'`; the build is done by copying the
tree into `MQL5\Experts\` first, per the documented workaround.

---

## A. Real bugs found and fixed

### A1. Non-atomic sidecar writes — `.stats`, `.arrows`, `.cfg`

`CNet::Save` staged the `.nnw` through a temp file + rename because
`FileOpen(FILE_WRITE)` **truncates on open**. The three sidecars written beside it
did not get that treatment.

1. **An interrupted write published a truncated sidecar.** Worst case is `.cfg`:
   `LoadAndCompareTopologyConfiguration()` reads a short file as a mismatch, which
   discards the trained model and restarts from era 0.
2. **The reader-side share flags were inert.** Windows sharing is a mutual
   contract — a writer opened with no `FILE_SHARE_*` blocks every concurrent open
   regardless of the reader's flags. All three read paths carry
   `FILE_SHARE_READ|FILE_SHARE_WRITE` precisely so a tester agent can read them
   while a live chart runs; an exclusive writer on the same path defeated that.

Fixed by extracting `CNet::Save`'s pattern into `System\AtomicFile.mqh`
(`AtomicWriteBegin` / `AtomicWriteEnd`) and routing all four writers through it.
That also encodes the `FileMove` trap **once**: the destination location comes
from `FILE_COMMON` inside the 4th argument and is *not* inherited from the
source — getting it wrong silently moves the file to the wrong sandbox.

### A2. Handle leak on every `.cfg` write-error path

`SaveTopologyConfiguration` had 13 copy-pasted 6-line error blocks that each
`return false` **without `FileClose(handle)`**. Collapsed into one ok-chain that
closes exactly once. The on-disk field order and types are unchanged — asserted
mechanically during the rewrite — so existing `.cfg` files still load.

### A3. `SaveChartSignals` documented a guarantee it did not provide

The comment said pruning runs only after a successful write ("a failed write
above leaves both the file AND the chart untouched"), but no write result was
ever checked, so a partial write still deleted the chart objects and lost those
arrows in both places. Results are checked now.

### A4. Topology drift between LSTM and HYBRID

`CSignalHYBRID` guarded the LSTM step with `MathMax(1, historyBars/2)`;
`CSignalLSTM` divided unguarded. A `historyBars` of 1 gave two different steps
for what the comments describe as the same layer. Unified on the guarded form.

### A5. Descriptor leak in topology construction

On a failed `topology.Add()` the `CLayerDescription` was neither owned by the
array nor deleted. Fixed in the extracted stages.

---

## B. Dead code removed

- **`CNet::SaveCheckpoint` / `CNet::LoadCheckpoint`** (123 lines). Superseded by
  the in-memory `CaptureWeights`/`RestoreWeights` pair; `Network.mqh:1312`
  already said so. Zero call sites — every remaining mention was a comment. The
  five comments that referenced them were reworded rather than left dangling.
- **`CExpertSignalCustom::CheckForDuplicateTrade` / `FindLastTradeIndex` /
  `UpdateTradeStatusAndExit`** — declared, never defined anywhere, never called.
  They only made it look as though duplicate-trade detection existed.

### Not removed, deliberately

- `TCIndexOk` and friends in `System\TradeChecks.mqh` are an intentional guard
  library, documented against MQL5 runtime error codes. Unused entries are API.
- `getPrevOutIndex` is one member of a symmetric `get*Index()` accessor family.
- **Stale worktree** `.claude/worktrees/agent-abfbb5952bdcac100` + branch
  `worktree-agent-abfbb5952bdcac100`: fully merged into `main`, zero unique
  commits. Removal was blocked by the permission classifier (forced delete), so
  it is left for you:
  ```
  git worktree remove .claude/worktrees/agent-abfbb5952bdcac100 --force
  git branch -D worktree-agent-abfbb5952bdcac100
  ```

### Checked and clean

A scan for `new`-into-local with early returns flagged 17 candidates; all were
inspected and all are correct — every failure path in the neuron-construction
switch deletes both the neuron and the partially built layer. Allocation
discipline in this codebase is good. Likewise, **every**
`FileOpen(...FILE_READ...)` already carried the required share flags: zero
violations of that project rule.

---

## C. Structure — `CExpertSignalAIBase` split by responsibility

The class was 8216 lines: declaration plus 87 method bodies covering training,
labelling, features, persistence, chart drawing, online learning, the GA tuner
and inference, all in one file. `Train()` alone is 1492 lines.

Bodies moved to `Expert\AIBase\`, included at the bottom of the original after
the class declaration:

| file | lines | holds |
|---|---:|---|
| `Training.mqh` | 1607 | era loop, plateau ladder, checkpoint select, deploy |
| `Features.mqh` | 1093 | indicator creation + per-bar input feature vector |
| `ChartUI.mqh` | 634 | arrows, arrow persistence, status panel, cleanup |
| `Persistence.mqh` | 492 | `.stats`/`.cfg` sidecars, CPU-inference validation, copy |
| `OnlineLearning.mqh` | 461 | live continual learning, EMA shadow, OOS simulator |
| `Labels.mqh` | 309 | ZigZag pivot labels, async label-cache prebuild |
| `AutoTune.mqh` | 275 | genetic tuner (population, crossover, halving) |
| `Inference.mqh` | 235 | softmax, prior calibration, class priors |
| `ExpertSignalAIBase.mqh` | **8216 → 3131** | declaration + topology build |

**This was verified to be a pure relocation, mechanically rather than by eye:**
HEAD's file reconstructed from the eight partials plus the surviving remainder
is byte-identical to HEAD, span for span. No declaration moved, no signature
changed, no code rewritten — behaviour is unchanged by construction.

### Topologies are now compositions

`AddConvPoolStage()` / `AddLstmStage()` on the base class; the three overrides
became:

```
CONV   = AddConvPoolStage
LSTM   = AddLstmStage
HYBRID = AddConvPoolStage && AddLstmStage
```

HYBRID's "matches the standalone CONV front-end exactly, then adds LSTM" is now
enforced by construction instead of by comment — which is what let A4 drift in
the first place.

---

## D. The MLP-slowness investigation — correcting what I told you

I previously said MLP is slowest because its first dense layer is the largest
weight matrix. **The arithmetic does not support that as an explanation for a
10× gap.** Parameter counts at the shipping defaults (`ind_Periods=20`,
21 features → input 420, `InitialNeurons=500`, `RF_70`, `MinNeurons=20`,
`ConvFilters=16`, `LstmHidden=32`, pool 3/2):

| topology | first dense | total weights |
|---|---|---:|
| MLP 3 dense | 420→500 = 210,500 | **~292,600** |
| MLP 4 dense | 420→500 = 210,500 | **~293,400** |
| CONV 2 dense | 160→500 = 80,000 | ~156,000 |
| LSTM 2 dense | 33→500 = 16,500 | ~150,100 |

1. **MLP_3L and MLP_4L are within 0.3% of each other.** The 4th dense layer is
   45→20 (900 weights) and it *shrinks* the output layer's fan-in. Nothing about
   topology can make 3L 10× slower than 4L.
2. **MLP vs CONV/LSTM is ~2×, not 10×.**

Your two observations together (OpenCL: all similar; CPU DLL: MLP 10× slower)
point at **CPU DLL dispatch, not network shape**. On OpenCL these matrices are
far too small to saturate the device, so every topology is dispatch-latency-bound
and *should* look identical — which is exactly what you see. The CPU tier is the
only one where real per-weight throughput shows, and there the gap is ~5× larger
than the weights justify.

**Not diagnosed. This needs profiling, not more static reading.** Concrete next
step: log wall-clock per `feedForward`/`backProp` call *per layer index* on the
CPU tier, MLP_3L vs MLP_4L. If one layer transition dominates, it is a
dispatch/threading bug in `WarriorCPU.cpp`. If cost is spread evenly, suspect
thread-pool oversubscription — `SetCpuLoadPercent` is applied **per network**,
and a run builds several (main + EMA shadow + sim/self-check clones).

One relevant note found while checking: `AIType` selects exactly one
architecture (`EnablePAI/CONV/LSTM/HYBRID` are mutually exclusive, derived from
it at `Warrior_EA.mq5:805-808`), so a four-topology comparison is four EA
instances, each with its own CPU-DLL thread pool on the same box.

---

## E. Still to do (not done here)

- Rebuild both DLLs (`build.bat` / `build_cpu.bat`). The `.cpp` sign-flip removal
  from the previous session requires it, and that rebuild also picks up the
  uncommitted D3D12 barrier fix that clears the DirectML LSTM `feedForward`
  errors.
- Redeploy the `.ex5` and retrain.
- The MLP CPU-DLL profiling in §D.

---

# 2026-07-29 (later): root cause of the collapse, and the architectural gap

## F. A `.nnw` pins the ARCHITECTURE, not just the weights — fixed in `1aa7df9`

`CNeuronBaseOCL::Save` writes `(int)activation` per neuron; `Load` reads it
straight back. So the activation chosen in `BuildFreshTopology()` only ever
reached a **brand-new** topology — every reload restored the file's value and
the next save wrote it back out. A wrong value could never heal on its own,
while the source read as though it were already fixed.

That is how five models kept training with an unbounded `NONE` classification
head for a full day after the 07-28 revert to `SIGMOID`. Confirmed by parsing
the binaries directly (parser recipe is in the memory note):

```
848cb42c.nnw  era=5  layer 4: BaseOCL act=NONE     out=3   <- was training
2e754b43.nnw  era=5  layer 5: BaseOCL act=NONE     out=3   <- was training
9d16f2c6.nnw  era=0  layer 4: BaseOCL act=SIGMOID  out=3   <- genuinely reset
```

In the log it showed as **negative** `OOS raw out` values — impossible under a
sigmoid — escalating to a `4.14e13` logit spread with all three classes
numerically identical (input-independent output) and balanced accuracy pinned
on the 33.3% one-class floor.

**Second half of the trap:** reset-weights only cleans the *current* fingerprint.
Changing any fingerprinted input moves the filename, and the EA then adopts
whatever stale `.nnw` already sits at the new name — resurrecting an obsolete
architecture the reset had just eliminated. Reset **after** an input change,
never before.

Fix: `OutputLayerActivation()` is now the single source of truth, called by both
`BuildFreshTopology()` and a new load-time repair that re-asserts it and logs
loudly when the file disagreed.

## G. Batch normalization — `30206cb`, `65dc1bd`

Audit against `references/MQL5/Experts/NeuroNet_DNG/NeuroNet.mqh` and
`references/neuronetworksbook.pdf` (extract with `pdftotext -layout`) found two
genuine gaps, and they turn out to be the same gap:

1. **No inter-layer normalization** (book ch. 6.1, reference class
   `CNeuronBatchNormOCL`). Inputs are ATR-normalized thoroughly, but every
   hidden stage is PRELU — the sigmoid head was the *only* bounded stage in the
   forward path. The observed collapse ordered exactly by **depth**, which is
   the signature of internal covariate shift.
2. **No dedicated SoftMax layer.** The reference runs
   `Dense(None) -> CNeuronSoftMaxOCL -> CCE` with the full Jacobian; this engine
   runs `Dense(SIGMOID) -> x6 -> softmax` with the gradient short-circuited to
   `(target - softmax)`. Defensible as an anti-saturation heuristic, but it caps
   expressible logits at [0,6] and forces saturation to reach confidence.

They are linked: the 07-27 attempt to unbound the head failed *because* nothing
upstream constrained scale. Batch norm before the head is the precondition for
ever retrying that (and would also need `CLASS_LOGIT_SCALE` -> 1.0 and
`BIAS_MAGNITUDE` -> ~0.5).

Implemented as `AI/NeuronBatchNorm.mqh`, host-side rather than a fourth copy of
a kernel across `Network.cl` + `WarriorCPU.cpp` + `WarriorDML.cpp` — the math is
elementwise O(n), so this way all four tiers behave identically, no DLL rebuild
is needed, and the backends cannot drift apart. Statistics are exponential
moving (training is pure online SGD; there is no mini-batch). `gamma`/`beta` are
excluded from weight decay.

Verified without a live run: analytic gradients match finite differences to
1.5e-7 relative over 200 random cases, and a faithful Python port of the whole
forward/backward chain collapses to the 33.3% floor by era 4 **without** the
layer and holds 36-43% **with** it.

Do **not** port the reference's batch-norm kernels verbatim if that is ever
revisited: `UpdateBatchOptionsAdam` reads the momentum slots (`+5..6`) where it
needs the second-momentum slots (`+7..8`), and carries the same sign-agreement
gate this project already removed as a downward ratchet.

---

# 2026-08-01: audit verification + the remaining god files

Run against the brief "remove dead code, shorten/refresh comments, enforce
SOLID/DRY, break down god files, favour efficiency, guards everywhere".
Both builds compile **0 errors, 0 warnings** (standard and
`WARRIOR_MARKET_BUILD`) after every step below.

## H. The multi-agent audit report was ~half fabricated

A generated "comprehensive audit" claiming cross-verified `file:line` findings
was checked claim by claim. Roughly ten findings, **including two P0 and two
P1**, describe code that does not exist:

| claimed | actual |
|---|---|
| `Signals/Signals.mqh` composite logic, hardcoded 0.25 average | 19 lines of `#include` |
| Divide-by-zero on `PipValue` in `MoneyFixedRisk.mqh` | empty subclass, no arithmetic |
| PReLU NaN guard needed in `NeuronPrimitives.mqh` | no activations there at all |
| LSTM kernels lack a gradient floor | `MIN_ACTIVATION_DERIVATIVE` already on every gate |
| `AtomicFile` retry loop, `NewsCache`, `m_confidenceHistory[]`, unchecked DirectML calls | none exist |

Two of the P1 items would have meant editing working BPTT to add something
already present. **Treat any such report as hypotheses.** Cheapest first filter:
pull line counts for every file it names.

## I. Guard audit — the real result

All 190 division sites outside `references/` were enumerated and inspected.
Coverage is genuinely good: ATR normalization, batch-norm `sd`, softmax,
risk-guard percentages, volume steps and Kelly `b` are all already guarded.
Two real gaps, both fixed:

- **`CMoneyRiskBase::CalculateLotSize`** divided by `loss` without checking it,
  while `CalculatePotentialLoss()` signals "no usable quote" by returning
  exactly `0.0`. Both current callers reject first, but the invariant now lives
  where the division is.
- **No non-finite detection at the inference boundary.** A NaN logit makes every
  comparison in `ApplyClassificationSoftmax()` false, so a corrupt model returns
  Neutral on every bar *forever* and is indistinguishable from a quiet one —
  the exact ambiguity this project has chased from the outside more than once.
  Now detected and reported (bounded to 3 lines; it cannot self-heal).

## J. God files broken down

Both splits are pure relocations of method bodies to files included **after**
the declarations. A body cannot run during compilation and every declaration it
could reference is already visible, so behaviour is unchanged by construction.
The `AI/Network.mqh` partition was verified mechanically: the multiset of lines
across the spine plus all ten modules is exactly HEAD's, with the only additions
being the new banners and `#include` lines.

| file | before | after |
|---|---:|---:|
| `AI/Network.mqh` | 6,266 | **1,105** |
| `Expert/ExpertSignalAIBase.mqh` | 4,026 | **2,176** |

`AI/Impl/` holds ten modules (largest 939 lines); `Expert/AIBase/` gained
`Lifecycle.mqh` and `Topology.mqh`.

The include chain inside `Network.mqh` is a **dependency graph, not a
preference** — each nested `#include` sits exactly where its base class becomes
visible. Do not reorder it.

## K. Dead code

The earlier sweeps had already taken the large items. A fresh whole-tree
reference count found only unused accessors, all removed:
`CNeuronBase::Optimization`/`SetOptimization`, the `CNeuronBaseOCL` pair,
`CNet::HasLogitAdjustment`, `CNet::HaveWeightSnapshot`,
`CExpertSignalAIBase::EraCount`, `BestBalancedAccuracy`.

Kept deliberately, as before: `TCIndexOk` and friends (a documented guard
library — unused entries are API), `getPrevOutIndex` and `WindowOut` (members of
documented symmetric accessor families).

## L. Comments

Compressed the tombstone blocks — comments describing code that no longer
exists — keeping every operative fact and dropping the incident retelling that
git and the memory notes already hold. Biggest: the 52-line block on
`desc.activation` (-32), the conv-stage history (-5), online-learning class
imbalance (-11), alternation gate (-7), NMS (-11), vote-gate census (-4).

Corrected as **factually stale**:

- Five sites describing `m_prevEraTrueBuyCount/Sell/Neutral` as feeding an
  "oversampling ratio" / "reps". Oversampling was removed 2026-07-31; those
  counters now feed `UpdateClassPriors()`. This is the same class of error that
  already sent one diagnosis down the wrong path.
- Online learning described as adapting to "the supervised ZigZag-pivot task it
  was trained on" — the target has been triple-barrier since 2026-08-01.
- `AI_NETWORK.md`: rewritten. It listed 3 activations (there are 4), 17 kernels
  (22), `MIN_ACTIVATION_DERIVATIVE = 1e-4` (1e-3), and recommended exactly the
  split performed in §J.
- `Warrior_EA_System_Overview.md`: claimed a **sign-agreement gate** on both
  optimizers (removed from all four backends 2026-07) and **dual-threaded
  training** with a background gradient thread (MQL5 is single-threaded; only
  the CPU-DLL tier is internally threaded).
- `Expert/README.md`: claimed "robust duplicate detection" in
  `CExpertSignalCustom` — those three methods were removed in 2026-07 precisely
  because they were declared but never defined, making the feature look real.

## M. Not done, deliberately

- `Expert/ExpertSignalAIBase.mqh` stays at 2,176 lines. What remains is one
  class declaration, which cannot be split further; its size is comment density,
  not structure.
- No efficiency changes. Nothing in the surveyed hot paths was measurably wrong,
  and the outstanding perf question (§D) still needs profiling, not reading.