From fbdeee049e3044c8321214b26a9fc8c60b4f45dd Mon Sep 17 00:00:00 2001 From: Elmehdi Aitbrahim Date: Sat, 15 Aug 2026 16:15:50 -0400 Subject: [PATCH 1/2] fix(compliance): pin the admission floor's value, not just its relationship This PR's own test_admission.py change replaced an equality assertion on DEFAULT_MIN_QUOTE_24H_VOLUME vs ScreenPolicy().min_median_daily_volume with a `<` relationship check. That relationship guard is correct and stays, but it was also the suite's only test pinning the admission floor's VALUE -- and nothing replaced that. Verified: dropping min_median_daily_volume from 1,000,000 to 200,000 (a 5x cut to the real criterion that decides which assets a money-moving tool may buy) left the whole suite green, 2741 passed / 1 skipped, `<` assertion included. Add an absolute pin alongside the `<` assertion so both are guarded: the relationship (discovery must never be stricter than the gate) and the criterion itself. Co-Authored-By: Claude Opus 5 (1M context) --- tests/commands/test_admission.py | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+) diff --git a/tests/commands/test_admission.py b/tests/commands/test_admission.py index 61492f20..f86a8ae9 100644 --- a/tests/commands/test_admission.py +++ b/tests/commands/test_admission.py @@ -851,3 +851,23 @@ def test_the_discovery_floor_is_strictly_below_the_admission_liquidity_floor(): from keel.compliance.screen import ScreenPolicy assert DEFAULT_MIN_QUOTE_24H_VOLUME < ScreenPolicy().min_median_daily_volume + + +def test_the_admission_liquidity_floor_is_pinned_to_its_actual_value(): + """`min_median_daily_volume` is the real admission criterion -- the number that decides + which assets a money-moving tool may buy -- and this is the ONLY test in the suite that pins + its VALUE rather than its relationship to something else. + + `test_the_discovery_floor_is_strictly_below_the_admission_liquidity_floor` above only asserts + `DEFAULT_MIN_QUOTE_24H_VOLUME < ScreenPolicy().min_median_daily_volume`. That `<` guards the + relationship between discovery's pre-filter and the gate correctly, and must stay -- but a + `<` assertion alone permits ANY value above the discovery floor: raise or, worse, silently + lower `min_median_daily_volume` by 5x (verified: dropping it from 1,000,000 to 200,000 still + left the whole suite green, 2741 passed / 1 skipped, with the relationship test still + passing) and nothing else in the suite would notice. Keep BOTH assertions: the `<` one + guards that discovery must never be stricter than the gate, this one guards the criterion + itself. + """ + from keel.compliance.screen import ScreenPolicy + + assert ScreenPolicy().min_median_daily_volume == Decimal("1000000") From b7542bfbea760faafd7411031446b08d5ea5acba Mon Sep 17 00:00:00 2001 From: Elmehdi Aitbrahim Date: Sat, 15 Aug 2026 16:19:00 -0400 Subject: [PATCH 2/2] fix(compliance): guard NaN/Infinity volume, assert the survivor invariant, fix the incident-cluster record, and make discovery's dataclasses genuinely frozen Four independent fixes from an adversarial review of #270: - discover_candidates parsed quote_24h_volume inside a try/except but compared it outside: Decimal("NaN")/"nan"/float("nan")/"sNaN" all parse cleanly and then crash the whole sweep on `<` with decimal.InvalidOperation. Decimal("Infinity") also parses cleanly, compares fine, and would silently become a candidate. Both are now caught and counted unreadable_volume, same bucket as a value that failed to parse outright -- a NaN/Infinity venue row is exactly as uninformative as one that failed to parse. - render_discover_report derives survivor_count by subtraction (venue_product_count - excluded.total) but nothing asserted the invariant that subtraction relies on, and DiscoverReport is a public frozen dataclass constructable directly with inconsistent fields -- confirmed one such report renders "10 venue products -> -89 candidates". Added a test pinning len(candidates) + excluded.total == len(products) over a mixed product list, and clamped the subtraction so it can never go negative. - The DiscoveryPolicy comment and the regression test both misnamed the 2026-08-15 incident cluster as (ATOM, BCH, CRV, ALGO)/"all but AAVE". Measured against the venue: the cluster is ATOM, AAVE, BCH, CRV; ALGO was a separate, lower outlier at 437,712 (430,520 an hour later) -- the lowest of the five. The regression test pinned only the cluster's top value (852,133), which a future floor of 500,000 would still pass while silently re-hiding ALGO. Now pinned to ALGO's 437,712, the actual constraint; verified 500,000 fails this corrected test and would not have failed the old one. - DiscoveryResult and DiscoverReport were @dataclass(frozen=True) but held list[Candidate]: mutable in place, and unhashable regardless of the decorator. Both candidates fields are now tuple[Candidate, ...], with callers and tests updated accordingly. Co-Authored-By: Claude Opus 5 (1M context) --- keel/commands/admission.py | 19 +++++- keel/compliance/screen.py | 34 +++++++++-- tests/commands/test_admission.py | 26 ++++++++ tests/commands/test_tui.py | 2 +- tests/compliance/test_screen.py | 102 +++++++++++++++++++++++++------ 5 files changed, 156 insertions(+), 27 deletions(-) diff --git a/keel/commands/admission.py b/keel/commands/admission.py index caa71162..df9838ff 100644 --- a/keel/commands/admission.py +++ b/keel/commands/admission.py @@ -415,9 +415,13 @@ def render_propose_view(view: ProposeView) -> list[str]: @dataclass(frozen=True) class DiscoverReport: + """See `keel.compliance.screen.DiscoveryResult` for why `candidates` is a `tuple`, not a + `list`: on a `frozen=True` dataclass a `list` field is still mutable in place and still makes + the dataclass unhashable, so a tuple is what makes `frozen` mean what it claims here too.""" + quote: str venue_product_count: int - candidates: list[Candidate] + candidates: tuple[Candidate, ...] min_quote_24h_volume: Decimal #: Per-reason exclusion counts over the WHOLE sweep -- every product in `venue_product_count` #: that did not become a candidate, not just the ones cut by `limit` below. See @@ -483,8 +487,17 @@ def render_discover_report(report: DiscoverReport) -> list[str]: how many survivors there were before that cut (see `build_discover_report`'s docstring for the identity this leans on). When that is more than `len(report.candidates)`, a line says so -- how many candidates exist, how many are shown, and that `--limit`/`limit=` controls it -- - so a truncated table can never read as the whole candidate set.""" - survivor_count = report.venue_product_count - report.excluded.total + so a truncated table can never read as the whole candidate set. + + `survivor_count` is clamped to 0: it relies on the invariant `len(candidates) + excluded.total + == venue_product_count` (pinned directly by + `test_discovery_survivors_plus_excluded_always_account_for_every_product` in + `tests/compliance/test_screen.py`, over `discover_candidates`'s output), but `DiscoverReport` + is a public frozen dataclass any caller -- test or otherwise -- can construct directly with + fields that do not actually satisfy it. Without the clamp, an inconsistent report renders a + negative count (`10 venue products -> -89 candidates`), which is not a real state and must + never reach an operator's screen.""" + survivor_count = max(0, report.venue_product_count - report.excluded.total) lines = [ f"{report.venue_product_count} venue products -> {survivor_count} candidates " f"(quote={report.quote}, 24h volume >= {report.min_quote_24h_volume:,.0f}, excluding " diff --git a/keel/compliance/screen.py b/keel/compliance/screen.py index 5feaaa44..017c7fb6 100644 --- a/keel/compliance/screen.py +++ b/keel/compliance/screen.py @@ -479,10 +479,12 @@ class DiscoveryPolicy: #: quiet trading day can push the snapshot below a floor the asset's own median clears many #: times over. #: - #: Measured 2026-08-15: four of five wrongly-dropped assets (ATOM, BCH, CRV, ALGO) sat in an + #: Measured 2026-08-15: four of five wrongly-dropped assets (ATOM, AAVE, BCH, CRV) sat in an #: 852,133-979,000 24h-snapshot cluster on a single quiet day, while their median-over-history - #: liquidity was 3.1x-6.3x the admission floor. 100,000 sits comfortably below that cluster, - #: with room to spare for a quieter day still. + #: liquidity was 3.1x-6.3x the admission floor. The fifth, ALGO, was a separate low outlier at + #: 437,712 (430,520 an hour later) -- the lowest of the five, not part of the cluster, and the + #: one that most tightly constrains how low this floor may safely sit. 100,000 sits + #: comfortably below both, with room to spare for a quieter day still. #: #: This bounds how many products get probed for history; it is not a liquidity verdict. #: `--probe-liquidity` computes the gate's own statistic and is the honest estimator of that. @@ -576,9 +578,15 @@ def summary_line(self) -> str: @dataclass(frozen=True) class DiscoveryResult: """`discover_candidates`'s return value: the survivors, plus a full accounting of everyone - who did not survive and why. See `DiscoveryExclusions` for the accounting half.""" + who did not survive and why. See `DiscoveryExclusions` for the accounting half. - candidates: list[Candidate] + `candidates` is a `tuple`, not a `list`, so that `frozen=True` means what it claims: a `list` + field is mutable in place (`result.candidates.append(...)` would succeed on a "frozen" + dataclass) and a dataclass holding one is unhashable regardless of the decorator. A tuple + closes both gaps. + """ + + candidates: tuple[Candidate, ...] excluded: DiscoveryExclusions @@ -656,6 +664,20 @@ def discover_candidates( except (TypeError, ArithmeticError, ValueError): unreadable_volume += 1 continue + # `Decimal("NaN")`/`Decimal("sNaN")` PARSE cleanly -- the `try` above does not catch + # them -- and then `<` raises `decimal.InvalidOperation`, which used to crash the whole + # sweep on one bad venue row. Treated as unreadable, same bucket as a value that failed + # to parse at all: a NaN is exactly as uninformative about liquidity as `"n/a"` is, and + # counting it separately would only teach an operator to distrust a count that means the + # same thing either way. + # + # `Decimal("Infinity")` also parses cleanly and compares fine (nothing is `>= Infinity`), + # so left unguarded it would silently become a candidate -- but a venue reporting + # infinite 24h volume is not credible data, it is a malformed feed, so it is counted + # unreadable rather than trusted as "definitely liquid". + if volume.is_nan() or volume.is_infinite(): + unreadable_volume += 1 + continue if volume < policy.min_quote_24h_volume: below_volume_floor += 1 continue @@ -670,7 +692,7 @@ def discover_candidates( ) return DiscoveryResult( - candidates=sorted(out, key=lambda c: c.quote_24h_volume, reverse=True), + candidates=tuple(sorted(out, key=lambda c: c.quote_24h_volume, reverse=True)), excluded=DiscoveryExclusions( wrong_quote_currency=wrong_quote_currency, not_online=not_online, diff --git a/tests/commands/test_admission.py b/tests/commands/test_admission.py index f86a8ae9..4af41ede 100644 --- a/tests/commands/test_admission.py +++ b/tests/commands/test_admission.py @@ -791,6 +791,32 @@ def test_render_discover_report_includes_the_exclusion_summary(repo: Repository) assert "below 24h volume floor 1" in text +def test_render_discover_report_never_renders_a_negative_survivor_count(): + """`survivor_count = venue_product_count - excluded.total` relies on the invariant + `len(candidates) + excluded.total == venue_product_count`, which `build_discover_report` + upholds but which nothing STOPS a caller from violating: `DiscoverReport` is a public frozen + dataclass, constructed directly in tests (and by anything else that imports it), so a report + whose fields simply do not agree is one bad construction away. Confirmed: an unclamped + `report.venue_product_count - report.excluded.total` on the report below renders + `10 venue products -> -89 candidates`, which is not a real state and must never reach an + operator. The subtraction in `render_discover_report` is clamped to 0 for exactly this.""" + candidate = screen_mod.Candidate( + product_id="SOL-USD", asset="SOL", base_name="Solana", quote_24h_volume=Decimal("9000000") + ) + report = DiscoverReport( + quote="USD", + venue_product_count=10, + candidates=(candidate,), + min_quote_24h_volume=Decimal("100000"), + excluded=screen_mod.DiscoveryExclusions(below_volume_floor=99), + ) + + text = "\n".join(render_discover_report(report)) + + assert "-89" not in text + assert "0 candidates" in text + + def test_render_discover_report_never_includes_probe_history_marker(): """Out of scope by design: `--probe-history` is one extra network request per candidate, which this offline module must never make.""" diff --git a/tests/commands/test_tui.py b/tests/commands/test_tui.py index 746adc49..30cc3230 100644 --- a/tests/commands/test_tui.py +++ b/tests/commands/test_tui.py @@ -2094,7 +2094,7 @@ def test_build_discover_overlay_with_report_is_nonempty_titled_and_headed() -> N report = DiscoverReport( quote="USD", venue_product_count=900, - candidates=[candidate], + candidates=(candidate,), min_quote_24h_volume=Decimal("5000000"), excluded=screen_mod.DiscoveryExclusions(below_volume_floor=899), ) diff --git a/tests/compliance/test_screen.py b/tests/compliance/test_screen.py index 3aeda027..5c262b4a 100644 --- a/tests/compliance/test_screen.py +++ b/tests/compliance/test_screen.py @@ -549,8 +549,8 @@ def test_discovery_keeps_liquid_online_products_in_the_settlement_currency(): def test_discovery_drops_the_wrong_quote_currency(): from keel.compliance.screen import discover_candidates - assert discover_candidates([_product(quote="USDC")]).candidates == [] - assert discover_candidates([_product(quote="BTC")]).candidates == [] + assert discover_candidates([_product(quote="USDC")]).candidates == () + assert discover_candidates([_product(quote="BTC")]).candidates == () def test_discovery_drops_untradable_products(): @@ -562,20 +562,50 @@ def test_discovery_drops_untradable_products(): {"is_disabled": True}, {"view_only": True}, ): - assert discover_candidates([_product(**kwargs)]).candidates == [], kwargs + assert discover_candidates([_product(**kwargs)]).candidates == (), kwargs def test_discovery_drops_thin_products(): from keel.compliance.screen import discover_candidates - assert discover_candidates([_product(volume="100")]).candidates == [] + assert discover_candidates([_product(volume="100")]).candidates == () def test_discovery_survives_a_malformed_volume_rather_than_crashing(): from keel.compliance.screen import discover_candidates - assert discover_candidates([_product(volume=None)]).candidates == [] - assert discover_candidates([_product(volume="n/a")]).candidates == [] + assert discover_candidates([_product(volume=None)]).candidates == () + assert discover_candidates([_product(volume="n/a")]).candidates == () + + +def test_discovery_treats_nan_volume_as_unreadable_rather_than_crashing(): + """`Decimal("NaN")` parses cleanly -- the `try/except` around the parse does not catch it -- + and then `volume < policy.min_quote_24h_volume` raises `decimal.InvalidOperation`, which used + to crash the whole sweep on a single bad venue row. A NaN is exactly as uninformative about + liquidity as an unparseable string, so it must land in the same `unreadable_volume` bucket, + not blow up the command. Covers a string `"NaN"`, a `float("nan")` (the `str()` call ahead of + `Decimal(...)` still produces the string `"nan"`), and the signaling `"sNaN"` form.""" + from keel.compliance.screen import discover_candidates + + for nan_volume in ("NaN", "nan", float("nan"), "sNaN"): + result = discover_candidates([_product(volume=nan_volume)]) + assert result.candidates == (), nan_volume + assert result.excluded.unreadable_volume == 1, nan_volume + assert result.excluded.below_volume_floor == 0, nan_volume + + +def test_discovery_treats_infinite_volume_as_unreadable_not_a_credible_candidate(): + """`Decimal("Infinity")` also parses cleanly and compares fine against the floor, so left + unguarded it would silently become a candidate. A venue reporting infinite 24h volume is not + credible data -- it is a malformed feed -- so it is counted `unreadable_volume`, the same + fail-closed bucket as any other value this module cannot trust, rather than treated as + "definitely liquid".""" + from keel.compliance.screen import discover_candidates + + result = discover_candidates([_product(volume="Infinity")]) + assert result.candidates == () + assert result.excluded.unreadable_volume == 1 + assert result.excluded.below_volume_floor == 0 def test_discovery_excludes_assets_we_already_hold(): @@ -633,20 +663,27 @@ def test_a_quiet_days_snapshot_no_longer_hides_an_asset_the_gate_would_admit(): Measured on 2026-08-15, five assets were silently dropped by discovery at the old 1,000,000 floor despite the gate's own statistic sitting far above the admission floor: ATOM 3,077,474 (3.08x), AAVE 6,315,463 (6.32x), BCH 5,464,940 (5.46x), CRV 3,329,753 - (3.33x) and ALGO 3,780,207 (3.78x). Four of the five -- all but AAVE -- had 24h snapshots - clustered between 852,133 and 979,000 on that single quiet day, i.e. comfortably below the - old floor while their median-over-history liquidity was multiples of the admission - requirement. - - This test uses ATOM's actual measured 24h figure, `852133`: well under the OLD floor of - 1,000,000 but above the new default of 100,000. Under the default `DiscoveryPolicy()` it - must survive the pre-filter, so the sweep can no longer hide an asset the gate would admit. + (3.33x) and ALGO 3,780,207 (3.78x). Four of the five -- ATOM, AAVE, BCH, CRV -- had 24h + snapshots clustered between 852,133 and 979,000 on that single quiet day, comfortably below + the old floor while their median-over-history liquidity was multiples of the admission + requirement. ALGO was NOT part of that cluster: it was a separate, lower outlier at + `437,712` (`430,520` measured again an hour later) -- the lowest 24h snapshot of the five, + and the one that most tightly constrains how low this floor may safely sit. + + This test pins ALGO's actual measured 24h figure, `437712`, not the cluster's -- the LOWEST + measured value in the incident, not the highest. That is deliberate: a regression test that + instead pinned the top of the cluster (`852133`) would keep passing under a floor as high as + 500,000, which would silently re-hide ALGO while looking green. `437712` is well under the + OLD floor of 1,000,000 but above the new default of 100,000. Under the default + `DiscoveryPolicy()` it must survive the pre-filter, so the sweep can no longer hide an asset + the gate would admit. (Verified separately: a floor of 500,000 fails this test once it is + pinned to `437712`, and would NOT have failed it pinned to `852133`.) """ from keel.compliance.screen import discover_candidates - found = discover_candidates([_product(pid="ATOM-USD", volume="852133")]) + found = discover_candidates([_product(pid="ALGO-USD", volume="437712")]) - assert [c.asset for c in found.candidates] == ["ATOM"] + assert [c.asset for c in found.candidates] == ["ALGO"] def test_discovery_counts_every_exclusion_by_reason(): @@ -692,6 +729,37 @@ def test_discovery_counts_every_exclusion_by_reason(): assert [c.asset for c in result.candidates] == ["SOL"] +def test_discovery_survivors_plus_excluded_always_account_for_every_product(): + """The invariant `len(candidates) + excluded.total == len(products)` that + `render_discover_report` (`keel/commands/admission.py`) leans on to derive its displayed + survivor count via subtraction (`venue_product_count - excluded.total`) instead of carrying + a redundant field. Nothing previously asserted this identity directly -- only that individual + counts landed in the right buckets -- so a future change that drops a product on the floor + (double-counts it, or skips it) without incrementing any `excluded` field or appending to + `candidates` would go unnoticed here and would render nonsense downstream. + + Uses the same one-product-per-reason mix as + `test_discovery_counts_every_exclusion_by_reason`, plus a SECOND survivor, so the identity is + checked with more than one candidate on each side of the equation.""" + from keel.compliance.screen import discover_candidates + + products = [ + _product("WRONGQ-USD", quote="EUR"), + _product("OFFLINE-USD", status="offline"), + _product("HALTED-USD", trading_disabled=True), + _product("VIEWONLY-USD", view_only=True), + _product("BTC-USD"), # already on the allowlist + _product("BADVOL-USD", volume="not-a-number"), + _product("THIN-USD", volume="1"), + _product("SOL-USD"), # survivor 1 + _product("ETH-USD"), # survivor 2 + ] + + result = discover_candidates(products, exclude_assets=frozenset({"BTC"})) + + assert len(result.candidates) + result.excluded.total == len(products) + + def test_discovery_counts_both_trading_disabled_flag_variants_together(): """`trading_disabled` covers BOTH the `trading_disabled` and `is_disabled` product flags -- feeding one of each must total 2 under the single `trading_disabled` reason, not split @@ -707,7 +775,7 @@ def test_discovery_counts_both_trading_disabled_flag_variants_together(): assert result.excluded.trading_disabled == 2 assert result.excluded.total == 2 - assert result.candidates == [] + assert result.candidates == () def test_discovery_exclusions_summary_line_names_every_reason_and_the_total():