# 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.