From ea678a8680dcd31a41aa94323422e287b7f8f3c5 Mon Sep 17 00:00:00 2001 From: Elmehdi Aitbrahim Date: Sun, 23 Aug 2026 21:09:50 -0400 Subject: [PATCH 1/2] feat(web): serialise the reports to JSON with money as strings (#533) The prerequisite the rest of the web-UI milestone inherits: one shared serialiser turning the frozen report dataclasses into browser-ready JSON, plus the tests that make its three rules checkable rather than merely intended. Endpoints are #534; this ships the contract and nothing that serves it. `keel/web/payload.py` is a FOURTH renderer over the same reports -- `keel/commands/*` renders them to terminal lines, `keel/web/render.py` to HTML, this to JSON -- and it re-gathers, re-derives and re-measures nothing. It consumes `gather_status` (status.py:336), `build_insights_report` (insights.py:251), `build_journal_report` (insights.py:370) and `build_activity_feed`. Rule 1, money crosses the wire as a string. `JSON.parse` yields IEEE-754 doubles and keel is `Decimal`-only precisely because binary floats corrupt money. The failure is silent -- nothing raises, nothing warns, and it only shows past the seventeenth digit or in the last cent of a large notional -- so it is pinned mechanically: a recursive walk over the real payload fails the build on ANY JSON number anywhere in it. That is stronger than "no monetary field is a number" on purpose; the weaker form needs a maintained list of which fields are money, and that list is what rots. The `5E+1` hazard shaped the file. `Decimal.normalize()` renders `Decimal("50")` as `Decimal("5E+1")`, a form that has reached the wire here before and broken real orders, and `str(Decimal(...))` does the same for any positive exponent. So exactly one place turns a Decimal into a string -- `_plain`, using `format(value, "f")`, the only rendering that cannot emit an exponent -- and `normalize()` appears nowhere. Trailing-zero trimming for display is string work on already-formatted text for the same reason. Rule 2, values arrive presentation-ready, with the formatting/computing line drawn where a figure has to already exist on the report. Two places that line was live: - An open position gets NO `notional` and NO `pnl`. `OpenPositionStatus` carries neither, so `qty * entry_price` would have put a figure on the wire the rails never saw -- and the spec's own illustrative payload shows one. Not emitted; the fix, if wanted, is `gather_status`. The spec's `pnl` example lands instead on `JournalEntry.pnl_net`, which `build_journal_report` genuinely computes. - A drawdown is NOT rescaled into a percentage. `drawdown_total_pct` is named for a percentage but holds a fraction (`_rail11_status` compares it against `max_total_dd_pct=Decimal("0.20")`). Multiplying by 100 to make the name true would be arithmetic in the serialiser, so it crosses unchanged through `ratio` and the display carries no `%`. `render.py`'s `pct()` appends one to this same raw fraction and therefore prints "0.05%" for a 5% drawdown; the contract does not inherit that, and the HTML fix is a separate surface. - `qty` carries no unit for the same reason at lower stakes: the base asset is not on the report and parsing it out of `product_id` would be a second place decoding product ids, which is what `guards.py::_asset` exists to be the only one of. Rule 3, semantic state is a field. `state` comes from a closed vocabulary and the `display` glyph carries the same distinction without colour -- today's `--good` and `--bad` differ by hue alone (1.01:1 luminance), which fails WCAG 1.4.1 in an application whose central signal is gain versus loss. `unknown` is kept distinct from `neutral`, so the classifier is deliberately not `render._tone_for_rail`: that helper falls through to "good" and would style a rail nobody has measured as if it passed. Rejected alternatives, recorded: - An integer companion field scaled to cents (the spec's own rejection, restated in the module because this is where it would be added). Precision is per-product -- `base_increment` varies by instrument, which is what #514 and #517 were about -- so a fixed 100x scale truncates anything finer than a cent and a 1e8 scale caps a USD notional near `Number.MAX_SAFE_INTEGER`. No `sort` field ships here; the guard test names itself as where its allowance would have to be written down. - Epoch seconds for a `moment`'s `value`. Reading them means `new Date(Number(value) * 1000)`, and that multiplication is client arithmetic -- small, but exactly the category Rule 2 exists to keep out. ISO-8601 with `Z` instead. - `state` only where interesting, as the spec's abbreviated example shows. Rejected: a client testing whether `state` is present is branching on payload shape, which is inference by another route, and #532's styling needs a word for every field. It is present on every field, always. This is the one deliberate departure from the spec's worked payload. - Bare JSON numbers for counts, which JSON would carry intact. Rejected: "it is only a count" is how the first double gets in, and a displayed count needs a `display` that a bare number cannot carry. The pin extends rather than forks. `keel/web/payload.py` is already inside `test_console_thinness.py`'s scanned set, so Rules 1-5 covered it on arrival; Rule 6 adds what is specific to a module whose output is a money contract rather than a screen -- no `normalize()`, no `float()` except on a timestamp (entry-scoped to `_gmt`), no `round()`, no `json.dumps(..., default=float)`, which is one keyword and looks like a helpful fix for the `TypeError` a `Decimal` raises. It carries a positive control, as Rule 5 does, and `SERIALISER_STEMS` is asserted inside the scanned set so a rename cannot silently drop the pin while Rules 1-5 keep passing. Both halves were watched red. Three mutations -- a leaked raw int, `normalize()` in `_plain`, and `money(qty * entry_price)` -- each failed the test that claims to guard it, including "the serialiser computes nothing", which named the invented notional by path. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01NyeggYtojNXCTHeD3JHxb6 --- keel/web/payload.py | 945 ++++++++++++++++++++++++ tests/commands/test_console_thinness.py | 175 ++++- tests/web/test_payload.py | 774 +++++++++++++++++++ 3 files changed, 1892 insertions(+), 2 deletions(-) create mode 100644 keel/web/payload.py create mode 100644 tests/web/test_payload.py diff --git a/keel/web/payload.py b/keel/web/payload.py new file mode 100644 index 0000000..49292f5 --- /dev/null +++ b/keel/web/payload.py @@ -0,0 +1,945 @@ +"""The serialisation contract (#533): frozen report dataclasses in, browser-ready JSON out. + +This module is a FOURTH renderer over the same reports, never a second place that computes them. +`keel/commands/*` builds the frozen report dataclasses and renders them to terminal lines +(`gather_status`/`render_human`, `build_insights_report`/`render_summary`); +`keel/web/render.py` renders the same dataclasses to HTML; this renders them to JSON. Nothing +here re-gathers, re-derives or re-measures anything -- which is what keeps +`tests/commands/test_console_thinness.py` able to pin this layer with the rules it already +applies to the console and the HTML renderer. + +Every endpoint in the web-UI milestone (#534 onwards) serialises through here, so the three +rules below are stated once, in one file a reviewer can read end to end. + +RULE 1 -- MONEY CROSSES THE WIRE AS A STRING, NEVER AS A JSON NUMBER. +`JSON.parse` in a browser yields IEEE-754 doubles. keel is `Decimal`-only precisely because +binary floats corrupt money, and JSON's number type would silently undo that at the boundary -- +silently being the operative word: nothing raises, nothing warns, and the error only shows up +past the seventeenth significant digit or in the last cent of a large notional. +`tests/web/test_payload.py::test_no_wire_value_is_ever_a_json_number` walks the parsed payload +and fails the build on ANY JSON number anywhere in it, which is deliberately stronger than "no +monetary field is a number": the weaker form needs a maintained list of which fields are money, +and that list is exactly the thing that rots. + +**The one measured hazard this file is shaped around.** `Decimal.normalize()` renders +`Decimal("50")` as `Decimal("5E+1")`, and `"5E+1"` has previously reached the wire in this +codebase and broken real orders. `str(Decimal(...))` does the same for any Decimal with a +positive exponent. So there is exactly ONE place a `Decimal` becomes a string here -- `_plain`, +which uses `format(value, "f")`, the only formatting path that cannot emit an exponent -- and +`normalize()` appears nowhere. `tests/commands/test_console_thinness.py`'s Rule 6 fails the build +if `normalize`, `float()` or `round()` is ever added to this module; `test_payload.py` checks the +absence at the level of the rendered payload as well, because an AST rule and a string check fail +for different reasons and neither subsumes the other. + +*An integer companion field scaled to cents was considered and rejected* (spec, § The data +contract). Precision here is PER-PRODUCT -- `base_increment` varies by instrument, which is what +#514 and #517 were about -- so a fixed 100x scale silently truncates anything finer than a cent, +and a 1e8 scale caps a USD notional near `Number.MAX_SAFE_INTEGER`. Should instant client-side +re-sorting ever be wanted, that field is named `sort`, is a plain JSON number, and is documented +as *ordering only -- never displayed, never summed*. It is not in this issue, and the guard test +above is where its allowance would have to be written down. + +RULE 2 -- VALUES ARRIVE PRESENTATION-READY; THE CLIENT PLACES THEM, NEVER DERIVES THEM. +Not `{"qty": "0.01", "price": "50000"}` for a client to multiply. Every figure a user will see is +formatted here, by the process that holds the trading rails, so the client needs no decimal +library at all and a reviewer can confirm the absence of client-side money arithmetic by reading +one file. + +The line between FORMATTING (allowed, and the whole job of this module) and COMPUTING +(forbidden) is: a figure on the wire must already exist on the report. `format`, grouping +separators, trailing-zero trimming, currency symbols and glyphs are formatting. Multiplication, +addition, rescaling and unit conversion are computing, and they belong upstream in the report +builder where the rails can see them. Two places where that line was live while writing this: + +* **An open position has no notional here.** `OpenPositionStatus` carries `qty` and + `entry_price` and nothing else numeric, so `qty * entry_price` was one multiplication away -- + and the spec's own illustrative payload shows a `notional` on a position. It is not emitted. + If a notional is wanted, it is added to `gather_status`. +* **A drawdown is NOT rescaled into a percentage.** `drawdown_total_pct` is named for a + percentage but holds a FRACTION: `_rail11_status` compares it against + `max_total_dd_pct=Decimal("0.20")`, so `0.05` means five percent. Multiplying by 100 to make + the name true would be arithmetic in the serialiser, so it crosses unchanged through `ratio` + and the display carries no `%`. (`render.py`'s `pct()` appends a `%` to this same raw + fraction and therefore prints "0.05%" for a 5% drawdown; the contract does not inherit that.) + +RULE 3 -- SEMANTIC STATE IS AN EXPLICIT FIELD, NEVER AN INFERENCE. +A client must never decide "this is bad" by inspecting a minus sign: that is arithmetic by +another name, and it relocates a trading judgement into a browser. Every field therefore carries +`state`, drawn from the closed vocabulary `STATES`, and the `display` string carries a glyph so +the same distinction survives without colour -- #532's palette work rests on this, and +`--good`/`--bad` in today's stylesheet are separated by hue alone (luminance ratio 1.01:1), which +fails WCAG 1.4.1 in an application whose central signal is gain versus loss. + +**`state` is present on every field, including ones with nothing interesting to say +(`"neutral"`) and ones with no value at all (`"unknown"`).** This is a deliberate departure from +the spec's abbreviated example, which shows `equity` with only `value` and `display`: a client +that must test whether `state` is present is branching on payload SHAPE, which is inference by +another route, and #532's styling table needs a word for every field it touches. Reversing it +would mean accepting that branch on the client, which is the thing this contract exists to +remove. + +WHAT IS *NOT* A FIELD. Identifiers and enum words -- `product_id`, `rule_name`, `mode`, +`as_of` -- cross as bare JSON strings. They carry no precision hazard, no rounding decision and +no judgement, and wrapping them would be ceremony rather than contract. Bare JSON *numbers* +never appear at all: counts, timestamps and open-ended log fields all cross as strings, because +"it is only a count" is how the first double gets in. +""" + +from __future__ import annotations + +import time +from collections.abc import Mapping +from decimal import Decimal, InvalidOperation +from typing import TYPE_CHECKING, Any, TypedDict + +# The CLI's own words for "how long ago" and "how much longer". Imported rather than +# re-implemented so the browser and the terminal can never disagree about the age of the same +# candle -- `keel/commands/insights.py` reaches into `status.py` for `_human_age` the same way, +# for the same reason. `keel.commands.*` is the SERVICE layer, which this layer is allowed to +# read; the compute trees (`keel.strategy`, `keel.execution.*`, `keel.analysis`) are not. +from keel.commands.status import _human_age, _human_remaining + +if TYPE_CHECKING: # pragma: no cover - typing only + from keel.commands.activity import ActivityCycle, ActivityEvent, ActivityFeed + from keel.commands.insights import ( + AccountSummary, + GateDistance, + InsightsReport, + JournalEntry, + JournalReport, + RuleTrackRecord, + ) + from keel.commands.status import ( + AutonomyStatus, + MarketSessionStatus, + OpenPositionStatus, + ProductFreshness, + RuleSummary, + StatusReport, + SubscriptionStatusRow, + WithdrawalAttestationStatus, + ) + + +class Field(TypedDict): + """One value, ready to place. `value` is machine input (exact, ungrouped, `Decimal`-parseable + for figures and ISO-8601 for instants); `display` is the human's, already formatted; `state` + is the judgement, from `STATES`.""" + + value: str + display: str + state: str + + +#: The closed `state` vocabulary. Closed on purpose: #532's palette and glyph table key off +#: exactly these words, and a free-text state would push the client back into guessing. +#: +#: `"unknown"` is NOT a synonym for `"neutral"`. `_rail11_status` returns `"unknown"` when a +#: drawdown scalar was never written, explicitly because reporting an unwritten value as a +#: confident "ok" would be a lie -- there may be a real breach the agent has not computed yet. +#: Collapsing that into a calm-looking neutral would put the lie back. +STATES: frozenset[str] = frozenset({"good", "warn", "bad", "neutral", "unknown"}) + +GOOD = "good" +WARN = "warn" +BAD = "bad" +NEUTRAL = "neutral" +UNKNOWN = "unknown" + +#: An em dash, for a value that was never recorded. `None` means "not recorded" and `0` means +#: "recorded as zero"; collapsing the first into the second is the shape of the always-passing +#: fee rail (#198) -- a missing number that reads as a real one. +ABSENT = "—" + +#: U+25B2 / U+25BC, and U+2212 MINUS SIGN rather than the hyphen-minus a keyboard produces. +#: The glyphs are the non-colour half of #532's gain/loss signal; the true minus is typography. +UP = "▲" +DOWN = "▼" +MINUS = "−" + + +def absent() -> Field: + """A value that was never recorded. + + A fresh dict each call, never a shared constant: every builder returns a `Field` a caller + may put straight into a payload, and one shared mutable default would let an edit in one + endpoint reach every other.""" + return {"value": "", "display": ABSENT, "state": UNKNOWN} + + +# -- the primitives ------------------------------------------------------------------------------ + + +def _plain(value: Decimal) -> str: + """THE one place a `Decimal` becomes a string. `format(value, "f")` is the only rendering + that cannot emit an exponent: `str()` and `normalize()` both turn `Decimal("50")` into + `"5E+1"`, which is the bug that has already reached the wire here. It preserves the full + coefficient, so the result re-parses to a `Decimal` equal to the input, for any input -- + including quantities finer than a cent and notionals past a double's 17 digits.""" + return format(value, "f") + + +def _decimalise(value: Decimal | float | None) -> Decimal | None: + """A finite `Decimal`, or `None` for anything that cannot honestly become one. + + A `float` arrives only from a non-monetary statistic (`RuleTrackRecord.win_rate`); money is + `Decimal` end to end and never takes this path. `repr()` gives the shortest string that + round-trips the float, so the figure on the wire is the figure the report held -- re-encoded, + not recomputed. NaN/Inf (from a corrupt log timestamp, or a `Decimal("NaN")` sentinel) become + `None` and render as absent: a payload is a worse place to raise than a cell is to be empty. + """ + if value is None: + return None + if isinstance(value, Decimal): + return value if value.is_finite() else None + try: + candidate = Decimal(repr(value)) + except (InvalidOperation, ValueError, TypeError): + return None + return candidate if candidate.is_finite() else None + + +def _magnitude(value: Decimal, places: int) -> tuple[bool, str]: + """`(is_negative, unsigned_grouped_text)`. + + The sign is taken off the FORMATTED string rather than off the number (`abs()`, `-value`), + because this layer does no arithmetic on money at all -- Rule 3 of the thinness pin, and the + property that makes "the browser never sees a computed figure" checkable rather than merely + likely. Note the sign here is the sign of the *rendered* text: a value that rounds to zero at + `places` still renders "0.00". The `state` a caller attaches is derived from the exact + `Decimal` instead, so a `-0.001` P&L is still labelled a loss. + """ + text = format(value, f",.{places}f") + return text.startswith("-"), text.lstrip("-") + + +def _trim(text: str) -> str: + """Drop a formatted decimal's trailing zeros -- `"0.01000000"` -> `"0.01"`, `"50.00000000"` -> + `"50"`. String work on an already-formatted number, deliberately, because the obvious + alternative is `Decimal.normalize()` and that is the `5E+1` bug.""" + if "." not in text: + return text + return text.rstrip("0").rstrip(".") + + +def _sign_state(value: Decimal) -> str: + """Rule 3's judgement, made HERE. A comparison, not arithmetic: the browser is told the + answer so it never has to look at the sign itself.""" + if value < 0: + return BAD + if value > 0: + return GOOD + return NEUTRAL + + +def stringify(value: Any) -> str: + """Any open-ended value, as a JSON string. + + Used for the two places keel's own reports carry values of unconstrained type: + `ActivityEvent.fields` (every `log_event` call site invents its own kwargs, and the parser + keeps them whole rather than projecting onto a fixed schema) and `JournalReport.filters` (the + query echoed back). Rule 1 applies to those exactly as it does to a named money field -- an + echoed `limit` is a JSON number like any other, and "it is only a count" is how the first + double gets in. + + Booleans render as `"true"`/`"false"` rather than Python's `"True"`/`"False"`: the reader is + JavaScript. + """ + if value is None: + return "" + if isinstance(value, bool): + return "true" if value else "false" + if isinstance(value, Decimal): + return _plain(value) if value.is_finite() else str(value) + if isinstance(value, float): + decimalised = _decimalise(value) + return _plain(decimalised) if decimalised is not None else str(value) + return str(value) + + +def _gmt(ts: float | int | None, fmt: str) -> str: + """`ts` in UTC under `fmt`, or `""` for anything unrenderable. + + UTC always, and labelled as such wherever it is shown: keel's day boundaries are UTC + everywhere -- gates, scoping, the activity feed -- and rendering in local time is what made + the activity feed show a stale date (#381), a "today" view that could be permanently empty. + + Total by construction. Log timestamps come from outside this process, and a NaN or an + out-of-range float must cost one cell, not the whole response. `float()` here is the one + conversion this module makes and it is on a TIMESTAMP, which has no cent to lose -- + `time.gmtime` accepts nothing else. `test_console_thinness.py`'s Rule 6b allows it by name + and by argument, so `float(price)` at the same site would still fail the build. + """ + if ts is None: + return "" + try: + return time.strftime(fmt, time.gmtime(float(ts))) + except (OverflowError, OSError, ValueError): + return "" + + +def iso(ts: float | int | None) -> str: + """An instant as ISO-8601 UTC with an explicit `Z`, or `""`. + + `Z` rather than a bare naive string so `new Date(value)` in a browser cannot read it as local. + This is what the top-level `as_of` on every payload carries. + """ + return _gmt(ts, "%Y-%m-%dT%H:%M:%SZ") + + +# -- the field builders -------------------------------------------------------------------------- + + +def money( + value: Decimal | None, + *, + places: int = 2, + signed: bool = False, + state: str | None = None, + symbol: str = "$", +) -> Field: + """A monetary figure. `value` is the exact source `Decimal`; `display` is the human's. + + `signed=True` marks a GAIN-OR-LOSS figure (a net P&L, an R-multiple): it gets the glyph, an + explicit sign, and a `state` derived from the exact source value -- which is Rule 3's whole + point, the judgement made in Python so the client never reads a minus sign. `signed=False` is + a magnitude (a balance, a high-water mark, a fee): no glyph, `neutral` unless the caller knows + better and passes `state`. + + Zero is deliberately glyph-less even when `signed=True`: `▲ +$0.00` claims a direction that + the number does not have. + """ + figure = _decimalise(value) + if figure is None: + return absent() + negative, magnitude = _magnitude(figure, places) + if signed and figure != 0: + glyph = DOWN if negative else UP + sign = MINUS if negative else "+" + display = f"{glyph} {sign}{symbol}{magnitude}" + elif negative: + display = f"{MINUS}{symbol}{magnitude}" + else: + display = f"{symbol}{magnitude}" + resolved = state if state is not None else (_sign_state(figure) if signed else NEUTRAL) + return {"value": _plain(figure), "display": display, "state": resolved} + + +def quantity(value: Decimal | None, *, unit: str = "", places: int = 8) -> Field: + """An instrument quantity. `value` keeps the ledger's own precision -- `0.01000000`, because + `base_increment` is per-product and truncating it is what #514 and #517 were about -- while + `display` trims the padding a human does not read: `0.01 BTC`. + + `places=8` is Coinbase's finest base increment, not a universal truth; it is the ceiling on + what is shown, never a rescaling of what is sent. + """ + figure = _decimalise(value) + if figure is None: + return absent() + negative, magnitude = _magnitude(figure, places) + trimmed = _trim(magnitude) + display = f"{MINUS}{trimmed}" if negative else trimmed + if unit: + display = f"{display} {unit}" + return {"value": _plain(figure), "display": display, "state": NEUTRAL} + + +def percent( + value: Decimal | float | None, + *, + places: int = 2, + signed: bool = False, + state: str | None = None, +) -> Field: + """A figure ALREADY IN PERCENT UNITS (`41.5` meaning 41.5%), suffixed with `%`. + + Never use this for a fraction: `ratio` exists for those, and the conversion between them is + arithmetic this layer does not do. See `ratio`'s note on `drawdown_total_pct`. + """ + figure = _decimalise(value) + if figure is None: + return absent() + negative, magnitude = _magnitude(figure, places) + if signed and figure != 0: + glyph = DOWN if negative else UP + sign = MINUS if negative else "+" + display = f"{glyph} {sign}{magnitude}%" + elif negative: + display = f"{MINUS}{magnitude}%" + else: + display = f"{magnitude}%" + resolved = state if state is not None else (_sign_state(figure) if signed else NEUTRAL) + return {"value": _plain(figure), "display": display, "state": resolved} + + +def ratio( + value: Decimal | float | None, + *, + places: int = 2, + signed: bool = False, + state: str | None = None, +) -> Field: + """A bare dimensionless figure -- a drawdown FRACTION, an R-multiple, a profit factor. + + Kept separate from `percent` because of a real trap: `StatusReport.drawdown_total_pct` is + named for a percentage but holds a fraction (`_rail11_status` compares it against + `max_total_dd_pct=Decimal("0.20")`). Rescaling it by 100 here to make the name true would be + the serialiser computing a figure the report never held -- the exact thing Rule 2 forbids -- + so it crosses unchanged and the display carries no `%`. Making the units honest is a change + to `gather_status`, not to this file. + """ + figure = _decimalise(value) + if figure is None: + return absent() + negative, magnitude = _magnitude(figure, places) + if signed and figure != 0: + glyph = DOWN if negative else UP + sign = MINUS if negative else "+" + display = f"{glyph} {sign}{magnitude}" + elif negative: + display = f"{MINUS}{magnitude}" + else: + display = magnitude + resolved = state if state is not None else (_sign_state(figure) if signed else NEUTRAL) + return {"value": _plain(figure), "display": display, "state": resolved} + + +def count(value: int | None, *, state: str = NEUTRAL) -> Field: + """A whole number of things -- trades, cycles, errors, lines read. + + A count would survive JSON's number type intact, and it still crosses as a string. Two + reasons: a payload with "only a few" numbers in it needs a per-field rule about which ones, + and that rule is what rots; and a count is displayed, so under Rule 2 it needs a `display` + (grouped for reading) that a bare number cannot carry. + """ + if value is None: + return absent() + return {"value": str(value), "display": f"{value:,}", "state": state} + + +def flag( + value: bool | None, + *, + on: str, + off: str, + on_state: str = NEUTRAL, + off_state: str = NEUTRAL, +) -> Field: + """A boolean an operator has to act on -- the kill switch, autonomy, a bracket. + + It arrives with the WORD to show and the state to style it by, rather than as a bare `true` + for a client to translate. Translating is a judgement ("engaged" is bad, "autonomous" is a + warning), and Rule 3 keeps judgements in Python. + """ + if value is None: + return absent() + return { + "value": "true" if value else "false", + "display": on if value else off, + "state": on_state if value else off_state, + } + + +def label(value: str | None, *, display: str | None = None, state: str = NEUTRAL) -> Field: + """An enum word that carries a judgement -- a rail status, an attestation state, an outcome. + + Distinct from a bare JSON string (a `product_id`, a `rule_name`) precisely because of the + judgement: `state` is why this is a field. + """ + if value is None: + return absent() + return {"value": value, "display": display if display is not None else value, "state": state} + + +def moment(ts: float | int | None, *, state: str = NEUTRAL) -> Field: + """An instant. `value` is ISO-8601 UTC so a client can hand it straight to `new Date(...)` + with no arithmetic; `display` is the same instant already written out, so no client ever + formats a date to put one in a table. + + Epoch seconds were the obvious alternative for `value` and were rejected: reading them means + `new Date(Number(value) * 1000)`, and that multiplication is client arithmetic -- small, but + exactly the category Rule 2 exists to keep out. + + A broken timestamp (NaN, an out-of-range float, a log line written by something else) + degrades to absent, the same choice `render.utc` already makes: a dash in one cell beats a + 500 on the page. + """ + text = _gmt(ts, "%Y-%m-%dT%H:%M:%SZ") + human = _gmt(ts, "%Y-%m-%d %H:%M:%S UTC") + if not text or not human: + return absent() + return {"value": text, "display": human, "state": state} + + +def duration(seconds: int | None, *, elapsed: bool = True, state: str = NEUTRAL) -> Field: + """A span of seconds, in the CLI's own words. + + `_human_age` ("5m ago") and `_human_remaining` ("6d") come from `keel/commands/status.py` -- + the functions `keel status` itself prints -- rather than being re-derived here, so the browser + and the terminal can never disagree about how old the same candle is or how long an + attestation has left. + """ + if seconds is None: + return absent() + human = _human_age(seconds) if elapsed else _human_remaining(seconds) + return {"value": str(seconds), "display": human, "state": state} + + +# -- state classifiers --------------------------------------------------------------------------- + + +def _rail_state(status: str | None) -> str: + """Rail 11's status word, judged. + + Deliberately NOT `render._tone_for_rail`: that helper matches on "breach"/"halt"/"warn" and + falls THROUGH to `"good"`, so `"unknown"` -- the word `_rail11_status` returns when a drawdown + scalar was never written -- is styled as if the rail had passed. `_rail11_status`'s own + docstring says that would be a lie, so the contract keeps `unknown` as its own state. (The + HTML renderer's version is a separate surface and a separate fix.) + """ + lowered = (status or "").lower() + if not lowered or "unknown" in lowered: + return UNKNOWN + if "halt" in lowered or "breach" in lowered or "trip" in lowered: + return BAD + if "warn" in lowered or "near" in lowered: + return WARN + return GOOD + + +#: Rail 17's four states, judged. `expired` and `suspended` are the two that halt live entries; +#: `unattested` has never been answered at all, which is a gap rather than a breach. +_ATTESTATION_STATES: Mapping[str, str] = { + "attested": GOOD, + "suspended": BAD, + "expired": BAD, + "unattested": WARN, + "unknown": UNKNOWN, +} + +#: The venue clock, judged. `clock_unavailable` is a WARNING and not an error: the agent +#: fails closed on it (cycles skip), so the deployment is safe but not trading, and an operator +#: needs to know which of those two it is looking at. +_SESSION_STATES: Mapping[str, str] = { + "open": GOOD, + "closed": NEUTRAL, + "clock_unavailable": WARN, +} + +#: `ActivityFeed.status`, judged. `missing` is the commonest state on a fresh install and is NOT +#: an error -- it also happens when keel is run from a directory that is not the deployment +#: folder -- so it is a warning, not a failure. +_FEED_STATES: Mapping[str, str] = { + "ok": GOOD, + "missing": WARN, + "empty": NEUTRAL, + "unparseable": WARN, + "oversized": WARN, + "unreadable": BAD, +} + + +# -- status --------------------------------------------------------------------------------------- + + +def _autonomy_payload(autonomy: AutonomyStatus) -> dict[str, Any]: + return { + "live": flag( + autonomy.live, + on="ON — orders placed without asking", + off="off", + on_state=WARN, + off_state=NEUTRAL, + ), + "configured": flag(autonomy.autonomous, on="on", off="off"), + "lapses_at": moment(autonomy.autonomous_until), + "updated_at": moment(autonomy.updated_ts), + # An unreadable profile row is reported as autonomy OFF (the safe reading) by + # `_autonomy_status`; the browser is told the reading was degraded rather than being shown + # a confident "off" it cannot distinguish from a measured one. + "profile_readable": flag( + autonomy.profile_readable, + on="yes", + off="unreadable — autonomy reported OFF", + on_state=NEUTRAL, + off_state=WARN, + ), + } + + +def _attestation_payload(attestation: WithdrawalAttestationStatus) -> dict[str, Any]: + return { + "state": label( + attestation.state, + state=_ATTESTATION_STATES.get(attestation.state or "", UNKNOWN), + ), + "enabled": flag(attestation.enabled, on="enabled", off="disabled"), + "attested_at": moment(attestation.attested_at), + "expires_in": duration(attestation.expires_in_sec, elapsed=False), + "expired_for": duration(attestation.expired_for_sec), + } + + +def _session_payload(session: MarketSessionStatus) -> dict[str, Any]: + return { + "state": label( + session.state, state=_SESSION_STATES.get(session.state or "", UNKNOWN) + ), + "recorded_at": moment(session.recorded_ts), + # `defused` is whether a recorded CLOSED still vouches for the quiet -- the reason a + # weekend's stale data must not alert. It is a fact the report already carries, and the + # freshness rows below key off the same word, so the two can never disagree. + "defused": flag(session.defused, on="staleness explained", off="staleness alerts"), + } + + +def _position_payload(position: OpenPositionStatus) -> dict[str, Any]: + """One open position. + + NO `notional` and NO `pnl`. `OpenPositionStatus` carries neither, so emitting one would mean + this layer multiplied `qty` by `entry_price` and put a figure on the wire that the trading + rails never saw. See the module docstring; the fix, if it is wanted, is upstream. + + NO unit on `qty` either, for a quieter version of the same reason. The spec's example shows + `"0.01 BTC"`, but the base asset is not on the report -- it would have to be parsed out of + `product_id`, and the audited place that decodes a product id into an asset is + `keel/execution/guards.py::_asset`, which this layer may not reach (Rules 1 and 2) precisely + so that a malformed id is decoded in ONE place with one set of consequences. `quantity` takes + a `unit` for the day `OpenPositionStatus` carries a `base_asset`; until then the exact + quantity sits beside the `product_id` and nothing is guessed. + """ + return { + "id": str(position.id), + "product_id": position.product_id, + "rule_name": position.rule_name, + "qty": quantity(position.qty), + "entry_price": money(position.entry_price), + "opened_at": moment(position.opened_at), + "bracket": flag( + position.has_bracket, + on="bracketed", + off="NO bracket", + on_state=GOOD, + off_state=WARN, + ), + } + + +def _rule_payload(rule: RuleSummary) -> dict[str, Any]: + return { + "id": str(rule.id), + "kind": rule.kind, + "status": label(rule.status), + "product_id": rule.product_id or "", + # Rule params are operator-supplied and open-ended (any rule kind may invent its own), so + # they cross as strings for the same reason `ActivityEvent.fields` does. + "params": {str(key): stringify(value) for key, value in sorted(rule.params.items())}, + } + + +def _freshness_payload(row: ProductFreshness, session_defused: bool) -> dict[str, Any]: + """One product's data freshness. + + The staleness judgement is NOT made from the age here -- there is no threshold in this file. + It reads `MarketSessionStatus.defused`, which the report already computed: closed AND inside + its trust window is the state under which staleness does not alert, and the TUI's freshness + styling keys off the same field so the cells and the session line can never disagree about + the same weekend. + """ + state = NEUTRAL if (row.last_ts is not None or session_defused) else WARN + return { + "product_id": row.product_id, + "granularity": row.granularity or "", + "last_candle_at": moment(row.last_ts), + "age": duration(row.age_sec, state=state), + } + + +def _subscription_payload(row: SubscriptionStatusRow) -> dict[str, Any]: + return { + "venue": row.venue, + "tier_name": row.tier_name, + "pacing": row.pacing, + "stored_status": label(row.stored_status), + "effective_status": label( + row.effective_status, + state=GOOD if row.effective_status == "active" else WARN, + ), + # `None` means UNLIMITED here, not "not recorded" -- the one place in this file where an + # absent Decimal is a fact rather than a gap, so it is spelled out instead of dashed. + "effective_cap": ( + label("unlimited", state=NEUTRAL) + if row.effective_cap is None + else money(row.effective_cap) + ), + } + + +def status_payload(report: StatusReport) -> dict[str, Any]: + """`gather_status`'s `StatusReport`, as JSON. Serialises; never re-gathers.""" + session_defused = bool(report.market_session.defused) + return { + "as_of": iso(report.now_ts), + "generated_at": moment(report.now_ts), + "mode": report.mode, + "kill_switch": flag( + report.kill_switch_engaged, + on="ENGAGED", + off="clear", + on_state=BAD, + off_state=GOOD, + ), + "autonomy": _autonomy_payload(report.autonomy), + "market_session": _session_payload(report.market_session), + "equity": { + "state_mode": label(report.equity_state_mode), + "high_water_mark": money(report.high_water_mark), + "paper_cash": money(report.paper_cash_usdc), + }, + "drawdown": { + "total": ratio(report.drawdown_total_pct), + "weekly": ratio(report.drawdown_weekly_pct), + "max_total": ratio(report.max_total_dd_pct), + "max_weekly": ratio(report.max_weekly_dd_pct), + "rail11": label(report.rail11_status, state=_rail_state(report.rail11_status)), + }, + "withdrawal_attestation": _attestation_payload(report.withdrawal_attestation), + "open_positions": [_position_payload(p) for p in report.open_positions], + # A LIST of pairs rather than an object, because the order is a presentation decision and + # Rule 2 says presentation decisions are made here. `sorted` matches `render_human`'s own + # ordering, so the two front-ends list the statuses the same way. + "rule_counts": [ + {"status": status, "count": count(number)} + for status, number in sorted(report.rule_counts.items()) + ], + "live_rules": [_rule_payload(r) for r in report.live_rules], + "data_freshness": [_freshness_payload(f, session_defused) for f in report.data_freshness], + "subscriptions": [_subscription_payload(s) for s in report.subscriptions], + } + + +# -- insights -------------------------------------------------------------------------------------- + + +def _account_payload(account: AccountSummary) -> dict[str, Any]: + return { + "mode": account.mode, + "state_mode": label(account.equity_state_mode), + "high_water_mark": money(account.high_water_mark), + "paper_cash": money(account.paper_cash_usdc), + "drawdown_total": ratio(account.drawdown_total_pct), + "drawdown_weekly": ratio(account.drawdown_weekly_pct), + "max_total_dd": ratio(account.max_total_dd_pct), + "max_weekly_dd": ratio(account.max_weekly_dd_pct), + "rail11": label(account.rail11_status, state=_rail_state(account.rail11_status)), + } + + +def _gate_payload(gate: GateDistance) -> dict[str, Any]: + """The promotion gate's distance. `passing` is the engine's verdict, copied; the floors are + the config's own values, copied. Nothing here re-runs `check_floors`.""" + return { + "rule_name": gate.rule_name, + "promotion_class": gate.promotion_class, + "n_trades": count(gate.n_trades), + "min_trades": count(gate.min_trades), + "trades_remaining": count( + gate.trades_remaining, state=GOOD if gate.trades_remaining == 0 else WARN + ), + "win_rate": percent(gate.win_rate, places=1), + "min_win_rate": percent(gate.min_win_rate, places=1), + "realized_rr": ratio(gate.realized_rr), + "min_rr": ratio(gate.min_rr), + "expectancy": money(gate.expectancy, places=4, signed=True), + "min_expectancy": money(gate.min_expectancy, places=4), + "passing": flag(gate.passing, on="passing", off="blocked", on_state=GOOD, off_state=WARN), + "blocking_reasons": list(gate.blocking_reasons), + } + + +def _track_record_payload(record: RuleTrackRecord) -> dict[str, Any]: + return { + "rule_name": record.rule_name, + "status": label(record.status), + "promotion_class": record.promotion_class, + "n_trades": count(record.n_trades), + "win_rate": percent(record.win_rate, places=1), + "avg_win": money(record.avg_win), + "avg_loss": money(record.avg_loss), + "realized_rr": ratio(record.realized_rr), + "expectancy": money(record.expectancy, places=4, signed=True), + "profit_factor": ratio(record.profit_factor), + "max_drawdown": money(record.max_drawdown), + # `significant` is `n_trades >= 30`, computed by the report. Below that floor a win rate + # is not distinguishable from random entry, so the payload SAYS so rather than leaving the + # number to speak for itself -- the same choice `render_insights` makes in HTML. + "significant": flag( + record.significant, + on="n≥30", + off="below the n=30 floor", + on_state=NEUTRAL, + off_state=WARN, + ), + "gate": _gate_payload(record.gate) if record.gate is not None else None, + } + + +def insights_payload(report: InsightsReport) -> dict[str, Any]: + """`build_insights_report`'s `InsightsReport`, as JSON.""" + 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), + "rules": [_track_record_payload(r) for r in report.rules], + } + + +# -- journal --------------------------------------------------------------------------------------- + + +def _journal_entry_payload(entry: JournalEntry) -> dict[str, Any]: + """One closed (or, with `--include-open`, one open) trade. + + This is where the spec's worked `pnl` example actually lands: `pnl_net` is a figure + `build_journal_report` computed off the fee-honest `trade_outcomes` ledger, so it can be + serialised without this layer deriving anything. `None` stays absent rather than becoming + `0.00` -- a trade with no recorded net is not a break-even trade, which is the same + distinction `render_insights` protects in HTML. + """ + return { + "closed_at": moment(entry.closed_at), + "opened_at": moment(entry.opened_at), + "rule_name": entry.rule_name or "", + "product_id": entry.product_id, + "qty": quantity(entry.qty), + "entry_fill": money(entry.entry_fill), + "exit_fill": money(entry.exit_fill), + "pnl": money(entry.pnl_net, signed=True), + "fees": money(entry.fees, places=4), + "r_multiple": ratio(entry.r_multiple, signed=True), + "is_dca": flag(entry.is_dca, on="DCA", off="rule"), + "outcome": label(entry.outcome, state=_OUTCOME_STATES.get(entry.outcome, NEUTRAL)), + } + + +#: `JournalEntry.outcome`'s vocabulary, judged. `dca` is neutral by design: a DCA row has no stop +#: to measure against, so `build_journal_report` labels it `"dca"` regardless of the P&L sign and +#: calling it a win or a loss here would invent a verdict the ledger declined to make. +_OUTCOME_STATES: Mapping[str, str] = {"win": GOOD, "loss": BAD, "dca": NEUTRAL, "open": NEUTRAL} + + +def journal_payload(report: JournalReport) -> dict[str, Any]: + """`build_journal_report`'s `JournalReport`, as JSON. + + `total_count` is the full filtered count BEFORE `--limit` truncated `entries`; both cross, so + a client showing "50 of 812" needs no subtraction to know it is looking at a page. + """ + return { + "as_of": iso(report.now_ts), + "generated_at": moment(report.now_ts), + "mode": report.mode, + "total_count": count(report.total_count), + "shown_count": count(len(report.entries)), + # The query echoed back. Open-ended by shape (`limit` and `since_ts` are ints), so it + # crosses as strings -- see `stringify`. + "filters": {str(key): stringify(value) for key, value in sorted(report.filters.items())}, + "entries": [_journal_entry_payload(e) for e in report.entries], + } + + +# -- activity ------------------------------------------------------------------------------------ + + +def _event_payload(event: ActivityEvent) -> dict[str, Any]: + return { + "at": moment(event.ts), + "level": event.level, + "event": event.event, + "cycle_id": event.cycle_id or "", + # Kept whole rather than projected onto a fixed schema: the event vocabulary grows with + # every `log_event` call site, and an overlay that silently dropped a field it had not + # been taught about would be worse than one that renders it as key=value. + "fields": {str(key): stringify(value) for key, value in event.fields.items()}, + } + + +def _cycle_payload(cycle: ActivityCycle) -> dict[str, Any]: + """One engine cycle. + + `quiet` is `neutral`, never `warn`. A cycle in which the agent looked at the market and + nothing happened is a POSITIVE observation -- a long run of them is how this feed answers "is + it alive" -- so it is rendered muted but never omitted and never flagged. + """ + return { + "key": cycle.key, + "cycle_id": cycle.cycle_id or "", + "started_at": moment(cycle.started_ts), + "ended_at": moment(cycle.ended_ts), + "mode": cycle.mode or "", + "products": list(cycle.products), + "rules": list(cycle.rules), + "signals": count(cycle.signals), + "blocked": count(cycle.blocked), + "entered": count(cycle.entered), + "exited": count(cycle.exited), + "errors": count(cycle.errors, state=BAD if cycle.errors else NEUTRAL), + "highlights": list(cycle.highlights), + "quiet": flag(cycle.is_quiet, on="quiet", off="active"), + "uncorrelated": flag(cycle.is_uncorrelated, on="uncorrelated", off="correlated"), + "events": [_event_payload(e) for e in cycle.events], + "events_dropped": count( + cycle.events_dropped, state=WARN if cycle.events_dropped else NEUTRAL + ), + } + + +def activity_payload(feed: ActivityFeed) -> dict[str, Any]: + """`build_activity_feed`'s `ActivityFeed`, as JSON. + + Every non-`ok` status crosses as a judged state rather than being suppressed: `missing` is the + commonest state on a fresh install and is not an error, and hiding it would leave a user + staring at a blank panel with nothing to act on. + + `scope_fully_covered` is the one field a client must not ignore. False means the bounded tail + read could not prove it reached back past the scope boundary, so an empty feed cannot be read + as "the scope was quiet" -- which is why it arrives already judged as a warning. + """ + return { + "as_of": iso(feed.now_ts), + "generated_at": moment(feed.now_ts), + "status": label(feed.status, state=_FEED_STATES.get(feed.status, UNKNOWN)), + "source": feed.source, + "detail": feed.detail or "", + "scope": feed.scope, + "scope_start_at": moment(feed.scope_start_ts), + "scope_fully_covered": flag( + feed.scope_fully_covered, + on="complete", + off="window may not cover the whole scope", + on_state=NEUTRAL, + off_state=WARN, + ), + "lines_read": count(feed.lines_read), + # Lines read but unusable -- a crash's half-written JSON, a record with no timestamp. + # Surfaced rather than swallowed: silently discarding input is how a feed comes to + # under-report reality while looking healthy. + "lines_skipped": count(feed.lines_skipped, state=WARN if feed.lines_skipped else NEUTRAL), + "window_truncated": flag( + feed.window_truncated, + on="window truncated", + off="whole window read", + on_state=WARN, + off_state=NEUTRAL, + ), + "cycles_dropped": count( + feed.cycles_dropped, state=WARN if feed.cycles_dropped else NEUTRAL + ), + "cycles_out_of_scope": count(feed.cycles_out_of_scope), + "last_cycle_before_scope": ( + _cycle_payload(feed.last_cycle_before_scope) + if feed.last_cycle_before_scope is not None + else None + ), + "cycles": [_cycle_payload(c) for c in feed.cycles], + } diff --git a/tests/commands/test_console_thinness.py b/tests/commands/test_console_thinness.py index 2133aaf..e837c0b 100644 --- a/tests/commands/test_console_thinness.py +++ b/tests/commands/test_console_thinness.py @@ -5,7 +5,8 @@ (`keel/commands/console.py`), the live loop (`keel/commands/tui.py`), every console sub-menu module (`keel/commands/*console*.py`), and -- since #435 -- every module of the local web UI (`keel/web/*.py`). Those files render and dispatch; all behavior must come from the services -the CLI calls. The pin is an AST scan enforcing five rules: +the CLI calls. The pin is an AST scan enforcing six rules -- five that hold across the whole +layer, and one (#533) scoped to the JSON serialiser: * **Rule 1 -- no compute-module imports.** Nothing may be imported from the compute trees -- `keel.strategy.*` (sizing/backtest/promotion math), `keel.execution.guards` / @@ -42,6 +43,18 @@ (`keel.commands.update`), which this pin does not scan -- so the rule needs no allowance at all, and a console module that inlines any of that orchestration fails here rather than shipping a second, unpinned place that replaces the running binary. +* **Rule 6 -- the serialisation contract** (issue #533), scoped to the module that turns the + frozen reports into the browser's JSON (`SERIALISER_STEMS`). That module is the one place in + this layer whose output is a machine-readable MONEY contract rather than a screen, so it + carries one extra pin over the four ways the `Decimal`-only guarantee dies at that boundary: + `Decimal.normalize()` (which renders `Decimal("50")` as `Decimal("5E+1")` -- a form that has + reached the wire in this codebase before and broken real orders), `float()` on anything but a + timestamp, `round()`, and `json.dumps(..., default=float)`, which is one keyword and looks like + a helpful fix for the `TypeError` a `Decimal` raises. Rules 1-5 already cover the serialiser as + a member of `keel/web/`; this covers what is specific to it. The runtime half of the same + contract -- a recursive walk over the real payload asserting that no JSON number appears + anywhere in it -- lives in `tests/web/test_payload.py`. An AST rule and a walk over the output + fail for different reasons and neither subsumes the other, which is why both exist. The allowlists are deliberately entry-scoped (module + enclosing function + callee), so an allowance cannot leak to a new call site: the same callee at a different place, or a @@ -63,7 +76,7 @@ #: The PRESENTATION layer: the console shell, the live loop, every sub-menu module -- and, since #: #435, every module of the local web UI (`keel/web/`). #: -#: The web UI is scanned by the same five rules rather than a parallel pin of its own, because it +#: The web UI is scanned by the same rules rather than a parallel pin of its own, because it #: is the same kind of thing: a second front-end over `keel/commands/*`. A separate pin would have #: drifted -- two files stating the same architecture, diverging one allowance at a time -- and #: the failure it is guarding against is identical in both. `keel/commands/serve.py` is NOT @@ -90,6 +103,29 @@ def _console_module_paths() -> list[str]: RULE5_IMPORT_ALLOWLIST: frozenset[tuple[str, str]] = frozenset({("server", "urllib.parse")}) +#: The SERIALISER: the module turning the frozen reports into the browser's JSON (#533). It is +#: already inside the scanned set (it lives under `keel/web/`), so Rules 1-5 apply to it as they +#: do to every other module of this layer. Rule 6 below is the extra, narrower pin it needs, +#: because it is the one module in the layer whose OUTPUT is a machine-readable money contract +#: rather than a screen. +SERIALISER_STEMS: frozenset[str] = frozenset({"payload"}) + + +#: Rule 6b's entry-scoped allowance, in the same (module, enclosing function, argument) shape as +#: every other allowance here. +#: +#: `float()` is banned in the serialiser because it is the one call that turns a `Decimal` into +#: the IEEE-754 double the whole contract exists to keep off the wire. It is allowed on a +#: TIMESTAMP, which is not money: `time.gmtime` takes a float and nothing else, and a UTC instant +#: has no cent to lose. Scoped to the argument NAME, so `float(price)` at the same call site +#: still fails. +RULE6_FLOAT_ALLOWLIST: frozenset[tuple[str, str, str]] = frozenset( + { + ("payload", "_gmt", "ts"), + } +) + + #: The compute trees -- where sizing/backtest/gate/screening/reporting math lives. The #: console layer reaches these ONLY through the `keel.commands.*` service layer (or the #: audited allowances below). @@ -430,6 +466,86 @@ def _inside_service_lambda(node: ast.AST) -> bool: "console layer never shells out, opens a socket or replaces " "its own process; that is keel.commands.update's job", ) + + # Rule 6 (#533): the serialisation contract, in the ONE module whose output is a + # machine-readable money contract rather than a screen. + if stem in SERIALISER_STEMS: + for key, messages in _rule6_findings(stem, tree, aliases, owners).items(): + for message in messages: + found.add(key, message) + return found + + +def _argument_identifier(node: ast.expr) -> str: + """The last identifier of a call argument -- `ts` for `ts`, `x.ts` and `self.x.ts`. + + Used only to scope Rule 6b's allowance to the ARGUMENT, so `float(ts)` can be allowed at a + site where `float(price)` still fails.""" + if isinstance(node, ast.Name): + return node.id + if isinstance(node, ast.Attribute): + return node.attr + return "" + + +def _rule6_findings( + stem: str, tree: ast.Module, aliases: dict[str, str], owners: dict[ast.AST, str] +) -> dict[str, list[str]]: + """The four ways the `Decimal`-only guarantee dies at the JSON boundary, as an AST scan. + + Written as its own function rather than inline in the fixture so the positive control below + can run it over a synthetic module and prove it is capable of finding anything at all.""" + found = _Findings() + for node in ast.walk(tree): + if not isinstance(node, ast.Call): + continue + owner = owners.get(node, "") + name = _dotted_call_name(node.func, aliases) + short = name.rsplit(".", 1)[-1] if name else None + + # 6a -- normalize(). THE hazard: `Decimal("50").normalize()` is `Decimal("5E+1")`, and + # "5E+1" on the wire is fifty read as five, or as nothing. Matched on the ATTRIBUTE, not + # on a resolved dotted name, because the receiver is a value and never resolves. + if isinstance(node.func, ast.Attribute) and node.func.attr == "normalize": + found.add( + "rule6_serialisation", + f"{stem}:{owner}: calls .normalize() (line {node.lineno}) -- it renders " + 'Decimal("50") as Decimal("5E+1"); format(value, "f") is the only rendering ' + "that cannot emit an exponent", + ) + + # 6b -- float(), the one call that turns a Decimal into the IEEE-754 double this whole + # contract exists to keep off the wire. Allowed on a timestamp, entry-scoped. + if short == "float" and name == "float": + argument = _argument_identifier(node.args[0]) if node.args else "" + if (stem, owner, argument) not in RULE6_FLOAT_ALLOWLIST: + found.add( + "rule6_serialisation", + f"{stem}:{owner}: calls float({argument}) (line {node.lineno}) -- money " + "crosses the wire as a string; only a timestamp may become a float here", + ) + + # 6c -- round(), which both computes and (on a float) reintroduces binary rounding. + # Rounding for DISPLAY is done with a format spec, which leaves the wire value exact. + if name == "round": + found.add( + "rule6_serialisation", + f"{stem}:{owner}: calls round() (line {node.lineno}) -- display rounding " + "belongs in a format spec, which leaves the wire value exact", + ) + + # 6d -- json.dumps(..., default=float). `json.dumps` raises a TypeError on a Decimal, + # and `default=float` is the one-keyword fix that silently converts every money value in + # the payload to a double. The serialiser returns plain types and dumps nothing itself. + if name in ("json.dumps", "json.dump"): + for keyword in node.keywords: + if keyword.arg in ("default", "cls"): + found.add( + "rule6_serialisation", + f"{stem}:{owner}: json.dumps(..., {keyword.arg}=...) (line " + f"{node.lineno}) -- a custom encoder is how every Decimal in the " + "payload becomes a double in one keyword", + ) return found @@ -476,6 +592,57 @@ def test_rule_5_no_process_or_network_orchestration_in_the_console_layer( assert not findings.get("rule5_orchestration"), findings["rule5_orchestration"] +def test_rule_6_the_serialisation_contract_holds_in_the_serialiser( + findings: dict[str, list[str]], +) -> None: + """Issue #533's rule: the module that writes keel's JSON never converts a money value to a + binary float, never rounds one, never renders one through `normalize()`, and never installs a + `json` encoder that would do any of those for it. + + All four are the same failure with different spellings -- a `Decimal` arriving in a browser as + an IEEE-754 double -- and all four are SILENT, which is why they are pinned mechanically + rather than left to review. `tests/web/test_payload.py` pins the same contract from the other + end, by walking the rendered payload. + """ + assert not findings.get("rule6_serialisation"), findings["rule6_serialisation"] + + +def test_rule_6_is_proven_false_capable() -> None: + """Rule 6's own positive control, run over a synthetic module that commits all four sins. + + Without this, a typo in an attribute name or a resolver that never matches would leave Rule 6 + green over a serialiser doing exactly what it forbids -- and Rule 6 guards a contract seven + downstream issues inherit, so a vacuously-green version of it is worse than none. + + The allowed shape is included too: `float(ts)` inside `_gmt` must NOT be flagged, or the rule + is a blanket ban wearing an allowlist. + """ + snippet = ( + "import json\n" + "def _gmt(ts, fmt):\n" + " return float(ts)\n" + "def bad(price, payload):\n" + " a = price.normalize()\n" + " b = float(price)\n" + " c = round(price, 2)\n" + " d = json.dumps(payload, default=float)\n" + " return a, b, c, d\n" + ) + tree = ast.parse(snippet) + owners = _enclosing_functions(tree) + + messages = _rule6_findings("payload", tree, _collect_aliases(tree), owners).get( + "rule6_serialisation", [] + ) + + assert len(messages) == 4, messages + assert any(".normalize()" in m for m in messages) + assert any("float(price)" in m for m in messages) + assert any("round()" in m for m in messages) + assert any("default=" in m for m in messages) + assert not any(":_gmt:" in m for m in messages), messages + + def test_rule_5_is_proven_false_capable() -> None: """The detector's own positive control: a synthetic module that shells out and execv's is flagged (an AST scan that silently matched nothing would make Rule 5 @@ -505,6 +672,10 @@ def test_the_scan_actually_scanned_the_console_layer() -> None: # Named explicitly so that deleting or renaming a web module fails HERE, loudly, rather than # quietly shrinking the scanned set and leaving the rules green over less code. assert {"render", "security", "server"} <= stems + # #533: the JSON serialiser joins them. Named here for the same reason and with an extra one: + # Rule 6 is scoped BY STEM, so a serialiser renamed out of `SERIALISER_STEMS` would keep + # passing Rules 1-5 while silently losing the money-contract pin entirely. + assert SERIALISER_STEMS <= stems assert any(os.path.join("keel", "web") in path for path in paths) assert { "compliance_console", diff --git a/tests/web/test_payload.py b/tests/web/test_payload.py new file mode 100644 index 0000000..cb9ac2a --- /dev/null +++ b/tests/web/test_payload.py @@ -0,0 +1,774 @@ +"""The serialisation contract (#533) -- the rule every endpoint in the web-UI milestone inherits. + +These tests exist because the failure they guard against is SILENT. A `Decimal` that becomes a +JSON number does not raise, does not warn, and looks right for every value a developer happens to +try by hand: `0.1 + 0.2` only misbehaves at the seventeenth digit, and a notional only loses a +cent once it is large enough that nobody is checking. The contract is therefore pinned +mechanically -- a recursive walk over the real payload -- rather than by review. + +Three properties are pinned, in the order the issue states them: + +* **Rule 1** -- money crosses the wire as a STRING. `test_no_wire_value_is_ever_a_json_number` + walks the parsed payload and fails on any `int`/`float` anywhere in it, which is a strictly + stronger statement than "no monetary field is a number" and needs no per-field judgement about + which fields are monetary. `test_the_number_walker_is_proven_false_capable` is its positive + control: a walker that silently matched nothing would make the whole milestone vacuously safe. +* **Rule 2** -- values arrive presentation-ready. + `test_the_serialiser_computes_nothing_every_wire_figure_came_from_the_report` proves the + serialiser reads rather than derives: every `Decimal`-parseable value on the wire must equal a + `Decimal`/`int` the report builder already put on the report. +* **Rule 3** -- semantic state is an explicit field. The `state` tests pin that a loss is + labelled `"bad"` in Python and that the glyph in `display` carries the same distinction without + colour (#532's non-colour signal). + +Plus the hazard that has already cost this codebase real orders: `Decimal.normalize()` renders +`Decimal("50")` as `Decimal("5E+1")`, and `"5E+1"` on the wire is a number no `Decimal(...)` +constructor on the far side will read as fifty in a form a human recognises. +`test_scientific_notation_never_reaches_the_wire` feeds the serialiser the exact shapes that +produce it. +""" + +from __future__ import annotations + +import json +from decimal import Decimal +from typing import Any + +import pytest + +from keel.commands.activity import ActivityCycle, ActivityEvent, ActivityFeed +from keel.commands.insights import ( + AccountSummary, + GateDistance, + InsightsReport, + JournalEntry, + JournalReport, + RuleTrackRecord, +) +from keel.commands.status import ( + AutonomyStatus, + MarketSessionStatus, + OpenPositionStatus, + ProductFreshness, + RuleSummary, + StatusReport, + SubscriptionStatusRow, + WithdrawalAttestationStatus, +) +from keel.web import payload + +NOW_TS = 1_756_000_000 + + +# -- fixtures: real report dataclasses, populated ------------------------------------------------ + + +def _status_report(**overrides: Any) -> StatusReport: + """A fully populated `StatusReport` -- every list non-empty, so the recursive walkers below + actually reach every branch of the payload instead of passing over empty collections.""" + base: dict[str, Any] = dict( + now_ts=NOW_TS, + mode="paper", + kill_switch_engaged=False, + autonomy=AutonomyStatus( + live=True, + autonomous=True, + autonomous_until=NOW_TS + 3600, + updated_ts=NOW_TS - 60, + profile_readable=True, + ), + equity_state_mode="paper", + high_water_mark=Decimal("12345.67"), + drawdown_total_pct=Decimal("0.05"), + drawdown_weekly_pct=Decimal("0.01"), + max_total_dd_pct=Decimal("0.20"), + max_weekly_dd_pct=Decimal("0.08"), + rail11_status="ok", + withdrawal_attestation=WithdrawalAttestationStatus( + state="attested", + enabled=True, + attested_at=NOW_TS - 86400, + expires_in_sec=6 * 86400, + expired_for_sec=None, + ), + paper_cash_usdc=Decimal("955.25"), + open_positions=[ + OpenPositionStatus( + id=7, + product_id="BTC-USD", + rule_name="turtle_breakout", + qty=Decimal("0.01000000"), + entry_price=Decimal("50000.00"), + opened_at=NOW_TS - 7200, + has_bracket=True, + ) + ], + rule_counts={"live": 1, "paper": 2}, + live_rules=[ + RuleSummary( + id=3, kind="dca", status="live", product_id="ETH-USD", params={"budget_usd": 50} + ) + ], + data_freshness=[ + ProductFreshness( + product_id="BTC-USD", granularity="ONE_HOUR", last_ts=NOW_TS - 300, age_sec=300 + ) + ], + subscriptions=[ + SubscriptionStatusRow( + venue="coinbase", + tier_name="advanced", + pacing="steady", + stored_status="active", + effective_status="active", + effective_cap=Decimal("2500.00"), + ) + ], + market_session=MarketSessionStatus(state="open", recorded_ts=NOW_TS - 120, defused=False), + ) + base.update(overrides) + return StatusReport(**base) + + +def _insights_report(**overrides: Any) -> InsightsReport: + base: dict[str, Any] = dict( + now_ts=NOW_TS, + account=AccountSummary( + mode="paper", + equity_state_mode="paper", + high_water_mark=Decimal("12345.67"), + drawdown_total_pct=Decimal("0.05"), + drawdown_weekly_pct=Decimal("0.01"), + max_total_dd_pct=Decimal("0.20"), + max_weekly_dd_pct=Decimal("0.08"), + rail11_status="ok", + paper_cash_usdc=Decimal("955.25"), + ), + rules=[ + RuleTrackRecord( + rule_name="turtle_breakout", + status="paper", + promotion_class="trend", + n_trades=12, + win_rate=41.5, + avg_win=Decimal("31.20"), + avg_loss=Decimal("-14.05"), + realized_rr=Decimal("2.22"), + expectancy=Decimal("0.8410"), + profit_factor=Decimal("1.57"), + max_drawdown=Decimal("-88.40"), + significant=False, + gate=GateDistance( + rule_name="turtle_breakout", + promotion_class="trend", + n_trades=12, + min_trades=30, + trades_remaining=18, + win_rate=41.5, + min_win_rate=40.0, + realized_rr=Decimal("2.22"), + min_rr=Decimal("2.0"), + expectancy=Decimal("0.8410"), + min_expectancy=Decimal("0.5"), + passing=False, + blocking_reasons=["n_trades 12 < 30"], + ), + ) + ], + closed_trade_count=12, + ) + base.update(overrides) + return InsightsReport(**base) + + +def _journal_report(**overrides: Any) -> JournalReport: + base: dict[str, Any] = dict( + now_ts=NOW_TS, + mode="paper", + entries=[ + JournalEntry( + closed_at=NOW_TS - 3600, + opened_at=NOW_TS - 86400, + rule_name="turtle_breakout", + product_id="BTC-USD", + qty=Decimal("0.01000000"), + entry_fill=Decimal("50000.00"), + exit_fill=Decimal("48766.00"), + pnl_net=Decimal("-12.34"), + fees=Decimal("0.6150"), + r_multiple=Decimal("-0.82"), + is_dca=False, + outcome="loss", + ), + JournalEntry( + closed_at=NOW_TS - 1800, + opened_at=NOW_TS - 90000, + rule_name="dca", + product_id="ETH-USD", + qty=Decimal("0.50000000"), + entry_fill=Decimal("2000.00"), + exit_fill=None, + pnl_net=Decimal("41.00"), + fees=None, + r_multiple=None, + is_dca=True, + outcome="dca", + ), + ], + total_count=2, + filters={ + "rule": None, + "asset": None, + "since_ts": NOW_TS - 604800, + "until_ts": None, + "limit": 50, + "include_open": False, + }, + ) + base.update(overrides) + return JournalReport(**base) + + +def _activity_feed(**overrides: Any) -> ActivityFeed: + cycle = ActivityCycle( + cycle_id="c-1", + started_ts=float(NOW_TS - 600), + ended_ts=float(NOW_TS - 598), + mode="paper", + products=("BTC-USD",), + rules=("turtle_breakout",), + signals=1, + blocked=1, + entered=0, + exited=0, + errors=2, + highlights=("rail 11 blocked an entry",), + events=( + ActivityEvent( + ts=float(NOW_TS - 599), + level="INFO", + event="cycle_start", + cycle_id="c-1", + fields={"products": 1, "budget_usd": Decimal("50.00"), "dry_run": True}, + ), + ), + events_dropped=3, + ) + base: dict[str, Any] = dict( + status="ok", + source="/tmp/keel.log", + cycles=(cycle,), + detail=None, + lines_read=120, + lines_skipped=2, + window_truncated=True, + cycles_dropped=1, + scope="today", + scope_start_ts=float(NOW_TS - 50000), + now_ts=float(NOW_TS), + cycles_out_of_scope=4, + last_cycle_before_scope=cycle, + scope_fully_covered=False, + ) + base.update(overrides) + return ActivityFeed(**base) + + +def _every_payload() -> dict[str, Any]: + """All four payload builders at once. The guards below run over the whole surface, because a + contract that holds for `status` and leaks on `journal` is not a contract.""" + return { + "status": payload.status_payload(_status_report()), + "insights": payload.insights_payload(_insights_report()), + "journal": payload.journal_payload(_journal_report()), + "activity": payload.activity_payload(_activity_feed()), + } + + +# -- recursive walkers --------------------------------------------------------------------------- + + +def _walk(node: Any, path: str = "$") -> list[tuple[str, Any]]: + """Every leaf in a parsed JSON document, with the path that reaches it. Written here rather + than imported so the guard cannot be weakened by a change to production code.""" + if isinstance(node, dict): + out: list[tuple[str, Any]] = [] + for key, value in node.items(): + out.extend(_walk(value, f"{path}.{key}")) + return out + if isinstance(node, list): + out = [] + for index, value in enumerate(node): + out.extend(_walk(value, f"{path}[{index}]")) + return out + return [(path, node)] + + +def _json_numbers(document: Any) -> list[str]: + """Paths whose leaf is a JSON number. `bool` is excluded FIRST and deliberately: Python's + `bool` is a subclass of `int`, so an `isinstance(x, int)` test alone would flag every JSON + `true` as a number and make this guard fail for the wrong reason.""" + return [ + path + for path, leaf in _walk(document) + if not isinstance(leaf, bool) and isinstance(leaf, (int, float)) + ] + + +# -- Rule 1: money crosses the wire as a string --------------------------------------------------- + + +def test_no_wire_value_is_ever_a_json_number() -> None: + """THE guard the whole milestone rests on. + + Stated as "no JSON numbers ANYWHERE" rather than "no monetary field is a number", because the + second form needs a list of which fields are monetary -- and that list is exactly the thing + that rots when a new field lands. The strong form needs no list and cannot rot. + + The spec's one reserved exception is the `sort` field (ordering only, never displayed, never + summed), which #533 explicitly does not ship. If it is ever added, THIS is where its allowance + goes, named and scoped, so that adding it is a decision rather than an accident. + """ + document = json.loads(json.dumps(_every_payload())) + + assert _json_numbers(document) == [] + + +def test_the_number_walker_is_proven_false_capable() -> None: + """The guard's own positive control. An `_json_numbers` that matched nothing -- a typo in the + isinstance, a walker that stops at the first list -- would make the test above green over a + payload full of doubles. So: plant one at depth, under a list, and require it to be found; + and require a JSON `true` NOT to be found, since `bool` is an `int` in Python.""" + planted = {"a": [{"b": {"c": 0.1}}], "flag": True, "text": "0.1"} + + assert _json_numbers(planted) == ["$.a[0].b.c"] + + +def test_a_decimal_round_trips_through_the_wire_string_without_loss() -> None: + """Rule 1's actual point: the string form is not merely "not a float", it is EXACT. + + Every value below is one `float` would corrupt or one that has bitten this codebase before -- + a quantity at Coinbase's eight-decimal base increment, a notional past the 17 significant + digits a double carries, and the `Decimal("50")` whose `normalize()` form is `5E+1`. + """ + hazards = [ + Decimal("0.1"), + Decimal("0.01000000"), + Decimal("50"), + Decimal("50.00"), + Decimal("12345.67"), + Decimal("-12.34"), + Decimal("0.00000001"), + Decimal("123456789012345678901234567890.123456789"), + Decimal("5E+1"), + Decimal("1E-9"), + Decimal("-0.000000000000000001"), + ] + + for source in hazards: + wire = payload.money(source)["value"] + assert Decimal(wire) == source, (source, wire) + + +def test_the_wire_value_is_decimal_parseable_and_the_display_string_is_not() -> None: + """The two strings do different jobs and must not be conflated. `value` is machine input -- + no grouping separators, no currency symbol, no glyph, so `Decimal(value)` just works. + `display` is the human's, and carries all of that.""" + field = payload.money(Decimal("12345.67")) + + assert field["value"] == "12345.67" + assert Decimal(field["value"]) == Decimal("12345.67") + assert field["display"] == "$12,345.67" + + +def test_scientific_notation_never_reaches_the_wire() -> None: + """The `5E+1` hazard, at the level of the whole payload rather than one helper. + + `Decimal.normalize()` turns `Decimal("50")` into `Decimal("5E+1")`, and `str()` on any Decimal + with a positive exponent does the same. A client reading `"5E+1"` as a price has read fifty as + five, or as nothing at all. Every string leaf of every payload is checked, not just the ones + the author remembered were money. + """ + report = _status_report( + high_water_mark=Decimal("5E+1"), + paper_cash_usdc=Decimal("1E-9"), + drawdown_total_pct=Decimal("2E-3"), + ) + document = json.loads(json.dumps(payload.status_payload(report))) + + offenders = [ + (path, leaf) + for path, leaf in _walk(document) + if isinstance(leaf, str) and ("E+" in leaf or "E-" in leaf or "e+" in leaf) + ] + + assert offenders == [] + assert payload.money(Decimal("5E+1"))["value"] == "50" + assert payload.money(Decimal("1E-9"), places=9)["display"] == "$0.000000001" + + +# -- Rule 2: the serialiser computes nothing ------------------------------------------------------ + + +def _report_figures(node: Any, seen: set[int] | None = None) -> set[Decimal]: + """Every `Decimal`/`int` the report builder already put on the report, as `Decimal`. + + Walks dataclasses, mappings and sequences generically rather than naming fields, so a field + added upstream is covered the day it appears instead of the day someone remembers to add it + here.""" + seen = set() if seen is None else seen + if id(node) in seen: + return set() + seen.add(id(node)) + if isinstance(node, bool): + return set() + if isinstance(node, Decimal): + return {node} + if isinstance(node, int): + return {Decimal(node)} + if isinstance(node, str): + return set() + if isinstance(node, dict): + out: set[Decimal] = set() + for key, value in node.items(): + out |= _report_figures(key, seen) | _report_figures(value, seen) + return out + if isinstance(node, (list, tuple, set, frozenset)): + out = set() + for item in node: + out |= _report_figures(item, seen) + return out + fields = getattr(node, "__dataclass_fields__", None) + if fields is not None: + out = set() + for name in fields: + out |= _report_figures(getattr(node, name), seen) + return out + return set() + + +def test_the_serialiser_computes_nothing_every_wire_figure_came_from_the_report() -> None: + """Rule 2, enforced rather than asserted in prose. + + Every `Decimal`-parseable `value` on the wire must equal a figure the report ALREADY held. A + serialiser that multiplied `qty` by `entry_price` to offer the client a notional, or scaled a + drawdown fraction by 100 to make it read as a percentage, would produce a number that appears + nowhere on the report -- and would fail here, by name and path. + + `StatusReport` and `JournalReport` are used because every figure they carry is a `Decimal` or + an `int`; `InsightsReport` carries `float` win rates, which have no exact decimal form and are + covered by their own test below. + """ + for name, report, built in ( + ("status", _status_report(), payload.status_payload(_status_report())), + ("journal", _journal_report(), payload.journal_payload(_journal_report())), + ): + available = _report_figures(report) + for path, leaf in _walk(json.loads(json.dumps(built))): + if not path.endswith(".value") or not isinstance(leaf, str) or not leaf: + continue + try: + figure = Decimal(leaf) + except ArithmeticError: + continue # an ISO-8601 instant or an enum word, not a figure + except ValueError: + continue + assert figure in available, f"{name} {path}: {leaf!r} is on no report field" + + +def test_an_open_position_carries_no_notional_because_the_report_holds_none() -> None: + """The single most tempting place to break Rule 2, pinned so the temptation fails loudly. + + `qty * entry_price` is one multiplication away, and the spec's illustrative payload even shows + a `notional` on a position. But `OpenPositionStatus` (`keel/commands/status.py`) carries + `qty`, `entry_price`, `opened_at`, `rule_name` and `has_bracket` -- and NOT a notional, and + not a P&L. Serialising one would mean this layer computed a figure the trading rails never + saw, which is precisely the invariant `test_console_thinness.py` exists to keep checkable. + + If a notional is wanted, it is added to `gather_status`, where the rails can see it. Then this + test changes -- deliberately, in a commit that says so. + """ + position = payload.status_payload(_status_report())["open_positions"][0] + + assert "notional" not in position + assert "pnl" not in position + assert position["qty"]["value"] == "0.01000000" + assert position["entry_price"]["value"] == "50000.00" + + +def test_a_drawdown_fraction_is_not_rescaled_into_a_percentage() -> None: + """`drawdown_total_pct` is named for a percentage but is a FRACTION: `_rail11_status` compares + it against `max_total_dd_pct=Decimal("0.20")`, so `0.05` means five percent, not five basis + points. Multiplying by 100 to make the label true would be arithmetic in the serialiser -- so + the value crosses unchanged and the display says what it is. + + (`keel/web/render.py:150` renders this same field through `pct()`, which appends a `%` to the + raw fraction and therefore prints "0.05%" for a 5% drawdown. The contract does not inherit + that; fixing the HTML renderer is a separate change against a separate surface.) + """ + built = payload.status_payload(_status_report()) + + assert built["drawdown"]["total"]["value"] == "0.05" + assert "%" not in built["drawdown"]["total"]["display"] + assert built["drawdown"]["max_total"]["value"] == "0.20" + + +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()) + + assert built["rules"][0]["win_rate"]["value"] == "41.5" + assert built["rules"][0]["win_rate"]["display"] == "41.5%" + + +# -- Rule 3: semantic state is a field, never an inference ---------------------------------------- + + +def test_a_loss_is_labelled_bad_in_python_not_by_the_sign_on_the_wire() -> None: + """Rule 3. The client must never read a minus sign and conclude "bad" -- that is arithmetic by + another name, and it relocates a trading judgement into a browser.""" + entries = payload.journal_payload(_journal_report())["entries"] + loss, gain = entries[0], entries[1] + + assert loss["pnl"]["state"] == "bad" + assert loss["pnl"]["value"] == "-12.34" + assert gain["pnl"]["state"] == "good" + + +def test_state_survives_without_colour() -> None: + """#532: colour must never be the only signal. `display` carries a glyph and an explicit sign, + so profit and loss stay distinguishable in greyscale, on e-ink, in sunlight, and to the + roughly one man in twelve who cannot separate the palette's red from its green.""" + entries = payload.journal_payload(_journal_report())["entries"] + + assert entries[0]["pnl"]["display"] == "▼ −$12.34" + assert entries[1]["pnl"]["display"] == "▲ +$41.00" + + +def test_every_field_carries_all_three_keys_always() -> None: + """Schema uniformity is itself part of the contract: a client that has to test whether `state` + is present is branching on payload SHAPE, which is inference by another route. Every field + object has `value`, `display` and `state`, in every payload, for every value including + absent ones.""" + for name, built in _every_payload().items(): + leaves = _walk(built) + by_parent: dict[str, set[str]] = {} + for path, _leaf in leaves: + parent, _, key = path.rpartition(".") + by_parent.setdefault(parent, set()).add(key) + for parent, keys in by_parent.items(): + if "value" in keys: + assert keys == {"value", "display", "state"}, f"{name} {parent}: {sorted(keys)}" + + +def test_every_state_word_is_one_the_contract_declares() -> None: + """A free-text `state` would push the client back into guessing. The vocabulary is closed, and + #532's palette and glyph table key off exactly these words.""" + for name, built in _every_payload().items(): + for path, leaf in _walk(built): + if path.endswith(".state"): + assert leaf in payload.STATES, f"{name} {path}: {leaf!r}" + + +def test_absence_is_unknown_and_never_a_zero() -> None: + """`None` means "not recorded"; `0` means "recorded as zero". Collapsing the first into the + second is the shape of the always-passing fee rail (#198) -- a missing number that reads as a + real one. On the wire, absence is an empty `value`, a dash, and `state: "unknown"`.""" + absent = payload.money(None) + + assert absent == {"value": "", "display": "—", "state": "unknown"} + assert payload.money(Decimal("0.00"))["value"] == "0.00" + assert payload.money(Decimal("0.00"))["state"] == "neutral" + + +def test_a_kill_switch_carries_its_own_judgement() -> None: + """A boolean the operator must act on is not a bare `true`: it arrives with the word to show + and the state to style it by.""" + engaged = payload.status_payload(_status_report(kill_switch_engaged=True))["kill_switch"] + clear = payload.status_payload(_status_report(kill_switch_engaged=False))["kill_switch"] + + assert engaged["display"] == "ENGAGED" + assert engaged["state"] == "bad" + assert clear["state"] == "good" + + +def test_an_unknown_rail_state_is_unknown_and_not_quietly_good() -> None: + """`_rail11_status` returns `"unknown"` when a drawdown scalar was never written -- explicitly, + because reporting an unwritten value as a confident "ok" would be a lie. The contract keeps + that distinction: `unknown` is its own state, not a green one. + + This is why the classifier here is NOT `render._tone_for_rail`, which matches on the words + "breach"/"halt"/"warn" and falls through to `"good"` -- so it styles a rail nobody has measured + as if it had passed. + """ + halted = payload.status_payload(_status_report(rail11_status="HALTED")) + unknown = payload.status_payload(_status_report(rail11_status="unknown")) + ok = payload.status_payload(_status_report(rail11_status="ok")) + + assert halted["drawdown"]["rail11"]["state"] == "bad" + assert unknown["drawdown"]["rail11"]["state"] == "unknown" + assert ok["drawdown"]["rail11"]["state"] == "good" + + +def test_an_expired_withdrawal_attestation_reads_as_bad() -> None: + """Rail 17's state is the one an operator must act on within a TTL; it arrives judged.""" + expired = payload.status_payload( + _status_report( + withdrawal_attestation=WithdrawalAttestationStatus( + state="expired", + enabled=True, + attested_at=NOW_TS - 12 * 86400, + expires_in_sec=None, + expired_for_sec=5 * 86400, + ) + ) + )["withdrawal_attestation"] + + assert expired["state"]["state"] == "bad" + # `_human_age`, exactly as `_rail17_line` prints it -- "EXPIRED 5d ago", not "5d". + assert expired["expired_for"]["display"] == "5d ago" + + +# -- presentation-ready: displays are built here, never derivable there --------------------------- + + +def test_a_quantity_shows_its_unit_and_drops_its_padding_without_normalize() -> None: + """`0.01000000 BTC` is how the ledger stores it and `0.01 BTC` is how a human reads it. The + trim is string work on an already-formatted decimal, NOT `Decimal.normalize()` -- normalize is + what turns `50` into `5E+1`.""" + assert payload.quantity(Decimal("0.01000000"), unit="BTC")["display"] == "0.01 BTC" + assert payload.quantity(Decimal("0.01000000"), unit="BTC")["value"] == "0.01000000" + assert payload.quantity(Decimal("50"), unit="BTC")["display"] == "50 BTC" + assert payload.quantity(Decimal("1234.5"), unit="ETH")["display"] == "1,234.5 ETH" + + +def test_counts_are_grouped_for_reading_and_exact_on_the_wire() -> None: + assert payload.count(1234567)["value"] == "1234567" + assert payload.count(1234567)["display"] == "1,234,567" + assert payload.count(None)["state"] == "unknown" + + +def test_an_instant_arrives_as_utc_in_both_forms() -> None: + """Every day boundary in keel is UTC. `value` is ISO-8601 with an explicit `Z` so a client can + hand it straight to `new Date(...)` without arithmetic; `display` is the same instant already + written out, so no client ever formats a date to show it in a table.""" + field = payload.moment(0) + + assert field["value"] == "1970-01-01T00:00:00Z" + assert field["display"] == "1970-01-01 00:00:00 UTC" + + +def test_a_broken_instant_degrades_instead_of_taking_the_payload_down() -> None: + """Log timestamps come from outside this process. A corrupt one renders as absent -- the same + choice `render.utc` already makes -- because a 500 on the whole page is a worse answer than a + dash in one cell.""" + for broken in (None, float("nan"), 1e30, float("inf")): + assert payload.moment(broken)["state"] == "unknown" + + +def test_a_duration_uses_the_cli_s_own_words() -> None: + """`_human_age`/`_human_remaining` live in `keel/commands/status.py` and are what `keel status` + prints. The web payload calls them rather than re-deriving the ladder, so the browser and the + terminal can never disagree about how old the same candle is.""" + assert payload.duration(300)["display"] == "5m ago" + assert payload.duration(6 * 86400, elapsed=False)["display"] == "6d" + assert payload.duration(300)["value"] == "300" + + +def test_the_activity_feed_stringifies_the_open_ended_event_fields() -> None: + """`ActivityEvent.fields` is deliberately open -- every `log_event` call site invents its own + kwargs, and the parser keeps them whole rather than projecting onto a fixed schema. That means + arbitrary ints and Decimals arrive here, and Rule 1 applies to them exactly as it does to a + known field: they cross as strings.""" + event = payload.activity_payload(_activity_feed())["cycles"][0]["events"][0] + + assert event["fields"] == {"products": "1", "budget_usd": "50.00", "dry_run": "true"} + + +def test_the_journal_filters_cross_as_strings_too() -> None: + """`JournalReport.filters` holds the query that produced the report -- `limit`, `since_ts` and + friends, which are ints. They are echoed back for the client to show, and an echoed int is a + JSON number like any other.""" + filters = payload.journal_payload(_journal_report())["filters"] + + assert filters["limit"] == "50" + assert filters["since_ts"] == str(NOW_TS - 604800) + assert filters["rule"] == "" + + +def test_an_error_count_is_bad_and_a_quiet_cycle_is_neutral() -> None: + """A cycle in which the agent looked and nothing happened is a POSITIVE observation, not an + absence of one -- it is how the feed answers "is it alive". So quiet is `neutral`, never + `warn`; errors are what turn a row `bad`.""" + cycle = payload.activity_payload(_activity_feed())["cycles"][0] + + assert cycle["errors"]["state"] == "bad" + assert cycle["quiet"]["state"] == "neutral" + + +# -- totality ------------------------------------------------------------------------------------ + + +def test_every_builder_survives_a_completely_empty_report() -> None: + """A fresh install is the commonest state there is, and it is not an error. Every list empty, + every optional `None`, no exception, and the guards still hold.""" + empty_status = _status_report( + autonomy=AutonomyStatus( + live=False, + autonomous=False, + autonomous_until=None, + updated_ts=None, + profile_readable=False, + ), + equity_state_mode=None, + high_water_mark=None, + drawdown_total_pct=None, + drawdown_weekly_pct=None, + rail11_status="unknown", + withdrawal_attestation=WithdrawalAttestationStatus( + state="unattested", + enabled=None, + attested_at=None, + expires_in_sec=None, + expired_for_sec=None, + ), + paper_cash_usdc=None, + open_positions=[], + rule_counts={}, + live_rules=[], + data_freshness=[], + subscriptions=[], + market_session=MarketSessionStatus(state=None, recorded_ts=None), + ) + documents = { + "status": payload.status_payload(empty_status), + "insights": payload.insights_payload(_insights_report(rules=[], closed_trade_count=0)), + "journal": payload.journal_payload(_journal_report(entries=[], total_count=0, filters={})), + "activity": payload.activity_payload( + ActivityFeed(status="missing", source="/tmp/nope.log") + ), + } + + parsed = json.loads(json.dumps(documents)) + assert _json_numbers(parsed) == [] + + +@pytest.mark.parametrize( + "builder", + ["status_payload", "insights_payload", "journal_payload", "activity_payload"], +) +def test_every_payload_is_json_serialisable_without_a_custom_encoder(builder: str) -> None: + """The builders return plain `dict`/`list`/`str` -- no `Decimal` reaches `json.dumps`. That + 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.""" + report = { + "status_payload": _status_report(), + "insights_payload": _insights_report(), + "journal_payload": _journal_report(), + "activity_payload": _activity_feed(), + }[builder] + + json.dumps(getattr(payload, builder)(report)) # no cls=, no default= From a3e8591392f3d150ea3d2448468c4cc344cd8f33 Mon Sep 17 00:00:00 2001 From: Elmehdi Aitbrahim Date: Sun, 23 Aug 2026 21:54:19 -0400 Subject: [PATCH 2/2] fix(web): read shown_count, dump nested fields as JSON, read the sign once MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three review findings on the serialisation contract, each a place the first draft's own stated rule did not actually hold. 1. `shown_count` computed, and its guard passed by coincidence. `journal_payload` shipped `count(len(report.entries))` -- a figure `JournalReport` does not hold, which is precisely the criterion "the serialiser computes nothing" claims to prove. It is numerically harmless (a list length is exact) and that is what made it dangerous: the runtime guard compares FIGURES, and the fixture set `total_count=2` with exactly 2 entries, so `Decimal(2)` was already in the report's figure set. One fixture edit from red. `JournalReport.shown_count` is now a property on the report, the way `ActivityCycle.is_quiet`/`.key` already are -- a derived reading rather than a stored field, because a second field holding `len(self.entries)` is state that can drift from the list it describes. `asdict()` skips properties, so `keel insights journal --json` is unchanged. The serialiser reads it. The fixture now sets `total_count=812` against 2 entries, so the two can no longer coincide; with the old code that guard fires by path (`journal $.shown_count.value: '2' is on no report field`). That is still not enough on its own. Once the property exists, `len(entries)` and `report.shown_count` are the same number, so the runtime guard cannot tell a measured length from a read one -- confirmed, it passes on that mutation. So Rule 6 of `test_console_thinness.py` gains 6e: `len()` is banned in the serialiser outright. An AST rule does not care that the coincidence is numerically benign, and it fails by file, function and line. The two guards fail for different reasons, which is the point of having both. 2. `stringify` rendered Python reprs. `ActivityEvent.fields` is parsed out of the engine's own log lines, so a value can be a list or an object, and the `str(value)` fallback produced `"{'a': 1, 'b': None}"` -- single quotes, `None`, `True`: text no client can parse and none should display. No JSON number leaked out of it, so the number-walker stayed green over a clear Rule 2 breach. Worse, `str([1e+50])` is `"[1e+50]"`. That is scientific notation reaching the wire through a path no money field touches, so every `Decimal` precaution in the module was irrelevant to it -- and the existing notation test could not see it, because it only feeds Decimals through named report fields. Nested values now cross as JSON text, normalised leaf by leaf through `stringify` itself first, so the numbers inside a structure are rendered by exactly the rules the top-level ones are. Flattened to one string rather than kept as a nested object because Rule 2 says an open-ended structure handed to the client is a structure the client must decide how to format. `json.dumps` is called plainly -- every leaf is already a string, so no encoder is needed, which is why Rule 6d's ban on `default=`/`cls=` costs nothing. Depth is capped at 6 with a `(nested)` marker: `json.loads` cannot build a cycle but it can build a thousand nested lists, and the depth of a line keel did not write must not decide how deep this process recurses. The notation test now has a sibling covering the open-ended path, including `[1e+50]` and a nested `1e-30`. 3. `-0.00` made display and state disagree. `money(Decimal("-0.00"))` displayed `−$0.00` with `state: "neutral"`: the sign was read off the FORMATTED text (`format(Decimal("-0.00"), ",.2f")` is `"-0.00"`) while the state was read off the exact value (`Decimal("-0.00") == 0`). A client styling by `state` and showing `display` printed a minus sign in a neutral colour. Rule 3's premise is that the two never disagree, and a premise with one exception is not a premise. There is now one reading, `_is_negative`, from the exact `Decimal`, feeding both the glyph and the state; `_magnitude` only strips and formats. Stripping rather than negating stays deliberate -- `abs()`/`-value` is arithmetic on money, which Rule 3 of the thinness pin forbids outright. A value that IS negative but rounds to zero at the display precision keeps its minus and keeps `bad` beside it: that is a small loss honestly labelled, not a disagreement, and suppressing the sign there would report a loss as a break-even. Pinned by a case-walk asserting the equivalence over negative zero, true zero, `0E-8`, a sub-precision loss and ordinary gains and losses, in `money`, `percent`, `ratio` and `quantity`. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01NyeggYtojNXCTHeD3JHxb6 --- keel/commands/insights.py | 17 +++ keel/web/payload.py | 120 +++++++++++++++--- tests/commands/test_console_thinness.py | 42 +++++-- tests/web/test_payload.py | 160 +++++++++++++++++++++++- 4 files changed, 306 insertions(+), 33 deletions(-) diff --git a/keel/commands/insights.py b/keel/commands/insights.py index 52b8e8e..94ec8b3 100644 --- a/keel/commands/insights.py +++ b/keel/commands/insights.py @@ -90,6 +90,23 @@ class JournalReport: """The full filtered (closed) entry count BEFORE `--limit` truncates for display.""" filters: dict[str, Any] + @property + def shown_count(self) -> int: + """How many entries this report actually carries, after `--limit` truncated. + + A derived reading rather than a stored field, for the reason `ActivityCycle.key` and + `.is_quiet` are: a second field holding `len(self.entries)` is state that can drift out of + agreement with the list it describes, and there is nothing to gain by letting it. + + It lives HERE rather than in a front-end because `total_count` alone does not answer "am I + looking at a page"; the pair does, and the pair is the report's statement about itself. + Every renderer -- the CLI's lines, the HTML, `keel/web/payload.py`'s JSON -- reads it + instead of measuring the list again, so none of them can disagree about how many rows the + same report held. `asdict()` does not include properties, so `--json`'s shape is + unchanged. + """ + return len(self.entries) + @dataclass(frozen=True) class GateDistance: diff --git a/keel/web/payload.py b/keel/web/payload.py index 49292f5..72889c0 100644 --- a/keel/web/payload.py +++ b/keel/web/payload.py @@ -87,6 +87,7 @@ from __future__ import annotations +import json import time from collections.abc import Mapping from decimal import Decimal, InvalidOperation @@ -199,18 +200,35 @@ def _decimalise(value: Decimal | float | None) -> Decimal | None: return candidate if candidate.is_finite() else None -def _magnitude(value: Decimal, places: int) -> tuple[bool, str]: - """`(is_negative, unsigned_grouped_text)`. +def _magnitude(value: Decimal, places: int) -> str: + """The unsigned, grouped, human-readable text for `value`. - The sign is taken off the FORMATTED string rather than off the number (`abs()`, `-value`), - because this layer does no arithmetic on money at all -- Rule 3 of the thinness pin, and the - property that makes "the browser never sees a computed figure" checkable rather than merely - likely. Note the sign here is the sign of the *rendered* text: a value that rounds to zero at - `places` still renders "0.00". The `state` a caller attaches is derived from the exact - `Decimal` instead, so a `-0.001` P&L is still labelled a loss. + The sign is stripped here and decided elsewhere -- by `_is_negative` below, from the exact + `Decimal` -- so that ONE reading of the sign feeds both the displayed glyph and the `state`. + + The first draft read the sign off this formatted string instead, and the two readings could + disagree: `format(Decimal("-0.00"), ",.2f")` is `"-0.00"`, so the display said `−$0.00` while + `state` said `neutral`, because `Decimal("-0.00") == 0`. A client styling by `state` and + showing `display` would then have printed a minus sign in a neutral colour -- a small thing, + but Rule 3's whole premise is that the two never disagree, and a premise with one exception + is not a premise. + + Stripping rather than negating is still deliberate: `abs()`/`-value` would be arithmetic on + money in this layer, which Rule 3 of the thinness pin forbids outright. + """ + return format(value, f",.{places}f").lstrip("-") + + +def _is_negative(value: Decimal) -> bool: + """THE reading of a money value's sign, for both the glyph and the state. + + A comparison, not arithmetic. Negative zero is NOT negative here -- `Decimal("-0.00") == 0` -- + which is the whole point of having one reading: a value that is not negative never displays a + minus. A value that IS negative but rounds to `0.00` at the requested precision still shows + its minus, and (when the field is a gain/loss) still reads `bad`; the two agree, which is what + matters. """ - text = format(value, f",.{places}f") - return text.startswith("-"), text.lstrip("-") + return value < 0 def _trim(text: str) -> str: @@ -232,8 +250,41 @@ def _sign_state(value: Decimal) -> str: return NEUTRAL -def stringify(value: Any) -> str: - """Any open-ended value, as a JSON string. +#: How deep `stringify` will follow a nested structure before giving up. +#: +#: Log fields are shallow by construction -- each one is a `log_event` kwarg -- so six levels is +#: already generous. The cap exists so that the depth of a value parsed from a line keel did not +#: write cannot decide how deep this process recurses: `json.loads` cannot build a cycle, but it +#: can build a thousand nested lists, and a `RecursionError` while rendering a log line would take +#: down the whole response for a row nobody needed. +_MAX_NESTING = 6 + +#: What a value too deep to render shows as. A marker, not the raw structure: this layer's job is +#: to hand the client something it can display, and "there is more here than we will render" is a +#: displayable fact where a truncated Python repr is not. +_TOO_DEEP = "(nested)" + + +def _normalise(value: Any, depth: int = 0) -> Any: + """A JSON-safe mirror of `value` with every SCALAR already rendered as a string. + + Structure is kept (a list stays a list, an object stays an object) so the shape a log line + recorded survives; only the leaves change. Because every leaf is already a string by the time + `json.dumps` sees it, no encoder is needed -- which matters more than it looks: the encoder a + hurried author reaches for is `default=float`, and that single keyword is the whole contract + dying quietly. Rule 6d of `test_console_thinness.py` fails the build on it. + """ + if depth >= _MAX_NESTING: + return _TOO_DEEP + if isinstance(value, Mapping): + return {str(key): _normalise(item, depth + 1) for key, item in value.items()} + if isinstance(value, (list, tuple)): + return [_normalise(item, depth + 1) for item in value] + return stringify(value, _depth=depth) + + +def stringify(value: Any, *, _depth: int = 0) -> str: + """Any open-ended value, as ONE display string. Used for the two places keel's own reports carry values of unconstrained type: `ActivityEvent.fields` (every `log_event` call site invents its own kwargs, and the parser @@ -244,6 +295,20 @@ def stringify(value: Any) -> str: Booleans render as `"true"`/`"false"` rather than Python's `"True"`/`"False"`: the reader is JavaScript. + + **A nested value crosses as JSON text, never as a Python repr.** `ActivityEvent.fields` is + parsed out of the engine's own log lines, so a value can be a list or an object, and the + obvious `str(value)` produced `"{'a': 1, 'b': None}"` -- single quotes, `None`, `True`: text + no client can parse and none should display. Worse, `str([1e+50])` is `"[1e+50]"`, which puts + scientific notation on the wire through a path no money field touches, so none of the Decimal + care taken elsewhere in this file would have caught it. The structure is normalised leaf by + leaf through this same function first, so the numbers inside a nested value are rendered by + exactly the rules the top-level ones are, and only then dumped as JSON. + + Flattened to a string rather than kept as a nested object because of Rule 2: an open-ended + structure handed to the client is a structure the CLIENT has to decide how to format, and + deciding how to format is what this layer exists to do instead. The activity view places one + string per field. """ if value is None: return "" @@ -253,7 +318,16 @@ def stringify(value: Any) -> str: return _plain(value) if value.is_finite() else str(value) if isinstance(value, float): decimalised = _decimalise(value) + # A non-finite float has no decimal form at all; `str` gives "nan"/"inf", which is at + # least a word rather than a number a client would try to read as one. return _plain(decimalised) if decimalised is not None else str(value) + if isinstance(value, (Mapping, list, tuple)): + if _depth >= _MAX_NESTING: + return _TOO_DEEP + # Plain `json.dumps`: `_normalise` has already turned every leaf into a string, so there + # is nothing left for an encoder to convert. `ensure_ascii=False` because the payload is + # UTF-8 and a product id or rule name has no business becoming an escape sequence. + return json.dumps(_normalise(value, _depth), ensure_ascii=False) return str(value) @@ -312,7 +386,7 @@ def money( figure = _decimalise(value) if figure is None: return absent() - negative, magnitude = _magnitude(figure, places) + negative, magnitude = _is_negative(figure), _magnitude(figure, places) if signed and figure != 0: glyph = DOWN if negative else UP sign = MINUS if negative else "+" @@ -336,7 +410,7 @@ def quantity(value: Decimal | None, *, unit: str = "", places: int = 8) -> Field figure = _decimalise(value) if figure is None: return absent() - negative, magnitude = _magnitude(figure, places) + negative, magnitude = _is_negative(figure), _magnitude(figure, places) trimmed = _trim(magnitude) display = f"{MINUS}{trimmed}" if negative else trimmed if unit: @@ -359,7 +433,7 @@ def percent( figure = _decimalise(value) if figure is None: return absent() - negative, magnitude = _magnitude(figure, places) + negative, magnitude = _is_negative(figure), _magnitude(figure, places) if signed and figure != 0: glyph = DOWN if negative else UP sign = MINUS if negative else "+" @@ -391,7 +465,7 @@ def ratio( figure = _decimalise(value) if figure is None: return absent() - negative, magnitude = _magnitude(figure, places) + negative, magnitude = _is_negative(figure), _magnitude(figure, places) if signed and figure != 0: glyph = DOWN if negative else UP sign = MINUS if negative else "+" @@ -832,15 +906,23 @@ def _journal_entry_payload(entry: JournalEntry) -> dict[str, Any]: def journal_payload(report: JournalReport) -> dict[str, Any]: """`build_journal_report`'s `JournalReport`, as JSON. - `total_count` is the full filtered count BEFORE `--limit` truncated `entries`; both cross, so - a client showing "50 of 812" needs no subtraction to know it is looking at a page. + `total_count` is the full filtered count BEFORE `--limit` truncated `entries`, and + `shown_count` is how many survived; both cross, so a client showing "50 of 812" needs no + subtraction to know it is looking at a page. + + `shown_count` is READ from the report, never measured here. It shipped in the first draft as + `count(len(report.entries))`, which is a figure `JournalReport` did not hold -- numerically + harmless, since a list length is exact, but a breach of the rule this module claims to keep, + and one whose guard passed only by coincidence (the fixture happened to set + `total_count == len(entries)`). It is now a property of the report, and Rule 6e of + `test_console_thinness.py` bans `len()` here so the shortcut cannot be taken again. """ return { "as_of": iso(report.now_ts), "generated_at": moment(report.now_ts), "mode": report.mode, "total_count": count(report.total_count), - "shown_count": count(len(report.entries)), + "shown_count": count(report.shown_count), # The query echoed back. Open-ended by shape (`limit` and `since_ts` are ints), so it # crosses as strings -- see `stringify`. "filters": {str(key): stringify(value) for key, value in sorted(report.filters.items())}, diff --git a/tests/commands/test_console_thinness.py b/tests/commands/test_console_thinness.py index e837c0b..d510f8d 100644 --- a/tests/commands/test_console_thinness.py +++ b/tests/commands/test_console_thinness.py @@ -46,15 +46,17 @@ * **Rule 6 -- the serialisation contract** (issue #533), scoped to the module that turns the frozen reports into the browser's JSON (`SERIALISER_STEMS`). That module is the one place in this layer whose output is a machine-readable MONEY contract rather than a screen, so it - carries one extra pin over the four ways the `Decimal`-only guarantee dies at that boundary: + carries one extra pin over the five ways the `Decimal`-only guarantee dies at that boundary: `Decimal.normalize()` (which renders `Decimal("50")` as `Decimal("5E+1")` -- a form that has reached the wire in this codebase before and broken real orders), `float()` on anything but a - timestamp, `round()`, and `json.dumps(..., default=float)`, which is one keyword and looks like - a helpful fix for the `TypeError` a `Decimal` raises. Rules 1-5 already cover the serialiser as - a member of `keel/web/`; this covers what is specific to it. The runtime half of the same - contract -- a recursive walk over the real payload asserting that no JSON number appears - anywhere in it -- lives in `tests/web/test_payload.py`. An AST rule and a walk over the output - fail for different reasons and neither subsumes the other, which is why both exist. + timestamp, `round()`, `json.dumps(..., default=float)` (one keyword, and it looks like a + helpful fix for the `TypeError` a `Decimal` raises), and `len()` -- the quiet way a serialiser + starts producing counts of its own instead of reading ones the report holds. Rules 1-5 already + cover the serialiser as a member of `keel/web/`; this covers what is specific to it. The + runtime half of the same contract -- a recursive walk over the real payload asserting that no + JSON number appears anywhere in it -- lives in `tests/web/test_payload.py`. An AST rule and a + walk over the output fail for different reasons and neither subsumes the other, which is why + both exist. The allowlists are deliberately entry-scoped (module + enclosing function + callee), so an allowance cannot leak to a new call site: the same callee at a different place, or a @@ -536,7 +538,8 @@ def _rule6_findings( # 6d -- json.dumps(..., default=float). `json.dumps` raises a TypeError on a Decimal, # and `default=float` is the one-keyword fix that silently converts every money value in - # the payload to a double. The serialiser returns plain types and dumps nothing itself. + # the payload to a double. A PLAIN `json.dumps` is fine and is used: the serialiser + # normalises every leaf to a string first, so it needs no encoder. if name in ("json.dumps", "json.dump"): for keyword in node.keywords: if keyword.arg in ("default", "cls"): @@ -546,6 +549,21 @@ def _rule6_findings( f"{node.lineno}) -- a custom encoder is how every Decimal in the " "payload becomes a double in one keyword", ) + + # 6e -- len(). A count on the wire must be one the REPORT holds, and `len()` is how a + # serialiser quietly starts producing its own. The first draft of `journal_payload` + # shipped `count(len(report.entries))`; it is numerically exact, which is precisely why + # the runtime "computes nothing" guard could not be relied on to catch it -- that guard + # compares figures, and a list length can coincide with one the report already holds. + # An AST ban does not care about the coincidence. `JournalReport.shown_count` is where + # that figure belongs, and any future count needs the same treatment upstream (or an + # entry-scoped allowance here, in RULE6_FLOAT_ALLOWLIST's shape, with its reasoning). + if name == "len": + found.add( + "rule6_serialisation", + f"{stem}:{owner}: calls len() (line {node.lineno}) -- a count on the wire " + "must be one the report already holds; add it to the report builder", + ) return found @@ -621,12 +639,13 @@ def test_rule_6_is_proven_false_capable() -> None: "import json\n" "def _gmt(ts, fmt):\n" " return float(ts)\n" - "def bad(price, payload):\n" + "def bad(price, payload, rows):\n" " a = price.normalize()\n" " b = float(price)\n" " c = round(price, 2)\n" " d = json.dumps(payload, default=float)\n" - " return a, b, c, d\n" + " e = len(rows)\n" + " return a, b, c, d, e\n" ) tree = ast.parse(snippet) owners = _enclosing_functions(tree) @@ -635,11 +654,12 @@ def test_rule_6_is_proven_false_capable() -> None: "rule6_serialisation", [] ) - assert len(messages) == 4, messages + assert len(messages) == 5, messages assert any(".normalize()" in m for m in messages) assert any("float(price)" in m for m in messages) assert any("round()" in m for m in messages) assert any("default=" in m for m in messages) + assert any("len()" in m for m in messages) assert not any(":_gmt:" in m for m in messages), messages diff --git a/tests/web/test_payload.py b/tests/web/test_payload.py index cb9ac2a..ec6be35 100644 --- a/tests/web/test_payload.py +++ b/tests/web/test_payload.py @@ -215,7 +215,12 @@ def _journal_report(**overrides: Any) -> JournalReport: outcome="dca", ), ], - total_count=2, + # Deliberately NOT `len(entries)`. `total_count` is the pre-`--limit` count, so a real + # report almost always has more of them than it carries entries -- and while these two + # were equal, `test_the_serialiser_computes_nothing...` passed over a `shown_count` the + # serialiser had measured with `len()` rather than read, because `Decimal(2)` happened to + # be on the report already. A guard that passes on a coincidence is not a guard. + total_count=812, filters={ "rule": None, "asset": None, @@ -249,7 +254,15 @@ def _activity_feed(**overrides: Any) -> ActivityFeed: level="INFO", event="cycle_start", cycle_id="c-1", - fields={"products": 1, "budget_usd": Decimal("50.00"), "dry_run": True}, + fields={ + "products": 1, + "budget_usd": Decimal("50.00"), + "dry_run": True, + # Structured values: a log line is parsed JSON, so a field can be a list or + # an object. `str()` would render these as Python reprs. + "symbols": ["BTC-USD", "ETH-USD"], + "limits": {"max": 1e50, "min": None, "on": False}, + }, ), ), events_dropped=3, @@ -407,6 +420,23 @@ def test_scientific_notation_never_reaches_the_wire() -> None: assert payload.money(Decimal("1E-9"), places=9)["display"] == "$0.000000001" +def test_scientific_notation_cannot_reach_the_wire_through_an_open_ended_field_either() -> None: + """The same hazard by the other road. + + The test above only feeds Decimals through NAMED report fields, so it can say nothing about + `ActivityEvent.fields` and `JournalReport.filters` -- the two places a value of unconstrained + type crosses. Those took a `str(value)` fallback in the first draft, and `str(1e+50)` is + `"1e+50"`: an exponent on the wire that every `Decimal` precaution in the module was + irrelevant to, because no `Decimal` was involved. + """ + hazards = [1e50, 1e-30, Decimal("5E+1"), [1e50], {"deep": [{"deeper": 1e-30}]}] + + for value in hazards: + rendered = payload.stringify(value) + assert "e+" not in rendered and "e-" not in rendered, (value, rendered) + assert "E+" not in rendered and "E-" not in rendered, (value, rendered) + + # -- Rule 2: the serialiser computes nothing ------------------------------------------------------ @@ -443,6 +473,19 @@ def _report_figures(node: Any, seen: set[int] | None = None) -> set[Decimal]: out = set() for name in fields: out |= _report_figures(getattr(node, name), seen) + # PROPERTIES count as figures the report returns, exactly as fields do. A frozen report + # here derives some of its readings from its own data rather than storing them -- + # `ActivityCycle.is_quiet`/`.key` and `JournalReport.shown_count` are the established + # pattern -- and a stored duplicate of a derived reading is state that can drift out of + # agreement with what it describes. What matters for this guard is that the REPORT vouches + # for the figure, not which side of a `@property` it was written on. + for name, attribute in vars(type(node)).items(): + if not isinstance(attribute, property): + continue + try: + out |= _report_figures(getattr(node, name), seen) + except Exception: # pragma: no cover - a property that raises is not a figure + continue return out return set() @@ -547,6 +590,75 @@ def test_state_survives_without_colour() -> None: assert entries[1]["pnl"]["display"] == "▲ +$41.00" +def test_the_displayed_sign_and_the_state_never_disagree() -> None: + """Rule 3's premise, checked rather than assumed: if `display` shows a minus, `state` must not + be calling the same value neutral. + + It failed on negative zero. `format(Decimal("-0.00"), ",.2f")` is `"-0.00"`, so a sign read + off the formatted TEXT said negative while `state`, read off the exact value, said neutral -- + `Decimal("-0.00") == 0`. A client styling by `state` and showing `display` printed a minus + sign in a neutral colour. There is now one reading of the sign, `_is_negative`, feeding both. + + A value that is genuinely negative but rounds to zero at the display precision keeps its + minus, and keeps `bad` alongside it. That is not a disagreement: it is a small loss, honestly + labelled and honestly styled. + """ + cases = [ + Decimal("-0.00"), + Decimal("0.00"), + Decimal("-0.001"), + Decimal("-12.34"), + Decimal("41.00"), + Decimal("0E-8"), + ] + + for value in cases: + field = payload.money(value, signed=True) + shows_negative = payload.MINUS in field["display"] or payload.DOWN in field["display"] + assert shows_negative == (field["state"] == "bad"), (value, field) + + unsigned = payload.money(value) + assert (payload.MINUS in unsigned["display"]) == (value < 0), (value, unsigned) + + +def test_negative_zero_is_not_negative_anywhere_it_is_rendered() -> None: + """`Decimal("-0.00")` is zero, and every builder that renders a sign must agree.""" + assert payload.money(Decimal("-0.00"))["display"] == "$0.00" + assert payload.money(Decimal("-0.00"), signed=True)["state"] == "neutral" + assert payload.percent(Decimal("-0.00"))["display"] == "0.00%" + assert payload.ratio(Decimal("-0.00"))["display"] == "0.00" + assert payload.quantity(Decimal("-0.00"))["display"] == "0" + # ...and the exact value still crosses untouched: nothing was normalised away to achieve it. + assert payload.money(Decimal("-0.00"))["value"] == "-0.00" + + +def test_a_loss_too_small_to_show_keeps_its_sign_and_its_judgement() -> None: + """The other side of the same rule. `-0.001` at two places renders `0.00`, and suppressing the + minus there would report a loss as a break-even. It shows, and `state` agrees.""" + field = payload.money(Decimal("-0.001"), signed=True) + + assert field["display"] == "▼ −$0.00" + assert field["state"] == "bad" + assert field["value"] == "-0.001" + + +def test_shown_count_is_read_from_the_report_not_measured_here() -> None: + """The count of rows a journal page carries is `JournalReport.shown_count`, a reading the + report makes about itself -- not `len(entries)` measured by the serialiser. + + The distinction is invisible in the number (a list length is exact) and that is exactly the + problem: the runtime "computes nothing" guard compares FIGURES, so it cannot tell a measured + length from a read one when they coincide. Rule 6e of `test_console_thinness.py` bans `len()` + in the serialiser for that reason, and this pins the pair a client needs to say "2 of 812" + without subtracting anything. + """ + built = payload.journal_payload(_journal_report()) + + assert built["total_count"]["value"] == "812" + assert built["total_count"]["display"] == "812" + assert built["shown_count"]["value"] == "2" + + def test_every_field_carries_all_three_keys_always() -> None: """Schema uniformity is itself part of the contract: a client that has to test whether `state` is present is branching on payload SHAPE, which is inference by another route. Every field @@ -684,7 +796,49 @@ def test_the_activity_feed_stringifies_the_open_ended_event_fields() -> None: known field: they cross as strings.""" event = payload.activity_payload(_activity_feed())["cycles"][0]["events"][0] - assert event["fields"] == {"products": "1", "budget_usd": "50.00", "dry_run": "true"} + assert event["fields"]["products"] == "1" + assert event["fields"]["budget_usd"] == "50.00" + assert event["fields"]["dry_run"] == "true" + + +def test_a_nested_log_field_crosses_as_json_never_as_a_python_repr() -> None: + """A log line is parsed JSON, so a field's value can be a list or an object. + + The first draft fell through to `str(value)` for those, which produced `"{'a': 1, 'b': None}"` + -- single quotes, `None`, `True`. No JSON number leaks out of that (it is one string), so the + recursive number-walker stayed green while the payload carried text no client can parse and + none should display. Rule 2 says values arrive presentation-ready, and a Python repr is not. + + The numbers INSIDE a nested value go through the same rendering as a top-level one, which is + the half that actually bites: `str([1e+50])` is `"[1e+50]"` -- scientific notation reaching + the wire through a path no money field touches, so every `Decimal` precaution in the module + would have been irrelevant to it. + """ + fields = payload.activity_payload(_activity_feed())["cycles"][0]["events"][0]["fields"] + + assert fields["symbols"] == '["BTC-USD", "ETH-USD"]' + assert fields["limits"] == ( + '{"max": "100000000000000000000000000000000000000000000000000",' + ' "min": "", "on": "false"}' + ) + assert "1e+50" not in fields["limits"] + assert "'" not in fields["limits"] and "None" not in fields["limits"] + + +def test_a_pathologically_nested_log_field_cannot_recurse_the_response_to_death() -> None: + """Depth in a log line is attacker-adjacent input: `json.loads` cannot build a cycle, but it + can build a thousand nested lists, and a `RecursionError` while rendering one field would take + down a whole response for a row nobody needed. Log fields are shallow by construction -- each + is a `log_event` kwarg -- so the cap costs nothing real and the depth of the input never + decides how deep this process goes.""" + deep: Any = "bottom" + for _ in range(200): + deep = [deep] + + rendered = payload.stringify(deep) + + assert "(nested)" in rendered + assert "bottom" not in rendered def test_the_journal_filters_cross_as_strings_too() -> None: