Skip to content

fix(reporting): resolve account kind from config; never guess LIVE - #11

Open
davdunc wants to merge 2 commits into
mainfrom
fix/account-kind-resolution
Open

fix(reporting): resolve account kind from config; never guess LIVE#11
davdunc wants to merge 2 commits into
mainfrom
fix/account-kind-resolution

Conversation

@davdunc

@davdunc davdunc commented Aug 29, 2026

Copy link
Copy Markdown
Owner

Fixes the 3 tests that have been failing on main, and the bug behind them.

The bug

reporting/ingest.py:

DEFAULT_ACCOUNT_KINDS: dict[str, AccountKind] = {}
# Load account mappings from environment or config file instead of hardcoding

Stripping broker account ids out of a public repo was the right call. The replacement loader was never written — so ingest.py fell through to kinds.get(account, AccountKind.LIVE) and every account resolved to LIVE, including a simulated book.

The empty dict is the visible symptom. The AccountKind.LIVE default is the dangerous part. LIVE and SIM carry independent risk units, and a simulated book scored against live risk is the single error this layer must not make. A missing config entry should never be able to produce it.

The fix

default_account_kinds() resolves, first hit wins:

  1. $TRADEKIT_ACCOUNT_KINDS"1RB16917=LIVE,TR4425=SIM"
  2. [accounts] in accounts_config()~/.config/tradekit/accounts.toml

Resolved per call rather than cached at import, so a caller or test that sets the environment is honoured. Malformed pairs and unknown kinds are skipped rather than raising: a typo in one entry must not take down a whole review.

The LIVE fallback is gone. An unmapped account resolves to None and renders as UNMAPPED (<id>):

── no mapping configured
| **UNMAPPED (1RB16917)** | 4 | 75% (3W/1L) | $+240.50 | … |
| **UNMAPPED (TR4425)**   | 2 | 50% (1W/1L) | $-15.00  | … |

── with TRADEKIT_ACCOUNT_KINDS set
| **LIVE (1RB16917)** | 4 | 75% (3W/1L) | $+240.50 | … |
| **SIM (TR4425)**    | 2 | 50% (1W/1L) | $-15.00  | … |

A visibly wrong label is recoverable. A plausible wrong one is not.

  • AccountPnL.kind is now Optional, making "unknown" a real state rather than something the schema forces you to fake.
  • render_daily_card still emits a row for an unmapped account — dropping it would hide real P&L behind a missing config entry.
  • paths.py gains xdg_config_home() / config_dir() / accounts_config(). paths.py owns every on-disk location, so the new one belongs there.
  • config/accounts.toml.example documents the format and states plainly that real ids are never committed.
  • DEFAULT_ACCOUNT_KINDS is kept as an inert, documented alias — it is exported from tradekit.reporting, so removing it would break importers.

Tests

90 passed, up from 80 passed / 3 failed.

The three TestIngest failures are fixed by supplying the mapping through an autouse env fixture, which also makes them hermetic — they no longer depend on whether the developer happens to have ~/.config/tradekit/accounts.toml.

Seven new tests in TestAccountKindResolution cover env parsing, malformed input, TOML loading, a missing config file, explicit per-record kind precedence, and the two behaviours that matter most: an unmapped account is never LIVE, and it still renders.

Notes

🤖 Generated with Claude Code

https://claude.ai/code/session_01BcNHYKg3TET8SzbFdyZPZb

DEFAULT_ACCOUNT_KINDS was {} with the comment "Load account mappings from
environment or config file instead of hardcoding". Stripping broker account ids
out of a public repo was right; the replacement loader was never written. So
ingest.py fell through to `kinds.get(account, AccountKind.LIVE)` and every
account — including a simulated book — was reported as LIVE. Three tests in
tests/test_reporting.py have been failing on main since.

That default is the dangerous part. LIVE and SIM carry independent risk units,
and a simulated book scored against live risk is the one error this layer must
not make. A missing config entry should never be able to produce it.

- default_account_kinds() resolves $TRADEKIT_ACCOUNT_KINDS first
  ("ID=LIVE,ID=SIM"), then [accounts] in accounts_config(). Resolved per call,
  not cached at import, so callers and tests that set the environment are
  honoured. Malformed pairs and unknown kinds are skipped rather than raising —
  a typo in one entry must not take down a review.
- The LIVE fallback is gone. An unmapped account resolves to None and renders
  as UNMAPPED (<id>). A visibly wrong label is recoverable; a plausible wrong
  one is not. AccountPnL.kind is now Optional to make "unknown" a real state
  rather than something the schema forces you to fake.
- render_daily_card still emits a row for an unmapped account. Dropping it
  would hide real P&L behind a missing config entry.
- paths.py gains xdg_config_home() / config_dir() / accounts_config(); paths.py
  owns every on-disk location, so the new one belongs there too.
- config/accounts.toml.example documents the format and says plainly that real
  ids never get committed.

DEFAULT_ACCOUNT_KINDS is kept as an inert, documented alias — it is exported
from tradekit.reporting and removing it would break importers.

Tests: the three TestIngest failures are fixed by supplying the mapping through
an autouse env fixture, which also makes them hermetic — they no longer depend
on whether the developer has ~/.config/tradekit/accounts.toml. Seven new tests
in TestAccountKindResolution cover env parsing, malformed input, TOML loading, a
missing config file, explicit per-record kind precedence, and the two behaviours
that matter most: an unmapped account is never LIVE, and it still renders.

90 passed, up from 80 passed / 3 failed. Fixes gap G8 in docs/SPEC.md (#10).

@amazon-q-developer amazon-q-developer Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Summary

This PR successfully addresses a critical security risk where unmapped accounts defaulted to LIVE, potentially misclassifying simulated books against live risk. The fix implements proper account kind resolution from environment variables and config files, eliminating the dangerous fallback behavior.

Critical Issue Found

Syntax Error (Line 86): Exception handling uses Python 2 comma syntax instead of Python 3 parentheses. This prevents compilation and must be fixed before merge.

Implementation Notes

The core fix is sound:

  • Account kinds now resolve from TRADEKIT_ACCOUNT_KINDS env var or ~/.config/tradekit/accounts.toml
  • Unmapped accounts render as "UNMAPPED" instead of silently defaulting to LIVE
  • Schema correctly models unknown state with AccountKind | None
  • Test coverage is comprehensive with 7 new tests validating the resolution logic

Once the syntax error is corrected, this PR properly closes the risk model gap.


You can now have the agent implement changes and create commits directly on your pull request's source branch. Simply comment with /q followed by your request in natural language to ask the agent to make changes.

Comment thread src/tradekit/reporting/ingest.py Outdated
Co-authored-by: amazon-q-developer[bot] <208079219+amazon-q-developer[bot]@users.noreply.github.com>
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