diff --git a/keel/commands/orders.py b/keel/commands/orders.py index 6847d36..49f36c4 100644 --- a/keel/commands/orders.py +++ b/keel/commands/orders.py @@ -49,16 +49,21 @@ import datetime import json +import logging 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 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 @@ -310,6 +315,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) @@ -473,7 +490,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. @@ -521,8 +544,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, @@ -634,7 +663,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 @@ -805,7 +841,205 @@ 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") + +#: 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. +#: 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", + "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.", +} + + + +# 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. + + 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 = "" + #: 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, 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. + """ + if row is None: + 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 = ( + 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": + 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, + invocation=CANCEL_INVOCATION.format(order_id=order_id), + headline=CANCEL_HEADLINES[kind].format(order_id=order_id), + ) + + +@click.group("orders", invoke_without_command=True) @click.option( "--scope", type=click.Choice(ORDERS_SCOPES), @@ -823,7 +1057,13 @@ def render_orders(report: OrdersReport) -> list[str]: @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` + 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 @@ -834,7 +1074,213 @@ 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 +def orders_list(ctx: click.Context, scope: str, limit: int) -> None: + """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) + + +#: 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 +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 CancelPending, CancelUnavailable, _cancel_at_exchange + + 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)) + + # 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) + 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 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 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/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..b0850a4 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,99 @@ 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; + // 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 + // 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 new file mode 100644 index 0000000..14c775c --- /dev/null +++ b/tests/commands/test_orders_cancel.py @@ -0,0 +1,672 @@ +"""`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, refuse: tuple[str, ...] = ()) -> None: + self.cancelled: list[str] = [] + self.refuse = refuse + + def cancel_order(self, native_id: str) -> bool: + self.cancelled.append(native_id) + # `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() +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 == [] + + +# -- 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" + + +# -- 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 6686936..6f1f889 100644 --- a/tests/web/test_orders_view.py +++ b/tests/web/test_orders_view.py @@ -702,3 +702,135 @@ 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: + """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" + 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_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 + 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"]