From cc0c6dfaf9dfd461d6e7b708756a9854fceedfc2 Mon Sep 17 00:00:00 2001 From: Elmehdi Aitbrahim Date: Thu, 3 Sep 2026 13:21:31 -0400 Subject: [PATCH 1/2] feat(compliance): the asymmetric lower-bound liquidity gate (#696, part 2 of 2) Part 1 recorded WHICH feed a series came from. This is what reads it. THE GATE. Venue volume is a LOWER BOUND on consolidated volume, never an upper one, so the two directions are not equally informative and the gate treats them differently: * at or above the floor, on ANY feed -> CONCLUSIVE PASS. A name that traded that much on one venue alone necessarily traded at least that much in total. A gate that refused here would ban MSFT and AAPL for thinness they do not have, and would turn a data-vendor pricing tier into a prerequisite for running the engine at all. * below the floor, on a partial feed -> NOT A VERDICT. Equally consistent with a thin asset and with a liquid one that barely trades HERE. Refused as `liquidity_unmeasured`, naming the feed and both dollar figures, and saying consolidated volume is UNMEASURED rather than low. A test asserts the line never contains "illiquid" or "too thin": the refusal must not assert the half it cannot see. * below the floor, on a consolidated feed -> unchanged. The crypto path is untouched, because Coinbase's own volume IS the scale the floor was calibrated against. * below the floor, scope UNRECORDED -> unchanged verdict, annotated. Every series cached before v17 is unrecorded, and treating unrecorded as partial would refuse the entire existing universe over metadata nobody wrote down. It self-heals as series are re-fetched. `market_facts` reads ONE_DAY provenance specifically -- that is the granularity the statistic medians, so an hourly series on another feed says nothing about it. `doctor` gains `data.feed_scope`, WARN and never FAIL: a free single-venue tier is a legitimate configuration, and the bound admits its liquid names honestly. What is not legitimate is not knowing, and the screen only speaks for a candidate somebody is actively screening while `slippage_for_quote_volume` prices off the same number every cycle in silence. Partial and unrecorded series are reported SEPARATELY: one should be re-fetched under a different feed, the other may already be consolidated. The cost-fidelity record is amended rather than rewritten (`docs/experiments` is append-only). The amendment retracts two claims that were asserted rather than measured -- "roughly 2% ... approximately IEX's share" (IEX publishes ~3.8% for Q2 2026, so the figure was simply wrong) and the "~50x" understatement -- notes that no number in the document depended on either, and records that what shipped is better than what that section proposed. Tests (written first, red before green): 27 across three files. tests/compliance/test_liquidity_lower_bound.py -- 11 tests/compliance/test_market_facts_carries_feed_scope.py -- 6 tests/commands/test_doctor_feed_scope.py -- 8 (+2 existing) Two EXISTING reachability guards -- one in tests/compliance, one in tests/test_proposer -- correctly failed: they assert every `DATA_DERIVED_FAILURES` tag really appears in `screen_asset` output, and `liquidity` / `liquidity_unmeasured` are mutually exclusive by construction, so no single set of facts emits both. Both now screen the partial-feed arm too, rather than the tag being dropped as unreachable. Mutation-verified, 7 mutants. One SURVIVED first time: gutting the fix line's explanatory half left `keel fetch` intact, and the assertion only looked for "keel". Re-fetching under the SAME feed changes nothing, so the pin now requires the word "consolidated" -- an operator who re-fetches identically gets the same warning and concludes the report is broken. Closes #696 Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01NzuKAe2RVrPt9acVAWjRyL --- .../2026-09-02-equities-cost-fidelity.md | 27 +++ keel/commands/assets.py | 7 + keel/commands/doctor.py | 88 +++++++++ keel/compliance/screen.py | 36 +++- tests/commands/test_doctor.py | 1 + tests/commands/test_doctor_feed_scope.py | 83 ++++++++ tests/compliance/test_assets_cli.py | 7 + .../compliance/test_liquidity_lower_bound.py | 183 ++++++++++++++++++ .../test_market_facts_carries_feed_scope.py | 84 ++++++++ tests/test_proposer.py | 6 + 10 files changed, 519 insertions(+), 3 deletions(-) create mode 100644 tests/commands/test_doctor_feed_scope.py create mode 100644 tests/compliance/test_liquidity_lower_bound.py create mode 100644 tests/compliance/test_market_facts_carries_feed_scope.py diff --git a/docs/experiments/2026-09-02-equities-cost-fidelity.md b/docs/experiments/2026-09-02-equities-cost-fidelity.md index 5d483752..a11c3464 100644 --- a/docs/experiments/2026-09-02-equities-cost-fidelity.md +++ b/docs/experiments/2026-09-02-equities-cost-fidelity.md @@ -132,6 +132,33 @@ the fee question it was filed beside. Two consequences worth separating: closed, so it is safe — but safe for the wrong reason, and it will misinform any equities universe decision. Filed as **#696**; nothing in this document changes it. +> **Amendment (2026-09-03), per #696.** Records are appended to, never rewritten, so nothing +> above has been altered — but the framing of this finding was sharper than the evidence, and +> the resolution improved on it. +> +> Two claims here were asserted rather than measured: that the cached figure is "roughly 2% … +> approximately IEX's share of US equity volume", and that the understatement is "~50×". IEX +> publishes its own overall share as roughly 3.8% for Q2 2026, so the 2% was simply wrong, and +> neither figure was derived from anything in the tables above. **No number in this document +> depends on either**; the finding is that the statistic is structurally unable to answer the +> question asked of it, which needs no percentage at all. +> +> What shipped is better than what this section proposed. The fix is not a fail-closed refusal +> of every partial-feed series — that would have made a data-vendor pricing tier a prerequisite +> for running the engine, and would have banned MSFT and AAPL for thinness they do not have. +> It is an **asymmetric lower-bound gate**: venue volume is a lower bound on consolidated +> volume, so at or above the admission floor a partial feed is CONCLUSIVE, while below it the +> screen refuses as `liquidity_unmeasured` rather than asserting an asset is thin. The bound +> holds for any venue share below 100%, so no percentage is encoded anywhere in the code — +> `keel/data/feed_scope.py` carries the argument, and a test greps its body to keep a market +> share from ever being multiplied into a volume statistic. +> +> Provenance is now recorded per series at fetch time (`candle_series_feed`, schema v17), so +> the feed is no longer inferred from whatever config is loaded when someone reads the series, +> and `doctor`'s `data.feed_scope` reports which cached series carry a bound rather than a +> measurement. Series cached before v17 read as *unrecorded* — deliberately distinct from +> *partial*, since one should be re-fetched and the other may already be consolidated. + ## Finding 3 — the crypto model is validated, in passing 332.27bp modelled against 306.31bp measured, conservative by 8%, using an estimator with no diff --git a/keel/commands/assets.py b/keel/commands/assets.py index ad86e37e..16855546 100644 --- a/keel/commands/assets.py +++ b/keel/commands/assets.py @@ -48,6 +48,7 @@ discover_candidates, ) from keel.config import Config +from keel.data.feed_scope import reports_consolidated_volume from keel.data.repository import Repository from keel.types import Granularity @@ -152,6 +153,7 @@ def market_facts(repo: Repository, product: str, quote: str) -> MarketFacts: """Everything the screen can compute for itself from data we already hold.""" asset = product.split("-")[0] candles = repo.get_candles(product, Granularity.ONE_DAY) + feeds = repo.get_series_feeds(product, Granularity.ONE_DAY) return MarketFacts( asset=asset, daily_bars=len(candles), @@ -169,6 +171,11 @@ def market_facts(repo: Repository, product: str, quote: str) -> MarketFacts: product_id=product, # The other half of the key the instrument statement is recorded under. See `VENUE`. venue=VENUE, + # WHICH FEED the median above was computed from (#696). Read for ONE_DAY specifically, + # because that is the granularity the statistic uses -- an hourly series fetched under a + # different feed says nothing about this number. + volume_feed=", ".join(feeds) or None, + volume_feed_is_consolidated=reports_consolidated_volume(feeds), ) diff --git a/keel/commands/doctor.py b/keel/commands/doctor.py index 45ec5691..cf01dfa5 100644 --- a/keel/commands/doctor.py +++ b/keel/commands/doctor.py @@ -40,6 +40,7 @@ from keel_core.telemetry import current_venue from keel_core.trade_scope import READ_ONLY, TRADING, TradeScopeState, VenueTradeScope +from keel.data.feed_scope import reports_consolidated_volume from keel.data.freshness import Freshness from keel.execution import sizing from keel.types import Granularity @@ -819,6 +820,81 @@ def balance_drift_findings(records: dict[str, Any]) -> list[Finding]: ] +def feed_scope_findings(series_feeds: dict[tuple[str, str], tuple[str, ...]]) -> list[Finding]: + """Cached series whose volume statistic is a LOWER BOUND rather than a measurement (#696). + + `median_daily_quote_volume` is read by the admission floor, by + `slippage_for_quote_volume`, and by the asset scout's discovery probe. On a feed that reports + one venue's own executions it is a bound on the market, not the market -- the cost-fidelity + run measured MSFT cached at $186M/day and priced it as thinner than the model's reference + liquidity. + + `screen_asset` already refuses honestly at screen time. This exists because that only fires + for a candidate somebody is actively screening, while the slippage model prices off the same + number on EVERY cycle and says nothing. The bound is invisible unless something reports it. + + WARN, never FAIL. A single-venue feed is a legitimate configuration -- it is the free tier, + and the asymmetric bound admits the liquid names from it honestly (volume at or above the + floor on one venue proves the floor is cleared). What is not legitimate is not knowing. + + UNRECORDED series are reported SEPARATELY from partial ones, because they are different + facts: a partial series should be re-fetched under a consolidated feed, an unrecorded one may + already BE consolidated and simply predates the provenance table. Telling an operator to + re-fetch the second would be advice based on the absence of evidence. + """ + partial: list[tuple[str, str, str]] = [] + unrecorded: list[str] = [] + for (product, granularity), feeds in sorted(series_feeds.items()): + if not feeds: + unrecorded.append(f"{product} {granularity}") + elif reports_consolidated_volume(feeds) is False: + partial.append((product, granularity, ", ".join(feeds))) + + if not partial and not unrecorded: + total = len(series_feeds) + return [ + Finding( + "data.feed_scope", + OK, + "every series' volume is consolidated" + if total + else "no cached series to judge", + f"{total} series carry a recorded, consolidated feed -- the liquidity statistic " + "measures the market rather than bounding it" + if total + else "nothing cached yet", + "-", + ) + ] + + if partial: + described = ", ".join(f"{p} {g} ({feeds})" for p, g, feeds in partial) + return [ + Finding( + "data.feed_scope", + WARN, + f"{len(partial)} series' volume is a LOWER BOUND, not a measurement", + f"{described} -- these feeds report one venue's own executions, so " + "median daily volume is a lower bound on consolidated volume. Above the " + "admission floor that is conclusive; below it, nothing is established", + "re-fetch under a consolidated feed to decide a below-floor series: " + "`keel fetch --refresh`", + ) + ] + + return [ + Finding( + "data.feed_scope", + WARN, + f"{len(unrecorded)} series predate feed provenance", + f"{', '.join(unrecorded)} -- feed provenance UNRECORDED, so whether this " + "volume measures the market or bounds it is unknown -- not the same as " + "knowing it is partial", + "re-fetch to stamp provenance: `keel fetch`", + ) + ] + + def orphan_bracket_findings(records: dict[str, Any]) -> list[Finding]: """Resting SELLs the orphan sweep cancelled because the account no longer held them (#668). @@ -1221,6 +1297,18 @@ def gather_findings(repo: Any, config: Any, log_lines: Iterable[str], now_ts: in market_closed = agent.recorded_market_closed(repo, config, now_ts) start_ts = now_ts - DOCTOR_WINDOW_SEC + # #696. The same (product, granularity) set the health report covers -- the series whose + # volume the slippage model and the admission floor actually read. + findings += feed_scope_findings( + { + (product, Granularity(granularity).value): repo.get_series_feeds( + product, granularity + ) + for product in products + for granularity in granularities + } + ) + findings += data_health_findings( [ SeriesHealth( diff --git a/keel/compliance/screen.py b/keel/compliance/screen.py index 7b55131b..3f6392e6 100644 --- a/keel/compliance/screen.py +++ b/keel/compliance/screen.py @@ -78,7 +78,7 @@ #: callers import it (or, better, call `split_failures`/`missing_history_lines` below rather than #: reimplementing the split) so a tag rename here cannot silently disable the suppression #: elsewhere. -DATA_DERIVED_FAILURES = frozenset({"history", "liquidity"}) +DATA_DERIVED_FAILURES = frozenset({"history", "liquidity", "liquidity_unmeasured"}) @dataclass(frozen=True) @@ -155,6 +155,17 @@ class MarketFacts: #: silently inherit "Coinbase, therefore spot" -- the fail-OPEN answer to the one question #: this field was added to ask. A construction site that forgets it must fail at the call. venue: str + #: Which data feed the cached candles came from (`candle_series_feed`, #696), or `None` when + #: unrecorded -- every series cached before that table existed. + volume_feed: str | None = None + #: Whether that feed reports CONSOLIDATED market volume. `True` = the statistic measures the + #: market; `False` = it is a LOWER BOUND on it; `None` = unrecorded, which is the absence of + #: evidence and NOT the same as either. + #: + #: Defaults are safe here, unlike `product_id`'s: a caller that forgets these gets `None`, + #: which preserves the pre-#696 verdict exactly rather than silently admitting or refusing + #: anything new. + volume_feed_is_consolidated: bool | None = None @dataclass(frozen=True) @@ -233,10 +244,29 @@ def screen_asset( f"history: {facts.daily_bars} daily bars < {policy.min_daily_bars} required " "(a rule cannot be validated on a series shorter than its evidence needs)" ) - if facts.median_daily_volume < policy.min_median_daily_volume: + if facts.median_daily_volume >= policy.min_median_daily_volume: + # CONCLUSIVE, on any feed. Venue volume is a LOWER BOUND on consolidated volume, so a + # name that traded this much on one venue alone necessarily traded at least this much in + # total (#696). Refusing here would ban the most liquid securities in the world for + # thinness they do not have. + pass + elif facts.volume_feed_is_consolidated is False: + # NOT a verdict. Below the floor on a single-venue feed is equally consistent with a thin + # asset and with a liquid one that barely trades HERE, and the gate has measured neither. + # Saying "illiquid" would assert the half it cannot see. + failures.append( + f"liquidity_unmeasured: {facts.volume_feed} reports one venue's own executions, and " + f"its median daily volume ${facts.median_daily_volume:,} is below the " + f"${policy.min_median_daily_volume:,} floor -- so CONSOLIDATED volume is UNMEASURED, " + "not low. Re-fetch this series under a consolidated feed to decide it" + ) + else: + scope = ( + "" if facts.volume_feed_is_consolidated else " (feed scope unrecorded for this series)" + ) failures.append( f"liquidity: median daily volume {facts.median_daily_volume} < " - f"{policy.min_median_daily_volume} required" + f"{policy.min_median_daily_volume} required{scope}" ) if policy.require_settlement_quote and not facts.quotable_in_settlement_currency: failures.append( diff --git a/tests/commands/test_doctor.py b/tests/commands/test_doctor.py index 7bb49e96..a440c368 100644 --- a/tests/commands/test_doctor.py +++ b/tests/commands/test_doctor.py @@ -594,6 +594,7 @@ def test_gather_findings_covers_every_check_over_a_seeded_db(tmp_path, valid_con "data.missing", "data.stale", "data.gaps", + "data.feed_scope", "sizing.admissible", } # an unattested, empty deployment fails the run, exactly as the command does diff --git a/tests/commands/test_doctor_feed_scope.py b/tests/commands/test_doctor_feed_scope.py new file mode 100644 index 00000000..9bf94699 --- /dev/null +++ b/tests/commands/test_doctor_feed_scope.py @@ -0,0 +1,83 @@ +"""`doctor` must say when a liquidity number is a bound rather than a measurement -- #696. + +The gate refuses honestly now, but only at screen time and only for a candidate someone is +actively screening. An operator running the equities profile day to day never sees it, while +`slippage_for_quote_volume` quietly prices MSFT off a $186M/day statistic on every cycle. This is +the standing report of that: which cached series carry a volume figure that is a LOWER BOUND. + +WARN, never FAIL. Running on a single-venue feed is a legitimate configuration -- it is the free +tier, and the asymmetric bound admits the liquid names from it honestly. What is not legitimate is +not knowing. +""" + +from __future__ import annotations + +from keel.commands.doctor import FAIL, OK, WARN, feed_scope_findings + + +def test_no_series_at_all_is_ok() -> None: + """A fresh deployment has nothing to report on, and that is not a warning.""" + (finding,) = feed_scope_findings({}) + assert finding.status == OK + + +def test_consolidated_series_only_is_ok() -> None: + (finding,) = feed_scope_findings({("BTC-USD", "ONE_DAY"): ("coinbase",)}) + assert finding.status == OK + assert "coinbase" in finding.detail or "consolidated" in finding.detail.lower() + + +def test_a_partial_feed_series_warns_and_names_it() -> None: + (finding,) = feed_scope_findings( + { + ("MSFT-USD", "ONE_DAY"): ("alpaca:iex",), + ("BTC-USD", "ONE_DAY"): ("coinbase",), + } + ) + assert finding.status == WARN + assert "MSFT-USD" in finding.detail + assert "alpaca:iex" in finding.detail + assert "BTC-USD" not in finding.detail, "a consolidated series is not a finding" + + +def test_the_warning_says_the_number_is_a_bound_not_that_the_asset_is_thin() -> None: + (finding,) = feed_scope_findings({("MSFT-USD", "ONE_DAY"): ("alpaca:iex",)}) + text = f"{finding.headline} {finding.detail}".lower() + assert "lower bound" in text + for asserted in ("illiquid", "too thin"): + assert asserted not in text + + +def test_it_never_fails_only_warns() -> None: + """A free data tier is a legitimate configuration. FAIL would make it a fault.""" + findings = feed_scope_findings({("MSFT-USD", "ONE_DAY"): ("alpaca:iex",)}) + assert all(f.status != FAIL for f in findings) + + +def test_an_unrecorded_series_is_reported_separately_from_a_partial_one() -> None: + """Unrecorded is the absence of evidence, partial is a known limitation. Reporting them the + same way would tell an operator to re-fetch a series that may already be consolidated.""" + (finding,) = feed_scope_findings({("ETH-USD", "ONE_DAY"): ()}) + assert finding.status == WARN + assert "unrecorded" in finding.detail.lower() + assert "lower bound" not in finding.detail.lower() + + +def test_a_mixed_series_is_reported_as_partial() -> None: + (finding,) = feed_scope_findings( + {("MSFT-USD", "ONE_DAY"): ("alpaca:iex", "alpaca:sip")} + ) + assert finding.status == WARN + assert "alpaca:iex" in finding.detail + + +def test_the_fix_line_says_the_re_fetch_must_be_under_a_different_feed() -> None: + """Naming `keel fetch` is not enough -- re-fetching under the SAME feed changes nothing, and + an operator who does it will get the identical warning and conclude the report is broken. + The fix has to say *consolidated*. + + A surviving mutant put this here: replacing the explanatory half with "ask someone" left the + command intact, so an assertion that merely looked for "keel" stayed green.""" + (finding,) = feed_scope_findings({("MSFT-USD", "ONE_DAY"): ("alpaca:iex",)}) + assert "keel fetch" in finding.fix + assert "consolidated" in finding.fix.lower() diff --git a/tests/compliance/test_assets_cli.py b/tests/compliance/test_assets_cli.py index 91245cd6..482f0511 100644 --- a/tests/compliance/test_assets_cli.py +++ b/tests/compliance/test_assets_cli.py @@ -3,6 +3,7 @@ from __future__ import annotations import json +from dataclasses import replace from decimal import Decimal from pathlib import Path @@ -1085,6 +1086,12 @@ def test_the_derived_failure_tags_actually_match_screen_asset_output(): venue="coinbase", ) tags = {f.split(":")[0] for f in screen_mod.screen_asset(facts, None).failures} + # `liquidity` and `liquidity_unmeasured` are mutually exclusive by construction (#696): which + # one a below-floor series produces depends on whether its feed sees the whole market, so no + # single set of facts can emit both. The partial-feed arm is screened here too, or the tag + # would look unreachable and the suppression would be dropped as dead. + partial = replace(facts, volume_feed="alpaca:iex", volume_feed_is_consolidated=False) + tags |= {f.split(":")[0] for f in screen_mod.screen_asset(partial, None).failures} missing = screen_mod.DATA_DERIVED_FAILURES - tags assert not missing, ( diff --git a/tests/compliance/test_liquidity_lower_bound.py b/tests/compliance/test_liquidity_lower_bound.py new file mode 100644 index 00000000..23278702 --- /dev/null +++ b/tests/compliance/test_liquidity_lower_bound.py @@ -0,0 +1,183 @@ +"""Volume from a single-venue feed is a LOWER BOUND, and the two directions differ -- #696. + +keel's liquidity floor was calibrated against venue-reported volume. On a crypto exchange that +is coherent: Coinbase's own volume is the scale the floor was chosen against. On Alpaca's IEX +feed it is not -- IEX reports one US equity exchange's own executions, and the cost-fidelity run +measured MSFT, one of the most liquid securities in the world, cached at $186M/day and priced as +a thin asset. + +THE ASYMMETRY IS THE WHOLE GATE, and getting it backwards fails in both directions at once: + +* at or above the floor on a partial feed -> **CONCLUSIVE PASS.** If a name traded that much on + one venue alone it necessarily traded at least that much in total. A gate that refused here + would ban MSFT and AAPL for thinness they do not have, and would turn a data-vendor pricing + tier into a hard prerequisite for running the engine. +* below the floor on a partial feed -> **NOT A VERDICT.** It is consistent with a genuinely thin + asset and with a liquid one that barely trades on this venue. Reporting it as "illiquid" would + be asserting something unmeasured; the refusal has to say so. + +`test_a_partial_feed_above_the_floor_is_admitted` and +`test_a_partial_feed_below_the_floor_refuses_without_calling_it_thin` are the two that carry it. +""" + +from __future__ import annotations + +from decimal import Decimal + +from keel.compliance.screen import ( + DATA_DERIVED_FAILURES, + WAIVABLE_CRITERIA, + AssetAttestation, + InstrumentAttestation, + MarketFacts, + ScreenPolicy, + screen_asset, +) + +_VENUE = "alpaca" +_FLOOR = Decimal("1000000") + + +def _facts(volume: str, scope: bool | None) -> MarketFacts: + return MarketFacts( + asset="MSFT", + daily_bars=2000, + median_daily_volume=Decimal(volume), + quotable_in_settlement_currency=True, + product_id="MSFT-USD", + venue=_VENUE, + volume_feed="alpaca:iex" if scope is False else ("alpaca:sip" if scope else None), + volume_feed_is_consolidated=scope, + ) + + +def _clean_attestations() -> tuple[AssetAttestation, InstrumentAttestation]: + return ( + AssetAttestation( + asset="MSFT", + sector="technology", + backing="equity", + pays_yield=False, + source="operator", + attested_by="tester", + attested_at=1_784_505_600, + ), + InstrumentAttestation( + venue=_VENUE, + product_id="MSFT-USD", + wrapper="spot", + source="venue product spec", + attested_by="tester", + attested_at=1_784_505_600, + ), + ) + + +def _screen(volume: str, scope: bool | None): + asset, instrument = _clean_attestations() + return screen_asset( + _facts(volume, scope), + asset, + policy=ScreenPolicy(min_median_daily_volume=_FLOOR, min_daily_bars=1), + instrument=instrument, + ) + + +def _tags(result) -> set[str]: + return {failure.split(":", 1)[0] for failure in result.failures} + + +# --- the conclusive direction ---------------------------------------------------------------- + + +def test_a_partial_feed_above_the_floor_is_admitted() -> None: + """The bound is one-sided. Clearing the floor on ONE venue proves the floor is cleared.""" + result = _screen("5000000", scope=False) + assert "liquidity" not in _tags(result) + assert "liquidity_unmeasured" not in _tags(result) + + +def test_a_consolidated_feed_above_the_floor_is_admitted() -> None: + assert "liquidity" not in _tags(_screen("5000000", scope=True)) + + +def test_exactly_at_the_floor_is_admitted_on_a_partial_feed() -> None: + """The boundary is `>= floor`, same as the consolidated path -- a partial feed must not + acquire a stricter threshold by accident.""" + assert "liquidity" not in _tags(_screen(str(_FLOOR), scope=False)) + assert "liquidity_unmeasured" not in _tags(_screen(str(_FLOOR), scope=False)) + + +# --- the inconclusive direction --------------------------------------------------------------- + + +def test_a_partial_feed_below_the_floor_refuses_without_calling_it_thin() -> None: + """The refusal must not assert illiquidity it has not measured.""" + result = _screen("500000", scope=False) + assert "liquidity_unmeasured" in _tags(result) + assert "liquidity" not in _tags(result) + line = next(f for f in result.failures if f.startswith("liquidity_unmeasured")) + assert "alpaca:iex" in line + assert "unmeasured" in line.lower() + for asserted in ("illiquid", "too thin", "insufficient liquidity"): + assert asserted not in line.lower(), f"the refusal asserts {asserted!r}" + + +def test_the_refusal_names_both_numbers_in_dollars() -> None: + """An operator has to be able to see how far short of the floor this venue's slice fell -- + $500,000 against $1,000,000 is a different conversation from $50 against $1,000,000.""" + line = next( + f for f in _screen("500000", scope=False).failures if f.startswith("liquidity_unmeasured") + ) + assert "500000" in line.replace(",", "") + assert "1000000" in line.replace(",", "") + assert "$" in line + + +def test_the_refusal_says_what_would_resolve_it() -> None: + line = next( + f for f in _screen("500000", scope=False).failures if f.startswith("liquidity_unmeasured") + ) + assert "consolidated" in line.lower() + + +# --- the consolidated and unrecorded paths ---------------------------------------------------- + + +def test_a_consolidated_feed_below_the_floor_is_still_a_plain_liquidity_failure() -> None: + """On a feed that sees the whole market, below the floor IS a measurement. Nothing about + the crypto path changes.""" + result = _screen("500000", scope=True) + assert "liquidity" in _tags(result) + assert "liquidity_unmeasured" not in _tags(result) + + +def test_an_unrecorded_scope_keeps_the_existing_verdict() -> None: + """Every series cached before #696 has unrecorded provenance. Treating unrecorded as partial + would refuse the entire existing crypto universe on a technicality about metadata nobody + wrote down -- so the verdict is unchanged, and the line says the scope is unrecorded.""" + result = _screen("500000", scope=None) + assert "liquidity" in _tags(result) + assert "liquidity_unmeasured" not in _tags(result) + line = next(f for f in result.failures if f.startswith("liquidity")) + assert "unrecorded" in line.lower() + + +def test_an_unrecorded_scope_above_the_floor_is_admitted() -> None: + assert "liquidity" not in _tags(_screen("5000000", scope=None)) + + +# --- how the new tag behaves in the rest of the module ---------------------------------------- + + +def test_the_new_tag_is_data_derived() -> None: + """It reports on OUR cache and OUR feed, never on the asset -- the same reason `liquidity` + and `history` are suppressed when there are no bars at all.""" + assert "liquidity_unmeasured" in DATA_DERIVED_FAILURES + + +def test_the_new_tag_is_not_waivable_yet() -> None: + """A human volume attestation is a reasonable future route and is deliberately NOT here. + The module says expanding this set is a decision, not a default, and no attestation record + exists for volume today -- a waiver with nothing to record would be a rubber stamp.""" + assert "liquidity_unmeasured" not in WAIVABLE_CRITERIA diff --git a/tests/compliance/test_market_facts_carries_feed_scope.py b/tests/compliance/test_market_facts_carries_feed_scope.py new file mode 100644 index 00000000..5e9d0297 --- /dev/null +++ b/tests/compliance/test_market_facts_carries_feed_scope.py @@ -0,0 +1,84 @@ +"""`market_facts` must read the provenance it has, or the gate is decorative -- #696. + +`screen_asset` decides between a plain liquidity failure and `liquidity_unmeasured` from +`MarketFacts.volume_feed_is_consolidated`. Both fields default to `None` so a caller that forgets +them preserves the pre-#696 verdict rather than admitting or refusing anything new -- which makes +forgetting them SILENT. This is the pin that says the one real caller does not. + +The same shape of miss already happened once on this issue: the provenance table landed with only +`keel fetch` writing to it, so the live path recorded nothing and the feature was nearly inert. +""" + +from __future__ import annotations + +from decimal import Decimal + +import pytest +from keel_core.types import Candle, Granularity + +from keel.commands.assets import market_facts +from keel.data.db import connect, migrate +from keel.data.repository import Repository + + +@pytest.fixture() +def repo() -> Repository: + conn = connect(":memory:") + migrate(conn) + return Repository(conn) + + +def _candles(n: int = 5) -> list[Candle]: + return [ + Candle( + ts=i * 86400, + open=Decimal("100"), + high=Decimal("101"), + low=Decimal("99"), + close=Decimal("100"), + volume=Decimal("1000"), + ) + for i in range(n) + ] + + +def test_a_partial_feed_is_carried_into_the_facts(repo: Repository) -> None: + repo.upsert_candles("MSFT-USD", Granularity.ONE_DAY, _candles(), feed="alpaca:iex") + facts = market_facts(repo, "MSFT-USD", "USD") + assert facts.volume_feed == "alpaca:iex" + assert facts.volume_feed_is_consolidated is False + + +def test_a_consolidated_feed_is_carried_into_the_facts(repo: Repository) -> None: + repo.upsert_candles("BTC-USD", Granularity.ONE_DAY, _candles(), feed="coinbase") + facts = market_facts(repo, "BTC-USD", "USD") + assert facts.volume_feed == "coinbase" + assert facts.volume_feed_is_consolidated is True + + +def test_an_unrecorded_series_carries_none(repo: Repository) -> None: + """Not `False`. A legacy series' scope is unknown, and the gate treats unknown as + "verdict unchanged" rather than as a known limitation.""" + repo.upsert_candles("ETH-USD", Granularity.ONE_DAY, _candles()) + facts = market_facts(repo, "ETH-USD", "USD") + assert facts.volume_feed is None + assert facts.volume_feed_is_consolidated is None + + +def test_a_mixed_series_names_both_feeds_and_reads_partial(repo: Repository) -> None: + """A median is not decomposable by source, so a series carrying any narrow-feed bars is a + lower bound overall -- and the operator needs to see WHICH feeds, to know what to re-fetch.""" + repo.upsert_candles("MSFT-USD", Granularity.ONE_DAY, _candles(), feed="alpaca:iex") + repo.upsert_candles("MSFT-USD", Granularity.ONE_DAY, _candles(), feed="alpaca:sip") + facts = market_facts(repo, "MSFT-USD", "USD") + assert facts.volume_feed == "alpaca:iex, alpaca:sip" + assert facts.volume_feed_is_consolidated is False + + +def test_the_scope_read_is_for_the_granularity_the_statistic_uses(repo: Repository) -> None: + """`market_facts` medians ONE_DAY bars, so ONE_DAY provenance is what governs. An hourly + series fetched under a different feed must not change this verdict.""" + repo.upsert_candles("MSFT-USD", Granularity.ONE_DAY, _candles(), feed="alpaca:sip") + repo.upsert_candles("MSFT-USD", Granularity.ONE_HOUR, _candles(), feed="alpaca:iex") + facts = market_facts(repo, "MSFT-USD", "USD") + assert facts.volume_feed_is_consolidated is True diff --git a/tests/test_proposer.py b/tests/test_proposer.py index a4ba88e8..b697aafa 100644 --- a/tests/test_proposer.py +++ b/tests/test_proposer.py @@ -1,4 +1,5 @@ import json +from dataclasses import replace from decimal import Decimal import pytest @@ -335,6 +336,11 @@ def test_data_derived_failures_tags_actually_match_screen_asset_output(): venue="coinbase", ) tags = {f.split(":")[0] for f in screen_mod.screen_asset(facts, None).failures} + # `liquidity` and `liquidity_unmeasured` are mutually exclusive by construction (#696): which + # one a below-floor series emits depends on whether its feed sees the whole market, so no + # single set of facts produces both. Both arms are screened, or the tag looks unreachable. + partial = replace(facts, volume_feed="alpaca:iex", volume_feed_is_consolidated=False) + tags |= {f.split(":")[0] for f in screen_mod.screen_asset(partial, None).failures} missing = DATA_DERIVED_FAILURES - tags assert not missing, ( f"{missing} no longer appear as failure tags in screen_asset -- the zero-bar " From 5754c4d4f47f05624c74797e74d35032d013aca2 Mon Sep 17 00:00:00 2001 From: Elmehdi Aitbrahim Date: Thu, 3 Sep 2026 14:43:37 -0400 Subject: [PATCH 2/2] fix(doctor,compliance): review findings on the lower-bound gate (#696) Three findings from the review of #711. The first silenced the report in the exact configuration the feature exists for. 1. `feed_scope_findings` returned early on the partial group, so unrecorded series were dropped whenever any partial series existed -- which is the ORDINARY state of this deployment: equities on a single-venue feed alongside crypto series cached before provenance existed. Verified: `{MSFT-USD: (alpaca:iex,), ETH-USD: ()}` reported only MSFT. Every test in the new file exercised one category alone, which is exactly how that survived a suite written test-first. Both groups now report in ONE finding -- one rather than two so `data.feed_scope` stays a single name for the name-coverage pin -- with a headline counting both and a fix naming both remedies, which are genuinely different actions: a bounded series needs re-fetching under a CONSOLIDATED feed, an unrecorded one needs re-fetching under any feed just to stamp it. 2. The doctor hook enumerated `products x granularities` unconditionally, so a never-fetched series read as "predates feed provenance". On the seeded test database that produced nine phantom warnings. A series with no bars has no provenance because it has no BARS, and `data.missing` already reports it. The hook now filters on `n_candles > 0`, reusing the `assess_products` rows the health report was already computing rather than sweeping twice. 3. `f"{Decimal('5E+5'):,}"` is `5E+5`, not `500,000` -- Decimal keeps whatever exponent its arithmetic produced, and `median = volume * close` can land there. The refusal line is what an operator reads to decide whether a series is worth re-fetching, and `5E+5` against `1,000,000` is not a comparison anyone should be asked to make. Both figures now go through a `_dollars` helper that normalises the exponent away. Tests written first, red before green: 4 new (3 on the coexisting groups, 1 on the exponent form) plus `test_feed_scope_ignores_series_with_no_bars`, which failed with all nine phantom series before the fix. Mutation-verified, 3 mutants, all killed: the early return restored; the bar-count filter widened to `>= 0`; the dollar helper's `quantize` removed. Refs #696 Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01NzuKAe2RVrPt9acVAWjRyL --- keel/commands/doctor.py | 87 +++++++++++-------- keel/compliance/screen.py | 22 ++++- tests/commands/test_doctor.py | 16 ++++ tests/commands/test_doctor_feed_scope.py | 44 ++++++++++ .../compliance/test_liquidity_lower_bound.py | 20 +++++ 5 files changed, 150 insertions(+), 39 deletions(-) diff --git a/keel/commands/doctor.py b/keel/commands/doctor.py index cf01dfa5..3191403f 100644 --- a/keel/commands/doctor.py +++ b/keel/commands/doctor.py @@ -856,9 +856,7 @@ def feed_scope_findings(series_feeds: dict[tuple[str, str], tuple[str, ...]]) -> Finding( "data.feed_scope", OK, - "every series' volume is consolidated" - if total - else "no cached series to judge", + "every series' volume is consolidated" if total else "no cached series to judge", f"{total} series carry a recorded, consolidated feed -- the liquidity statistic " "measures the market rather than bounding it" if total @@ -867,30 +865,41 @@ def feed_scope_findings(series_feeds: dict[tuple[str, str], tuple[str, ...]]) -> ) ] + # BOTH groups, in ONE finding. They coexist in the ordinary case -- equities on a + # single-venue feed alongside crypto series cached before provenance existed -- and an + # early return on `partial` dropped the unrecorded group silently in exactly that + # configuration. One finding rather than two keeps `data.feed_scope` a single name in the + # report, which the name-coverage pin depends on. + headline_parts: list[str] = [] + detail_parts: list[str] = [] + fix_parts: list[str] = [] if partial: - described = ", ".join(f"{p} {g} ({feeds})" for p, g, feeds in partial) - return [ - Finding( - "data.feed_scope", - WARN, - f"{len(partial)} series' volume is a LOWER BOUND, not a measurement", - f"{described} -- these feeds report one venue's own executions, so " - "median daily volume is a lower bound on consolidated volume. Above the " - "admission floor that is conclusive; below it, nothing is established", - "re-fetch under a consolidated feed to decide a below-floor series: " - "`keel fetch --refresh`", - ) - ] + headline_parts.append(f"{len(partial)} series bounded") + detail_parts.append( + ", ".join(f"{p} {g} ({feeds})" for p, g, feeds in partial) + + " -- these feeds report one venue's own executions, so median daily volume is a " + "LOWER BOUND on consolidated volume. Above the admission floor that is conclusive; " + "below it, nothing is established" + ) + fix_parts.append( + "re-fetch a below-floor bounded series under a CONSOLIDATED feed: " + "`keel fetch --refresh`" + ) + if unrecorded: + headline_parts.append(f"{len(unrecorded)} series unrecorded") + detail_parts.append( + f"{', '.join(unrecorded)} -- feed provenance UNRECORDED, so whether this volume " + "measures the market or bounds it is unknown -- not the same as knowing it is partial" + ) + fix_parts.append("re-fetch an unrecorded series to stamp provenance: `keel fetch`") return [ Finding( "data.feed_scope", WARN, - f"{len(unrecorded)} series predate feed provenance", - f"{', '.join(unrecorded)} -- feed provenance UNRECORDED, so whether this " - "volume measures the market or bounds it is unknown -- not the same as " - "knowing it is partial", - "re-fetch to stamp provenance: `keel fetch`", + ", ".join(headline_parts), + "; ".join(detail_parts), + " | ".join(fix_parts), ) ] @@ -1297,15 +1306,29 @@ def gather_findings(repo: Any, config: Any, log_lines: Iterable[str], now_ts: in market_closed = agent.recorded_market_closed(repo, config, now_ts) start_ts = now_ts - DOCTOR_WINDOW_SEC - # #696. The same (product, granularity) set the health report covers -- the series whose - # volume the slippage model and the admission floor actually read. + # Computed ONCE and read twice: the health report needs every row, and the feed-scope + # report needs to know which series actually HAVE bars. + health = fetch.assess_products( + repo, + products, + granularities, + now_ts, + start_ts, + freshness_mod.DEFAULT_TOLERANCE_BARS, + market_closed, + ) + + # #696. Only series that carry candles. A never-fetched series has no provenance because it + # has no BARS, not because it predates the provenance table -- `data.missing` already reports + # an empty series, and on a fresh deployment this would otherwise say "predates feed + # provenance" about every one of them. findings += feed_scope_findings( { - (product, Granularity(granularity).value): repo.get_series_feeds( - product, granularity + (row.product, row.granularity.value): repo.get_series_feeds( + row.product, row.granularity ) - for product in products - for granularity in granularities + for row, _unexplained in health + if row.n_candles > 0 } ) @@ -1317,15 +1340,7 @@ def gather_findings(repo: Any, config: Any, log_lines: Iterable[str], now_ts: in freshness=row, unexplained_gaps=unexplained, ) - for row, unexplained in fetch.assess_products( - repo, - products, - granularities, - now_ts, - start_ts, - freshness_mod.DEFAULT_TOLERANCE_BARS, - market_closed, - ) + for row, unexplained in health ] ) findings += admissibility_findings( diff --git a/keel/compliance/screen.py b/keel/compliance/screen.py index 3f6392e6..23f0beb1 100644 --- a/keel/compliance/screen.py +++ b/keel/compliance/screen.py @@ -254,11 +254,16 @@ def screen_asset( # NOT a verdict. Below the floor on a single-venue feed is equally consistent with a thin # asset and with a liquid one that barely trades HERE, and the gate has measured neither. # Saying "illiquid" would assert the half it cannot see. + # `:,` on a Decimal keeps whatever exponent the arithmetic produced -- `Decimal("5E+5")` + # formats as `5E+5`, not `500,000`, and `volume * close` can land in that form. An + # operator comparing `5E+5` against `1,000,000` is being asked to do the wrong work. + observed = _dollars(facts.median_daily_volume) + floor = _dollars(policy.min_median_daily_volume) failures.append( f"liquidity_unmeasured: {facts.volume_feed} reports one venue's own executions, and " - f"its median daily volume ${facts.median_daily_volume:,} is below the " - f"${policy.min_median_daily_volume:,} floor -- so CONSOLIDATED volume is UNMEASURED, " - "not low. Re-fetch this series under a consolidated feed to decide it" + f"its median daily volume {observed} is below the {floor} floor -- so CONSOLIDATED " + "volume is UNMEASURED, not low. Re-fetch this series under a consolidated feed to " + "decide it" ) else: scope = ( @@ -522,6 +527,17 @@ class DiscoveryPolicy: min_quote_24h_volume: Decimal = Decimal("100000") +def _dollars(amount: Decimal) -> str: + """`$1,234,567` -- thousands-separated, exponent form normalised away. + + `f"{Decimal('5E+5'):,}"` is `5E+5`: Decimal preserves the exponent its arithmetic produced, + and `format` does not normalise it. Every figure this renders comes from `volume * close`, + so the form is not hypothetical, and these strings are read by an operator deciding whether + a series is worth re-fetching. + """ + return f"${amount.quantize(Decimal(1)):,}" + + def median_daily_quote_volume(candles: Sequence[Any]) -> Decimal: """Median of `volume * close` over `candles` -- the liquidity statistic, defined ONCE. diff --git a/tests/commands/test_doctor.py b/tests/commands/test_doctor.py index a440c368..de9715dd 100644 --- a/tests/commands/test_doctor.py +++ b/tests/commands/test_doctor.py @@ -18,6 +18,7 @@ from keel_core.trade_scope import READ_ONLY, TRADING, TradeScopeState, VenueTradeScope from keel.commands.doctor import ( + OK, AdmissibilityRow, Finding, SeriesHealth, @@ -569,6 +570,21 @@ def _seeded_repo(db_path: Path): return Repository(conn) +def test_feed_scope_ignores_series_with_no_bars(tmp_path, valid_config_path) -> None: + """A product that was never fetched has no provenance because it has no CANDLES, not because + it predates the provenance table (#696). `data.missing` already reports an empty series; the + feed-scope report saying "predates feed provenance" about it is advice from the absence of + bars, and on a fresh deployment it would say that about everything. + """ + repo = _seeded_repo(tmp_path / "keel.db") + config = load_config(valid_config_path) + findings = gather_findings(repo, config, [], NOW) + (scope,) = [f for f in findings if f.name == "data.feed_scope"] + assert scope.status == OK, scope.detail + for product in config.allowlist: + assert product not in scope.detail + + def test_gather_findings_covers_every_check_over_a_seeded_db(tmp_path, valid_config_path) -> None: repo = _seeded_repo(tmp_path / "keel.db") config = load_config(valid_config_path) diff --git a/tests/commands/test_doctor_feed_scope.py b/tests/commands/test_doctor_feed_scope.py index 9bf94699..37b50a95 100644 --- a/tests/commands/test_doctor_feed_scope.py +++ b/tests/commands/test_doctor_feed_scope.py @@ -81,3 +81,47 @@ def test_the_fix_line_says_the_re_fetch_must_be_under_a_different_feed() -> None (finding,) = feed_scope_findings({("MSFT-USD", "ONE_DAY"): ("alpaca:iex",)}) assert "keel fetch" in finding.fix assert "consolidated" in finding.fix.lower() + + +# --- the two states coexist, because that IS this deployment -------------------------------- + + +def test_partial_and_unrecorded_are_both_reported_when_both_exist() -> None: + """The normal state of this deployment: equities on a single-venue feed, plus legacy crypto + series cached before provenance existed. An early return on the partial group dropped the + unrecorded one entirely -- and every test here exercised one category alone, which is + exactly how that survived. + """ + (finding,) = feed_scope_findings( + { + ("MSFT-USD", "ONE_DAY"): ("alpaca:iex",), + ("ETH-USD", "ONE_DAY"): (), + } + ) + assert finding.status == WARN + assert "MSFT-USD" in finding.detail + assert "ETH-USD" in finding.detail, "the unrecorded series was dropped" + assert "lower bound" in finding.detail.lower() + assert "unrecorded" in finding.detail.lower() + + +def test_the_fix_covers_both_remedies_when_both_states_exist() -> None: + """They are different actions: re-fetch a partial series under a CONSOLIDATED feed; re-fetch + an unrecorded one under any feed just to stamp it.""" + (finding,) = feed_scope_findings( + {("MSFT-USD", "ONE_DAY"): ("alpaca:iex",), ("ETH-USD", "ONE_DAY"): ()} + ) + assert "consolidated" in finding.fix.lower() + assert "keel fetch" in finding.fix + + +def test_the_headline_counts_both_groups() -> None: + findings = feed_scope_findings( + { + ("MSFT-USD", "ONE_DAY"): ("alpaca:iex",), + ("AAPL-USD", "ONE_DAY"): ("alpaca:iex",), + ("ETH-USD", "ONE_DAY"): (), + } + ) + (finding,) = findings + assert "2" in finding.headline and "1" in finding.headline diff --git a/tests/compliance/test_liquidity_lower_bound.py b/tests/compliance/test_liquidity_lower_bound.py index 23278702..b1541b90 100644 --- a/tests/compliance/test_liquidity_lower_bound.py +++ b/tests/compliance/test_liquidity_lower_bound.py @@ -181,3 +181,23 @@ def test_the_new_tag_is_not_waivable_yet() -> None: The module says expanding this set is a decision, not a default, and no attestation record exists for volume today -- a waiver with nothing to record would be a rubber stamp.""" assert "liquidity_unmeasured" not in WAIVABLE_CRITERIA + + +def test_an_exponent_form_decimal_is_still_printed_as_dollars() -> None: + """`f"{Decimal('5E+5'):,}"` is `5E+5`, not `500,000` -- Decimal keeps the exponent its + arithmetic produced. The median is `volume * close`, so exponent form is reachable, and this + string is what an operator reads to decide whether $500,000 against a $1,000,000 floor is + worth a re-fetch. `5E+5` against `1,000,000` is not a comparison anyone should have to make. + """ + asset, instrument = _clean_attestations() + facts = _facts("5E+5", scope=False) + assert facts.median_daily_volume == Decimal("500000") # same number, different form + result = screen_asset( + facts, + asset, + policy=ScreenPolicy(min_median_daily_volume=_FLOOR, min_daily_bars=1), + instrument=instrument, + ) + line = next(f for f in result.failures if f.startswith("liquidity_unmeasured")) + assert "500,000" in line + assert "E+" not in line