From 8e787b79144b1001f3afd7873830b5d052ca3fab Mon Sep 17 00:00:00 2001 From: Elmehdi Aitbrahim Date: Fri, 4 Sep 2026 04:12:47 -0400 Subject: [PATCH 1/3] feat(engine): the equity the agent computed every cycle and threw away (#698) `agent._mark_to_market_equity` has always run once per cycle, and nothing kept the answer. What survived was `agent_state["equity_history"]`: a 7-day window that exists for the weekly drawdown rail and that `record_external_flow` REWRITES on a declared deposit. That is a rail's working set, not a record -- so the series is written alongside it rather than derived from it, and neither reconstructs the other. `equity_points` (schema v19) is the record. Per row: the reading, the mode that produced it, and the high-water mark in force AFTER it was folded in -- read back by the chart rather than recomputed, because a rebased HWM is not the running maximum of the equity and the overlay's whole point is the ceiling rail 11 actually had. `mode` is the load-bearing partition, taken from the same `equity_state_mode` stamp `_clear_live_mode_if_needed` reads before wiping the shared HWM on a flip. Deriving it a second way at the call site would let two answers to one question drift, and the failure is silent: a mislabelled row does not go missing, it lands in the other account's curve. An UNSTAMPED mode writes nothing and never guesses -- and never raises, so the chart can never take rail 11 down with it. `cash` and `unrealized` are the split, and they are passed, never derived: this function has a total and no positions. NULL means "not recorded", never zero -- the `orders.filled_quantity` convention. Both agent branches now supply them from ONE pass over ONE set of balances and marks (`_mark_to_market_parts`, `PaperTrader.unrealized`), so a price cannot tick between the total and its parts and file a row that does not reconcile. `unrealized_on_marks` mirrors `mark_positions` guard for guard: a position valued at cost contributes zero, because the equity in the same row valued it at cost too. Tested as an identity: cash + cost basis + unrealized == equity. NO BACKFILL, and what could be backfilled is exactly what must not be. Replaying the shifted 7-day window would publish equities the account never had; the orders ledger would give a closed-trade curve with no cash leg, a different quantity wearing this table's name. An empty table means "not observed before v19", which is true. `build_equity_series` is a NEW builder, not a change to `build_equity_curve`. That one plots cumulative net P&L over closed trades and argues for its trade-order axis on its own terms -- correctly, for that quantity. This plots what the account was worth whether or not it traded, where the gaps between cycles are the information. Segmented by RUNS of mode (paper, live, paper is three), bounds spanning the overlays so a rail floor cannot fall off the box, and zero deliberately NOT forced onto the axis: an account is not up or down against nothing. Three departures from the issue text: - v19, not v16: main reached 18 after the issue was filed. - `ts INTEGER`, not TEXT: every timestamp in this schema is an epoch int, and TEXT orders epochs lexically -- the `ts >= ?` window the chart reads by would silently return the wrong rows across a digit-count change. Pinned by a test. - the record is `EquityReading`: `keel.commands.insights` already owns an `EquityPoint`, a PLOT point, and the chart module now uses both. Still to come on #698: the payload/template/chart.js wiring that puts the series on the Insights page. Co-Authored-By: Claude Opus 5 (1M context) --- keel/agent.py | 76 ++++++++++- keel/commands/insights.py | 186 ++++++++++++++++++++++++++ keel/data/db.py | 44 +++++- keel/data/repository.py | 73 +++++++++- keel/execution/equity.py | 103 +++++++++++++- keel/strategy/paper.py | 15 +++ packages/keel-core/keel_core/types.py | 30 ++++- tests/commands/test_equity_series.py | 185 +++++++++++++++++++++++++ tests/data/test_db.py | 4 +- tests/data/test_equity_points.py | 135 +++++++++++++++++++ tests/data/test_migrations.py | 8 +- tests/data/test_trade_outcomes.py | 4 +- tests/execution/test_equity.py | 92 +++++++++++++ tests/execution/test_paper_equity.py | 87 +++++++++++- tests/strategy/test_paper.py | 46 +++++++ tests/test_agent.py | 68 ++++++++++ 16 files changed, 1136 insertions(+), 20 deletions(-) create mode 100644 tests/commands/test_equity_series.py create mode 100644 tests/data/test_equity_points.py diff --git a/keel/agent.py b/keel/agent.py index 1aaf6f28..e7aa66e3 100644 --- a/keel/agent.py +++ b/keel/agent.py @@ -415,6 +415,22 @@ def _position_state(repo: Repository, product_id: str) -> dict[str, Any] | None: return raw +@dataclass(frozen=True) +class _EquityParts: + """One read of the live account, split into the pieces `equity_points` records (#698). + + `equity` is the whole reading; `cash` and `unrealized` are two of its legs (the third, the + cost basis of open positions, is implied: `cash + cost + unrealized == equity`). They come + out of ONE pass over ONE set of balances and marks, which is the point -- reading the total + and the split separately would let a price tick between them and file a row that does not + reconcile. + """ + + equity: Decimal + cash: Decimal + unrealized: Decimal + + def _mark_to_market_equity( repo: Repository, broker: Any, @@ -425,6 +441,9 @@ def _mark_to_market_equity( """Quote balance + mark-to-market value of every open position, or `None` if the quote balance could not be read. + The scalar face of `_mark_to_market_parts`, kept because most callers want only the total. + Every word below describes that function's single pass; nothing is computed twice. + Unrealized P&L is included on purpose: a breaker that saw only realized P&L would read 0% while a position bled and would notice only after the loss was booked -- backwards for a circuit breaker, which has to fire WHILE you are losing. @@ -452,6 +471,27 @@ def _mark_to_market_equity( simply unknowable then, and a wrong one corrupts the high-water mark PERMANENTLY (an HWM never falls, so an under-read arms the breaker on a phantom drawdown from then on). """ + parts = _mark_to_market_parts(repo, broker, products, price_by_product, quote_currency) + return None if parts is None else parts.equity + + +def _mark_to_market_parts( + repo: Repository, + broker: Any, + products: list[str], + price_by_product: dict[str, Decimal], + quote_currency: str, +) -> _EquityParts | None: + """`_mark_to_market_equity`'s single pass, returning the split as well as the total (#698). + + See that function's docstring for every rule this implements -- the FX bound, the + cost-basis fallback, the product union, and why an unreadable balance is `None` rather than + a partial total. This is the same computation; it just keeps the legs it was already + computing instead of discarding them. + + `unrealized` follows `equity.unrealized_on_marks`: a position valued at cost (no fresh + price) contributes ZERO, because the equity in the same record valued it at cost too. + """ currencies: list[str] = [] scanned = (*products, *repo.held_products()) for candidate in (quote_currency, *(quote_currency_of(p) for p in scanned)): @@ -477,6 +517,7 @@ def _mark_to_market_equity( # is still open would otherwise drop that holding out of equity in a single step. valued: set[str] = set() total = quote + unrealized = Decimal("0") for product_id in (*products, *repo.held_products()): if product_id in valued: continue @@ -489,13 +530,17 @@ def _mark_to_market_equity( # A held product with no fresh price is valued at its cost basis rather than dropped: # dropping it understates equity and would trip rail 11 on a DATA GAP rather than on a # loss. `avg_cost` comes from the same audit log as `qty`, so the two always agree. - mark = price_by_product.get(product_id) - if mark is None: - mark = avg_cost + fresh = price_by_product.get(product_id) + mark = avg_cost if fresh is None else fresh if mark <= 0: continue total += qty * mark - return total + # Only a FRESH price books a gain or loss. On the fallback the mark IS the basis, so + # `qty * (mark - avg_cost)` would be zero anyway -- but stating it as a branch keeps the + # meaning ("nothing was observed") rather than leaning on the arithmetic. + if fresh is not None and fresh > 0: + unrealized += qty * (fresh - avg_cost) + return _EquityParts(equity=total, cash=quote, unrealized=unrealized) def _seed_paper_account_if_needed( @@ -1707,15 +1752,28 @@ def run_once( equity_mod.record_external_flow(repo, amount=contribution) repo.set_state("paper_last_contribution_month", month_start) equity_now = paper_trader.equity(latest_price_by_product) + # The same reading, split for the `equity_points` row (#698). `None` when the paper + # account is unseeded -- there is nothing to split -- and never derived from + # `equity_now`, which would invent the split rather than record it. + equity_cash = paper_trader.get_cash() + equity_unrealized = ( + None if equity_cash is None else paper_trader.unrealized(latest_price_by_product) + ) else: # Mirrors `_seed_paper_account_if_needed`'s clear: unconditional, at the top of the # branch, BEFORE the broker-equity read, so an unreadable broker on the first live # cycle after a paper->live flip can't skip the clear (see # `_clear_live_mode_if_needed`'s docstring). _clear_live_mode_if_needed(repo) - equity_now = _mark_to_market_equity( + # One pass, three numbers (#698): the total rail 11 reads, and the cash/unrealized + # legs the series records. Read together so a price cannot tick between them and + # file a row whose parts contradict its own total. + live_parts = _mark_to_market_parts( repo, broker, products, latest_price_by_product, config.quote_currency ) + equity_now = None if live_parts is None else live_parts.equity + equity_cash = None if live_parts is None else live_parts.cash + equity_unrealized = None if live_parts is None else live_parts.unrealized # Task 9: paper-forward observability -- the synthetic equity + drawdown scalars this # cycle advanced, surfaced on `LoopResult` (rendered by # `keel.commands.trading.render_loop_result` + this log line) instead @@ -1735,7 +1793,13 @@ def run_once( # The live-side mode clear now happens up-front in the live branch above (see # `_clear_live_mode_if_needed`), symmetrically with the paper-side clear in # `_seed_paper_account_if_needed`. Nothing left to stamp/clear here. - equity_mod.update_drawdown(repo, equity=equity_now, now_ts=now_ts) + equity_mod.update_drawdown( + repo, + equity=equity_now, + now_ts=now_ts, + cash=equity_cash, + unrealized=equity_unrealized, + ) if paper_trader is not None: result_paper_equity = equity_now diff --git a/keel/commands/insights.py b/keel/commands/insights.py index 1afc3088..8d34f175 100644 --- a/keel/commands/insights.py +++ b/keel/commands/insights.py @@ -47,6 +47,7 @@ from typing import Any import click +from keel_core.types import EquityReading from keel import agent as agent_mod from keel.commands._common import DISCLAIMER, _load_cfg, _open_repo @@ -151,6 +152,16 @@ def shown_count(self) -> int: #: readable in view-source, which is the whole argument for this interface. _COORD = Decimal("0.01") +#: A plain arithmetic zero, deliberately NOT `_BASELINE`. +#: +#: `_BASELINE` is a claim about an axis -- "zero is always on this canvas" -- and it is +#: true of cumulative net P&L, where zero separates a rule that has made money from one +#: that has lost it. `build_equity_series` plots ACCOUNT EQUITY, which has no such line: +#: an account is not up or down against nothing. Forcing zero into that range would +#: squash a $10,000 account's real moves into a sliver at the top of a box that is +#: mostly empty space, so the two zeros are kept apart by name. +_ZERO = Decimal("0") + @dataclass(frozen=True) class EquityPoint: @@ -206,12 +217,187 @@ def point_count(self) -> int: return len(self.points) +@dataclass(frozen=True) +class EquitySeriesPoint: + """One cycle's account equity, and where it is drawn (#698). + + `equity`, `hwm` and `dd_floor` are the exact figures; `x` and `y` are plot coordinates in + the same `PLOT_WIDTH` x `PLOT_HEIGHT` box `EquityPoint` uses, with `y` growing DOWNWARD for + the reason documented there. + + `hwm_y` and `dd_floor_y` are the overlay coordinates for the same instant, so the ceiling + lines are drawn from the same pass and cannot drift from the point they belong to. + `dd_floor` is `None` when no rail setting was supplied -- an unknown ceiling, which is not + the same as a ceiling of zero. + """ + + ts: int + mode: str + equity: Decimal + cash: Decimal | None + unrealized: Decimal | None + hwm: Decimal + dd_floor: Decimal | None + x: Decimal + y: Decimal + hwm_y: Decimal + dd_floor_y: Decimal | None + + +@dataclass(frozen=True) +class EquitySeriesSegment: + """A run of consecutive readings in ONE mode -- one unbroken polyline. + + Segments exist because paper and live are different accounts sharing a database, and a line + drawn across the flip states a continuity that does not exist. A mode that resumes after a + flip is a NEW segment rather than a continuation of its earlier one: grouping by mode alone + would draw a line across the stretch the account spent somewhere else. + """ + + mode: str + points: list[EquitySeriesPoint] + + +@dataclass(frozen=True) +class EquitySeries: + """Account equity over TIME, as the agent marked it each cycle (#698). + + A different chart from `EquityCurve`, not a replacement for it. That one plots cumulative + net P&L over closed TRADES and argues -- correctly, for that quantity -- that its axis + should be trade order. This plots what the account was worth whether or not it traded, and + for that the gaps between cycles are information: a week the agent did not run is a week + with no readings, and it has to look like one. + + `low`/`high` are the axis bounds actually used and include the OVERLAYS, so a drawdown floor + is guaranteed to fit inside the box. A floor drawn off the bottom edge reads as absent, and + an absent rail ceiling is the one thing this chart must never imply. + + Empty is a real answer: a deployment that has not completed a cycle since the v19 upgrade + has no series, and `segments == []` says exactly that. There is no synthetic flat line, + because a flat line is what an account that did not move looks like. + """ + + segments: list[EquitySeriesSegment] + low: Decimal + high: Decimal + width: Decimal + height: Decimal + + @property + def point_count(self) -> int: + """How many readings the series holds, across every segment. + + Derived rather than stored, for the reason `EquityCurve.point_count` is: a stored count + can drift from the list it describes, and `keel/web/payload.py` may not call `len()`. + """ + return sum(len(segment.points) for segment in self.segments) + + def _plot_y(value: Decimal, *, low: Decimal, span: Decimal) -> Decimal: """`value` mapped into `0..PLOT_HEIGHT`, with the top of the box being `low + span`.""" fraction = (value - low) / span return (PLOT_HEIGHT - PLOT_HEIGHT * fraction).quantize(_COORD) +def build_equity_series( + readings: Sequence[EquityReading], + *, + max_total_dd_pct: Decimal | None = None, +) -> EquitySeries: + """The account-equity series over `readings`, oldest first, ready to draw (#698). + + **The horizontal axis is TIME**, unlike `build_equity_curve`'s. The quantity is different: + that curve plots closed trades, where a quiet week is not an event, while this plots what + the account was worth every cycle -- and there, a week with no readings IS the event. Even + spacing would draw a gap in the record as an ordinary step between two cycles. + + **Readings are segmented by mode**, and by RUNS of it rather than by the mode set: paper, + live, paper is three segments. Paper and live are unrelated accounts that share a database + (they flip within one, which is why `agent._clear_live_mode_if_needed` wipes the shared + high-water mark), so a polyline crossing a flip asserts a continuity that does not exist -- + a $10k paper account followed by a $250 live one would draw a 97.5% collapse that never + happened. + + **`hwm` is read off each row, never recomputed.** It is not the running maximum of the + equity: `execution.equity.record_external_flow` REBASES it on a declared deposit so the + drawdown keeps measuring trading performance. The overlay's whole purpose is to show the + ceiling rail 11 actually had in force, so a recomputed maximum would be a different line + wearing its name. + + `max_total_dd_pct` is the rail's own setting (`config.money_mgmt.max_total_dd_pct`). Given + it, each point carries the equity at which rail 11 would start vetoing entries -- derived + from THAT point's high-water mark, so the floor moves with the rebase. Omitted, `dd_floor` + is `None`: the ceiling is unknown, which is not a ceiling of zero. + + Callers pass the readings they are displaying, so the chart and any table beside it can + never disagree about which cycles they describe. + """ + if not readings: + return EquitySeries( + segments=[], low=_ZERO, high=_ZERO, width=PLOT_WIDTH, height=PLOT_HEIGHT + ) + + floors = [ + None if max_total_dd_pct is None else reading.hwm * (Decimal("1") - max_total_dd_pct) + for reading in readings + ] + + # The bounds span the OVERLAYS as well as the equity: the floor and the high-water mark are + # drawn in this box, and a line outside it is a line a reader cannot see. + values = [reading.equity for reading in readings] + values += [reading.hwm for reading in readings] + values += [floor for floor in floors if floor is not None] + low, high = min(values), max(values) + span = high - low + + first_ts, last_ts = readings[0].ts, readings[-1].ts + time_span = last_ts - first_ts + + def _y(value: Decimal) -> Decimal: + # Zero span has no range to normalise against, so a flat account is drawn mid-box: + # pinning it to an edge would imply it sat at an extreme of something. + if span == _ZERO: + return (PLOT_HEIGHT / 2).quantize(_COORD) + return _plot_y(value, low=low, span=span) + + def _x(ts: int) -> Decimal: + # A single reading (or several within one second) has no span to place points along. The + # left edge, because one cycle is the BEGINNING of the record, not the whole of it. + if time_span <= 0: + return _ZERO + return (PLOT_WIDTH * (Decimal(ts - first_ts) / Decimal(time_span))).quantize(_COORD) + + segments: list[EquitySeriesSegment] = [] + current: list[EquitySeriesPoint] = [] + current_mode: str | None = None + for reading, floor in zip(readings, floors): + if reading.mode != current_mode: + if current: + segments.append(EquitySeriesSegment(mode=str(current_mode), points=current)) + current, current_mode = [], reading.mode + current.append( + EquitySeriesPoint( + ts=reading.ts, + mode=reading.mode, + equity=reading.equity, + cash=reading.cash, + unrealized=reading.unrealized, + hwm=reading.hwm, + dd_floor=floor, + x=_x(reading.ts), + y=_y(reading.equity), + hwm_y=_y(reading.hwm), + dd_floor_y=None if floor is None else _y(floor), + ) + ) + if current: + segments.append(EquitySeriesSegment(mode=str(current_mode), points=current)) + + return EquitySeries( + segments=segments, low=low, high=high, width=PLOT_WIDTH, height=PLOT_HEIGHT + ) + + def build_equity_curve(entries: Sequence[JournalEntry]) -> EquityCurve: """The cumulative net-P&L curve over `entries`, oldest first. diff --git a/keel/data/db.py b/keel/data/db.py index eab962d2..0774a459 100644 --- a/keel/data/db.py +++ b/keel/data/db.py @@ -20,7 +20,7 @@ from pathlib import Path from typing import Any -SCHEMA_VERSION = 18 +SCHEMA_VERSION = 19 # Creation order matters for readability (and for backends that validate FK targets eagerly); # SQLite itself only checks FK targets at DML time, but we still declare referenced tables first. @@ -369,6 +369,30 @@ updated_ts INTEGER NOT NULL ) """, + """ + CREATE TABLE IF NOT EXISTS equity_points ( + id INTEGER PRIMARY KEY AUTOINCREMENT, + -- Epoch seconds, INTEGER, like every other timestamp in this schema (`positions. + -- opened_at`, `asset_attestations.attested_at`) and like the `now_ts` the agent passes + -- down its whole cycle. TEXT would order an epoch LEXICALLY, so a `ts >= ?` window -- + -- which is how the chart reads this table -- would silently return the wrong rows. + ts INTEGER NOT NULL, + -- 'paper' | 'live'. NO `profile` column: the database is already one-per-profile + -- (ADR 0002), while paper and live flip WITHIN one database -- so mode is the + -- partition that actually needs storing, and the one a reader must never blend. + mode TEXT NOT NULL, + equity TEXT NOT NULL, + -- NULL is "not recorded", never zero: a cycle can know its total while the split is + -- unavailable (`orders.filled_quantity`'s convention, and for the same reason). + cash TEXT, + unrealized TEXT, + -- The high-water mark AFTER this reading, so the row carries the rail-11 ceiling that + -- was in force rather than one a reader recomputes -- `record_external_flow` rebases + -- the HWM on a declared deposit, and a recomputed maximum would miss that. + hwm TEXT NOT NULL + ) + """, + "CREATE INDEX IF NOT EXISTS idx_equity_points_mode_ts ON equity_points(mode, ts)", ) @@ -816,6 +840,23 @@ def _migrate_v18_venue_cash_postures(conn: sqlite3.Connection) -> None: """ +def _migrate_v19_equity_points(conn: sqlite3.Connection) -> None: + """v19 adds `equity_points`. Table creation is handled by `_SCHEMA_STATEMENTS`; there is + deliberately NO backfill, and what could be backfilled is exactly what must not be. + + Two sources look like history. `agent_state["equity_history"]` holds at most 7 days, and it + is a RAIL's working set, not a record: `record_external_flow` shifts every point in it by a + declared deposit so the weekly drawdown keeps measuring trading performance. Replaying those + shifted numbers as observations would publish equities the account never had. The `orders` + ledger could reconstruct a curve, but only for closed trades, only at trade resolution, and + with no cash leg -- a different quantity wearing this table's name. + + An empty table means "not observed before v19", which is true: nothing wrote it down. The + chart starts at the first cycle after the upgrade and says so, rather than opening on a + fabricated past. + """ + + _MIGRATIONS: dict[int, Callable[[sqlite3.Connection], None]] = { 2: _migrate_v2_broker_subscriptions, 3: _migrate_v3_trade_outcomes, @@ -834,6 +875,7 @@ def _migrate_v18_venue_cash_postures(conn: sqlite3.Connection) -> None: 16: _migrate_v16_orders_submit_book, 17: _migrate_v17_candle_series_feed, 18: _migrate_v18_venue_cash_postures, + 19: _migrate_v19_equity_points, } diff --git a/keel/data/repository.py b/keel/data/repository.py index 63ba591c..72eb6a40 100644 --- a/keel/data/repository.py +++ b/keel/data/repository.py @@ -18,7 +18,7 @@ from keel_core.subscription import BrokerSubscription, SubscriptionStatus from keel_core.trade_scope import TradeScopeState, VenueTradeScope -from keel.types import Candle, Granularity, Profile +from keel.types import Candle, EquityReading, Granularity, Profile _TRANSACTION_COLUMNS = ( "coinbase_id", @@ -152,6 +152,24 @@ def _cash_posture_from_row(row: Any) -> VenueCashPosture: credential_fingerprint=row["credential_fingerprint"], ) + +def _equity_point_from_row(row: Any) -> EquityReading: + """Map an `equity_points` row to the domain record (#698). + + `cash` and `unrealized` stay `None` when the column is NULL rather than becoming + `Decimal("0")`: the column means "not recorded", and a reader must be able to tell an + unobserved split from an observed flat one. + """ + return EquityReading( + ts=int(row["ts"]), + mode=row["mode"], + equity=Decimal(row["equity"]), + cash=_text_to_dec(row["cash"]), + unrealized=_text_to_dec(row["unrealized"]), + hwm=Decimal(row["hwm"]), + ) + + def _json_default(obj: Any) -> Any: if isinstance(obj, Decimal): return {"__decimal__": str(obj)} @@ -696,6 +714,59 @@ def list_venue_cash_postures(self) -> list[VenueCashPosture]: rows = self._conn.execute("SELECT * FROM venue_cash_postures ORDER BY venue").fetchall() return [_cash_posture_from_row(row) for row in rows] + # -- equity points (the mark-to-market series; #698) -------------------- + + def record_equity_point(self, point: EquityReading) -> None: + """Append one cycle's mark-to-market reading. Append-only: never updated, never deleted. + + There is no uniqueness constraint on `(ts, mode)` and deliberately so. This is an + observation log, not a keyed record: two readings that genuinely happened at the same + epoch second are two observations, and silently collapsing them with an upsert would + hide a double-run of the cycle -- exactly the operational fact an operator would want + the series to show. `update_drawdown` calls this once per cycle; anything more is a + symptom worth seeing. + """ + self._conn.execute( + """ + INSERT INTO equity_points (ts, mode, equity, cash, unrealized, hwm) + VALUES (?, ?, ?, ?, ?, ?) + """, + ( + point.ts, + point.mode, + _dec_to_text(point.equity), + _dec_to_text(point.cash), + _dec_to_text(point.unrealized), + _dec_to_text(point.hwm), + ), + ) + self._conn.commit() + + def get_equity_points( + self, mode: str | None = None, since_ts: int | None = None + ) -> list[EquityReading]: + """The series, oldest first, optionally narrowed to one mode and/or a time window. + + `mode=None` returns paper AND live rows interleaved by time. That is the honest raw + read, but it is NOT a curve: a caller drawing it as one line joins two unrelated + accounts across the flip. Readers that plot must group by `mode` (`insights` does). + """ + query = "SELECT * FROM equity_points" + clauses: list[str] = [] + params: list[object] = [] + if mode is not None: + clauses.append("mode = ?") + params.append(mode) + if since_ts is not None: + clauses.append("ts >= ?") + params.append(since_ts) + if clauses: + query += " WHERE " + " AND ".join(clauses) + # `id` breaks the tie so two readings at the same epoch second keep insertion order -- + # `ts` alone leaves that to SQLite, and a chart would draw them in an arbitrary one. + query += " ORDER BY ts, id" + return [_equity_point_from_row(row) for row in self._conn.execute(query, params)] + # -- trade outcomes (closed round-trips; rails 11 and 16) --------------- def insert_trade_outcome(self, outcome: dict[str, Any]) -> int: diff --git a/keel/execution/equity.py b/keel/execution/equity.py index 421d6830..16680bf3 100644 --- a/keel/execution/equity.py +++ b/keel/execution/equity.py @@ -16,6 +16,7 @@ from decimal import Decimal from keel_core.telemetry import log_event +from keel_core.types import EquityReading from keel.compliance.purification import build_report from keel.data.repository import Repository @@ -56,6 +57,40 @@ def mark_positions( return total +def unrealized_on_marks( + positions: list[tuple[Decimal, Decimal]], + price_by_product: dict[str, Decimal], + product_ids: list[str], +) -> Decimal: + """Unrealized P&L = Σ qty·(mark − cost_basis), on the SAME marks `mark_positions` used (#698). + + Deliberately a sibling of `mark_positions` with an identical shape and identical guards: the + two are written into one `equity_points` row for one cycle, so a position the equity valued + at cost must contribute ZERO here. A helper that skipped the unpriced position instead, or + marked it differently, would file a `cash`/`unrealized`/`equity` triple that does not add up + -- and the row is only worth keeping if it reconciles (see + `test_cash_plus_cost_basis_plus_unrealized_reconstructs_the_equity`). + + Signed: negative while a position is under water, which is the direction that matters, since + the whole reason equity is marked to market is to see a loss WHILE it is happening. + """ + total = Decimal("0") + for (qty, cost_basis), product_id in zip(positions, product_ids): + if qty <= 0: + continue + mark = price_by_product.get(product_id) + if mark is None or mark <= 0: + # Valued at cost by `mark_positions`, so there is no observed gain or loss to book. + # (When the basis is non-positive too, `mark_positions` drops the position outright + # and this `continue` has already matched it -- the two stay in step either way.) + continue + # No guard on a non-positive `cost_basis`: `mark_positions` values a fresh-priced + # position at `qty * mark` whatever it cost, so a zero-basis holding is ALL unrealized + # gain. Skipping it here would leave the row's parts short of its own equity. + total += qty * (mark - cost_basis) + return total + + def pending_purification_usd(repo: Repository) -> Decimal: """Accrued-but-unpurified non-compliant income, in USD, from the repo's own ledger (#490). @@ -140,8 +175,27 @@ def record_external_flow(repo: Repository, *, amount: Decimal) -> None: ) -def update_drawdown(repo: Repository, *, equity: Decimal, now_ts: int) -> None: - """Record `equity` and refresh the drawdown scalars rail 11 consumes.""" +def update_drawdown( + repo: Repository, + *, + equity: Decimal, + now_ts: int, + cash: Decimal | None = None, + unrealized: Decimal | None = None, +) -> None: + """Record `equity` and refresh the drawdown scalars rail 11 consumes. + + Also appends one row to the durable `equity_points` series (#698). The two records are NOT + redundant: `equity_history` below is a 7-day window that `record_external_flow` REWRITES on + a declared deposit, because the weekly rail must keep measuring trading performance. That + makes it a working set, not a record -- so the series is written alongside it rather than + derived from it, and neither is reconstructable from the other. + + `cash` and `unrealized` are the optional split of `equity`. They are passed, never derived + here: this function has a total and no positions, and inventing the split from the total is + exactly the fabrication `None` exists to avoid. Callers that know it (the agent's paper and + live branches both do) pass it; callers that do not leave it unrecorded. + """ _warn_on_unexplained_jump(repo, equity=equity) hwm = repo.get_state("equity_high_water_mark") @@ -170,6 +224,51 @@ def update_drawdown(repo: Repository, *, equity: Decimal, now_ts: int) -> None: else max((weekly_peak - equity) / weekly_peak, Decimal("0")), ) + _append_equity_point( + repo, equity=equity, now_ts=now_ts, hwm=hwm, cash=cash, unrealized=unrealized + ) + + +def _append_equity_point( + repo: Repository, + *, + equity: Decimal, + now_ts: int, + hwm: Decimal, + cash: Decimal | None, + unrealized: Decimal | None, +) -> None: + """Append this cycle's reading to the durable series, stamped with the mode that produced it. + + The mode comes from `equity_state_mode` -- the SAME stamp `agent._clear_live_mode_if_needed` + and `agent._seed_paper_account_if_needed` read before wiping the shared HWM on a flip. + Deriving it a second way here (from a passed flag, say) would let two answers to one question + drift apart, and the failure would be silent: a row filed under the wrong mode does not go + missing, it lands in the other account's curve. + + An UNSTAMPED mode writes nothing, and does not raise. Every agent path stamps it + unconditionally before this runs, so the unstamped case is not a real cycle (a direct call in + a test, a caller yet to be written). Two things it must not do: guess a mode -- that is the + mislabelling above, manufactured -- or fail the call, which would take rail 11's scalars down + with the chart. The drawdown scalars are already written by the time this is reached, so a + skipped point costs a gap in a chart and nothing else. + + LAST in `update_drawdown` for that reason: the rail is served before the record is kept. + """ + mode = repo.get_state("equity_state_mode") + if mode is None: + return + repo.record_equity_point( + EquityReading( + ts=now_ts, + mode=str(mode), + equity=equity, + cash=cash, + unrealized=unrealized, + hwm=hwm, + ) + ) + def _warn_on_unexplained_jump(repo: Repository, *, equity: Decimal) -> None: """Log a WARNING when equity moves more than `UNEXPLAINED_JUMP_PCT` between cycles. diff --git a/keel/strategy/paper.py b/keel/strategy/paper.py index 72115e3e..aacfad07 100644 --- a/keel/strategy/paper.py +++ b/keel/strategy/paper.py @@ -242,6 +242,21 @@ def equity(self, price_by_product: dict[str, Decimal]) -> Decimal | None: positions = [(self._open[p].qty, self._open[p].entry_fill) for p in product_ids] return mark_positions(self._cash, positions, price_by_product, product_ids) + def unrealized(self, price_by_product: dict[str, Decimal]) -> Decimal | None: + """The unrealized leg of the SAME reading `equity()` reports (#698). + + `None` under exactly `equity()`'s condition -- unseeded cash -- so the two are recorded + together or not at all, and the same `costed=False` exclusion applies: a position + nothing paid for is not in the equity, so its gain must not be in the P&L either. + """ + if self._cash is None: + return None + from keel.execution.equity import unrealized_on_marks + + product_ids = [p for p in self._open if self._open[p].costed] + positions = [(self._open[p].qty, self._open[p].entry_fill) for p in product_ids] + return unrealized_on_marks(positions, price_by_product, product_ids) + def on_signal( self, signal: Signal, candle: Candle | None = None, qty: Decimal = _QTY ) -> int | None: diff --git a/packages/keel-core/keel_core/types.py b/packages/keel-core/keel_core/types.py index fb5e95a7..6d9667aa 100644 --- a/packages/keel-core/keel_core/types.py +++ b/packages/keel-core/keel_core/types.py @@ -41,6 +41,34 @@ class Candle: volume: Decimal +@dataclass(frozen=True) +class EquityReading: + """One cycle's mark-to-market equity reading, as the agent computed it (#698). + + `mode` is `"paper"` or `"live"` -- the same partition `agent._clear_live_mode_if_needed` + enforces on the shared high-water mark. Two modes share one database (ADR 0002 gives each + profile its own), and their equities are unrelated accounts: a reader that blends them draws + a cliff at the flip and calls it a drawdown. + + `cash` and `unrealized` are `None` for "not recorded", never zero -- the `orders.filled_ + quantity` convention. A cycle can know its total equity while the split is unavailable (an + unseeded paper account, a broker that answered for the total but not per-currency), and + writing a zero there would state a flat position that was never observed. + + `hwm` is the high-water mark AFTER this reading was folded in, so a row carries the rail-11 + ceiling that was actually in force when the agent acted on it -- the chart's drawdown + overlay reads it rather than recomputing a monotonic maximum the engine may have rebased + (`execution.equity.record_external_flow` shifts the HWM on a declared deposit). + """ + + ts: int + mode: str + equity: Decimal + cash: Decimal | None + unrealized: Decimal | None + hwm: Decimal + + @dataclass(frozen=True) class Profile: """The user's own settings, as opposed to operational state or file configuration. @@ -70,4 +98,4 @@ def is_autonomous(self, now_ts: int) -> bool: return now_ts < self.autonomous_until -__all__ = ["Granularity", "Side", "Candle", "Profile"] +__all__ = ["Granularity", "Side", "Candle", "EquityReading", "Profile"] diff --git a/tests/commands/test_equity_series.py b/tests/commands/test_equity_series.py new file mode 100644 index 00000000..dbf75122 --- /dev/null +++ b/tests/commands/test_equity_series.py @@ -0,0 +1,185 @@ +"""The time-axised account-equity chart over `equity_points` -- issue #698. + +This is NOT a replacement for `build_equity_curve`. That one plots cumulative net P&L over +CLOSED TRADES on a trade-order axis, and its docstring argues for that axis on its own terms: a +quiet week must not get the visual weight of fifty trades when the subject is a track record. +This builder plots a different quantity -- what the ACCOUNT was worth, every cycle, whether it +traded or not -- and for that quantity time is the only honest axis, because the gaps are the +information. + +The properties pinned here are the ones that would otherwise let the chart tell a lie the data +does not support: a flip between two accounts drawn as one line, a gap drawn as a straight +segment, and a rail ceiling recomputed rather than read. +""" + +from __future__ import annotations + +from decimal import Decimal + +from keel_core.types import EquityReading + +from keel.commands.insights import PLOT_HEIGHT, PLOT_WIDTH, build_equity_series + +NOW = 1_800_000_000 +DAY = 86_400 + + +def _reading( + ts: int, + equity: str, + *, + mode: str = "live", + hwm: str | None = None, + cash: str | None = None, + unrealized: str | None = None, +) -> EquityReading: + return EquityReading( + ts=ts, + mode=mode, + equity=Decimal(equity), + cash=None if cash is None else Decimal(cash), + unrealized=None if unrealized is None else Decimal(unrealized), + hwm=Decimal(hwm if hwm is not None else equity), + ) + + +def test_no_readings_is_a_real_answer_not_an_empty_chart() -> None: + """A deployment that has not run a cycle since v19 has no series. Distinct from a flat + line, which is what an account that did not move looks like.""" + series = build_equity_series([]) + assert series.segments == [] + assert series.point_count == 0 + + +def test_the_x_axis_is_time_so_a_quiet_stretch_leaves_a_gap() -> None: + """The whole reason this chart exists next to the trade-order one: equity is a property of + the calendar. Three cycles, the last of them a week after the second, must NOT be evenly + spaced -- even spacing would draw a week of silence as one ordinary step.""" + series = build_equity_series( + [ + _reading(NOW, "10000"), + _reading(NOW + DAY, "10100"), + _reading(NOW + 8 * DAY, "10200"), + ] + ) + xs = [point.x for point in series.segments[0].points] + assert xs[0] == Decimal("0") + assert xs[-1] == PLOT_WIDTH + # 1 day into an 8-day span is one eighth across, not one half. + assert xs[1] == (PLOT_WIDTH / 8).quantize(Decimal("0.01")) + + +def test_a_mode_flip_produces_two_segments_not_one_blended_curve() -> None: + """The acceptance criterion this chart is judged on. $10k of paper and $250 of live are two + unrelated accounts; joined, the flip draws a 97.5% collapse that never happened.""" + series = build_equity_series( + [ + _reading(NOW, "10000", mode="paper"), + _reading(NOW + DAY, "10100", mode="paper"), + _reading(NOW + 2 * DAY, "250", mode="live"), + _reading(NOW + 3 * DAY, "260", mode="live"), + ] + ) + assert [segment.mode for segment in series.segments] == ["paper", "live"] + assert [len(segment.points) for segment in series.segments] == [2, 2] + + +def test_a_mode_that_resumes_after_a_flip_is_a_third_segment() -> None: + """Segments follow the ORDER of the readings, not the set of modes: paper, live, paper is + three runs. Grouping by mode alone would join the two paper stretches across the live one + and draw a line through time the account did not spend in paper.""" + series = build_equity_series( + [ + _reading(NOW, "10000", mode="paper"), + _reading(NOW + DAY, "250", mode="live"), + _reading(NOW + 2 * DAY, "10100", mode="paper"), + ] + ) + assert [segment.mode for segment in series.segments] == ["paper", "live", "paper"] + + +def test_the_high_water_mark_overlay_is_read_from_the_rows_not_recomputed() -> None: + """`record_external_flow` REBASES the HWM on a declared deposit, so it is not the running + maximum of the equity series. Recomputing it here would draw a ceiling rail 11 never used -- + and the point of the overlay is to show the ceiling that was actually in force.""" + readings = [ + _reading(NOW, "10000", hwm="10000"), + _reading(NOW + DAY, "9000", hwm="10000"), + # A $5k deposit: equity jumps and the operator declares the flow, so the HWM is rebased + # UP rather than the deposit reading as a recovery. A running maximum would say 14000. + _reading(NOW + 2 * DAY, "14000", hwm="15000"), + ] + series = build_equity_series(readings) + assert [point.hwm for point in series.segments[0].points] == [ + Decimal("10000"), + Decimal("10000"), + Decimal("15000"), + ] + + +def test_the_drawdown_ceiling_is_drawn_beneath_each_points_own_high_water_mark() -> None: + """Rail 11 vetoes entries at `drawdown >= max_total_dd_pct` measured from the HWM, so the + ceiling is a function of the HWM in force at that instant -- it MOVES with the rebase, and a + single horizontal line would be wrong from the first deposit onwards.""" + series = build_equity_series( + [_reading(NOW, "10000", hwm="10000"), _reading(NOW + DAY, "14000", hwm="15000")], + max_total_dd_pct=Decimal("0.20"), + ) + assert [point.dd_floor for point in series.segments[0].points] == [ + Decimal("8000"), + Decimal("12000"), + ] + + +def test_without_a_ceiling_there_is_no_floor_line_rather_than_a_zero_one() -> None: + """A caller that did not supply the rail's setting has not told us the ceiling is zero.""" + series = build_equity_series([_reading(NOW, "10000")]) + assert series.segments[0].points[0].dd_floor is None + + +def test_the_axis_bounds_contain_the_overlays_not_only_the_equity() -> None: + """The ceiling is drawn on this canvas, so it has to fit on it. Bounds taken from the equity + alone would push a floor line off the bottom of the box, where it reads as absent -- the one + thing a rail's ceiling must never look like.""" + series = build_equity_series( + [_reading(NOW, "10000", hwm="12000"), _reading(NOW + DAY, "10500", hwm="12000")], + max_total_dd_pct=Decimal("0.20"), + ) + assert series.low == Decimal("9600") # the floor, below every equity reading + assert series.high == Decimal("12000") # the HWM, above every equity reading + + +def test_zero_is_not_forced_into_the_equity_axis() -> None: + """`build_equity_curve` puts zero on its canvas unconditionally, because it plots net P&L + and zero is the line between making and losing money. Account equity has no such line: an + account is not "up" or "down" against nothing. Forcing zero in would squash every real move + into the top sliver of a box that is mostly empty space below the account.""" + series = build_equity_series([_reading(NOW, "10000"), _reading(NOW + DAY, "10500")]) + assert series.low == Decimal("10000") + assert series.high == Decimal("10500") + + +def test_a_flat_account_is_drawn_on_a_line_not_at_the_edge_of_the_box() -> None: + """Zero span has no range to normalise against. Drawn mid-box: the account did not move, + and pinning it to the top or the bottom would imply it sat at an extreme of something.""" + series = build_equity_series([_reading(NOW, "10000"), _reading(NOW + DAY, "10000")]) + ys = [point.y for point in series.segments[0].points] + assert ys == [(PLOT_HEIGHT / 2).quantize(Decimal("0.01"))] * 2 + + +def test_a_single_reading_sits_at_the_start_of_the_time_axis() -> None: + """One cycle is a real state (the first after upgrading). A zero-width time span must not + divide by zero, and the point belongs at the left edge: it is the beginning of the record, + not the whole of it.""" + series = build_equity_series([_reading(NOW, "10000")]) + point = series.segments[0].points[0] + assert point.x == Decimal("0") + assert series.point_count == 1 + + +def test_y_grows_downward_like_svgs_does() -> None: + """The same convention `EquityPoint` documents: the browser is not allowed to do the one + subtraction that would flip it.""" + series = build_equity_series([_reading(NOW, "10000"), _reading(NOW + DAY, "20000")]) + low_point, high_point = series.segments[0].points + assert low_point.y > high_point.y diff --git a/tests/data/test_db.py b/tests/data/test_db.py index 62f2467a..48a8f01a 100644 --- a/tests/data/test_db.py +++ b/tests/data/test_db.py @@ -103,11 +103,11 @@ def test_agent_state_table_has_key_primary_key(): assert pk_columns == {"key"} -def test_schema_version_is_18(): +def test_schema_version_is_19(): """Deliberate tripwire: bump this literal consciously on every schema change.""" from keel.data.db import SCHEMA_VERSION - assert SCHEMA_VERSION == 18 + assert SCHEMA_VERSION == 19 def test_a_v6_database_migrates_up_and_gains_the_profile_table(tmp_path): diff --git a/tests/data/test_equity_points.py b/tests/data/test_equity_points.py new file mode 100644 index 00000000..22455296 --- /dev/null +++ b/tests/data/test_equity_points.py @@ -0,0 +1,135 @@ +"""Storage for the mark-to-market equity series -- issue #698. + +The agent has always computed mark-to-market equity every cycle and always thrown it away: +`agent_state["equity_history"]` is a 7-day rolling window kept for the weekly drawdown rail, and +nothing else persisted. This table is the long-term record, and the tests below pin the two +properties that make it usable as one. + +`mode` is the load-bearing partition. The DB is already one-per-profile (ADR 0002), but paper and +live flip WITHIN one database, and `_clear_live_mode_if_needed` wipes the shared HWM on every +flip precisely because one mode's equity is not the other's. A series that blended them would +draw a cliff between two unrelated accounts and call it a drawdown. + +Money is TEXT holding `str(Decimal(...))`, the standing convention (`db.py` module docstring): +these rows feed a chart whose whole claim is that the numbers are the ones the engine acted on. +""" + +from __future__ import annotations + +from decimal import Decimal + +import pytest +from keel_core.types import EquityReading + +from keel.data.db import connect, migrate +from keel.data.repository import Repository + +NOW = 1_800_000_000 +DAY = 86_400 + + +@pytest.fixture() +def repo() -> Repository: + conn = connect(":memory:") + migrate(conn) + return Repository(conn) + + +def _point( + ts: int = NOW, + mode: str = "paper", + equity: str = "10000.55", + cash: str | None = "9000.25", + unrealized: str | None = "-12.30", + hwm: str = "10500.00", +) -> EquityReading: + return EquityReading( + ts=ts, + mode=mode, + equity=Decimal(equity), + cash=None if cash is None else Decimal(cash), + unrealized=None if unrealized is None else Decimal(unrealized), + hwm=Decimal(hwm), + ) + + +def test_the_schema_carries_the_table() -> None: + conn = connect(":memory:") + migrate(conn) + names = {r["name"] for r in conn.execute("SELECT name FROM sqlite_master WHERE type='table'")} + assert "equity_points" in names + + +def test_a_fresh_database_carries_no_points(repo: Repository) -> None: + """No backfill, by design (`_migrate_v19_equity_points`): the 7-day window is a rail's + working set that `record_external_flow` rewrites, and replaying it would publish equities + the account never had.""" + assert repo.get_equity_points() == [] + + +def test_a_point_round_trips_exactly(repo: Repository) -> None: + repo.record_equity_point(_point()) + assert repo.get_equity_points() == [_point()] + + +def test_money_is_stored_as_exact_decimal_strings(repo: Repository) -> None: + """The standing TEXT convention, checked at the storage layer rather than trusted: a float + column would round 10000.55 and the chart's claim is that these are the numbers the engine + acted on.""" + repo.record_equity_point(_point(equity="10000.55", cash="9000.25", unrealized="-12.30")) + row = repo._conn.execute("SELECT equity, cash, unrealized, hwm FROM equity_points").fetchone() + assert (row["equity"], row["cash"], row["unrealized"], row["hwm"]) == ( + "10000.55", + "9000.25", + "-12.30", + "10500.00", + ) + + +def test_an_unrecorded_split_round_trips_as_none_not_zero(repo: Repository) -> None: + """`None` means the cycle knew its total but not the split. Zero would state a flat cash + position and a flat unrealized P&L, neither of which was observed.""" + repo.record_equity_point(_point(cash=None, unrealized=None)) + got = repo.get_equity_points()[0] + assert got.cash is None + assert got.unrealized is None + assert got.equity == Decimal("10000.55") + + +def test_points_read_back_oldest_first(repo: Repository) -> None: + """A chart draws left to right; ordering at the read keeps every caller from re-sorting.""" + for offset in (2 * DAY, 0, DAY): + repo.record_equity_point(_point(ts=NOW + offset)) + assert [p.ts for p in repo.get_equity_points()] == [NOW, NOW + DAY, NOW + 2 * DAY] + + +def test_a_mode_reads_back_only_its_own_points(repo: Repository) -> None: + """The partition that matters: paper and live share one database and flip within it, and + `_clear_live_mode_if_needed` wipes the shared HWM on every flip for this reason.""" + repo.record_equity_point(_point(ts=NOW, mode="paper", equity="10000")) + repo.record_equity_point(_point(ts=NOW + DAY, mode="live", equity="250")) + assert [p.equity for p in repo.get_equity_points(mode="paper")] == [Decimal("10000")] + assert [p.equity for p in repo.get_equity_points(mode="live")] == [Decimal("250")] + assert len(repo.get_equity_points()) == 2 + + +def test_a_since_window_is_bounded_by_epoch_order_not_string_order(repo: Repository) -> None: + """Guards the `ts INTEGER` choice. As TEXT, "1800086400" < "1800000000" is false but + "999999999" > "1800000000" is true -- a window straddling a digit-count change would drop + or admit the wrong rows, silently.""" + repo.record_equity_point(_point(ts=999_999_999)) + repo.record_equity_point(_point(ts=NOW)) + assert [p.ts for p in repo.get_equity_points(since_ts=NOW)] == [NOW] + + +def test_an_existing_database_gains_the_table_on_migration() -> None: + conn = connect(":memory:") + migrate(conn) + # Literal 18, not `SCHEMA_VERSION - 1`: every version pin in tests/data/ is a literal + # precisely so a bump is acknowledged rather than silently absorbed. + conn.execute("UPDATE schema_version SET version = 18") + conn.execute("DROP TABLE equity_points") + conn.commit() + migrate(conn) + names = {r["name"] for r in conn.execute("SELECT name FROM sqlite_master WHERE type='table'")} + assert "equity_points" in names diff --git a/tests/data/test_migrations.py b/tests/data/test_migrations.py index b6ee8425..ddb9b224 100644 --- a/tests/data/test_migrations.py +++ b/tests/data/test_migrations.py @@ -49,7 +49,7 @@ def test_fresh_database_is_stamped_at_the_current_version() -> None: conn = db.connect(":memory:") db.migrate(conn) version = conn.execute("SELECT version FROM schema_version").fetchone()["version"] - assert version == db.SCHEMA_VERSION == 18 + assert version == db.SCHEMA_VERSION == 19 def test_fresh_database_gets_no_subscription_row() -> None: @@ -612,7 +612,7 @@ def test_v14_migration_bumps_the_stored_version() -> None: conn = _v12_database() db.migrate(conn) stamped = conn.execute("SELECT version FROM schema_version").fetchone()["version"] - assert stamped == db.SCHEMA_VERSION == 18 + assert stamped == db.SCHEMA_VERSION == 19 def test_v14_migration_step_is_not_blocked_by_another_venues_existing_row() -> None: @@ -773,7 +773,7 @@ def test_v15_migration_bumps_the_stored_version() -> None: conn = _v12_database() db.migrate(conn) stamped = conn.execute("SELECT version FROM schema_version").fetchone()["version"] - assert stamped == db.SCHEMA_VERSION == 18 + assert stamped == db.SCHEMA_VERSION == 19 def test_v15_the_12_to_15_chain_creates_the_table_with_the_column_already_present() -> None: @@ -875,7 +875,7 @@ def test_an_existing_orders_table_gains_the_submit_book_by_ALTER() -> None: assert row["submit_best_bid"] is None assert row["submit_best_ask"] is None stamped = conn.execute("SELECT version FROM schema_version").fetchone()["version"] - assert stamped == db.SCHEMA_VERSION == 18 + assert stamped == db.SCHEMA_VERSION == 19 def test_v16_is_idempotent_per_column() -> None: diff --git a/tests/data/test_trade_outcomes.py b/tests/data/test_trade_outcomes.py index 3c1945d5..6c2052f7 100644 --- a/tests/data/test_trade_outcomes.py +++ b/tests/data/test_trade_outcomes.py @@ -35,11 +35,11 @@ def _outcome(**overrides: object) -> dict: return base -def test_schema_is_at_version_18() -> None: +def test_schema_is_at_version_19() -> None: conn = db.connect(":memory:") db.migrate(conn) version = conn.execute("SELECT version FROM schema_version").fetchone()["version"] - assert version == db.SCHEMA_VERSION == 18 + assert version == db.SCHEMA_VERSION == 19 def test_fresh_database_has_no_outcomes() -> None: diff --git a/tests/execution/test_equity.py b/tests/execution/test_equity.py index ce7e5600..aab6b7f8 100644 --- a/tests/execution/test_equity.py +++ b/tests/execution/test_equity.py @@ -238,3 +238,95 @@ def test_pending_purification_usd_is_zero_on_a_clean_ledger() -> None: repo.upsert_transaction(_reward_tx("cl1", "500", tx_type="Buy")) assert equity.pending_purification_usd(repo) == Decimal("0") + + +# -- the persisted series (#698) ------------------------------------------------------------ +# +# `equity_history` in `agent_state` is a 7-day window kept for the WEEKLY rail, and +# `record_external_flow` rewrites every point in it on a declared deposit. It is a rail's +# working set, so it cannot double as the record. These tests pin the durable one. + + +def test_a_cycle_appends_one_point_to_the_series() -> None: + repo = _repo() + repo.set_state("equity_state_mode", "paper") + equity.update_drawdown(repo, equity=Decimal("10000"), now_ts=NOW) + points = repo.get_equity_points() + assert len(points) == 1 + assert points[0].ts == NOW + assert points[0].equity == Decimal("10000") + + +def test_the_point_carries_the_high_water_mark_in_force_after_this_reading() -> None: + """The chart's rail-11 overlay reads `hwm` off the row rather than recomputing a running + maximum, because the two are not the same series: `record_external_flow` REBASES the HWM on + a declared deposit, and a recomputed maximum would draw a ceiling the rail never used.""" + repo = _repo() + repo.set_state("equity_state_mode", "paper") + equity.update_drawdown(repo, equity=Decimal("12000"), now_ts=NOW) + equity.update_drawdown(repo, equity=Decimal("9000"), now_ts=NOW + DAY) + assert [p.hwm for p in repo.get_equity_points()] == [Decimal("12000"), Decimal("12000")] + + +def test_the_point_is_stamped_with_the_mode_that_produced_it() -> None: + """`equity_state_mode` is the same stamp `_clear_live_mode_if_needed` reads before wiping + the shared HWM on a flip. Deriving the mode a second way at the call site would let the two + disagree, and a mislabelled row is worse than a missing one -- it lands in the wrong curve.""" + repo = _repo() + repo.set_state("equity_state_mode", "live") + equity.update_drawdown(repo, equity=Decimal("250"), now_ts=NOW) + assert [p.mode for p in repo.get_equity_points()] == ["live"] + + +def test_a_mode_flip_leaves_two_series_not_one_blended_curve() -> None: + """The whole reason for the `mode` column. Paper equity of $10k and live equity of $250 are + two unrelated accounts; joined into one line the flip reads as a 97.5% drawdown that never + happened.""" + repo = _repo() + repo.set_state("equity_state_mode", "paper") + equity.update_drawdown(repo, equity=Decimal("10000"), now_ts=NOW) + # What the agent does on a flip: the shared scalars are cleared, then the stamp changes. + repo.set_state("equity_high_water_mark", None) + repo.set_state("equity_history", []) + repo.set_state("equity_state_mode", "live") + equity.update_drawdown(repo, equity=Decimal("250"), now_ts=NOW + DAY) + + assert [p.equity for p in repo.get_equity_points(mode="paper")] == [Decimal("10000")] + assert [p.equity for p in repo.get_equity_points(mode="live")] == [Decimal("250")] + + +def test_the_split_is_recorded_when_the_caller_knows_it() -> None: + repo = _repo() + repo.set_state("equity_state_mode", "paper") + equity.update_drawdown( + repo, + equity=Decimal("10000"), + now_ts=NOW, + cash=Decimal("9000"), + unrealized=Decimal("-25.50"), + ) + point = repo.get_equity_points()[0] + assert point.cash == Decimal("9000") + assert point.unrealized == Decimal("-25.50") + + +def test_an_unknown_split_is_recorded_as_none_not_zero() -> None: + """A caller that knows only the total says so. Zero would assert a flat cash balance and a + flat unrealized P&L, and nothing observed either.""" + repo = _repo() + repo.set_state("equity_state_mode", "paper") + equity.update_drawdown(repo, equity=Decimal("10000"), now_ts=NOW) + point = repo.get_equity_points()[0] + assert point.cash is None + assert point.unrealized is None + + +def test_an_unstamped_mode_writes_no_point_and_still_updates_the_rail() -> None: + """Rail 11 must never be held hostage to the chart. The agent stamps the mode before every + `update_drawdown` (both branches do, unconditionally), so an unstamped call is not a real + cycle -- and a row labelled with a guessed mode would land in the wrong curve, which is the + one failure the partition exists to prevent. The scalars still advance.""" + repo = _repo() + equity.update_drawdown(repo, equity=Decimal("10000"), now_ts=NOW) + assert repo.get_equity_points() == [] + assert repo.get_state("equity_high_water_mark") == Decimal("10000") diff --git a/tests/execution/test_paper_equity.py b/tests/execution/test_paper_equity.py index 873a7d20..0c18c771 100644 --- a/tests/execution/test_paper_equity.py +++ b/tests/execution/test_paper_equity.py @@ -6,7 +6,7 @@ from decimal import Decimal -from keel.execution.equity import mark_positions +from keel.execution.equity import mark_positions, unrealized_on_marks def test_mark_positions_uses_fresh_price(): @@ -49,6 +49,91 @@ def test_mark_positions_skips_non_positive_qty(): assert eq == Decimal("1000") +# -- the unrealized leg of the same reading (#698) -------------------------------------------- +# +# `equity_points.unrealized` is written from here, so it MUST answer the marks `mark_positions` +# used for the SAME cycle: two helpers reading one set of positions under different fallback +# rules would file an equity and a P&L that cannot both be true. + + +def test_unrealized_is_the_gain_over_cost_at_the_fresh_price(): + pnl = unrealized_on_marks( + positions=[(Decimal("2"), Decimal("100"))], + price_by_product={"BTC-USD": Decimal("150")}, + product_ids=["BTC-USD"], + ) + assert pnl == Decimal("2") * (Decimal("150") - Decimal("100")) + + +def test_unrealized_is_zero_when_the_price_is_missing(): + """The cost-basis fallback, restated in P&L terms: `mark_positions` values that position AT + cost, so the only unrealized figure consistent with the equity it just reported is zero. + Anything else books a gain against a price nobody observed.""" + pnl = unrealized_on_marks( + positions=[(Decimal("2"), Decimal("100"))], + price_by_product={}, + product_ids=["BTC-USD"], + ) + assert pnl == Decimal("0") + + +def test_unrealized_is_zero_when_the_price_is_non_positive(): + pnl = unrealized_on_marks( + positions=[(Decimal("2"), Decimal("100"))], + price_by_product={"BTC-USD": Decimal("0")}, + product_ids=["BTC-USD"], + ) + assert pnl == Decimal("0") + + +def test_unrealized_skips_non_positive_qty(): + pnl = unrealized_on_marks( + positions=[(Decimal("0"), Decimal("100"))], + price_by_product={"BTC-USD": Decimal("150")}, + product_ids=["BTC-USD"], + ) + assert pnl == Decimal("0") + + +def test_unrealized_is_negative_on_a_losing_position(): + pnl = unrealized_on_marks( + positions=[(Decimal("2"), Decimal("100"))], + price_by_product={"BTC-USD": Decimal("90")}, + product_ids=["BTC-USD"], + ) + assert pnl == Decimal("-20") + + +def test_a_zero_cost_basis_position_is_all_unrealized_gain(): + """`mark_positions` values a fresh-priced position at `qty * mark` regardless of what it + cost, so a zero-basis holding (an airdrop, a migrated row with no recorded fill) is entirely + unrealized gain. Skipping it here would leave `cash + cost + unrealized` short of the equity + reported for the same cycle.""" + pnl = unrealized_on_marks( + positions=[(Decimal("2"), Decimal("0"))], + price_by_product={"BTC-USD": Decimal("150")}, + product_ids=["BTC-USD"], + ) + assert pnl == Decimal("300") + + +def test_cash_plus_cost_basis_plus_unrealized_reconstructs_the_equity(): + """The invariant that makes three columns one reading rather than three: a row whose parts + do not add back to `equity` cannot be reconciled, and the chart's whole claim is that these + are the numbers the engine acted on. The second position is deliberately unpriced, so the + identity is checked across BOTH the fresh-price and the fallback leg.""" + positions = [(Decimal("2"), Decimal("100")), (Decimal("5"), Decimal("20"))] + products = ["BTC-USD", "ETH-USD"] + prices = {"BTC-USD": Decimal("150")} + cash = Decimal("1000") + + equity = mark_positions(cash, positions, prices, products) + unrealized = unrealized_on_marks(positions, prices, products) + cost_basis_total = sum((qty * basis for qty, basis in positions), Decimal("0")) + + assert cash + cost_basis_total + unrealized == equity + + def test_mark_positions_with_no_positions_returns_cash(): eq = mark_positions(cash=Decimal("1000"), positions=[], price_by_product={}, product_ids=[]) assert eq == Decimal("1000") diff --git a/tests/strategy/test_paper.py b/tests/strategy/test_paper.py index 3ced0983..70dea9c2 100644 --- a/tests/strategy/test_paper.py +++ b/tests/strategy/test_paper.py @@ -506,6 +506,52 @@ def test_paper_equity_seed_and_mark(repo): assert eq > Decimal("29000") +def test_paper_unrealized_is_none_under_exactly_the_same_condition_as_equity(repo): + """Unseeded means there is no synthetic account to mark. The two must answer `None` + together, or `equity_points` would file a total with a split from a different account + state (#698).""" + trader = PaperTrader(repo) + assert trader.equity({"BTC-USD": Decimal("100")}) is None + assert trader.unrealized({"BTC-USD": Decimal("100")}) is None + + +def test_paper_unrealized_and_cash_reconcile_against_the_marked_equity(repo): + """The row-level invariant on the paper side: `cash + cost basis + unrealized == equity`, + all four read off one account state. The fill price is whatever slippage and fees made it, + which is exactly why the cost basis is read back from the trader rather than assumed.""" + trader = PaperTrader(repo) + trader.seed_cash(Decimal("30000"), now_ts=1_700_000_000) + trader.on_signal( + _enter_signal(setup=_setup(entry="100", stop="90", target="130")), qty=Decimal("5") + ) + + marks = {"BTC-USD": Decimal("120")} + equity = trader.equity(marks) + cash = trader.get_cash() + unrealized = trader.unrealized(marks) + + entry_fill = repo.get_orders(mode="paper")[0]["actual_fill"] + cost_basis = Decimal("5") * entry_fill + + assert unrealized == Decimal("5") * (Decimal("120") - entry_fill) + assert cash + cost_basis + unrealized == equity + + +def test_paper_unrealized_excludes_a_position_nothing_paid_for(repo): + """`equity()` drops an uncosted position (opened before cash was seeded) because marking it + would inflate equity with a position nothing paid for. Its gain has to be dropped for the + same reason -- otherwise the split reports a P&L the equity does not contain.""" + trader = PaperTrader(repo) + trader.on_signal( + _enter_signal(setup=_setup(entry="100", stop="90", target="130")), qty=Decimal("5") + ) + trader.seed_cash(Decimal("30000"), now_ts=1_700_000_000) + + marks = {"BTC-USD": Decimal("120")} + assert trader.equity(marks) == Decimal("30000") + assert trader.unrealized(marks) == Decimal("0") + + def test_paper_funding_check_rejects_when_cash_insufficient(repo): trader = PaperTrader(repo) trader.seed_cash(Decimal("50"), now_ts=1_700_000_000) diff --git a/tests/test_agent.py b/tests/test_agent.py index 4c5ef75d..0407b28c 100644 --- a/tests/test_agent.py +++ b/tests/test_agent.py @@ -1393,6 +1393,74 @@ def get_balances(self) -> list[Balance]: assert repo.get_state("drawdown_total_pct") == Decimal("0.3") +# -- the persisted equity series (#698) -------------------------------------------------------- + + +def test_a_live_cycle_records_the_equity_point_with_its_cash_and_unrealized( + repo: Repository, +) -> None: + """The agent already computed all three every cycle and persisted none of them. The point + has to carry the SPLIT, not just the total: a chart that can only draw one line cannot show + whether an equity move was a position marking up or cash arriving.""" + _seed_open_position(repo, PRODUCT, Decimal("2"), Decimal("100"), ts=1_000) + series = {(PRODUCT, Granularity.ONE_DAY): [_candle(1_000 + i * 86_400) for i in range(30)]} + broker = FakeBroker(series=series) + now = 1_000 + 29 * 86_400 + + run_once(broker, repo, _config(), now_ts=now) + + points = repo.get_equity_points() + assert len(points) == 1 + point = points[0] + assert point.mode == "live" + assert point.ts == now + # FakeBroker's cash, and 2 units bought at 100 now marked at the candle close of 100. + assert point.cash == Decimal("1000000") + assert point.unrealized == Decimal("0") + assert point.equity == Decimal("1000000") + Decimal("2") * Decimal("100") + + +def test_the_live_split_reconciles_against_the_equity_it_was_read_with(repo: Repository) -> None: + """`cash + cost basis + unrealized == equity`, on ONE read of the account. + + Called directly with an explicit price map, like its neighbours below: through `run_once` + the map only covers products with a LIVE RULE, so a held-only position takes the cost-basis + fallback and the marked-up case -- the one where the parts can disagree -- never arises. + """ + _seed_open_position(repo, PRODUCT, Decimal("2"), Decimal("100"), ts=1_000) + broker = FakeBroker() + + parts = agent._mark_to_market_parts( + repo, broker, [PRODUCT], {PRODUCT: Decimal("150")}, "USD" + ) + + assert parts is not None + cost_basis = Decimal("2") * Decimal("100") + assert parts.unrealized == Decimal("2") * (Decimal("150") - Decimal("100")) + assert parts.cash + cost_basis + parts.unrealized == parts.equity + # And the total is exactly what the existing scalar path reports for the same read. + assert parts.equity == agent._mark_to_market_equity( + repo, broker, [PRODUCT], {PRODUCT: Decimal("150")}, "USD" + ) + + +def test_an_unreadable_equity_records_no_point(repo: Repository) -> None: + """`_mark_to_market_equity` returns `None` when NO balance could be read -- equity is + genuinely unknown, and the cycle already declines to touch rail 11's scalars for exactly + that reason. The series must decline too: a gap in the chart is honest, a point carrying + last cycle's number dressed as this cycle's reading is not.""" + + class _MuteBroker(FakeBroker): + def get_balances(self) -> list[Balance]: + return [] + + series = {(PRODUCT, Granularity.ONE_DAY): [_candle(1_000 + i * 86_400) for i in range(30)]} + + run_once(_MuteBroker(series=series), repo, _config(), now_ts=1_000 + 29 * 86_400) + + assert repo.get_equity_points() == [] + + # -- rail 11: equity must be valued from the ORDERS LOG, not from position_rule ---------------- From 7b799990aeb85c041e32dbab1b307a5bd5eed490 Mon Sep 17 00:00:00 2001 From: Elmehdi Aitbrahim Date: Fri, 4 Sep 2026 04:42:40 -0400 Subject: [PATCH 2/3] feat(web): the account-equity series, stacked above the closed-trade curve (#698) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Portfolio reality above statistical expectancy. `equityChart` keeps plotting cumulative net P&L over closed TRADES on a trade-order axis -- `build_equity_curve` argues for that axis on its own terms, and the argument is still right for that quantity. Above it now sits what the ACCOUNT was worth every cycle, traded or not, on a time axis, where the quiet week is the information rather than noise. Two questions, two charts; neither replaces the other. payload.py -- `equity_series_payload`, and `insights_payload` gains a REQUIRED `series=` keyword for the reason `journal_payload`'s `curve=` is one: a default would quietly serve a view with no chart. Segments cross AS SEGMENTS, never as one flat list; the mode partition is the whole reason the shape exists, and flattening is the single operation this wire must not make easy. Coordinates are bare strings, everything a reader is told is a `Field`, and `dd_floor_y` is `null` rather than `"0"` when the rail setting is unknown -- zero is the TOP of an SVG box, so a `"0"` would draw a ceiling in force above every reading. The reading -- the sentence a reader who cannot see the chart is given -- names the modes, because that is the part a spoken summary would otherwise flatten into "an account went from ten thousand dollars to two hundred and fifty". It says the lines are separate accounts ONLY when there is a partition to explain: told to a paper-only deployment it sends a reader looking for a line that is not there. `state` is neutral, unlike the curve's: a curve closes on a P&L, which is a verdict, and a series closes on a balance, which is not one. `modes` and `is_partitioned` are properties of `EquitySeries`, not a list the serialiser assembles. Rule 6e bans `len()` in payload.py so that a count on the wire is one the report already holds, and the first draft of that sentence broke it -- the rule was written down in `EquityCurve.point_count`'s own docstring the whole time. api.py -- the series is read on `/api/insights`, not on `/api/journal` beside the curve it is drawn above, because it describes the ACCOUNT and not the closed trades. The journal's curve narrows with that endpoint's `?limit=`; this narrows with nothing, and two charts that answer a query differently must not share a payload where a reader would assume they agree. `max_total_dd_pct` comes off the same loaded config `build_insights_report` reads, so the floor under the chart and the ceiling quoted in the account card beside it are one setting rather than two reads that can drift. Every recorded cycle, never a default window: cutting it to a span here would reintroduce the exact horizon this issue exists to remove. chart.js -- `equitySeriesChart`: one polyline per segment, plus the high-water mark and the drawdown floor drawn beneath the account line in the token the baseline already uses. The canvas is `svg.series` and NOT `svg.curve` on purpose: #602's wheel-zoom, drag-to-pan and cursor legend reach for `svg.curve` with `querySelector`, which takes the first match, and this chart renders first. Paper is dashed and live is solid, so the two accounts stay distinguishable in greyscale, on e-ink and with red-green colour deficiency -- the same rule the losing-segment dash already follows, and the same principle as payload.py's ▲/▼. render.js -- stacked, with a note saying the top chart covers every recorded cycle rather than the journal's row cap, since two charts side by side otherwise invite the assumption that the cap applies to both. A payload with no `equity_series` at all draws nothing and says nothing: that is not a deployment with no readings, it is a response from before the field existed, which a service worker can still be holding after an upgrade -- and reading `.reading.display` off it would blank the whole view over a stale cache entry that the next refresh fixes on its own. Co-Authored-By: Claude Opus 5 (1M context) --- keel/commands/insights.py | 29 +++++ keel/web/api.py | 26 +++- keel/web/payload.py | 104 +++++++++++++++- keel/web/static/css/keel.css | 39 ++++++ keel/web/static/js/chart.js | 129 ++++++++++++++++++- keel/web/static/js/render.js | 23 +++- tests/commands/test_equity_series.py | 34 +++++ tests/web/test_client_assets.py | 59 +++++++++ tests/web/test_payload.py | 178 ++++++++++++++++++++++++++- 9 files changed, 609 insertions(+), 12 deletions(-) diff --git a/keel/commands/insights.py b/keel/commands/insights.py index 8d34f175..86419301 100644 --- a/keel/commands/insights.py +++ b/keel/commands/insights.py @@ -292,6 +292,35 @@ def point_count(self) -> int: """ return sum(len(segment.points) for segment in self.segments) + @property + def modes(self) -> list[str]: + """The distinct accounts this series spans, in the order they first appear. + + NOT the segment list: paper, live, paper is three segments and two modes. The segments + are what the chart draws; this is what a sentence about the chart names, and conflating + them would have a spoken summary announce the same account twice. + + It lives on the report for the reason `point_count` does -- `keel/web/payload.py` may + not call `len()` (Rule 6e), and the serialiser deriving its own mode list would be a + second answer to a question the report already answers. + """ + seen: list[str] = [] + for segment in self.segments: + if segment.mode not in seen: + seen.append(segment.mode) + return seen + + @property + def is_partitioned(self) -> bool: + """Whether this series spans more than one account. + + The thing the chart's text equivalent turns on: the sentence explaining that each mode + is drawn as its own line must be said only when there IS a split. Told to a deployment + that has only ever run paper, it sends a reader looking for a second line that is not + there -- the same false continuity the segments prevent, pointing the other way. + """ + return len(self.modes) > 1 + def _plot_y(value: Decimal, *, low: Decimal, span: Decimal) -> Decimal: """`value` mapped into `0..PLOT_HEIGHT`, with the top of the box being `low + span`.""" diff --git a/keel/web/api.py b/keel/web/api.py index f3365944..213b5bf3 100644 --- a/keel/web/api.py +++ b/keel/web/api.py @@ -279,21 +279,39 @@ def read_orders(cfg: ServeConfig, query: Query, _state: Any, _now_ts: int) -> di def read_insights(cfg: ServeConfig, _query: Query, _state: Any, now_ts: int) -> dict[str, Any]: - """The per-rule track records and the promotion-gate distances. + """The per-rule track records, the promotion-gate distances, and the account-equity series. The journal the HTML `/insights` page renders below these is `/api/journal` instead of a second table here. One sortable collection per endpoint keeps `?sort=` unambiguous without a `?table=` beside it, and it gives the journal somewhere to carry its own `?limit=` -- the cap - the HTML page apologises for in a comment ("a cap, not a paginator").""" - from keel.commands.insights import build_insights_report + the HTML page apologises for in a comment ("a cap, not a paginator"). + + The equity series (#698) is read here rather than on `/api/journal`, next to the curve it is + drawn above, because it describes the ACCOUNT and not the closed trades. The journal's curve + narrows with that endpoint's `?limit=`; this does not narrow with anything, and two charts + that answer a query differently must not share one payload where a reader would assume they + agree. + + `max_total_dd_pct` comes off the SAME loaded config `build_insights_report` reads, so the + drawdown floor drawn under the curve and the ceiling quoted in the account card beside it are + one setting rather than two reads that can disagree. + """ + from keel.commands.insights import build_equity_series, build_insights_report repo = open_repo(cfg.db_path) try: config = load_config(cfg.config_path) report = build_insights_report(repo, config, _status_report(cfg, now_ts), now_ts) + # Every reading, not a window: the series is the long record the 7-day rolling window in + # `agent_state` never was, and cutting it to a default span here would reintroduce the + # exact horizon #698 exists to remove. A span control is a client asking for new bounds, + # which `insights.py`'s own note says would make the builder a service -- not today. + series = build_equity_series( + repo.get_equity_points(), max_total_dd_pct=config.money_mgmt.max_total_dd_pct + ) finally: close_repo(repo) - return payload.insights_payload(report) + return payload.insights_payload(report, series=series) def read_journal(cfg: ServeConfig, query: Query, _state: Any, now_ts: int) -> dict[str, Any]: diff --git a/keel/web/payload.py b/keel/web/payload.py index 82fcddf6..24178f0e 100644 --- a/keel/web/payload.py +++ b/keel/web/payload.py @@ -124,6 +124,8 @@ AccountSummary, EquityCurve, EquityPoint, + EquitySeries, + EquitySeriesPoint, GateDistance, InsightsReport, JournalEntry, @@ -907,13 +909,25 @@ def _track_record_payload(record: RuleTrackRecord) -> dict[str, Any]: } -def insights_payload(report: InsightsReport) -> dict[str, Any]: - """`build_insights_report`'s `InsightsReport`, as JSON.""" +def insights_payload(report: InsightsReport, *, series: EquitySeries) -> dict[str, Any]: + """`build_insights_report`'s `InsightsReport`, as JSON, with the equity series beside it. + + `series` is a REQUIRED keyword for the reason `journal_payload`'s `curve` is: a default would + quietly serve an insights view with no chart, and the caller is the one place that knows + which readings and which rail setting the chart is drawn from. + + It rides the insights payload rather than the journal's because it describes the ACCOUNT, not + the closed trades. The distinction is load-bearing at the other end: the journal's curve is + built from `report.entries` and therefore narrows with a `?limit=`, while this series does + not narrow with anything the journal does. Two charts that respond differently to the same + query belong to two different payloads. + """ return { "as_of": iso(report.now_ts), "generated_at": moment(report.now_ts), "account": _account_payload(report.account), "closed_trade_count": count(report.closed_trade_count), + "equity_series": equity_series_payload(series), "rules": [_track_record_payload(r) for r in report.rules], } @@ -1026,6 +1040,92 @@ def equity_curve_payload(curve: EquityCurve) -> dict[str, Any]: } +def _equity_series_point_payload(point: EquitySeriesPoint) -> dict[str, Any]: + """One cycle's reading: where to draw it, and what it says (#698). + + `x`, `y`, `hwm_y` and `dd_floor_y` are BARE STRINGS for the reason `_equity_point_payload` + gives -- they are positions inside a viewBox, not figures a human reads. `dd_floor_y` is + `null`, not `"0"`, when the rail setting is unknown: a zero coordinate is the TOP of an SVG + box, so it would draw a ceiling line in force above every reading rather than none at all. + + `unrealized` is the one leg that carries a verdict, because it is the one that is a + gain-or-loss figure. `equity`, `cash` and `hwm` are magnitudes -- an account balance is not + good or bad on its own, and `money(signed=True)` would put a ▲ on a number that has no + direction. `dd_floor` is a magnitude too: it is the rail's ceiling, not a judgement about + how close this reading sits to it. + """ + return { + "x": _plain(point.x), + "y": _plain(point.y), + "hwm_y": _plain(point.hwm_y), + "dd_floor_y": None if point.dd_floor_y is None else _plain(point.dd_floor_y), + "at": moment(point.ts), + "mode": point.mode, + "equity": money(point.equity), + "cash": money(point.cash), + "unrealized": money(point.unrealized, signed=True), + "hwm": money(point.hwm), + "dd_floor": money(point.dd_floor), + } + + +def equity_series_payload(series: EquitySeries) -> dict[str, Any]: + """`build_equity_series`'s `EquitySeries`, as JSON (#698). + + **Segments cross as segments, never as one flat list of points.** The mode partition is the + whole reason this shape exists: paper and live are unrelated accounts that share a database, + and a client handed one list would join them into a polyline showing a collapse that never + happened. Flattening is the one operation the wire must not make easy. + + `reading` is the chart's text equivalent, written here for the reason + `equity_curve_payload`'s is: summarising a chart is a judgement, and a reader who cannot see + it must be told the same thing in words. It NAMES THE MODES, because the mode split is the + part a spoken summary would otherwise flatten -- a sentence saying an account went from ten + thousand dollars to two hundred and fifty is exactly the false continuity the segments exist + to prevent, and it would be no less false for being spoken. + + `state` on that sentence is `neutral`, unlike the curve's. The curve closes on a cumulative + net P&L, which is a verdict; a series closes on an account balance, which is not one. Taking + the state from the last reading's sign would call an account "good" for holding money. + """ + low, high = money(series.low), money(series.high) + cycles = count(series.point_count) + if series.segments: + # `series.modes` and `series.is_partitioned`, never a mode list assembled here: Rule 6e + # bans `len()` in this module precisely so a count on the wire is one the report already + # holds, and a serialiser deriving its own answer is how two answers start to differ. + span = " and ".join(series.modes) + reading = ( + f"Account equity over time across {cycles['display']} cycle(s) in {span}, " + f"ranging from {low['display']} to {high['display']}." + ) + # Said only when there IS a partition. A deployment that has only ever run paper would + # otherwise be told about a second line that is not on the chart -- the same false + # continuity the segments exist to prevent, pointing the other way. + if series.is_partitioned: + reading += " Each mode is drawn as its own line; they are separate accounts." + else: + # Not "the account was flat", and not an empty string. A deployment that has not run a + # cycle since the series began recording has no readings at all, and telling those two + # apart is the whole difference between an empty chart and a broken one. + reading = "No equity readings recorded yet, so there is no series to draw." + return { + "width": _plain(series.width), + "height": _plain(series.height), + "point_count": cycles, + "low": low, + "high": high, + "reading": label("series", display=reading, state=NEUTRAL), + "segments": [ + { + "mode": segment.mode, + "points": [_equity_series_point_payload(p) for p in segment.points], + } + for segment in series.segments + ], + } + + def journal_payload(report: JournalReport, *, curve: EquityCurve) -> dict[str, Any]: """`build_journal_report`'s `JournalReport`, as JSON. diff --git a/keel/web/static/css/keel.css b/keel/web/static/css/keel.css index d942c4fc..8b364278 100644 --- a/keel/web/static/css/keel.css +++ b/keel/web/static/css/keel.css @@ -712,6 +712,45 @@ pre.joblines { .chart .dot { fill: var(--accent); opacity: 0.55; } .chart figcaption { margin: 0.6rem 0 0; } +/* ── the account-equity series (#698) ───────────────────────────────────────────────────────── + * + * The chart stacked ABOVE the closed-trade curve: what the account was worth each cycle, on a + * time axis. No `cursor: crosshair` and no `touch-action: none`, because `main.js`'s #602 + * gestures deliberately bind to `svg.curve` and not to this canvas. + * + * PAPER IS DASHED, LIVE IS SOLID, and that is not decoration. The two modes are unrelated + * accounts drawn on one canvas, so the distinction has to survive greyscale, e-ink and + * red-green colour deficiency exactly as the ▲/▼ glyphs do for the figures -- the same rule + * `chart.js::outcomeOf` follows for a losing segment. Solid reads as the real one. + * + * Both rails are drawn beneath the account line in `--muted`, the token the baseline already + * uses: they are the context the equity is read against, never the subject. The floor is dashed + * more finely than the high-water mark so the pair are told apart without colour either. + */ +.chart svg.series { + display: block; + width: 100%; + height: 14rem; + overflow: visible; +} +.chart svg.series .line.paper { stroke-dasharray: 7 4; } +.chart svg.series .hwm { + fill: none; + stroke: var(--muted); + stroke-width: 1; + stroke-dasharray: 5 3; + vector-effect: non-scaling-stroke; +} +.chart svg.series .floor { + fill: none; + stroke: var(--muted); + stroke-width: 1; + stroke-dasharray: 2 3; + opacity: 0.75; + vector-effect: non-scaling-stroke; +} +.chart svg.series .dot { r: 3; } + /* ── the cursor legend and the trade highlight (#602) ──────────────────────────────────────── * * Every colour below is one of the tokens `tests/web/test_palette_contrast.py` already measures diff --git a/keel/web/static/js/chart.js b/keel/web/static/js/chart.js index 1ae8b5db..b425c8c4 100644 --- a/keel/web/static/js/chart.js +++ b/keel/web/static/js/chart.js @@ -110,7 +110,23 @@ function svg(tag, attributes) { * @returns {string} */ function polylinePoints(points) { - return points.map(/** @param {any} point */ (point) => [point.x, point.y].join(",")).join(" "); + return polylineOn(points, "y"); +} + +/** + * The `points` attribute of a polyline over one named vertical coordinate. + * + * `key` picks which line a point contributes to -- its equity (`"y"`), the high-water mark in + * force at that instant (`"hwm_y"`), or the drawdown floor beneath it (`"dd_floor_y"`). All + * three arrive finished from `build_equity_series`; choosing between them is a lookup, not the + * arithmetic this file is not allowed to do. + * + * @param {any[]} points + * @param {string} key + * @returns {string} + */ +function polylineOn(points, key) { + return points.map(/** @param {any} point */ (point) => [point.x, point[key]].join(",")).join(" "); } /** @@ -214,6 +230,117 @@ function outcomeOf(point) { * @param {string} id the id given to the caption, which names the chart for a screen reader. * @returns {HTMLElement|null} */ +/** + * The account-equity series over time (#698), drawn ABOVE the closed-trade curve. + * + * ── WHY IT IS A SECOND CHART AND NOT A REPLACEMENT ─────────────────────────────────────────── + * + * `equityChart` plots cumulative net P&L over closed TRADES, on a trade-order axis, and + * `build_equity_curve` argues for that axis on its own terms: a quiet week must not carry the + * visual weight of fifty trades when the subject is statistical expectancy. This plots what the + * ACCOUNT was worth every cycle, traded or not, where the quiet week is the information. Two + * questions, two charts, stacked -- portfolio reality above, expectancy below. + * + * ── ONE POLYLINE PER SEGMENT, NEVER ONE PER SERIES ─────────────────────────────────────────── + * + * Paper and live are unrelated accounts that share a database and flip within it. The payload + * hands this file `segments`, already split by `build_equity_series`, precisely so that joining + * them is not something this file can do by accident: a single line from a $10,000 paper account + * to a $250 live one would draw a collapse that never happened. The modes are told apart by + * DASH as well as by colour, the same rule the losing-trade segment below follows -- paper is + * dashed because it is the synthetic one. + * + * ── THE TWO OVERLAYS ARE READ, NOT DERIVED ─────────────────────────────────────────────────── + * + * Each point carries `hwm_y` (the rail-11 high-water mark in force when the agent acted) and + * `dd_floor_y` (the equity at which rail 11 starts vetoing entries). Both are computed in + * Python from the recorded row, never here -- and `dd_floor_y` is `null` rather than `"0"` when + * the rail setting is unknown, because a zero coordinate is the TOP of the box and would draw a + * ceiling that is in force above every reading. + * + * ── NO ZOOM, NO PAN, NO CURSOR LEGEND ──────────────────────────────────────────────────────── + * + * `main.js`'s #602 gestures bind to `svg.curve`, and this canvas is `svg.series` on purpose: the + * two would otherwise fight over `contentNode.querySelector("svg.curve")`, which takes the FIRST + * match and would find this chart instead of the one the gestures were written for. Adding them + * here is a separate piece of work with its own arithmetic to place in `main.js`. + * + * @param {any} series `/api/insights`'s `data.equity_series`. + * @param {string} id the `
` id this chart is named by. + * @returns {HTMLElement|null} + */ +export function equitySeriesChart(series, id) { + if (!series || !Array.isArray(series.segments) || series.segments.length === 0) return null; + + const canvas = svg("svg", { + viewBox: ["0", "0", series.width, series.height].join(" "), + preserveAspectRatio: "none", + class: "series", + role: "img", + "aria-labelledby": id, + }); + + for (const segment of series.segments) { + // The floor first, then the ceiling, then the account: the equity line is the subject and + // sits over both rails rather than under them. + const floored = segment.points.filter( + /** @param {any} point */ (point) => point.dd_floor_y !== null, + ); + // `!== 0`, never `> 0`: `test_render_never_judges_a_value_itself` bans relational operators + // in this file, and an emptiness check has no business needing an ordering anyway. + if (floored.length !== 0) { + canvas.append( + svg("polyline", { + class: ["floor", segment.mode].join(" "), + points: polylineOn(floored, "dd_floor_y"), + }), + ); + } + canvas.append( + svg("polyline", { + class: ["hwm", segment.mode].join(" "), + points: polylineOn(segment.points, "hwm_y"), + }), + ); + const line = svg("polyline", { + class: ["line", segment.mode].join(" "), + points: polylinePoints(segment.points), + }); + // Names the account this line belongs to on hover. Redundant for a screen reader -- the + // `role="img"` above flattened this subtree and `aria-labelledby` names the modes in a + // sentence -- which is why it is a convenience and not the accessible name. + const lineTip = svg("title", {}); + lineTip.textContent = [segment.mode, "equity"].join(" "); + line.append(lineTip); + canvas.append(line); + + // A marker per reading. Same reason `equityChart` draws them: a segment of one point is a + // polyline with nothing to draw between, so the first cycle after an upgrade -- or the + // first live cycle after a flip -- would otherwise render as empty space. + for (const point of segment.points) { + const dot = svg("circle", { + class: ["dot", segment.mode].join(" "), + cx: point.x, + cy: point.y, + r: "3", + }); + const tip = svg("title", {}); + tip.textContent = [point.at.display, point.equity.display, point.mode].join(" · "); + dot.append(tip); + canvas.append(dot); + } + } + + const figure = document.createElement("figure"); + figure.className = "chart"; + const caption = document.createElement("figcaption"); + caption.className = "note"; + caption.id = id; + caption.textContent = series.reading.display; + figure.append(canvas, caption); + return figure; +} + export function equityChart(curve, id) { if (!curve || !Array.isArray(curve.points) || curve.points.length === 0) return null; diff --git a/keel/web/static/js/render.js b/keel/web/static/js/render.js index 4223dedd..e7a132e9 100644 --- a/keel/web/static/js/render.js +++ b/keel/web/static/js/render.js @@ -55,7 +55,7 @@ * the fix is in `keel/web/payload.py`. There is nowhere in this file to put it. */ -import { equityChart } from "./chart.js"; +import { equityChart, equitySeriesChart } from "./chart.js"; import { termLink } from "./docs.js"; import { instant } from "./format.js"; @@ -1676,6 +1676,27 @@ export function insightsView(insights, journal, sort, onSort, journalSort, onJou shown.append(field(journal.shown_count), " of ", field(journal.total_count), " closed trade(s)"); fragment.append(shown); + // #698: portfolio reality above statistical expectancy. The account-equity series is what the + // account was WORTH each cycle; the curve below it is what the closed trades DID. Stacked + // rather than merged because they are different quantities on different axes -- see + // `chart.js::equitySeriesChart`. + // + // It comes off `/api/insights`, not `/api/journal`, so it is NOT narrowed by the journal's + // `?limit=`. The note below says so: a reader looking at two stacked charts would otherwise + // reasonably assume the row cap above applies to both. + const series = equitySeriesChart(insights.equity_series, "h-series"); + if (series) { + const scope = el("p", "note"); + scope.append("Every recorded cycle, not only the ", field(journal.shown_count), " shown below."); + fragment.append(series, scope); + } else if (insights.equity_series) { + fragment.append(el("p", "empty", plain(insights.equity_series.reading.display))); + } + // No `else` for a payload with no `equity_series` at all. That is not a deployment with no + // readings -- it is a RESPONSE FROM BEFORE THIS FIELD EXISTED, which the service worker can + // still be holding after an upgrade. Reading `.reading.display` off it would throw and blank + // the whole insights view over a stale cache entry the next refresh fixes on its own. + const chart = equityChart(journal.curve, "h-curve"); if (chart) { fragment.append(chart); diff --git a/tests/commands/test_equity_series.py b/tests/commands/test_equity_series.py index dbf75122..e3fda811 100644 --- a/tests/commands/test_equity_series.py +++ b/tests/commands/test_equity_series.py @@ -43,6 +43,40 @@ def _reading( ) +def test_the_series_names_the_modes_it_spans_in_the_order_they_appear() -> None: + """The report holds this, not the serialiser. `keel/web/payload.py` may not call `len()` + (Rule 6e of `tests/commands/test_console_thinness.py`), and the sentence it writes for a + reader who cannot see the chart has to name the accounts -- so the list and the count are + the report's own statement about itself, exactly like `point_count`.""" + series = build_equity_series( + [ + _reading(NOW, "10000", mode="paper"), + _reading(NOW + DAY, "250", mode="live"), + _reading(NOW + 2 * DAY, "10100", mode="paper"), + ] + ) + # Three SEGMENTS, two MODES: the run order is the chart's, the mode set is the sentence's. + assert [segment.mode for segment in series.segments] == ["paper", "live", "paper"] + assert series.modes == ["paper", "live"] + assert series.is_partitioned is True + + +def test_a_single_mode_series_is_not_partitioned() -> None: + """What the sentence turns on: with one account there is no split to explain, and telling a + reader the lines are separate accounts would send them looking for a line that is not there.""" + series = build_equity_series([_reading(NOW, "10000", mode="paper")]) + + assert series.modes == ["paper"] + assert series.is_partitioned is False + + +def test_an_empty_series_spans_no_modes() -> None: + series = build_equity_series([]) + + assert series.modes == [] + assert series.is_partitioned is False + + def test_no_readings_is_a_real_answer_not_an_empty_chart() -> None: """A deployment that has not run a cycle since v19 has no series. Distinct from a flat line, which is what an account that did not move looks like.""" diff --git a/tests/web/test_client_assets.py b/tests/web/test_client_assets.py index 7c5e42f8..3b3cb11a 100644 --- a/tests/web/test_client_assets.py +++ b/tests/web/test_client_assets.py @@ -1330,3 +1330,62 @@ def test_the_status_key_scan_found_the_keys_it_claims_to_check() -> None: # And the scan is bounded: `data.build` belongs to `/api/config` and is read by `buildLine`, # which lives BELOW `statusView` in the same file. assert "build" not in keys, keys + + +# -- the account-equity series, stacked above the curve (#698) --------------------------------- + + +def test_paper_and_live_are_told_apart_by_dash_not_only_by_colour() -> None: + """The same constraint the losing segment above answers, for the other pair of lines a reader + has to tell apart. Paper and live are unrelated accounts drawn on one canvas, so if the only + difference between their lines were a hue, a reader with red-green colour deficiency -- or + anyone on e-ink or a greyscale printout -- would see one continuous account.""" + css = staticfiles.STATIC_ROOT.joinpath("css", "keel.css").read_text(encoding="utf-8") + paper_rule = re.search(r"\.chart svg\.series \.line\.paper\s*\{([^}]*)\}", css) + assert paper_rule is not None, "the paper equity line has no rule of its own" + assert "stroke-dasharray" in paper_rule.group(1), ( + "the paper line must be dashed so the synthetic account survives greyscale" + ) + # And live must NOT be dashed, or dash stops being the thing that separates them. + live_rule = re.search(r"\.chart svg\.series \.line\.live\s*\{([^}]*)\}", css) + assert live_rule is None or "stroke-dasharray" not in live_rule.group(1), ( + "the live line must stay solid -- if both are dashed, dash is no longer the signal" + ) + + +def test_the_series_canvas_does_not_answer_to_the_curves_selector() -> None: + """`main.js`'s #602 wheel-zoom, drag-to-pan and cursor legend bind to `svg.curve` and reach + for it with `querySelector`, which takes the FIRST match in the DOM. The series is rendered + ABOVE the curve, so if it also called itself `curve` every one of those gestures would + silently retarget onto a chart they were not written for.""" + chart_code = _source("chart.js") + assert 'class: "series"' in chart_code, "the series canvas must not be class 'curve'" + + render_code = _source("render.js") + series_at = render_code.index("equitySeriesChart(") + curve_at = render_code.index("equityChart(journal.curve") + assert series_at < curve_at, "the equity series is stacked ABOVE the closed-trade curve" + + +def test_the_series_draws_each_mode_as_its_own_polyline() -> None: + """The one thing this chart must never do is join two accounts into one line. It is checked + on the source rather than a rendered DOM because a browser cannot run here: the loop over + `segments` IS the guarantee, and a `points` attribute built from a flattened list would be + the bug.""" + chart_code = _source("chart.js") + assert "for (const segment of series.segments)" in chart_code, ( + "the series must be drawn one segment at a time, never as one flat list of points" + ) + assert ".flat(" not in chart_code and "concat(" not in chart_code, ( + "flattening the segments would draw the paper/live flip as a single continuous line" + ) + + +def test_an_unknown_drawdown_ceiling_draws_no_floor_line() -> None: + """`dd_floor_y` is `null` when the rail setting is unknown, and a `null` coordinate must be + filtered out rather than drawn: SVG's zero is the TOP of the box, so a floor placed there + reads as a ceiling in force above every reading -- the opposite of "not known".""" + chart_code = _source("chart.js") + assert "point.dd_floor_y !== null" in chart_code, ( + "points with no recorded drawdown floor must be filtered before the floor is drawn" + ) diff --git a/tests/web/test_payload.py b/tests/web/test_payload.py index d999333e..0d07d01b 100644 --- a/tests/web/test_payload.py +++ b/tests/web/test_payload.py @@ -37,6 +37,7 @@ from typing import Any import pytest +from keel_core.types import EquityReading from keel.capabilities import CAPABILITIES, GATES from keel.commands.activity import ActivityCycle, ActivityEvent, ActivityFeed @@ -48,6 +49,7 @@ JournalReport, RuleTrackRecord, build_equity_curve, + build_equity_series, ) from keel.commands.jobs import JobStatus from keel.commands.setup import ACTIONS, STEPS, DeploymentState, StepState @@ -399,7 +401,12 @@ def _every_payload() -> dict[str, Any]: second copy to drift.""" return { "status": payload.status_payload(_status_report()), - "insights": payload.insights_payload(_insights_report()), + "insights": payload.insights_payload( + _insights_report(), + series=build_equity_series( + _equity_readings(), max_total_dd_pct=Decimal("0.20") + ), + ), "journal": _journal_json(), "activity": payload.activity_payload(_activity_feed()), "config": payload.config_payload( @@ -805,7 +812,9 @@ def test_a_win_rate_float_is_re_encoded_not_recomputed() -> None: """`RuleTrackRecord.win_rate` is a `float` upstream -- a statistic, never money. It reaches the wire through its own shortest round-trip repr, so the figure on the wire is the figure the report held and nothing was recomputed on the way.""" - built = payload.insights_payload(_insights_report()) + built = payload.insights_payload( + _insights_report(), series=build_equity_series(_equity_readings()) + ) assert built["rules"][0]["win_rate"]["value"] == "41.5" assert built["rules"][0]["win_rate"]["display"] == "41.5%" @@ -1316,7 +1325,10 @@ def test_every_builder_survives_a_completely_empty_report() -> None: ) documents = { "status": payload.status_payload(empty_status), - "insights": payload.insights_payload(_insights_report(rules=[], closed_trade_count=0)), + "insights": payload.insights_payload( + _insights_report(rules=[], closed_trade_count=0), + series=build_equity_series([]), + ), "journal": _journal_json(entries=[], total_count=0, filters={}), "activity": payload.activity_payload( ActivityFeed(status="missing", source="/tmp/nope.log") @@ -1336,6 +1348,16 @@ def test_every_payload_is_json_serialisable_without_a_custom_encoder(builder: st matters beyond tidiness: `json.dumps(Decimal(...))` raises, and the natural fix a hurried author reaches for is `default=float`, which is the contract's exact failure mode installed as a convenience.""" + if builder == "insights_payload": + # A second builder with a required keyword, for the same reason: a default + # `series` would serve an insights view with no chart. + json.dumps( + payload.insights_payload( + _insights_report(), series=build_equity_series(_equity_readings()) + ) + ) + return + if builder == "journal_payload": # The one builder with a second required argument. Spelled out rather than folded into # the table below, because folding it in would mean a default somewhere -- and the whole @@ -1346,8 +1368,156 @@ def test_every_payload_is_json_serialisable_without_a_custom_encoder(builder: st other = { "status_payload": _status_report(), - "insights_payload": _insights_report(), "activity_payload": _activity_feed(), }[builder] json.dumps(getattr(payload, builder)(other)) # no cls=, no default= + + +# -- the account-equity series (#698) --------------------------------------------------------- +# +# Serialised beside the closed-trade curve, never instead of it: the two answer different +# questions (what the ACCOUNT was worth, whichever cycles ran; what the closed TRADES did, in the +# order they closed). The rules below are the ones the contract already applies to the curve -- +# money as strings, coordinates bare, judgements written here -- plus the one this chart adds: +# a mode is a partition, and the payload must never let a client join across it. + + +def _equity_readings() -> list[EquityReading]: + """Two modes, a rebased high-water mark, and a cycle whose split was never recorded.""" + return [ + EquityReading( + ts=NOW_TS - 3 * 86_400, + mode="paper", + equity=Decimal("10000.55"), + cash=Decimal("9000.25"), + unrealized=Decimal("-12.30"), + hwm=Decimal("10500.00"), + ), + EquityReading( + ts=NOW_TS - 2 * 86_400, + mode="paper", + equity=Decimal("10600"), + cash=None, + unrealized=None, + hwm=Decimal("10600"), + ), + EquityReading( + ts=NOW_TS, + mode="live", + equity=Decimal("250.10"), + cash=Decimal("250.10"), + unrealized=Decimal("0"), + hwm=Decimal("250.10"), + ), + ] + + +def _series_json(max_total_dd_pct: Decimal | None = Decimal("0.20")) -> dict[str, Any]: + return payload.equity_series_payload( + build_equity_series(_equity_readings(), max_total_dd_pct=max_total_dd_pct) + ) + + +def test_the_series_crosses_as_one_run_of_points_per_mode() -> None: + """The partition, on the wire. Two paper cycles then a live one is two segments, and a client + handed one flat list would draw a line from $10,600 of paper money to $250 of real money and + call the drop a drawdown.""" + modes = [segment["mode"] for segment in _series_json()["segments"]] + counts = [len(segment["points"]) for segment in _series_json()["segments"]] + + assert modes == ["paper", "live"] + assert counts == [2, 1] + + +def test_a_segments_mode_is_a_bare_string_not_a_field() -> None: + """`mode` is an identifier, like `product_id` and `rule_name`: no precision hazard, no + rounding, no judgement. WHAT IS NOT A FIELD, in the module docstring.""" + assert _series_json()["segments"][0]["mode"] == "paper" + + +def test_coordinates_are_bare_strings_and_the_figures_beside_them_are_fields() -> None: + """The same split `_equity_point_payload` documents: `x`/`y` are positions inside a viewBox, + with nothing to format and no judgement to carry, while everything a reader is TOLD arrives + as a field.""" + point = _series_json()["segments"][0]["points"][0] + + assert point["x"] == "0.00" + assert isinstance(point["y"], str) + assert point["equity"]["display"] == "$10,000.55" + assert point["equity"]["value"] == "10000.55" + assert point["at"]["value"].endswith("Z") + + +def test_an_unrecorded_split_crosses_as_absent_not_as_zero() -> None: + """A cycle that knew its total but not its split. `$0.00` would state a flat cash balance + and a flat unrealized P&L, and neither was observed -- the same distinction `pnl_net` draws + for a trade with no recorded net.""" + point = _series_json()["segments"][0]["points"][1] + + assert point["cash"]["state"] == "unknown" + assert point["unrealized"]["state"] == "unknown" + assert point["equity"]["state"] == "neutral" + + +def test_the_unrealized_leg_carries_its_own_sign_as_a_state() -> None: + """Rule 3: a client must never decide "this is bad" from a minus sign. Unrealized P&L is a + gain-or-loss figure, so it is the one leg of the split that carries a verdict.""" + point = _series_json()["segments"][0]["points"][0] + + assert point["unrealized"]["state"] == "bad" + assert point["unrealized"]["display"].startswith("\u25bc") + + +def test_the_drawdown_floor_is_absent_rather_than_zero_when_the_rail_is_unknown() -> None: + """A caller that did not supply `max_total_dd_pct` has not said the ceiling is zero. The + coordinate goes `null` for the same reason: a `"0"` would place the line at the top of the + box, where it reads as a ceiling in force.""" + point = _series_json(max_total_dd_pct=None)["segments"][0]["points"][0] + + assert point["dd_floor"]["state"] == "unknown" + assert point["dd_floor_y"] is None + + +def test_the_drawdown_floor_crosses_with_a_coordinate_when_the_rail_is_known() -> None: + point = _series_json()["segments"][0]["points"][0] + + # 10500.00 * (1 - 0.20), exact: the payload never rounds a source Decimal. + assert point["dd_floor"]["value"] == "8400.0000" + assert point["dd_floor"]["display"] == "$8,400.00" + assert isinstance(point["dd_floor_y"], str) + + +def test_the_reading_says_what_the_chart_shows_including_the_flip() -> None: + """The chart's text equivalent, written HERE for the reason every other sentence on this wire + is. It has to name the mode split: a reader who cannot see the two separate lines is + otherwise told a single account went from ten thousand dollars to two hundred.""" + reading = _series_json()["reading"] + + assert "paper" in reading["display"] + assert "live" in reading["display"] + assert reading["state"] in ("good", "warn", "bad", "neutral", "unknown") + + +def test_a_single_mode_is_not_told_it_is_two_separate_accounts() -> None: + """The sentence explaining the split is what a reader who cannot see the chart is told about + the mode partition -- so it must not be said when there is no partition. A deployment that + has only ever run paper hears "they are separate accounts" and goes looking for a second line + that is not there, which is the same false continuity the segments prevent, inverted.""" + only_paper = payload.equity_series_payload(build_equity_series(_equity_readings()[:2])) + + assert "paper" in only_paper["reading"]["display"] + assert "separate accounts" not in only_paper["reading"]["display"] + # And the two-mode case still explains itself. + assert "separate accounts" in _series_json()["reading"]["display"] + + +def test_an_empty_series_says_so_rather_than_describing_an_empty_chart() -> None: + """Not "the account was flat", and not an empty string. A deployment that has not completed a + cycle since the upgrade has no readings at all, and which of those two a reader is looking at + is the whole difference between an empty chart and a broken one.""" + empty = payload.equity_series_payload(build_equity_series([])) + + assert empty["segments"] == [] + assert empty["point_count"]["value"] == "0" + assert "no" in empty["reading"]["display"].lower() From 04b422ac49ef1e6bd39d418c1e774532898b3d07 Mon Sep 17 00:00:00 2001 From: Elmehdi Aitbrahim Date: Fri, 4 Sep 2026 07:36:59 -0400 Subject: [PATCH 3/3] fix(web): the series must not steal the curve's figure, or read the table unbounded (#698) Two review findings on the equity series, plus the test that was missing. THE REGRESSION. `equitySeriesChart` set `figure.className = "chart"`, and `main.js` reaches for the chart's WRAPPER twice -- `highlightJournalRow` and the chart-action handler -- with `contentNode.querySelector("figure.chart")`, which takes the first match in document order. The series is appended ABOVE the curve, so both lookups landed on it; `highlightTrade` then found no `.highlight` group, returned early, and hovering a journal row on /insights silently highlighted nothing. It worked before this branch, so it was a pure regression. The original reasoning got the CANVAS right and stopped there: `svg.series` was chosen deliberately so the #602 pointer gestures could not retarget, and the comment says so at length. It never asked what else `main.js` selects on. The figure is now `"chart series"` and both lookups carry `:not(.series)`; a test pins both halves, because either alone leaves the collision live. THE UNBOUNDED READ. `/api/insights` read the whole `equity_points` table on every request. The table is append-only and grows one row per cycle forever -- at the default `auto_trade.interval_sec` of 900 that is ~35,000 rows a year -- and `main.js` re-polls the view every 15 seconds, rebuilding one SVG circle per point each time. Measured at 580 bytes per point in the rendered payload, a year of the hourly paper profile is 4.8 MiB per response, re-fetched four times a minute. The previous note called the unbounded read deliberate ("every recorded cycle, never a default window"); the choice was defensible and its cost was not bounded, which is the half that did not hold. `EQUITY_POINT_LIMIT = 1000`, newest first, sized off `PLOT_WIDTH`: past one reading per plot unit the extra rows land on coordinates already drawn, so they cost bytes with nothing on screen to show for it. A fixed cap rather than a `?limit=` because this route carries no collection to page and the whole series is on disk for anyone who wants it. A BOUNDED READ HAS TO SAY SO. Silently starting the line wherever the cap fell would misstate the span of the record while every individual point stayed true -- the harder kind of wrong to notice, and the kind this chart exists to avoid. So `count_equity_points()` rides along, `EquitySeries` carries `total_recorded` and `is_truncated`, the spoken reading says "the most recent N" and names the total, and the note under the chart reads "N of M recorded cycle(s)". `total_recorded=None` means the caller did not say, which is deliberately NOT a claim of completeness: inventing "complete" from silence is the failure that guard exists for. `get_equity_points(limit=)` keeps the most recent N and still returns them oldest first -- the ordering is the caller's contract, not an artefact of how rows were selected -- and the limit applies inside the mode filter, so asking for the last two live readings on a paper-heavy database cannot return nothing. THE MISSING TEST. All three agent tests were on the live branch. The paper branch now has one through the real loop, asserting the recorded parts add back to the recorded total on a cycle whose account seeded and traded before the reading -- the case where a total and a split could come from two different states of the account. Both halves of the wiring were mutation-checked. Co-Authored-By: Claude Opus 5 (1M context) --- keel/commands/insights.py | 33 +++++++++++++++-- keel/data/repository.py | 53 ++++++++++++++++++++++++---- keel/web/api.py | 32 ++++++++++++++--- keel/web/payload.py | 16 ++++++++- keel/web/static/js/chart.js | 9 ++++- keel/web/static/js/main.js | 12 +++++-- keel/web/static/js/render.js | 13 ++++++- tests/commands/test_equity_series.py | 29 +++++++++++++++ tests/data/test_equity_points.py | 42 ++++++++++++++++++++++ tests/test_agent.py | 44 +++++++++++++++++++++++ tests/web/test_client_assets.py | 25 +++++++++++++ tests/web/test_payload.py | 22 ++++++++++++ 12 files changed, 311 insertions(+), 19 deletions(-) diff --git a/keel/commands/insights.py b/keel/commands/insights.py index 86419301..e6630a62 100644 --- a/keel/commands/insights.py +++ b/keel/commands/insights.py @@ -282,6 +282,11 @@ class EquitySeries: high: Decimal width: Decimal height: Decimal + #: How many readings the TABLE holds, when the caller bounded its read and knows. `None` + #: means the caller did not say -- which is deliberately not the same as "nothing was left + #: out", because a series that asserted completeness on behalf of a caller that never + #: counted would be the chart lying about its own span. + total_recorded: int | None = None @property def point_count(self) -> int: @@ -292,6 +297,19 @@ def point_count(self) -> int: """ return sum(len(segment.points) for segment in self.segments) + @property + def is_truncated(self) -> bool: + """Whether this series is a WINDOW onto a longer record. + + What the text equivalent turns on. A bounded read is honest only if it says so: a chart + that quietly begins wherever a row cap fell would misstate the span of the record while + every individual point remained true, which is the harder kind of wrong to notice. + + `False` when the total is unknown -- an unstated total is not a claim in either + direction, and inventing "complete" from silence is the failure this guards. + """ + return self.total_recorded is not None and self.total_recorded > self.point_count + @property def modes(self) -> list[str]: """The distinct accounts this series spans, in the order they first appear. @@ -332,6 +350,7 @@ def build_equity_series( readings: Sequence[EquityReading], *, max_total_dd_pct: Decimal | None = None, + total_recorded: int | None = None, ) -> EquitySeries: """The account-equity series over `readings`, oldest first, ready to draw (#698). @@ -363,7 +382,12 @@ def build_equity_series( """ if not readings: return EquitySeries( - segments=[], low=_ZERO, high=_ZERO, width=PLOT_WIDTH, height=PLOT_HEIGHT + segments=[], + low=_ZERO, + high=_ZERO, + width=PLOT_WIDTH, + height=PLOT_HEIGHT, + total_recorded=total_recorded, ) floors = [ @@ -423,7 +447,12 @@ def _x(ts: int) -> Decimal: segments.append(EquitySeriesSegment(mode=str(current_mode), points=current)) return EquitySeries( - segments=segments, low=low, high=high, width=PLOT_WIDTH, height=PLOT_HEIGHT + segments=segments, + low=low, + high=high, + width=PLOT_WIDTH, + height=PLOT_HEIGHT, + total_recorded=total_recorded, ) diff --git a/keel/data/repository.py b/keel/data/repository.py index 72eb6a40..5a2bb678 100644 --- a/keel/data/repository.py +++ b/keel/data/repository.py @@ -743,15 +743,32 @@ def record_equity_point(self, point: EquityReading) -> None: self._conn.commit() def get_equity_points( - self, mode: str | None = None, since_ts: int | None = None + self, + mode: str | None = None, + since_ts: int | None = None, + limit: int | None = None, ) -> list[EquityReading]: - """The series, oldest first, optionally narrowed to one mode and/or a time window. + """The series, oldest first, optionally narrowed to one mode, a time window, or a count. `mode=None` returns paper AND live rows interleaved by time. That is the honest raw read, but it is NOT a curve: a caller drawing it as one line joins two unrelated accounts across the flip. Readers that plot must group by `mode` (`insights` does). + + `limit` keeps the MOST RECENT `limit` readings and still returns them oldest first. Two + halves of one decision: + + * **Most recent, not first.** This table is append-only and grows one row per cycle + forever -- at the default `auto_trade.interval_sec` of 900 that is ~35,000 rows a + year. A cap that kept the OLDEST rows would answer "where is this account now?" with + the readings furthest from the answer, and would freeze the chart the day the cap was + reached. + * **Still oldest first.** The ordering is the caller's contract, not an artefact of how + the rows were selected. `ORDER BY ts DESC LIMIT n` in a subquery, re-ordered outside + it, so a bounded read and an unbounded one differ only in how much they return. + + A caller that bounds a read is showing a WINDOW of the record and must say so: + `count_equity_points` is how it learns what it is leaving out. """ - query = "SELECT * FROM equity_points" clauses: list[str] = [] params: list[object] = [] if mode is not None: @@ -760,13 +777,35 @@ def get_equity_points( if since_ts is not None: clauses.append("ts >= ?") params.append(since_ts) - if clauses: - query += " WHERE " + " AND ".join(clauses) + where = (" WHERE " + " AND ".join(clauses)) if clauses else "" # `id` breaks the tie so two readings at the same epoch second keep insertion order -- - # `ts` alone leaves that to SQLite, and a chart would draw them in an arbitrary one. - query += " ORDER BY ts, id" + # `ts` alone leaves that to SQLite, and a chart would draw them in an arbitrary one. It + # is applied in BOTH directions below so the newest-N and the oldest-first re-order + # agree about which of two same-second readings is the newer. + if limit is None: + query = f"SELECT * FROM equity_points{where} ORDER BY ts, id" + else: + query = ( + f"SELECT * FROM (SELECT * FROM equity_points{where} " + "ORDER BY ts DESC, id DESC LIMIT ?) ORDER BY ts, id" + ) + params.append(limit) return [_equity_point_from_row(row) for row in self._conn.execute(query, params)] + def count_equity_points(self, mode: str | None = None) -> int: + """How many readings the table holds -- the total a bounded read is a window ONTO. + + A `COUNT(*)`, never `len(get_equity_points())`: the entire point is to learn the size + without paying to materialise the rows, which is what the caller just declined to do. + """ + query = "SELECT COUNT(*) AS n FROM equity_points" + params: list[object] = [] + if mode is not None: + query += " WHERE mode = ?" + params.append(mode) + row = self._conn.execute(query, params).fetchone() + return int(row["n"]) + # -- trade outcomes (closed round-trips; rails 11 and 16) --------------- def insert_trade_outcome(self, outcome: dict[str, Any]) -> int: diff --git a/keel/web/api.py b/keel/web/api.py index 213b5bf3..6a07fd6b 100644 --- a/keel/web/api.py +++ b/keel/web/api.py @@ -54,6 +54,23 @@ #: journal` is, and it runs in the operator's own process against their own machine's limits. MAX_JOURNAL_LIMIT = 1000 +#: How many equity readings `/api/insights` draws, newest first (#698). +#: +#: A fixed cap rather than a `?limit=`: this route carries no collection a caller sorts or pages, +#: and a chart is not a bulk export -- the whole series is on disk for anyone who wants it. +#: +#: 1000 because the plot box is `PLOT_WIDTH` = 1000 units wide, so past one reading per unit the +#: extra rows land on coordinates the chart has already drawn: cost with nothing on screen to +#: show for it. Measured at 580 bytes per point in the rendered payload, this caps the response +#: near 580 KB; the page re-polls every 15 seconds (`main.js`'s `POLL_MS`), which is what makes +#: an unbounded read here a recurring cost rather than a one-off one. At the default +#: `auto_trade.interval_sec` of 900 the table passes this cap in about ten days. +#: +#: What it would take to change: a chart the operator can pan or zoom over a range they choose. +#: That makes the range a client concern and this builder a service the client re-asks with new +#: bounds -- the same condition `insights.py`'s own module note already names. +EQUITY_POINT_LIMIT = 1000 + #: The two directions, and no third spelling. `desc`/`descending`/`down` would all have to be #: accepted forever once accepted once, and a client reading `sort.direction` back off the #: response needs one word to compare against. @@ -295,6 +312,13 @@ def read_insights(cfg: ServeConfig, _query: Query, _state: Any, now_ts: int) -> `max_total_dd_pct` comes off the SAME loaded config `build_insights_report` reads, so the drawdown floor drawn under the curve and the ceiling quoted in the account card beside it are one setting rather than two reads that can disagree. + + The series read is BOUNDED (`EQUITY_POINT_LIMIT`) and says so. `equity_points` is + append-only and grows one row per cycle forever, so an unbounded read here would be this + route's memory cost rising without limit for the life of the deployment -- the exact hazard + `MAX_JOURNAL_LIMIT`'s note names, on a route with no `?limit=` for an operator to moderate + it with. `count_equity_points` is passed alongside so the chart can state what it is not + showing rather than quietly beginning wherever the cap fell. """ from keel.commands.insights import build_equity_series, build_insights_report @@ -302,12 +326,10 @@ def read_insights(cfg: ServeConfig, _query: Query, _state: Any, now_ts: int) -> try: config = load_config(cfg.config_path) report = build_insights_report(repo, config, _status_report(cfg, now_ts), now_ts) - # Every reading, not a window: the series is the long record the 7-day rolling window in - # `agent_state` never was, and cutting it to a default span here would reintroduce the - # exact horizon #698 exists to remove. A span control is a client asking for new bounds, - # which `insights.py`'s own note says would make the builder a service -- not today. series = build_equity_series( - repo.get_equity_points(), max_total_dd_pct=config.money_mgmt.max_total_dd_pct + repo.get_equity_points(limit=EQUITY_POINT_LIMIT), + max_total_dd_pct=config.money_mgmt.max_total_dd_pct, + total_recorded=repo.count_equity_points(), ) finally: close_repo(repo) diff --git a/keel/web/payload.py b/keel/web/payload.py index 24178f0e..eacc623e 100644 --- a/keel/web/payload.py +++ b/keel/web/payload.py @@ -1090,15 +1090,22 @@ def equity_series_payload(series: EquitySeries) -> dict[str, Any]: """ low, high = money(series.low), money(series.high) cycles = count(series.point_count) + truncated_from = count(series.total_recorded) if series.segments: # `series.modes` and `series.is_partitioned`, never a mode list assembled here: Rule 6e # bans `len()` in this module precisely so a count on the wire is one the report already # holds, and a serialiser deriving its own answer is how two answers start to differ. span = " and ".join(series.modes) + # "the most recent N", never a bare N, when the read was bounded. The sentence is the + # whole of what a reader who cannot see the chart is told about its span, so a window + # described as a history is a lie by omission that every individual point survives. + scope = "the most recent " if series.is_truncated else "" reading = ( - f"Account equity over time across {cycles['display']} cycle(s) in {span}, " + f"Account equity over time across {scope}{cycles['display']} cycle(s) in {span}, " f"ranging from {low['display']} to {high['display']}." ) + if series.is_truncated: + reading += f" {truncated_from['display']} readings are recorded in total." # Said only when there IS a partition. A deployment that has only ever run paper would # otherwise be told about a second line that is not on the chart -- the same false # continuity the segments exist to prevent, pointing the other way. @@ -1113,6 +1120,13 @@ def equity_series_payload(series: EquitySeries) -> dict[str, Any]: "width": _plain(series.width), "height": _plain(series.height), "point_count": cycles, + "total_recorded": truncated_from, + # A `flag`, not a bare boolean: a client must not have to compare `point_count` against + # `total_recorded` to learn this. That comparison is arithmetic, and the answer is a + # statement about how much of the record the chart is showing -- a judgement, made here. + "is_truncated": flag( + series.is_truncated, on="a window onto a longer record", off="the whole record" + ), "low": low, "high": high, "reading": label("series", display=reading, state=NEUTRAL), diff --git a/keel/web/static/js/chart.js b/keel/web/static/js/chart.js index b425c8c4..f2426452 100644 --- a/keel/web/static/js/chart.js +++ b/keel/web/static/js/chart.js @@ -332,7 +332,14 @@ export function equitySeriesChart(series, id) { } const figure = document.createElement("figure"); - figure.className = "chart"; + // `"chart series"`, never a bare `"chart"`. `svg.series` keeps the #602 POINTER gestures off + // this canvas, but `main.js` also reaches for the WRAPPER twice -- `highlightJournalRow` and + // the chart-action handler -- with `querySelector`, which takes the first match in document + // order, and this figure is appended ABOVE the curve's. Left as a bare `figure.chart` it + // intercepts both: `highlightTrade` finds no `.highlight` group here, returns early, and + // hovering a journal row silently stops highlighting anything. The second half of the fix is + // the `:not(.series)` in those two selectors. + figure.className = "chart series"; const caption = document.createElement("figcaption"); caption.className = "note"; caption.id = id; diff --git a/keel/web/static/js/main.js b/keel/web/static/js/main.js index acc97c0c..9d89f5f7 100644 --- a/keel/web/static/js/main.js +++ b/keel/web/static/js/main.js @@ -977,7 +977,11 @@ function reapplyChartView() { * @param {HTMLElement|null} row */ function highlightJournalRow(row) { - const figure = contentNode.querySelector("figure.chart"); + // `:not(.series)` because /insights renders TWO `figure.chart`s and the account-equity series + // (#698) is the first in document order. Without it this lookup returns that figure, which + // carries no `.highlight` group, and `highlightTrade` returns early -- the highlight silently + // stops working, with nothing in the console to say why. + const figure = contentNode.querySelector("figure.chart:not(.series)"); if (!(figure instanceof HTMLElement)) return; if (!row || !activeCurve) { @@ -1111,7 +1115,11 @@ contentNode.addEventListener("click", (event) => { if (!button || !activeCurve) return; const canvas = contentNode.querySelector("svg.curve"); - const figure = contentNode.querySelector("figure.chart"); + // Paired with the canvas above, so both must name the SAME chart: `svg.curve` already + // excludes the #698 series canvas, and `:not(.series)` is what keeps the wrapper lookup in + // step with it. Mismatched, "Save as image" would read its background colour off one figure + // and its geometry off another. + const figure = contentNode.querySelector("figure.chart:not(.series)"); if (!(canvas instanceof SVGSVGElement) || !(figure instanceof HTMLElement)) return; const action = button.getAttribute("data-chart-action"); diff --git a/keel/web/static/js/render.js b/keel/web/static/js/render.js index e7a132e9..a4dd719d 100644 --- a/keel/web/static/js/render.js +++ b/keel/web/static/js/render.js @@ -1686,8 +1686,19 @@ export function insightsView(insights, journal, sort, onSort, journalSort, onJou // reasonably assume the row cap above applies to both. const series = equitySeriesChart(insights.equity_series, "h-series"); if (series) { + // What the top chart's span actually is, in words, next to the chart. `is_truncated` is a + // `flag` whose `display` already says which case this is -- the client never compares + // `point_count` against `total_recorded` to find out, because that comparison is arithmetic + // and the answer is a claim about how much of the record is on screen. const scope = el("p", "note"); - scope.append("Every recorded cycle, not only the ", field(journal.shown_count), " shown below."); + scope.append( + field(insights.equity_series.point_count), + " of ", + field(insights.equity_series.total_recorded), + " recorded cycle(s) — ", + field(insights.equity_series.is_truncated), + ". Not narrowed by the journal row cap below.", + ); fragment.append(series, scope); } else if (insights.equity_series) { fragment.append(el("p", "empty", plain(insights.equity_series.reading.display))); diff --git a/tests/commands/test_equity_series.py b/tests/commands/test_equity_series.py index e3fda811..95917db3 100644 --- a/tests/commands/test_equity_series.py +++ b/tests/commands/test_equity_series.py @@ -43,6 +43,35 @@ def _reading( ) +def test_a_window_onto_a_longer_record_knows_it_is_one() -> None: + """A bounded read must not draw as though it were the whole history. The chart's own claim + is that these are the numbers the engine acted on; starting the line wherever a row cap + happened to fall, silently, would make the span of the record a lie by omission.""" + series = build_equity_series( + [_reading(NOW, "10000"), _reading(NOW + DAY, "10100")], total_recorded=900 + ) + + assert series.is_truncated is True + assert series.total_recorded == 900 + assert series.point_count == 2 + + +def test_a_series_holding_every_recorded_reading_is_not_truncated() -> None: + series = build_equity_series([_reading(NOW, "10000")], total_recorded=1) + + assert series.is_truncated is False + + +def test_an_unstated_total_is_not_a_claim_that_nothing_was_left_out() -> None: + """`total_recorded=None` means the caller did not say. Defaulting it to the point count + would have the series assert it is complete on behalf of a caller that never checked -- + so the honest answer to "is this truncated?" is not-known, which reads as no claim.""" + series = build_equity_series([_reading(NOW, "10000")]) + + assert series.total_recorded is None + assert series.is_truncated is False + + def test_the_series_names_the_modes_it_spans_in_the_order_they_appear() -> None: """The report holds this, not the serialiser. `keel/web/payload.py` may not call `len()` (Rule 6e of `tests/commands/test_console_thinness.py`), and the sentence it writes for a diff --git a/tests/data/test_equity_points.py b/tests/data/test_equity_points.py index 22455296..06725c48 100644 --- a/tests/data/test_equity_points.py +++ b/tests/data/test_equity_points.py @@ -122,6 +122,48 @@ def test_a_since_window_is_bounded_by_epoch_order_not_string_order(repo: Reposit assert [p.ts for p in repo.get_equity_points(since_ts=NOW)] == [NOW] +def test_a_limit_takes_the_MOST_RECENT_readings_still_oldest_first(repo: Repository) -> None: + """A bounded read has to keep the END of the series, not the beginning. The chart's subject + is where the account is now; the first N rows ever written are the least interesting answer + to that, and on a long-running deployment they are also the ones furthest from the truth.""" + for offset in range(5): + repo.record_equity_point(_point(ts=NOW + offset * DAY, equity=str(1000 + offset))) + + got = repo.get_equity_points(limit=2) + + assert [p.ts for p in got] == [NOW + 3 * DAY, NOW + 4 * DAY] + assert [p.equity for p in got] == [Decimal("1003"), Decimal("1004")] + + +def test_a_limit_larger_than_the_table_returns_everything(repo: Repository) -> None: + repo.record_equity_point(_point(ts=NOW)) + assert len(repo.get_equity_points(limit=500)) == 1 + + +def test_a_limit_composes_with_the_mode_partition(repo: Repository) -> None: + """The limit must be applied WITHIN the mode, not to a blended read that is then filtered -- + otherwise asking for the last two live readings on a paper-heavy database returns nothing.""" + for offset in range(5): + repo.record_equity_point(_point(ts=NOW + offset * DAY, mode="paper")) + for offset in range(5, 8): + repo.record_equity_point(_point(ts=NOW + offset * DAY, mode="live", equity="250")) + + got = repo.get_equity_points(mode="live", limit=2) + + assert [p.ts for p in got] == [NOW + 6 * DAY, NOW + 7 * DAY] + + +def test_the_recorded_count_is_readable_without_reading_the_rows(repo: Repository) -> None: + """What lets a bounded chart say how much it is NOT showing. A count is the whole reason the + truncation can be stated honestly instead of the series quietly starting wherever the cap + happened to fall.""" + for offset in range(7): + repo.record_equity_point(_point(ts=NOW + offset * DAY)) + + assert repo.count_equity_points() == 7 + assert repo.count_equity_points(mode="live") == 0 + + def test_an_existing_database_gains_the_table_on_migration() -> None: conn = connect(":memory:") migrate(conn) diff --git a/tests/test_agent.py b/tests/test_agent.py index 0407b28c..b75e2318 100644 --- a/tests/test_agent.py +++ b/tests/test_agent.py @@ -1420,6 +1420,50 @@ def test_a_live_cycle_records_the_equity_point_with_its_cash_and_unrealized( assert point.equity == Decimal("1000000") + Decimal("2") * Decimal("100") +def test_a_paper_cycle_records_the_split_off_the_synthetic_account(repo, monkeypatch) -> None: + """The paper branch's wiring, through the real loop rather than on `PaperTrader` alone. + + The two are different code: `tests/strategy/test_paper.py` proves `get_cash()` and + `unrealized()` agree with `equity()` on one account state, and this proves the AGENT hands + those two to `update_drawdown` for the same cycle it read the equity from -- a paper account + that seeds itself DURING this cycle is exactly where the total and the split could come from + two different states of the account. + """ + from keel.strategy.paper import PaperTrader + + _seed_rule(repo, monkeypatch, _AlwaysEnterRule(PRODUCT), status="paper") + cfg = _paper_config(paper=PaperConfig(starting_equity_usd=Decimal("10000"))) + + trader = PaperTrader(repo) + trader.seed_cash(Decimal("10000"), now_ts=0) + repo.set_state("equity_state_mode", "paper") + trader.on_signal( + _paper_enter_signal( + product_id=PRODUCT, entry=Decimal("100"), stop=Decimal("50"), target=Decimal("200"), + ts=0, + ), + qty=Decimal("10"), + ) + + broker = _MarketDataOnlyBroker( + series={(PRODUCT, Granularity.ONE_DAY): [_candle(0, "100"), _candle(86_400, "120")]} + ) + repo.set_state("kill_switch", False) + repo.set_state("last_feed_ts", 86_400) + + run_once(broker, repo, cfg, now_ts=86_400) + + point = repo.get_equity_points()[0] + assert point.mode == "paper" + assert point.cash == trader.get_cash() + assert point.cash is not None and point.unrealized is not None + # The reconciliation invariant, on the paper side and through the loop: the recorded parts + # add back to the recorded total. The entry fill is whatever slippage and fees made it, so + # the cost basis is read back rather than assumed. + entry_fill = repo.get_orders(mode="paper")[0]["actual_fill"] + assert point.cash + Decimal("10") * entry_fill + point.unrealized == point.equity + + def test_the_live_split_reconciles_against_the_equity_it_was_read_with(repo: Repository) -> None: """`cash + cost basis + unrealized == equity`, on ONE read of the account. diff --git a/tests/web/test_client_assets.py b/tests/web/test_client_assets.py index 3b3cb11a..639ba10a 100644 --- a/tests/web/test_client_assets.py +++ b/tests/web/test_client_assets.py @@ -1367,6 +1367,31 @@ def test_the_series_canvas_does_not_answer_to_the_curves_selector() -> None: assert series_at < curve_at, "the equity series is stacked ABOVE the closed-trade curve" +def test_the_series_figure_does_not_intercept_the_curves_figure_lookups() -> None: + """The collision that `svg.series` alone does NOT fix, and that cost the journal-row + highlight once already. + + `main.js` reaches for the chart's WRAPPER with `contentNode.querySelector("figure.chart")` + in two places -- `highlightJournalRow` and the chart-action handler -- and that takes the + first match in document order. The series figure is appended ABOVE the curve, so if it were + a bare `figure.chart` those lookups would land on it; `highlightTrade` would then find no + `.highlight` group, return early, and hovering a journal row would silently do nothing. + Both halves are pinned here: the class the series wears, and the selector that excludes it. + """ + chart_code = _source("chart.js") + assert 'figure.className = "chart series"' in chart_code, ( + "the series figure must be distinguishable from the curve's figure by class" + ) + + main_code = _source("main.js") + assert 'querySelector("figure.chart")' not in main_code, ( + "a bare figure.chart lookup takes the series figure, which renders first" + ) + assert main_code.count('querySelector("figure.chart:not(.series)")') == 2, ( + "both figure lookups in main.js must exclude the series figure" + ) + + def test_the_series_draws_each_mode_as_its_own_polyline() -> None: """The one thing this chart must never do is join two accounts into one line. It is checked on the source rather than a rendered DOM because a browser cannot run here: the loop over diff --git a/tests/web/test_payload.py b/tests/web/test_payload.py index 0d07d01b..12cbd698 100644 --- a/tests/web/test_payload.py +++ b/tests/web/test_payload.py @@ -1512,6 +1512,28 @@ def test_a_single_mode_is_not_told_it_is_two_separate_accounts() -> None: assert "separate accounts" in _series_json()["reading"]["display"] +def test_a_truncated_series_says_how_much_it_is_not_showing() -> None: + """A bounded read reaches the reader as a bounded read. The sentence is the only place a + screen-reader user is told the chart is a window rather than the whole record -- and a + sighted reader gets the same words, so neither is told a span that is not there.""" + truncated = payload.equity_series_payload( + build_equity_series(_equity_readings(), total_recorded=900) + ) + + assert "most recent" in truncated["reading"]["display"] + assert truncated["total_recorded"]["value"] == "900" + assert truncated["is_truncated"]["value"] == "true" + + +def test_a_complete_series_makes_no_truncation_claim() -> None: + complete = payload.equity_series_payload( + build_equity_series(_equity_readings(), total_recorded=3) + ) + + assert "most recent" not in complete["reading"]["display"] + assert complete["is_truncated"]["value"] == "false" + + def test_an_empty_series_says_so_rather_than_describing_an_empty_chart() -> None: """Not "the account was flat", and not an empty string. A deployment that has not completed a cycle since the upgrade has no readings at all, and which of those two a reader is looking at