From a73a1d64ff241fb0dfcf153f2c6dbaec86310e6e Mon Sep 17 00:00:00 2001 From: Elmehdi Aitbrahim Date: Thu, 23 Jul 2026 10:11:09 -0400 Subject: [PATCH 1/2] feat(compliance): add documented allowlist-screen exceptions (waivers) Adds a general, per-asset per-criterion exception mechanism so a human can record a DOCUMENTED, auditable waiver of an admission criterion -- the motivating case being PAXG's 441 daily bars falling short of the 4-year history floor while shariah/liquidity screening pass clean. The set of criteria that may EVER be waived is restricted to WAIVABLE_CRITERIA (currently just `history`): the shariah checks (attestation, haram_sector, riba_yield, dayn/unknown backing) and `settlement` are never consulted for a waiver, both by construction (screen_asset only reads `waived` inside the history branch) and by an explicit WAIVABLE_CRITERIA membership guard as defense in depth. A waiver is surfaced as a loud warning (never a silent pass) and is self-retiring: once bars clear the floor, screen_asset emits nothing about it. - keel/data/db.py: `screen_exceptions` table, SCHEMA_VERSION 8 -> 9, a documented no-op v9 migration (table creation is handled by the additive `_SCHEMA_STATEMENTS` pass, same pattern as v6/v7). - keel/data/repository.py: upsert/get/list/delete_screen_exception. - keel/compliance/screen.py: `screen_asset(..., waived=None)` and `WAIVABLE_CRITERIA`. - keel/cli.py: `assets exempt`/`assets unexempt` (Choice-restricted to WAIVABLE_CRITERIA), `assets screen` now loads and passes waivers, and `assets list` prints a `exceptions:` section when any are recorded. Co-Authored-By: Claude Opus 4.8 (1M context) --- keel/cli.py | 77 +++++++++++++++++- keel/compliance/screen.py | 38 +++++++-- keel/data/db.py | 28 ++++++- keel/data/repository.py | 45 ++++++++++ tests/compliance/test_assets_cli.py | 122 ++++++++++++++++++++++++++++ tests/compliance/test_screen.py | 61 ++++++++++++++ tests/data/test_db.py | 40 ++++++++- tests/data/test_migrations.py | 19 ++++- tests/data/test_repository.py | 72 ++++++++++++++++ tests/data/test_trade_outcomes.py | 4 +- 10 files changed, 492 insertions(+), 14 deletions(-) diff --git a/keel/cli.py b/keel/cli.py index f0eaeee1..288961a8 100644 --- a/keel/cli.py +++ b/keel/cli.py @@ -530,7 +530,8 @@ def _screen_product( if raw is not None else None ) - return facts, screen_mod.screen_asset(facts, attestation) + waived = repo.get_screen_exceptions(asset) + return facts, screen_mod.screen_asset(facts, attestation, waived=waived) # Failure classes that are DOWNSTREAM of having no cached history: with zero bars `liquidity` @@ -820,13 +821,76 @@ def assets_attest( click.echo(f"attested {asset}: sector={sector} backing={backing} pays_yield={pays_yield}") +@assets_group.command("exempt") +@click.option("--asset", required=True, help="Asset code, e.g. PAXG.") +@click.option( + "--criterion", + required=True, + type=click.Choice(sorted(screen_mod.WAIVABLE_CRITERIA)), + help="The admission criterion to waive. Restricted to WAIVABLE_CRITERIA -- a DATA/market " + "criterion, never a shariah one.", +) +@click.option("--rationale", required=True, help="Why this waiver is granted.") +@click.option("--granted-by", required=True, help="Who granted it.") +@click.pass_context +@with_disclaimer +def assets_exempt( + ctx: click.Context, asset: str, criterion: str, rationale: str, granted_by: str +) -> None: + """Record a DOCUMENTED exception waiving one admission criterion for one asset. + + This waives ONLY a computed DATA/market criterion (history depth today) -- never a shariah + one (a missing attestation, haram sector, riba yield, or dayn/unknown backing): the + `--criterion` Choice is restricted to `screen_mod.WAIVABLE_CRITERIA`, so this command cannot + reach those checks no matter what is typed. The exception is recorded and then surfaced + loudly by `keel assets screen` as a WARNING, never silently -- it is not a default pass. It + is also self-retiring: once the underlying condition it was granted for no longer holds (the + asset accumulates enough history, say), `screen_asset` stops mentioning it at all. + + Not passphrase-gated, for the same reason `keel assets attest` is not: recording an exception + cannot itself place an order or raise a cap, and the screen it feeds only ever ADMITS to a + list that `guards.py` rail 1 still enforces per-trade regardless. + """ + repo = _open_repo(ctx) + repo.upsert_screen_exception( + asset=asset, + criterion=criterion, + rationale=rationale, + granted_by=granted_by, + granted_at=int(time.time()), + ) + click.echo(f"recorded exception: {asset} waives '{criterion}' criterion (by {granted_by})") + + +@assets_group.command("unexempt") +@click.option("--asset", required=True, help="Asset code, e.g. PAXG.") +@click.option( + "--criterion", + required=True, + type=click.Choice(sorted(screen_mod.WAIVABLE_CRITERIA)), + help="The waived criterion to revoke.", +) +@click.pass_context +@with_disclaimer +def assets_unexempt(ctx: click.Context, asset: str, criterion: str) -> None: + """Revoke a documented allowlist-screen exception. A de-risking action, always allowed. + + After this, `keel assets screen` re-evaluates the criterion normally -- if it still fails, + the asset is rejected again. + """ + repo = _open_repo(ctx) + repo.delete_screen_exception(asset, criterion) + click.echo(f"revoked exception: {asset} no longer waives '{criterion}' criterion") + + @assets_group.command("list") @click.pass_context def assets_list(ctx: click.Context) -> None: - """List recorded attestations.""" + """List recorded attestations and any documented screen exceptions.""" repo = _open_repo(ctx) rows = repo.get_asset_attestations() - if not rows: + exceptions = repo.list_screen_exceptions() + if not rows and not exceptions: click.echo("no attestations recorded") return for row in rows: @@ -834,6 +898,13 @@ def assets_list(ctx: click.Context) -> None: f"{row['asset']:<8} sector={row['sector']:<16} backing={row['backing']:<8} " f"pays_yield={bool(row['pays_yield'])!s:<5} by={row['attested_by']}" ) + if exceptions: + click.echo("\nexceptions:") + for row in exceptions: + click.echo( + f"{row['asset']:<8} waives={row['criterion']:<10} by={row['granted_by']} -- " + f"{row['rationale']}" + ) # -- withdrawals ------------------------------------------------------------------------------ diff --git a/keel/compliance/screen.py b/keel/compliance/screen.py index 3912f5c7..74df36f4 100644 --- a/keel/compliance/screen.py +++ b/keel/compliance/screen.py @@ -22,6 +22,7 @@ from __future__ import annotations +from collections.abc import Mapping from dataclasses import dataclass, field from decimal import Decimal @@ -46,6 +47,15 @@ BACKING_NATIVE = "native" # a base-layer coin, neither a claim nor a warehouse receipt KNOWN_BACKINGS = frozenset({BACKING_AYN, BACKING_DAYN, BACKING_NATIVE}) +#: Criteria a documented, human-recorded exception (`keel assets exempt`) may EVER waive. Only +#: DATA/market criteria belong here -- history depth, liquidity, that kind of thing -- because +#: those are facts about our own cache, not about the asset's shariah status. The shariah +#: criteria (a missing attestation, `haram_sector`, `riba_yield`, `dayn`/unknown backing) and +#: `settlement` can NEVER be waived: nothing in this module consults `waived` for them, and the +#: CLI's `--criterion` Choice is restricted to this set. Expanding it is a deliberate future +#: decision, not a default -- do not add to it to make a test pass. +WAIVABLE_CRITERIA = frozenset({"history"}) + @dataclass(frozen=True) class AssetAttestation: @@ -100,18 +110,36 @@ def screen_asset( facts: MarketFacts, attestation: AssetAttestation | None, policy: ScreenPolicy | None = None, + waived: Mapping[str, str] | None = None, ) -> ScreenResult: - """Deterministic admission decision. `attestation=None` fails closed.""" + """Deterministic admission decision. `attestation=None` fails closed. + + `waived` is `{criterion: rationale}` from a documented human exception (`keel assets + exempt` / `repository.get_screen_exceptions`). It is consulted ONLY when a check would + otherwise FAIL, and ONLY for criteria in `WAIVABLE_CRITERIA` -- a waiver for anything else + (a stray `screen_exceptions` row for, say, `attestation`) is silently ignored and that + criterion still fails closed. A waiver never affects any criterion other than its own. + """ policy = policy or ScreenPolicy() + waived = waived or {} failures: list[str] = [] warnings: list[str] = [] # -- computed market facts ------------------------------------------------- if facts.daily_bars < policy.min_daily_bars: - failures.append( - 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 "history" in WAIVABLE_CRITERIA and "history" in waived: + # Self-retiring: this branch is only reached when the check WOULD fail, so a stale + # waiver on an asset that has since accumulated enough history produces no output at + # all -- see the `>=` branch below, which never looks at `waived`. + warnings.append( + f"history: {facts.daily_bars} daily bars < {policy.min_daily_bars} required -- " + f"WAIVED by documented exception: {waived['history']}" + ) + else: + failures.append( + 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: failures.append( f"liquidity: median daily volume {facts.median_daily_volume} < " diff --git a/keel/data/db.py b/keel/data/db.py index 6d174236..844d7483 100644 --- a/keel/data/db.py +++ b/keel/data/db.py @@ -19,7 +19,7 @@ from pathlib import Path from typing import Any -SCHEMA_VERSION = 8 +SCHEMA_VERSION = 9 # Creation order matters for readability (and for backends that validate FK targets eagerly); # SQLite itself only checks FK targets at DML time, but we still declare referenced tables first. @@ -239,6 +239,16 @@ ) """, """ + CREATE TABLE IF NOT EXISTS screen_exceptions ( + asset TEXT NOT NULL, + criterion TEXT NOT NULL, + rationale TEXT NOT NULL, + granted_by TEXT NOT NULL, + granted_at INTEGER NOT NULL, + PRIMARY KEY (asset, criterion) + ) + """, + """ CREATE TABLE IF NOT EXISTS profile ( id INTEGER PRIMARY KEY CHECK (id = 1), autonomous INTEGER NOT NULL DEFAULT 0, @@ -395,6 +405,21 @@ def _migrate_v8_autonomy_expiry(conn: sqlite3.Connection) -> None: conn.execute("ALTER TABLE profile ADD COLUMN autonomous_until INTEGER") +def _migrate_v9_screen_exceptions(conn: sqlite3.Connection) -> None: + """v9 adds `screen_exceptions`. Table creation is handled by `_SCHEMA_STATEMENTS`; there is + deliberately NO backfill. + + A row asserts a human documented a waiver for one asset/criterion pair, with a rationale and + who granted it. Seeding one would fabricate an exception nobody granted; an empty table + correctly says nothing has been excepted yet. + + Unlike v8's `profile.autonomous_until` ADD COLUMN, this is a genuine no-op: `migrate()` runs + every `_SCHEMA_STATEMENTS` statement (all `IF NOT EXISTS`) before the version loop below, so + a database already stamped at v8 picks up the new table from that pass alone. This step only + exists to advance the stamp. + """ + + _MIGRATIONS: dict[int, Callable[[sqlite3.Connection], None]] = { 2: _migrate_v2_broker_subscriptions, 3: _migrate_v3_trade_outcomes, @@ -403,6 +428,7 @@ def _migrate_v8_autonomy_expiry(conn: sqlite3.Connection) -> None: 6: _migrate_v6_asset_attestations, 7: _migrate_v7_profile, 8: _migrate_v8_autonomy_expiry, + 9: _migrate_v9_screen_exceptions, } diff --git a/keel/data/repository.py b/keel/data/repository.py index 26d9b669..da72a0d8 100644 --- a/keel/data/repository.py +++ b/keel/data/repository.py @@ -741,3 +741,48 @@ def get_asset_attestation(self, asset: str) -> dict | None: def get_asset_attestations(self) -> list[dict]: rows = self._conn.execute("SELECT * FROM asset_attestations ORDER BY asset").fetchall() return [dict(row) for row in rows] + + # -- screen exceptions ------------------------------------------------------ + # Documented, per-asset per-criterion waivers of an allowlist-screen admission criterion (KB + # PAXG/history case). See `keel/compliance/screen.py` -- only criteria in `WAIVABLE_CRITERIA` + # are ever honoured, so a row here can never bypass the shariah core. + + def upsert_screen_exception( + self, + asset: str, + criterion: str, + rationale: str, + granted_by: str, + granted_at: int, + ) -> None: + self._conn.execute( + """ + INSERT INTO screen_exceptions + (asset, criterion, rationale, granted_by, granted_at) + VALUES (?, ?, ?, ?, ?) + ON CONFLICT(asset, criterion) DO UPDATE SET + rationale = excluded.rationale, + granted_by = excluded.granted_by, + granted_at = excluded.granted_at + """, + (asset, criterion, rationale, granted_by, granted_at), + ) + self._conn.commit() + + def get_screen_exceptions(self, asset: str) -> dict[str, str]: + rows = self._conn.execute( + "SELECT criterion, rationale FROM screen_exceptions WHERE asset = ?", (asset,) + ).fetchall() + return {row["criterion"]: row["rationale"] for row in rows} + + def list_screen_exceptions(self) -> list[dict]: + rows = self._conn.execute( + "SELECT * FROM screen_exceptions ORDER BY asset, criterion" + ).fetchall() + return [dict(row) for row in rows] + + def delete_screen_exception(self, asset: str, criterion: str) -> None: + self._conn.execute( + "DELETE FROM screen_exceptions WHERE asset = ? AND criterion = ?", (asset, criterion) + ) + self._conn.commit() diff --git a/tests/compliance/test_assets_cli.py b/tests/compliance/test_assets_cli.py index d2e0167d..738e34de 100644 --- a/tests/compliance/test_assets_cli.py +++ b/tests/compliance/test_assets_cli.py @@ -144,6 +144,128 @@ def test_attestations_round_trip_through_list(tmp_path, valid_config_path): assert "ayn" in result.output +# -- documented allowlist-screen exceptions (waivers) -------------------------- +# +# `assets exempt` records a DOCUMENTED, per-asset per-criterion waiver; `assets screen` surfaces +# it loudly (a warning, never a silent pass); `assets unexempt` revokes it. The CLI only lets a +# human waive a criterion in `screen_mod.WAIVABLE_CRITERIA` -- the shariah core is never reachable +# through this surface. + + +def _exempt(runner, db_path, config_path, **over): + args = { + "--asset": "PAXG", + "--criterion": "history", + "--rationale": "441 daily bars, human-reviewed", + "--granted-by": "tester", + } + args.update(over) + flat = [item for pair in args.items() for item in pair] + return runner.invoke( + cli, ["--db", str(db_path), "--config", str(config_path), "assets", "exempt", *flat] + ) + + +def _unexempt(runner, db_path, config_path, asset="PAXG", criterion="history"): + return runner.invoke( + cli, + ["--db", str(db_path), "--config", str(config_path), "assets", "unexempt", + "--asset", asset, "--criterion", criterion], + ) + + +def test_exempt_admits_a_history_failing_asset_and_screen_prints_WAIVED( + tmp_path, valid_config_path +): + db_path = tmp_path / "t.db" + repo = _repo_at(db_path) + _seed_history(repo, "PAXG-USD", bars=400) + runner = CliRunner() + attested = _attest(runner, db_path, valid_config_path, "PAXG", **{"--backing": "ayn"}) + assert attested.exit_code == 0 + + # Before the exception: REJECT on history. + before = runner.invoke( + cli, + ["--db", str(db_path), "--config", str(valid_config_path), + "assets", "screen", "--products", "PAXG-USD"], + ) + assert "0/1 admitted" in before.output + assert "history" in before.output + + result = _exempt(runner, db_path, valid_config_path) + assert result.exit_code == 0, result.output + assert "PAXG" in result.output and "history" in result.output + + after = runner.invoke( + cli, + ["--db", str(db_path), "--config", str(valid_config_path), + "assets", "screen", "--products", "PAXG-USD"], + ) + assert "1/1 admitted" in after.output + assert "WAIVED" in after.output + + +def test_exempt_rejects_a_non_waivable_criterion_at_the_cli_boundary( + tmp_path, valid_config_path +): + db_path = tmp_path / "t.db" + _repo_at(db_path) + result = _exempt(CliRunner(), db_path, valid_config_path, **{"--criterion": "bogus"}) + assert result.exit_code != 0 + + +def test_exempt_rejects_a_shariah_criterion_at_the_cli_boundary(tmp_path, valid_config_path): + """The Choice restricts to WAIVABLE_CRITERIA -- 'attestation' must never be a valid value.""" + db_path = tmp_path / "t.db" + _repo_at(db_path) + result = _exempt(CliRunner(), db_path, valid_config_path, **{"--criterion": "attestation"}) + assert result.exit_code != 0 + + +def test_assets_list_shows_recorded_exceptions(tmp_path, valid_config_path): + db_path = tmp_path / "t.db" + _repo_at(db_path) + runner = CliRunner() + assert _exempt(runner, db_path, valid_config_path).exit_code == 0 + + result = runner.invoke( + cli, ["--db", str(db_path), "--config", str(valid_config_path), "assets", "list"] + ) + assert "exceptions:" in result.output + assert "PAXG" in result.output + assert "history" in result.output + assert "tester" in result.output + + +def test_unexempt_revokes_and_screen_rejects_again(tmp_path, valid_config_path): + db_path = tmp_path / "t.db" + repo = _repo_at(db_path) + _seed_history(repo, "PAXG-USD", bars=400) + runner = CliRunner() + attested = _attest(runner, db_path, valid_config_path, "PAXG", **{"--backing": "ayn"}) + assert attested.exit_code == 0 + assert _exempt(runner, db_path, valid_config_path).exit_code == 0 + + admitted = runner.invoke( + cli, + ["--db", str(db_path), "--config", str(valid_config_path), + "assets", "screen", "--products", "PAXG-USD"], + ) + assert "1/1 admitted" in admitted.output + + revoke = _unexempt(runner, db_path, valid_config_path) + assert revoke.exit_code == 0, revoke.output + + rejected = runner.invoke( + cli, + ["--db", str(db_path), "--config", str(valid_config_path), + "assets", "screen", "--products", "PAXG-USD"], + ) + assert "0/1 admitted" in rejected.output + assert "✗" in rejected.output + + # -- discover ------------------------------------------------------------------ diff --git a/tests/compliance/test_screen.py b/tests/compliance/test_screen.py index f529b081..20ff9c91 100644 --- a/tests/compliance/test_screen.py +++ b/tests/compliance/test_screen.py @@ -150,6 +150,67 @@ def test_policy_thresholds_are_configurable(): assert screen_asset(_facts(bars=200, volume="5"), _attestation(), lenient).admitted is True +# -- documented allowlist-screen exceptions (waivers) -------------------------- +# +# Motivating case: PAXG passes shariah/liquidity screening but fails the 4-year history floor +# (441 bars < 1460). A human can record a DOCUMENTED exception that waives ONLY the `history` +# criterion -- surfaced loudly as a warning, never a silent exemption -- and only for criteria in +# `WAIVABLE_CRITERIA`, so the shariah core can never be bypassed this way. + + +def test_insufficient_history_with_no_waiver_still_rejects(): + result = screen_asset(_facts(bars=400), _attestation()) + assert result.admitted is False + assert any("history" in f for f in result.failures) + + +def test_a_documented_history_waiver_admits_and_warns_loudly(): + result = screen_asset( + _facts(bars=400), _attestation(), waived={"history": "PAXG: 441 bars, human-reviewed"} + ) + assert result.admitted is True + assert not any("history" in f for f in result.failures) + assert any( + "WAIVED" in w and "PAXG: 441 bars, human-reviewed" in w for w in result.warnings + ) + + +def test_a_waiver_is_self_retiring_once_history_clears_the_floor(): + """No leftover warning once the underlying condition it was granted for no longer holds.""" + result = screen_asset(_facts(bars=2000), _attestation(), waived={"history": "stale reason"}) + assert result.admitted is True + assert not any("WAIVED" in w for w in result.warnings) + assert not any("history" in f for f in result.failures) + + +def test_a_waiver_for_a_non_waivable_criterion_is_ignored_and_fails_closed(): + """SAFETY: a stray `screen_exceptions` row for a shariah criterion must never bypass it.""" + result = screen_asset(_facts(), None, waived={"attestation": "someone tried to waive this"}) + assert result.admitted is False + assert any("attestation: MISSING" in f for f in result.failures) + assert not any("WAIVED" in w for w in result.warnings) + + +def test_a_history_waiver_does_not_rescue_a_different_real_failure(): + """The waiver is scoped to history alone -- it must not paper over an unrelated rejection.""" + result = screen_asset( + _facts(bars=400), None, waived={"history": "reason"} + ) + assert result.admitted is False + assert any("attestation: MISSING" in f for f in result.failures) + + +def test_a_history_waiver_does_not_rescue_a_dayn_backing_failure(): + result = screen_asset( + _facts(bars=400), + _attestation(backing="dayn"), + waived={"history": "reason"}, + ) + assert result.admitted is False + assert any("dayn" in f for f in result.failures) + assert not any("history" in f for f in result.failures) + + # -- discovery (proposal stage) ------------------------------------------------ diff --git a/tests/data/test_db.py b/tests/data/test_db.py index 690c30d5..2315e252 100644 --- a/tests/data/test_db.py +++ b/tests/data/test_db.py @@ -103,11 +103,11 @@ def test_agent_state_table_has_key_primary_key(): assert pk_columns == {"key"} -def test_schema_version_is_8(): +def test_schema_version_is_9(): """Deliberate tripwire: bump this literal consciously on every schema change.""" from keel.data.db import SCHEMA_VERSION - assert SCHEMA_VERSION == 8 + assert SCHEMA_VERSION == 9 def test_a_v6_database_migrates_up_and_gains_the_profile_table(tmp_path): @@ -179,3 +179,39 @@ def test_migrating_a_v6_database_twice_is_idempotent(tmp_path): assert int(conn.execute("SELECT version FROM schema_version").fetchone()["version"]) == ( SCHEMA_VERSION ) + + +def test_a_v8_database_migrates_up_and_gains_the_screen_exceptions_table(tmp_path): + """v9 is a documented no-op migration: `_SCHEMA_STATEMENTS` (IF NOT EXISTS) already creates + the new table on an existing v8 DB before the version loop runs, so the migration step itself + has nothing to do -- unlike v8's ADD COLUMN case, which needed real work.""" + from keel.data.db import SCHEMA_VERSION, connect, migrate + + conn = connect(str(tmp_path / "v8.db")) + migrate(conn) + conn.execute("UPDATE schema_version SET version = 8") + conn.commit() + + migrate(conn) + + assert int(conn.execute("SELECT version FROM schema_version").fetchone()["version"]) == ( + SCHEMA_VERSION + ) + named = conn.execute( + "SELECT name FROM sqlite_master WHERE type='table' AND name='screen_exceptions'" + ).fetchone() + assert named is not None, "v9 must add the screen_exceptions table" + + +def test_migrating_a_v8_database_twice_is_idempotent(tmp_path): + from keel.data.db import SCHEMA_VERSION, connect, migrate + + conn = connect(str(tmp_path / "v8_twice.db")) + migrate(conn) + conn.execute("UPDATE schema_version SET version = 8") + conn.commit() + migrate(conn) + migrate(conn) # must not raise + assert int(conn.execute("SELECT version FROM schema_version").fetchone()["version"]) == ( + SCHEMA_VERSION + ) diff --git a/tests/data/test_migrations.py b/tests/data/test_migrations.py index e5d8e29d..4abbafe2 100644 --- a/tests/data/test_migrations.py +++ b/tests/data/test_migrations.py @@ -46,7 +46,7 @@ def test_fresh_database_is_stamped_at_the_current_version() -> None: conn = db.connect(":memory:") db.migrate(conn) version = conn.execute("SELECT version FROM schema_version").fetchone()["version"] - assert version == db.SCHEMA_VERSION == 8 + assert version == db.SCHEMA_VERSION == 9 def test_fresh_database_gets_no_subscription_row() -> None: @@ -209,3 +209,20 @@ def test_migration_to_v6_creates_the_asset_attestations_table_EMPTY() -> None: assert cols >= {"asset", "sector", "backing", "pays_yield", "source", "attested_by"} (count,) = conn.execute("SELECT COUNT(*) FROM asset_attestations").fetchone() assert count == 0 + + +def test_migration_to_v9_creates_the_screen_exceptions_table_EMPTY() -> None: + """Additive DDL, and deliberately NO backfill. + + A row asserts a human documented a waiver for one asset/criterion pair. Seeding one would + fabricate an exception nobody granted; an empty table correctly says nothing has been + excepted yet. + """ + conn = _v1_database() + db.migrate(conn) + cols = {r["name"] for r in conn.execute("PRAGMA table_info(screen_exceptions)")} + assert cols >= {"asset", "criterion", "rationale", "granted_by", "granted_at"} + (count,) = conn.execute("SELECT COUNT(*) FROM screen_exceptions").fetchone() + assert count == 0 + stamped = conn.execute("SELECT version FROM schema_version").fetchone()["version"] + assert stamped == db.SCHEMA_VERSION diff --git a/tests/data/test_repository.py b/tests/data/test_repository.py index 1cd7d963..a5b90620 100644 --- a/tests/data/test_repository.py +++ b/tests/data/test_repository.py @@ -452,3 +452,75 @@ def test_profile_readable_reports_damage_that_get_profile_hides(repo): assert repo.profile_readable() is False assert repo.get_profile().autonomous is False # still fails closed, still no exception + + +# -- screen exceptions -------------------------------------------------------- + + +def test_upsert_screen_exception_round_trips_through_get(repo): + repo.upsert_screen_exception( + asset="PAXG", + criterion="history", + rationale="only 441 daily bars; sector/backing/liquidity all clear", + granted_by="tester", + granted_at=1_800_000_000, + ) + assert repo.get_screen_exceptions("PAXG") == { + "history": "only 441 daily bars; sector/backing/liquidity all clear" + } + + +def test_get_screen_exceptions_is_empty_for_an_asset_with_none(repo): + assert repo.get_screen_exceptions("PAXG") == {} + + +def test_upsert_screen_exception_on_conflict_updates_rationale_and_grant_fields(repo): + repo.upsert_screen_exception( + asset="PAXG", criterion="history", rationale="first", granted_by="alice", + granted_at=1_000, + ) + repo.upsert_screen_exception( + asset="PAXG", criterion="history", rationale="second", granted_by="bob", + granted_at=2_000, + ) + + assert repo.get_screen_exceptions("PAXG") == {"history": "second"} + (row,) = repo.list_screen_exceptions() + assert row["granted_by"] == "bob" + assert row["granted_at"] == 2_000 + + +def test_list_screen_exceptions_returns_all_rows_ordered(repo): + repo.upsert_screen_exception( + asset="SOL", criterion="history", rationale="r2", granted_by="b", granted_at=2 + ) + repo.upsert_screen_exception( + asset="PAXG", criterion="history", rationale="r1", granted_by="a", granted_at=1 + ) + + rows = repo.list_screen_exceptions() + assert [(r["asset"], r["criterion"]) for r in rows] == [ + ("PAXG", "history"), + ("SOL", "history"), + ] + + +def test_delete_screen_exception_removes_the_row(repo): + repo.upsert_screen_exception( + asset="PAXG", criterion="history", rationale="r", granted_by="a", granted_at=1 + ) + repo.delete_screen_exception("PAXG", "history") + assert repo.get_screen_exceptions("PAXG") == {} + assert repo.list_screen_exceptions() == [] + + +def test_get_screen_exceptions_is_scoped_to_the_asset(repo): + repo.upsert_screen_exception( + asset="PAXG", criterion="history", rationale="paxg reason", granted_by="a", granted_at=1 + ) + repo.upsert_screen_exception( + asset="SOL", criterion="history", rationale="sol reason", granted_by="a", granted_at=1 + ) + + assert repo.get_screen_exceptions("PAXG") == {"history": "paxg reason"} + assert "SOL" not in repo.get_screen_exceptions("PAXG") diff --git a/tests/data/test_trade_outcomes.py b/tests/data/test_trade_outcomes.py index 22075335..9273505c 100644 --- a/tests/data/test_trade_outcomes.py +++ b/tests/data/test_trade_outcomes.py @@ -35,11 +35,11 @@ def _outcome(**overrides: object) -> dict: return base -def test_schema_is_at_version_8() -> None: +def test_schema_is_at_version_9() -> None: conn = db.connect(":memory:") db.migrate(conn) version = conn.execute("SELECT version FROM schema_version").fetchone()["version"] - assert version == db.SCHEMA_VERSION == 8 + assert version == db.SCHEMA_VERSION == 9 def test_fresh_database_has_no_outcomes() -> None: From ecd37e29561707bd9493e3c26adf16dca7dc4307 Mon Sep 17 00:00:00 2001 From: Elmehdi Aitbrahim Date: Thu, 23 Jul 2026 10:22:57 -0400 Subject: [PATCH 2/2] fix(compliance): harden screen-exception review findings (PR #134) Adversarial review of #134 found 2 should-fix + 2 nits: - WAIVABLE_CRITERIA guard was tautological (`"history" in WAIVABLE_CRITERIA` is a compile-time True) and its safety test was vacuous (default bars=2000 never entered the history branch, so `waived` was never consulted). Replaced the inline check with an up-front `effective_waived` filter in `screen_asset` -- real defense-in-depth against a future WAIVABLE_CRITERIA shrink or a careless new branch -- and rewrote the safety test to actually enter the history branch, plus added a test that a stray non-waivable key riding alongside a real waiver is dropped, not honored. - A blank/whitespace rationale used to ADMIT with an empty "WAIVED by documented exception: " warning -- an undocumented "documented exception." `screen_asset` now only honors a waiver whose rationale is non-blank (mirrors the existing unsourced-attestation guard); `assets exempt` rejects a blank `--rationale` at the CLI boundary too. - `assets exempt`/`assets unexempt` now uppercase `--asset`, matching the uppercase asset code `_screen_product` looks waivers up by -- a `--asset paxg` waiver no longer silently no-ops against `PAXG-USD`. - `delete_screen_exception` now returns the rowcount removed; `assets unexempt` only echoes a revoke confirmation when a row actually existed, otherwise reports "no such exception" instead of a false success. Co-Authored-By: Claude Opus 4.8 (1M context) --- keel/cli.py | 16 ++++++- keel/compliance/screen.py | 19 ++++++-- keel/data/repository.py | 7 ++- tests/compliance/test_assets_cli.py | 73 +++++++++++++++++++++++++++++ tests/compliance/test_screen.py | 46 +++++++++++++++++- tests/data/test_repository.py | 12 ++++- 6 files changed, 160 insertions(+), 13 deletions(-) diff --git a/keel/cli.py b/keel/cli.py index 288961a8..b6dda2b5 100644 --- a/keel/cli.py +++ b/keel/cli.py @@ -851,6 +851,14 @@ def assets_exempt( cannot itself place an order or raise a cap, and the screen it feeds only ever ADMITS to a list that `guards.py` rail 1 still enforces per-trade regardless. """ + if not rationale.strip(): + # Mirrors the unsourced-attestation guard in `screen_asset` (`if not + # attestation.source.strip()`): an unsourced claim is not evidence, and a blank rationale + # is not documentation -- it would be an "undocumented documented exception." + raise click.BadParameter( + "rationale must be a non-empty documented reason", param_hint="--rationale" + ) + asset = asset.upper() # matches the uppercase asset `_screen_product` looks waivers up by repo = _open_repo(ctx) repo.upsert_screen_exception( asset=asset, @@ -878,9 +886,13 @@ def assets_unexempt(ctx: click.Context, asset: str, criterion: str) -> None: After this, `keel assets screen` re-evaluates the criterion normally -- if it still fails, the asset is rejected again. """ + asset = asset.upper() # matches the uppercase asset `assets exempt` records under repo = _open_repo(ctx) - repo.delete_screen_exception(asset, criterion) - click.echo(f"revoked exception: {asset} no longer waives '{criterion}' criterion") + removed = repo.delete_screen_exception(asset, criterion) + if removed: + click.echo(f"revoked exception: {asset} no longer waives '{criterion}' criterion") + else: + click.echo(f"no such exception: {asset} has no '{criterion}' waiver") @assets_group.command("list") diff --git a/keel/compliance/screen.py b/keel/compliance/screen.py index 74df36f4..6346d04c 100644 --- a/keel/compliance/screen.py +++ b/keel/compliance/screen.py @@ -118,22 +118,31 @@ def screen_asset( exempt` / `repository.get_screen_exceptions`). It is consulted ONLY when a check would otherwise FAIL, and ONLY for criteria in `WAIVABLE_CRITERIA` -- a waiver for anything else (a stray `screen_exceptions` row for, say, `attestation`) is silently ignored and that - criterion still fails closed. A waiver never affects any criterion other than its own. + criterion still fails closed. A waiver never affects any criterion other than its own, and a + blank/whitespace rationale is treated as no waiver at all (fail closed -- see the `.strip()` + check below, mirroring the unsourced-attestation guard further down). """ policy = policy or ScreenPolicy() - waived = waived or {} + # Filtered ONCE, up front, rather than inline per-branch: this is the actual defense-in-depth + # for a criterion that is not in WAIVABLE_CRITERIA. `history` is currently the SOLE consumer + # of a waiver (screen_asset only ever reads `effective_waived["history"]`, so a shariah check + # is already structurally unreachable from `waived`) -- but filtering here means that even if + # a future edit wires a waiver lookup into another branch, it can never see an entry for a + # criterion nobody was allowed to grant one for, because it was dropped before any branch ran. + effective_waived = {c: r for c, r in (waived or {}).items() if c in WAIVABLE_CRITERIA} failures: list[str] = [] warnings: list[str] = [] # -- computed market facts ------------------------------------------------- if facts.daily_bars < policy.min_daily_bars: - if "history" in WAIVABLE_CRITERIA and "history" in waived: + history_rationale = effective_waived.get("history", "").strip() + if history_rationale: # Self-retiring: this branch is only reached when the check WOULD fail, so a stale # waiver on an asset that has since accumulated enough history produces no output at - # all -- see the `>=` branch below, which never looks at `waived`. + # all -- see the `>=` branch below, which never looks at `effective_waived`. warnings.append( f"history: {facts.daily_bars} daily bars < {policy.min_daily_bars} required -- " - f"WAIVED by documented exception: {waived['history']}" + f"WAIVED by documented exception: {history_rationale}" ) else: failures.append( diff --git a/keel/data/repository.py b/keel/data/repository.py index da72a0d8..63f1ac93 100644 --- a/keel/data/repository.py +++ b/keel/data/repository.py @@ -781,8 +781,11 @@ def list_screen_exceptions(self) -> list[dict]: ).fetchall() return [dict(row) for row in rows] - def delete_screen_exception(self, asset: str, criterion: str) -> None: - self._conn.execute( + def delete_screen_exception(self, asset: str, criterion: str) -> int: + """Returns the number of rows removed (0 or 1), so a caller can tell a real revoke from + a no-op on a row that never existed rather than echoing success either way.""" + cursor = self._conn.execute( "DELETE FROM screen_exceptions WHERE asset = ? AND criterion = ?", (asset, criterion) ) self._conn.commit() + return cursor.rowcount diff --git a/tests/compliance/test_assets_cli.py b/tests/compliance/test_assets_cli.py index 738e34de..b9e8083d 100644 --- a/tests/compliance/test_assets_cli.py +++ b/tests/compliance/test_assets_cli.py @@ -5,6 +5,7 @@ from decimal import Decimal from pathlib import Path +import pytest from click.testing import CliRunner import keel.cli as cli_module @@ -223,6 +224,41 @@ def test_exempt_rejects_a_shariah_criterion_at_the_cli_boundary(tmp_path, valid_ assert result.exit_code != 0 +@pytest.mark.parametrize("blank", ["", " "]) +def test_exempt_rejects_a_blank_rationale(tmp_path, valid_config_path, blank): + """'documented, never silent' -- a blank rationale is not documentation, so the CLI must + refuse to record it rather than write an undocumented 'documented exception.'""" + db_path = tmp_path / "t.db" + _repo_at(db_path) + result = _exempt(CliRunner(), db_path, valid_config_path, **{"--rationale": blank}) + assert result.exit_code != 0 + + +def test_exempt_normalizes_a_lowercase_asset_so_screening_still_finds_the_waiver( + tmp_path, valid_config_path +): + """A `--asset paxg` waiver must not silently no-op against the uppercase `PAXG` a product's + asset code resolves to.""" + db_path = tmp_path / "t.db" + repo = _repo_at(db_path) + _seed_history(repo, "PAXG-USD", bars=400) + runner = CliRunner() + attested = _attest(runner, db_path, valid_config_path, "PAXG", **{"--backing": "ayn"}) + assert attested.exit_code == 0 + + result = _exempt(runner, db_path, valid_config_path, **{"--asset": "paxg"}) + assert result.exit_code == 0, result.output + assert "PAXG" in result.output + + screened = runner.invoke( + cli, + ["--db", str(db_path), "--config", str(valid_config_path), + "assets", "screen", "--products", "PAXG-USD"], + ) + assert "1/1 admitted" in screened.output + assert "WAIVED" in screened.output + + def test_assets_list_shows_recorded_exceptions(tmp_path, valid_config_path): db_path = tmp_path / "t.db" _repo_at(db_path) @@ -256,6 +292,7 @@ def test_unexempt_revokes_and_screen_rejects_again(tmp_path, valid_config_path): revoke = _unexempt(runner, db_path, valid_config_path) assert revoke.exit_code == 0, revoke.output + assert "revoked exception" in revoke.output rejected = runner.invoke( cli, @@ -266,6 +303,42 @@ def test_unexempt_revokes_and_screen_rejects_again(tmp_path, valid_config_path): assert "✗" in rejected.output +def test_unexempt_on_a_nonexistent_row_reports_no_such_exception_not_false_success( + tmp_path, valid_config_path +): + """Revoking an exception that was never granted must not read as a successful revoke.""" + db_path = tmp_path / "t.db" + _repo_at(db_path) + runner = CliRunner() + + result = _unexempt(runner, db_path, valid_config_path) + + assert result.exit_code == 0, result.output + assert "no such exception" in result.output + assert "revoked exception" not in result.output + + +def test_unexempt_normalizes_a_lowercase_asset(tmp_path, valid_config_path): + db_path = tmp_path / "t.db" + repo = _repo_at(db_path) + _seed_history(repo, "PAXG-USD", bars=400) + runner = CliRunner() + attested = _attest(runner, db_path, valid_config_path, "PAXG", **{"--backing": "ayn"}) + assert attested.exit_code == 0 + assert _exempt(runner, db_path, valid_config_path).exit_code == 0 + + revoke = _unexempt(runner, db_path, valid_config_path, asset="paxg") + assert revoke.exit_code == 0, revoke.output + assert "revoked exception" in revoke.output + + rejected = runner.invoke( + cli, + ["--db", str(db_path), "--config", str(valid_config_path), + "assets", "screen", "--products", "PAXG-USD"], + ) + assert "0/1 admitted" in rejected.output + + # -- discover ------------------------------------------------------------------ diff --git a/tests/compliance/test_screen.py b/tests/compliance/test_screen.py index 20ff9c91..7b81215b 100644 --- a/tests/compliance/test_screen.py +++ b/tests/compliance/test_screen.py @@ -4,6 +4,8 @@ from decimal import Decimal +import pytest + from keel.compliance.screen import ( AssetAttestation, MarketFacts, @@ -175,6 +177,16 @@ def test_a_documented_history_waiver_admits_and_warns_loudly(): ) +@pytest.mark.parametrize("blank", ["", " ", "\t\n"]) +def test_a_blank_rationale_waiver_does_not_admit_undocumented_is_not_documented(blank): + """The whole thesis is 'documented, never silent'. A blank rationale is not documentation, + so it must fail closed exactly like an unsourced attestation does.""" + result = screen_asset(_facts(bars=400), _attestation(), waived={"history": blank}) + assert result.admitted is False + assert any("history" in f for f in result.failures) + assert not any("WAIVED" in w for w in result.warnings) + + def test_a_waiver_is_self_retiring_once_history_clears_the_floor(): """No leftover warning once the underlying condition it was granted for no longer holds.""" result = screen_asset(_facts(bars=2000), _attestation(), waived={"history": "stale reason"}) @@ -184,13 +196,43 @@ def test_a_waiver_is_self_retiring_once_history_clears_the_floor(): def test_a_waiver_for_a_non_waivable_criterion_is_ignored_and_fails_closed(): - """SAFETY: a stray `screen_exceptions` row for a shariah criterion must never bypass it.""" - result = screen_asset(_facts(), None, waived={"attestation": "someone tried to waive this"}) + """SAFETY, non-vacuous: `bars` is BELOW the floor, so the history branch is actually + entered -- if the `WAIVABLE_CRITERIA` filter were broken, an attestation-keyed waiver could + only matter here if it somehow leaked into the history check too, which this also rules out. + A stray `screen_exceptions` row for a shariah criterion must never bypass it, and must not + incidentally waive history either (no "history" key was ever granted).""" + result = screen_asset( + _facts(bars=400), None, waived={"attestation": "someone tried to waive this"} + ) assert result.admitted is False assert any("attestation: MISSING" in f for f in result.failures) + assert any("history" in f for f in result.failures) # NOT waived -- no "history" key granted + assert not any("WAIVED" in w for w in result.warnings) + + +def test_a_non_waivable_key_does_not_rescue_the_asset_it_was_stray_recorded_on(): + """Same shape as above, but on an asset that is otherwise CLEAN except for low history: an + `attestation`-keyed waiver (never granted for `history`) must still leave history REJECTED.""" + result = screen_asset(_facts(bars=400), _attestation(), waived={"attestation": "x"}) + assert result.admitted is False + assert any("history" in f for f in result.failures) assert not any("WAIVED" in w for w in result.warnings) +def test_a_stray_non_waivable_key_alongside_a_real_waiver_is_dropped_not_honored(): + """The up-front filter drops non-`WAIVABLE_CRITERIA` keys one at a time -- a `settlement` + entry riding along with a legitimate `history` waiver must have zero effect.""" + result = screen_asset( + _facts(bars=400), + _attestation(), + waived={"history": "documented reason", "settlement": "someone tried to waive this too"}, + ) + assert result.admitted is True + assert any("WAIVED" in w and "documented reason" in w for w in result.warnings) + assert not any("settlement" in w for w in result.warnings) + assert result.failures == [] + + def test_a_history_waiver_does_not_rescue_a_different_real_failure(): """The waiver is scoped to history alone -- it must not paper over an unrelated rejection.""" result = screen_asset( diff --git a/tests/data/test_repository.py b/tests/data/test_repository.py index a5b90620..d06c5016 100644 --- a/tests/data/test_repository.py +++ b/tests/data/test_repository.py @@ -505,15 +505,23 @@ def test_list_screen_exceptions_returns_all_rows_ordered(repo): ] -def test_delete_screen_exception_removes_the_row(repo): +def test_delete_screen_exception_removes_the_row_and_returns_rowcount_1(repo): repo.upsert_screen_exception( asset="PAXG", criterion="history", rationale="r", granted_by="a", granted_at=1 ) - repo.delete_screen_exception("PAXG", "history") + removed = repo.delete_screen_exception("PAXG", "history") + assert removed == 1 assert repo.get_screen_exceptions("PAXG") == {} assert repo.list_screen_exceptions() == [] +def test_delete_screen_exception_on_a_nonexistent_row_returns_0(repo): + """The caller must be able to tell a real revoke from a no-op -- a nonexistent row is not an + error, but it must not be reported as a successful revoke either.""" + removed = repo.delete_screen_exception("PAXG", "history") + assert removed == 0 + + def test_get_screen_exceptions_is_scoped_to_the_asset(repo): repo.upsert_screen_exception( asset="PAXG", criterion="history", rationale="paxg reason", granted_by="a", granted_at=1