From d0f7b4c40e5545c862387277ece0db61a4071357 Mon Sep 17 00:00:00 2001 From: Elmehdi Aitbrahim Date: Sun, 9 Aug 2026 19:21:01 -0400 Subject: [PATCH 1/3] fix(cli): render Preview.synthetic at the confirm gate; an estimate must not look like a quote `_interactive_confirm` took a raw `dict` and knew nothing about the port's `Preview`, so it had nowhere to render `synthetic` -- the flag that says whether a human is approving numbers the venue stands behind or an estimate keel computed from a price lookup that validated nothing and reserved nothing. `Preview`'s own docstring makes surfacing that distinction a requirement of anything rendering one. Latent today: Coinbase has a native preview endpoint, so nothing synthetic reaches a human. Real for the first synthesizing venue -- and worse there, because an unpriceable synthesized preview comes back as zeroes, which on an undecorated key/value screen reads as a harmless "$0.00 order" rather than as "keel has no idea what this costs". The gate now: - accepts BOTH the port's `Preview` and the legacy Coinbase dict. Phase B has not landed -- `executor.py` still calls `broker.place_order(product_id, side, order_configuration)` and `_common.py` still builds `CoinbaseClient` -- so `dict` is what the only trading venue sends today. `_read_preview` is the seam, and its docstring says why it is transitional. - renders provenance ABOVE the numbers, not below: a footnote under a tidy key/value block is read after the decision has been made. A broker quote gets a `=` rule; a synthetic estimate gets a `!` block stating that the venue priced, validated and reserved nothing. - shows `Preview.errors` in their own alarm block instead of one quiet line among ten. The legacy dict's `errs` is promoted the same way. - calls out an unpriced preview explicitly as an order whose cost could NOT be determined, rather than letting a zero read as cheap. - upgrades the question from `[y/N]` to a typed "place anyway" when the preview is unpriced, carries errors, or cannot be parsed at all. Friction, never a refusal: an unpriced preview is exactly what a human sees when a venue's pricing endpoint is down and they are trying to close a position, and walling off the exit path is a worse money outcome than a warned approval. - never lets free-form `Preview.detail` shadow a money field -- a colliding key is namespaced. Unchanged where it matters: the live Coinbase dict still renders every field it did, in order, under the same header, and still takes an ordinary `y`. Verified end-to-end through `cb_client.preview_order` against the real `cb_preview_order.json` fixture. Still fails closed off a TTY, for every shape. `ConfirmFn` and `ExecutionResult.preview` widen to both shapes to document the contract at the call site. `_run_order`'s dict-only `fee` extraction is deliberately untouched -- that belongs to the port migration (#198), not to the gate. Closes #199. Refs #198. Co-Authored-By: Claude Opus 5 (1M context) --- keel/cli.py | 223 ++++++++++++++++++++++++++-- keel/execution/executor.py | 16 +- tests/test_confirm_gate.py | 293 +++++++++++++++++++++++++++++++++++++ 3 files changed, 519 insertions(+), 13 deletions(-) create mode 100644 tests/test_confirm_gate.py diff --git a/keel/cli.py b/keel/cli.py index b5ab1cdc..2f221e16 100644 --- a/keel/cli.py +++ b/keel/cli.py @@ -33,6 +33,13 @@ turning autonomy off binds on the next cycle. It changes who is asked, never what is allowed: `guards.check` runs first in every mode, and autonomy never releases a halt. +**The confirm gate says where its numbers came from.** `_interactive_confirm` is the only place +in keel that renders an order preview to a human. It renders the provenance of the figures -- +a broker's own quote versus an estimate keel synthesized -- above them rather than below, and +escalates from `[y/N]` to a typed phrase when the preview is unpriced, carries errors, or cannot +be read at all. See that function and `_ask_to_place` for why that is friction rather than a +refusal. + **No interactive hangs in tests.** `_is_interactive()` is the single TTY predicate, with deliberately no env-var or flag override -- any such seam would be settable from cron and would defeat every fail-closed built on it. Tests patch the predicate. @@ -60,12 +67,14 @@ import json import time +from collections.abc import Mapping from datetime import UTC, datetime from decimal import Decimal, InvalidOperation from pathlib import Path from typing import Any import click +from keel_broker_api.results import Preview from keel_core.products import quote_currency_of from keel import agent @@ -1169,26 +1178,220 @@ def monitor( # -- agent ------------------------------------------------------------------------------ -def _interactive_confirm(preview: dict) -> bool: +#: The banner a venue-priced preview carries. Exported (no underscore) because the tests assert +#: on the exact text a human sees -- the failure this gate defends against is a *misread screen*, +#: so the rendered string is the contract, not an implementation detail. +NATIVE_PREVIEW_MARKER = "BROKER QUOTE -- the venue priced this order itself." +SYNTHETIC_PREVIEW_MARKER = "SYNTHETIC ESTIMATE -- NOT A BROKER QUOTE." +UNPRICED_PREVIEW_MARKER = "UNPRICED -- this preview carries no usable size." +UNREADABLE_PREVIEW_MARKER = "PREVIEW UNREADABLE -- this gate cannot interpret what came back." +#: What a human must type to place a degraded (unpriced / error-carrying / unreadable) preview. +#: Compared case-insensitively after stripping: see `_ask_to_place`. +DEGRADED_PREVIEW_PHRASE = "place anyway" + +_RULE_NATIVE = " " + "=" * 72 +_RULE_ALARM = " " + "!" * 72 + + +def _preview_decimal(value: Any) -> Decimal | None: + """Best-effort `Decimal` for a money field that may be a `Decimal`, a string or junk.""" + if value is None or isinstance(value, bool): + return None + try: + return Decimal(str(value)) + except (InvalidOperation, ValueError, TypeError): + return None + + +def _read_preview(preview: Any) -> tuple[bool, dict[str, Any], tuple[str, ...], bool, bool]: + """Normalize either accepted preview shape into `(synthetic, fields, errors, unpriced, ok)`. + + **Why two shapes.** The port's `Preview` is where this is going; the raw dict is where the + live path still is. `executor.py` calls `broker.preview_order(product_id, side, + order_configuration)` and `keel/commands/_common.py` builds a `CoinbaseClient`, so every + preview a human sees TODAY is `cb_client.preview_order`'s dict. Phase B has not landed. + Accepting only `Preview` would break the one venue that actually trades; accepting only + `dict` is the bug this exists to fix. So the gate accepts both and this function is the seam + -- deliberately transitional, and deletable the day `preview_order` returns `Preview`. + + **Why a dict is read as native.** The dict shape *is* the Coinbase native-preview response + (`supports_native_preview=True`), so treating it as a broker quote is accurate rather than + optimistic. But "accurate today" is not "safe forever": if some future adapter returns a dict + while synthesizing, silently labelling it a broker quote is exactly the failure this issue is + about. So a dict that carries a truthy `synthetic` key is believed over the default. Any new + adapter should return `Preview` and not rely on that. + + `ok=False` means neither shape was recognized -- which is itself a warning, not a blank. + """ + if isinstance(preview, Preview): + fields: dict[str, Any] = { + "product_id": preview.product_id, + "side": getattr(preview.side, "value", preview.side), + "est_base_size": preview.est_base_size, + "est_quote_size": preview.est_quote_size, + "est_fee": preview.est_fee, + } + # NOT `fields.update(...)`: `detail` is free-form text an adapter chose, and a key + # collision would let it silently overwrite a money field with something that merely + # looks like one. A shadowed `est_fee` is a wrong number on a spending screen with + # nothing to indicate it was substituted, so a colliding key is namespaced instead. + for key, value in preview.detail.items(): + fields[key if key not in fields else f"detail.{key}"] = value + # Either leg at zero means the adapter could not size this order. That is NOT a cheap + # order; it is an unknown one, and the two must never render alike. + unpriced = preview.est_quote_size <= 0 or preview.est_base_size <= 0 + return preview.synthetic, fields, tuple(preview.errors), unpriced, True + + if isinstance(preview, Mapping) and preview: + errors = preview.get("errs") or preview.get("errors") or () + sized = next( + ( + _preview_decimal(preview[key]) + for key in ("order_total", "quote_size", "est_quote_size") + if key in preview + ), + None, + ) + return ( + bool(preview.get("synthetic", False)), + dict(preview), + tuple(str(error) for error in errors), + sized is None or sized <= 0, + True, + ) + + return False, {}, (), True, False + + +def _preview_lines(preview: Any) -> tuple[list[str], bool]: + """Render `preview` for a human; return `(lines, degraded)`. + + `degraded` is true when the preview is unpriced, carries errors, or could not be read at all + -- the three cases where the numbers on screen do not mean what they appear to mean. + + Provenance is rendered ABOVE the numbers, not below them, because a footnote under a tidy + key/value block is read after the decision has already been made. + """ + synthetic, fields, errors, unpriced, readable = _read_preview(preview) + + lines: list[str] = [] + if not readable: + lines += [ + _RULE_ALARM, + f" !! {UNREADABLE_PREVIEW_MARKER}", + " !! Nothing below has been checked. Do not read it as a quote.", + _RULE_ALARM, + f" {preview!r}", + ] + return lines, True + + if synthetic: + lines += [ + _RULE_ALARM, + f" !! {SYNTHETIC_PREVIEW_MARKER}", + " !! keel's adapter computed these figures from a price lookup. The", + " !! venue has NOT priced, validated or reserved anything, and is bound", + " !! by none of the numbers below. The fill can differ.", + _RULE_ALARM, + ] + else: + lines += [_RULE_NATIVE, f" {NATIVE_PREVIEW_MARKER}", _RULE_NATIVE] + + lines += [f" {key}: {value}" for key, value in fields.items()] + + # One block below the numbers, not one per problem: two rules butted together read as a + # rendering glitch, and a glitch is the last thing a money screen should look like. + alarms: list[str] = [] + if unpriced: + alarms += [ + f" !! {UNPRICED_PREVIEW_MARKER}", + " !! This is NOT a zero-cost order -- it is an order whose cost could", + " !! not be determined. Approving it sends an order to the venue with", + " !! no idea what it will spend.", + ] + if errors: + alarms += [f" !! PREVIEW ERRORS ({len(errors)}) -- reported against this order:"] + alarms += [f" !! - {error}" for error in errors] + if alarms: + lines += [_RULE_ALARM, *alarms, _RULE_ALARM] + + return lines, bool(unpriced or errors) + + +def _ask_to_place(degraded: bool) -> bool: + """The question itself. A clean preview takes an ordinary y/n; a degraded one does not. + + **Why extra friction, and why not a block.** A `y` at a `[y/N]` prompt is muscle memory after + the tenth order of the day, and the whole point of the banners above is that this particular + screen is not like the last ten. Demanding a typed phrase forces the operator to have read + *something*. It is deliberately NOT a refusal: the exit path is the one that must never be + walled off, and an unpriced preview is exactly what a human would see when a venue's pricing + endpoint is down and they are trying to close a position. Refusing outright would trap a + position behind a broken preview endpoint -- a worse money outcome than a warned-and-approved + order. So: harder to do, never impossible. + + An abort (Ctrl-C, EOF) at either prompt is a decline, not a traceback out of the gate. + """ + try: + if not degraded: + return bool(click.confirm("Place this order?", default=False)) + click.echo( + "This preview is NOT a reliable quote. To place it anyway you must type the " + f'phrase "{DEGRADED_PREVIEW_PHRASE}" -- anything else declines.' + ) + typed = click.prompt( + f'Type "{DEGRADED_PREVIEW_PHRASE}" to place, or press Enter to decline', + default="", + show_default=False, + ) + except (click.Abort, EOFError): + click.echo("aborted -- declining.", err=True) + return False + return str(typed).strip().lower() == DEGRADED_PREVIEW_PHRASE + + +def _interactive_confirm(preview: Preview | Mapping[str, Any] | None) -> bool: """Human-in-the-loop order confirmation for `mode="confirm"`. Called by the executor ONLY after the intent has already passed every hard rail -- this is - an additional human gate, never a replacement for the rails. Renders the broker's preview - and asks for an explicit yes. + an additional human gate, never a replacement for the rails. + + **What this renders, and why it shouts.** `Preview`'s own docstring sets the requirement: + "approving an estimate must never look identical to approving a broker's own quote." A + venue that has a preview endpoint returns numbers it is willing to stand behind; a venue + without one gets an estimate keel computed from a price lookup that validated nothing and + reserved nothing. Those two screens used to be byte-identical, because this function took a + raw dict and had nowhere to put `Preview.synthetic`. They are now visually unmistakable, and + the distinction sits ABOVE the numbers rather than under them. + + Three further states get their own alarm block: `Preview.errors` (the adapter or the venue + said something went wrong), an unpriced preview (a zero size renders as a harmless "$0.00 + order" unless something says otherwise), and a preview shape this gate cannot read at all. + Each of those also upgrades the question from `[y/N]` to a typed phrase -- see `_ask_to_place` + for why that is friction and not a refusal. + + Accepts both the port's `Preview` and the legacy Coinbase dict; `_read_preview` documents why + both, and which one the live path actually sends today. Fails closed: a non-TTY invocation (a script, a cron job, a headless run) declines rather than blocking on stdin, so `mode="confirm"` never trades unattended. """ - click.echo("\nRails PASSED. Coinbase order preview:") - if isinstance(preview, dict) and preview: - for key, value in preview.items(): - click.echo(f" {key}: {value}") - else: - click.echo(f" {preview!r}") + lines, degraded = _preview_lines(preview) + # The legacy header names Coinbase because the dict shape IS the Coinbase client's response. + # A `Preview` can come from any venue, so it gets the venue-neutral header. + # An unreadable preview is not "Coinbase's" either -- naming a venue over a shape this gate + # could not parse asserts a provenance it does not have. + legacy_coinbase_dict = ( + isinstance(preview, Mapping) and bool(preview) and not preview.get("synthetic", False) + ) + header = "Coinbase order preview" if legacy_coinbase_dict else "Order preview" + click.echo(f"\nRails PASSED. {header}:") + for line in lines: + click.echo(line) if not _common._is_interactive(): click.echo("no TTY -- declining (confirm mode fails closed).", err=True) return False - return click.confirm("Place this order?", default=False) + return _ask_to_place(degraded) def _print_loop_result(result: agent.LoopResult) -> None: diff --git a/keel/execution/executor.py b/keel/execution/executor.py index 2f4a8f58..4e697a22 100644 --- a/keel/execution/executor.py +++ b/keel/execution/executor.py @@ -58,11 +58,12 @@ import json import logging import time -from collections.abc import Callable +from collections.abc import Callable, Mapping from dataclasses import dataclass, replace from decimal import Decimal from typing import Any, Literal +from keel_broker_api.results import Preview from keel_core.products import quote_currency_of from keel_core.telemetry import log_event, log_exception, log_venue_failure @@ -88,7 +89,10 @@ class ExecutionResult: placed: bool order_id: int | None vetoed_by: list[str] - preview: dict[str, Any] | None + # Whatever `broker.preview_order` handed back, verbatim. A dict from the pre-port + # `CoinbaseClient` today; a `Preview` once Phase B migrates the call. Anything reading money + # out of this must branch on the shape -- see `ConfirmFn` below and `_run_order`'s `fee`. + preview: Preview | dict[str, Any] | None reason: str # The local `orders.id` of the exit bracket this entry left resting, when one was placed. # `execute` places the bracket itself, so this is the ONLY way a caller can learn its id -- @@ -99,7 +103,13 @@ class ExecutionResult: bracket_order_id: int | None = None -ConfirmFn = Callable[[dict[str, Any]], bool] +#: The human confirm gate for `mode="confirm"`. Takes BOTH shapes on purpose: `broker` here is +#: still the pre-port `CoinbaseClient`, whose `preview_order` returns a dict, but the port's +#: `Preview` is what carries `synthetic` -- the flag that tells a human whether they are +#: approving a venue's quote or an estimate keel computed. A gate typed to `dict` alone has +#: nowhere to render that (issue #199), and one typed to `Preview` alone breaks the only venue +#: that trades today. Both, until Phase B makes `preview_order` return `Preview` everywhere. +ConfirmFn = Callable[[Preview | Mapping[str, Any]], bool] # -- main entry point ----------------------------------------------------------------------- diff --git a/tests/test_confirm_gate.py b/tests/test_confirm_gate.py new file mode 100644 index 00000000..865dc5e4 --- /dev/null +++ b/tests/test_confirm_gate.py @@ -0,0 +1,293 @@ +"""The confirm gate is the last thing standing between a rule and real money. + +`Preview`'s docstring states the requirement these tests enforce: "approving an estimate must +never look identical to approving a broker's own quote." The gate used to take a raw `dict` and +had nowhere to render `Preview.synthetic`, so it could not tell a human which of the two they +were looking at. Today that is latent (Coinbase has a native preview endpoint), but the first +synthesizing venue makes it real -- and a synthesized preview that could not be priced comes back +as zeroes, which on an undecorated key/value screen reads as a harmless "$0.00 order" rather than +as "keel has no idea what this costs". + +So these tests assert on the *rendered text a human sees*, not on a return value: the failure +mode being defended against is a human misreading the screen, and only the screen can be wrong. +They also pin the two shapes the gate accepts during the port migration -- the legacy Coinbase +dict and the port's `Preview` -- because breaking the dict path breaks the only venue that +actually trades. +""" + +from __future__ import annotations + +from decimal import Decimal + +import click +import pytest +from keel_broker_api.results import Preview +from keel_core.types import Side + +import keel.cli as cli_module + + +@pytest.fixture +def at_a_terminal(monkeypatch): + """A human is watching. The TTY predicate lives in `keel.commands._common`.""" + monkeypatch.setattr("keel.commands._common._is_interactive", lambda: True) + + +@pytest.fixture +def never_asked(monkeypatch): + """Make both prompts explode, so a test can prove which one the gate reached.""" + + def _confirm(*args, **kwargs): + raise AssertionError("the gate used the ordinary yes/no confirm") + + def _prompt(*args, **kwargs): + raise AssertionError("the gate used the typed-phrase prompt") + + monkeypatch.setattr(cli_module.click, "confirm", _confirm) + monkeypatch.setattr(cli_module.click, "prompt", _prompt) + + +def _preview(**overrides) -> Preview: + fields: dict = { + "product_id": "BTC-USD", + "side": Side.BUY, + "est_base_size": Decimal("0.00005000"), + "est_quote_size": Decimal("5.00"), + "est_fee": Decimal("0.03"), + "synthetic": False, + } + fields.update(overrides) + return Preview(**fields) + + +def _answers(monkeypatch, *, confirm=None, prompt=None) -> None: + if confirm is not None: + monkeypatch.setattr(cli_module.click, "confirm", lambda *a, **k: confirm) + if prompt is not None: + monkeypatch.setattr(cli_module.click, "prompt", lambda *a, **k: prompt) + + +# -- native vs synthetic ------------------------------------------------------------------------ + + +def test_native_preview_renders_as_a_broker_quote(monkeypatch, capsys, at_a_terminal): + """A venue-priced preview says so, and asks the ordinary yes/no question.""" + _answers(monkeypatch, confirm=True) + monkeypatch.setattr( + cli_module.click, + "prompt", + lambda *a, **k: pytest.fail("a clean native preview must not demand a typed phrase"), + ) + + assert cli_module._interactive_confirm(_preview()) is True + + out = capsys.readouterr().out + assert cli_module.NATIVE_PREVIEW_MARKER in out + assert cli_module.SYNTHETIC_PREVIEW_MARKER not in out + assert "est_quote_size: 5.00" in out + + +def test_synthetic_preview_is_unmistakably_marked(monkeypatch, capsys, at_a_terminal): + """The same numbers, computed by keel instead of quoted by the venue, must not look the + same. A priced synthetic preview is still approvable with the ordinary confirm -- the + warning is the banner, not extra ceremony on every Robinhood exit.""" + _answers(monkeypatch, confirm=True) + monkeypatch.setattr( + cli_module.click, + "prompt", + lambda *a, **k: pytest.fail("a priced synthetic preview must not demand a typed phrase"), + ) + + assert cli_module._interactive_confirm(_preview(synthetic=True)) is True + + out = capsys.readouterr().out + assert cli_module.SYNTHETIC_PREVIEW_MARKER in out + assert cli_module.NATIVE_PREVIEW_MARKER not in out + # Not a footnote: the warning is on screen before the numbers it qualifies. + assert out.index(cli_module.SYNTHETIC_PREVIEW_MARKER) < out.index("est_quote_size") + + +def test_detail_can_never_shadow_a_money_field(monkeypatch, capsys, at_a_terminal): + """`Preview.detail` is free-form text the adapter chose. If a detail key could overwrite + `est_fee`, an adapter bug would put a wrong number on a spending screen with nothing marking + it as substituted. The real figure must survive, and the impostor must be namespaced.""" + _answers(monkeypatch, confirm=False) + + cli_module._interactive_confirm( + _preview(est_fee=Decimal("0.03"), detail={"est_fee": "not-a-fee", "price": "103700.00"}) + ) + + out = capsys.readouterr().out + assert " est_fee: 0.03" in out + assert " detail.est_fee: not-a-fee" in out + assert " price: 103700.00" in out + + +# -- errors and unpriced previews --------------------------------------------------------------- + + +def test_preview_errors_are_shown(monkeypatch, capsys, at_a_terminal): + """`Preview.errors` is the adapter saying "this did not go cleanly". It must be on screen.""" + _answers(monkeypatch, prompt=cli_module.DEGRADED_PREVIEW_PHRASE) + + cli_module._interactive_confirm( + _preview(synthetic=True, errors=("INSUFFICIENT_FUND", "PREVIEW_INVALID_BASE_SIZE")) + ) + + out = capsys.readouterr().out + assert "PREVIEW ERRORS" in out + assert "INSUFFICIENT_FUND" in out + assert "PREVIEW_INVALID_BASE_SIZE" in out + + +def test_unpriced_synthetic_preview_cannot_render_as_a_real_quote( + monkeypatch, capsys, at_a_terminal +): + """Zeroes are the dangerous case: they render as a legitimate, very cheap order. The gate + must say the size is UNKNOWN, not that it is zero, and must not accept a reflexive `y`.""" + _answers(monkeypatch, prompt="") + monkeypatch.setattr( + cli_module.click, + "confirm", + lambda *a, **k: pytest.fail("an unpriced preview must not be approvable with a bare y/n"), + ) + + assert ( + cli_module._interactive_confirm( + _preview( + synthetic=True, est_base_size=Decimal("0"), est_quote_size=Decimal("0") + ) + ) + is False + ) + + out = capsys.readouterr().out + assert cli_module.UNPRICED_PREVIEW_MARKER in out + assert cli_module.SYNTHETIC_PREVIEW_MARKER in out + + +def test_the_typed_phrase_gates_a_degraded_preview(monkeypatch, capsys, at_a_terminal): + """Wrong phrase declines; the exact phrase places. `yes` is deliberately not enough.""" + unpriced = _preview(synthetic=True, est_quote_size=Decimal("0")) + + _answers(monkeypatch, prompt="yes") + assert cli_module._interactive_confirm(unpriced) is False + + _answers(monkeypatch, prompt=cli_module.DEGRADED_PREVIEW_PHRASE.upper() + " ") + assert cli_module._interactive_confirm(unpriced) is True + + +def test_a_degraded_preview_is_never_a_silent_block(monkeypatch, at_a_terminal): + """The friction must not become a wall: a human closing a position has to be able to act + even when the preview is unpriced AND carries errors. Refusing outright would trap a + position behind a broken preview endpoint.""" + _answers(monkeypatch, prompt=cli_module.DEGRADED_PREVIEW_PHRASE) + + stuck_exit = _preview( + side=Side.SELL, + synthetic=True, + est_base_size=Decimal("0"), + est_quote_size=Decimal("0"), + est_fee=Decimal("0"), + errors=("no price available for BTC-USD",), + ) + assert cli_module._interactive_confirm(stuck_exit) is True + + +def test_an_aborted_typed_prompt_declines(monkeypatch, at_a_terminal): + """Ctrl-C / EOF at the phrase prompt is a decline, never a traceback out of the gate.""" + + def _abort(*args, **kwargs): + raise click.Abort() + + monkeypatch.setattr(cli_module.click, "prompt", _abort) + assert cli_module._interactive_confirm(_preview(est_quote_size=Decimal("0"))) is False + + +# -- the legacy Coinbase dict, which is what actually trades today ------------------------------ + + +def test_legacy_coinbase_dict_still_renders_and_confirms(monkeypatch, capsys, at_a_terminal): + """The live path passes `cb_client.preview_order`'s dict. Do not break it.""" + _answers(monkeypatch, confirm=True) + + assert cli_module._interactive_confirm( + { + "order_total": Decimal("5.00"), + "commission_total": Decimal("0.03"), + "errs": [], + "warning": [], + } + ) + + out = capsys.readouterr().out + assert "Coinbase order preview" in out + assert "order_total: 5.00" in out + assert cli_module.NATIVE_PREVIEW_MARKER in out + assert cli_module.SYNTHETIC_PREVIEW_MARKER not in out + + +def test_legacy_dict_errs_are_promoted_out_of_the_key_value_list( + monkeypatch, capsys, at_a_terminal +): + """A Coinbase preview that came back with `errs` used to be one quiet line among ten.""" + _answers(monkeypatch, prompt="") + monkeypatch.setattr( + cli_module.click, + "confirm", + lambda *a, **k: pytest.fail("an error-carrying preview must not take a bare y/n"), + ) + + assert ( + cli_module._interactive_confirm( + {"order_total": Decimal("5.00"), "errs": ["INSUFFICIENT_FUND"], "warning": []} + ) + is False + ) + + out = capsys.readouterr().out + assert "PREVIEW ERRORS" in out + assert "INSUFFICIENT_FUND" in out + + +def test_a_dict_that_declares_itself_synthetic_is_believed(monkeypatch, capsys, at_a_terminal): + """Defense in depth: dicts are assumed to be Coinbase's native shape, but a dict that says + otherwise is taken at its word rather than dressed up as a broker quote.""" + _answers(monkeypatch, confirm=True) + + cli_module._interactive_confirm({"order_total": Decimal("5.00"), "synthetic": True}) + + out = capsys.readouterr().out + assert cli_module.SYNTHETIC_PREVIEW_MARKER in out + assert cli_module.NATIVE_PREVIEW_MARKER not in out + + +def test_an_unreadable_preview_is_treated_as_degraded(monkeypatch, capsys, at_a_terminal): + """Nothing to render is not a reason to render nothing alarming.""" + _answers(monkeypatch, prompt="") + monkeypatch.setattr( + cli_module.click, + "confirm", + lambda *a, **k: pytest.fail("an unreadable preview must not take a bare y/n"), + ) + + assert cli_module._interactive_confirm(None) is False + assert cli_module.UNREADABLE_PREVIEW_MARKER in capsys.readouterr().out + + +# -- fails closed ------------------------------------------------------------------------------- + + +@pytest.mark.parametrize( + "preview", + [ + {"order_total": Decimal("5.00")}, + _preview(), + _preview(synthetic=True), + _preview(synthetic=True, est_quote_size=Decimal("0")), + ], +) +def test_fails_closed_without_a_tty(monkeypatch, preview, never_asked): + """No human, no order -- for every shape, degraded or not.""" + monkeypatch.setattr("keel.commands._common._is_interactive", lambda: False) + assert cli_module._interactive_confirm(preview) is False From 4d9dd33b0a81f6e79704e06ba2953bd5c3ca0308 Mon Sep 17 00:00:00 2001 From: Elmehdi Aitbrahim Date: Sun, 9 Aug 2026 19:22:23 -0400 Subject: [PATCH 2/3] docs(runbook): show the new confirm-gate banners and what a shouting gate means The runbook printed the old undecorated preview screen and told the operator, flatly, that what they were looking at was 'the broker's own numbers, not keel's estimate'. That is now something the screen itself asserts rather than something the docs promise, and there are three screens where it is not true. Documents the '!' blocks, and the typed-phrase escalation -- including why it is passable, so nobody reads it as a wall while trying to exit a position. Co-Authored-By: Claude Opus 5 (1M context) --- docs/go-live-runbook.md | 23 ++++++++++++++++++++--- 1 file changed, 20 insertions(+), 3 deletions(-) diff --git a/docs/go-live-runbook.md b/docs/go-live-runbook.md index 763a8966..9fbf4ec2 100644 --- a/docs/go-live-runbook.md +++ b/docs/go-live-runbook.md @@ -126,18 +126,35 @@ Autonomy is **off** by default, so this is what you should see: ``` Rails PASSED. Coinbase order preview: + ======================================================================== + BROKER QUOTE -- the venue priced this order itself. + ======================================================================== order_total: 5.00 ... Place this order? [y/N]: ``` -**Read the preview before answering.** It is the broker's own numbers, not keel's estimate. Check -the product, the side, and the total. If anything surprises you, answer `N` — declining places -nothing and costs nothing. +**Read the banner first, then the numbers.** The `=` rule above means Coinbase priced this order +itself — those are the venue's own figures. Check the product, the side, and the total. If +anything surprises you, answer `N`; declining places nothing and costs nothing. The rails have already passed at this point. The prompt is an **additional** human gate, never a replacement for them. +**A `!` block instead of the `=` rule means stop and read.** The gate shouts in three cases, and +none of them should appear against Coinbase today: + +- `SYNTHETIC ESTIMATE -- NOT A BROKER QUOTE` — the figures are keel's own estimate from a price + lookup. The venue has priced, validated and reserved nothing, and is bound by none of them. + Only a venue with no preview endpoint produces this; Coinbase has one. +- `UNPRICED -- this preview carries no usable size` — a zero here is **not** a cheap order, it is + an order whose cost could not be determined. +- `PREVIEW ERRORS (n)` — the venue or the adapter reported a problem with this specific order. + +The last two replace `[y/N]` with a typed `place anyway`. That is friction, not a wall: it stays +possible on purpose so a broken pricing endpoint can never trap you out of *closing* a position. +If you are opening one, the right answer to a shouting gate is almost always to decline. + **If you instead see `signals=0` and no preview**, no rule produced a setup this cycle — the cycle itself ran fine. With a DCA test vehicle this is almost always the cadence gotcha (see *What can still go wrong*), not a failure. From 450810359642a2b0d7b247c16810231d95bfdee1 Mon Sep 17 00:00:00 2001 From: Elmehdi Aitbrahim Date: Sun, 9 Aug 2026 19:25:24 -0400 Subject: [PATCH 3/3] fix(cli): make the dict unpriced check sign-agnostic, not `<= 0` Review flagged the assumption: the gate read a non-positive `order_total` as unpriced, but whether Coinbase reports a SELL's `order_total` as signed proceeds is UNVERIFIED against a real sell preview. Guessing wrong in that direction would demand the typed phrase on every live sell -- which is worse than useless: it trains the operator to type it by reflex and destroys the signal on the previews that actually need it. A zero is unambiguous (no size, unknown cost); a negative is not. So the legacy dict branch keys on `== 0` and stays out of the sign question until a live probe settles it. The `Preview` branch owns its own types and stays stricter. Co-Authored-By: Claude Opus 5 (1M context) --- keel/cli.py | 10 +++++++++- tests/test_confirm_gate.py | 28 ++++++++++++++++++++++++++++ 2 files changed, 37 insertions(+), 1 deletion(-) diff --git a/keel/cli.py b/keel/cli.py index 2f221e16..2ae5ebed 100644 --- a/keel/cli.py +++ b/keel/cli.py @@ -1252,11 +1252,19 @@ def _read_preview(preview: Any) -> tuple[bool, dict[str, Any], tuple[str, ...], ), None, ) + # `== 0`, NOT `<= 0`, and the difference is deliberate. A zero size is unambiguous: the + # order has no size and its cost is unknown. A NEGATIVE one is not -- it could just as + # easily be Coinbase reporting a SELL's `order_total` as signed proceeds, and that + # convention has not been verified against a real sell preview. Guessing wrong in the + # `<= 0` direction would demand a typed phrase on EVERY live sell, which trains the + # operator to type it by reflex and destroys the signal on the previews that need it. + # Sign-agnostic here until a live probe settles it; the `Preview` branch above owns its + # own types and can afford to be stricter. return ( bool(preview.get("synthetic", False)), dict(preview), tuple(str(error) for error in errors), - sized is None or sized <= 0, + sized is None or sized == 0, True, ) diff --git a/tests/test_confirm_gate.py b/tests/test_confirm_gate.py index 865dc5e4..28e7145a 100644 --- a/tests/test_confirm_gate.py +++ b/tests/test_confirm_gate.py @@ -250,6 +250,34 @@ def test_legacy_dict_errs_are_promoted_out_of_the_key_value_list( assert "INSUFFICIENT_FUND" in out +@pytest.mark.parametrize("order_total", [Decimal("5.00"), Decimal("-5.00")]) +def test_a_signed_dict_order_total_does_not_manufacture_friction( + monkeypatch, at_a_terminal, order_total +): + """Whether Coinbase reports a SELL's `order_total` as signed proceeds is UNVERIFIED. If it + does and the gate read `<= 0` as unpriced, every live sell would demand the typed phrase -- + which trains the operator to type it by reflex and destroys the signal on the previews that + actually need it. Only a genuine zero means "no size". Revisit if a live probe settles it.""" + _answers(monkeypatch, confirm=True) + monkeypatch.setattr( + cli_module.click, + "prompt", + lambda *a, **k: pytest.fail("a priced order must not demand a typed phrase"), + ) + assert cli_module._interactive_confirm({"order_total": order_total, "errs": []}) is True + + +def test_a_zero_dict_order_total_is_still_unpriced(monkeypatch, at_a_terminal): + """The sign-agnostic rule above must not soften the case it exists for.""" + _answers(monkeypatch, prompt="") + monkeypatch.setattr( + cli_module.click, + "confirm", + lambda *a, **k: pytest.fail("a zero-sized preview must not take a bare y/n"), + ) + assert cli_module._interactive_confirm({"order_total": Decimal("0"), "errs": []}) is False + + def test_a_dict_that_declares_itself_synthetic_is_believed(monkeypatch, capsys, at_a_terminal): """Defense in depth: dicts are assumed to be Coinbase's native shape, but a dict that says otherwise is taken at its word rather than dressed up as a broker quote."""