From cdd51bcdd7d5b86609f3651cb90cd0b5af4d6fbe Mon Sep 17 00:00:00 2001 From: Elmehdi Aitbrahim Date: Fri, 7 Aug 2026 13:56:27 -0400 Subject: [PATCH] fix(admission): close the non-UTF-8 shortlist hole and pin the claims #176 made An adversarial review of #176 confirmed the network gating and read-only guarantees, and found ten things to fix. This is all ten. The two real bugs: - `build_propose_view` guarded `source.read_text()` with `except OSError`, but `UnicodeDecodeError` subclasses **ValueError**. A UTF-16LE+BOM shortlist -- valid JSON, and what a scout run on Windows writes -- escaped both handlers and broke the function's own "Never raises. Every failure mode is FAIL-SOFT" contract: the TUI overlay repainted `propose read failed: 'utf-8' codec can't decode byte 0xff...` every poll forever, naming no file and no next step, and `keel assets propose --from` exited on a raw traceback. Both now take the existing `unreadable` fail-soft path, which names the file. The two docstrings that claimed an exception "can only come from `open_state()`, never from the shortlist read itself" said something that was false then and is still not the whole truth now (`build_propose_view` screens every parsed candidate, so a locked DB surfaces there too) -- both corrected. - `assets propose --json` emitted `sc.result.failures` raw, so at zero cached bars the payload carried `history: 0 daily bars < 1460 required` unflagged while every human surface suppressed that exact line. Same report, two surfaces, two different answers to "is this asset too young?". `--json` now applies the same `split_failures` the renderers do, and says so explicitly: `failures` / `not_assessable` / `missing_history`. The single-admission-path property (design constraint 4) was convention-only exactly where it is wired: replacing `_screen_product` with an always-ADMIT stub in `_do_screen_report` OR `_do_propose_view` left all 2062 tests green, because the overlay tests only asserted that a title paints and Esc closes. Two `run_live` tests now seed an unattested asset with ample history and liquidity and assert the overlay paints REJECT plus `attestation: MISSING` -- a verdict only the real gate produces. Both stubs now die. Honesty fixes to comments that asserted more than the code can know: - `split_failures` claimed a shallow-but-non-empty cache means the asset "really is too young". `MarketFacts` carries no first-bar timestamp, so it cannot tell that from `keel fetch --years 2`, an aborted fetch, or a venue not serving the full window. Reworded to say so, and to tell the operator to check the fetch window first. - `missing_history_lines` said a zero-bar asset "is not too young" -- same overclaim, opposite direction. It now refuses to rule either way. - "the SECOND deliberate network exception" appeared in the module docstring, the help screen's Safety notes and the operator-facing ARMED overlay, forgetting the ~30s live-balance refresh that has been firing since v3. It is the THIRD of exactly three, as `tui_cmd`'s docstring already said. The help's "Live balance" section now also says the refresh is itself a live venue call. - `run_live` said "Seven modes"; there are six. Coverage for things that were correct but untested, each verified to die under a mutation: `config.proposals_dir` resolution and `~` expansion (the default is `~/keel/proposals`, inside the live deployment root, so a silent regression means reading a right-looking wrong place); `_DISCOVER_TIMEOUT_SEC`'s wiring, pinned per-call so collapsing it into the balance timeout is caught; `missing_history_lines`' semantic sentence and its promise never to restate a suppressed failure verbatim. `test_run_live_discover_closing_discards_the_held_result` is renamed to `..._reopening_after_a_run_is_armed_not_stale`. Deleting the close-branch clear alone leaves it green -- verified -- because the normal-mode `d` branch clears too, and nothing observable from outside `run_live` can separate them, since `mode` only becomes `discover` via that branch. The name now matches what it pins. Co-Authored-By: Claude Opus 5 (1M context) --- keel/cli.py | 6 +- keel/commands/admission.py | 12 +- keel/commands/tui.py | 66 ++++--- keel/compliance/screen.py | 39 ++++- keel/proposer.py | 32 +++- tests/commands/test_admission.py | 63 +++++++ tests/commands/test_tui.py | 258 +++++++++++++++++++++++++++- tests/compliance/test_assets_cli.py | 46 +++++ tests/compliance/test_screen.py | 36 ++++ tests/test_proposer.py | 52 ++++++ 10 files changed, 567 insertions(+), 43 deletions(-) diff --git a/keel/cli.py b/keel/cli.py index 83f8dc08..f02d25df 100644 --- a/keel/cli.py +++ b/keel/cli.py @@ -848,7 +848,11 @@ def assets_propose(ctx: click.Context, from_file: str, as_json: bool) -> None: repo = _open_repo(ctx) try: raw = json.loads(Path(from_file).read_text()) - except (OSError, json.JSONDecodeError) as exc: + # `UnicodeDecodeError` subclasses ValueError, NOT OSError, so it needs naming here or a + # non-UTF-8 shortlist (a scout run on Windows writes UTF-16LE+BOM -- valid JSON, undecodable + # as UTF-8) crashes out with a raw traceback instead of this message. Same fix, same reason, + # as `keel.commands.admission.build_propose_view`'s own read. + except (OSError, UnicodeDecodeError, json.JSONDecodeError) as exc: raise click.ClickException(f"could not read/parse {from_file}: {exc}") from exc try: parsed = parse_proposal(raw) diff --git a/keel/commands/admission.py b/keel/commands/admission.py index bbc734ee..0f377282 100644 --- a/keel/commands/admission.py +++ b/keel/commands/admission.py @@ -217,7 +217,8 @@ def build_propose_view( `Path(config.proposals_dir).expanduser()`. Every failure mode is FAIL-SOFT: a missing directory, an empty one, an unreadable file - (`OSError` -- e.g. a permissions problem, or the file vanishing mid-read), invalid JSON + (`OSError` -- a permissions problem, the file vanishing mid-read -- or `UnicodeDecodeError`, + which is NOT an `OSError` and needs naming separately; see the read below), invalid JSON (`json.JSONDecodeError`), or a malformed top-level shortlist (`ProposalError` from `parse_proposal`, e.g. `{"candidates": "nope"}`) each produce a `ProposeView` carrying the matching `status` and a plain-English `detail` -- never an exception. This function backs a @@ -266,7 +267,14 @@ def build_propose_view( try: raw_text = source.read_text() - except OSError as exc: + except (OSError, UnicodeDecodeError) as exc: + # `UnicodeDecodeError` subclasses **ValueError, not OSError** -- so `except OSError` alone + # let a non-UTF-8 shortlist escape this function entirely, breaking the "Never raises" + # contract above. It is not a hypothetical: a scout run on Windows writes UTF-16LE+BOM, + # which is perfectly valid JSON and unreadable here. Uncaught, the TUI's propose overlay + # repainted `'utf-8' codec can't decode byte 0xff...` every poll forever, naming neither + # the file nor a next step, and `keel assets propose --from` exited on a raw traceback. + # Both now get the same calm, file-naming `unreadable` report a permissions error gets. return ProposeView( source=source, status="unreadable", diff --git a/keel/commands/tui.py b/keel/commands/tui.py index 62c712c5..306cb841 100644 --- a/keel/commands/tui.py +++ b/keel/commands/tui.py @@ -37,14 +37,17 @@ shortlist file in `config.proposals_dir` (or names why there is none). OFFLINE, DB + local filesystem reads only. - `d` **discover** -- `build_discover_overlay` over `build_discover_report`: proposes NEW - candidates from the venue's own product list. This is the one of the three that needs the - network, and it is the SECOND deliberate network exception in this dashboard (the first is `f` - fetch): opening the overlay makes no call at all (it renders an ARMED, not-yet-run - explanation), and only an explicit Enter keypress *inside* the overlay triggers - `_do_discover_report`'s one `_build_broker(config).list_products()` call. The result is then - HELD -- every following poll while the overlay stays open repaints the same cached result (or - error) rather than re-fetching, and closing the overlay discards it, so reopening is armed but - not yet run again. + candidates from the venue's own product list. This is the one of the three overlays that needs + the network, and it is the THIRD deliberate network exception in this dashboard -- the other + two being the automatic ~30s live-balance refresh (`_refresh_balance`, a real `get_accounts` + call that has been firing on its own cadence since v3) and `f` fetch. Counting only fetch, as + this docstring used to, understates by one and tells an operator the dashboard is offline + between keypresses when it is not. Opening the overlay makes no call at all (it renders an + ARMED, not-yet-run explanation), and only an explicit Enter keypress *inside* the overlay + triggers `_do_discover_report`'s one `_build_broker(config).list_products()` call. The result + is then HELD -- every following poll while the overlay stays open repaints the same cached + result (or error) rather than re-fetching, and closing the overlay discards it, so reopening is + armed but not yet run again. None of the three attests, admits, or trades -- `attest` (the human judgment the whole gate rests on) stays deliberately CLI-only, `keel assets attest`. `screen`/`propose`/`discover` only ever @@ -390,6 +393,9 @@ def _note(text: str) -> None: _row("Live balance") _note(" 'live account' shows the REAL account's spendable quote balance (e.g. USDC),") _note(" refreshed every ~30s and immediately on 'r' or 'f' -- so a deposit or sell shows up.") + _note(" Each refresh is a LIVE call to the venue (get_accounts) -- one of the three network") + _note(" touches this dashboard makes, and the only one that happens without a keypress. It") + _note(" is a read: it places no orders and changes nothing.") _note(" In paper mode, paper buys spend paper_cash_usdc instead -- not this balance.") lines.append(_blank()) _row("Help mode (this screen)") @@ -457,11 +463,12 @@ def _note(text: str) -> None: _note(" reads only. Neither ever constructs a broker or touches the network.") lines.append(_blank()) _note( - " discover is the SECOND deliberate network exception in this dashboard (after fetch):" + " discover is the THIRD deliberate network exception in this dashboard. The other two" ) - _note(" it never fires on opening the overlay and never fires again on its own while the") - _note(" overlay stays open -- only an explicit Enter, pressed inside it, runs the one live") - _note(" venue call it ever makes.") + _note(" are the automatic ~30s live-balance refresh above and [f] fetch -- three in total,") + _note(" and nothing else here ever leaves this machine. Discover never fires on opening the") + _note(" overlay and never fires again on its own while the overlay stays open -- only an") + _note(" explicit Enter, pressed inside it, runs the one live venue call it ever makes.") lines.append(_blank()) _note( " NONE of screen, propose or discover attests, admits, or trades. They can only PROPOSE" @@ -669,9 +676,10 @@ def build_discover_overlay( ) lines.append( ScreenLine( - "This is the second deliberate network exception in this dashboard (the first " - "is [f] fetch): it never fires just from opening this overlay, and it never " - "fires again on its own while this overlay stays open.", + "This is the third deliberate network exception in this dashboard -- the other " + "two are the automatic ~30s live-balance refresh and [f] fetch. It never fires " + "just from opening this overlay, and it never fires again on its own while this " + "overlay stays open.", "normal", ) ) @@ -987,9 +995,17 @@ def _do_propose_view(open_state: OpenState) -> ProposeView: """Build a fresh `ProposeView` over the newest shortlist in `config.proposals_dir` -- OFFLINE (DB + local filesystem reads only), rebuilt every poll while the propose overlay is open. `_screen_product` is lazy-imported for the identical reason `_do_screen_report` lazy-imports - it. `build_propose_view` itself never raises (see its docstring) -- any exception reaching - this function's caller can only come from `open_state()` (e.g. a locked DB), never from the - shortlist read itself.""" + it. + + `build_propose_view` is fail-soft about the shortlist FILE -- a missing directory, a + permissions error, a non-UTF-8 file, invalid JSON, a malformed top-level shape all come back + as a `status`/`detail` pair. That is not the same as "this function cannot raise", and the + earlier claim that any exception here could only come from `open_state()` was simply wrong: + it once let a `UnicodeDecodeError` from the read itself through (that specific hole is now + closed -- see `build_propose_view`'s own `except (OSError, UnicodeDecodeError)`), and + `build_propose_view` still SCREENS every parsed candidate afterwards, which means real DB + reads through `_screen_product`. A locked DB, mid-screen, surfaces here exactly like one from + `open_state()` does. The caller's `try/except` is load-bearing for both.""" from keel.cli import _screen_product repo, config = open_state() @@ -1026,7 +1042,7 @@ def run_live(open_state: OpenState, now_fn: NowFn, interval: float) -> None: process -- gathers a fresh report, paints it, then waits up to `interval` seconds for a keypress. - Seven modes: `normal` (the dashboard, plus a transient one-line `message` toast from the last + Six modes: `normal` (the dashboard, plus a transient one-line `message` toast from the last action), `help` (a scrolled window of `build_help_screen()`), `insights` (a scrolled window of `build_insights_screen()` -- a READ-ONLY overlay over `build_insights_report`/ `build_journal_report`, rebuilt fresh each poll while open, fail-soft exactly like the @@ -1179,9 +1195,15 @@ def _loop(stdscr: Any) -> None: continue if mode == "propose": - # OFFLINE + fail-soft, same shape as screen above. `_do_propose_view` itself - # never raises (`build_propose_view`'s own docstring) -- an exception here can - # only come from `open_state()` (e.g. a locked DB), never from the shortlist read. + # OFFLINE + fail-soft, same shape as screen above. `build_propose_view` turns + # every SHORTLIST-FILE problem into a calm `status`/`detail` pair rather than an + # exception, so this handler is not what renders "no shortlist yet" or "not valid + # JSON" -- the overlay does. It catches what is left: a locked DB, either from + # `open_state()` or from the per-candidate screening `build_propose_view` runs + # after parsing. It is also the net that caught a `UnicodeDecodeError` escaping + # the read for a non-UTF-8 shortlist -- as an unreadable `'utf-8' codec can't + # decode byte 0xff...` toast, repainted every poll, naming no file. That hole is + # fixed at the source now; this stays as the backstop, not as the explanation. try: propose_lines = build_propose_overlay(_do_propose_view(open_state)) except Exception as exc: diff --git a/keel/compliance/screen.py b/keel/compliance/screen.py index c35d3d00..89577bae 100644 --- a/keel/compliance/screen.py +++ b/keel/compliance/screen.py @@ -269,12 +269,27 @@ def split_failures(facts: MarketFacts, result: ScreenResult) -> tuple[list[str], there is exactly one place either can change. With `facts.daily_bars > 0` every failure is returned as `about_the_asset` and - `about_our_cache` is empty, UNSPLIT -- including a genuine `history` shortfall, because an - asset with some cached history that still falls short of the floor really is too young, and - that verdict must not be silenced. The split only ever happens at EXACTLY zero bars, which is - the one condition where `history` (and `liquidity`) measure our cache instead of the asset. - Original ordering is preserved within each returned list, so a caller that renders them in - order does not see failures reshuffled relative to how `screen_asset` produced them. + `about_our_cache` is empty, UNSPLIT -- including a `history` shortfall. + + Be clear about what that does and does not mean. It does NOT mean a shallow cache proves the + asset is young. `MarketFacts` carries a bar COUNT and no first-bar timestamp, so this function + cannot distinguish "listed 18 months ago" from "we fetched an 18-month window": `keel fetch + --years 2`, a fetch that aborted partway, or a venue that simply does not serve the full + window all leave an OLD asset shallow (`keel/cli.py`'s `fetch` prints a note about exactly + that -- "some series are still short... an asset younger than the requested window"). Every + surface nonetheless renders `✗ history: 730 daily bars < 1460 required` as a verdict, because + a partial cache is genuinely ambiguous and the gate must fail closed on ambiguity rather than + admit on it. + + So the operator reading a non-zero `history` failure should CHECK THE FETCH WINDOW before + concluding the asset is too young: `keel fetch --products --years 5`, then re-screen. If + the count does not move, it is the asset. + + The split is confined to EXACTLY zero bars because that is the only count where there is no + ambiguity to resolve: with no candles at all, `history` and `liquidity` are reporting the + emptiness of our cache and nothing whatsoever about the asset. Original ordering is preserved + within each returned list, so a caller that renders them in order does not see failures + reshuffled relative to how `screen_asset` produced them. """ if facts.daily_bars > 0: return list(result.failures), [] @@ -304,11 +319,19 @@ def missing_history_lines(product_id: str, not_assessable: Sequence[str]) -> lis after the colon. Tags are deduplicated and sorted so two failures sharing a tag collapse to one mention and the order is stable regardless of how `screen_asset` happened to emit them. """ + # The second line is the SEMANTIC one -- it says what a zero-bar report means -- and it is + # deliberately agnostic about the asset. It used to read "it is not too young, we have simply + # never fetched candles for it", which asserts something we cannot know: at exactly zero bars + # a listing three days old and one three years old are the same input, since `MarketFacts` + # carries no first-bar timestamp. Refusing to rule is the honest answer, and it is also the + # useful one -- it points at the fetch, which is the only thing that can resolve it. + # `test_missing_history_lines_claim_nothing_about_the_assets_age` pins this. lines = [ f"no local history for {product_id} -- run `keel fetch --products {product_id}` first, " "then re-screen.", - "This is a MISSING-DATA verdict, not a verdict about the asset: it is not too young, " - "we have simply never fetched candles for it.", + "This is a MISSING-DATA verdict, not a verdict about the asset: with no candles at all " + "we cannot tell a genuinely young asset from one we have simply never fetched, so this " + "says nothing about its age either way.", ] if not_assessable: tags = sorted({f.split(":")[0] for f in not_assessable}) diff --git a/keel/proposer.py b/keel/proposer.py index 0838fd33..ddac293e 100644 --- a/keel/proposer.py +++ b/keel/proposer.py @@ -225,8 +225,27 @@ def render_proposal_report(report: ProposalReport) -> list[str]: def report_to_jsonable(report: ProposalReport) -> dict[str, Any]: - return { - "screened": [ + """The `--json` shape -- and it must tell the SAME story `render_proposal_report` tells. + + It used to emit `sc.result.failures` raw, so at zero cached bars the payload carried + `history: 0 daily bars < 1460 required` as a plain failure while every human surface + (`render_proposal_report` two functions up, `assets holdings --screen`, `assets screen`, the + TUI's screen/propose overlays) suppressed that exact line and printed the MISSING-DATA + explanation instead. One report, two surfaces, two different answers to "is this asset too + young?" -- the drift `screen.split_failures` exists to end, surviving in the one surface a + script reads and cannot argue with. + + So the same `split_failures` the renderers call decides this too, and the split is made + EXPLICIT rather than merely applied: `failures` is what the human report prints with a `✗` + (verdicts about the asset), `not_assessable` is what it withholds at zero bars (statements + about the emptiness of OUR cache -- suppressed, never dropped, so a consumer that wants them + can still read them correctly labelled), and `missing_history` is the flag that says which + regime the row is in without the consumer having to infer it from `daily_bars == 0`. + """ + rows: list[dict[str, Any]] = [] + for sc in report.screened: + failures, not_assessable = split_failures(sc.facts, sc.result) + rows.append( { "asset": sc.candidate.asset, "product": sc.product, @@ -238,11 +257,14 @@ def report_to_jsonable(report: ProposalReport) -> dict[str, Any]: "admitted": sc.result.admitted, "summary": sc.result.summary, "daily_bars": sc.facts.daily_bars, - "failures": sc.result.failures, + "missing_history": sc.facts.daily_bars == 0, + "failures": failures, + "not_assessable": not_assessable, "warnings": sc.result.warnings, } - for sc in report.screened - ], + ) + return { + "screened": rows, "invalid": [{"reason": e.reason, "raw": e.raw} for e in report.invalid], "admitted_count": report.admitted_count, } diff --git a/tests/commands/test_admission.py b/tests/commands/test_admission.py index 74d70382..cff550d2 100644 --- a/tests/commands/test_admission.py +++ b/tests/commands/test_admission.py @@ -332,6 +332,69 @@ def test_build_propose_view_unreadable_file_is_fail_soft(repo: Repository, tmp_p assert view.report is None +def test_build_propose_view_non_utf8_shortlist_is_fail_soft(repo: Repository, tmp_path): + """A UTF-16LE+BOM shortlist -- valid JSON, and exactly what a scout run on a Windows box + writes -- makes `source.read_text()` raise `UnicodeDecodeError`, which subclasses + **ValueError, not OSError** (`issubclass(UnicodeDecodeError, OSError)` is False). The original + `except OSError` therefore let it escape a function whose docstring promises it never raises, + which in the TUI meant the propose overlay repainting `propose read failed: 'utf-8' codec + can't decode byte 0xff...` every poll forever, naming neither the file nor a next step. The + fail-soft branch must own this exactly like it owns a permissions error.""" + config = _config() + directory = tmp_path / "proposals" + directory.mkdir() + path = directory / "shortlist.json" + path.write_bytes(json.dumps({"candidates": [_SOL_CANDIDATE]}).encode("utf-16")) + + view = build_propose_view(repo, config, cli_module._screen_product, directory=directory) + + assert view.status == "unreadable" + assert view.source == path + assert view.detail is not None + assert str(path) in view.detail # the operator is told WHICH file, unlike the raw traceback + assert view.report is None + + +def test_build_propose_view_defaults_the_directory_to_config_proposals_dir( + repo: Repository, tmp_path, monkeypatch: pytest.MonkeyPatch +): + """Every other test in this file passes `directory=` explicitly, so the `directory is None` + branch -- `Path(config.proposals_dir).expanduser()` -- was never executed by the suite at all: + hardcoding any path there left 2062 tests green. It resolves to `~/keel/proposals` by default, + INSIDE the live deployment root, so a silent regression means the overlay quietly reads a + right-looking wrong place. `HOME` is redirected at `tmp_path` so this never touches a real + deployment.""" + monkeypatch.setenv("HOME", str(tmp_path)) + monkeypatch.setenv("USERPROFILE", str(tmp_path)) # `expanduser` on Windows reads this instead + config = _config(proposals_dir="~/scout/shortlists") + shortlist = _write_shortlist(tmp_path / "scout" / "shortlists", [_SOL_CANDIDATE]) + + view = build_propose_view(repo, config, cli_module._screen_product) + + assert view.status == "ok" + assert view.source == shortlist + + +def test_build_propose_view_default_proposals_dir_is_keel_proposals_under_home( + repo: Repository, tmp_path, monkeypatch: pytest.MonkeyPatch +): + """The other half of the same branch: with `proposals_dir` left at its own default, the + directory actually looked in is `/keel/proposals` -- `~` EXPANDED. A missing + `.expanduser()` would look in a literal `./~/keel/proposals` relative to the cwd, find + nothing, and report `no proposals directory` forever while the operator's real shortlists sat + untouched. Pinned via the reported path, which the overlay shows verbatim.""" + monkeypatch.setenv("HOME", str(tmp_path)) + monkeypatch.setenv("USERPROFILE", str(tmp_path)) + config = _config() + assert config.proposals_dir == DEFAULT_PROPOSALS_DIR # guards the premise of this test + + view = build_propose_view(repo, config, cli_module._screen_product) + + assert view.status == "no-directory" + assert view.detail is not None + assert str(tmp_path / "keel" / "proposals") in view.detail + + def test_build_propose_view_invalid_json_is_fail_soft(repo: Repository, tmp_path): config = _config() directory = tmp_path / "proposals" diff --git a/tests/commands/test_tui.py b/tests/commands/test_tui.py index eaabf43a..75c00ee1 100644 --- a/tests/commands/test_tui.py +++ b/tests/commands/test_tui.py @@ -9,6 +9,7 @@ from __future__ import annotations +import json import sqlite3 import sys import time @@ -19,6 +20,7 @@ import pytest from click.testing import CliRunner +import keel.commands.tui as tui_mod from keel.cli import cli from keel.commands.admission import DiscoverReport from keel.commands.insights import ( @@ -40,6 +42,8 @@ SubscriptionStatusRow, ) from keel.commands.tui import ( + _BALANCE_TIMEOUT_SEC, + _DISCOVER_TIMEOUT_SEC, _REFRESH_MESSAGE, _SHORT_VERSION, AvailableBalance, @@ -69,6 +73,7 @@ run_live, run_once, toggle_autonomy, + tui_cmd, ) from keel.compliance import screen as screen_mod from keel.config import ( @@ -81,7 +86,7 @@ ) from keel.data.db import connect, migrate from keel.data.repository import Repository -from keel.types import Granularity +from keel.types import Candle, Granularity NOW_TS = 1_800_000_000 @@ -960,6 +965,143 @@ def open_state() -> tuple[Repository, Any]: assert dashboard_after_idx > propose_idx +def _seed_daily_history(repo: Repository, product: str, bars: int) -> None: + """Enough cached daily bars for `history` and `liquidity` to PASS the screen outright + (`volume * close` = 10,000,000 per bar, well over the 1,000,000 median floor), so a REJECT + from the gate can only be the shariah criterion the asset has no attestation for.""" + repo.upsert_candles( + product, + Granularity.ONE_DAY, + [ + Candle( + ts=i * 86400, + open=Decimal("100"), + high=Decimal("101"), + low=Decimal("99"), + close=Decimal("100"), + volume=Decimal("100000"), + ) + for i in range(bars) + ], + ) + + +def test_run_live_screen_overlay_paints_the_real_verdict_from_the_single_gate( + repo: Repository, monkeypatch: pytest.MonkeyPatch +) -> None: + """Design constraint 4 -- every candidate routes through `keel.cli._screen_product`, "so + nothing drifts onto a laxer gate" -- was convention-only exactly where it is WIRED. Replacing + `_screen_product` with an always-ADMIT stub in `_do_screen_report` left the entire suite + green, because the s/p overlay tests only assert that a title paints and Esc closes; not one + of them ever looked at a verdict. + + So: BTC is seeded with ample history and liquidity but is never attested. The only thing that + can reject it is the shariah criterion, which only the real gate applies -- `screen_asset` + fails CLOSED on `attestation=None`. The overlay must therefore paint `REJECT` and name + `attestation: MISSING`. An always-ADMIT stub paints `ADMIT` with no failure lines and kills + both assertions.""" + config = _config(allowlist=["BTC"]) + _seed_daily_history(repo, "BTC-USD", 1500) + keys = [ord("s"), -1, 27] + stdscr = _KeySequenceStdscr(height=40, width=120, keys=keys) + + fake_curses = _fake_curses() + fake_curses.wrapper = lambda fn: fn(stdscr) + monkeypatch.setitem(sys.modules, "curses", fake_curses) + + def open_state() -> tuple[Repository, Any]: + return repo, config + + run_live(open_state, lambda: NOW_TS, interval=0.01) + + painted_texts = [call[2] for call in stdscr.calls] + overlay_idx = next(i for i, t in enumerate(painted_texts) if "keel tui -- screen" in t) + after = painted_texts[overlay_idx:] + assert any(t.startswith("REJECT") and "BTC" in t for t in after) + assert any("attestation: MISSING" in t for t in after) + # The premise: the data criteria really did pass, so REJECT above is the shariah gate's doing + # and not an incidental history/liquidity shortfall that any stub would also produce. + assert not any("✗ history" in t or "✗ liquidity" in t for t in after) + + +def test_run_live_propose_overlay_paints_the_real_verdict_from_the_single_gate( + repo: Repository, monkeypatch: pytest.MonkeyPatch, tmp_path +) -> None: + """The `p` half of the same wiring gap the `s` test above closes -- `_do_propose_view` passes + `_screen_product` into `build_propose_view` on the same convention-only basis, and swapping it + for an always-ADMIT stub was equally invisible to the suite. + + SOL is shortlisted with ample cached history and liquidity but no attestation, so the only + thing that can reject it is `screen_asset` failing CLOSED on `attestation=None` -- something + only the real gate does.""" + proposals = tmp_path / "proposals" + proposals.mkdir() + (proposals / "shortlist.json").write_text( + json.dumps( + {"candidates": [{"asset": "SOL", "rationale": "r", "sources": ["https://x.invalid"]}]} + ) + ) + config = _config(proposals_dir=str(proposals)) + _seed_daily_history(repo, "SOL-USD", 1500) + keys = [ord("p"), -1, 27] + stdscr = _KeySequenceStdscr(height=40, width=200, keys=keys) + + fake_curses = _fake_curses() + fake_curses.wrapper = lambda fn: fn(stdscr) + monkeypatch.setitem(sys.modules, "curses", fake_curses) + + def open_state() -> tuple[Repository, Any]: + return repo, config + + run_live(open_state, lambda: NOW_TS, interval=0.01) + + painted_texts = [call[2] for call in stdscr.calls] + overlay_idx = next(i for i, t in enumerate(painted_texts) if "keel tui -- propose" in t) + after = painted_texts[overlay_idx:] + assert any(t.startswith("REJECT") and "SOL" in t for t in after) + assert any("attestation: MISSING" in t for t in after) + assert any("keel assets attest SOL" in t for t in after) # the next step, not just a verdict + # The premise: the data criteria really did pass, so REJECT is the shariah gate's doing. + assert not any("✗ history" in t or "✗ liquidity" in t for t in after) + + +def test_run_live_propose_overlay_reports_a_non_utf8_shortlist_calmly( + repo: Repository, monkeypatch: pytest.MonkeyPatch, tmp_path +) -> None: + """The TUI half of the `UnicodeDecodeError`-is-not-an-`OSError` fix. A UTF-16LE+BOM shortlist + escaped `build_propose_view`'s fail-soft branches entirely and was caught only by the propose + branch's broad `except Exception`, which repainted `propose read failed: 'utf-8' codec can't + decode byte 0xff...` on every poll forever -- naming no file and offering no next step. It + must render as the same calm, actionable `unreadable` overlay a permissions error renders + as.""" + proposals = tmp_path / "proposals" + proposals.mkdir() + (proposals / "shortlist.json").write_bytes( + json.dumps( + {"candidates": [{"asset": "SOL", "rationale": "r", "sources": ["https://x.invalid"]}]} + ).encode("utf-16") + ) + config = _config(proposals_dir=str(proposals)) + keys = [ord("p"), -1, 27] + # Wide enough that `_paint`'s clip-to-window-width does not truncate the tmp_path before the + # filename this test is about -- the clipping is real terminal behaviour, not the bug here. + stdscr = _KeySequenceStdscr(height=40, width=400, keys=keys) + + fake_curses = _fake_curses() + fake_curses.wrapper = lambda fn: fn(stdscr) + monkeypatch.setitem(sys.modules, "curses", fake_curses) + + def open_state() -> tuple[Repository, Any]: + return repo, config + + run_live(open_state, lambda: NOW_TS, interval=0.01) + + painted_texts = [call[2] for call in stdscr.calls] + assert any("could not read the shortlist file" in t for t in painted_texts) + assert any("shortlist.json" in t for t in painted_texts) # WHICH file, by name + assert not any("propose read failed" in t for t in painted_texts) + + def test_run_live_screen_survives_transient_read_error_and_keeps_polling( repo: Repository, monkeypatch: pytest.MonkeyPatch ) -> None: @@ -1202,11 +1344,21 @@ def open_state() -> tuple[Repository, Any]: assert any(i > failed_idx and "paper mode" in t for i, t in enumerate(painted_texts)) -def test_run_live_discover_closing_discards_the_held_result( +def test_run_live_discover_reopening_after_a_run_is_armed_not_stale( repo: Repository, monkeypatch: pytest.MonkeyPatch ) -> None: - """Closing the overlay (Esc) must discard the held result -- reopening it must be armed but - not yet run again, not silently show the previous run's stale candidates.""" + """Reopening the overlay after a run must be ARMED again, never a silent repaint of the + previous run's stale candidates. + + NAMED for what it actually pins, which is a DISJUNCTION, not a single line. Two independent + clears stand between the held result and the reopened overlay -- the close branch's + (`run_live`, discover mode, `q`/`Esc`/`d`) and the normal-mode `d` branch's self-labelled + belt-and-braces one -- and reaching the reopened overlay necessarily runs BOTH. Deleting + either one alone leaves this test green. It was previously called + `..._closing_discards_the_held_result`, which claimed to pin the close branch specifically; + nothing observable from outside `run_live` can distinguish the two, because `mode` only ever + becomes `discover` via the normal-mode `d` branch that also clears. Keeping both clears is + deliberate defence in depth; this test guards the property they jointly provide.""" config = _config() class _FakeBroker: @@ -1259,6 +1411,52 @@ def open_state() -> tuple[Repository, Any]: ) +def test_run_live_discover_bounds_its_one_network_call_with_the_discover_timeout( + repo: Repository, monkeypatch: pytest.MonkeyPatch +) -> None: + """`_DISCOVER_TIMEOUT_SEC` was correctly wired but had no test: dropping the `timeout=` kwarg + (or reusing `_BALANCE_TIMEOUT_SEC`) left the suite green. It exists because the operator waits + on this call with the screen frozen behind a "contacting venue" frame, so a hung connection + must fail and say so rather than freeze the dashboard until Ctrl-C. Pinned per-call, not + globally: the unrelated balance refresh in the same run uses its OWN, shorter bound, and this + test would not notice the two being collapsed into one if it only checked "some timeout was + passed".""" + config = _config() + timeouts: list[tuple[str, Any]] = [] + + class _FakeBroker: + def get_accounts(self) -> list[Any]: + return [] + + def list_products(self) -> list[dict]: + return [] + + def _fake_build_broker(cfg: Any, timeout: int | None = None) -> _FakeBroker: + timeouts.append(("build", timeout)) + return _FakeBroker() + + monkeypatch.setattr("keel.commands._common._build_broker", _fake_build_broker) + + # poll1: normal -> 'd'. poll2: discover ARMED -> Enter runs the one call. poll3: Esc closes. + keys = [ord("d"), 10, 27] + stdscr = _KeySequenceStdscr(height=24, width=80, keys=keys) + + fake_curses = _fake_curses() + fake_curses.wrapper = lambda fn: fn(stdscr) + monkeypatch.setitem(sys.modules, "curses", fake_curses) + + def open_state() -> tuple[Repository, Any]: + return repo, config + + run_live(open_state, lambda: NOW_TS, interval=0.01) + + passed = [t for _, t in timeouts] + assert _DISCOVER_TIMEOUT_SEC in passed + # The balance refresh (the other broker build in this run) keeps its own, distinct bound. + assert _BALANCE_TIMEOUT_SEC in passed + assert _DISCOVER_TIMEOUT_SEC != _BALANCE_TIMEOUT_SEC + + def test_run_live_read_error_does_not_swallow_keyboard_interrupt( repo: Repository, monkeypatch: pytest.MonkeyPatch ) -> None: @@ -1296,11 +1494,61 @@ def test_build_help_screen_documents_discover_network_gating_and_attest_is_cli_o allowlist -- is never reachable from here.""" lines = build_help_screen() text = " ".join(line.text.lower() for line in lines) - assert "second deliberate network exception" in text + assert "third deliberate network exception" in text assert "cli-only" in text assert "keel assets attest" in text +#: Every operator-facing place that counts this dashboard's network touches. The count is a +#: SAFETY claim -- an operator deciding whether a keypress can reach the venue reads it and stops +#: looking -- so an undercount is a bug, not a typo, and it must be pinned wherever it is stated. +_NETWORK_COUNT_SURFACES = { + "module docstring": tui_mod.__doc__ or "", + "tui_cmd --help": tui_cmd.__doc__ or "", + "help overlay": "\n".join(line.text for line in build_help_screen()), + "ARMED discover overlay": "\n".join( + line.text for line in build_discover_overlay(None) + ), +} + + +@pytest.mark.parametrize("surface", sorted(_NETWORK_COUNT_SURFACES)) +def test_no_surface_undercounts_the_dashboards_network_touches(surface: str) -> None: + """`run_live` touches the network in exactly THREE places: the automatic ~30s live-balance + refresh (`_refresh_balance` -> `get_accounts`), `f` fetch, and `d`+Enter. Three surfaces -- + including the operator-facing ARMED overlay, read at the moment of deciding whether to make + a live call -- used to call discover "the SECOND deliberate network exception", silently + forgetting the balance refresh that had been firing every 30 seconds since v3. Only + `tui_cmd`'s own docstring had it right. An operator who trusts "second" concludes that + closing the overlay leaves the dashboard offline; it does not.""" + text = _NETWORK_COUNT_SURFACES[surface].lower() + assert "second deliberate network exception" not in text + # Either phrasing states the true count -- three surfaces frame it from discover's side + # ("the THIRD..."), `tui_cmd --help` from the dashboard's ("there are exactly three"). + assert "third deliberate network exception" in text or "exactly three" in text + # Naming the other two is what makes the count checkable rather than a bare number, and it is + # specifically the balance refresh that every undercount forgot. + assert "balance" in text + assert "fetch" in text + + +def test_help_says_the_live_balance_line_is_itself_a_venue_call() -> None: + """The help's own Safety notes tell the operator that screen and propose are offline and that + fetch/discover are the exceptions -- but the "Live balance" section only ever said the number + is "refreshed every ~30s", never that refreshing it CONTACTS THE VENUE. That omission is what + made "second deliberate network exception" read as plausible three sections later.""" + balance_section: list[str] = [] + lines = build_help_screen() + start = next(i for i, line in enumerate(lines) if line.text.strip() == "Live balance") + for line in lines[start:]: + if not line.text.strip(): + break + balance_section.append(line.text.lower()) + text = " ".join(balance_section) + assert "live call" in text or "network" in text + assert "get_accounts" in text + + def test_build_help_screen_is_longer_than_a_small_terminal() -> None: lines = build_help_screen() assert len(lines) > 24 diff --git a/tests/compliance/test_assets_cli.py b/tests/compliance/test_assets_cli.py index 99da78e1..7e57d21f 100644 --- a/tests/compliance/test_assets_cli.py +++ b/tests/compliance/test_assets_cli.py @@ -1100,6 +1100,29 @@ def test_propose_json_is_valid_and_has_no_trailing_prose(tmp_path, valid_config_ assert payload["screened"][0]["asset"] == "SOL" +def test_propose_json_tells_the_same_zero_bar_story_the_human_output_tells( + tmp_path, valid_config_path +): + """End-to-end companion to `test_zero_cached_bars_never_prints_a_history_depth_failure_via_ + propose`, through the REAL gate: the human surface suppresses `history: 0 daily bars < 1460 + required` and prints the MISSING-DATA explanation, so `--json` -- the surface a script trusts + -- must not hand back that same line as an unflagged verdict about the asset.""" + db_path = tmp_path / "t.db" + _repo_at(db_path) # no candles seeded -- SOL has zero cached bars + shortlist = _write_shortlist(tmp_path, [_SOL]) + + result = CliRunner().invoke( + cli, ["--db", str(db_path), "--config", str(valid_config_path), + "assets", "propose", "--from", str(shortlist), "--json"], + ) + + row = json.loads(result.output)["screened"][0] + assert row["daily_bars"] == 0 + assert row["missing_history"] is True + assert not any(f.startswith("history") for f in row["failures"]) + assert any(f.startswith("history") for f in row["not_assessable"]) + + def test_propose_missing_file_is_a_clean_error(tmp_path, valid_config_path): db_path = tmp_path / "t.db" _repo_at(db_path) @@ -1110,6 +1133,29 @@ def test_propose_missing_file_is_a_clean_error(tmp_path, valid_config_path): assert result.exit_code != 0 +def test_propose_non_utf8_shortlist_is_a_clean_error_not_a_traceback(tmp_path, valid_config_path): + """`UnicodeDecodeError` subclasses **ValueError, not OSError**, so the original + `except (OSError, json.JSONDecodeError)` did not catch it: a UTF-16LE+BOM shortlist (valid + JSON, and what a scout run on a Windows box writes) crashed out of `assets propose` with a + raw traceback instead of the `could not read/parse ` message every other unreadable + input gets. Exit code alone is not enough here -- an uncaught exception also exits non-zero, + which is why the message and the absence of a traceback are both pinned.""" + db_path = tmp_path / "t.db" + _repo_at(db_path) + shortlist = tmp_path / "shortlist.json" + shortlist.write_bytes(json.dumps({"candidates": [_SOL]}).encode("utf-16")) + + result = CliRunner().invoke( + cli, ["--db", str(db_path), "--config", str(valid_config_path), + "assets", "propose", "--from", str(shortlist)], + ) + + assert result.exit_code != 0 + assert result.exception is None or isinstance(result.exception, SystemExit) + assert "could not read/parse" in result.output + assert str(shortlist) in result.output + + def test_propose_hypothesis_never_admits(tmp_path, valid_config_path): db_path = tmp_path / "t.db" _repo_at(db_path) diff --git a/tests/compliance/test_screen.py b/tests/compliance/test_screen.py index fe65d127..32c28b75 100644 --- a/tests/compliance/test_screen.py +++ b/tests/compliance/test_screen.py @@ -485,6 +485,42 @@ def test_missing_history_lines_omits_the_third_line_when_nothing_is_suppressed() assert not any("not assessable" in line for line in lines) +def test_missing_history_lines_claim_nothing_about_the_assets_age(): + """The SEMANTIC sentence -- the one that says what a zero-bar report actually means -- was + pinned by nothing. Every test around it asserted proxies at the call sites (`"no local + history" in text`, `"✗ history" not in text`), so the sentence could be reworded into + anything and the suite stayed green. + + It used to read "it is not too young, we have simply never fetched candles for it", which + asserts a fact about the ASSET this function cannot possibly know: at exactly zero bars a + brand-new listing and a never-fetched veteran are the same input, and `MarketFacts` carries + no first-bar timestamp to tell them apart. Refusing to rule is the honest position, and it is + the one the explanation must state.""" + lines = missing_history_lines("SOL-USD", ["history: 0 daily bars < 1460 required"]) + semantic = lines[1] + + assert "MISSING-DATA verdict, not a verdict about the asset" in semantic + assert "cannot tell" in semantic + assert "not too young" not in semantic + + +def test_missing_history_lines_never_restate_a_suppressed_failure_verbatim(): + """These lines are shown INSTEAD OF `not_assessable`, so leaking one back in verbatim would + reprint the very depth verdict the suppression exists to withhold -- and the call-site proxy + `"✗ history" not in text` would not catch it, because the leak carries no `✗`. Only the TAGS + survive, on the third line.""" + suppressed = [ + "history: 0 daily bars < 1460 required", + "liquidity: median daily volume 0 < 1000000 required", + ] + joined = "\n".join(missing_history_lines("SOL-USD", suppressed)) + + for failure in suppressed: + assert failure not in joined + assert "1460" not in joined + assert "0 daily bars" not in joined + + def test_missing_history_lines_dedupes_and_sorts_tags(): """Two failures sharing a tag collapse to one mention, and the tags print in a stable, predictable order rather than whatever order `screen_asset` happened to emit them in.""" diff --git a/tests/test_proposer.py b/tests/test_proposer.py index f4b89c9c..a3324056 100644 --- a/tests/test_proposer.py +++ b/tests/test_proposer.py @@ -266,6 +266,58 @@ def test_jsonable_is_json_serializable_and_has_keys(): assert "shariah_hypothesis" in row +def _report_with_real_failure_strings(bars): + """A report whose failure strings are shaped like `screen_asset`'s real ones, so a test can + assert on the `history:` depth line the JSON surface used to leak. `_report` above emits no + `history` failure at zero bars, which is precisely why this asymmetry went unnoticed.""" + parsed = parse_proposal({"candidates": [_entry(asset="SOL")]}) + + def screen_fn(repo, product, quote): + facts = screen_mod.MarketFacts("SOL", bars, Decimal("0"), True, "SOL-USD") + failures = [ + f"history: {bars} daily bars < 1460 required", + "liquidity: median daily volume 0 < 1000000 required", + "attestation: MISSING. Sector and backing cannot be derived from price data", + ] + return facts, screen_mod.ScreenResult("SOL", admitted=False, failures=failures) + + return build_proposal_report(parsed, _repo(), "USD", [], screen_fn) + + +def test_jsonable_flags_zero_bar_failures_exactly_like_the_human_report_does(): + """`--json` emitted `sc.result.failures` RAW, so at zero cached bars the payload carried + `history: 0 daily bars < 1460 required` with nothing marking it as a fact about OUR CACHE -- + while `render_proposal_report`, `assets holdings --screen`, `assets screen` and the TUI + overlay all suppress that exact line and print the MISSING-DATA explanation instead. One + report, two surfaces, two different stories: the very drift `split_failures` was created to + end. `--json` is the surface a script trusts, so it is the worst one to leave unflagged.""" + payload = report_to_jsonable(_report_with_real_failure_strings(bars=0)) + row = payload["screened"][0] + + assert row["missing_history"] is True + assert not any(f.startswith("history") for f in row["failures"]) + assert not any(f.startswith("liquidity") for f in row["failures"]) + assert any(f.startswith("attestation") for f in row["failures"]) + # Suppressed, never DROPPED -- a consumer that wants them can still read them, correctly + # labelled as not-assessable-until-fetched rather than as verdicts about the asset. + assert sorted(f.split(":")[0] for f in row["not_assessable"]) == ["history", "liquidity"] + assert set(row["failures"]) | set(row["not_assessable"]) == { + f for f in _report_with_real_failure_strings(bars=0).screened[0].result.failures + } + + +def test_jsonable_leaves_a_nonzero_bar_history_failure_in_failures(): + """The other side of the split, so the fix cannot become "always hide `history`": with ANY + cached bars nothing moves, `missing_history` is False, and the depth failure stays a verdict + in `failures` where a consumer will act on it.""" + payload = report_to_jsonable(_report_with_real_failure_strings(bars=400)) + row = payload["screened"][0] + + assert row["missing_history"] is False + assert row["not_assessable"] == [] + assert any(f.startswith("history") for f in row["failures"]) + + def test_data_derived_failures_tags_actually_match_screen_asset_output(): """Pins the shared `DATA_DERIVED_FAILURES` constant to what `screen_asset` actually emits.