fix(compliance): stop discovery hiding assets the gate would admit - #270
Merged
Conversation
`keel assets discover` was silently dropping assets that clear the admission gate comfortably, and its own output gave an operator no way to notice. Two defects, one symptom. (a) A one-day statistic compared against a multi-year threshold. The `--min-volume-24h` default was 1,000,000, pinned EQUAL to the admission floor `ScreenPolicy.min_median_daily_volume`. The intent of that pinning (2026-08-08, superseding a 5,000,000 floor that had hidden FET at $2.94M/24h while it measured 4.8x the admission floor) was right: a pre-filter must never be stricter than the criterion it screens for. The mechanism was wrong. Discovery's number is a 24-hour venue snapshot; admission's is the median of volume x close over ALL cached history. Equal NUMBERS cannot make one non-stricter than the other when the two sides measure different statistics -- a single quiet trading day pushes the snapshot below a floor the asset's own median clears many times over. Measured 2026-08-15, five assets were silently dropped whose real gate statistic sits 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), ALGO 3,780,207 (3.78x). Four of the five had 24h volumes clustered between 852,133 and 979,000 on that one quiet day. The discovery floor now sits an order of magnitude BELOW the admission floor (100,000), and the test that used to assert the two are equal now asserts discovery's is STRICTLY LESS -- preserving 2026-08-08's goal while replacing the mechanism that could not deliver it. The admission floor itself is unchanged: it is the real criterion. (b) Nothing recorded what was excluded. `discover_candidates` dropped products with a bare `continue` and returned only survivors, and the CLI printed only `N venue products -> M candidates`. A filter that can silently remove admissible assets must be auditable from its own output. It now returns a `DiscoveryResult` carrying per-reason exclusion counts (wrong quote currency, not online, trading disabled, view only, already on allowlist, unreadable 24h volume, below the 24h floor), surfaced by both `keel assets discover` and the TUI's discover overlay. The new named return type -- rather than a tuple -- is deliberate: an un-updated caller fails loudly at the attribute access instead of silently mis-indexing. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
PR #270 lowered the discovery pre-filter so the five assets it exists to surface (ATOM, AAVE, BCH, CRV, ALGO) become candidates again. But `keel assets discover` sorts candidates by descending 24h volume and defaults --limit to 25, which was sized for the ~35-candidate sweeps the OLD 1,000,000 floor produced. At the new 100,000 floor a typical sweep is ~130 candidates, so the five recovered assets land at ranks 33-59 -- below the CLI's own default view, cut off with no indication anything was hidden. Same defect class the PR exists to fix, one step later in the pipeline. Two changes, both in `assets_discover` (keel/cli.py) and the offline `build_discover_report`/`render_discover_report` path it shares with the TUI's discover overlay (keel/commands/admission.py): - Never truncate silently. When more candidates survive than `--limit` shows, say so: how many exist, how many are shown, that --limit controls it. `render_discover_report` derives the pre-truncation survivor count from `venue_product_count - excluded.total` rather than adding a redundant field to `DiscoverReport`. - Raise the default --limit from 25 to 100. Verified --limit is applied before the probe loop in both paths, so this is free with neither probe flag (`assets discover` still makes exactly one venue request regardless of --limit). --probe-history/--probe-liquidity each cost one request per candidate SHOWN, so combining a probe with a large --limit multiplies the request count -- called out in --limit's own help text. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
eaitbrahim
added a commit
that referenced
this pull request
Aug 15, 2026
…or, stop a NaN crash (#273) * 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) <noreply@anthropic.com> * 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) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The problem
keel assets discoverwas silently dropping assets that clear the admission gate comfortably, and its own output gave an operator no way to notice. Two defects, one symptom.(a) A one-day statistic compared against a multi-year threshold
--min-volume-24hdefaulted to 1,000,000, pinned EQUAL to the admission floorScreenPolicy.min_median_daily_volume.That pinning (2026-08-08, superseding a 5,000,000 floor that had hidden FET at $2.94M/24h while it measured 4.8x the admission floor) had the right intent: a pre-filter must never be stricter than the criterion it screens for. It had the wrong mechanism. The two numbers are not the same statistic:
--min-volume-24hquote_24h_volumemin_median_daily_volumevolume x closeEqual numbers cannot make one non-stricter than the other when the two sides measure different things — one quiet trading day pushes the snapshot below a floor the asset's own median clears many times over.
Measured 2026-08-15, five assets were silently dropped whose real gate statistic sits far above the admission floor:
Four of the five had 24h volumes clustered between 852,133 and 979,000 on that one quiet day — comfortably under the 1,000,000 floor.
The discovery floor now sits an order of magnitude below the admission floor (100,000), leaving real room beneath that measured cluster. This supersedes the 2026-08-08 pinning without abandoning its goal: the test that asserted the two floors are equal now asserts discovery's is strictly less than the gate's, which is the property the original fix was reaching for. The admission floor itself is untouched — it is the real criterion.
(b) Nothing recorded what was excluded
discover_candidatesdropped products with a barecontinueand returned only survivors; the CLI printed onlyN venue products -> M candidates. A filter that can silently remove admissible assets must be auditable from its own output.It now returns a
DiscoveryResultcarrying per-reason exclusion counts, surfaced by bothkeel assets discoverand the TUI's discover overlay:Every reason is listed even at 0, so the line has a fixed shape an operator can scan rather than one that changes with the data. The new named return type — rather than a tuple — is deliberate: an un-updated caller fails loudly at the attribute access instead of silently mis-indexing.
Candidateis unchanged, anddiscover_candidatesstays pure and offline.Changes
keel/compliance/screen.py— newDiscoveryExclusions/DiscoveryResult;discover_candidatescounts each drop against the first criterion it fails;DiscoveryPolicy.min_quote_24h_volume-> 100,000.keel/commands/admission.py—DEFAULT_MIN_QUOTE_24H_VOLUME-> 100,000;DiscoverReport.excluded;render_discover_reportemits the summary.keel/cli.py— option default and help text; echoes the summary.Tests
Written first, confirmed failing for the right reasons (19 red), then implemented:
trading_disabled/is_disabledflag variants pinned to the singletrading_disabledbucket.summary_line()wording pinned exactly.keel assets discoverprints the summary.Gates
🤖 Generated with Claude Code