Skip to content

fix: wire JSON risk namespace through to Config (close PR #148 gap)#149

Merged
keitaj merged 1 commit into
mainfrom
fix/json-config-risk-namespace
May 4, 2026
Merged

fix: wire JSON risk namespace through to Config (close PR #148 gap)#149
keitaj merged 1 commit into
mainfrom
fix/json-config-risk-namespace

Conversation

@keitaj
Copy link
Copy Markdown
Owner

@keitaj keitaj commented May 4, 2026

Summary

  • Close the gap from PR feat: add JSON config file as a layered input source #148 where the JSON config layer's risk: namespace flattened correctly but the values never reached the Config class that RiskManager reads from. Operators relying on \"daily_loss_limit\": 200 in JSON would have seen no effect on the risk guardrails.
  • Wire JSON risk values through to Config.{KEY.upper()} only when the same parameter is not also supplied on the command line. CLI > JSON precedence is preserved.
  • Risk keys are popped from json_overrides after application so they do not leak into strategy_config or trigger the typo detector.

Addresses the should-fix from PR #148 review (#148 (review)), option (b).

Changes

  • bot.py: promote _RISK_PARAMS to module level. Add _apply_json_risk_overrides(json_overrides, args) that walks the list, applies each JSON value to Config when CLI is unset, and pops the key. Call it right after load_json_configs and before the existing CLI risk loop.

  • validation/strategy_validator.py: known_market_making_keys() now includes _RISK_PARAMS so the JSON typo detector treats risk keys as known when they appear at flat-top-level (e.g., mixed flat + nested forms).

  • README.md: clarify that the risk namespace targets Config, not strategy_config, and that CLI flags still beat JSON.

  • tests/test_bot_config_layering.py: 5 new cases under TestJsonRiskOverrides:

    • JSON value sets Config when CLI is unset (and pops from json_overrides)
    • CLI value beats JSON (helper is a no-op for that key)
    • Multiple risk params apply together; non-risk keys pass through untouched
    • Empty / None inputs are no-ops
    • Unknown risk-shaped keys pass through untouched

    Tests snapshot and restore the affected Config class attributes via setup_method / teardown_method so they stay isolated from each other and from the wider suite.

Test plan

  • Unit tests added: 5 new cases under TestJsonRiskOverrides
  • flake8 . --max-line-length=120 passes
  • pytest tests/ -q passes (1044 tests, 0 failures)
  • No regressions in existing tests
  • CLI > JSON precedence verified by direct test (test_cli_value_beats_json_for_risk)
  • Default behaviour (no JSON) unchanged — Config only mutated when json_overrides actually contains a recognised risk key

🤖 Generated with Claude Code

The previous JSON config layer (#148) flattened ``{"risk": {...}}`` into
flat keys correctly, but those values only flowed into ``strategy_config``
— the strategy-level merged dict — not into the ``Config`` class that
``RiskManager`` actually reads from. Operators putting
``"daily_loss_limit": 200`` in their JSON would see no effect on the
risk guardrails.

This change closes that gap:

* Promote ``_RISK_PARAMS`` from ``main()`` to module level so the JSON
  helper and the CLI override loop share the list.
* Add ``_apply_json_risk_overrides(json_overrides, args)``: for each
  risk param present in JSON and *not* already supplied via CLI args,
  write the value to ``Config.{KEY.upper()}``. Pop the key from
  ``json_overrides`` so it does not leak into ``strategy_config``
  (which the strategy would not know what to do with) or trigger the
  typo detector.
* Call the helper right after ``load_json_configs`` and before the
  existing CLI risk loop. CLI > JSON precedence is preserved because
  the CLI loop runs last and unconditionally writes ``args.{param}``
  when present.
* ``known_market_making_keys()`` now includes ``_RISK_PARAMS`` so the
  JSON loader's typo detector treats risk keys as known when they
  appear at flat-top-level (e.g., a user mixing flat + nested forms).
* README clarifies that the ``risk`` namespace targets ``Config``,
  not ``strategy_config``, and that CLI flags still beat JSON.

5 new test cases pin the behaviour: JSON sets Config when CLI is
unset; CLI beats JSON; multiple risk params apply together; empty /
None inputs are no-ops; unknown risk-shaped keys pass through
untouched. Tests snapshot and restore the relevant ``Config`` class
attributes so they stay isolated from each other and from the wider
suite.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Copy link
Copy Markdown
Owner Author

@keitaj keitaj left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review: Wire JSON risk namespace through to Config

Surgical follow-up to the should-fix from PR #148 review, executed cleanly. _RISK_PARAMS is properly deduplicated (now defined once at module level, removed from the validator's "extras" set since it now flows through _RISK_PARAMS), the helper preserves CLI > JSON precedence by checking args.{param} instead of Config, and the popped-after-apply semantic prevents both strategy_config pollution and false typo warnings. The 5 new tests snapshot Config attributes per-method so they stay isolated. No blockers; 2 nits below.


1. JSON ints land on Config.* as int, not declared Optional[float] (nit)

bot.py L75 and config.py:

Config.DAILY_LOSS_LIMIT (and most risk attrs) are declared as Optional[float] and read from os.getenv(...) via float(...). When _apply_json_risk_overrides does setattr(Config, 'DAILY_LOSS_LIMIT', value) with a value parsed from JSON, the value's type follows JSON syntax — 200 is int, 200.0 is float. Comparisons against floats work fine in Python (1.5 < 200 is True), so this isn't a runtime correctness issue, but the attribute briefly violates its declared type during the bot's lifetime.

Suggestion (optional): coerce floats explicitly for the float-shaped risk params:

_FLOAT_RISK = {'max_position_pct', 'max_margin_usage', 'force_close_margin',
               'force_close_leverage', 'daily_loss_limit', 'per_trade_stop_loss'}
_INT_RISK   = {'max_open_positions', 'cooldown_after_stop'}
...
value = json_overrides.pop(param)
if param in _FLOAT_RISK:
    value = float(value)
elif param in _INT_RISK:
    value = int(value)
setattr(Config, param.upper(), value)

Or accept the looser typing for now and tighten if it matters later.


2. Mixed flat + nested for the same risk key — last wins by dict ordering (nit)

json_config_loader.py (out of scope for this PR but exposed by the new behaviour):

A user could write:

{
  "daily_loss_limit": 300,
  "risk": { "daily_loss_limit": 200 }
}

Both flatten to the same key (daily_loss_limit), and the resulting dict has one value — whichever JSON property was iterated last (insertion order, which mirrors the file order). The current helper applies whatever survives the merge, so behaviour is deterministic but possibly surprising.

Suggestion (optional): in a future PR, add a sanity check inside _to_flat that warns when both forms set the same key with different values, similar to the unknown-key warning. Not necessary now — operators using the example template won't hit this.


Verdict: LGTM

No blockers, no should-fix. The risk-namespace gap from PR #148 is closed cleanly with proper test coverage and precedence semantics. Two nits are minor polish items — neither affects correctness or default behaviour. Ready to merge.

🤖 Generated with Claude Code

@keitaj keitaj merged commit 2edad25 into main May 4, 2026
6 checks passed
@keitaj keitaj deleted the fix/json-config-risk-namespace branch May 4, 2026 02:23
keitaj added a commit that referenced this pull request May 4, 2026
PR #149 promoted ``_RISK_PARAMS`` to module scope so
``validation.strategy_validator.known_market_making_keys`` could
introspect risk-guardrail names lazily. The other two lists
(``_STRATEGY_PARAMS``, ``_COMMON_PARAMS``) were left as locals inside
``main()``, so the lazy import in ``known_market_making_keys`` would
raise ``ImportError`` at runtime as soon as the JSON config typo
detector tried to enumerate known MM keys.

This PR moves both lists out of ``main()`` to module scope right
after ``_RISK_PARAMS``. ``main()`` keeps a short comment pointing
readers at the new location; the surrounding ``_collect_params`` /
``argparse.Namespace`` call-sites are unchanged.

A regression test class is added to ``test_strategy_validator.py`` —
it pins the contract that all three name lists remain at module
level (any future demotion will fail the import-style assertions).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant