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