From a56481f7dfa0712329411e4f4bfaba6759b77598 Mon Sep 17 00:00:00 2001 From: Elmehdi Aitbrahim Date: Tue, 21 Jul 2026 19:47:28 -0400 Subject: [PATCH 1/4] docs(spec): broker holdings as a candidate source (item 3) A new SOURCE, not a new gate: the user's Coinbase balances feed the existing fail-closed screen. Extracts one _screen_product helper so discover, holdings and a future LLM proposer cannot diverge onto a laxer path. Co-Authored-By: Claude Opus 4.8 (1M context) --- ...-21-holdings-as-candidate-source-design.md | 129 ++++++++++++++++++ 1 file changed, 129 insertions(+) create mode 100644 docs/superpowers/specs/2026-07-21-holdings-as-candidate-source-design.md diff --git a/docs/superpowers/specs/2026-07-21-holdings-as-candidate-source-design.md b/docs/superpowers/specs/2026-07-21-holdings-as-candidate-source-design.md new file mode 100644 index 00000000..00d17f83 --- /dev/null +++ b/docs/superpowers/specs/2026-07-21-holdings-as-candidate-source-design.md @@ -0,0 +1,129 @@ +# Broker holdings as a candidate source — design + +**Date:** 2026-07-21 +**Status:** Approved design (pending user review) +**Workstream:** C of three. Item 3 of the 2026-07-21 requirements. + +## Context + +> "Fetch user's list of assets to trade from his broker, coinbase for the time being, then vet each +> of them through our rules. In the future, our LLM will propose a list of assets to trade and must +> go through the same vetting process and test simulations before adding to the user's asset list." + +The vetting gate **already exists** and is the valuable part of this codebase's compliance work: + +- `compliance/screen.screen_asset(facts, attestation, policy) -> ScreenResult` — deterministic + admission. Checks history depth (4y of daily bars), liquidity (median daily volume), settlement + in the quote currency, and the **attested** shariah classification (sector / backing / yield). + **`attestation=None` fails closed** — sector and backing cannot be derived from price data, so an + unclassified asset is *unknown*, and unknown is a rejection. +- `keel assets discover` proposes candidates from **venue-wide** metadata (~936 products → a + shortlist), `keel assets screen` vets, `keel assets attest` records a human classification with a + source, `keel assets list` shows what has been attested. + +What is missing is a **source**: the user's own Coinbase holdings. Today the only proposer is +venue-wide discovery, which answers "what could anyone trade?" rather than "what do *I* already +hold that this system might trade?". + +## Goals + +1. **A new candidate source, not a new gate.** Fetch the assets the user actually holds at the + broker and run each through the *existing, unmodified* `screen_asset` path. +2. **Make "held" and "admitted" impossible to confuse.** Holding an asset is not a reason to trade + it. The command admits nothing. +3. **Leave a clean seam for the LLM proposer**, which must enter the same gate rather than a + parallel one. + +## Non-goals + +- **No change to `screen_asset`, `ScreenPolicy`, or the attestation requirement.** If this work + needed to weaken the gate to admit the user's own holdings, that would be evidence against the + holdings, not against the gate. +- No automatic attestation. A holding is not a classification; a human still records sector and + backing with a source. +- No automatic allowlist mutation. `config.allowlist` stays a deliberate human edit. +- The LLM proposer itself is **not** built here (it is off-by-default, API-key-gated, and needs its + own design). This spec only ensures the seam it must use exists. + +## Design + +### 3.1 `keel assets holdings` + +``` +keel assets holdings [--min-balance 0] [--screen] [--quote USDC] +``` + +1. Fetch `broker.get_accounts()` (already implemented, read-only, proven against the live API). +2. Derive the candidate set: accounts with `available_balance > --min-balance`, **excluding** the + settlement/quote currency and fiat — you cannot "trade" the currency you settle in, and listing + it as a rejected candidate is noise, not information. +3. For each remaining asset, report: + - **held** balance; + - whether it is on the current `config.allowlist`; + - whether an attestation exists (`repo.get_asset_attestation`); + - with `--screen`, the full `ScreenResult` — the same `ADMIT`/`REJECT` plus failure reasons that + `keel assets screen` prints. + +The command **never** writes: no attestation, no allowlist change, no DB mutation. It is a +read-only report, in the same family as `assets discover`. + +### 3.2 One gate, shared by construction + +`assets_screen` currently inlines the attestation lookup, the `AssetAttestation` construction and +the `screen_asset` call. That block is extracted to: + +```python +def _screen_product(repo, product, quote) -> screen_mod.ScreenResult +``` + +and **both** `assets screen` and `assets holdings --screen` call it. This is the mechanism that +makes "the same vetting process" true rather than aspirational: there is one call site of +`screen_asset` behind one helper, so a future LLM-proposed candidate cannot accidentally get a +laxer path. A test asserts both commands produce the same verdict for the same asset. + +### 3.3 "No local history" is not the same as "bad asset" + +`_market_facts` computes `daily_bars` from **cached candles in the local DB**. A newly-surfaced +holding will usually have none, so the screen rejects it for insufficient history — which is +correct (we genuinely cannot validate a rule on data we do not have) but easy to misread as "this +asset is unsuitable". + +So when `daily_bars == 0`, the output says so explicitly and names the fix: + +``` +REJECT SOL no local history -- run `keel fetch --products SOL-USDC` first, + then re-screen. This is a MISSING-DATA verdict, not a verdict about the asset. +``` + +This distinction is the single most likely misreading of the feature, so it is handled in the +output rather than left to the operator. + +### 3.4 The seam for the LLM proposer + +A proposer is anything that produces a list of asset codes. `assets discover` (venue metadata) and +`assets holdings` (broker balances) are the two implemented ones; an LLM proposer is a third. All +three converge on `_screen_product`, and none of them can admit anything — admission requires a +human `keel assets attest` plus a passing screen plus a deliberate `config.allowlist` edit. + +Recorded here so the LLM work inherits it: per the project's §5 asymmetry, an LLM may *propose* and +may *veto*, but may never *admit*. Nothing in this spec grants a proposer new authority. + +## Components + +| File | Change | +|---|---| +| `keel/cli.py` | extract `_screen_product`; new `assets holdings` command | +| `tests/compliance/test_assets_cli.py` (or nearest existing) | new tests | + +No new modules, no schema change, no new dependency. + +## Testing + +- Holdings are fetched from the broker and the quote currency / fiat are excluded. +- `--min-balance` filters dust. +- A held asset that is unattested is reported **REJECT** (fail closed) — holding it changes nothing. +- A held asset already on the allowlist is marked as such. +- `--screen` produces the *same* verdict as `keel assets screen` for the same asset (one gate). +- `daily_bars == 0` produces the explicit missing-data message, not a bare rejection. +- The command writes nothing: no attestation row, no allowlist change, no DB mutation. +- Broker failure is reported as an error, not as an empty (and therefore falsely clean) result. From 7ca12752c548f7a07476f41e3ece51178a9e7b4b Mon Sep 17 00:00:00 2001 From: Elmehdi Aitbrahim Date: Tue, 21 Jul 2026 19:50:03 -0400 Subject: [PATCH 2/4] feat(assets): the user's broker holdings as a candidate source 'keel assets holdings [--min-balance N] [--screen]' answers 'what do I already own that this system might trade?', where 'assets discover' answers 'what could anyone trade?'. A SOURCE, not a gate. Holding an asset is not a reason to trade it, so this admits nothing and writes nothing -- no attestation, no allowlist change, no DB write. Extracts _screen_product so 'assets screen', 'assets holdings --screen' and any future proposer (an LLM shortlist) share ONE call site of screen_asset; a test pins that the two commands agree on the same asset, which is what makes 'the same vetting process' a property of the code rather than an intention. Two things found while building it: - Screening '{asset}-{quote_currency}' would have found zero cached bars for every asset forever, because history is keyed '-USD' throughout the codebase. The convention is now single-sourced in _history_product. - A rejection for zero cached bars is a statement about our DATA, not the asset -- the likeliest misreading of the whole feature, so it is called out in the output and names the 'keel fetch' that fixes it. Co-Authored-By: Claude Opus 4.8 (1M context) --- keel/cli.py | 141 +++++++++++++++++++--- tests/compliance/test_assets_cli.py | 175 ++++++++++++++++++++++++++++ 2 files changed, 300 insertions(+), 16 deletions(-) diff --git a/keel/cli.py b/keel/cli.py index 365f0ccf..1977edee 100644 --- a/keel/cli.py +++ b/keel/cli.py @@ -623,6 +623,130 @@ def _market_facts(repo: Repository, product: str, quote: str) -> screen_mod.Mark ) +def _screen_product( + repo: Repository, product: str, quote: str +) -> tuple[screen_mod.MarketFacts, screen_mod.ScreenResult]: + """THE admission decision, for every candidate source. + + `assets screen`, `assets holdings --screen` and any future proposer (an LLM shortlist, say) + all route through here, so none of them can drift onto a laxer path -- which is what makes + "the same vetting process" a property of the code rather than an intention. Returns the facts + alongside the verdict so a caller can explain WHY without recomputing them. + """ + asset = product.split("-")[0] + facts = _market_facts(repo, product, quote) + raw = repo.get_asset_attestation(asset) + attestation = ( + screen_mod.AssetAttestation( + asset=raw["asset"], + sector=raw["sector"], + backing=raw["backing"], + pays_yield=bool(raw["pays_yield"]), + source=raw["source"], + attested_by=raw["attested_by"], + attested_at=raw["attested_at"], + ) + if raw is not None + else None + ) + return facts, screen_mod.screen_asset(facts, attestation) + + +#: The product id this codebase uses for an asset's daily history. `assets screen`, +#: `keel simulate` and `keel fetch` all key on `-USD` (see `_default_sim_products`), so holdings +#: must too -- screening `{asset}-{quote_currency}` instead would find zero cached bars for every +#: asset and report "no local history" forever, which is worse than useless: it looks like a +#: verdict about the asset. +def _history_product(asset: str) -> str: + return f"{asset}-USD" + + +#: Never candidates: you cannot trade the currency you settle in, and fiat balances are funding, +#: not positions. Listing them as rejections would be noise, not information. +_NON_TRADABLE_CURRENCIES = frozenset({"USD", "EUR", "GBP"}) + + +@assets_group.command("holdings") +@click.option( + "--min-balance", default="0", show_default=True, + help="Ignore balances at or below this (dust from airdrops, forks and rounding).", +) +@click.option( + "--screen", "run_screen", is_flag=True, default=False, + help="Also run each holding through the admission screen.", +) +@click.pass_context +@with_disclaimer +def assets_holdings(ctx: click.Context, min_balance: str, run_screen: bool) -> None: + """List the assets you actually hold at the broker, as allowlist CANDIDATES. + + This is a SOURCE, not a gate. **Holding an asset is not a reason to trade it**, so this + command admits nothing and writes nothing -- no attestation, no allowlist change, no DB + write. It answers "what do I already own that this system might trade?", where + `keel assets discover` answers "what could anyone trade?". + + With `--screen`, each holding goes through the SAME fail-closed screen as + `keel assets screen`: unattested assets are REJECTED, because sector and backing cannot be + derived from a balance any more than from a price. + """ + config = _load_cfg(ctx) + repo = _open_repo(ctx) + broker = _build_broker(config) + quote = config.quote_currency + floor = Decimal(min_balance) + + try: + accounts = broker.get_accounts() + except Exception as exc: # noqa: BLE001 -- an unreachable venue is an error, not "nothing held" + raise click.ClickException( + f"could not fetch balances from the broker: {exc}. Reporting nothing would read as " + "'you hold nothing', which is not what we learned." + ) from exc + + excluded = _NON_TRADABLE_CURRENCIES | {quote} + holdings = sorted( + (a for a in accounts if a["currency"] not in excluded and a["available_balance"] > floor), + key=lambda a: a["currency"], + ) + + if not holdings: + click.echo(f"no holdings above {floor} (excluding {quote} and fiat).") + return + + allowlist = {asset.upper() for asset in config.allowlist} + click.echo(f"{len(holdings)} holding(s) above {floor}, excluding {quote} and fiat:\n") + + for account in holdings: + asset = account["currency"] + on_allowlist = "on-allowlist" if asset.upper() in allowlist else "not-on-allowlist" + attested = "attested" if repo.get_asset_attestation(asset) else "UNATTESTED" + click.echo(f" {asset:<8} balance={account['available_balance']:<18} " + f"{on_allowlist:<16} {attested}") + + if not run_screen: + continue + + product = _history_product(asset) + facts, result = _screen_product(repo, product, quote) + click.echo(f" {result.summary} ({facts.daily_bars} daily bars cached)") + if facts.daily_bars == 0: + # The likeliest misreading of this whole feature. A rejection for zero cached bars + # is a statement about our DATA, not about the asset. + click.echo( + f" ! no local history -- run `keel fetch --products {product}` first, then " + "re-screen.\n" + " This is a MISSING-DATA verdict, not a verdict about the asset." + ) + for failure in result.failures: + click.echo(f" ✗ {failure}") + + click.echo( + "\n⚠️ Holdings are CANDIDATES, not admissions. Nothing here has been admitted to " + "trading:\nthat needs `keel assets attest` with a source, a passing screen, and a " + "deliberate edit to\n`allowlist` in config.yaml." + ) + + @assets_group.command("discover") @click.option("--quote", default=None, help="Settlement currency (default: config.quote_currency).") @click.option( @@ -714,22 +838,7 @@ def assets_screen(ctx: click.Context, products: str | None) -> None: admitted = 0 for product in product_list: asset = product.split("-")[0] - facts = _market_facts(repo, product, config.quote_currency) - raw = repo.get_asset_attestation(asset) - attestation = ( - screen_mod.AssetAttestation( - asset=raw["asset"], - sector=raw["sector"], - backing=raw["backing"], - pays_yield=bool(raw["pays_yield"]), - source=raw["source"], - attested_by=raw["attested_by"], - attested_at=raw["attested_at"], - ) - if raw is not None - else None - ) - result = screen_mod.screen_asset(facts, attestation) + facts, result = _screen_product(repo, product, config.quote_currency) admitted += int(result.admitted) click.echo( diff --git a/tests/compliance/test_assets_cli.py b/tests/compliance/test_assets_cli.py index 7029fe34..4138fac7 100644 --- a/tests/compliance/test_assets_cli.py +++ b/tests/compliance/test_assets_cli.py @@ -256,3 +256,178 @@ def get_candles(self, *a, **k): sol_line = next(ln for ln in result.output.splitlines() if "SOL-USDC" in ln) assert "?" in sol_line assert "NO" not in sol_line + + +# -- assets holdings: the user's own broker balances as a candidate SOURCE ------ +# +# A source, not a gate. Holding an asset is not a reason to trade it, so this command admits +# nothing and writes nothing -- it routes the user's balances through the SAME screen. + + +class _FakeBroker: + """Duck-types the bits of CoinbaseClient this command uses.""" + + def __init__(self, accounts, fail=False): + self._accounts = accounts + self._fail = fail + self.calls = 0 + + def get_accounts(self): + self.calls += 1 + if self._fail: + raise RuntimeError("venue unreachable") + return self._accounts + + +def _account(currency, balance): + return { + "uuid": f"u-{currency}", + "currency": currency, + "available_balance": Decimal(balance), + "default": False, + "active": True, + } + + +def _with_broker(monkeypatch, broker): + monkeypatch.setattr(cli_module, "_build_broker", lambda config: broker) + + +def _holdings(db_path, config_path, *extra): + return CliRunner().invoke( + cli, + ["--db", str(db_path), "--config", str(config_path), "assets", "holdings", *extra], + ) + + +def test_holdings_lists_what_the_user_holds(tmp_path, valid_config_path, monkeypatch): + db_path = tmp_path / "t.db" + _repo_at(db_path) + _with_broker(monkeypatch, _FakeBroker([_account("BTC", "0.5"), _account("SOL", "12")])) + + result = _holdings(db_path, valid_config_path) + + assert result.exit_code == 0, result.output + assert "BTC" in result.output and "SOL" in result.output + + +def test_holdings_excludes_the_settlement_currency_and_fiat( + tmp_path, valid_config_path, monkeypatch +): + """You cannot trade the currency you settle in; listing it as a candidate is noise.""" + db_path = tmp_path / "t.db" + _repo_at(db_path) + _with_broker( + monkeypatch, + _FakeBroker([_account("BTC", "0.5"), _account("USDC", "500"), _account("USD", "100")]), + ) + + result = _holdings(db_path, valid_config_path) + + holding_lines = [ln for ln in result.output.splitlines() if ln.startswith(" ")] + assets_listed = {ln.split()[0] for ln in holding_lines if ln.split()} + assert "BTC" in assets_listed + assert "USDC" not in assets_listed, "cannot trade the currency you settle in" + assert "USD" not in assets_listed, "fiat is funding, not a position" + + +def test_holdings_filters_dust_by_min_balance(tmp_path, valid_config_path, monkeypatch): + db_path = tmp_path / "t.db" + _repo_at(db_path) + _with_broker(monkeypatch, _FakeBroker([_account("BTC", "0.5"), _account("XLM", "0.00001")])) + + result = _holdings(db_path, valid_config_path, "--min-balance", "0.001") + + assert "BTC" in result.output + assert "XLM" not in result.output + + +def test_a_HELD_but_unattested_asset_is_still_REJECTED(tmp_path, valid_config_path, monkeypatch): + """The point of the whole feature: owning it changes nothing about admission.""" + db_path = tmp_path / "t.db" + repo = _repo_at(db_path) + _seed_history(repo, "SOL-USD") # perfect market data... + _with_broker(monkeypatch, _FakeBroker([_account("SOL", "12")])) + + result = _holdings(db_path, valid_config_path, "--screen") + + assert result.exit_code == 0, result.output + assert "REJECT" in result.output + assert "attestation: MISSING" in result.output + + +def test_holdings_screen_agrees_with_assets_screen_for_the_same_asset( + tmp_path, valid_config_path, monkeypatch +): + """One gate, shared by construction -- a proposer must not get a laxer path.""" + db_path = tmp_path / "t.db" + repo = _repo_at(db_path) + _seed_history(repo, "BTC-USD") + runner = CliRunner() + assert _attest(runner, db_path, valid_config_path, "BTC").exit_code == 0 + _with_broker(monkeypatch, _FakeBroker([_account("BTC", "0.5")])) + + screened = runner.invoke( + cli, + ["--db", str(db_path), "--config", str(valid_config_path), + "assets", "screen", "--products", "BTC-USD"], + ) + held = _holdings(db_path, valid_config_path, "--screen") + + assert ("ADMIT" in screened.output) == ("ADMIT" in held.output) + + +def test_no_local_history_is_reported_as_MISSING_DATA_not_a_bad_asset( + tmp_path, valid_config_path, monkeypatch +): + """The likeliest misreading: a rejection for zero cached bars says nothing about the asset.""" + db_path = tmp_path / "t.db" + _repo_at(db_path) # no candles seeded at all + _with_broker(monkeypatch, _FakeBroker([_account("SOL", "12")])) + + result = _holdings(db_path, valid_config_path, "--screen") + + assert "no local history" in result.output + assert "keel fetch" in result.output + + +def test_holdings_writes_nothing(tmp_path, valid_config_path, monkeypatch): + """A read-only report: no attestation, no allowlist change, no DB mutation.""" + db_path = tmp_path / "t.db" + repo = _repo_at(db_path) + _seed_history(repo, "SOL-USD") + _with_broker(monkeypatch, _FakeBroker([_account("SOL", "12")])) + + _holdings(db_path, valid_config_path, "--screen") + + assert repo.get_asset_attestations() == [] + assert _repo_at(db_path).get_asset_attestation("SOL") is None + + +def test_a_broker_failure_is_an_ERROR_not_an_empty_clean_result( + tmp_path, valid_config_path, monkeypatch +): + """An unreachable venue must not read as 'you hold nothing suspicious'.""" + db_path = tmp_path / "t.db" + _repo_at(db_path) + _with_broker(monkeypatch, _FakeBroker([], fail=True)) + + result = _holdings(db_path, valid_config_path) + + assert result.exit_code != 0 + assert "unreachable" in result.output.lower() or "error" in result.output.lower() + + +def test_holdings_marks_assets_already_on_the_allowlist( + tmp_path, valid_config_path, monkeypatch +): + db_path = tmp_path / "t.db" + _repo_at(db_path) + _with_broker(monkeypatch, _FakeBroker([_account("BTC", "0.5"), _account("SOL", "12")])) + + result = _holdings(db_path, valid_config_path) + + btc_line = next(ln for ln in result.output.splitlines() if "BTC" in ln) + sol_line = next(ln for ln in result.output.splitlines() if "SOL" in ln) + assert "allowlist" in btc_line.lower() or "yes" in btc_line.lower() + assert btc_line != sol_line From 78ba23b3d582f7c6b2c5943e93c2c78b0b5b4e5f Mon Sep 17 00:00:00 2001 From: Elmehdi Aitbrahim Date: Tue, 21 Jul 2026 19:58:47 -0400 Subject: [PATCH 3/4] fix(assets): stop dropping compliance warnings; do not assert data artifacts MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Independent review of this PR found two blocking defects: 1. --screen printed result.failures but never result.warnings, so a PAXG holder saw a bare ADMIT and never learned about §65.5's bay' al-sarf regime that 'assets screen' reports for the identical asset. The 'one gate' claim was true only for the admitted boolean; the compliance-relevant half of ScreenResult was silently discarded. 2. With zero cached bars, the liquidity and settlement failures report on our DATA, not the asset -- median volume is 0 because there are no bars, and quotable_in_settlement_currency degenerates to bool(candles). Printing them as findings asserted exactly what the missing-data message exists to deny. They now render as '· not assessable without history'; the real history failure stays. Also: lowercase currency codes are uppercased before exclusion (a 'usdc' balance was being offered as tradable); --min-balance rejects garbage/NaN/ negative like the rest of this CLI; broker CONSTRUCTION is inside the guarded block so a credentials problem is not reported as a venue problem; the fiat list is broader; and two tests were vacuous -- 'not-on-allowlist' contains 'on-allowlist', and the agreement test passed when BOTH sides were False. Records, but does NOT fix, a pre-existing weakness this surfaced: because every screened product is -USD while quote_currency is USDC, require_settlement_quote currently re-checks 'do we have bars'. That is a compliance rail and deserves its own deliberate change. Co-Authored-By: Claude Opus 4.8 (1M context) --- ...-21-holdings-as-candidate-source-design.md | 26 +++++- keel/cli.py | 69 +++++++++++---- tests/compliance/test_assets_cli.py | 88 +++++++++++++++++-- 3 files changed, 160 insertions(+), 23 deletions(-) diff --git a/docs/superpowers/specs/2026-07-21-holdings-as-candidate-source-design.md b/docs/superpowers/specs/2026-07-21-holdings-as-candidate-source-design.md index 00d17f83..32504060 100644 --- a/docs/superpowers/specs/2026-07-21-holdings-as-candidate-source-design.md +++ b/docs/superpowers/specs/2026-07-21-holdings-as-candidate-source-design.md @@ -50,7 +50,7 @@ hold that this system might trade?". ### 3.1 `keel assets holdings` ``` -keel assets holdings [--min-balance 0] [--screen] [--quote USDC] +keel assets holdings [--min-balance 0] [--screen] ``` 1. Fetch `broker.get_accounts()` (already implemented, read-only, proven against the live API). @@ -96,7 +96,29 @@ REJECT SOL no local history -- run `keel fetch --products SOL-USDC` first, ``` This distinction is the single most likely misreading of the feature, so it is handled in the -output rather than left to the operator. +output rather than left to the operator. For the same reason, the **liquidity** and **settlement** +failures are suppressed on that path (shown as `· not assessable without history`): with zero bars +median volume is 0 *because* there are no bars, and `quotable_in_settlement_currency` degenerates +to `bool(candles)` — printing either as a finding would assert about the asset exactly what the +missing-data message exists to deny. The `history` failure remains, because it is the real one. + +⚠️ **A pre-existing weakness this surfaced, deliberately NOT fixed here.** Because every product +this codebase screens is `-USD` while `quote_currency` is `USDC`, +`MarketFacts.quotable_in_settlement_currency` reduces to `bool(candles)` for all of them — so +`ScreenPolicy.require_settlement_quote` currently re-checks "do we have bars" rather than +settlement. That affects `assets screen` today, independently of this work. Changing it alters a +**compliance rail's** behaviour and deserves its own deliberate change, not a quiet edit inside a +feature PR. + +### 3.3a Known limits of the holdings source + +- Staked/wrapped balance types (`ETH2`, `CBETH`) and non-settlement stablecoins (`USDT`, `DAI`) + appear as candidates, and for a balance with no `-USD` product the `keel fetch` hint will not + resolve. They are correctly REJECTED either way (unattested, no history), so the cost is a + redundant row, never a wrong admission. Filtering them properly needs the venue product list, + which is a network call this read-only report deliberately does not make. +- The fiat exclusion list is static. A fiat Coinbase quotes that is missing from it shows up as an + extra rejected row — cosmetic, never an admission. ### 3.4 The seam for the LLM proposer diff --git a/keel/cli.py b/keel/cli.py index 1977edee..4fbbf809 100644 --- a/keel/cli.py +++ b/keel/cli.py @@ -661,9 +661,16 @@ def _history_product(asset: str) -> str: return f"{asset}-USD" -#: Never candidates: you cannot trade the currency you settle in, and fiat balances are funding, -#: not positions. Listing them as rejections would be noise, not information. -_NON_TRADABLE_CURRENCIES = frozenset({"USD", "EUR", "GBP"}) +# Failure classes that are DOWNSTREAM of having no cached history: with zero bars they report +# on our data, not on the asset, so `assets holdings` must not print them as verdicts. +_DATA_DERIVED_FAILURES = frozenset({"liquidity", "settlement"}) + +# Never candidates: you cannot trade the currency you settle in, and fiat is funding rather than +# a position. Coinbase quotes many fiats, so the list is deliberately broad -- a missing one is +# only cosmetic (an extra row), never an admission. +_FIAT_CURRENCIES = frozenset( + {"USD", "EUR", "GBP", "CAD", "AUD", "JPY", "CHF", "SGD", "BRL", "MXN", "TRY", "INR", "KRW"} +) @assets_group.command("holdings") @@ -681,8 +688,9 @@ def assets_holdings(ctx: click.Context, min_balance: str, run_screen: bool) -> N """List the assets you actually hold at the broker, as allowlist CANDIDATES. This is a SOURCE, not a gate. **Holding an asset is not a reason to trade it**, so this - command admits nothing and writes nothing -- no attestation, no allowlist change, no DB - write. It answers "what do I already own that this system might trade?", where + command admits nothing and mutates nothing -- no attestation, no allowlist change, no data + write. (Opening the database does apply any pending schema migration, as every command + does.) It answers "what do I already own that this system might trade?", where `keel assets discover` answers "what could anyone trade?". With `--screen`, each holding goes through the SAME fail-closed screen as @@ -691,22 +699,37 @@ def assets_holdings(ctx: click.Context, min_balance: str, run_screen: bool) -> N """ config = _load_cfg(ctx) repo = _open_repo(ctx) - broker = _build_broker(config) quote = config.quote_currency - floor = Decimal(min_balance) + try: + floor = Decimal(min_balance) + except InvalidOperation as exc: + raise click.BadParameter(f"--min-balance must be a number; got {min_balance!r}") from exc + if not floor.is_finite() or floor < 0: + # A NaN floor makes every `balance > floor` comparison raise; a negative one lists every + # dust and zero balance as a candidate. Same guard `record-flow`/`subscription set` use. + raise click.BadParameter(f"--min-balance must be finite and >= 0; got {min_balance!r}") try: - accounts = broker.get_accounts() + accounts = _build_broker(config).get_accounts() except Exception as exc: # noqa: BLE001 -- an unreachable venue is an error, not "nothing held" + # Includes broker CONSTRUCTION, so a missing/!invalid `.env` credential surfaces here + # rather than as a raw traceback. Reporting an empty list instead would read as + # "you hold nothing", which is not what we learned. raise click.ClickException( - f"could not fetch balances from the broker: {exc}. Reporting nothing would read as " - "'you hold nothing', which is not what we learned." + f"could not read balances from the broker: {exc}\n" + " If this is an authentication error, check CDP_API_KEY/CDP_API_SECRET in .env." ) from exc - excluded = _NON_TRADABLE_CURRENCIES | {quote} + excluded = _FIAT_CURRENCIES | {quote.upper()} + # Currency codes are compared UPPERCASED: a `usdc` balance is still the settlement currency, + # and must not be presented as something tradable on a casing accident. holdings = sorted( - (a for a in accounts if a["currency"] not in excluded and a["available_balance"] > floor), - key=lambda a: a["currency"], + ( + a + for a in accounts + if a["currency"].upper() not in excluded and a["available_balance"] > floor + ), + key=lambda a: a["currency"].upper(), ) if not holdings: @@ -729,16 +752,30 @@ def assets_holdings(ctx: click.Context, min_balance: str, run_screen: bool) -> N product = _history_product(asset) facts, result = _screen_product(repo, product, quote) click.echo(f" {result.summary} ({facts.daily_bars} daily bars cached)") + + failures = result.failures if facts.daily_bars == 0: - # The likeliest misreading of this whole feature. A rejection for zero cached bars - # is a statement about our DATA, not about the asset. + # The likeliest misreading of this whole feature. With no cached bars the liquidity + # and settlement checks CANNOT say anything about the asset -- median volume is 0 + # because there are no bars, and `quotable_in_settlement_currency` degenerates to + # `bool(candles)`. Printing them as findings would assert about the asset exactly + # what this message exists to deny, so they are shown as derived, not as verdicts. + derived = [f for f in failures if f.split(":")[0] in _DATA_DERIVED_FAILURES] + failures = [f for f in failures if f not in derived] click.echo( f" ! no local history -- run `keel fetch --products {product}` first, then " "re-screen.\n" " This is a MISSING-DATA verdict, not a verdict about the asset." ) - for failure in result.failures: + for failure in derived: + click.echo(f" · ({failure.split(':')[0]}: not assessable without history)") + for failure in failures: click.echo(f" ✗ {failure}") + # Warnings carry compliance constraints that apply even to an ADMITted asset (§65.5's + # bay' al-sarf regime for gold/silver backing, say). Dropping them would make this + # command quietly less informative than `assets screen` for the same asset. + for warning in result.warnings: + click.echo(f" ! {warning}") click.echo( "\n⚠️ Holdings are CANDIDATES, not admissions. Nothing here has been admitted to " diff --git a/tests/compliance/test_assets_cli.py b/tests/compliance/test_assets_cli.py index 4138fac7..09de7b46 100644 --- a/tests/compliance/test_assets_cli.py +++ b/tests/compliance/test_assets_cli.py @@ -374,7 +374,10 @@ def test_holdings_screen_agrees_with_assets_screen_for_the_same_asset( ) held = _holdings(db_path, valid_config_path, "--screen") - assert ("ADMIT" in screened.output) == ("ADMIT" in held.output) + # Assert the verdict POSITIVELY -- `x in a == x in b` also passes when both are False, or + # when holdings prints ADMIT unconditionally. + assert "ADMIT" in screened.output + assert "ADMIT" in held.output def test_no_local_history_is_reported_as_MISSING_DATA_not_a_bad_asset( @@ -427,7 +430,82 @@ def test_holdings_marks_assets_already_on_the_allowlist( result = _holdings(db_path, valid_config_path) - btc_line = next(ln for ln in result.output.splitlines() if "BTC" in ln) - sol_line = next(ln for ln in result.output.splitlines() if "SOL" in ln) - assert "allowlist" in btc_line.lower() or "yes" in btc_line.lower() - assert btc_line != sol_line + btc_line = next(ln for ln in result.output.splitlines() if ln.strip().startswith("BTC")) + sol_line = next(ln for ln in result.output.splitlines() if ln.strip().startswith("SOL")) + # "not-on-allowlist" CONTAINS "on-allowlist", so the negative must be excluded explicitly -- + # asserting the substring alone passes even if the check is inverted. + assert "on-allowlist" in btc_line and "not-on-allowlist" not in btc_line + assert "not-on-allowlist" in sol_line + + +def test_holdings_screen_does_not_DROP_compliance_warnings( + tmp_path, valid_config_path, monkeypatch +): + """`ScreenResult.warnings` carry constraints that bind even on an ADMITted asset (§65.5's + bay' al-sarf regime for gold/silver backing). Dropping them made this command quietly less + informative than `assets screen` for the very same asset.""" + db_path = tmp_path / "t.db" + repo = _repo_at(db_path) + _seed_history(repo, "PAXG-USD") + runner = CliRunner() + assert _attest( + runner, db_path, valid_config_path, "PAXG", **{"--backing": "ayn"} + ).exit_code == 0 + _with_broker(monkeypatch, _FakeBroker([_account("PAXG", "3")])) + + screened = runner.invoke( + cli, + ["--db", str(db_path), "--config", str(valid_config_path), + "assets", "screen", "--products", "PAXG-USD"], + ) + held = _holdings(db_path, valid_config_path, "--screen") + + assert "bay' al-sarf" in screened.output, "fixture no longer triggers the warning" + assert "bay' al-sarf" in held.output, "holdings dropped a compliance warning" + + +def test_derivative_failures_are_not_asserted_as_verdicts_without_history( + tmp_path, valid_config_path, monkeypatch +): + """With zero cached bars, liquidity and settlement report on our DATA, not the asset: + median volume is 0 because there are no bars. Printing them as findings would assert + exactly what the missing-data message exists to deny.""" + db_path = tmp_path / "t.db" + _repo_at(db_path) + _with_broker(monkeypatch, _FakeBroker([_account("SOL", "12")])) + + result = _holdings(db_path, valid_config_path, "--screen") + + assert "no local history" in result.output + assert "✗ settlement" not in result.output, "settlement is a naming artifact here" + assert "✗ liquidity" not in result.output, "median volume is 0 only because bars are 0" + assert "not assessable without history" in result.output + assert "✗ history" in result.output # the REAL, primary finding stays + + +def test_a_lowercase_settlement_currency_is_still_excluded( + tmp_path, valid_config_path, monkeypatch +): + """A casing accident must not present the currency you settle in as tradable.""" + db_path = tmp_path / "t.db" + _repo_at(db_path) + _with_broker(monkeypatch, _FakeBroker([_account("btc", "0.5"), _account("usdc", "500")])) + + result = _holdings(db_path, valid_config_path) + + listed = {ln.split()[0].upper() for ln in result.output.splitlines() if ln.startswith(" ")} + assert "USDC" not in listed + assert "BTC" in listed + + +def test_min_balance_rejects_garbage_and_non_finite_values( + tmp_path, valid_config_path, monkeypatch +): + """A NaN floor makes every comparison raise; a negative one lists every zero balance.""" + db_path = tmp_path / "t.db" + _repo_at(db_path) + _with_broker(monkeypatch, _FakeBroker([_account("BTC", "0.5")])) + + for bad in ("abc", "nan", "-1", "inf"): + result = _holdings(db_path, valid_config_path, "--min-balance", bad) + assert result.exit_code != 0, f"--min-balance {bad} should be rejected: {result.output}" From 3d44f47c2fe354eae11d9b57f12c4a3ca64d637c Mon Sep 17 00:00:00 2001 From: Elmehdi Aitbrahim Date: Tue, 21 Jul 2026 20:04:39 -0400 Subject: [PATCH 4/4] fix(assets): uppercase the asset before screening; pin the derived-failure tags Third review round, all non-blocking: - A lowercase 'btc' balance was screened raw: it read 'on-allowlist' and 'UNATTESTED' for the same asset simultaneously, and handed the operator 'keel fetch --products btc-USD', an id that never resolves. Fail-closed (over-rejects, never over-admits) but self-contradictory. - get_accounts defaults a missing currency to None, so .upper() could crash the command outright. Tolerated now. - The zero-bars suppression matches failure tags by string, so renaming a tag in screen_asset would silently make it inert and reintroduce 'data artifacts printed as verdicts' with a green suite. A test now pins the tags -- and I mutation-checked it: renaming 'liquidity' to 'depth' makes it fail, which is the point. - Spec: _screen_product returns (MarketFacts, ScreenResult), not ScreenResult. Co-Authored-By: Claude Opus 4.8 (1M context) --- ...-21-holdings-as-candidate-source-design.md | 2 +- keel/cli.py | 10 +++- tests/compliance/test_assets_cli.py | 59 +++++++++++++++++++ 3 files changed, 67 insertions(+), 4 deletions(-) diff --git a/docs/superpowers/specs/2026-07-21-holdings-as-candidate-source-design.md b/docs/superpowers/specs/2026-07-21-holdings-as-candidate-source-design.md index 32504060..6ff9f015 100644 --- a/docs/superpowers/specs/2026-07-21-holdings-as-candidate-source-design.md +++ b/docs/superpowers/specs/2026-07-21-holdings-as-candidate-source-design.md @@ -73,7 +73,7 @@ read-only report, in the same family as `assets discover`. the `screen_asset` call. That block is extracted to: ```python -def _screen_product(repo, product, quote) -> screen_mod.ScreenResult +def _screen_product(repo, product, quote) -> tuple[MarketFacts, ScreenResult] ``` and **both** `assets screen` and `assets holdings --screen` call it. This is the mechanism that diff --git a/keel/cli.py b/keel/cli.py index 4fbbf809..91756180 100644 --- a/keel/cli.py +++ b/keel/cli.py @@ -727,9 +727,10 @@ def assets_holdings(ctx: click.Context, min_balance: str, run_screen: bool) -> N ( a for a in accounts - if a["currency"].upper() not in excluded and a["available_balance"] > floor + if (a.get("currency") or "").upper() not in excluded + and a["available_balance"] > floor ), - key=lambda a: a["currency"].upper(), + key=lambda a: (a.get("currency") or "").upper(), ) if not holdings: @@ -740,7 +741,10 @@ def assets_holdings(ctx: click.Context, min_balance: str, run_screen: bool) -> N click.echo(f"{len(holdings)} holding(s) above {floor}, excluding {quote} and fiat:\n") for account in holdings: - asset = account["currency"] + # Uppercase here too, not just for the exclusion set: screening the raw code would look + # up `btc` (UNATTESTED) while the allowlist check matched `BTC`, and would hand the + # operator `keel fetch --products btc-USD`, a product id that never resolves. + asset = (account.get("currency") or "").upper() on_allowlist = "on-allowlist" if asset.upper() in allowlist else "not-on-allowlist" attested = "attested" if repo.get_asset_attestation(asset) else "UNATTESTED" click.echo(f" {asset:<8} balance={account['available_balance']:<18} " diff --git a/tests/compliance/test_assets_cli.py b/tests/compliance/test_assets_cli.py index 09de7b46..f53dbd10 100644 --- a/tests/compliance/test_assets_cli.py +++ b/tests/compliance/test_assets_cli.py @@ -509,3 +509,62 @@ def test_min_balance_rejects_garbage_and_non_finite_values( for bad in ("abc", "nan", "-1", "inf"): result = _holdings(db_path, valid_config_path, "--min-balance", bad) assert result.exit_code != 0, f"--min-balance {bad} should be rejected: {result.output}" + + +def test_the_derived_failure_tags_actually_match_screen_asset_output(): + """Pins the string coupling that the zero-bars suppression depends on. + + `assets holdings` suppresses derivative failures by matching `failure.split(":")[0]` against + `_DATA_DERIVED_FAILURES`. Renaming a tag in `screen_asset` would silently stop the + suppression -- reintroducing 'data artifacts printed as verdicts about the asset' with a + fully green suite. This asserts the tags are real. + """ + from keel.compliance import screen as screen_mod + + facts = screen_mod.MarketFacts( + asset="SOL", + daily_bars=0, + median_daily_volume=Decimal(0), + quotable_in_settlement_currency=False, + ) + tags = {f.split(":")[0] for f in screen_mod.screen_asset(facts, None).failures} + + missing = cli_module._DATA_DERIVED_FAILURES - tags + assert not missing, ( + f"{missing} no longer appear as failure tags in screen_asset -- the holdings " + "suppression is now silently inert; update _DATA_DERIVED_FAILURES" + ) + + +def test_a_lowercase_holding_is_screened_as_the_attested_uppercase_asset( + tmp_path, valid_config_path, monkeypatch +): + """A `btc` balance must not read as UNATTESTED while `BTC` is attested, nor be handed a + `btc-USD` fetch hint that will never resolve.""" + db_path = tmp_path / "t.db" + repo = _repo_at(db_path) + _seed_history(repo, "BTC-USD") + runner = CliRunner() + assert _attest(runner, db_path, valid_config_path, "BTC").exit_code == 0 + _with_broker(monkeypatch, _FakeBroker([_account("btc", "0.5")])) + + result = _holdings(db_path, valid_config_path, "--screen") + + assert "UNATTESTED" not in result.output + assert "btc-USD" not in result.output + assert "ADMIT" in result.output + + +def test_an_account_with_no_currency_field_does_not_crash( + tmp_path, valid_config_path, monkeypatch +): + """`CoinbaseClient.get_accounts` defaults a missing currency to None.""" + db_path = tmp_path / "t.db" + _repo_at(db_path) + broken = {"uuid": "u", "currency": None, "available_balance": Decimal("1"), "active": True} + _with_broker(monkeypatch, _FakeBroker([broken, _account("BTC", "0.5")])) + + result = _holdings(db_path, valid_config_path) + + assert result.exit_code == 0, result.output + assert "BTC" in result.output