Skip to content

fix(compliance): review follow-ups to #270 — re-pin the admission floor, stop a NaN crash - #273

Merged
eaitbrahim merged 2 commits into
mainfrom
fix/discover-exclusions-review-followups
Aug 15, 2026
Merged

fix(compliance): review follow-ups to #270 — re-pin the admission floor, stop a NaN crash#273
eaitbrahim merged 2 commits into
mainfrom
fix/discover-exclusions-review-followups

Conversation

@eaitbrahim

Copy link
Copy Markdown
Contributor

Follow-ups from the independent adversarial review of #270. That PR was merged before these landed, so they come as a separate change against current main.

1. HIGH — the admission floor lost its only value pin

#270 changed the floor assertion from DEFAULT_MIN_QUOTE_24H_VOLUME == ScreenPolicy().min_median_daily_volume to <. The relationship guard is right and stays. But it was also the only test pinning the admission floor's value, and nothing replaced it.

Verified on main before this change: editing ScreenPolicy.min_median_daily_volume from 1000000 to 200000 — a 5× loosening of the criterion deciding which assets a money-moving tool may buy — passed all 2756 tests.

This adds test_the_admission_liquidity_floor_is_pinned_to_its_actual_value alongside the < test. The two are not redundant: one guards the relationship (discovery must never be stricter than the gate), the other guards the criterion itself.

Mutation proof on this branch:

floor -> 200000 :  1 failed, 2760 passed
  FAILED test_the_admission_liquidity_floor_is_pinned_to_its_actual_value
reverted        :  2761 passed, 1 skipped

2. MEDIUM — NaN volume crashed assets discover outright

The parse was inside a try, the comparison was not. Decimal("NaN") parses fine, then < raises decimal.InvalidOperation and takes the whole command down. Separately "Infinity" parsed and silently became a candidate.

Both are now counted as unreadable_volume, with the reasoning commented. Verified end to end:

survived crash -> ['GOOD']
unreadable counted -> 2
accounting: 1 + 2 == 3 -> True

3. MEDIUM — the survivor-count identity was load-bearing but unguarded

render_discover_report derives survivor_count = venue_product_count - excluded.total. Nothing asserted the underlying invariant, and an inconsistent DiscoverReport rendered nonsense (10 venue products -> -89 candidates). Adds a test pinning len(candidates) + excluded.total == len(products), and makes the subtraction defensive.

4. LOW/MEDIUM — the incident record was wrong, and the regression test pinned the wrong end

The comments described the 852,133–979,000 cluster as including ALGO. Measured against the live venue, the cluster is ATOM, AAVE, BCH, CRV; ALGO was a separate low outlier at 437,712.

Worse, the regression test pinned only 852133 — the top. A future floor of 500,000 would have passed it while silently re-hiding ALGO. Re-pinned to ALGO's 437712, which is what actually constrains the floor. Verified: a floor of 500,000 fails the corrected test and would not have failed the old one.

5. LOW — frozen=True was cosmetic

DiscoveryResult and DiscoverReport were frozen=True but held a mutable list[Candidate] — appendable and unhashable. Now tuple[Candidate, ...].

Gates

ruff check keel tests packages   All checks passed!
mypy                             Success: no issues found in 224 source files
pytest -q                        2761 passed, 1 skipped

🤖 Generated with Claude Code

eaitbrahim and others added 2 commits August 15, 2026 16:21
…onship

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>
…iant, 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>
@eaitbrahim
eaitbrahim merged commit f758d14 into main Aug 15, 2026
1 check passed
@eaitbrahim
eaitbrahim deleted the fix/discover-exclusions-review-followups branch August 15, 2026 20:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant