From a33686c7f71b08588a4c36193d8d9bb562745d07 Mon Sep 17 00:00:00 2001 From: "coderabbitai[bot]" <136622811+coderabbitai[bot]@users.noreply.github.com> Date: Thu, 14 May 2026 05:19:55 +0000 Subject: [PATCH] CodeRabbit Generated Unit Tests: Add unit tests for PR changes --- .../src-python/tests/test_config_security.py | 288 ++++++++++++++++++ .../tests/test_trading_signal_amount.py | 271 ++++++++++++++++ 2 files changed, 559 insertions(+) create mode 100644 money-machine/src-python/tests/test_config_security.py create mode 100644 money-machine/src-python/tests/test_trading_signal_amount.py diff --git a/money-machine/src-python/tests/test_config_security.py b/money-machine/src-python/tests/test_config_security.py new file mode 100644 index 0000000..d3d814f --- /dev/null +++ b/money-machine/src-python/tests/test_config_security.py @@ -0,0 +1,288 @@ +from __future__ import annotations + +import json +import sys +from pathlib import Path + +import pytest + +SRC_PYTHON = Path(__file__).resolve().parent.parent +if str(SRC_PYTHON) not in sys.path: + sys.path.insert(0, str(SRC_PYTHON)) + +from engine.trading_core import validate_config_update # noqa: E402 +from utils import config as config_module # noqa: E402 + + +def test_save_config_redacts_nested_secrets(tmp_path, monkeypatch) -> None: + fake_config_module_path = tmp_path / "utils" / "config.py" + fake_config_module_path.parent.mkdir() + fake_config_module_path.write_text("") + monkeypatch.setattr(config_module, "__file__", str(fake_config_module_path)) + + assert config_module.save_config( + { + "exchange": { + "name": "binance", + "api_key": "should-not-hit-disk", + "secret": "should-not-hit-disk", + }, + "gemini_api_key": "also-secret", + "max_risk_per_trade": 0.02, + } + ) + + persisted = json.loads((tmp_path / "config.json").read_text()) + assert persisted == { + "exchange": {"name": "binance"}, + "max_risk_per_trade": 0.02, + } + + +def test_load_config_ignores_disk_secrets(tmp_path, monkeypatch) -> None: + fake_config_module_path = tmp_path / "utils" / "config.py" + fake_config_module_path.parent.mkdir() + fake_config_module_path.write_text("") + (tmp_path / "config.json").write_text( + json.dumps( + { + "exchange": {"api_key": "disk-key", "secret": "disk-secret"}, + "gemini_api_key": "disk-gemini", + "max_risk_per_trade": 0.03, + } + ) + ) + monkeypatch.setattr(config_module, "__file__", str(fake_config_module_path)) + + loaded = config_module.load_config() + assert loaded["max_risk_per_trade"] == 0.03 + assert loaded["gemini_api_key"] == "" + assert loaded["exchange"]["api_key"] == "" + assert loaded["exchange"]["secret"] == "" + + +def test_validate_config_update_rejects_secrets_and_unknown_keys() -> None: + with pytest.raises(ValueError): + validate_config_update({"exchange": {"api_key": "nope"}}) + with pytest.raises(ValueError): + validate_config_update({"gemini_model": "gemini-1.5-flash"}) + with pytest.raises(ValueError): + validate_config_update({"unsafe": True}) + + +def test_validate_config_update_enforces_ranges() -> None: + assert validate_config_update({"max_risk_per_trade": 0.02}) == { + "max_risk_per_trade": 0.02 + } + with pytest.raises(ValueError): + validate_config_update({"max_risk_per_trade": 0.5}) + with pytest.raises(ValueError): + validate_config_update({"initial_balance": 50}) + with pytest.raises(ValueError): + validate_config_update({"max_daily_loss": 0.0}) + + +# --------------------------------------------------------------------------- +# Additional validate_config_update tests — boundary conditions +# --------------------------------------------------------------------------- + + +def test_validate_config_update_rejects_non_dict_input() -> None: + """Non-dict inputs must always raise ValueError.""" + with pytest.raises(ValueError): + validate_config_update("initial_balance=5000") # type: ignore[arg-type] + with pytest.raises(ValueError): + validate_config_update(["initial_balance", 5000]) # type: ignore[arg-type] + with pytest.raises(ValueError): + validate_config_update(None) # type: ignore[arg-type] + + +def test_validate_config_update_empty_dict_returns_empty() -> None: + """An empty update is a no-op and should return an empty dict.""" + assert validate_config_update({}) == {} + + +def test_validate_config_update_rejects_bool_values() -> None: + """Boolean values must be rejected even for otherwise valid keys, + because `isinstance(True, int)` is True in Python and would pass + a naive numeric check.""" + with pytest.raises(ValueError): + validate_config_update({"max_risk_per_trade": True}) + with pytest.raises(ValueError): + validate_config_update({"initial_balance": False}) + with pytest.raises(ValueError): + validate_config_update({"max_daily_loss": True}) + + +def test_validate_config_update_accepts_integer_values() -> None: + """Integer literals must be accepted and coerced to float.""" + result = validate_config_update({"initial_balance": 5000}) + assert result == {"initial_balance": 5000.0} + assert isinstance(result["initial_balance"], float) + + +def test_validate_config_update_initial_balance_inclusive_min() -> None: + """initial_balance has an inclusive lower bound of 100.0.""" + assert validate_config_update({"initial_balance": 100.0}) == { + "initial_balance": 100.0 + } + with pytest.raises(ValueError): + validate_config_update({"initial_balance": 99.99}) + + +def test_validate_config_update_initial_balance_inclusive_max() -> None: + """initial_balance has an inclusive upper bound of 1_000_000.0.""" + assert validate_config_update({"initial_balance": 1_000_000.0}) == { + "initial_balance": 1_000_000.0 + } + with pytest.raises(ValueError): + validate_config_update({"initial_balance": 1_000_001.0}) + + +def test_validate_config_update_max_risk_exclusive_min() -> None: + """max_risk_per_trade has an exclusive lower bound of 0.0 (must be > 0).""" + with pytest.raises(ValueError): + validate_config_update({"max_risk_per_trade": 0.0}) + + +def test_validate_config_update_max_risk_inclusive_max() -> None: + """max_risk_per_trade accepts its maximum value of 0.1 exactly.""" + assert validate_config_update({"max_risk_per_trade": 0.1}) == { + "max_risk_per_trade": 0.1 + } + with pytest.raises(ValueError): + validate_config_update({"max_risk_per_trade": 0.101}) + + +def test_validate_config_update_max_daily_loss_inclusive_max() -> None: + """max_daily_loss accepts its maximum value of 0.2 exactly.""" + assert validate_config_update({"max_daily_loss": 0.2}) == { + "max_daily_loss": 0.2 + } + with pytest.raises(ValueError): + validate_config_update({"max_daily_loss": 0.21}) + + +def test_validate_config_update_all_valid_keys_together() -> None: + """All three supported keys can be updated in a single call.""" + result = validate_config_update( + { + "initial_balance": 20_000.0, + "max_risk_per_trade": 0.05, + "max_daily_loss": 0.10, + } + ) + assert result == { + "initial_balance": 20_000.0, + "max_risk_per_trade": 0.05, + "max_daily_loss": 0.10, + } + # All values must be floats regardless of input type. + for v in result.values(): + assert isinstance(v, float) + + +def test_validate_config_update_rejects_mix_of_valid_and_invalid_keys() -> None: + """If any key is unsupported the whole update must be rejected. + Partial application of a multi-key update would leave the config + in an inconsistent state. + """ + with pytest.raises(ValueError): + validate_config_update( + {"max_risk_per_trade": 0.01, "gemini_api_key": "secret"} + ) + + +# --------------------------------------------------------------------------- +# Additional config.py tests — _without_secrets with list values +# --------------------------------------------------------------------------- + + +def test_save_config_redacts_secrets_inside_lists(tmp_path, monkeypatch) -> None: + """_without_secrets must recurse into list elements so a config + that stores exchange credentials in a list is still sanitised.""" + fake_config_module_path = tmp_path / "utils" / "config.py" + fake_config_module_path.parent.mkdir() + fake_config_module_path.write_text("") + monkeypatch.setattr(config_module, "__file__", str(fake_config_module_path)) + + config_module.save_config( + { + "exchanges": [ + {"name": "binance", "api_key": "key-a", "secret": "sec-a"}, + {"name": "kraken", "api_key": "key-b", "secret": "sec-b"}, + ], + } + ) + + persisted = json.loads((tmp_path / "config.json").read_text()) + assert persisted == { + "exchanges": [ + {"name": "binance"}, + {"name": "kraken"}, + ] + } + + +def test_save_config_preserves_non_secret_data(tmp_path, monkeypatch) -> None: + """Non-secret fields must survive the redaction pass unchanged.""" + fake_config_module_path = tmp_path / "utils" / "config.py" + fake_config_module_path.parent.mkdir() + fake_config_module_path.write_text("") + monkeypatch.setattr(config_module, "__file__", str(fake_config_module_path)) + + config_module.save_config( + { + "initial_balance": 5000.0, + "max_risk_per_trade": 0.01, + "exchange": {"name": "bybit", "sandbox": True}, + } + ) + + persisted = json.loads((tmp_path / "config.json").read_text()) + assert persisted["initial_balance"] == 5000.0 + assert persisted["max_risk_per_trade"] == 0.01 + assert persisted["exchange"]["name"] == "bybit" + assert persisted["exchange"]["sandbox"] is True + + +# --------------------------------------------------------------------------- +# TradingEngine.update_config integration with validate_config_update +# --------------------------------------------------------------------------- + + +def test_trading_engine_update_config_accepts_valid_values() -> None: + """update_config() should persist a validated value into the engine config.""" + import asyncio + + from engine.trading_core import TradingEngine + + engine = TradingEngine({"initial_balance": 10_000.0, "max_risk_per_trade": 0.02}) + asyncio.run(engine.update_config({"max_risk_per_trade": 0.05})) + assert engine.config["max_risk_per_trade"] == 0.05 + + +def test_trading_engine_update_config_rejects_invalid_values() -> None: + """update_config() must propagate ValueError for out-of-range values + and leave the engine config unchanged.""" + import asyncio + + from engine.trading_core import TradingEngine + + engine = TradingEngine({"initial_balance": 10_000.0, "max_risk_per_trade": 0.02}) + with pytest.raises(ValueError): + asyncio.run(engine.update_config({"max_risk_per_trade": 0.99})) + # Config must remain unchanged. + assert engine.config["max_risk_per_trade"] == 0.02 + + +def test_trading_engine_update_config_rejects_unknown_keys() -> None: + """update_config() must not allow arbitrary key injection.""" + import asyncio + + from engine.trading_core import TradingEngine + + engine = TradingEngine({"initial_balance": 10_000.0}) + with pytest.raises(ValueError): + asyncio.run(engine.update_config({"exchange": {"api_key": "injected"}})) + assert "exchange" not in engine.config or "api_key" not in engine.config.get("exchange", {}) diff --git a/money-machine/src-python/tests/test_trading_signal_amount.py b/money-machine/src-python/tests/test_trading_signal_amount.py new file mode 100644 index 0000000..fc23882 --- /dev/null +++ b/money-machine/src-python/tests/test_trading_signal_amount.py @@ -0,0 +1,271 @@ +""" +Tests for changes introduced in the 'harden desktop security' PR: + + 1. TradingSignal.amount field — added to engine/strategies/base.py. + 2. signal_generator.py — TradingSignal is now imported from + engine.strategies.base (the local duplicate was removed), and + _parse_json_response maps 'amount_pct' from the AI response to + the signal's `amount` field. + +Scope is limited to the code that was modified in this PR. The +surrounding Strategy / SignalGenerator behaviour is pre-existing and +tested elsewhere. +""" + +from __future__ import annotations + +import json +import sys +from pathlib import Path +from typing import List + +import pytest + +SRC_PYTHON = Path(__file__).resolve().parent.parent +if str(SRC_PYTHON) not in sys.path: + sys.path.insert(0, str(SRC_PYTHON)) + +from engine.strategies.base import TradingSignal # noqa: E402 +from engine.signal_generator import SignalGenerator # noqa: E402 + + + +# --------------------------------------------------------------------------- +# TradingSignal.amount field +# --------------------------------------------------------------------------- + + +def test_trading_signal_amount_defaults_to_none() -> None: + """The new `amount` field must be Optional and default to None so that + existing code that does not pass an amount continues to work.""" + signal = TradingSignal(symbol="BTC/USDT", action="HOLD", confidence=0.0) + assert signal.amount is None + + +def test_trading_signal_amount_can_be_set() -> None: + """Callers that explicitly pass amount= must be able to read it back.""" + signal = TradingSignal( + symbol="BTC/USDT", + action="BUY", + confidence=0.75, + amount=0.02, + ) + assert signal.amount == pytest.approx(0.02) + + +def test_trading_signal_amount_zero_is_allowed() -> None: + """Zero is a valid explicit amount (signals a 0-size order intent).""" + signal = TradingSignal( + symbol="ETH/USDT", + action="SELL", + confidence=0.5, + amount=0.0, + ) + assert signal.amount == 0.0 + + +def test_trading_signal_to_dict_includes_amount() -> None: + """to_dict() must serialise the amount field so IPC consumers receive it.""" + signal = TradingSignal( + symbol="BTC/USDT", + action="BUY", + confidence=0.8, + amount=0.015, + ) + d = signal.to_dict() + assert "amount" in d + assert d["amount"] == pytest.approx(0.015) + + +def test_trading_signal_to_dict_amount_none_when_not_set() -> None: + """When amount is not provided, to_dict() must include the key with None.""" + signal = TradingSignal(symbol="BTC/USDT", action="HOLD", confidence=0.0) + d = signal.to_dict() + assert "amount" in d + assert d["amount"] is None + + +def test_trading_signal_amount_position_in_field_order() -> None: + """Verify that amount sits between take_profit and reasoning as specified.""" + signal = TradingSignal( + symbol="BTC/USDT", + action="BUY", + confidence=0.7, + entry_price=50_000.0, + stop_loss=49_000.0, + take_profit=52_000.0, + amount=0.01, + reasoning="test", + ) + assert signal.take_profit == pytest.approx(52_000.0) + assert signal.amount == pytest.approx(0.01) + assert signal.reasoning == "test" + + +# --------------------------------------------------------------------------- +# TradingSignal import in signal_generator.py +# --------------------------------------------------------------------------- + + +def test_signal_generator_imports_trading_signal_from_base() -> None: + """signal_generator.py must import TradingSignal from + engine.strategies.base, not define its own copy.""" + import engine.signal_generator as sg_module + + # The module must not define TradingSignal itself. + assert not hasattr(sg_module, "TradingSignal") or ( + sg_module.TradingSignal is TradingSignal + ), ( + "signal_generator defines its own TradingSignal instead of " + "importing from engine.strategies.base" + ) + + +def test_signal_generator_trading_signal_is_same_class() -> None: + """The TradingSignal used by the generator must be the canonical one + from engine.strategies.base so that downstream consumers (risk shield, + pipeline) receive the right type.""" + import engine.signal_generator as sg_module + import importlib + + # Reload to get a fresh reference, unaffected by other test ordering. + importlib.reload(sg_module) + from engine.signal_generator import SignalGenerator as SG + from engine.strategies.base import TradingSignal as TS + + gen = SG(api_key="") + # Verify that the fallback rule-based path returns the canonical type. + minimal_data = [[i * 1000, 50000 + i, 50100 + i, 49900 + i, 50050 + i, 100] for i in range(25)] + import asyncio + signal = asyncio.run( + gen.generate_signal("BTC/USDT", minimal_data, portfolio_balance=10_000.0) + ) + assert isinstance(signal, TS) + + +# --------------------------------------------------------------------------- +# SignalGenerator._parse_json_response — amount_pct mapping +# --------------------------------------------------------------------------- + + +def _make_market_data(n: int = 5, price: float = 50_000.0) -> List[List]: + """Return minimal OHLCV rows for parse tests.""" + return [[i * 1_000, price, price + 100, price - 100, price, 100] for i in range(n)] + + +def test_parse_json_response_maps_amount_pct_to_amount() -> None: + """_parse_json_response must read 'amount_pct' from the AI JSON and + store it in TradingSignal.amount so position sizers can use it.""" + gen = SignalGenerator(api_key="") + market_data = _make_market_data() + + response_json = json.dumps( + { + "action": "BUY", + "confidence": 0.8, + "entry_price": 50_000.0, + "stop_loss": 49_000.0, + "take_profit": 52_000.0, + "amount_pct": 0.015, + "reasoning": "strong momentum", + } + ) + + signal = gen._parse_json_response("BTC/USDT", response_json, market_data) + + assert signal.action == "BUY" + assert signal.confidence == pytest.approx(0.8) + assert signal.amount == pytest.approx(0.015) + + +def test_parse_json_response_amount_is_none_when_key_absent() -> None: + """If 'amount_pct' is absent from the AI response, signal.amount must + be None (not crash or default to a hard-coded value).""" + gen = SignalGenerator(api_key="") + market_data = _make_market_data() + + response_json = json.dumps( + { + "action": "SELL", + "confidence": 0.6, + "reasoning": "overbought", + } + ) + + signal = gen._parse_json_response("BTC/USDT", response_json, market_data) + + assert signal.action == "SELL" + assert signal.amount is None + + +def test_parse_json_response_amount_pct_null_maps_to_none() -> None: + """An explicit null/None in the AI response must also yield amount=None.""" + gen = SignalGenerator(api_key="") + market_data = _make_market_data() + + response_json = json.dumps( + { + "action": "HOLD", + "confidence": 0.3, + "amount_pct": None, + "reasoning": "unclear", + } + ) + + signal = gen._parse_json_response("BTC/USDT", response_json, market_data) + assert signal.amount is None + + +def test_parse_json_response_falls_back_on_invalid_json() -> None: + """If the AI returns garbage, _parse_json_response must return a HOLD + signal rather than raising.""" + gen = SignalGenerator(api_key="") + market_data = _make_market_data() + + signal = gen._parse_json_response("BTC/USDT", "this is not json {{{{", market_data) + + assert signal.action == "HOLD" + assert isinstance(signal, TradingSignal) + + +def test_parse_json_response_invalid_action_falls_back_to_hold() -> None: + """An unrecognised action string in the AI response must not crash + _parse_json_response; a HOLD fallback should be returned instead.""" + gen = SignalGenerator(api_key="") + market_data = _make_market_data() + + response_json = json.dumps( + { + "action": "YOLO", + "confidence": 0.9, + "amount_pct": 0.02, + "reasoning": "moon", + } + ) + + # TradingSignal.__post_init__ raises ValueError for invalid actions, + # so _parse_json_response must catch that and return a HOLD. + signal = gen._parse_json_response("BTC/USDT", response_json, market_data) + assert signal.action == "HOLD" + + +# --------------------------------------------------------------------------- +# Regression: amount field round-trips through to_dict / constructor +# --------------------------------------------------------------------------- + + +def test_trading_signal_round_trip_with_amount() -> None: + """A signal reconstructed from to_dict() must preserve the amount.""" + original = TradingSignal( + symbol="ETH/USDT", + action="BUY", + confidence=0.65, + entry_price=3_000.0, + amount=0.02, + reasoning="round-trip test", + ) + d = original.to_dict() + reconstructed = TradingSignal(**d) + assert reconstructed.amount == pytest.approx(0.02) + assert reconstructed.symbol == "ETH/USDT" + assert reconstructed.reasoning == "round-trip test"