From 435ebe66c1632cf45e94a009e3f692d4ef497809 Mon Sep 17 00:00:00 2001 From: BossX Date: Mon, 13 Jul 2026 15:23:32 +0800 Subject: [PATCH] fix: harden maker notification layer (fixes #221) Six small notification-layer fixes grouped together: 1. PositionRiskKind passthrough: position_jump() now reads event.kind and maps it to a distinct webhook kind (direction_flip / max_position_crossed / inventory_exit_crossed / position_jump), escalating DirectionFlip and MaxPositionCrossed to severity "critical" so a reversal or breach is no longer indistinguishable from an ordinary threshold jump. 2. Webhook text now carries the same [severity/kind] symbol: prefix the stderr line uses, so a phone alert identifies its source across instances. 3. MakerFileConfig gets serde(deny_unknown_fields) so a typo like `alert_los` errors instead of silently disabling a guard. 4. alert_loss / alert_inventory_pct / alert_uptime are range-validated at startup (reject negatives and percentages >100) via a testable validate_alert_thresholds helper, mirroring alert_position_change_pct. 5. Order-response stream now emits risk_notifications on disconnect, reconnect success, and reconnect failure, matching the account-stream path (previously entirely silent on the webhook). 6. Webhook POST retries twice with linear backoff on 5xx/timeout, and alert() gained an await-capable path so a firing alert on the final cycle before shutdown is not dropped with its spawned task. Tests: kind-descriptor mapping, deny_unknown_fields typo rejection, and alert threshold range validation. Co-Authored-By: Claude Opus 4.8 --- .../standx-cli/src/commands/maker/config.rs | 12 ++ crates/standx-cli/src/commands/maker/mod.rs | 16 ++ .../standx-cli/src/commands/maker/notify.rs | 125 +++++++++++---- .../standx-cli/src/commands/maker/runtime.rs | 147 ++++++++++++++++-- 4 files changed, 263 insertions(+), 37 deletions(-) diff --git a/crates/standx-cli/src/commands/maker/config.rs b/crates/standx-cli/src/commands/maker/config.rs index 314ff7c..af28287 100644 --- a/crates/standx-cli/src/commands/maker/config.rs +++ b/crates/standx-cli/src/commands/maker/config.rs @@ -8,6 +8,7 @@ use std::path::{Path, PathBuf}; /// Values are optional so an explicit CLI flag can override one field without /// requiring every strategy default to be repeated in TOML. #[derive(Debug, Default, Deserialize)] +#[serde(deny_unknown_fields)] pub(super) struct MakerFileConfig { pub spread_bps: Option, pub band_bps: Option, @@ -89,6 +90,17 @@ mod tests { assert_eq!(config.account_stream_reconnect_backoff, Some(2)); } + #[test] + fn rejects_unknown_keys_so_a_typo_does_not_silently_disable_a_guard() { + // `alert_los` is a typo for `alert_loss`; without deny_unknown_fields it + // parses fine and the loss guard stays off without warning. + let error = toml::from_str::("alert_los = 3.0\n").unwrap_err(); + assert!( + error.to_string().contains("alert_los"), + "error should name the offending key: {error}" + ); + } + #[test] fn xag_example_enables_twenty_percent_position_jump_alert() { let config: MakerFileConfig = toml::from_str(include_str!(concat!( diff --git a/crates/standx-cli/src/commands/maker/mod.rs b/crates/standx-cli/src/commands/maker/mod.rs index c9df805..eee0da9 100644 --- a/crates/standx-cli/src/commands/maker/mod.rs +++ b/crates/standx-cli/src/commands/maker/mod.rs @@ -444,6 +444,22 @@ mod tests { assert!(error.to_string().contains("stable trade ID")); } + #[test] + fn alert_thresholds_reject_silent_disable_and_unfireable_ranges() { + use runtime::validate_alert_thresholds; + // Baseline: all valid. + assert!(validate_alert_thresholds(50.0, 80.0, 20.0, 3600.0).is_ok()); + // Zero everywhere means "disabled" and is allowed. + assert!(validate_alert_thresholds(0.0, 0.0, 0.0, 0.0).is_ok()); + // Negative thresholds silently disable the guard. + assert!(validate_alert_thresholds(-1.0, 80.0, 20.0, 3600.0).is_err()); + assert!(validate_alert_thresholds(50.0, -1.0, 20.0, 3600.0).is_err()); + assert!(validate_alert_thresholds(50.0, 80.0, 20.0, -1.0).is_err()); + // Percentages above 100 can never fire. + assert!(validate_alert_thresholds(50.0, 170.0, 20.0, 3600.0).is_err()); + assert!(validate_alert_thresholds(50.0, 80.0, 170.0, 3600.0).is_err()); + } + #[test] fn webhook_body_shapes() { let txt = "🚨 ALERT [BTC-USD] loss — PnL -50 breached"; diff --git a/crates/standx-cli/src/commands/maker/notify.rs b/crates/standx-cli/src/commands/maker/notify.rs index 20cc72f..da67253 100644 --- a/crates/standx-cli/src/commands/maker/notify.rs +++ b/crates/standx-cli/src/commands/maker/notify.rs @@ -1,7 +1,10 @@ use crate::cli::{AlertWebhookFormat, OutputFormat}; -use standx_maker::{Alert, PositionAlertAnchor}; +use standx_maker::{Alert, PositionAlertAnchor, PositionRiskKind}; use std::time::Duration; +/// Number of extra attempts after the first POST fails or returns a 5xx. +const WEBHOOK_RETRIES: u32 = 2; + pub(super) fn webhook_body( format: AlertWebhookFormat, text: &str, @@ -20,18 +23,56 @@ pub(super) fn webhook_body( } async fn post_webhook(client: reqwest::Client, url: String, body: serde_json::Value) { - match client - .post(&url) - .json(&body) - .timeout(Duration::from_secs(5)) - .send() - .await - { - Ok(response) if !response.status().is_success() => { - eprintln!("⚠️ maker webhook returned {}", response.status()) + // A single POST drops the alert on a transient 5xx or timeout, so retry a + // couple of times with linear backoff before giving up. + for attempt in 0..=WEBHOOK_RETRIES { + match client + .post(&url) + .json(&body) + .timeout(Duration::from_secs(5)) + .send() + .await + { + Ok(response) if response.status().is_success() => return, + Ok(response) => { + let status = response.status(); + if attempt == WEBHOOK_RETRIES { + eprintln!("⚠️ maker webhook returned {status}"); + return; + } + eprintln!( + "⚠️ maker webhook returned {status}; retrying ({}/{})", + attempt + 1, + WEBHOOK_RETRIES + ); + } + Err(error) => { + if attempt == WEBHOOK_RETRIES { + eprintln!("⚠️ maker webhook POST failed: {error}"); + return; + } + eprintln!( + "⚠️ maker webhook POST failed: {error}; retrying ({}/{})", + attempt + 1, + WEBHOOK_RETRIES + ); + } } - Err(error) => eprintln!("⚠️ maker webhook POST failed: {error}"), - _ => {} + tokio::time::sleep(Duration::from_secs(u64::from(attempt) + 1)).await; + } +} + +/// Stable machine-readable name and delivery severity for a position-risk kind. +/// +/// `Jump` keeps its historical `position_jump` name for backward compatibility; +/// the direction flip and max-position crossings escalate to `critical` so a +/// reversal or breach is distinguishable from an ordinary threshold jump. +fn risk_kind_descriptor(kind: PositionRiskKind) -> (&'static str, &'static str) { + match kind { + PositionRiskKind::Jump => ("position_jump", "warning"), + PositionRiskKind::DirectionFlip => ("direction_flip", "critical"), + PositionRiskKind::MaxPositionCrossed => ("max_position_crossed", "critical"), + PositionRiskKind::InventoryExitCrossed => ("inventory_exit_crossed", "warning"), } } @@ -88,8 +129,14 @@ impl MakerNotifier { .position_before .zip(notice.position_after) .map(|(before, after)| after - before); + // Give the webhook the same `[severity/kind] symbol:` prefix the stderr + // line carries so a phone alert identifies its source across instances. + let text = format!( + "[{}/{}] {}: {}", + notice.severity, notice.kind, notice.symbol, notice.message + ); let raw = serde_json::json!({ - "text": notice.message, + "text": text, "ts": ts, "symbol": notice.symbol, "cycle": notice.cycle, @@ -112,7 +159,7 @@ impl MakerNotifier { notice.severity, notice.kind, notice.symbol, notice.message ); } - self.deliver(notice.message, raw, await_delivery).await; + self.deliver(&text, raw, await_delivery).await; } pub(super) async fn position_jump( @@ -131,6 +178,7 @@ impl MakerNotifier { let before = event.before; let after = event.after; let delta = event.delta; + let (kind, severity) = risk_kind_descriptor(event.kind); let attribution = if (change.observed - change.expected).abs() <= change.qty_tolerance { "current-run maker fills" } else { @@ -141,8 +189,8 @@ impl MakerNotifier { ); self.risk( RiskNotice { - kind: "position_jump", - severity: "warning", + kind, + severity, event: "detected", message: &message, symbol: change.symbol, @@ -157,7 +205,7 @@ impl MakerNotifier { .await; } - pub(super) fn alert(&self, alert: &Alert, symbol: &str) { + pub(super) async fn alert(&self, alert: &Alert, symbol: &str, await_delivery: bool) { let ts = chrono::Utc::now().to_rfc3339_opts(chrono::SecondsFormat::Secs, true); let label = if alert.firing { "🚨 ALERT" @@ -176,15 +224,12 @@ impl MakerNotifier { } else { eprintln!("{} [{}] {} — {}", label, alert.kind, symbol, alert.message); } - if let (Some(client), Some(url)) = (&self.http, &self.webhook_url) { - let text = format!("{} [{}] {} — {}", label, symbol, alert.kind, alert.message); - let raw = serde_json::json!({ - "text": text, "ts": ts, "symbol": symbol, "action": "alert", - "kind": alert.kind, "firing": alert.firing, "message": alert.message, - }); - let body = webhook_body(self.webhook_format, &text, &raw); - tokio::spawn(post_webhook(client.clone(), url.clone(), body)); - } + let text = format!("{} [{}] {} — {}", label, symbol, alert.kind, alert.message); + let raw = serde_json::json!({ + "text": text, "ts": ts, "symbol": symbol, "action": "alert", + "kind": alert.kind, "firing": alert.firing, "message": alert.message, + }); + self.deliver(&text, raw, await_delivery).await; } async fn deliver(&self, text: &str, raw: serde_json::Value, await_delivery: bool) { @@ -222,3 +267,31 @@ pub(super) struct PositionChange<'a> { pub(super) symbol: &'a str, pub(super) cycle: u64, } + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn risk_kind_descriptor_escalates_flip_and_breach_to_critical() { + // Ordinary jump keeps its historical name and warning severity. + assert_eq!( + risk_kind_descriptor(PositionRiskKind::Jump), + ("position_jump", "warning") + ); + // A reversal and a max-position breach must be distinguishable and + // escalated so they are not lost among routine jumps. + assert_eq!( + risk_kind_descriptor(PositionRiskKind::DirectionFlip), + ("direction_flip", "critical") + ); + assert_eq!( + risk_kind_descriptor(PositionRiskKind::MaxPositionCrossed), + ("max_position_crossed", "critical") + ); + assert_eq!( + risk_kind_descriptor(PositionRiskKind::InventoryExitCrossed), + ("inventory_exit_crossed", "warning") + ); + } +} diff --git a/crates/standx-cli/src/commands/maker/runtime.rs b/crates/standx-cli/src/commands/maker/runtime.rs index d350d66..221a29b 100644 --- a/crates/standx-cli/src/commands/maker/runtime.rs +++ b/crates/standx-cli/src/commands/maker/runtime.rs @@ -301,6 +301,34 @@ fn emit_startup_rejected( } } +/// Reject alert thresholds that silently defeat the guard they configure. +/// +/// A negative threshold turns the alert off without warning, and a percentage +/// above 100 can never fire; both leave an operator believing a protection is +/// armed when it is not. +pub(super) fn validate_alert_thresholds( + alert_loss: f64, + alert_inventory_pct: f64, + alert_position_change_pct: f64, + alert_uptime: f64, +) -> Result<()> { + if alert_loss < 0.0 { + return Err(anyhow::anyhow!("--alert-loss must be >= 0")); + } + if !(0.0..=100.0).contains(&alert_inventory_pct) { + return Err(anyhow::anyhow!("--alert-inventory-pct must be 0..=100")); + } + if !(0.0..=100.0).contains(&alert_position_change_pct) { + return Err(anyhow::anyhow!( + "--alert-position-change-pct must be 0..=100" + )); + } + if alert_uptime < 0.0 { + return Err(anyhow::anyhow!("--alert-uptime must be >= 0")); + } + Ok(()) +} + pub(super) async fn run_maker( symbol: String, args: MakerRunArgs, @@ -410,11 +438,12 @@ pub(super) async fn run_maker( "active inventory exit requires both --inventory-exit-pct and --inventory-exit-qty" )); } - if !(0.0..=100.0).contains(&args.alert_position_change_pct) { - return Err(anyhow::anyhow!( - "--alert-position-change-pct must be 0..=100" - )); - } + validate_alert_thresholds( + args.alert_loss, + args.alert_inventory_pct, + args.alert_position_change_pct, + args.alert_uptime, + )?; if cfg.band_bps <= cfg.spread_bps { return Err(anyhow::anyhow!( "--band-bps ({}) must be greater than --spread-bps ({}): quotes clamped to the band edge would sit exactly at the boundary", @@ -1081,6 +1110,27 @@ pub(super) async fn run_maker( order_response_reconnect_attempts_used, args.order_response_reconnect_attempts, ); + // Mirror the account-stream path: the order-response stream was + // previously silent on the webhook across disconnect/reconnect. + let disconnect_message = + format!("order-response stream unavailable; placements frozen: {detail}"); + notifier + .risk( + RiskNotice { + kind: "order_response", + severity: "warning", + event: "disconnected_frozen", + message: &disconnect_message, + symbol: &symbol, + cycle, + position_before: None, + position_after: None, + expected: Some(ledger.expected_position), + observed: None, + }, + false, + ) + .await; if reconnect_available { if let Some(handle) = order_response_handle.take() { handle.abort(); @@ -1113,6 +1163,23 @@ pub(super) async fn run_maker( adopted.clear(); pending.clear(); consecutive_errors = 0; + notifier + .risk( + RiskNotice { + kind: "order_response", + severity: "resolved", + event: "reconnected", + message: "order-response stream reconnected; maker book verified empty before quoting resumes", + symbol: &symbol, + cycle, + position_before: None, + position_after: None, + expected: Some(ledger.expected_position), + observed: None, + }, + false, + ) + .await; continue; } Err(error) => { @@ -1133,11 +1200,49 @@ pub(super) async fn run_maker( }) ); } + let reconciliation_message = format!( + "order-response reconnect failed reconciliation: {error}" + ); + notifier + .risk( + RiskNotice { + kind: "order_response", + severity: "critical", + event: "reconnect_failed", + message: &reconciliation_message, + symbol: &symbol, + cycle, + position_before: None, + position_after: None, + expected: Some(ledger.expected_position), + observed: None, + }, + true, + ) + .await; break MakerExit::PositionReconciliation(error.to_string()); } - break MakerExit::OrderResponse(format!( + let reconnect_failed_message = format!( "{detail}; safe reconnect failed: {error}; refusing further live orders" - )); + ); + notifier + .risk( + RiskNotice { + kind: "order_response", + severity: "critical", + event: "reconnect_failed", + message: &reconnect_failed_message, + symbol: &symbol, + cycle, + position_before: None, + position_after: None, + expected: Some(ledger.expected_position), + observed: None, + }, + true, + ) + .await; + break MakerExit::OrderResponse(reconnect_failed_message); } } } @@ -1152,9 +1257,26 @@ pub(super) async fn run_maker( args.order_response_reconnect_attempts ) }; - break MakerExit::OrderResponse(format!( - "{detail}; {reconnect_note}; refusing further live orders" - )); + let refuse_message = + format!("{detail}; {reconnect_note}; refusing further live orders"); + notifier + .risk( + RiskNotice { + kind: "order_response", + severity: "critical", + event: "reconnect_unavailable", + message: &refuse_message, + symbol: &symbol, + cycle, + position_before: None, + position_after: None, + expected: Some(ledger.expected_position), + observed: None, + }, + true, + ) + .await; + break MakerExit::OrderResponse(refuse_message); } } if let Some(receiver) = order_responses.as_mut() { @@ -1491,7 +1613,10 @@ pub(super) async fn run_maker( let fired = alerts.evaluate(&stats, stats.position(), mark, cfg.max_position, cycle); for alert in fired { - notifier.alert(&alert, &symbol); + // Await firing alerts so a breach raised on the final + // cycle before shutdown is not dropped with its task. + let await_delivery = alert.firing; + notifier.alert(&alert, &symbol, await_delivery).await; } } }