diff --git a/docs/plans/2026-02-23-production-safety-audit-implementation.md b/docs/plans/2026-02-23-production-safety-audit-implementation.md new file mode 100644 index 000000000..e23994663 --- /dev/null +++ b/docs/plans/2026-02-23-production-safety-audit-implementation.md @@ -0,0 +1,2444 @@ +# Production Safety Audit — Implementation Plan + +> **For Claude:** REQUIRED SUB-SKILL: Use superpowers:executing-plans to implement this plan task-by-task. + +**Goal:** Fix all 38 production safety issues identified by 4-domain code audit, organized in dependency order so each layer builds on a correct foundation. + +**Architecture:** 5-layer remediation — Layer 0 (Data Integrity) → Layer 1 (Risk Enforcement) → Layer 2 (ML Pipeline) → Layer 3 (Broker Safety) → Layer 4 (Auth & Ops). Each layer has a verification checkpoint. + +**Tech Stack:** Rust, Candle v0.9.1, Tokio, tonic/gRPC, Axum 0.7.9, sqlx, chrono-tz + +**Build:** `SQLX_OFFLINE=true cargo check --workspace` +**Test:** `SQLX_OFFLINE=true cargo test -p --lib` +**Clippy:** `SQLX_OFFLINE=true cargo clippy -p -- -D warnings` + +--- + +## Layer 0: Data Integrity Foundation (7 fixes) + +### Task 1: Fix get_orders status filter no-op (H1) + +**Files:** +- Modify: `trading_engine/src/trading/order_manager.rs:132-147` +- Test: `trading_engine/src/trading/order_manager.rs` (existing tests) or new test in same module + +**Context:** `matches!(order.status, _status)` creates an irrefutable pattern binding — `_status` is a new variable, not the captured `status`. Every order matches regardless of filter. + +**Step 1: Write the failing test** + +In `trading_engine/src/trading/order_manager.rs`, add a test (or in an existing test module) that verifies filtering: + +```rust +#[tokio::test] +async fn test_get_orders_status_filter_only_returns_matching() { + let manager = OrderManager::new(); + // Add orders with different statuses + let mut order1 = create_test_order("O1"); + order1.status = OrderStatus::Filled; + manager.add_order(order1).await; + + let mut order2 = create_test_order("O2"); + order2.status = OrderStatus::Created; + manager.add_order(order2).await; + + // Filter for Filled only + let filled = manager.get_orders(Some(OrderStatus::Filled)).await; + assert_eq!(filled.len(), 1, "Should only return filled orders"); + assert_eq!(filled[0].order_id, "O1"); + + // Filter for Created only + let created = manager.get_orders(Some(OrderStatus::Created)).await; + assert_eq!(created.len(), 1, "Should only return created orders"); + + // No filter returns all + let all = manager.get_orders(None).await; + assert_eq!(all.len(), 2); +} +``` + +**Step 2: Run test to verify it fails** + +Run: `SQLX_OFFLINE=true cargo test -p trading_engine --lib test_get_orders_status_filter` +Expected: FAIL — filter returns 2 orders instead of 1 + +**Step 3: Fix the filter** + +Replace lines 138-141 in `order_manager.rs`: +```rust +// BEFORE (broken): +if let Some(ref _status) = status_filter { + matches!(order.status, _status) +} + +// AFTER (correct): +if let Some(ref status) = status_filter { + order.status == *status +} +``` + +**Step 4: Run test to verify it passes** + +Run: `SQLX_OFFLINE=true cargo test -p trading_engine --lib test_get_orders_status_filter` +Expected: PASS + +**Step 5: Run full crate tests + clippy** + +Run: `SQLX_OFFLINE=true cargo test -p trading_engine --lib && SQLX_OFFLINE=true cargo clippy -p trading_engine -- -D warnings` +Expected: All 301 tests pass, 0 clippy warnings + +**Step 6: Commit** + +```bash +git add trading_engine/src/trading/order_manager.rs +git commit -m "fix(trading_engine): order status filter was irrefutable pattern binding + +matches!(order.status, _status) creates a new binding, always matches. +Replace with direct equality check." +``` + +--- + +### Task 2: Fix price_to_fixed negative price truncation (M6) + +**Files:** +- Modify: `services/trading_service/src/core/position_manager.rs:254-256` + +**Context:** `(price * 10000.0) as u64` casts negative f64 to u64, saturating to 0. Oil spreads or bad fill reports could produce negative prices, corrupting avg_price and all PnL. + +**Step 1: Write the failing test** + +```rust +#[test] +fn test_price_to_fixed_rejects_negative_and_nan() { + // Negative price should fail + assert!(AtomicPosition::price_to_fixed_checked(-1.5).is_err()); + // NaN should fail + assert!(AtomicPosition::price_to_fixed_checked(f64::NAN).is_err()); + // Infinity should fail + assert!(AtomicPosition::price_to_fixed_checked(f64::INFINITY).is_err()); + // Zero should fail (zero price is invalid for financial instruments) + assert!(AtomicPosition::price_to_fixed_checked(0.0).is_err()); + // Positive valid price should succeed + assert_eq!(AtomicPosition::price_to_fixed_checked(1.5).unwrap(), 15000); +} +``` + +**Step 2: Run test to verify it fails** + +Run: `SQLX_OFFLINE=true cargo test -p trading_service --lib test_price_to_fixed` +Expected: FAIL — method doesn't exist yet + +**Step 3: Implement checked version** + +Add new method and make `price_to_fixed` call it: + +```rust +fn price_to_fixed_checked(price: f64) -> Result { + if !price.is_finite() || price <= 0.0 { + return Err(PositionError::CalculationError { + message: format!("Invalid price for fixed-point conversion: {}", price), + }); + } + Ok((price * 10000.0) as u64) +} + +fn price_to_fixed(price: f64) -> u64 { + // Delegate to checked version, defaulting to 0 for backward compat in non-critical paths + Self::price_to_fixed_checked(price).unwrap_or(0) +} +``` + +Update `update_with_execution` to use `price_to_fixed_checked`: +```rust +let execution_price_fixed = Self::price_to_fixed_checked(execution_price)?; +``` + +**Step 4: Run test + full crate** + +Run: `SQLX_OFFLINE=true cargo test -p trading_service --lib` +Expected: PASS + +**Step 5: Commit** + +```bash +git add services/trading_service/src/core/position_manager.rs +git commit -m "fix(trading_service): reject negative/NaN prices in fixed-point conversion + +(price * 10000.0) as u64 saturates to 0 for negatives. Add validation." +``` + +--- + +### Task 3: Fix market data NaN/zero price validation (C13) + +**Files:** +- Modify: `services/trading_service/src/core/market_data_ingestion.rs:462-486` + +**Context:** Binary market data parsed to f64 without finiteness check. A zeroed packet produces `price=0.0`, broadcast as valid tick, causing 100% loss display on all positions. + +**Step 1: Write the failing test** + +```rust +#[test] +fn test_parse_binary_rejects_zero_price() { + let ingestion = create_test_ingestion(); + // Construct a 40-byte binary packet with price=0.0 + let mut data = vec![0u8; 40]; + // ... set header fields but leave price bytes as zeros + let result = ingestion.parse_binary_message(&data); + // Should be dropped (returns None or Err) + assert!(result.is_none() || result.is_err()); +} + +#[test] +fn test_parse_binary_rejects_nan_price() { + let ingestion = create_test_ingestion(); + let mut data = vec![0u8; 40]; + // Set price bytes to NaN + let nan_bytes = f64::NAN.to_le_bytes(); + data[24..32].copy_from_slice(&nan_bytes); + let result = ingestion.parse_binary_message(&data); + assert!(result.is_none() || result.is_err()); +} +``` + +**Step 2: Run test to verify it fails** + +Expected: FAIL — currently accepts any price value + +**Step 3: Add validation after price parsing** + +After line 465 (`let price = f64::from_le_bytes([...]);`), add: + +```rust +// Validate price and quantity are finite and non-negative +if !price.is_finite() || price <= 0.0 { + self.drop_count.fetch_add(1, Ordering::Relaxed); + warn!("Dropped tick with invalid price: {}", price); + return None; // or early return depending on function signature +} +``` + +Similarly validate quantity: +```rust +if !quantity.is_finite() || quantity < 0.0 { + self.drop_count.fetch_add(1, Ordering::Relaxed); + warn!("Dropped tick with invalid quantity: {}", quantity); + return None; +} +``` + +**Step 4: Run tests** + +Run: `SQLX_OFFLINE=true cargo test -p trading_service --lib` +Expected: PASS + +**Step 5: Commit** + +```bash +git add services/trading_service/src/core/market_data_ingestion.rs +git commit -m "fix(trading_service): validate market data prices are finite and positive + +Zero/NaN prices from binary packets would corrupt position PnL." +``` + +--- + +### Task 4: Fix clone_for_async independent counters + heartbeat init (M7 + C14 partial) + +**Files:** +- Modify: `services/trading_service/src/core/market_data_ingestion.rs` + +**Context:** `clone_for_async` creates new `AtomicU64` instances (not shared). Spawned task increments its own counters while `get_stats()` reads originals (always 0). Also `last_heartbeat` initializes to 0, causing immediate "connection dead" on first check. + +**Step 1: Write the failing test** + +```rust +#[tokio::test] +async fn test_clone_for_async_shares_counters() { + let ingestion = create_test_ingestion(); + let cloned = ingestion.clone_for_async(); + + // Increment on clone + cloned.message_count.fetch_add(1, Ordering::Relaxed); + + // Should be visible from original + assert_eq!(ingestion.message_count.load(Ordering::Relaxed), 1, + "message_count should be shared via Arc"); +} + +#[test] +fn test_heartbeat_initialized_to_now() { + let ingestion = create_test_ingestion(); + let hb = ingestion.last_heartbeat.load(Ordering::Relaxed); + assert!(hb > 0, "last_heartbeat should be initialized to current time, not 0"); +} +``` + +**Step 2: Run test to verify it fails** + +Expected: FAIL — counters are independent, heartbeat is 0 + +**Step 3: Implement shared counters** + +Change struct fields from `AtomicU64` to `Arc`: +```rust +// In struct definition: +message_count: Arc, +drop_count: Arc, +last_heartbeat: Arc, +reconnect_attempts: Arc, +``` + +In `new()`: +```rust +message_count: Arc::new(AtomicU64::new(0)), +drop_count: Arc::new(AtomicU64::new(0)), +last_heartbeat: Arc::new(AtomicU64::new(HardwareTimestamp::now().as_nanos())), +reconnect_attempts: Arc::new(AtomicU64::new(0)), +``` + +In `clone_for_async()`: +```rust +message_count: Arc::clone(&self.message_count), +drop_count: Arc::clone(&self.drop_count), +last_heartbeat: Arc::clone(&self.last_heartbeat), +reconnect_attempts: Arc::clone(&self.reconnect_attempts), +``` + +**Step 4: Fix all call sites that use `.load()`/`.fetch_add()` — they work identically through Arc** + +Run: `SQLX_OFFLINE=true cargo test -p trading_service --lib` +Expected: PASS + +**Step 5: Commit** + +```bash +git add services/trading_service/src/core/market_data_ingestion.rs +git commit -m "fix(trading_service): share atomic counters via Arc in clone_for_async + +Independent counters meant get_stats() always returned 0. +Also initialize last_heartbeat to now() instead of 0." +``` + +--- + +### Task 5: Fix account never deducting trade value (H2) + +**Files:** +- Modify: `trading_engine/src/trading/account_manager.rs:104-135` + +**Context:** `_execution_value = quantity * price` is computed but discarded. Only commission is deducted. Buying power is never reduced after buys → unlimited ordering possible. + +**Step 1: Write the failing test** + +```rust +#[tokio::test] +async fn test_update_from_execution_deducts_buy_value() { + let manager = AccountManager::new(); + manager.create_demo_account(100_000.0).await.unwrap(); + + let execution = ExecutionResult { + order_id: "E1".into(), + symbol: "AAPL".into(), + executed_quantity: Decimal::from(10), + execution_price: Decimal::from(150), + execution_time: Utc::now(), + commission: Decimal::from(5), + liquidity_flag: LiquidityFlag::Maker, + side: OrderSide::Buy, // New field (see Task 6) + }; + + manager.update_from_execution(&execution).await.unwrap(); + + let accounts = manager.accounts.read().await; + let account = accounts.get("DEMO_ACCOUNT").unwrap(); + // Should deduct: 10 * 150 + 5 commission = 1505 + assert!(account.cash_balance < Decimal::from(100_000)); + let expected = Decimal::from(100_000) - Decimal::from(1505); + assert_eq!(account.cash_balance, expected); +} +``` + +**Step 2: Run test to verify it fails** + +Expected: FAIL — cash_balance only reduced by commission (5), not the full 1505 + +**Step 3: Implement proper cash deduction** + +```rust +pub async fn update_from_execution(&self, execution: &ExecutionResult) -> Result<(), String> { + let mut accounts = self.accounts.write().await; + + let account = accounts + .get_mut("DEMO_ACCOUNT") + .ok_or("Demo account not found")?; + + let execution_value = execution.executed_quantity * execution.execution_price; + let commission = execution.commission; + + // Deduct/add based on order side + match execution.side { + OrderSide::Buy => { + account.cash_balance -= execution_value + commission; + }, + OrderSide::Sell => { + account.cash_balance += execution_value - commission; + }, + } + + // Recalculate total value (positions + cash) + // For now, update based on cash change + account.total_value = account.cash_balance; // Will be refined when position tracking is wired + + info!( + "Account updated: side={:?}, value={}, commission={}, new_cash={}", + execution.side, execution_value, commission, account.cash_balance + ); + + Ok(()) +} +``` + +**Step 4: Run tests** + +Run: `SQLX_OFFLINE=true cargo test -p trading_engine --lib` +Expected: PASS + +**Step 5: Commit** + +```bash +git add trading_engine/src/trading/account_manager.rs +git commit -m "fix(trading_engine): deduct execution value from account cash balance + +_execution_value was computed but discarded. Buying power was never reduced." +``` + +--- + +### Task 6: Fix position direction always buy (H4) + add side to ExecutionResult + +**Files:** +- Modify: `trading_engine/src/trading/trading_operations.rs:342-361` (add `side` field) +- Modify: `trading_engine/src/trading/position_manager.rs:64-67` +- Modify: all `ExecutionResult` construction sites + +**Context:** `is_buy = execution.executed_quantity > 0` but `executed_quantity` is always positive (it's a magnitude). Short positions never open through this path. + +**Step 1: Add `side` field to `ExecutionResult`** + +```rust +pub struct ExecutionResult { + pub order_id: OrderId, + pub symbol: String, + pub executed_quantity: Decimal, + pub execution_price: Decimal, + pub execution_time: DateTime, + pub commission: Decimal, + pub liquidity_flag: LiquidityFlag, + pub side: OrderSide, // NEW: determines buy vs sell +} +``` + +**Step 2: Fix position direction logic** + +```rust +// BEFORE: +let is_buy = execution.executed_quantity > Decimal::ZERO; + +// AFTER: +let is_buy = execution.side == OrderSide::Buy; +``` + +**Step 3: Fix all ExecutionResult construction sites to include `side`** + +Search codebase for `ExecutionResult {` and add `side: OrderSide::Buy` or pass through from order. Update test fixtures. + +**Step 4: Run tests** + +Run: `SQLX_OFFLINE=true cargo test -p trading_engine --lib` +Expected: PASS + +**Step 5: Commit** + +```bash +git add trading_engine/ +git commit -m "fix(trading_engine): add OrderSide to ExecutionResult, fix always-buy direction + +executed_quantity is always positive magnitude. Used side field from order." +``` + +--- + +### Task 7: Fix position update_with_execution race condition (C11) + +**Files:** +- Modify: `services/trading_service/src/core/position_manager.rs:316-394` + +**Context:** `update_position` drops the `positions` read-lock, gets Arc, then calls `update_with_execution` outside the lock. Two fills for the same position race: both load old quantity, both compute new quantity, last store wins. Worst case: one fill's effect is completely lost. + +**Step 1: Write the failing test** + +```rust +#[tokio::test] +async fn test_concurrent_fills_dont_lose_quantity() { + let pm = PositionManager::new(Default::default()); + pm.create_or_get_position("ACC1", "AAPL").await; + + // Spawn 10 concurrent fills of +1 each + let mut handles = Vec::new(); + for i in 0..10 { + let pm = pm.clone(); + handles.push(tokio::spawn(async move { + pm.update_position("ACC1", "AAPL", 1, 150.0).await + })); + } + + for h in handles { + h.await.unwrap().unwrap(); + } + + // All 10 fills must be accounted for + let positions = pm.positions.read().await; + let pos = positions.get("ACC1:AAPL").unwrap(); + assert_eq!(pos.quantity.load(Ordering::Acquire), 10, + "Concurrent fills must not lose quantity"); +} +``` + +**Step 2: Run test to verify it can fail (may need stress iteration)** + +Expected: Can fail intermittently — two fills read same old_quantity, both compute same new_quantity, one write is lost. + +**Step 3: Serialize fills per position** + +Option A (simplest): Hold the positions write-lock across the entire update: + +```rust +pub async fn update_position( + &self, + account_id: &str, + symbol: &str, + quantity_delta: i64, + execution_price: f64, +) -> Result { + let position_key = format!("{}:{}", account_id, symbol); + let timestamp_ns = HardwareTimestamp::now().as_nanos(); + let sequence = self.sequence_generator.next(); + + // Hold write lock for entire operation to prevent concurrent fill race + let mut positions = self.positions.write().await; + + let position = if let Some(pos) = positions.get(&position_key) { + Arc::clone(pos) + } else { + let symbol_hash = self.get_symbol_hash(symbol).await; + let account_hash = self.get_account_hash(account_id).await; + let new_position = Arc::new(AtomicPosition::new(symbol_hash, account_hash)); + positions.insert(position_key.clone(), Arc::clone(&new_position)); + self.position_count.fetch_add(1, Ordering::Relaxed); + new_position + }; + + // Still under write lock — no concurrent fill can interleave + let update_result = position.update_with_execution( + quantity_delta, + execution_price, + timestamp_ns, + sequence, + )?; + + Ok(update_result) +} +``` + +Option B (per-position lock): Add `Mutex` per position for finer granularity — more complex but better throughput. Start with Option A, profile later. + +Note: Remove the RDTSC latency tracking from this critical path initially to keep the fix clean. Re-add timing after correctness is verified. + +**Step 4: Run concurrent test + full suite** + +Run: `SQLX_OFFLINE=true cargo test -p trading_service --lib` +Expected: PASS (including concurrent test) + +**Step 5: Commit** + +```bash +git add services/trading_service/src/core/position_manager.rs +git commit -m "fix(trading_service): hold write lock across position update to prevent fill race + +Individual atomic loads/stores raced under concurrent fills. Last store +could discard another fill's quantity change." +``` + +--- + +### Layer 0 Checkpoint + +Run: `SQLX_OFFLINE=true cargo test -p trading_engine --lib && SQLX_OFFLINE=true cargo test -p trading_service --lib` +Expected: All tests pass. Data layer is now correct. + +--- + +## Layer 1: Risk Enforcement (10 fixes) + +### Task 8: Fix order validate/add race condition (C1) + +**Files:** +- Modify: `trading_engine/src/trading/engine.rs:103-113` +- Modify: `trading_engine/src/trading/order_manager.rs` (add `validate_and_add_order`) + +**Context:** `validate_order` and `add_order` are separate operations. Gap between them allows duplicate order IDs to both pass validation. + +**Step 1: Write the failing test** + +```rust +#[tokio::test] +async fn test_duplicate_order_id_rejected() { + let engine = create_test_engine(); + let order = create_test_order("DUP-1"); + + // First submission should succeed + engine.submit_order(order.clone()).await.unwrap(); + + // Second with same ID should fail + let result = engine.submit_order(order).await; + assert!(result.is_err(), "Duplicate order ID must be rejected"); +} +``` + +**Step 2: Combine validate and add into atomic operation** + +In `order_manager.rs`: +```rust +/// Validate and add order atomically (prevents duplicate race) +pub async fn validate_and_add_order(&self, order: TradingOrder) -> Result<(), String> { + let mut orders = self.orders.write().await; + + // Duplicate check under write lock + if orders.contains_key(&order.order_id) { + return Err(format!("Duplicate order ID: {}", order.order_id)); + } + + // Validate order fields + if order.quantity <= Decimal::ZERO { + return Err("Order quantity must be positive".to_string()); + } + + orders.insert(order.order_id.clone(), order); + Ok(()) +} +``` + +In `engine.rs`, replace lines 103-113: +```rust +// BEFORE: +self.order_manager.validate_order(&order).await?; +// ... other checks ... +self.order_manager.add_order(order.clone()).await; + +// AFTER: +self.account_manager.check_buying_power(&order).await?; +let order_id = self.trading_ops.submit_order(order.clone()).await?; +self.order_manager.validate_and_add_order(order.clone()).await?; +``` + +**Step 3: Run tests** + +Run: `SQLX_OFFLINE=true cargo test -p trading_engine --lib` + +**Step 4: Commit** + +```bash +git add trading_engine/src/trading/engine.rs trading_engine/src/trading/order_manager.rs +git commit -m "fix(trading_engine): atomic validate-and-add prevents duplicate order race" +``` + +--- + +### Task 9: Fix overfill protection (C2) + +**Files:** +- Modify: `trading_engine/src/trading/order_manager.rs:95-130` + +**Context:** `fill_quantity += execution.executed_quantity` with no cap. Duplicate execution report doubles fill and corrupts average price. + +**Step 1: Write the failing test** + +```rust +#[tokio::test] +async fn test_overfill_rejected() { + let manager = OrderManager::new(); + let mut order = create_test_order("O1"); + order.quantity = Decimal::from(100); + order.status = OrderStatus::Submitted; + manager.add_order(order).await; + + // Fill 80 + let exec1 = create_execution("O1", 80.0, 150.0); + manager.process_execution(&exec1).await.unwrap(); + + // Try to fill 30 more (total would be 110 > 100) + let exec2 = create_execution("O1", 30.0, 151.0); + let result = manager.process_execution(&exec2).await; + assert!(result.is_err(), "Overfill must be rejected"); +} + +#[tokio::test] +async fn test_fill_on_already_filled_order_rejected() { + let manager = OrderManager::new(); + let mut order = create_test_order("O1"); + order.quantity = Decimal::from(100); + order.status = OrderStatus::Filled; + order.fill_quantity = Decimal::from(100); + manager.add_order(order).await; + + let exec = create_execution("O1", 10.0, 150.0); + let result = manager.process_execution(&exec).await; + assert!(result.is_err(), "Fill on already-filled order must be rejected"); +} +``` + +**Step 2: Add overfill guard** + +In `process_execution`, before accumulating: +```rust +// Reject fills on already-filled orders +if order.status == OrderStatus::Filled { + return Err(format!("Order {} already fully filled", execution.order_id)); +} + +// Check for overfill +let new_fill = order.fill_quantity + execution.executed_quantity; +if new_fill > order.quantity { + return Err(format!( + "Overfill rejected: {} + {} = {} > order qty {}", + order.fill_quantity, execution.executed_quantity, new_fill, order.quantity + )); +} + +order.fill_quantity = new_fill; +``` + +**Step 3: Run tests** + +Run: `SQLX_OFFLINE=true cargo test -p trading_engine --lib` + +**Step 4: Commit** + +```bash +git add trading_engine/src/trading/order_manager.rs +git commit -m "fix(trading_engine): reject overfills and fills on completed orders + +Duplicate execution reports could double fill quantity and corrupt avg price." +``` + +--- + +### Task 10: Fix risk checks bypassing when broker_account_service is None (C9) + +**Files:** +- Modify: `risk/src/risk_engine.rs:1260-1263` (check_position_limits else branch) +- Modify: `risk/src/risk_engine.rs:1388` (check_leverage_limits missing else branch) + +**Context:** Both `check_position_limits` and `check_leverage_limits` skip all checks and return `Approved` when `broker_account_service` is `None`. This means running without a broker service = zero risk checks. + +**Step 1: Write the failing test** + +```rust +#[tokio::test] +async fn test_position_limits_reject_when_no_broker_service() { + let risk_engine = RiskEngine::new(config, None); // No broker service + let order = create_test_order_info(); + let result = risk_engine.check_position_limits(&order, "account1").await; + assert!(result.is_err(), "Should reject when broker service not configured"); +} +``` + +**Step 2: Replace else branches** + +In `check_position_limits` (line 1260-1263): +```rust +// BEFORE: +} else { + debug!("No broker service available for leverage check, approving by default"); +} +Ok(RiskCheckResult::Approved) + +// AFTER: +} else { + return Err(RiskError::Config( + "broker_account_service not configured — cannot verify position limits".to_owned(), + )); +} +Ok(RiskCheckResult::Approved) +``` + +Same pattern for `check_leverage_limits` (line 1388): add else branch: +```rust +} else { + return Err(RiskError::Config( + "broker_account_service not configured — cannot verify leverage limits".to_owned(), + )); +} +``` + +**Step 3: Run tests** + +Run: `SQLX_OFFLINE=true cargo test -p risk --lib` + +**Step 4: Commit** + +```bash +git add risk/src/risk_engine.rs +git commit -m "fix(risk): fail-safe when broker service not configured + +Position and leverage checks returned Approved by default. Now returns +config error to prevent unvalidated orders." +``` + +--- + +### Task 11: Fix check_order() hardcoded "default" account (C10) + +**Files:** +- Modify: `risk/src/risk_engine.rs:1885-1888` +- Modify: `risk/src/risk_types.rs:167-186` (add `account_id` to `OrderInfo`) + +**Context:** `check_order()` always passes `"default"` as account_id. Per-account risk state is never checked for real accounts. + +**Step 1: Add account_id field to OrderInfo** + +```rust +pub struct OrderInfo { + // ... existing fields ... + /// Account ID for per-account risk tracking + pub account_id: Option, +} +``` + +**Step 2: Use it in check_order** + +```rust +pub async fn check_order(&self, order_info: &OrderInfo) -> RiskResult { + let account_id = order_info.account_id.as_deref().unwrap_or("default"); + if account_id == "default" { + warn!("check_order called without explicit account_id, using 'default'"); + } + self.check_pre_trade_risk(order_info, account_id).await +} +``` + +**Step 3: Update all OrderInfo construction sites to include `account_id`** + +Search for `OrderInfo {` across codebase, add `account_id: Some("...".to_owned())` or `account_id: None`. + +**Step 4: Run tests** + +Run: `SQLX_OFFLINE=true cargo test -p risk --lib` + +**Step 5: Commit** + +```bash +git add risk/src/risk_engine.rs risk/src/risk_types.rs +git commit -m "fix(risk): extract account_id from OrderInfo instead of hardcoding 'default' + +Per-account circuit breaker and risk state was never applied to real accounts." +``` + +--- + +### Task 12: Fix check_risk_limits only warns (H5) + +**Files:** +- Modify: `adaptive-strategy/src/risk/mod.rs:904-930` + +**Context:** `check_risk_limits` checks VaR, drawdown, leverage — all only `warn!()` and return `Ok(())`. Never blocks orders. + +**Step 1: Write the failing test** + +```rust +#[tokio::test] +async fn test_check_risk_limits_errors_on_breach() { + let mut config = RiskConfig::default(); + config.max_portfolio_var = 1000.0; + let risk_mgr = create_risk_manager(config); + + // Set portfolio VaR above limit + risk_mgr.set_test_portfolio_var(2000.0).await; + + let result = risk_mgr.check_risk_limits().await; + assert!(result.is_err(), "VaR breach must return error, not just warn"); +} +``` + +**Step 2: Replace warn with error return** + +```rust +async fn check_risk_limits(&self) -> Result<()> { + let portfolio_metrics = self.get_portfolio_risk_metrics().await?; + + if portfolio_metrics.portfolio_var > self.config.max_portfolio_var { + return Err(anyhow::anyhow!( + "Portfolio VaR limit breached: {:.4} > {:.4}", + portfolio_metrics.portfolio_var, self.config.max_portfolio_var + )); + } + + if portfolio_metrics.current_drawdown > self.config.max_drawdown_threshold { + return Err(anyhow::anyhow!( + "Maximum drawdown breached: {:.4} > {:.4}", + portfolio_metrics.current_drawdown, self.config.max_drawdown_threshold + )); + } + + if portfolio_metrics.leverage > self.config.max_leverage { + return Err(anyhow::anyhow!( + "Maximum leverage breached: {:.2} > {:.2}", + portfolio_metrics.leverage, self.config.max_leverage + )); + } + + Ok(()) +} +``` + +**Step 3: Run tests** + +Run: `SQLX_OFFLINE=true cargo test -p adaptive-strategy --lib` + +**Step 4: Commit** + +```bash +git add adaptive-strategy/src/risk/mod.rs +git commit -m "fix(adaptive-strategy): risk limit breaches return errors instead of warnings + +VaR, drawdown, and leverage checks only logged warnings. Now return errors +to block order generation." +``` + +--- + +### Task 13: Fix drawdown HWM caller-supplied (H10) + +**Files:** +- Modify: `risk/src/drawdown_monitor.rs` + +**Context:** `check_drawdown_thresholds` reads `metrics.high_water_mark` from caller. If stale or wrong, drawdown understated. + +**Step 1: Add internal HWM tracking** + +Add to `DrawdownMonitor`: +```rust +/// Internal high-water mark tracking per portfolio +internal_hwm: RwLock>, +``` + +**Step 2: Update HWM on every check** + +```rust +async fn check_drawdown_thresholds( + &self, + metrics: &PnLMetrics, + config: &DrawdownAlertConfig, +) -> RiskResult<()> { + // Use internally tracked HWM, not caller-supplied + let mut hwm_map = self.internal_hwm.write().await; + let current_pnl_price = metrics.total_pnl; + let current_pnl = current_pnl_price.to_f64(); + + let hwm = hwm_map + .entry(metrics.portfolio_id.clone()) + .or_insert(current_pnl_price); + + // Update running maximum + if current_pnl_price > *hwm { + *hwm = current_pnl_price; + } + + let hwm_f64 = hwm.to_f64(); + + let current_drawdown_pct = if hwm_f64 > 0.0 { + ((hwm_f64 - current_pnl) / hwm_f64) * 100.0 + } else { + 0.0 + }; + // ... rest of threshold checking unchanged ... +``` + +**Step 3: Run tests** + +Run: `SQLX_OFFLINE=true cargo test -p risk --lib` + +**Step 4: Commit** + +```bash +git add risk/src/drawdown_monitor.rs +git commit -m "fix(risk): track high-water mark internally instead of trusting caller + +Caller-supplied HWM could be stale, understating drawdown." +``` + +--- + +### Task 14: Fix circuit breaker HalfOpen unlimited probes (H11) + +**Files:** +- Modify: `common/src/resilience/circuit_breaker.rs:241` + +**Context:** In HalfOpen state, `can_execute()` returns `true` unconditionally. N concurrent requests all probe simultaneously. + +**Step 1: Write the failing test** + +```rust +#[tokio::test] +async fn test_half_open_allows_only_one_probe() { + let cb = CircuitBreaker::new("test", config); + // Force into HalfOpen state + cb.force_half_open().await; + + // First should be allowed + assert!(cb.can_execute().await); + // Second should be denied (probe in flight) + assert!(!cb.can_execute().await, "HalfOpen should only allow one probe at a time"); +} +``` + +**Step 2: Add probe_in_flight flag** + +Add to inner state: +```rust +probe_in_flight: bool, +``` + +In `can_execute`, HalfOpen branch: +```rust +CircuitBreakerState::HalfOpen => { + if !inner.probe_in_flight { + inner.probe_in_flight = true; + true + } else { + false + } +} +``` + +In `record_success` and `record_failure`, reset: +```rust +inner.probe_in_flight = false; +``` + +**Step 3: Run tests** + +Run: `SQLX_OFFLINE=true cargo test -p common --lib` + +**Step 4: Commit** + +```bash +git add common/src/resilience/circuit_breaker.rs +git commit -m "fix(common): limit HalfOpen circuit breaker to single probe + +Unlimited concurrent probes could send N orders during recovery." +``` + +--- + +### Task 15: Fix kill switch engage() error semantics (H13) + +**Files:** +- Modify: `risk/src/safety/kill_switch.rs:67-124` + +**Context:** Sets local state (AtomicBool), then returns Err if Redis publish fails. Caller thinks kill switch didn't engage, but local state is already set. Inconsistent. + +**Step 1: Write test** + +```rust +#[tokio::test] +async fn test_engage_succeeds_even_without_redis() { + let ks = KillSwitch::new(None); // No Redis + let result = ks.engage( + KillSwitchScope::Global, + "test".into(), + "admin".into(), + false, + ).await; + assert!(result.is_ok(), "Kill switch should succeed locally without Redis"); + assert!(ks.is_triggered()); +} +``` + +**Step 2: Change error handling** + +After Redis publish failure, warn but don't return Err: +```rust +if let Err(e) = conn.publish::<_, _, ()>(&channel, message.to_string()).await { + self.failure_count.fetch_add(1, Ordering::Relaxed); + warn!( + "Kill switch engaged locally but Redis publish failed: {}. \ + Other nodes may not be notified.", + e + ); + // Don't return Err — local state is authoritative +} +``` + +Same for connection failure: +```rust +Err(e) => { + self.failure_count.fetch_add(1, Ordering::Relaxed); + warn!( + "Kill switch engaged locally but Redis connection failed: {}", + e + ); +} +``` + +**Step 3: Run tests** + +Run: `SQLX_OFFLINE=true cargo test -p risk --lib` + +**Step 4: Commit** + +```bash +git add risk/src/safety/kill_switch.rs +git commit -m "fix(risk): kill switch engage succeeds locally regardless of Redis + +Local state is authoritative. Redis failure should warn, not return error +that makes caller think kill switch didn't engage." +``` + +--- + +### Task 16: Fix validate_order gRPC skipping most risk checks (H14) + +**Files:** +- Modify: `services/trading_service/src/services/risk.rs:565-648` + +**Context:** gRPC `validate_order` only checks quantity limit and VaR. Skips position limits, leverage, circuit breaker. + +**Step 1: Convert gRPC request to OrderInfo and use full check_pre_trade_risk** + +```rust +async fn validate_order( + &self, + request: Request, +) -> Result, Status> { + let req = request.into_inner(); + + // Convert gRPC request to risk engine's OrderInfo + let order_info = risk::risk_types::OrderInfo { + order_id: req.order_id.clone().unwrap_or_default(), + symbol: req.symbol.clone().into(), + instrument_id: req.symbol.clone(), + side: if req.side == 0 { OrderSide::Buy } else { OrderSide::Sell }, + quantity: Quantity::from_f64(req.quantity), + price: Price::from_f64(req.price), + order_type: None, + portfolio_id: req.portfolio_id.clone(), + strategy_id: req.strategy_id.clone(), + account_id: Some(req.account_id.clone()), + }; + + let risk_engine = self.state.risk_engine.read().await; + match risk_engine.check_order(&order_info).await { + Ok(RiskCheckResult::Approved) => { + Ok(Response::new(ValidateOrderResponse { + is_valid: true, + violations: vec![], + risk_score: Some(default_risk_score(3.0)), + message: "Order validation passed".to_string(), + })) + }, + Ok(RiskCheckResult::Rejected { reason, violations, .. }) => { + let proto_violations = violations.iter().map(|v| /* convert */).collect(); + Ok(Response::new(ValidateOrderResponse { + is_valid: false, + violations: proto_violations, + risk_score: Some(default_risk_score(8.0)), + message: reason, + })) + }, + Err(e) => Err(Status::internal(format!("Risk check failed: {}", e))), + } +} +``` + +**Step 2: Run tests** + +Run: `SQLX_OFFLINE=true cargo test -p trading_service --lib` + +**Step 3: Commit** + +```bash +git add services/trading_service/src/services/risk.rs +git commit -m "fix(trading_service): validate_order gRPC uses full pre-trade risk checks + +Was only checking quantity limit and VaR. Now runs all 5 risk checks." +``` + +--- + +### Task 17: Fix VaR z-score extrapolation (M10) + +**Files:** +- Modify: `risk/src/var_calculator/parametric.rs:149-167` + +**Context:** Linear extrapolation above 0.99 gives z=2.477 for 99.9% (correct: 3.09). VaR understated by ~25%. + +**Step 1: Write the failing test** + +```rust +#[test] +fn test_z_score_accuracy() { + assert!((ParametricVaR::get_z_score(0.90) - 1.282).abs() < 0.01); + assert!((ParametricVaR::get_z_score(0.95) - 1.645).abs() < 0.01); + assert!((ParametricVaR::get_z_score(0.99) - 2.326).abs() < 0.01); + assert!((ParametricVaR::get_z_score(0.999) - 3.090).abs() < 0.05, + "99.9% z-score should be ~3.09, not ~2.48"); +} +``` + +**Step 2: Implement Abramowitz-Stegun rational approximation** + +```rust +fn get_z_score(confidence_level: f64) -> f64 { + // Abramowitz-Stegun rational approximation for inverse normal CDF + // Accurate to ~4.5e-4 across full range + let p = 1.0 - confidence_level; + if p <= 0.0 || p >= 1.0 { + return 2.326; // fallback to 99% z-score + } + + let t = (-2.0 * p.ln()).sqrt(); + + // Coefficients from Abramowitz-Stegun formula 26.2.23 + let c0 = 2.515517; + let c1 = 0.802853; + let c2 = 0.010328; + let d1 = 1.432788; + let d2 = 0.189269; + let d3 = 0.001308; + + t - (c0 + c1 * t + c2 * t * t) / (1.0 + d1 * t + d2 * t * t + d3 * t * t * t) +} +``` + +**Step 3: Run tests** + +Run: `SQLX_OFFLINE=true cargo test -p risk --lib` + +**Step 4: Commit** + +```bash +git add risk/src/var_calculator/parametric.rs +git commit -m "fix(risk): accurate z-score via Abramowitz-Stegun approximation + +Linear extrapolation gave z=2.48 for 99.9% (correct: 3.09). VaR was +understated by ~25% at high confidence." +``` + +--- + +### Layer 1 Checkpoint + +Run: `SQLX_OFFLINE=true cargo test -p trading_engine --lib && SQLX_OFFLINE=true cargo test -p risk --lib && SQLX_OFFLINE=true cargo test -p common --lib && SQLX_OFFLINE=true cargo test -p trading_service --lib && SQLX_OFFLINE=true cargo test -p adaptive-strategy --lib` + +--- + +## Layer 2: ML Pipeline Correctness (10 fixes) + +### Task 18: Fix DQN epsilon-greedy in production inference (C6) + +**Files:** +- Modify: `services/trading_service/src/services/enhanced_ml.rs:1412-1415` + +**Context:** `select_action` uses epsilon-greedy. With `epsilon_start=0.1`, 10% of production trades are random. + +**Step 1: Write the failing test** + +```rust +#[test] +fn test_dqn_inference_uses_greedy_action() { + // Verify that production inference doesn't use random exploration + let agent = create_trained_dqn_agent(); + // epsilon should be 0 during inference + assert_eq!(agent.get_metrics().epsilon, 0.0, + "DQN should use greedy (epsilon=0) during production inference"); +} +``` + +**Step 2: Set epsilon to 0 before inference** + +In the DQN prediction path (around line 1412): +```rust +// Set epsilon to 0 for pure greedy inference (no random exploration) +agent.set_epsilon(0.0); + +let action = agent + .select_action(&trading_state) + .map_err(|e| ml::MLError::InferenceError(format!("DQN prediction failed: {}", e)))?; +``` + +Or if `DQN` has a `select_greedy_action` / `act_greedy` method, use that instead. + +**Step 3: Run tests** + +Run: `SQLX_OFFLINE=true cargo test -p trading_service --lib` + +**Step 4: Commit** + +```bash +git add services/trading_service/src/services/enhanced_ml.rs +git commit -m "fix(trading_service): DQN uses greedy inference (epsilon=0) in production + +10% of production trades were random due to epsilon-greedy exploration." +``` + +--- + +### Task 19: Fix ensemble NaN/Inf validation (C7) + +**Files:** +- Modify: `ml/src/ensemble/coordinator.rs:236-251` + +**Context:** Adapter can return NaN/Inf direction or confidence. NaN propagates through weighted_sum and consensus_confidence. + +**Step 1: Write the failing test** + +```rust +#[test] +fn test_ensemble_skips_nan_predictions() { + let mut coordinator = create_test_coordinator(); + // Mock adapter that returns NaN confidence + coordinator.add_mock_adapter("bad_model", f64::NAN, 0.8); + coordinator.add_mock_adapter("good_model", 0.7, 0.85); + + let result = coordinator.generate_predictions(&features); + // Should have 1 valid prediction, not 2 + assert_eq!(result.len(), 1); + assert!(result[0].confidence.is_finite()); +} +``` + +**Step 2: Add validation after adapter prediction** + +After line 243 (`predictions.push(prediction)`): +```rust +match adapter.predict(&fv) { + Ok(ensemble_pred) => { + // Validate prediction values are finite + if !ensemble_pred.direction.is_finite() || !ensemble_pred.confidence.is_finite() { + warn!( + "Adapter {} returned non-finite prediction (dir={}, conf={}), skipping", + model_id, ensemble_pred.direction, ensemble_pred.confidence + ); + continue; + } + let prediction = ModelPrediction::new( + model_id.clone(), + ensemble_pred.direction, + ensemble_pred.confidence, + ); + predictions.push(prediction); + } + // ... +} +``` + +**Step 3: Run tests** + +Run: `SQLX_OFFLINE=true cargo test -p ml --lib` + +**Step 4: Commit** + +```bash +git add ml/src/ensemble/coordinator.rs +git commit -m "fix(ml): validate ensemble predictions for NaN/Inf before aggregation + +Non-finite values from adapters propagated through weighted sums." +``` + +--- + +### Task 20: Fix confidence threshold enforcement (C8) + +**Files:** +- Modify: `services/trading_service/src/services/enhanced_ml.rs:529-621` + +**Context:** `confidence_threshold=0.7` is set but never checked. Any confidence level is returned as a trade signal. + +**Step 1: Write the failing test** + +```rust +#[tokio::test] +async fn test_low_confidence_returns_hold() { + let service = create_test_ml_service(); + // Mock models that return low confidence + service.add_mock_model("m1", 0.8, 0.3); // Buy but confidence 0.3 + + let features = create_test_features(); + let result = service.get_ensemble_prediction(&features, "AAPL").await; + + match result { + Ok(vote) => { + // Low confidence should force Hold + assert_eq!(vote.consensus_prediction, PredictionType::Hold as i32, + "Below-threshold confidence must return Hold"); + }, + Err(_) => { /* Also acceptable to return error */ } + } +} +``` + +**Step 2: Add threshold check** + +After computing `consensus_confidence` (around line 596): +```rust +let consensus_confidence = total_confidence / valid_predictions as f64; + +// Enforce confidence threshold +if consensus_confidence < config.confidence_threshold { + info!( + "Ensemble confidence {:.3} below threshold {:.3}, forcing Hold for {}", + consensus_confidence, config.confidence_threshold, symbol + ); + return Ok(EnsembleVote { + symbol: symbol.to_string(), + consensus_prediction: crate::proto::ml::PredictionType::Hold as i32, + consensus_confidence, + votes_buy: buy_votes, + votes_sell: sell_votes, + votes_hold: hold_votes, + total_models: valid_predictions, + signal_strength: crate::proto::ml::SignalStrength::VeryWeak as i32, + }); +} +``` + +**Step 3: Run tests** + +Run: `SQLX_OFFLINE=true cargo test -p trading_service --lib` + +**Step 4: Commit** + +```bash +git add services/trading_service/src/services/enhanced_ml.rs +git commit -m "fix(trading_service): enforce confidence threshold in ensemble prediction + +Threshold was configured but never checked. Low-confidence signals became trades." +``` + +--- + +### Task 21: Fix TFT/Mamba2 loading random weights (C5) + +**Files:** +- Modify: `services/trading_service/src/services/enhanced_ml.rs:1686-1702` (TFT) +- Modify: `services/trading_service/src/services/enhanced_ml.rs:1794-1807` (Mamba2) + +**Context:** `from_checkpoint` creates fresh model with random weights, sets `is_trained=true`. Trades on noise. + +**Step 1: Write the failing test** + +```rust +#[test] +fn test_tft_from_checkpoint_requires_real_file() { + let result = RealTFTModel::from_checkpoint( + "test_tft", + Path::new("/nonexistent/checkpoint.safetensors"), + ); + assert!(result.is_err(), "TFT must fail if checkpoint doesn't exist"); +} + +#[test] +fn test_mamba2_from_checkpoint_requires_real_file() { + let result = RealMamba2Model::from_checkpoint( + "test_mamba2", + Path::new("/nonexistent/checkpoint.safetensors"), + ); + assert!(result.is_err(), "Mamba2 must fail if checkpoint doesn't exist"); +} +``` + +**Step 2: Implement real checkpoint loading or fail** + +For TFT: +```rust +pub fn from_checkpoint( + model_id: &str, + checkpoint_path: &Path, +) -> Result { + // Verify checkpoint exists + if !checkpoint_path.exists() { + return Err(ml::MLError::ModelError(format!( + "TFT checkpoint not found: {}. Cannot trade with random weights.", + checkpoint_path.display() + ))); + } + + let config = /* ... same config ... */; + + // Load weights from safetensors + let device = ml::prelude::Device::Cpu; + let vb = ml::prelude::VarBuilder::from_mmaped_safetensors( + &[checkpoint_path], + ml::prelude::DType::F32, + &device, + ).map_err(|e| ml::MLError::ModelError(format!("Failed to load TFT checkpoint: {}", e)))?; + + let tft = TemporalFusionTransformer::from_varbuilder(config.clone(), vb) + .map_err(|e| ml::MLError::ModelError(format!("Failed to create TFT from checkpoint: {}", e)))?; + + // Verify model is actually trained (has non-zero weights) + // tft.is_trained should be set by the loading process, not forced + + info!( + "Loaded TFT model {} from checkpoint {}", + model_id, checkpoint_path.display() + ); + + Ok(Self { + model_id: model_id.to_string(), + model: Arc::new(RwLock::new(tft)), + config, + }) +} +``` + +Same pattern for Mamba2 — use `Mamba2SSM::from_varbuilder` or equivalent. If model doesn't have `from_varbuilder`, implement it using existing candle `VarBuilder::from_mmaped_safetensors` patterns (same as DQN). + +If model crate doesn't support VarBuilder loading yet, return error: +```rust +return Err(ml::MLError::ModelError(format!( + "TFT checkpoint loading not yet implemented. Cannot use random weights in production." +))); +``` + +**Step 3: Run tests** + +Run: `SQLX_OFFLINE=true cargo test -p trading_service --lib` + +**Step 4: Commit** + +```bash +git add services/trading_service/src/services/enhanced_ml.rs +git commit -m "fix(trading_service): TFT/Mamba2 must load real checkpoints, not random weights + +from_checkpoint was creating fresh models with random weights and setting +is_trained=true. Now validates checkpoint exists and loads real weights." +``` + +--- + +### Task 22: Fix fake model performance metrics (H6) + retrain stub (H7) + +**Files:** +- Modify: `services/trading_service/src/services/enhanced_ml.rs:1146-1199` + +**Context:** `get_model_performance` returns hardcoded `accuracy=0.85, sharpe=1.45`. `retrain_model` returns success without training. + +**Step 1: Fix get_model_performance** + +```rust +async fn get_model_performance( + &self, + request: Request, +) -> Result, Status> { + let req = request.into_inner(); + + // Try to get real metrics from performance monitor + let models = self.models.read().await; + let model_meta = models.get(&req.model_name).ok_or_else(|| { + Status::not_found(format!("Model '{}' not found", req.model_name)) + })?; + + let performance = ModelPerformance { + model_name: req.model_name.clone(), + accuracy: 0.0, // Real metrics not yet tracked + precision: 0.0, + recall: 0.0, + f1_score: 0.0, + sharpe_ratio: 0.0, + win_rate: 0.0, + avg_return: 0.0, + max_drawdown: 0.0, + total_predictions: model_meta.inference_count as i64, + performance_period_start: req.start_time.unwrap_or(0), + performance_period_end: req.end_time.unwrap_or(0), + daily_performance: vec![], + }; + + Ok(Response::new(GetModelPerformanceResponse { + performance: Some(performance), + })) +} +``` + +**Step 2: Fix retrain_model** + +```rust +async fn retrain_model( + &self, + request: Request, +) -> Result, Status> { + let req = request.into_inner(); + Err(Status::unimplemented(format!( + "Model retraining for '{}' not yet connected to training service", + req.model_name + ))) +} +``` + +**Step 3: Run tests** + +Run: `SQLX_OFFLINE=true cargo test -p trading_service --lib` + +**Step 4: Commit** + +```bash +git add services/trading_service/src/services/enhanced_ml.rs +git commit -m "fix(trading_service): remove fake model metrics, make retrain_model honest + +Hardcoded accuracy/sharpe created false confidence. retrain_model now +returns Unimplemented instead of faking success." +``` + +--- + +### Task 23: Fix DST-broken trading session (H8) + +**Files:** +- Modify: `ml/src/ensemble/coordinator.rs:381-393` +- Modify: `ml/Cargo.toml` (add `chrono-tz`) + +**Context:** Hardcoded UTC-5 (EST). During EDT (March-November), session boundaries off by 1 hour. + +**Step 1: Add dependency** + +In `ml/Cargo.toml`: +```toml +chrono-tz = "0.10" +``` + +**Step 2: Write the failing test** + +```rust +#[test] +fn test_trading_session_handles_dst() { + // During EDT (e.g., June), market opens at 9:30 ET = 13:30 UTC + // With UTC-5 hardcode, 13:30 UTC → 08:30 ET → PreMarket (wrong!) + // With proper tz, 13:30 UTC → 09:30 ET → Regular (correct!) + let june_930_et = Utc.with_ymd_and_hms(2026, 6, 15, 13, 30, 0).unwrap(); + let session = EnsembleCoordinator::trading_session_at(june_930_et); + assert_eq!(session, TradingSession::Regular, + "9:30 AM ET in June should be Regular session"); +} +``` + +**Step 3: Implement DST-aware conversion** + +```rust +use chrono_tz::America::New_York; + +fn current_trading_session() -> TradingSession { + Self::trading_session_at(Utc::now()) +} + +fn trading_session_at(utc_time: DateTime) -> TradingSession { + let et = utc_time.with_timezone(&New_York); + let et_time = et.hour() * 60 + et.minute(); + + match et_time { + t if t < 240 => TradingSession::AfterHours, // 00:00-04:00 ET + t if t < 570 => TradingSession::PreMarket, // 04:00-09:30 ET + t if t < 960 => TradingSession::Regular, // 09:30-16:00 ET + _ => TradingSession::AfterHours, // 16:00-24:00 ET + } +} +``` + +**Step 4: Run tests** + +Run: `SQLX_OFFLINE=true cargo test -p ml --lib` + +**Step 5: Commit** + +```bash +git add ml/Cargo.toml ml/src/ensemble/coordinator.rs +git commit -m "fix(ml): DST-aware trading session detection using chrono-tz + +Hardcoded UTC-5 made all session boundaries wrong during EDT (Mar-Nov)." +``` + +--- + +### Task 24: Fix ensemble weights not normalized (H9) + +**Files:** +- Modify: `ml/src/ensemble/decision.rs:189-206` +- Add normalization step wherever weights are updated + +**Context:** `effective_weight() = static_weight * dynamic_weight`. Sum can range 0.5-1.5, not 1.0. + +**Step 1: Write the failing test** + +```rust +#[test] +fn test_weights_sum_to_one_after_update() { + let mut models = vec![ + ModelWeight::new("m1", 0.4), + ModelWeight::new("m2", 0.3), + ModelWeight::new("m3", 0.3), + ]; + + // Simulate dynamic update + for m in &mut models { + m.update_dynamic_weight(); + } + + // Normalize + normalize_weights(&mut models); + + let sum: f64 = models.iter().map(|m| m.effective_weight()).sum(); + assert!((sum - 1.0).abs() < 1e-9, "Weights must sum to 1.0, got {}", sum); +} +``` + +**Step 2: Add normalization function** + +```rust +/// Normalize effective weights across all models to sum to 1.0 +pub fn normalize_weights(models: &mut [ModelWeight]) { + let sum: f64 = models.iter().map(|m| m.effective_weight()).sum(); + if sum > 0.0 && sum.is_finite() { + for m in models.iter_mut() { + m.dynamic_weight *= 1.0 / sum * m.static_weight.recip(); + // Alternatively, store a normalization factor + } + } +} +``` + +Or simpler approach — normalize at usage point: +```rust +// In coordinator, after collecting weights: +let weight_sum: f64 = weights.values().sum(); +let normalized_weight = if weight_sum > 0.0 { + weight / weight_sum +} else { + 1.0 / weights.len() as f64 +}; +``` + +**Step 3: Run tests** + +Run: `SQLX_OFFLINE=true cargo test -p ml --lib` + +**Step 4: Commit** + +```bash +git add ml/src/ensemble/decision.rs ml/src/ensemble/coordinator.rs +git commit -m "fix(ml): normalize ensemble weights after dynamic updates + +Unnormalized weights ranged 0.5-1.5 instead of summing to 1.0." +``` + +--- + +### Task 25: Fix Mamba2 swallowing errors (M3) + +**Files:** +- Modify: `services/trading_service/src/services/enhanced_ml.rs:1837-1839` + +**Context:** `unwrap_or_else(|_| 0.5)` swallows inference errors. Not logged, not counted. Fallback manager never degrades model health. + +**Step 1: Propagate error** + +```rust +// BEFORE: +let raw_prediction = mamba2 + .predict_single_fast(&input) + .unwrap_or_else(|_| 0.5); + +// AFTER: +let raw_prediction = mamba2 + .predict_single_fast(&input) + .map_err(|e| ml::MLError::InferenceError( + format!("Mamba2 inference failed: {}", e) + ))?; +``` + +**Step 2: Run tests** + +Run: `SQLX_OFFLINE=true cargo test -p trading_service --lib` + +**Step 3: Commit** + +```bash +git add services/trading_service/src/services/enhanced_ml.rs +git commit -m "fix(trading_service): propagate Mamba2 inference errors instead of swallowing + +unwrap_or(0.5) hid failures. Model health was never degraded on errors." +``` + +--- + +### Task 26: Fix MarketStateTracker NaN initialization (M4) + +**Files:** +- Modify: `adaptive-strategy/src/risk/ppo_position_sizer.rs:1142-1148` + +**Context:** Feature vectors initialized with `f64::NAN`. If update fails before get_current_state, NaN propagates through PPO network. + +**Step 1: Write the failing test** + +```rust +#[test] +fn test_market_state_tracker_no_nan_on_init() { + let tracker = MarketStateTracker::new(12).unwrap(); + let state = tracker.get_current_state(); + for (i, v) in state.iter().enumerate() { + assert!(v.is_finite(), "Feature {} is NaN/Inf on init", i); + } +} +``` + +**Step 2: Initialize with 0.0** + +```rust +Ok(Self { + market_features: vec![0.0; state_dim / 3], + portfolio_features: vec![0.0; state_dim / 3], + risk_features: vec![0.0; state_dim / 3], + // ... +}) +``` + +**Step 3: Run tests** + +Run: `SQLX_OFFLINE=true cargo test -p adaptive-strategy --lib` + +**Step 4: Commit** + +```bash +git add adaptive-strategy/src/risk/ppo_position_sizer.rs +git commit -m "fix(adaptive-strategy): initialize MarketStateTracker with 0.0 not NaN + +NaN init propagated through PPO network if update hadn't run yet." +``` + +--- + +### Task 27: Fix avg_latency_us overwriting (M5) + +**Files:** +- Modify: `services/trading_service/src/services/enhanced_ml.rs:818` + +**Context:** `avg_latency_us = latency_us` replaces on every call. Not a running average. + +**Step 1: Implement exponential moving average** + +```rust +// BEFORE: +model_meta.avg_latency_us = latency_us as f64; + +// AFTER: +if model_meta.inference_count <= 1 { + model_meta.avg_latency_us = latency_us as f64; +} else { + // Exponential moving average (alpha=0.1) + model_meta.avg_latency_us = 0.9 * model_meta.avg_latency_us + 0.1 * latency_us as f64; +} +``` + +**Step 2: Run tests** + +Run: `SQLX_OFFLINE=true cargo test -p trading_service --lib` + +**Step 3: Commit** + +```bash +git add services/trading_service/src/services/enhanced_ml.rs +git commit -m "fix(trading_service): use EMA for avg_latency_us instead of overwriting + +Was replacing the average on every call, losing history." +``` + +--- + +### Layer 2 Checkpoint + +Run: `SQLX_OFFLINE=true cargo test -p ml --lib && SQLX_OFFLINE=true cargo test -p trading_service --lib && SQLX_OFFLINE=true cargo test -p adaptive-strategy --lib` + +--- + +## Layer 3: Broker Safety (5 fixes) + +### Task 28: Fix set_broker_client silently dropping connection (C3) + +**Files:** +- Modify: `services/broker_gateway_service/src/service.rs:44-51` + +**Context:** `try_write()` is non-blocking. If lock contended, `CTraderClient` is dropped silently. Service runs without broker. + +**Step 1: Use blocking write** + +```rust +#[cfg(feature = "icmarkets")] +pub async fn set_broker_client(&self, client: CTraderClient) { + let mut guard = self.broker_client.write().await; + *guard = Some(client); + info!("Broker client set successfully"); +} +``` + +Note: Change signature from `fn` to `async fn` since `.write().await` requires async. + +**Step 2: Update all call sites to `.await`** + +**Step 3: Run tests** + +Run: `SQLX_OFFLINE=true cargo test -p broker_gateway_service --lib` + +**Step 4: Commit** + +```bash +git add services/broker_gateway_service/src/service.rs +git commit -m "fix(broker_gateway): use blocking write for set_broker_client + +try_write() silently dropped CTraderClient if lock was contended." +``` + +--- + +### Task 29: Fix cTrader volume truncation (C4) + +**Files:** +- Modify: `services/broker_gateway_service/src/service.rs:223` + +**Context:** `(req.quantity * 100_000.0) as i64` truncates and can overflow. + +**Step 1: Write the failing test** + +```rust +#[test] +fn test_volume_conversion_rejects_overflow() { + // Very large quantity that would overflow i64 + let result = convert_quantity_to_volume(f64::MAX / 100_000.0 + 1.0); + assert!(result.is_err()); +} + +#[test] +fn test_volume_conversion_rounds_correctly() { + // 1.5 lots = 150,000 volume + assert_eq!(convert_quantity_to_volume(1.5).unwrap(), 150_000); + // 0.01 lots = 1,000 volume + assert_eq!(convert_quantity_to_volume(0.01).unwrap(), 1_000); +} +``` + +**Step 2: Add safe conversion function** + +```rust +fn convert_quantity_to_volume(quantity: f64) -> Result { + let volume_f = quantity * 100_000.0; + if !volume_f.is_finite() || volume_f < 0.0 || volume_f > i64::MAX as f64 { + return Err(Status::invalid_argument(format!( + "Volume overflow: quantity {} produces volume {}", + quantity, volume_f + ))); + } + Ok(volume_f.round() as i64) +} +``` + +Replace line 223: +```rust +let volume = convert_quantity_to_volume(req.quantity)?; +``` + +**Step 3: Run tests** + +Run: `SQLX_OFFLINE=true cargo test -p broker_gateway_service --lib` + +**Step 4: Commit** + +```bash +git add services/broker_gateway_service/src/service.rs +git commit -m "fix(broker_gateway): safe volume conversion with overflow check + +(quantity * 100_000.0) as i64 truncated fractional lots and could overflow." +``` + +--- + +### Task 30: Fix DB update failure silently discarded (H3) + +**Files:** +- Modify: `services/broker_gateway_service/src/service.rs:248-254` + +**Context:** `let _ = sqlx::query(...).execute(...).await;` — if DB update fails, order is live at broker but DB shows PENDING_SUBMIT. + +**Step 1: Handle the error** + +```rust +// BEFORE: +let _ = sqlx::query( + "UPDATE broker_orders SET status = 'SUBMITTED', broker_order_id = $1, updated_at = NOW() WHERE client_order_id = $2", +) +.bind(&broker_order_id) +.bind(&client_order_id) +.execute(&self.db_pool) +.await; + +// AFTER: +if let Err(db_err) = sqlx::query( + "UPDATE broker_orders SET status = 'SUBMITTED', broker_order_id = $1, updated_at = NOW() WHERE client_order_id = $2", +) +.bind(&broker_order_id) +.bind(&client_order_id) +.execute(&self.db_pool) +.await { + // CRITICAL: Order is live at broker but DB state is stale + error!( + broker_order_id = %broker_order_id, + client_order_id = %client_order_id, + error = %db_err, + "CRITICAL: DB update failed after broker submission. Order is LIVE but DB shows PENDING_SUBMIT. Manual reconciliation required." + ); + // Don't fail the RPC — order IS submitted. But alert for reconciliation. +} +``` + +**Step 2: Run tests** + +Run: `SQLX_OFFLINE=true cargo test -p broker_gateway_service --lib` + +**Step 3: Commit** + +```bash +git add services/broker_gateway_service/src/service.rs +git commit -m "fix(broker_gateway): log critical error when DB update fails after submission + +Silent discard meant order was live at broker but DB showed PENDING_SUBMIT. +Recovery would re-submit, creating duplicate live orders." +``` + +--- + +### Task 31: Fix reconnect() faking success (M1) + +**Files:** +- Modify: `services/broker_gateway_service/src/recovery/mod.rs:129-172` + +**Context:** `sleep(500ms)` then unconditionally sets `SessionState::Active`. Reports healthy during real outage. + +**Step 1: Gate on actual health check** + +```rust +pub async fn reconnect(&self) -> anyhow::Result<()> { + info!("Attempting to reconnect FIX session: {}", self.session_id); + + { + let mut state = self.session_state.write().await; + *state = SessionState::LoggingIn; + } + + sqlx::query( + "UPDATE broker_sessions SET session_state = 'RECONNECTING', updated_at = NOW() WHERE session_id = $1", + ) + .bind(&self.session_id) + .execute(&self.db_pool) + .await?; + + // Attempt actual reconnection via broker client + #[cfg(feature = "icmarkets")] + { + let broker = self.broker_client.read().await; + if let Some(ref client) = *broker { + // Real health check + client.send_heartbeat().await.map_err(|e| { + anyhow::anyhow!("Reconnection failed — broker unreachable: {}", e) + })?; + } else { + return Err(anyhow::anyhow!("No broker client configured for reconnection")); + } + } + + // Only set Active if health check passed + { + let mut state = self.session_state.write().await; + *state = SessionState::Active; + } + + sqlx::query( + "UPDATE broker_sessions SET session_state = 'ACTIVE', connected_at = NOW(), updated_at = NOW() WHERE session_id = $1", + ) + .bind(&self.session_id) + .execute(&self.db_pool) + .await?; + + info!("FIX session {} reconnected successfully", self.session_id); + Ok(()) +} +``` + +**Step 2: Run tests** + +Run: `SQLX_OFFLINE=true cargo test -p broker_gateway_service --lib` + +**Step 3: Commit** + +```bash +git add services/broker_gateway_service/src/recovery/mod.rs +git commit -m "fix(broker_gateway): gate reconnect on actual broker health check + +Was sleep(500ms) then unconditionally Active. Reported healthy during outage." +``` + +--- + +### Task 32: Fix SessionState unused in order routing (M9) + +**Files:** +- Modify: `services/broker_gateway_service/src/service.rs` (route_order method) + +**Context:** `session_state` is initialized to Active but never checked before routing orders. + +**Step 1: Add state check at top of route_order** + +```rust +// At the start of the order routing logic: +{ + let state = self.session_state.read().await; + if *state != SessionState::Active { + return Err(Status::unavailable(format!( + "Broker gateway not connected (state: {:?}). Cannot route orders.", + *state + ))); + } +} + +// Also check broker client +#[cfg(feature = "icmarkets")] +{ + let broker = self.broker_client.read().await; + if broker.is_none() { + return Err(Status::unavailable( + "Broker client not initialized. Cannot route orders." + )); + } +} +``` + +**Step 2: Run tests** + +Run: `SQLX_OFFLINE=true cargo test -p broker_gateway_service --lib` + +**Step 3: Commit** + +```bash +git add services/broker_gateway_service/src/service.rs +git commit -m "fix(broker_gateway): check session state before routing orders + +Orders were accepted even when broker connection was down." +``` + +--- + +### Layer 3 Checkpoint + +Run: `SQLX_OFFLINE=true cargo test -p broker_gateway_service --lib` + +--- + +## Layer 4: Auth & Operational Hardening (6 fixes) + +### Task 33: Fix auth stub accepts any password (C12) + +**Files:** +- Modify: `web-gateway/src/routes/auth.rs:27-61` + +**Context:** Any non-empty username+password → valid 24hr JWT. No dev-mode gate. + +**Step 1: Add dev-mode gate** + +```rust +async fn login( + State(state): State, + Json(body): Json, +) -> Result, AppError> { + if body.username.is_empty() || body.password.is_empty() { + return Err(AppError::BadRequest("Username and password required".into())); + } + + // SECURITY: Require explicit dev-mode opt-in for stub auth + let dev_auth = std::env::var("FOXHUNT_DEV_AUTH").unwrap_or_default(); + if dev_auth != "true" { + return Err(AppError::Internal(anyhow::anyhow!( + "Production auth provider not configured. Set FOXHUNT_DEV_AUTH=true for development stub." + ))); + } + + warn!( + "DEV AUTH STUB: Accepting login for '{}' without password verification. \ + DO NOT USE IN PRODUCTION.", + body.username + ); + + // ... rest of JWT generation unchanged ... +``` + +**Step 2: Update tests to set env var** + +Existing tests that call login need `std::env::set_var("FOXHUNT_DEV_AUTH", "true")` in setup. + +**Step 3: Run tests** + +Run: `SQLX_OFFLINE=true cargo test -p web-gateway --lib` + +**Step 4: Commit** + +```bash +git add web-gateway/src/routes/auth.rs +git commit -m "fix(web-gateway): gate auth stub behind FOXHUNT_DEV_AUTH env var + +Any username/password was accepted. Now requires explicit opt-in for dev mode." +``` + +--- + +### Task 34: Fix cTrader Live/Demo single env var (C-new) + +**Files:** +- Modify: `services/broker_gateway_service/src/main.rs:72-79` + +**Context:** `CTRADER_LIVE=true` → real money with no confirmation. + +**Step 1: Add confirmation requirement** + +```rust +environment: if std::env::var("CTRADER_LIVE") + .map(|v| v == "true") + .unwrap_or(false) +{ + // Require explicit confirmation for live trading + let confirmed = std::env::var("CTRADER_LIVE_CONFIRMED") + .unwrap_or_default(); + if confirmed != "I_UNDERSTAND_REAL_MONEY" { + anyhow::bail!( + "CTRADER_LIVE=true requires CTRADER_LIVE_CONFIRMED=I_UNDERSTAND_REAL_MONEY\n\ + ⚠️ WARNING: Live mode trades REAL MONEY. This is NOT a drill.\n\ + Set CTRADER_LIVE_CONFIRMED=I_UNDERSTAND_REAL_MONEY to confirm." + ); + } + + eprintln!("╔══════════════════════════════════════════════╗"); + eprintln!("║ ⚠️ LIVE TRADING MODE — REAL MONEY ⚠️ ║"); + eprintln!("║ Orders will execute on live markets. ║"); + eprintln!("║ Starting in 5 seconds... ║"); + eprintln!("╚══════════════════════════════════════════════╝"); + tokio::time::sleep(std::time::Duration::from_secs(5)).await; + + ctrader_openapi::CTraderEnvironment::Live +} else { + ctrader_openapi::CTraderEnvironment::Demo +}, +``` + +**Step 2: Run tests** + +Run: `SQLX_OFFLINE=true cargo test -p broker_gateway_service --lib` + +**Step 3: Commit** + +```bash +git add services/broker_gateway_service/src/main.rs +git commit -m "fix(broker_gateway): require CTRADER_LIVE_CONFIRMED for live trading + +Single env var flip to real money was too easy. Now requires explicit +confirmation string and shows prominent warning with 5s delay." +``` + +--- + +### Task 35: Fix VaR static hardcoded volatility (H12) + +**Files:** +- Modify: `risk/src/risk_engine.rs:334-339` + +**Context:** Asset classification returns static volatilities. Not real market data. + +**Step 1: Add logging so we know when static vol is used** + +```rust +fn get_symbol_volatility(&self, instrument_id: &str) -> RiskResult { + // TODO: Wire to market data service for live implied/realized volatility + warn!( + "Using STATIC volatility for '{}'. Wire to market data service for production.", + instrument_id + ); + let daily_volatility = self + .asset_classification + .get_daily_volatility(instrument_id); + f64_to_decimal_safe(daily_volatility, "daily volatility conversion") +} +``` + +Additionally, add `VolatilityOverrides` config support: + +```rust +fn get_symbol_volatility(&self, instrument_id: &str) -> RiskResult { + // Check overrides first + if let Some(override_vol) = self.volatility_overrides.get(instrument_id) { + return f64_to_decimal_safe(*override_vol, "volatility override"); + } + + warn!( + "Using STATIC volatility for '{}' — not market data", + instrument_id + ); + let daily_volatility = self.asset_classification.get_daily_volatility(instrument_id); + f64_to_decimal_safe(daily_volatility, "daily volatility conversion") +} +``` + +**Step 2: Run tests** + +Run: `SQLX_OFFLINE=true cargo test -p risk --lib` + +**Step 3: Commit** + +```bash +git add risk/src/risk_engine.rs +git commit -m "fix(risk): log warning when using static volatility, add overrides + +Static volatility understates risk in high-vol regimes. Added override +config and prominent warning for auditing." +``` + +--- + +### Task 36: Fix Kelly sizing fake return history (M2) + +**Files:** +- Modify: `adaptive-strategy/src/risk/mod.rs:559-568` + +**Context:** Hardcoded 20-value return vector for all symbols. Kelly fraction wrong for every instrument. + +**Step 1: Return error instead of fake data** + +```rust +async fn get_historical_returns(&self, symbol: &str) -> Result> { + // Historical return data not yet connected to market data service + Err(anyhow::anyhow!( + "Historical return data not available for '{}'. \ + Connect market data service for Kelly criterion sizing. \ + Callers should fall back to fixed-fraction sizing.", + symbol + )) +} +``` + +**Step 2: Ensure callers handle the error gracefully** + +The caller (`calculate_kelly_position`) should fall back to a simpler sizing method when Kelly data is unavailable. + +**Step 3: Run tests** + +Run: `SQLX_OFFLINE=true cargo test -p adaptive-strategy --lib` + +**Step 4: Commit** + +```bash +git add adaptive-strategy/src/risk/mod.rs +git commit -m "fix(adaptive-strategy): return error instead of fake Kelly returns + +Hardcoded 20-value return vector gave wrong Kelly fraction for every instrument." +``` + +--- + +### Task 37: Fix rate limiter spoofable via X-Forwarded-For (M8) + +**Files:** +- Modify: `web-gateway/src/rate_limit.rs:67-82` + +**Context:** First value from X-Forwarded-For is client-controlled. Attacker cycles fake IPs to bypass rate limit. + +**Step 1: Write the failing test** + +```rust +#[test] +fn test_client_ip_ignores_xff_by_default() { + let mut req = Request::new(Body::empty()); + req.headers_mut().insert("x-forwarded-for", "1.2.3.4".parse().unwrap()); + // Should NOT trust XFF without trusted proxy config + let ip = client_ip(&req); + assert_ne!(ip, "1.2.3.4", "Should not trust X-Forwarded-For without trusted proxy"); +} +``` + +**Step 2: Use ConnectInfo as primary, XFF only from trusted proxies** + +```rust +fn client_ip(request: &Request) -> String { + // Primary: Use TCP connection address (not spoofable) + if let Some(connect_info) = request.extensions().get::>() { + return connect_info.0.ip().to_string(); + } + + // Fallback: Only trust X-Forwarded-For from known proxy ranges + // For now, don't trust any XFF — use "unknown" + // TODO: Add TRUSTED_PROXY_CIDRS config for Cloudflare/LB ranges + "unknown".to_owned() +} +``` + +**Step 3: Run tests** + +Run: `SQLX_OFFLINE=true cargo test -p web-gateway --lib` + +**Step 4: Commit** + +```bash +git add web-gateway/src/rate_limit.rs +git commit -m "fix(web-gateway): rate limiter uses ConnectInfo instead of spoofable XFF + +X-Forwarded-For is client-controlled. Attacker could cycle fake IPs +to bypass rate limits. Now uses TCP connection address." +``` + +--- + +### Task 38: Final verification + +Run full workspace check and all crate tests: + +```bash +SQLX_OFFLINE=true cargo check --workspace +SQLX_OFFLINE=true cargo clippy --workspace -- -D warnings +SQLX_OFFLINE=true cargo test -p trading_engine --lib +SQLX_OFFLINE=true cargo test -p trading_service --lib +SQLX_OFFLINE=true cargo test -p risk --lib +SQLX_OFFLINE=true cargo test -p common --lib +SQLX_OFFLINE=true cargo test -p ml --lib +SQLX_OFFLINE=true cargo test -p adaptive-strategy --lib +SQLX_OFFLINE=true cargo test -p web-gateway --lib +SQLX_OFFLINE=true cargo test -p broker_gateway_service --lib +``` + +Expected: All tests pass, 0 warnings, 0 clippy errors. + +--- + +## Summary + +| Layer | Tasks | CRITICALs | Crates Affected | +|-------|-------|-----------|-----------------| +| 0: Data Integrity | 1-7 | C11, C13 | trading_engine, trading_service | +| 1: Risk Enforcement | 8-17 | C1, C2, C9, C10 | trading_engine, risk, common, trading_service, adaptive-strategy | +| 2: ML Pipeline | 18-27 | C5, C6, C7, C8 | ml, trading_service, adaptive-strategy | +| 3: Broker Safety | 28-32 | C3, C4 | broker_gateway_service | +| 4: Auth & Ops | 33-38 | C12, C-new | web-gateway, broker_gateway_service, risk, adaptive-strategy | +| **Total** | **38** | **13** | **8 crates** |