From 558e5aa356150b572345bc03050eba2044275db9 Mon Sep 17 00:00:00 2001 From: Elmehdi Aitbrahim Date: Sun, 6 Sep 2026 17:26:49 -0400 Subject: [PATCH 1/3] =?UTF-8?q?feat(cli):=20the=20cancel=20asymmetry,=20te?= =?UTF-8?q?rminal=20side=20=E2=80=94=20entries=20ask=20once,=20protection?= =?UTF-8?q?=20is=20typed=20(#707)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Cancelling an open ENTRY is refusing risk, and the constitution makes refusing risk frictionless: `y/N`, then it is gone. Cancelling an open EXIT or a protective bracket is REMOVING PROTECTION -- the same class of action as disabling a stop -- and it takes the typed friction every capability-increasing step in this program takes. No venue draws this distinction; it falls out of keel's own rails. A PROTECTIVE LEG IS CLASSIFIED BY WHAT DEPENDS ON IT, NOT BY ITS SIDE LABEL `positions.bracket_order_id` is the link, and the check runs BEFORE the side test. A row wearing the BUY side while a live tranche points at it for protection would pass a side-only rule and be cancelled on a `y` -- stripping a stop from a position that still holds inventory. THE PARTIAL FILL IS THE CASE THE ISSUE'S RULE DOES NOT COVER `executor.execute` places the bracket as soon as the entry is PLACED rather than once it fills, so a resting entry can already have a protective leg. Cancel a ZERO-FILLED entry and that leg commits base inventory which was never acquired -- an orphan, cleared with the entry by the #519 protocol. Cancel a PARTIALLY FILLED entry and the opposite is true: the operator holds real inventory and the bracket is what protects it. Clearing it "because we cancelled an entry" would strip a stop from a live tranche -- the exit-side hazard reappearing inside an entry-side action, which is exactly what this asymmetry exists to prevent. The remainder is cancelled; the protection stays. And when the orphaned bracket cannot be cleared, the command FAILS LOUDLY rather than reporting the cancel it did manage: an entry gone with a protective leg still working over inventory that does not exist is a state an operator has to be told about rather than discover from a fill. `keel orders` STILL LISTS It became a group and `invoke_without_command=True` is what keeps `keel orders --scope 7d` answering exactly as it did. `keel orders list` is the same thing under its own name. The typed phrase NAMES THE ORDER, so a phrase copied from one prompt cannot answer a different one -- otherwise the friction is a ritual rather than a check on which protection is being removed. A refusal is always a named sentence: "cancelled" reported over an order still live at the venue is worse than being told why it could not be. `CANCELLABLE_STATUSES` is read from `executor.RESTING_STATUSES` rather than restated, so this surface cannot come to disagree with the engine about what is still live. The web half of #707 is not in this commit: it needs a decision about whether `keel serve` may hold a trading credential, which no route in that package does today. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01KZZxmspQXe5qJ9FAsG13s6 --- keel/commands/orders.py | 228 +++++++++++++++++++- tests/commands/test_orders_cancel.py | 312 +++++++++++++++++++++++++++ 2 files changed, 539 insertions(+), 1 deletion(-) create mode 100644 tests/commands/test_orders_cancel.py diff --git a/keel/commands/orders.py b/keel/commands/orders.py index 6847d36..5fb0341 100644 --- a/keel/commands/orders.py +++ b/keel/commands/orders.py @@ -59,6 +59,7 @@ from keel.commands._common import _open_repo_ro, with_disclaimer from keel.data.repository import Repository +from keel.execution.executor import RESTING_STATUSES #: The scope vocabulary, spelled exactly as `keel.commands.activity.ACTIVITY_SCOPES` spells #: it. Two views over the same deployment answering "how far back" with different words would @@ -805,7 +806,122 @@ def render_orders(report: OrdersReport) -> list[str]: # -- the command ----------------------------------------------------------------------------- -@click.command("orders") +# -- the cancel asymmetry (#707) ------------------------------------------------------------------- +# +# Cancelling an open ENTRY is refusing risk. The constitution makes refusing risk frictionless, so +# it asks once and does it. +# +# Cancelling an open EXIT or a protective bracket is REMOVING PROTECTION -- the same class of +# action as disabling a stop -- and it takes the typed friction every capability-increasing step in +# this program takes. No venue draws this distinction; it falls out of keel's own rails, and the +# whole feature is getting the classification right in the cases where a row does not announce +# which kind it is. + +#: The statuses a cancel can reach, READ from the executor rather than restated. +#: +#: Two copies would drift the day the engine learned a third resting state, and this surface would +#: then refuse to cancel something the engine still considers live. `partially_filled` is in the +#: list for the reason `_clear_resting_bracket` gives: its unfilled remainder is working at the +#: exchange exactly like a pending order's whole size. +CANCELLABLE_STATUSES = RESTING_STATUSES + +#: What a cancel is, as a closed vocabulary. +#: +#: `protective` is separate from `exit` because they are refused for different reasons and an +#: operator should be told which: an `exit` is liquidating inventory, a `protective` leg is the +#: stop that stands under a live tranche. +CANCEL_KINDS: tuple[str, ...] = ("entry", "exit", "protective", "unknown") + + +@dataclass(frozen=True) +class CancelDecision: + """What this order is, and therefore what it takes to cancel it. + + ONE classification, read by both front-ends. The CLI turns `typed` into a phrase prompt and the + console turns it into a 403; if each decided for itself, the console could one-click something + the terminal makes you type. + """ + + order_id: int + kind: str + cancellable: bool + #: Whether cancelling this needs the typed phrase rather than a `y/N`. True for everything but + #: an entry -- including `unknown`, so a row this build cannot classify is never the easy case. + typed: bool + #: Why not, when `cancellable` is False. Always a sentence naming the order and its state: a + #: cancel that silently does nothing is the worst answer here, because the operator then + #: believes they have cancelled something still live at the venue. + reason: str = "" + #: Whether cancelling this must also clear the product's resting bracket. + #: + #: TRUE ONLY FOR A ZERO-FILLED ENTRY. `executor.execute` places the bracket as soon as the + #: entry is PLACED rather than once it fills, so a resting entry can already have a protective + #: leg -- and if nothing filled, that leg commits base inventory which was never acquired. It + #: is an orphan and goes with the entry (the #519 protocol). + #: + #: A PARTIALLY FILLED entry is the opposite case and the issue's rule does not cover it: the + #: operator holds real inventory, the bracket is what protects it, and clearing it "because we + #: cancelled an entry" would strip a stop from a live tranche. That is the exit-side hazard + #: reappearing inside an entry-side action, which is the thing this asymmetry exists to + #: prevent. The remainder is cancelled; the protection stays. + clears_bracket: bool = False + product_id: str = "" + + +def classify_cancel(repo: Repository, order_id: int) -> CancelDecision: + """What cancelling `order_id` would be, and what it therefore takes. + + Reads only. It decides nothing about whether the operator may proceed -- that is the caller's + gate -- and it never touches a broker. + """ + row = repo.get_order(order_id) + if row is None: + return CancelDecision( + order_id=order_id, + kind="unknown", + cancellable=False, + typed=True, + reason=f"no order {order_id} in this deployment's book", + ) + + product_id = str(row.get("product_id") or "") + status = str(row.get("status") or "") + # PROTECTIVE first, and the order of these two checks is the guard. A protective leg is the + # real hazard rather than the word "sell": a row wearing the BUY side while a position points + # at it as its protection would pass a side-only rule and be cancelled one-click, stripping a + # stop from a live tranche. + protective = repo.get_position_for_bracket(order_id) is not None + if protective: + kind = "protective" + elif str(row.get("side") or "").lower() == "sell": + kind = "exit" + else: + kind = "entry" + + if status not in CANCELLABLE_STATUSES: + return CancelDecision( + order_id=order_id, + kind=kind, + cancellable=False, + typed=kind != "entry", + reason=( + f"order {order_id} is {status}, and only a resting order can be cancelled " + f"({', '.join(CANCELLABLE_STATUSES)})" + ), + product_id=product_id, + ) + + return CancelDecision( + order_id=order_id, + kind=kind, + cancellable=True, + typed=kind != "entry", + clears_bracket=kind == "entry" and status == "pending", + product_id=product_id, + ) + + +@click.group("orders", invoke_without_command=True) @click.option( "--scope", type=click.Choice(ORDERS_SCOPES), @@ -825,6 +941,10 @@ def render_orders(report: OrdersReport) -> list[str]: def orders_cmd(ctx: click.Context, scope: str, limit: int) -> None: """What keel actually bought and sold, and at what price -- read-only. + `keel orders` still LISTS, with no subcommand and the same options it always took (#707 turned + it into a group and `invoke_without_command=True` is what keeps that true). `keel orders list` + is the same thing under its own name, and `keel orders cancel ` is the write. + One row per order the engine placed, newest first, straight from the `orders` table: who placed it (a human at the terminal, or keel on its own), what was expected versus what the venue gave, the fee as charged, and the venue's own order id. No broker call and no write: @@ -834,7 +954,113 @@ def orders_cmd(ctx: click.Context, scope: str, limit: int) -> None: Rows from BOTH modes are shown and the mode is on the row. A deployment book holds one mode, so a filter here would render empty on the other books and read as "nothing traded". """ + if ctx.invoked_subcommand is not None: + return + _list_orders(ctx, scope=scope, limit=limit) + + +def _list_orders(ctx: click.Context, *, scope: str, limit: int) -> None: repo = _open_repo_ro(ctx) report = gather_orders(repo, now_ts=int(time.time()), scope=scope, limit=limit) for line in render_orders(report): click.echo(line) + + +@orders_cmd.command("list") +@click.option( + "--scope", + type=click.Choice(ORDERS_SCOPES), + default=DEFAULT_ORDERS_SCOPE, + show_default=True, + help="How far back to read: a UTC calendar window, or every order in the book.", +) +@click.option( + "--limit", + type=int, + default=DEFAULT_ORDERS_LIMIT, + show_default=True, + help=f"How many rows to show, newest first (1-{MAX_ORDERS_LIMIT}).", +) +@click.pass_context +@with_disclaimer +def orders_list(ctx: click.Context, scope: str, limit: int) -> None: + """The same listing `keel orders` prints, under its own name.""" + _list_orders(ctx, scope=scope, limit=limit) + + +#: What an operator types to cancel protection. Long enough that it cannot be muscle memory, and +#: it NAMES the order, so a phrase copied from one prompt cannot answer a different one. +CANCEL_EXIT_PHRASE = "remove protection from order {order_id}" + + +@orders_cmd.command("cancel") +@click.argument("order_id", type=int) +@click.pass_context +@with_disclaimer +def orders_cancel(ctx: click.Context, order_id: int) -> None: + """Cancel a resting order. Entries ask once; exits and protective legs are typed. + + THE ASYMMETRY. Cancelling an entry is refusing risk, and the constitution makes refusing risk + frictionless -- so it asks `y/N` and does it. Cancelling an exit or a protective bracket is + REMOVING PROTECTION, the same class of action as disabling a stop, and it takes the typed + friction every capability-increasing step in this program takes. + + A protective leg is classified by the POSITION that points at it, not by its side label: + `positions.bracket_order_id` is the link, and a row wearing the entry side while a tranche + relies on it for protection would pass a side-only rule. + + Needs a terminal either way. The venue is reached only after the confirmation, and only the + venue's own confirmation lets the local row be marked -- `executor._cancel_at_exchange` owns + that rule and this calls it rather than restating it. + """ + from keel.commands._common import _build_broker, _is_interactive, _load_cfg, _open_repo + from keel.execution.executor import _cancel_at_exchange, _clear_resting_bracket + + if not _is_interactive(): + raise click.ClickException( + f"refusing to cancel order {order_id}: this needs an interactive terminal." + ) + + repo = _open_repo(ctx) + decision = classify_cancel(repo, order_id) + if not decision.cancellable: + # A NAMED refusal, never a silent success. An operator told "cancelled" about an order + # still live at the venue is worse off than one told why it could not be. + raise click.ClickException(decision.reason) + + click.echo(f"order {order_id}: {decision.kind} on {decision.product_id}") + if decision.typed: + phrase = CANCEL_EXIT_PHRASE.format(order_id=order_id) + click.echo( + "This is protection, not risk. Cancelling it leaves the position it stands under " + "with nothing beneath it." + ) + typed = click.prompt(f'Type "{phrase}" to confirm', default="", show_default=False) + if typed.strip() != phrase: + raise click.ClickException("aborted (phrase not typed).") + elif not click.confirm("Cancel this resting entry?", default=False): + raise click.ClickException("aborted.") + + now_ts = int(time.time()) + broker = _build_broker(_load_cfg(ctx)) + row = repo.get_order(order_id) + assert row is not None # classify_cancel already read it inside this same process + _cancel_at_exchange(broker, repo, row) + repo.update_order(order_id, status="canceled", updated_at=now_ts) + click.echo(f"cancelled order {order_id} at the venue.") + + if decision.clears_bracket: + # The #519 protocol, reused rather than restated. An entry that nothing filled can still + # have a bracket -- `executor.execute` places it as soon as the entry is PLACED -- and + # that leg now commits base inventory which was never acquired. + if _clear_resting_bracket(broker, repo, decision.product_id, now_ts): + click.echo(f"cleared the resting bracket on {decision.product_id}.") + else: + # LOUD. The entry is gone and a protective leg may still be working at the venue + # against inventory that does not exist, which is a state an operator has to know + # about rather than discover from a fill. + raise click.ClickException( + f"order {order_id} was cancelled, but the resting bracket on " + f"{decision.product_id} could NOT be cleared -- it may still be live at the " + "venue over inventory that was never acquired. Check the venue directly." + ) diff --git a/tests/commands/test_orders_cancel.py b/tests/commands/test_orders_cancel.py new file mode 100644 index 0000000..f3136df --- /dev/null +++ b/tests/commands/test_orders_cancel.py @@ -0,0 +1,312 @@ +"""`keel orders cancel` -- the cancel asymmetry (#707). + +Cancelling an open ENTRY is refusing risk. The constitution says refusing risk is frictionless, so +it asks once and does it. + +Cancelling an open EXIT or a protective bracket is REMOVING PROTECTION. That is the same class of +action as disabling a stop, and it takes the typed friction every other capability-increasing step +in this program takes. No broker makes this distinction; it falls straight out of keel's own rails. + +The classification is the whole feature, so most of what follows is about getting it right in the +cases where a row does not announce which kind it is. +""" + +from __future__ import annotations + +from decimal import Decimal +from typing import Any + +import pytest + +from keel.commands import orders as orders_mod +from keel.data.db import connect, migrate +from keel.data.repository import Repository + +NOW = 1_756_000_000 + + +@pytest.fixture() +def repo(tmp_path) -> Repository: + conn = connect(str(tmp_path / "keel.db")) + migrate(conn) + return Repository(conn) + + +def _order(repo: Repository, **overrides: Any) -> int: + row: dict[str, Any] = { + "mode": "live", + "product_id": "BTC-USD", + "side": "buy", + "qty": Decimal("1"), + "status": "pending", + "created_at": NOW - 100, + "raw_response": '{"order_id": "venue-1"}', + } + row.update(overrides) + return repo.insert_order(row) + + +# -- classification -------------------------------------------------------------------------------- + + +def test_a_resting_buy_is_an_entry(repo: Repository) -> None: + order_id = _order(repo, side="buy") + assert orders_mod.classify_cancel(repo, order_id).kind == "entry" + + +def test_a_resting_sell_is_an_exit(repo: Repository) -> None: + """The side label alone is enough to make it typed-friction. A SELL that is not protecting + anything is still liquidating inventory the operator holds.""" + order_id = _order(repo, side="sell") + assert orders_mod.classify_cancel(repo, order_id).kind == "exit" + + +def test_a_buy_that_is_some_positions_bracket_is_PROTECTIVE_not_an_entry( + repo: Repository, +) -> None: + """The guard that matters, and the reason the side label is not enough on its own. + + `positions.bracket_order_id` is the link, and a protective leg is the real hazard rather than + the word "sell": a row wearing the entry side while a position points at it as its protection + would be cancelled one-click under a side-only rule, stripping a stop from a live tranche. + """ + order_id = _order(repo, side="buy") + position_id = repo.open_position( + product_id="BTC-USD", + rule_name="breakout", + qty=Decimal("1"), + entry_fill=Decimal("50000"), + entry_fee=Decimal("5"), + opened_at=NOW - 200, + bracket_order_id=order_id, + ) + assert position_id + decision = orders_mod.classify_cancel(repo, order_id) + assert decision.kind == "protective" + assert decision.typed is True + + +def test_an_entry_asks_once_and_a_protective_leg_demands_the_phrase(repo: Repository) -> None: + """The asymmetry, as the one field both front-ends read.""" + entry = _order(repo, side="buy") + exit_order = _order(repo, side="sell") + + assert orders_mod.classify_cancel(repo, entry).typed is False + assert orders_mod.classify_cancel(repo, exit_order).typed is True + + +# -- refusals -------------------------------------------------------------------------------------- + + +def test_an_unknown_order_is_a_named_refusal(repo: Repository) -> None: + decision = orders_mod.classify_cancel(repo, 999) + assert decision.kind == "unknown" + assert decision.cancellable is False + assert "999" in decision.reason + + +def test_a_filled_order_is_refused_and_never_silently_ignored(repo: Repository) -> None: + """A no-op that reports success is the worst answer here: the operator believes they have + cancelled something that is still live, or already spent.""" + order_id = _order(repo, status="filled") + decision = orders_mod.classify_cancel(repo, order_id) + + assert decision.cancellable is False + assert "filled" in decision.reason + + +def test_an_already_canceled_order_is_refused_by_name(repo: Repository) -> None: + """Idempotency without a lie. A second cancel does not reach the venue and does not claim to + have done anything.""" + order_id = _order(repo, status="canceled") + decision = orders_mod.classify_cancel(repo, order_id) + + assert decision.cancellable is False + assert "canceled" in decision.reason + + +def test_a_partially_filled_order_is_still_cancellable(repo: Repository) -> None: + """Its remainder is working at the exchange exactly like a pending order's whole size -- + `executor.RESTING_STATUSES` is the same list, and this reads it rather than restating it.""" + order_id = _order(repo, status="partially_filled") + assert orders_mod.classify_cancel(repo, order_id).cancellable is True + + +def test_the_resting_statuses_come_from_the_executor(repo: Repository) -> None: + """One list, not two. A second copy would drift the day the executor learned a third resting + state, and this surface would then refuse to cancel something the engine considers live.""" + from keel.execution.executor import RESTING_STATUSES + + assert orders_mod.CANCELLABLE_STATUSES == RESTING_STATUSES + + +# -- the orphaned protective leg ------------------------------------------------------------------- + + +def test_cancelling_a_zero_filled_entry_clears_its_orphaned_bracket(repo: Repository) -> None: + """`executor.execute` places the bracket as soon as the entry is PLACED, not once it fills, so + a resting entry can already have a protective leg. Cancel the entry and that leg is committing + base inventory that was never acquired -- an orphan, and it must go with the entry.""" + entry = _order(repo, side="buy", status="pending") + assert orders_mod.classify_cancel(repo, entry).clears_bracket is True + + +def test_cancelling_a_PARTIALLY_filled_entry_leaves_its_bracket_alone(repo: Repository) -> None: + """The case the issue's rule does not cover, and the direction that matters. + + A partially filled entry means inventory the operator ACTUALLY HOLDS, and the bracket is what + protects it. Clearing it "because we cancelled an entry" would strip a stop from a live + tranche -- the exit-side hazard reappearing inside an entry-side action, which is exactly what + the asymmetry exists to prevent. The remainder is cancelled; the protection stays. + """ + entry = _order(repo, side="buy", status="partially_filled", filled_quantity=Decimal("0.4")) + assert orders_mod.classify_cancel(repo, entry).clears_bracket is False + + +# -- the CLI --------------------------------------------------------------------------------------- + + +class _Broker: + """A broker that confirms every cancel, and remembers which ones it was asked for.""" + + def __init__(self) -> None: + self.cancelled: list[str] = [] + + def cancel_order(self, native_id: str) -> bool: + self.cancelled.append(native_id) + return True + + +@pytest.fixture() +def deployment(tmp_path, monkeypatch: pytest.MonkeyPatch): + from tests.conftest import VALID_CONFIG_YAML + + db_path = tmp_path / "keel.db" + conn = connect(str(db_path)) + migrate(conn) + conn.close() + config_path = tmp_path / "config.yaml" + config_path.write_text(VALID_CONFIG_YAML) + + broker = _Broker() + monkeypatch.setattr("keel.commands._common._is_interactive", lambda: True) + monkeypatch.setattr("keel.commands.orders._build_broker", lambda _cfg: broker, raising=False) + monkeypatch.setattr("keel.commands._common._build_broker", lambda _cfg: broker) + return db_path, config_path, broker + + +def _run(deployment, args: list[str], stdin: str = ""): + from click.testing import CliRunner + + from keel.cli import cli + + db_path, config_path, _broker = deployment + return CliRunner().invoke( + cli, ["--db", str(db_path), "--config", str(config_path), *args], input=stdin + ) + + +def _book(deployment) -> Repository: + db_path, _config, _broker = deployment + conn = connect(str(db_path)) + migrate(conn) + return Repository(conn) + + +def test_keel_orders_still_lists_with_no_subcommand(deployment) -> None: + """#707 turned this into a group. `keel orders --scope 7d` is what an operator's fingers and + every runbook already know, and a group that stopped answering it would be a breaking change + dressed as a feature.""" + result = _run(deployment, ["orders", "--scope", "7d"]) + assert result.exit_code == 0, result.output + + +def test_cancelling_an_entry_asks_once_and_reaches_the_venue(deployment) -> None: + repo = _book(deployment) + order_id = _order(repo, side="buy") + repo._conn.close() # noqa: SLF001 + + result = _run(deployment, ["orders", "cancel", str(order_id)], stdin="y\n") + + assert result.exit_code == 0, result.output + assert deployment[2].cancelled == ["venue-1"] + assert _book(deployment).get_order(order_id)["status"] == "canceled" + + +def test_declining_the_entry_prompt_cancels_nothing(deployment) -> None: + repo = _book(deployment) + order_id = _order(repo, side="buy") + repo._conn.close() # noqa: SLF001 + + result = _run(deployment, ["orders", "cancel", str(order_id)], stdin="n\n") + + assert result.exit_code != 0 + assert deployment[2].cancelled == [] + assert _book(deployment).get_order(order_id)["status"] == "pending" + + +def test_an_exit_needs_the_typed_phrase_and_a_y_will_not_do(deployment) -> None: + """The friction is the feature. A `y` here is the muscle memory an entry prompt trains, and + it must not reach a protective leg.""" + repo = _book(deployment) + order_id = _order(repo, side="sell") + repo._conn.close() # noqa: SLF001 + + result = _run(deployment, ["orders", "cancel", str(order_id)], stdin="y\n") + + assert result.exit_code != 0 + assert "phrase not typed" in result.output + assert deployment[2].cancelled == [] + + +def test_the_phrase_names_the_order_so_it_cannot_be_reused(deployment) -> None: + """A phrase copied from one prompt must not answer a different one -- otherwise the friction + is a ritual rather than a check on WHICH protection is being removed.""" + repo = _book(deployment) + first = _order(repo, side="sell") + second = _order(repo, side="sell") + repo._conn.close() # noqa: SLF001 + + wrong = orders_mod.CANCEL_EXIT_PHRASE.format(order_id=first) + result = _run(deployment, ["orders", "cancel", str(second)], stdin=wrong + "\n") + + assert result.exit_code != 0 + assert deployment[2].cancelled == [] + + +def test_the_typed_phrase_cancels_a_protective_leg(deployment) -> None: + repo = _book(deployment) + order_id = _order(repo, side="sell") + repo._conn.close() # noqa: SLF001 + + phrase = orders_mod.CANCEL_EXIT_PHRASE.format(order_id=order_id) + result = _run(deployment, ["orders", "cancel", str(order_id)], stdin=phrase + "\n") + + assert result.exit_code == 0, result.output + assert deployment[2].cancelled == ["venue-1"] + + +def test_cancelling_off_a_terminal_is_refused(deployment, monkeypatch) -> None: + monkeypatch.setattr("keel.commands._common._is_interactive", lambda: False) + repo = _book(deployment) + order_id = _order(repo, side="buy") + repo._conn.close() # noqa: SLF001 + + result = _run(deployment, ["orders", "cancel", str(order_id)]) + + assert result.exit_code != 0 + assert "interactive terminal" in result.output + assert deployment[2].cancelled == [] + + +def test_a_filled_order_is_refused_before_the_broker_is_built(deployment) -> None: + """No venue call for an order that cannot be cancelled. The refusal is a read.""" + repo = _book(deployment) + order_id = _order(repo, status="filled") + repo._conn.close() # noqa: SLF001 + + result = _run(deployment, ["orders", "cancel", str(order_id)], stdin="y\n") + + assert result.exit_code != 0 + assert "filled" in result.output + assert deployment[2].cancelled == [] From cc7f4db10219ffd363dd0aa0458f182ecbdc7324 Mon Sep 17 00:00:00 2001 From: Elmehdi Aitbrahim Date: Sun, 6 Sep 2026 19:12:59 -0400 Subject: [PATCH 2/3] =?UTF-8?q?feat(web):=20the=20console=20classifies=20a?= =?UTF-8?q?=20cancel=20and=20hands=20over=20the=20command=20=E2=80=94=20it?= =?UTF-8?q?=20never=20cancels=20(#707)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit #707 asked for `POST /api/orders/cancel`. Building it would have required `keel serve` to hold a trading credential, and that is the boundary this deployment does not cross: the console holds no venue credential and no broker handle, which is what keeps the worst case of a bug in the HTTP layer at "reads a local SQLite file" rather than "exfiltrates live trading keys". Unlocking the keychain and signing a request to a venue happens inside a terminal invocation the operator started, never from an ambient loopback daemon. So there is NO cancel route. Not a guarded one -- none. A guard is a thing that can be got wrong; an absent endpoint cannot be, and a test asserts no route carries the word. WHAT THE CONSOLE DOES INSTEAD IS CLASSIFY, WHICH IS A READ Every order row carries what cancelling it would be and the exact command that would do it. Clicking Cancel opens a dialog naming the order, saying what cancelling it means, and offering the command to copy. ONE CLASSIFICATION, TWO FRONT-ENDS. `classify_cancel` is what the CLI gates on and what the payload renders. If each decided for itself, the console could describe an order as a frictionless entry while `keel orders cancel` demanded the typed phrase for it -- and the operator would learn which was right by typing. THE INVOCATION IS COMPOSED IN PYTHON A client concatenating `keel orders cancel ` and an id could print a command that does not exist, and it would have to read `Field.value` to find the id -- which `render.js` may not do. The headline is composed there too: "Entry order #42" and "Protective bracket #43 — live protection" are two different warnings, and choosing between them is a judgement (Rule 2). An order that cannot be cancelled gets the reason instead of a button. Offering a command that would be refused is worse than offering none: the operator runs it, it fails, and they learn the console does not know what it is looking at. ONE QUERY FOR THE PAGE, NOT ONE PER ROW `get_position_for_bracket` per row is a query per row against a page capped at 2,000. `open_bracket_order_ids` answers every protective link at once, and `classify_cancel` takes the set when it has one and falls back to the single lookup when it does not -- the same rule applied efficiently, not a fast implementation beside a careful one. AND A TOKEN #704 GOT WRONG The mode banner asked for `var(--surface)`, which this stylesheet does not define -- the site calls it that and `keel.css` calls it `--card`. An undefined custom property falls back to transparent, so the banner has been painting no background since it shipped. Found because the modal reached for the same wrong name. Twelve mutants killed, including: the bracket-link guard removed so a protective leg reads as an entry, the side guard removed, an exit no longer needing the typed phrase, the orphaned bracket never cleared, a PARTIALLY FILLED entry having its live protection cleared too, a filled order becoming cancellable, the phrase no longer naming the order, cancel no longer needing a terminal, a failed bracket clear reported as success, the web package gaining a broker import, the classification going back to per-row, and the console composing the command itself. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01KZZxmspQXe5qJ9FAsG13s6 --- keel/commands/orders.py | 126 +++++++++++++++++++++-- keel/data/repository.py | 15 +++ keel/web/payload.py | 66 ++++++++++++ keel/web/static/css/keel.css | 52 +++++++++- keel/web/static/js/render.js | 94 +++++++++++++++++ tests/commands/test_orders.py | 14 +++ tests/commands/test_orders_cancel.py | 145 ++++++++++++++++++++++++++- tests/web/test_orders_view.py | 91 +++++++++++++++++ 8 files changed, 590 insertions(+), 13 deletions(-) diff --git a/keel/commands/orders.py b/keel/commands/orders.py index 5fb0341..514b603 100644 --- a/keel/commands/orders.py +++ b/keel/commands/orders.py @@ -51,7 +51,7 @@ import json import time from collections.abc import Mapping -from dataclasses import dataclass +from dataclasses import dataclass, field from decimal import Decimal, InvalidOperation from typing import Any @@ -215,6 +215,7 @@ class OrderRow: #: The `orders.id` primary key -- the number `keel doctor` and the log lines name. id: int + #: `live` or `paper`, per ROW. Never inferred from the book: see the module docstring. mode: str @@ -311,6 +312,18 @@ class OrderRow: rule_id: int | None created_at: int | None updated_at: int | None + #: What cancelling this order would be, and what it would take (#707). + #: + #: On the ROW rather than looked up by a front-end, because both front-ends need it and the + #: classification is the feature: the console renders a modal from it and the CLI gates on it, + #: and a console that classified for itself could describe an order as a frictionless entry + #: while the terminal demanded the typed phrase for it. + #: + #: Defaulted and LAST, so every existing `OrderRow(...)` construction in this module and its + #: tests is untouched -- a dataclass cannot carry a default ahead of a required field. + cancel: CancelDecision = field( + default_factory=lambda: CancelDecision(0, "unknown", False, True) + ) @dataclass(frozen=True) @@ -474,7 +487,13 @@ def _adverse(side: str, difference: Decimal | None) -> bool | None: return None -def _row_from_dict(row: dict[str, Any], rule_names: Mapping[int, str] | None = None) -> OrderRow: +def _row_from_dict( + row: dict[str, Any], + rule_names: Mapping[int, str] | None = None, + *, + repo: Repository | None = None, + bracket_ids: frozenset[int] | None = None, +) -> OrderRow: """One repository dict, projected. Every judgement this report makes about a row is made here, once, so neither renderer has to make it twice. @@ -522,8 +541,14 @@ def _row_from_dict(row: dict[str, Any], rule_names: Mapping[int, str] | None = N else: book_detail = LIVE_NO_BOOK_DETAIL + order_id = int(row["id"]) return OrderRow( - id=int(row["id"]), + id=order_id, + cancel=( + CancelDecision(order_id, "unknown", False, True) + if repo is None + else classify_cancel(repo, order_id, row=row, bracket_ids=bracket_ids) + ), mode=mode, product_id=str(row.get("product_id") or ""), side=side, @@ -635,7 +660,14 @@ def gather_orders( # Newest first, HERE. `get_orders` orders by `id` ascending; reversing in each renderer # would be the same decision made twice and the second copy is the one that drifts. filtered.reverse() - shown = tuple(_row_from_dict(row, rule_names) for row in filtered[:resolved_limit]) + # ONE query for every protective link on the page, not one per row (#707). `classify_cancel` + # takes the set when it has one and falls back to the single lookup when it does not, so this + # is the same rule applied efficiently rather than a second one. + bracket_ids = repo.open_bracket_order_ids() + shown = tuple( + _row_from_dict(row, rule_names, repo=repo, bracket_ids=bracket_ids) + for row in filtered[:resolved_limit] + ) # Ordered widest cause first: a book with nothing in it is not a scope problem, and a scope # that excluded everything is not the tab's doing. Reporting the narrowest true cause would @@ -832,6 +864,47 @@ def render_orders(report: OrdersReport) -> list[str]: #: stop that stands under a live tranche. CANCEL_KINDS: tuple[str, ...] = ("entry", "exit", "protective", "unknown") +#: The command that cancels one order. Composed in Python and placed by the client, so the console +#: cannot come to print a command that does not exist. +CANCEL_INVOCATION = "keel orders cancel {order_id}" + + +#: The badge every order page carries, and the sentence behind it (#707). +#: +#: Not an apology for a missing feature. `keel serve` holds no venue credential and no broker +#: handle, and that is the property that keeps the worst case of a bug in the HTTP layer at "reads +#: a local SQLite file" rather than "exfiltrates live trading keys". Unlocking the keychain and +#: signing a request to a venue happens during a terminal invocation the operator started, never +#: from an ambient loopback daemon. +WEB_READ_ONLY_BADGE = "read-only" +WEB_READ_ONLY_NOTE = ( + "The web console is read-only for security: it holds no venue credentials. " + "State-changing actions run through the CLI." +) + +#: What each kind of cancel MEANS, for the modal that hands over the command. The word is a term of +#: art; the sentence is what an operator decides on. +CANCEL_HEADLINES: Mapping[str, str] = { + "entry": "Entry order #{order_id} — resting", + "exit": "⚠️ Exit order #{order_id} — live liquidation", + "protective": "⚠️ Protective bracket #{order_id} — live protection", + "unknown": "⚠️ Order #{order_id} — unclassified", +} + +CANCEL_NOTES: Mapping[str, str] = { + "entry": "Cancelling a resting entry refuses risk. Nothing is protecting anything here.", + "exit": ( + "This order liquidates inventory you hold. Cancelling it leaves the position open with " + "no exit working." + ), + "protective": ( + "This is live downside protection — a position relies on it for its stop. Cancelling it " + "leaves that position with nothing beneath it, and the terminal will ask you to type a " + "phrase before it does." + ), + "unknown": "keel could not classify this order, so it is treated as protection.", +} + @dataclass(frozen=True) class CancelDecision: @@ -866,15 +939,42 @@ class CancelDecision: #: prevent. The remainder is cancelled; the protection stays. clears_bracket: bool = False product_id: str = "" - - -def classify_cancel(repo: Repository, order_id: int) -> CancelDecision: + #: The exact terminal command, composed HERE (Rule 2) so the console places it rather than + #: building it. `""` when the order cannot be cancelled: handing an operator a command that + #: would be refused is worse than handing them nothing -- they run it, it fails, and they + #: learn the console does not know what it is looking at. + invocation: str = "" + #: The modal's heading, composed HERE because it is a judgement about what the reader is + #: looking at -- "Entry order #42" and "Protective bracket #43 — live protection" are two + #: different warnings, and choosing between them is Rule 2's territory. It also keeps the + #: client from having to read `Field.value` to find the id, which `render.js` may not do. + headline: str = "" + + +def classify_cancel( + repo: Repository, + order_id: int, + *, + row: dict[str, Any] | None = None, + bracket_ids: frozenset[int] | None = None, +) -> CancelDecision: """What cancelling `order_id` would be, and what it therefore takes. Reads only. It decides nothing about whether the operator may proceed -- that is the caller's - gate -- and it never touches a broker. + gate -- and it never touches a broker, which is why the web console can call it: `keel serve` + holds no venue credential and no broker handle, and #707 settled that it never will. + + ONE function for both front-ends. The CLI turns `typed` into a phrase prompt; the console + turns it into a modal that hands over `invocation`. If each classified for itself, the console + could describe an order as a frictionless entry while the terminal demanded the phrase for it. + + `row` and `bracket_ids` are the BATCH form, for a caller classifying a whole page: the row is + already in hand and `repo.open_bracket_order_ids()` answers every protective link in one query + rather than one per order. Same rule either way -- the lookups below are what this function + falls back to when a caller has neither, not a second implementation. """ - row = repo.get_order(order_id) + if row is None: + row = repo.get_order(order_id) if row is None: return CancelDecision( order_id=order_id, @@ -890,7 +990,11 @@ def classify_cancel(repo: Repository, order_id: int) -> CancelDecision: # real hazard rather than the word "sell": a row wearing the BUY side while a position points # at it as its protection would pass a side-only rule and be cancelled one-click, stripping a # stop from a live tranche. - protective = repo.get_position_for_bracket(order_id) is not None + protective = ( + order_id in bracket_ids + if bracket_ids is not None + else repo.get_position_for_bracket(order_id) is not None + ) if protective: kind = "protective" elif str(row.get("side") or "").lower() == "sell": @@ -918,6 +1022,8 @@ def classify_cancel(repo: Repository, order_id: int) -> CancelDecision: typed=kind != "entry", clears_bracket=kind == "entry" and status == "pending", product_id=product_id, + invocation=CANCEL_INVOCATION.format(order_id=order_id), + headline=CANCEL_HEADLINES[kind].format(order_id=order_id), ) diff --git a/keel/data/repository.py b/keel/data/repository.py index c01e745..61f636c 100644 --- a/keel/data/repository.py +++ b/keel/data/repository.py @@ -1280,6 +1280,21 @@ def get_open_positions(self, product_id: str | None = None) -> list[dict[str, An sql += " ORDER BY opened_at, id" return [self._position_row_to_dict(row) for row in self._conn.execute(sql, params)] + def open_bracket_order_ids(self) -> frozenset[int]: + """Every order id an OPEN tranche relies on for protection, in one query. + + The batch form of `get_position_for_bracket` below, for a caller classifying a page of + orders: that method is a query per row, and the orders page is capped at 2,000. Same + predicate, same `status = 'open'` -- `commands/orders.py::classify_cancel` takes this set + when it has one and falls back to the single lookup when it does not, so there is one + rule rather than a fast one and a careful one that can disagree. + """ + rows = self._conn.execute( + "SELECT bracket_order_id FROM positions " + "WHERE bracket_order_id IS NOT NULL AND status = 'open'" + ).fetchall() + return frozenset(int(row["bracket_order_id"]) for row in rows) + def get_position_for_bracket(self, bracket_order_id: int) -> dict[str, Any] | None: """The OPEN tranche whose bracket is `bracket_order_id`, or `None`. diff --git a/keel/web/payload.py b/keel/web/payload.py index 56772f4..09caa99 100644 --- a/keel/web/payload.py +++ b/keel/web/payload.py @@ -1672,6 +1672,62 @@ def activity_payload(feed: ActivityFeed) -> dict[str, Any]: } +#: The state each cancel kind carries. `entry` is NEUTRAL and never GOOD -- cancelling one is +#: ordinary, not an achievement -- and `protective` warns because acting on it removes a stop. +_CANCEL_STATES: Mapping[str, str] = { + "entry": NEUTRAL, + "exit": WARN, + "protective": WARN, + "unknown": UNKNOWN, +} + + +def _cancel_payload(decision: Any) -> dict[str, Any]: + """How an operator cancels this order, and what cancelling it would mean (#707). + + **There is no route behind any of this, and that is the design rather than a gap.** `keel + serve` holds no venue credential and no broker handle; the whole console is a loopback reader + of a SQLite file. Cancelling reaches a venue, so it happens in a terminal invocation the + operator started -- and what the console does instead is CLASSIFY (a read) and hand over the + exact command. + + The `invocation` is composed in `commands/orders.py` and placed here unread. A client building + the command itself could print one that does not exist; a payload that omitted it would leave + the operator to reconstruct an order id from a table. + + `kind` carries its judgement as a state (Rule 3) because the two readings are not symmetric: an + entry is ordinary, and a protective leg is the one an operator should hesitate over. + """ + from keel.commands.orders import CANCEL_NOTES + + return { + "headline": decision.headline, + "kind": label( + decision.kind, + display=decision.kind, + state=_CANCEL_STATES.get(decision.kind, UNKNOWN), + ), + "note": CANCEL_NOTES.get(decision.kind, ""), + # A flag rather than a bare bool: "this cannot be cancelled" and "this can" are different + # sentences, and the reason belongs beside the second one. + "cancellable": flag( + bool(decision.cancellable), + on="resting — cancellable from the terminal", + off=decision.reason or "not cancellable", + on_state=NEUTRAL, + off_state=UNKNOWN, + ), + "typed": flag( + bool(decision.typed), + on="typed phrase required", + off="asks once", + on_state=WARN, + off_state=NEUTRAL, + ), + "invocation": decision.invocation, + } + + def _order_row_payload(row: OrderRow) -> dict[str, Any]: """One `OrderRow`, placed. Nothing is decided here. @@ -1789,6 +1845,9 @@ def _order_row_payload(row: OrderRow) -> dict[str, Any]: "rule_id": count(row.rule_id), "created_at": moment(row.created_at), "updated_at": moment(row.updated_at), + # #707: how to cancel this, and what cancelling it would mean. No route behind it -- + # see `_cancel_payload`. + "cancel": _cancel_payload(row.cancel), } @@ -1807,6 +1866,8 @@ def orders_payload(report: OrdersReport) -> dict[str, Any]: `modes` is every mode present in the whole book. A deployment book holds one in practice, so stating which means a reader never concludes it from an empty section. """ + from keel.commands.orders import WEB_READ_ONLY_BADGE, WEB_READ_ONLY_NOTE + return { "as_of": iso(report.now_ts), "generated_at": moment(report.now_ts), @@ -1826,6 +1887,11 @@ def orders_payload(report: OrdersReport) -> dict[str, Any]: "modes": [str(mode) for mode in report.modes], "empty_reason": report.empty_reason, "empty_note": _EMPTY_NOTES.get(report.empty_reason, ""), + # The badge the console wears on this page (#707). Written here, not in the client: it is a + # claim about how this deployment is built, and Rule 2 keeps claims in Python. + "write_posture": label( + WEB_READ_ONLY_BADGE, display=WEB_READ_ONLY_NOTE, state=NEUTRAL + ), "rows": [_order_row_payload(row) for row in report.rows], } diff --git a/keel/web/static/css/keel.css b/keel/web/static/css/keel.css index 8834e45..7cd0770 100644 --- a/keel/web/static/css/keel.css +++ b/keel/web/static/css/keel.css @@ -256,6 +256,51 @@ header .mode-paper { color: var(--muted); } header .mode-confirm, header .mode-live { color: var(--accent); border-color: var(--accent); } +/* THE CANCEL-HELP MODAL (#707). + * + * A ``, so focus trapping, Escape and the backdrop are the browser's rather than + * hand-built: a modal that traps focus badly is worse than none on a page an operator reaches with + * a keyboard. + * + * `.linklike` is a button that does not look like a filled action, because none of these buttons + * DOES anything to the deployment -- one opens instructions, one copies text, one closes. The + * filled `button` rule is reserved for a control that changes something on the server, and there + * is no such control anywhere in this console. */ +.cancelhelp { + max-width: 34rem; + border: 1px solid var(--line); + border-radius: 6px; + background: var(--card); + color: var(--fg); + padding: 1.25rem; +} +.cancelhelp::backdrop { background: rgba(0, 0, 0, 0.45); } +.cancelhelp h2 { margin: 0 0 0.5rem; font-size: 1rem; } +/* The command itself: selectable, wrapping, and monospaced, so an operator who cannot use the + * clipboard button can still read and select it. The copy button is a convenience, never the only + * way to get the command. */ +.invocation { + margin: 0.75rem 0; + padding: 0.6rem 0.75rem; + border: 1px solid var(--line); + border-radius: 4px; + background: var(--bg); + font-family: ui-monospace, SFMono-Regular, Menlo, monospace; + font-size: 0.85rem; + white-space: pre-wrap; + word-break: break-word; + user-select: all; +} +.linklike { + background: none; + border: none; + padding: 0; + color: var(--accent); + font: inherit; + text-decoration: underline; + cursor: pointer; +} + /* THE PLANS PAGE's claim list (#706). * * A quoted sentence with its citation beside it, not beneath it: a citation a reader has to go @@ -331,7 +376,12 @@ header .sessionpart:empty { display: none; } margin: 0; padding: 0.45rem 1rem; border-bottom: 1px solid var(--line); - background: var(--surface); + /* `--card`, not `--surface`. #704 shipped `var(--surface)`, which this stylesheet does not + define -- the site's name for the token is `--surface` and keel.css's is `--card`, and an + undefined custom property falls back to transparent, so the banner has been painting no + background at all. Found while adding the modal below, which reached for the same wrong + name. */ + background: var(--card); font-size: 0.85rem; letter-spacing: 0.01em; text-align: center; diff --git a/keel/web/static/js/render.js b/keel/web/static/js/render.js index 9521fc0..9afb4aa 100644 --- a/keel/web/static/js/render.js +++ b/keel/web/static/js/render.js @@ -1869,6 +1869,10 @@ export function ordersView(data, sort, onSort, onScope, onStatus) { const sub = el("p", "sub"); sub.append(field(data.generated_at)); + // #707. The badge, on the page where an operator would otherwise expect a Cancel that acts. + // It is not an apology for a missing feature: `keel serve` holds no venue credentials, which + // is what keeps the worst case of a bug in this layer at "reads a local file". + if (data.write_posture) sub.append(" · ", field(data.write_posture)); fragment.append(sub); fragment.append(scopeSwitch(plain(data.scope), onScope, "Orders scope")); @@ -1919,6 +1923,8 @@ export function ordersView(data, sort, onSort, onScope, onStatus) { { label: "divergence", numeric: true, key: "fill_divergence" }, { label: "fee", numeric: true, key: "fee" }, { label: "placed (UTC)", numeric: false, key: "created_at" }, + // No `key`: there is nothing to sort by, and the column is an action rather than a fact. + { label: "cancel", numeric: false }, ], rows.map( /** @param {any} row */ (row) => [ @@ -1938,6 +1944,7 @@ export function ordersView(data, sort, onSort, onScope, onStatus) { row.fill_divergence, row.fee, row.created_at, + cancelCell(row), ], ), // Never reached when `rows` is empty, because `emptyOrders` below answers first with the @@ -1981,6 +1988,93 @@ function emptyOrders(data) { * @param {any} row * @returns {HTMLElement} */ +/** + * The cancel cell: a button that opens instructions, never a button that cancels (#707). + * + * **This console cannot cancel anything, and that is the design.** `keel serve` holds no venue + * credential and no broker handle — the whole application is a loopback reader of a SQLite file — + * so the worst case of a bug in this layer stays "reads a local database" rather than becoming + * "exfiltrates live trading keys". Unlocking the keychain and signing a request to a venue happens + * inside a terminal invocation the operator started, never from an ambient daemon. + * + * So what the button does is CLASSIFY and hand over the exact command. The classification is a + * read, it comes off the payload, and it is the same `classify_cancel` the terminal gates on — a + * console that decided for itself could call an order a frictionless entry while `keel orders + * cancel` demanded the typed phrase for it. + * + * An order that cannot be cancelled gets the reason instead of a button. Offering a command that + * would be refused is worse than offering none. + * + * @param {any} row + * @returns {HTMLElement} + */ +function cancelCell(row) { + const cancel = row.cancel; + if (!cancel) return el("span", "muted", "—"); + // The INVOCATION's presence is the fact, not a `.value` read: `classify_cancel` composes a + // command only for an order that can actually take one, so an empty string here means the + // terminal would refuse it too. `render.js` may place `display` and style by `state` and may + // never inspect `value` -- and this is a bare string, not a `Field`. + if (!plain(cancel.invocation)) { + return el("span", "muted", plain(cancel.cancellable.display)); + } + const open = el("button", "linklike", "Cancel…"); + open.setAttribute("type", "button"); + open.addEventListener("click", () => openCancelHelp(row)); + return open; +} + +/** + * The modal that tells an operator how to cancel, and copies the command (#707). + * + * The heading says WHAT the order is, because that is the decision: cancelling an entry refuses + * risk, and cancelling a protective leg removes a stop from a position that is relying on it. Both + * sentences come from the payload (Rule 2) and both invocations are the same command — the + * asymmetry lives in the terminal, where an exit will ask for a typed phrase. + * + * `` rather than a hand-built overlay: focus trapping, Escape, and the backdrop are the + * browser's, and a modal that traps focus badly is worse than none on a page an operator reaches + * with a keyboard. + * + * @param {any} row + */ +function openCancelHelp(row) { + const cancel = row.cancel; + const dialog = el("dialog", "cancelhelp"); + + // Composed in Python: "Entry order #42" and "Protective bracket #43 — live protection" are two + // different warnings, and choosing between them is a judgement (Rule 2). + dialog.append(el("h2", undefined, plain(cancel.headline))); + + dialog.append(el("p", undefined, plain(cancel.note))); + + const command = el("pre", "invocation", plain(cancel.invocation)); + dialog.append(command); + + const actions = el("p", "note"); + const copy = el("button", "linklike", "Copy command"); + copy.setAttribute("type", "button"); + copy.addEventListener("click", () => { + // Clipboard writes are permitted on a secure context, and `localhost` is one. Where it is + // refused the command is still selectable text above — this is a convenience, never the only + // way to get the command. + if (navigator.clipboard) void navigator.clipboard.writeText(plain(cancel.invocation)); + copy.textContent = "Copied"; + }); + actions.append(copy, " "); + actions.append(field(cancel.typed)); + dialog.append(actions); + + const close = el("button", "linklike", "Close"); + close.setAttribute("type", "button"); + close.addEventListener("click", () => dialog.close()); + dialog.append(close); + + dialog.addEventListener("close", () => dialog.remove()); + document.body.append(dialog); + dialog.showModal(); +} + function orderDetail(row) { const node = el("details", "cycle"); const summary = el("summary"); diff --git a/tests/commands/test_orders.py b/tests/commands/test_orders.py index 1cf211a..1255f26 100644 --- a/tests/commands/test_orders.py +++ b/tests/commands/test_orders.py @@ -99,6 +99,12 @@ def get_orders(self, *args: Any, **kwargs: Any) -> list[dict[str, Any]]: self.calls.append((args, kwargs)) return list(self.rows) + def open_bracket_order_ids(self) -> frozenset[int]: + """#707. Deliberately NOT recorded in `calls`: this pin is about the arguments + `get_orders` is handed, and a second read appearing there would make the assertion below + about two calls rather than one.""" + return frozenset() + def get_rules(self, *args: Any, **kwargs: Any) -> list[dict[str, Any]]: # `gather_orders` resolves rule NAMES off this (#700). Recorded nowhere: this stub # exists to pin how `get_orders` is called, and that pin is unchanged. @@ -830,11 +836,19 @@ def __init__(self, orders: list[dict[str, Any]], rules: list[dict[str, Any]]) -> self._rules = rules self.order_reads = 0 self.rule_reads = 0 + self.bracket_reads = 0 def get_orders(self, *args: Any, **kwargs: Any) -> list[dict[str, Any]]: self.order_reads += 1 return list(self._orders) + def open_bracket_order_ids(self) -> frozenset[int]: + """#707's protective links, batched. Counted like the others: one read for the page, never + one per row -- which is the property `classify_cancel`'s `bracket_ids` argument exists + for.""" + self.bracket_reads += 1 + return frozenset() + def get_rules(self, *args: Any, **kwargs: Any) -> list[dict[str, Any]]: self.rule_reads += 1 return list(self._rules) diff --git a/tests/commands/test_orders_cancel.py b/tests/commands/test_orders_cancel.py index f3136df..6eaeb45 100644 --- a/tests/commands/test_orders_cancel.py +++ b/tests/commands/test_orders_cancel.py @@ -169,12 +169,15 @@ def test_cancelling_a_PARTIALLY_filled_entry_leaves_its_bracket_alone(repo: Repo class _Broker: """A broker that confirms every cancel, and remembers which ones it was asked for.""" - def __init__(self) -> None: + def __init__(self, refuse: tuple[str, ...] = ()) -> None: self.cancelled: list[str] = [] + self.refuse = refuse def cancel_order(self, native_id: str) -> bool: self.cancelled.append(native_id) - return True + # `False` is a REFUSED cancel on a successful call -- Coinbase answers per order, and + # `_cancel_at_exchange` treats anything but CONFIRMED as "still live at the venue". + return native_id not in self.refuse @pytest.fixture() @@ -310,3 +313,141 @@ def test_a_filled_order_is_refused_before_the_broker_is_built(deployment) -> Non assert result.exit_code != 0 assert "filled" in result.output assert deployment[2].cancelled == [] + + +# -- the report carries the classification, and the console reads it ------------------------------ +# +# The web console never cancels anything: `keel serve` holds no venue credential and no broker +# handle, and #707's decision is that it never will. What it CAN do is classify -- that is a read -- +# and hand the operator the exact terminal invocation. The classification therefore has to reach +# the report, and it has to be the SAME function the CLI gates on, or the console could describe an +# order one way while the terminal treats it another. + + +def test_the_report_classifies_every_row(repo: Repository) -> None: + from keel.commands.orders import gather_orders + + entry = _order(repo, side="buy") + exit_order = _order(repo, side="sell") + bracket = _order(repo, side="buy") + repo.open_position( + product_id="BTC-USD", + rule_name="breakout", + qty=Decimal("1"), + entry_fill=Decimal("50000"), + entry_fee=Decimal("5"), + opened_at=NOW - 200, + bracket_order_id=bracket, + ) + + report = gather_orders(repo, now_ts=NOW, scope="all") + kinds = {row.id: row.cancel.kind for row in report.rows} + + assert kinds[entry] == "entry" + assert kinds[exit_order] == "exit" + assert kinds[bracket] == "protective" + + +def test_the_report_and_the_cli_gate_on_one_classification(repo: Repository) -> None: + """One function, two front-ends. If each decided for itself, the console could tell an + operator an order is a frictionless entry while the terminal demanded the phrase for it.""" + from keel.commands.orders import gather_orders + + for side in ("buy", "sell"): + _order(repo, side=side) + report = gather_orders(repo, now_ts=NOW, scope="all") + + for row in report.rows: + assert row.cancel == orders_mod.classify_cancel(repo, row.id) + + +def test_classifying_a_page_of_orders_does_not_query_per_row(repo: Repository) -> None: + """`get_position_for_bracket` per row is a query per row, and this page is capped at 2,000. + + The batch and the single lookup are the SAME rule -- `classify_cancel` takes the precomputed + set when it has one and looks the row up when it does not -- so there is one classification, + not a fast one and a careful one that can disagree. + + Counted through `sqlite3`'s own trace callback rather than by patching `execute`, which is + read-only on a Connection. + """ + from keel.commands.orders import gather_orders + + for _ in range(25): + _order(repo, side="buy") + + seen: list[str] = [] + repo._conn.set_trace_callback(seen.append) # noqa: SLF001 + try: + gather_orders(repo, now_ts=NOW, scope="all") + finally: + repo._conn.set_trace_callback(None) # noqa: SLF001 + + lookups = [sql for sql in seen if "bracket_order_id" in sql] + assert len(lookups) == 1, f"{len(lookups)} bracket queries for 25 rows" + assert not [sql for sql in seen if sql.strip().upper().startswith(("INSERT", "UPDATE"))] + + +def test_the_invocation_is_composed_in_python_and_names_the_order(repo: Repository) -> None: + """Rule 2: the client places this string and does not build it. A console that concatenated + the command itself could drift from the command that exists.""" + order_id = _order(repo, side="buy") + decision = orders_mod.classify_cancel(repo, order_id) + assert decision.invocation == f"keel orders cancel {order_id}" + + +def test_an_order_that_cannot_be_cancelled_offers_no_invocation(repo: Repository) -> None: + """Handing an operator a command that would be refused is worse than handing them nothing: + they run it, it fails, and they learn the console does not know what it is looking at.""" + order_id = _order(repo, status="filled") + decision = orders_mod.classify_cancel(repo, order_id) + + assert decision.cancellable is False + assert decision.invocation == "" + + +def test_a_bracket_that_cannot_be_cleared_fails_the_command_loudly( + tmp_path, monkeypatch: pytest.MonkeyPatch +) -> None: + """The entry is gone and a protective leg may still be working at the venue over inventory + that was never acquired. + + Reporting the cancel that DID succeed and stopping there would leave the operator believing + the position is flat while a sell sits at the exchange. It is the same rule + `_clear_resting_bracket` states for the executor -- an uncancellable bracket means we do not + know what the exchange will do with that inventory -- and the operator is the only one who can + act on it. + """ + from tests.conftest import VALID_CONFIG_YAML + + db_path = tmp_path / "keel.db" + conn = connect(str(db_path)) + migrate(conn) + repo = Repository(conn) + entry = _order(repo, side="buy", status="pending", raw_response='{"order_id": "venue-entry"}') + _order(repo, side="sell", status="pending", raw_response='{"order_id": "venue-bracket"}') + conn.close() + + config_path = tmp_path / "config.yaml" + config_path.write_text(VALID_CONFIG_YAML) + broker = _Broker(refuse=("venue-bracket",)) + monkeypatch.setattr("keel.commands._common._is_interactive", lambda: True) + monkeypatch.setattr("keel.commands._common._build_broker", lambda _cfg: broker) + + from click.testing import CliRunner + + from keel.cli import cli + + result = CliRunner().invoke( + cli, + ["--db", str(db_path), "--config", str(config_path), "orders", "cancel", str(entry)], + input="y\n", + ) + + assert result.exit_code != 0, result.output + assert "could NOT be cleared" in result.output + # The entry cancel itself still happened and is still recorded -- the failure is about what + # is left behind, not about pretending the first call did not occur. + conn = connect(str(db_path)) + migrate(conn) + assert Repository(conn).get_order(entry)["status"] == "canceled" diff --git a/tests/web/test_orders_view.py b/tests/web/test_orders_view.py index 6686936..9fa8e64 100644 --- a/tests/web/test_orders_view.py +++ b/tests/web/test_orders_view.py @@ -702,3 +702,94 @@ def test_the_status_tab_reaches_the_query_string_not_a_client_side_filter() -> N the fifty rows that arrived and label the result "every canceled order".""" code = _code("main.js") assert ".status = status" in code, "the tab must set the endpoint's status param" + + +# -- the cancel asymmetry, console side (#707) --------------------------------------------------- +# +# THE DECISION THIS PINS: `keel serve` holds no venue credential and no broker handle, and #707 +# settled that it never will. Cancelling reaches a venue, so the console classifies (a read) and +# hands over the exact terminal command. The refusals below are the boundary itself, so they are +# asserted structurally rather than trusted. + + +def test_there_is_no_cancel_route_at_all() -> None: + """Not a guarded route -- NO route. A guard is a thing that can be got wrong; an absent + endpoint cannot be. The console's only POST remains `keel.commands.setup.ACTIONS`.""" + from keel.web.api import API_ROUTES + + assert not [path for path in API_ROUTES if "cancel" in path] + + +def test_the_web_package_cannot_reach_a_broker_or_a_credential() -> None: + """The property the whole decision rests on: the worst case of a bug in this layer stays + "reads a local SQLite file" rather than becoming "exfiltrates live trading keys". + + Scanned over the source rather than reasoned about, because the tempting shortcut when adding + a cancel is one import. + """ + import pathlib + + web = pathlib.Path(__file__).resolve().parents[2] / "keel" / "web" + for path in sorted(web.glob("*.py")): + text = path.read_text(encoding="utf-8") + for forbidden in ("_build_broker", "cancel_order", "_cancel_at_exchange", "load_secret"): + assert forbidden not in text, f"{path.name} reaches for {forbidden}" + + +def test_the_console_never_builds_a_control_that_cancels() -> None: + """The button opens instructions. `openCancelHelp` may copy text and close itself, and it may + not post, fetch or navigate -- there is nothing to post to (see the route test above), and a + client that tried would fail silently rather than loudly.""" + body = _source("render.js") + start = body.index("function openCancelHelp(") + end = body.index("\nfunction ", start + 1) + modal = body[start:end] + + for forbidden in ("fetch(", "XMLHttpRequest", "location", "submit", "method:", '"POST"'): + assert forbidden not in modal, f"the cancel modal reaches for {forbidden}" + + +def test_the_console_hands_over_the_command_rather_than_composing_it() -> None: + """Rule 2. A client that concatenated `keel orders cancel ` and an id could print a command + that does not exist -- and it would have to read `Field.value` to find the id, which this + file may not do.""" + body = _source("render.js") + start = body.index("function openCancelHelp(") + end = body.index("\nfunction ", start + 1) + modal = body[start:end] + + assert "cancel.invocation" in modal + assert "keel orders cancel" not in modal + + +def test_the_orders_payload_carries_the_classification_and_the_command() -> None: + from decimal import Decimal + + from keel.commands.orders import gather_orders + from keel.data.db import connect, migrate + from keel.data.repository import Repository + from keel.web import payload + + conn = connect(":memory:") + migrate(conn) + repo = Repository(conn) + order_id = repo.insert_order( + { + "mode": "live", + "product_id": "BTC-USD", + "side": "buy", + "qty": Decimal("1"), + "status": "pending", + "created_at": 1_000, + } + ) + body = payload.orders_payload(gather_orders(repo, now_ts=2_000, scope="all")) + + (row,) = body["rows"] + assert row["cancel"]["invocation"] == f"keel orders cancel {order_id}" + assert row["cancel"]["kind"]["value"] == "entry" + assert row["cancel"]["typed"]["value"] == "false" + assert "#" in row["cancel"]["headline"] + # And the page says why it cannot do this itself. + assert "read-only" in body["write_posture"]["value"] + assert "no venue credentials" in body["write_posture"]["display"] From 5c8a6b58fe8dbcd8eef17f8a164f9d567d5803a5 Mon Sep 17 00:00:00 2001 From: Elmehdi Aitbrahim Date: Sun, 6 Sep 2026 19:45:54 -0400 Subject: [PATCH 3/3] =?UTF-8?q?fix(cli):=20security=20review=20on=20#707?= =?UTF-8?q?=20=E2=80=94=20cancelling=20an=20entry=20was=20stripping=20a=20?= =?UTF-8?q?live=20tranche's=20stop?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit An independent security review found one HIGH and it was a one-`y` path to a naked position, on the branch deliberately built to be frictionless. THE ENTRY CANCEL WAS CALLING A PRODUCT-WIDE SWEEP `executor._clear_resting_bracket` cancels EVERY resting SELL for a product. That is right where the executor calls it: the caller is about to place a replacement sell over the same inventory, so clearing the old bracket is the point. It is catastrophic here. Nothing replaces anything -- the entry is going away -- so cancelling an entry on a product that already held an open bracketed tranche cancelled that tranche's stop too. Reproduced end to end: one `y`, and the broker was asked for `['venue-entry', 'venue-bracket']`. Cancelling that bracket DIRECTLY demands the typed phrase; reaching it sideways through an entry was a shortcut past the whole asymmetry. And the aftermath was silent. The tranche kept pointing at a cancelled order, and `reconcile_unbracketed_positions` skips a tranche with no `unbracketed:` record by design, so nothing healed it and nothing said anything. The position would have stayed open with no stop indefinitely. `_clear_orphaned_brackets` replaces it: the same read, filtered by `open_bracket_order_ids()` -- the same set `classify_cancel` calls `protective`. What is left is a resting sell no open tranche depends on, which is what "orphan" means. Failures are collected rather than raised, so one uncancellable orphan does not hide the others. THE DECISION IS RE-DERIVED AFTER THE PROMPT A typed phrase is 34 characters and a resting order can fill while it is being typed. The first cut classified once, before the prompt, so an entry that had become `filled` still ran the orphan sweep on a stale `clears_bracket`. The row is re-read and re-classified now, and a status change REFUSES: the operator answered a question about a different order from the one in front of them. THE FILL IS BOOKED BEFORE THE TERMINAL WRITE `execution.reconcile` states the rule this missed -- "A CANCELLED/EXPIRED order can still have SOLD something" -- and `canceled` is terminal, so `_polled_rows` never revisits the row and the fill would be dropped for good. `CANCELLABLE_STATUSES` deliberately includes `partially_filled`, which is exactly the row that carries one. Reconcile's own recorder is reused rather than restated, and wrapped: a read-back failure must not strip the cancel it follows. AND A VENUE REFUSAL IS A SENTENCE, NOT A TRACEBACK `CancelUnavailable` is a `RuntimeError` and `cli.main` re-raises everything, so the likely outcome of the window above -- the order filled while the operator typed -- was a Python traceback. Local state was correct either way; `_cancel_at_exchange` marks nothing on failure. THE BOUNDARY TEST WAS PASSING EITHER WAY It scanned `keel/web/*.py` for `_build_broker`, and this PR had put a broker-building, venue-cancelling function into `keel.commands.orders` -- the module `read_orders` imports on every request. Nothing routes to it, and "nothing routes to it" is the part that needed asserting. A real `/api/orders` call now runs in a subprocess and the test fails if `keel_core.secrets` reached `sys.modules`. Also: `CANCEL_KINDS` was declared and never read, so the headline and note tables could drift from it silently -- checked at import now. The disclaimer printed twice (the group callback carries it and click runs that first), and above the usage text on `--help`. The group docstring still said "read-only" while the group had grown a write. And an open modal survived the 15-second repaint, offering a command for an order that may have filled. Eight mutants killed: the sweep going back to product-wide, a protected bracket treated as an orphan, the decision not re-derived after the prompt, the fill not booked before the terminal status, a venue refusal becoming a traceback, a credential import creeping into the request path, a kind losing its headline, and the fill read-back aborting the cancel. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01KZZxmspQXe5qJ9FAsG13s6 --- keel/commands/orders.py | 156 ++++++++++++++++--- keel/web/static/js/render.js | 6 + tests/commands/test_orders_cancel.py | 219 +++++++++++++++++++++++++++ tests/web/test_orders_view.py | 53 ++++++- 4 files changed, 407 insertions(+), 27 deletions(-) diff --git a/keel/commands/orders.py b/keel/commands/orders.py index 514b603..49f36c4 100644 --- a/keel/commands/orders.py +++ b/keel/commands/orders.py @@ -49,6 +49,7 @@ import datetime import json +import logging import time from collections.abc import Mapping from dataclasses import dataclass, field @@ -56,11 +57,14 @@ from typing import Any import click +from keel_core.telemetry import log_exception from keel.commands._common import _open_repo_ro, with_disclaimer from keel.data.repository import Repository from keel.execution.executor import RESTING_STATUSES +logger = logging.getLogger(__name__) + #: The scope vocabulary, spelled exactly as `keel.commands.activity.ACTIVITY_SCOPES` spells #: it. Two views over the same deployment answering "how far back" with different words would #: be a needless thing for an operator to hold in their head, and the meanings are identical: @@ -215,7 +219,6 @@ class OrderRow: #: The `orders.id` primary key -- the number `keel doctor` and the log lines name. id: int - #: `live` or `paper`, per ROW. Never inferred from the book: see the module docstring. mode: str @@ -884,6 +887,9 @@ def render_orders(report: OrdersReport) -> list[str]: #: What each kind of cancel MEANS, for the modal that hands over the command. The word is a term of #: art; the sentence is what an operator decides on. +#: Checked at import: a kind with no headline or no note would render an empty modal, and a table +#: that has drifted from the vocabulary is exactly the thing nobody notices until a reader sees the +#: gap. This is what `CANCEL_KINDS` is FOR -- declared and never read, it was decoration. CANCEL_HEADLINES: Mapping[str, str] = { "entry": "Entry order #{order_id} — resting", "exit": "⚠️ Exit order #{order_id} — live liquidation", @@ -906,6 +912,12 @@ def render_orders(report: OrdersReport) -> list[str]: } + +# The vocabulary and its tables, checked here rather than hoped about. `CANCEL_KINDS` existed and +# nothing read it; now a kind added to the set without a headline or a note fails at import. +assert set(CANCEL_HEADLINES) == set(CANCEL_KINDS), "CANCEL_HEADLINES does not cover CANCEL_KINDS" +assert set(CANCEL_NOTES) == set(CANCEL_KINDS), "CANCEL_NOTES does not cover CANCEL_KINDS" + @dataclass(frozen=True) class CancelDecision: """What this order is, and therefore what it takes to cancel it. @@ -1045,7 +1057,9 @@ def classify_cancel( @click.pass_context @with_disclaimer def orders_cmd(ctx: click.Context, scope: str, limit: int) -> None: - """What keel actually bought and sold, and at what price -- read-only. + """What keel actually bought and sold, and at what price. + + THE LISTING is read-only; `cancel` below is not, and it is the only write in this group. `keel orders` still LISTS, with no subcommand and the same options it always took (#707 turned it into a group and `invoke_without_command=True` is what keeps that true). `keel orders list` @@ -1088,9 +1102,12 @@ def _list_orders(ctx: click.Context, *, scope: str, limit: int) -> None: help=f"How many rows to show, newest first (1-{MAX_ORDERS_LIMIT}).", ) @click.pass_context -@with_disclaimer def orders_list(ctx: click.Context, scope: str, limit: int) -> None: - """The same listing `keel orders` prints, under its own name.""" + """The same listing `keel orders` prints, under its own name. + + NO `@with_disclaimer`: click runs the group callback first, and it carries one. Both would + print it twice -- and on `--help`, print it above the usage text. + """ _list_orders(ctx, scope=scope, limit=limit) @@ -1102,7 +1119,6 @@ def orders_list(ctx: click.Context, scope: str, limit: int) -> None: @orders_cmd.command("cancel") @click.argument("order_id", type=int) @click.pass_context -@with_disclaimer def orders_cancel(ctx: click.Context, order_id: int) -> None: """Cancel a resting order. Entries ask once; exits and protective legs are typed. @@ -1120,7 +1136,7 @@ def orders_cancel(ctx: click.Context, order_id: int) -> None: that rule and this calls it rather than restating it. """ from keel.commands._common import _build_broker, _is_interactive, _load_cfg, _open_repo - from keel.execution.executor import _cancel_at_exchange, _clear_resting_bracket + from keel.execution.executor import CancelPending, CancelUnavailable, _cancel_at_exchange if not _is_interactive(): raise click.ClickException( @@ -1149,24 +1165,122 @@ def orders_cancel(ctx: click.Context, order_id: int) -> None: now_ts = int(time.time()) broker = _build_broker(_load_cfg(ctx)) + + # RE-READ AND RE-CLASSIFY after the prompt. A typed phrase is 34 characters, and a resting + # order can fill while it is being typed -- at which point `clears_bracket`, decided before the + # prompt, would run an orphan sweep against inventory that now exists. Everything downstream + # reads `fresh`, and a status change is a refusal rather than a proceed: the operator answered + # a question about a different order than the one in front of them now. row = repo.get_order(order_id) - assert row is not None # classify_cancel already read it inside this same process - _cancel_at_exchange(broker, repo, row) + if row is None: + raise click.ClickException(f"order {order_id} vanished from the book while you answered.") + fresh = classify_cancel(repo, order_id, row=row) + if not fresh.cancellable: + raise click.ClickException( + f"not cancelling: {fresh.reason}. That changed while you were answering." + ) + + try: + _cancel_at_exchange(broker, repo, row) + except (CancelUnavailable, CancelPending) as exc: + # A CLI failure, not a traceback. This is the LIKELY outcome of the window above -- the + # order filled while the operator typed -- and "the exchange refused" is a sentence they + # can act on where a `RuntimeError` is not. Nothing local was written: `_cancel_at_exchange` + # marks no state on failure, which is its own first rule. + raise click.ClickException(f"the venue did not cancel order {order_id}: {exc}") from exc + + # The fill BEFORE the terminal status, because `canceled` is terminal and `execution.reconcile` + # states the rule this would otherwise break: "A CANCELLED/EXPIRED order can still have SOLD + # something -- Coinbase reports `filled_size > 0` for an order that partly filled before being + # cancelled." `_polled_rows` only revisits RESTING statuses, so a fill dropped here is dropped + # for good -- and `CANCELLABLE_STATUSES` deliberately includes `partially_filled`, which is + # exactly the row that carries one. + _record_fill_before_cancel(broker, repo, row, now_ts) repo.update_order(order_id, status="canceled", updated_at=now_ts) click.echo(f"cancelled order {order_id} at the venue.") - if decision.clears_bracket: - # The #519 protocol, reused rather than restated. An entry that nothing filled can still - # have a bracket -- `executor.execute` places it as soon as the entry is PLACED -- and - # that leg now commits base inventory which was never acquired. - if _clear_resting_bracket(broker, repo, decision.product_id, now_ts): - click.echo(f"cleared the resting bracket on {decision.product_id}.") - else: - # LOUD. The entry is gone and a protective leg may still be working at the venue - # against inventory that does not exist, which is a state an operator has to know - # about rather than discover from a fill. + if fresh.clears_bracket: + cleared, stranded = _clear_orphaned_brackets(broker, repo, fresh.product_id, now_ts) + for cleared_id in cleared: + click.echo(f"cleared orphaned bracket {cleared_id} on {fresh.product_id}.") + if stranded: + # LOUD. The entry is gone and a protective leg may still be working at the venue over + # inventory that was never acquired, which is a state an operator has to know about + # rather than discover from a fill. raise click.ClickException( - f"order {order_id} was cancelled, but the resting bracket on " - f"{decision.product_id} could NOT be cleared -- it may still be live at the " - "venue over inventory that was never acquired. Check the venue directly." + f"order {order_id} was cancelled, but orphaned bracket(s) " + f"{', '.join(str(one) for one in stranded)} on {fresh.product_id} could NOT be " + "cleared -- they may still be live at the venue over inventory that was never " + "acquired. Check the venue directly." ) + + +def _record_fill_before_cancel( + broker: Any, repo: Repository, row: dict[str, Any], now_ts: int +) -> None: + """Book whatever the order filled before it is marked terminal. + + Reuses `execution.reconcile`'s own recorder rather than restating it: that module owns the + rule that a cancelled order can still have filled, and owns what booking a fill means. + + WRAPPED, because this is the one step here that must not be able to strip the cancel it + follows. The venue has already confirmed; failing to read back a fill is a reporting gap the + next reconcile pass can close, while raising would leave the order live locally and cancelled + at the exchange -- the disagreement this whole command exists to avoid. + """ + from keel.execution import reconcile as reconcile_mod + + recorder = getattr(reconcile_mod, "_try_record_fill", None) + if recorder is None: # pragma: no cover - the seam is present in every shipped build + return + try: + recorder(broker, repo, row, now_ts) + except Exception: + log_exception(logger, "orders.cancel_fill_readback_failed", order_id=row.get("id")) + + +def _clear_orphaned_brackets( + broker: Any, repo: Repository, product_id: str, now_ts: int +) -> tuple[list[int], list[int]]: + """Cancel the resting SELLs on `product_id` that NO OPEN TRANCHE relies on. + + `(cleared, stranded)`. + + **Deliberately NOT `executor._clear_resting_bracket`, and the difference nearly shipped as a + one-`y` path to a naked position.** That function is product-wide: it cancels every resting + SELL for the product, which is right where the executor calls it because the caller is about to + place a REPLACEMENT sell over the same inventory. Here nothing replaces anything -- the entry + is going away -- so cancelling every resting sell strips the stop from any other open tranche + on that product, on the one path built to be frictionless. Cancelling that bracket directly + demands the typed phrase; reaching it sideways through an entry must not be a shortcut past + that. + + So the filter is `open_bracket_order_ids()`, the same set `classify_cancel` calls `protective`. + What is left is a resting sell nothing depends on -- an order committing base inventory that no + open position accounts for, which is what "orphan" means here. + + Failures are COLLECTED rather than raised, so one uncancellable orphan does not hide the others + from the operator, and the caller reports them all at once. + """ + from keel.execution.executor import CancelPending, CancelUnavailable, _cancel_at_exchange + + protected = repo.open_bracket_order_ids() + rows: list[dict[str, Any]] = [] + for status in CANCELLABLE_STATUSES: + rows.extend(repo.get_orders(mode="live", product_id=product_id, status=status)) + + cleared: list[int] = [] + stranded: list[int] = [] + for row in sorted(rows, key=lambda one: int(one["id"])): + order_id = int(row["id"]) + if str(row.get("side") or "").lower() != "sell" or order_id in protected: + continue + try: + _cancel_at_exchange(broker, repo, row) + except (CancelUnavailable, CancelPending): + log_exception(logger, "orders.orphan_bracket_cancel_failed", order_id=order_id) + stranded.append(order_id) + continue + repo.update_order(order_id, status="canceled", updated_at=now_ts) + cleared.append(order_id) + return cleared, stranded diff --git a/keel/web/static/js/render.js b/keel/web/static/js/render.js index 9afb4aa..b0850a4 100644 --- a/keel/web/static/js/render.js +++ b/keel/web/static/js/render.js @@ -2040,6 +2040,12 @@ function cancelCell(row) { */ function openCancelHelp(row) { const cancel = row.cancel; + // Any dialog still open belongs to a previous read. The view repaints every 15 seconds and + // `main.js` replaces `#content`, which this node is deliberately outside of -- so without this + // an open modal survives the repaint and can go on offering a command for an order that has + // since filled. The CLI would refuse it by name, but a console showing a stale instruction is + // the console being wrong rather than the terminal being careful. + for (const stale of document.querySelectorAll("dialog.cancelhelp")) stale.remove(); const dialog = el("dialog", "cancelhelp"); // Composed in Python: "Entry order #42" and "Protective bracket #43 — live protection" are two diff --git a/tests/commands/test_orders_cancel.py b/tests/commands/test_orders_cancel.py index 6eaeb45..14c775c 100644 --- a/tests/commands/test_orders_cancel.py +++ b/tests/commands/test_orders_cancel.py @@ -451,3 +451,222 @@ def test_a_bracket_that_cannot_be_cleared_fails_the_command_loudly( conn = connect(str(db_path)) migrate(conn) assert Repository(conn).get_order(entry)["status"] == "canceled" + + +# -- what an entry cancel may and may not take with it --------------------------------------------- + + +def _deployment_with(tmp_path, monkeypatch, refuse: tuple[str, ...] = ()): + from tests.conftest import VALID_CONFIG_YAML + + db_path = tmp_path / "keel.db" + conn = connect(str(db_path)) + migrate(conn) + config_path = tmp_path / "config.yaml" + config_path.write_text(VALID_CONFIG_YAML) + broker = _Broker(refuse=refuse) + monkeypatch.setattr("keel.commands._common._is_interactive", lambda: True) + monkeypatch.setattr("keel.commands._common._build_broker", lambda _cfg: broker) + return db_path, config_path, broker, conn + + +def test_cancelling_an_entry_NEVER_touches_a_live_tranches_bracket(tmp_path, monkeypatch) -> None: + """THE finding this test exists for, and it was a one-`y` path to a naked position. + + The first cut reused `executor._clear_resting_bracket`, whose contract is PRODUCT-WIDE: it + cancels every resting SELL for the product. That is right where the executor calls it, because + the caller is about to place a replacement SELL over the same inventory. It is catastrophic + here -- the entry is going away and nothing replaces the protection, so cancelling an entry on + a product that already held an open bracketed tranche stripped that tranche's stop behind a + single `y`, on the one code path deliberately built to be frictionless. + + Cancelling that bracket DIRECTLY demands the typed phrase. Reaching it sideways through an + entry must not be a shortcut past that. + + The aftermath was silent: the tranche kept pointing at a cancelled order, and + `reconcile_unbracketed_positions` skips a tranche with no `unbracketed:` record by design, so + nothing healed it and nothing said anything. + """ + db_path, config_path, broker, conn = _deployment_with(tmp_path, monkeypatch) + repo = Repository(conn) + + bracket = _order( + repo, side="sell", status="pending", raw_response='{"order_id": "venue-bracket"}' + ) + repo.open_position( + product_id="BTC-USD", + rule_name="breakout", + qty=Decimal("1"), + entry_fill=Decimal("50000"), + entry_fee=Decimal("5"), + opened_at=NOW - 500, + bracket_order_id=bracket, + ) + entry = _order(repo, side="buy", status="pending", raw_response='{"order_id": "venue-entry"}') + conn.close() + + from click.testing import CliRunner + + from keel.cli import cli + + result = CliRunner().invoke( + cli, + ["--db", str(db_path), "--config", str(config_path), "orders", "cancel", str(entry)], + input="y\n", + ) + + assert result.exit_code == 0, result.output + assert broker.cancelled == ["venue-entry"], "the live tranche's bracket was cancelled too" + + conn = connect(str(db_path)) + migrate(conn) + book = Repository(conn) + assert book.get_order(entry)["status"] == "canceled" + assert book.get_order(bracket)["status"] == "pending", "a live tranche was left with no stop" + + +def test_cancelling_an_entry_does_clear_a_bracket_no_position_relies_on( + tmp_path, monkeypatch +) -> None: + """The orphan the rule is actually for: a resting SELL that no OPEN tranche points at commits + base inventory nothing acquired, and it goes with the entry.""" + db_path, config_path, broker, conn = _deployment_with(tmp_path, monkeypatch) + repo = Repository(conn) + + orphan = _order( + repo, side="sell", status="pending", raw_response='{"order_id": "venue-orphan"}' + ) + entry = _order(repo, side="buy", status="pending", raw_response='{"order_id": "venue-entry"}') + conn.close() + + from click.testing import CliRunner + + from keel.cli import cli + + result = CliRunner().invoke( + cli, + ["--db", str(db_path), "--config", str(config_path), "orders", "cancel", str(entry)], + input="y\n", + ) + + assert result.exit_code == 0, result.output + assert broker.cancelled == ["venue-entry", "venue-orphan"] + + conn = connect(str(db_path)) + migrate(conn) + assert Repository(conn).get_order(orphan)["status"] == "canceled" + + +def test_a_fill_landing_while_the_operator_answers_stops_the_cancel( + tmp_path, monkeypatch +) -> None: + """A typed phrase is 34 characters, and a resting order can fill while it is being typed. + + The first cut classified once, before the prompt, and everything downstream read that stale + decision -- so an entry that had become `filled` still ran the orphan sweep, and + `clears_bracket` was answering a question about an order that no longer existed in that + state. The operator answered a question about a different order from the one in front of them + now, so the honest response is to refuse rather than to proceed on the old answer. + """ + db_path, config_path, broker, conn = _deployment_with(tmp_path, monkeypatch) + repo = Repository(conn) + entry = _order(repo, side="buy", status="pending", raw_response='{"order_id": "venue-entry"}') + conn.close() + + def _fill_then_confirm(*_args: object, **_kwargs: object) -> bool: + book = connect(str(db_path)) + migrate(book) + Repository(book).update_order(entry, status="filled", updated_at=NOW) + book.close() + return True + + monkeypatch.setattr("click.confirm", _fill_then_confirm) + + from click.testing import CliRunner + + from keel.cli import cli + + result = CliRunner().invoke( + cli, ["--db", str(db_path), "--config", str(config_path), "orders", "cancel", str(entry)] + ) + + assert result.exit_code != 0 + assert "changed while you were answering" in result.output + assert broker.cancelled == [], "the venue was asked to cancel an order that had filled" + + +def test_whatever_the_order_filled_is_booked_before_it_is_marked_canceled( + tmp_path, monkeypatch +) -> None: + """`execution.reconcile` states the rule: a CANCELLED order can still have SOLD something, and + `canceled` is terminal -- `_polled_rows` only revisits resting statuses, so a fill dropped here + is dropped for good. `CANCELLABLE_STATUSES` deliberately includes `partially_filled`, which is + exactly the row that carries one.""" + db_path, config_path, broker, conn = _deployment_with(tmp_path, monkeypatch) + repo = Repository(conn) + entry = _order( + repo, + side="buy", + status="partially_filled", + filled_quantity=Decimal("0.4"), + raw_response='{"order_id": "venue-entry"}', + ) + conn.close() + + seen: list[str] = [] + + def _spy(_broker: object, _repo: object, row: dict, _now: int) -> None: + # The status at the moment the fill is read back: still resting, because the terminal + # write has not happened yet. Booked after it, the row would be unreachable. + seen.append(str(row["status"])) + + monkeypatch.setattr("keel.execution.reconcile._try_record_fill", _spy, raising=False) + + from click.testing import CliRunner + + from keel.cli import cli + + result = CliRunner().invoke( + cli, + ["--db", str(db_path), "--config", str(config_path), "orders", "cancel", str(entry)], + input="y\n", + ) + + assert result.exit_code == 0, result.output + assert seen == ["partially_filled"], "the fill was never read back before the terminal write" + + +def test_a_venue_that_refuses_the_cancel_is_a_message_not_a_traceback( + tmp_path, monkeypatch +) -> None: + """`CancelUnavailable` is a `RuntimeError`, and `cli.main` re-raises everything. This is the + LIKELY outcome of the window above -- the order filled while the operator typed -- and "the + exchange refused" is a sentence they can act on where a Python traceback is not. + + Local state is untouched either way: `_cancel_at_exchange` marks nothing on failure, which is + its own first rule. + """ + db_path, config_path, broker, conn = _deployment_with( + tmp_path, monkeypatch, refuse=("venue-entry",) + ) + repo = Repository(conn) + entry = _order(repo, side="buy", status="pending", raw_response='{"order_id": "venue-entry"}') + conn.close() + + from click.testing import CliRunner + + from keel.cli import cli + + result = CliRunner().invoke( + cli, + ["--db", str(db_path), "--config", str(config_path), "orders", "cancel", str(entry)], + input="y\n", + ) + + assert result.exit_code != 0 + assert result.exception is None or isinstance(result.exception, SystemExit), result.exception + assert "the venue did not cancel" in result.output + + conn = connect(str(db_path)) + migrate(conn) + assert Repository(conn).get_order(entry)["status"] == "pending" diff --git a/tests/web/test_orders_view.py b/tests/web/test_orders_view.py index 9fa8e64..6f1f889 100644 --- a/tests/web/test_orders_view.py +++ b/tests/web/test_orders_view.py @@ -721,12 +721,7 @@ def test_there_is_no_cancel_route_at_all() -> None: def test_the_web_package_cannot_reach_a_broker_or_a_credential() -> None: - """The property the whole decision rests on: the worst case of a bug in this layer stays - "reads a local SQLite file" rather than becoming "exfiltrates live trading keys". - - Scanned over the source rather than reasoned about, because the tempting shortcut when adding - a cancel is one import. - """ + """No web module NAMES the broker or credential seams. The narrow half of the property.""" import pathlib web = pathlib.Path(__file__).resolve().parents[2] / "keel" / "web" @@ -736,6 +731,52 @@ def test_the_web_package_cannot_reach_a_broker_or_a_credential() -> None: assert forbidden not in text, f"{path.name} reaches for {forbidden}" +def test_serving_the_orders_page_imports_no_credential_code(tmp_path) -> None: + """The property the whole decision rests on, asserted where it actually lives. + + The source scan above would pass either way, and this PR is the demonstration: it put a + broker-building, venue-cancelling function (`orders_cancel`) into `keel.commands.orders` -- + the module `read_orders` imports on every request -- and that scan never looked past + `keel/web/`. Nothing routes to it, and "nothing routes to it" is what needs asserting. + + So: drive a real request and check that `keel_core.secrets` was never imported. It is the + module that reaches the OS keychain, and its absence from `sys.modules` is the difference + between "reads a local SQLite file" and "holds live trading keys". + """ + import subprocess + import sys + + from keel.data.db import connect, migrate + + db_path = tmp_path / "keel.db" + conn = connect(str(db_path)) + migrate(conn) + conn.close() + + probe = "\n".join( + ( + "import sys", + "from keel.web import api", + "from keel.web.server import ServeConfig", + "cfg = ServeConfig(", + " host='127.0.0.1', port=0, token='t',", + f" db_path={str(db_path)!r}, config_path={str(tmp_path / 'nope.yaml')!r},", + ")", + "api.read_orders(cfg, {}, None, 0)", + "print('secrets' if 'keel_core.secrets' in sys.modules else 'clean')", + ) + ) + + result = subprocess.run( + [sys.executable, "-c", probe], capture_output=True, text=True, check=False + ) + assert result.returncode == 0, result.stderr + assert result.stdout.strip() == "clean", ( + "serving /api/orders imported keel_core.secrets -- the console is no longer " + "credential-free" + ) + + def test_the_console_never_builds_a_control_that_cancels() -> None: """The button opens instructions. `openCancelHelp` may copy text and close itself, and it may not post, fetch or navigate -- there is nothing to post to (see the route test above), and a