refactor(cli): decompose the 2627-line cli.py into a keel/commands package

keel/commands/__init__.py

@@ -0,0 +1,15 @@
+"""Per-topic `click` command groups for the keel CLI.
+
+`keel/cli.py` is the composition root: it defines the root `cli` group, the broker-touching
+commands (`fetch`, `agent`, `monitor`, `simulate`, `assets`) that share the `_build_broker`
+seam, and the other top-level commands, then registers each group defined here via
+`cli.add_command(...)`.
+
+The broker-free command groups live here as standalone modules -- `db`, `trials`, `withdrawals`,
+`autonomy`, `rules`, `subscription` -- so the large CLI file stays a thin wiring layer. They draw
+the shared seams they need (`_open_repo`, `_load_cfg`, the confirmation gate, the disclaimer
+decorator) from `_common`, and the shared product-id derivation from `_products`; neither of those
+helper modules imports `keel.cli`, so there is no cycle. The `assets` group stays in `keel/cli.py`
+on purpose: it uses the `_build_broker` seam the top-level commands also patch, and keeping it
+alongside them keeps that seam a single, coherent monkeypatch target.
+"""

keel/commands/_common.py

@@ -0,0 +1,146 @@
+"""Shared seams and plumbing for the keel CLI command groups.
+
+These helpers are the boundaries that the CLI tests drive: the disclaimer footer, the
+interactive-confirmation gate, DB/config construction, and the one broker-construction seam
+that tests monkeypatch to keep the network out. They live here (rather than in `keel/cli.py`)
+so that command groups extracted into `keel/commands/*` can share them without importing the
+composition root, and so the seams have a single, obvious home.
+
+**Monkeypatch targets.** Historically tests patched `keel.cli._build_broker`, `keel.cli._open_repo`
+and `keel.cli._load_cfg`; `keel/cli.py` re-imports those names, so patching `keel.cli.X` still
+rebinds the copy the top-level commands defined there resolve. The TTY predicate `_is_interactive`
+is different: it is called *internally* by `_require_interactive_confirmation`, so every caller --
+here and in `keel/cli.py` -- reaches it as `_common._is_interactive()` (attribute access on this
+module), and tests patch `keel.commands._common._is_interactive`. That keeps a single, consistent
+patch point no matter which module the calling command lives in.
+
+Note the asymmetry the other way: an extracted command group (`keel/commands/*`) resolves
+`_open_repo`/`_load_cfg`/`_build_broker` in *its own* namespace -- the copies imported from here --
+so patching `keel.cli._open_repo` affects only cli-resident commands (e.g. `agent`). A test that
+drives an extracted group either patches `keel.commands._common.<name>` or, as the group tests
+do, runs against a real `--db` temp path instead of patching at all.
+"""
+
+from __future__ import annotations
+
+import functools
+import sys
+from dataclasses import replace
+from typing import Any
+
+import click
+from keel_core.telemetry import bind_venue
+
+from keel.config import Config, load_config
+from keel.data.db import connect, migrate
+from keel.data.repository import Repository
+from keel.execution.guards import DEFAULT_VENUE
+from keel.logging_setup import configure_logging
+
+DISCLAIMER = (
+    "keel is a personal tool, not financial advice and not religious (Shariah) advice. "
+    "Consult a qualified financial advisor and a knowledgeable scholar before trading. "
+    "You are solely responsible for your own trading decisions."
+)
+
+DEFAULT_DB_PATH = "keel.db"
+DEFAULT_CONFIG_PATH = "config.yaml"
+
+
+# -- disclaimer -------------------------------------------------------------------------------
+
+
+def _print_disclaimer() -> None:
+    click.echo("")
+    click.echo(DISCLAIMER)
+
+
+def with_disclaimer(f: Any) -> Any:
+    """Print the disclaimer footer after `f` runs, whether it succeeds, errors, or aborts."""
+
+    @functools.wraps(f)
+    def wrapper(*args: Any, **kwargs: Any) -> Any:
+        try:
+            return f(*args, **kwargs)
+        finally:
+            _print_disclaimer()
+
+    return wrapper
+
+
+# -- interactive confirmation (no interactive hangs under CliRunner) -----------------------------
+
+
+def _is_interactive() -> bool:
+    """True when a human is at a terminal.
+
+    Deliberately has NO env-var or flag override: any such seam would be settable from cron and
+    would defeat the fail-closed behaviour of every gate built on it. Tests patch this predicate.
+    """
+    return sys.stdin is not None and sys.stdin.isatty()
+
+
+def _require_interactive_confirmation(action: str, detail: str) -> None:
+    """Demand an explicit typed `yes` from a human at a terminal before a dangerous action.
+
+    This replaces the former scrypt passphrase gate. Once placing a real, money-spending order
+    needs only a typed confirmation, requiring a remembered secret to release a safety halt is
+    ceremony without a matching threat model -- on a single-user machine the honest boundary is
+    the OS account, as the old gate's own docstring conceded. One rule ("dangerous actions need a
+    human at a terminal; nothing needs a stored secret") is easier to reason about and to audit.
+
+    Demands the full word `yes` rather than a bare `y`: these actions are rarer and heavier than
+    an order confirmation. **Fails closed off a TTY**, so cron jobs, pipes and scripts can never
+    release a halt.
+    """
+    if not _is_interactive():
+        raise click.ClickException(
+            f"refusing to {action}: this needs confirmation from an interactive terminal."
+        )
+    click.echo(f"About to {action}.")
+    click.echo(f"  {detail}")
+    if click.prompt('Type "yes" to confirm', default="", show_default=False).strip() != "yes":
+        raise click.ClickException("aborted (confirmation not given).")
+
+
+# -- DB / config / broker construction ---------------------------------------------------------
+
+
+def _open_repo(ctx: click.Context) -> Repository:
+    conn = connect(ctx.obj["db_path"])
+    migrate(conn)
+    return Repository(conn)
+
+
+def _load_cfg(ctx: click.Context) -> Config:
+    """Load `config.yaml` and wire up engine-activity logging from it.
+
+    `--verbose`/`-v` on the root `cli` group (`ctx.obj["verbose"]`) overrides
+    `config.logging.verbose` to `True` before `configure_logging` is called, so the flag always
+    wins over whatever `config.yaml` says. Every command that loads config (agent/monitor/
+    simulate/etc.) gets logging configured this way, right when the config it's built from
+    becomes available.
+    """
+    config = load_config(ctx.obj["config_path"])
+    if ctx.obj.get("verbose"):
+        config = replace(config, logging=replace(config.logging, verbose=True))
+    configure_logging(config.logging)
+    # Spec §10.2 names `venue` a stable field on every event. Bound once here, at the one
+    # process entry point, rather than passed into ~26 `log_event` call sites -- the engine is
+    # single-venue today, so threading a constant through every payload would mean revisiting
+    # all of them again the moment it stops being one. A process driving several venues rebinds
+    # per cycle instead; nothing else changes.
+    bind_venue(DEFAULT_VENUE)
+    return config
+
+
+def _build_broker(config: Config) -> Any:  # pragma: no cover -- exercised only against fakes
+    """Construct the real, network-talking `CoinbaseClient`. Tests monkeypatch this function."""
+    from coinbase.rest import RESTClient
+
+    from keel.config import load_secrets
+    from keel.data.cb_client import CoinbaseClient
+
+    secrets = load_secrets()
+    transport = RESTClient(api_key=secrets.get("api_key"), api_secret=secrets.get("api_secret"))
+    return CoinbaseClient(transport)

keel/commands/_products.py

@@ -0,0 +1,32 @@
+"""Product-id derivation shared across CLI commands.
+
+`fetch`, `simulate`, `screen`, `holdings` and `rules seed` must all agree on which product id an
+allowlist asset means, in the deployment's settlement currency. Keeping that derivation in one
+leaf module (depended on by both `keel/cli.py` and the extracted command groups, importing
+neither) is what prevents them from disagreeing.
+"""
+
+from __future__ import annotations
+
+from keel.config import Config
+
+
+def _history_product(asset: str, quote: str) -> str:
+    """The product id for an asset, in the deployment's settlement currency.
+
+    ONE source of truth, shared with `_default_sim_products`. Hardcoding `-USD` here while the
+    screen compared against `config.quote_currency` is what let a `quote_currency: USDC` config
+    reject every asset on a settlement failure it could never fix -- a default change does not
+    change configs already on disk. Deriving both from the same setting means the worst case is
+    an honest "no local history, run `keel fetch`", not a silent unfixable rejection.
+    """
+    return f"{asset}-{quote.upper()}"
+
+
+def _default_sim_products(config: Config) -> list[str]:
+    """Allowlist assets as product ids, in the configured settlement currency.
+
+    Shares `_history_product`'s derivation so `fetch`, `simulate`, `screen` and `holdings`
+    cannot disagree about which product an asset means.
+    """
+    return [_history_product(asset, config.quote_currency) for asset in config.allowlist]

keel/commands/autonomy.py

@@ -0,0 +1,126 @@
+"""`keel autonomy` -- whether the agent places orders without asking first.
+
+Autonomy is a profile choice stored in the repository and re-read by `agent.run_once` every
+cycle. `on` RELEASES the confirm prompt, so it demands a typed `yes` at a terminal via the shared
+gate; `off` only ever reduces capability and stays ungated. It changes *who is asked*, never
+*what is allowed* -- the hard rails run in every mode.
+"""
+
+from __future__ import annotations
+
+import time
+
+import click
+
+from keel.commands._common import (
+    _load_cfg,
+    _open_repo,
+    _require_interactive_confirmation,
+    with_disclaimer,
+)
+
+#: Upper bound on `keel autonomy on --for-hours` (1 year). Guards against inf/nan/overflow
+#: and against a "window" so long it is indistinguishable from no expiry at all.
+_MAX_AUTONOMY_HOURS = 8760.0
+#: One second. Below this the window rounds to zero and we would write an already-lapsed row
+#: while printing "autonomy ON until ..." -- fails safe, but the message would be a lie.
+_MIN_AUTONOMY_HOURS = 1.0 / 3600.0
+
+
+@click.group("autonomy")
+def autonomy_group() -> None:
+    """Whether the agent places orders without asking you first."""
+
+
+@autonomy_group.command("show")
+@click.pass_context
+@with_disclaimer
+def autonomy_show(ctx: click.Context) -> None:
+    """Print the current autonomy setting."""
+    repo = _open_repo(ctx)
+    profile = repo.get_profile()
+    # `_open_repo` runs `migrate()`, which recreates a merely MISSING table -- so this covers
+    # damage migrate cannot heal (a corrupt page, a table of the wrong shape), not a fresh DB.
+    if not repo.profile_readable():
+        click.echo(
+            "WARNING: the profile row could not be read (see the log). Reporting autonomy as "
+            "OFF, which is the safe reading -- but the stored setting is UNKNOWN.",
+            err=True,
+        )
+    now_ts = int(time.time())
+    live = profile.is_autonomous(now_ts)
+    state = (
+        "ON -- orders are placed WITHOUT asking"
+        if live
+        else "off -- every order asks first"
+    )
+    click.echo(f"autonomy: {state}")
+    if profile.autonomous and not live:
+        click.echo(f"  (was ON but LAPSED at {profile.autonomous_until})")
+    elif live and profile.autonomous_until is not None:
+        left = profile.autonomous_until - now_ts
+        click.echo(f"  lapses at {profile.autonomous_until} ({left}s left)")
+    elif live:
+        click.echo("  no expiry set -- stays on until `keel autonomy off`")
+    if profile.updated_ts:
+        click.echo(f"  last changed: {profile.updated_ts}")
+
+
+@autonomy_group.command("on")
+@click.option(
+    "--for-hours",
+    "for_hours",
+    type=float,
+    default=None,
+    help="Let autonomy LAPSE automatically after this many hours (default: never lapses).",
+)
+@click.pass_context
+@with_disclaimer
+def autonomy_on(ctx: click.Context, for_hours: float | None) -> None:
+    """Let the agent place orders without asking (dangerous: asks for confirmation).
+
+    Every order is still subject to all hard rails -- autonomy changes who is asked, never what
+    is allowed. It does NOT let the agent clear a safety halt: releasing the kill-switch or a
+    drawdown breaker always needs a human, whatever this is set to.
+    """
+    config = _load_cfg(ctx)
+    repo = _open_repo(ctx)
+    now_ts = int(time.time())
+    if for_hours is not None and not (_MIN_AUTONOMY_HOURS <= for_hours <= _MAX_AUTONOMY_HOURS):
+        # 0/negative would write an already-lapsed row while printing "autonomy ON until ...",
+        # and inf/nan/1e18 would overflow int() after the operator had already typed `yes`.
+        raise click.BadParameter(
+            f"--for-hours must be at least {_MIN_AUTONOMY_HOURS} (one second) and at most "
+            f"{_MAX_AUTONOMY_HOURS} ({int(_MAX_AUTONOMY_HOURS) // 24} days); got {for_hours!r}."
+        )
+    expires_ts = None if for_hours is None else now_ts + int(for_hours * 3600)
+    window = (
+        "until you turn it off" if expires_ts is None else f"for {for_hours}h (until {expires_ts})"
+    )
+    _require_interactive_confirmation(
+        "turn autonomy ON",
+        f"Orders will be placed with NO further prompt, {window} "
+        f"(mode={config.auto_trade.mode}, allowlist={config.allowlist}).",
+    )
+    repo.set_autonomous(True, now_ts, expires_ts=expires_ts)
+    if expires_ts is None:
+        click.echo(
+            "autonomy ON, with NO expiry -- it stays on until you run `keel autonomy off`.\n"
+            "  Consider `--for-hours N` for a supervised session, so a forgotten `on` cannot "
+            "grant unattended trading indefinitely."
+        )
+    else:
+        click.echo(f"autonomy ON until {expires_ts}. It lapses on its own after that.")
+
+
+@autonomy_group.command("off")
+@click.pass_context
+@with_disclaimer
+def autonomy_off(ctx: click.Context) -> None:
+    """Require confirmation before every order again.
+
+    Deliberately ungated and usable without a terminal: reducing risk must never be obstructed,
+    so this works from a script, a cron job or a pipe. Arming is what needs a human.
+    """
+    _open_repo(ctx).set_autonomous(False, int(time.time()))
+    click.echo("autonomy off: every order will ask for confirmation.")

keel/commands/db.py

@@ -0,0 +1,26 @@
+"""`keel db` -- local data import/maintenance (read-only with respect to the exchange)."""
+
+from __future__ import annotations
+
+import click
+
+from keel.commands._common import _open_repo, with_disclaimer
+from keel.data.csv_import import import_dir
+
+
+@click.group("db")
+def db_group() -> None:
+    """Local data import/maintenance commands."""
+
+
+@db_group.command("import")
+@click.argument("dir_path", type=click.Path(exists=True, file_okay=False))
+@click.pass_context
+@with_disclaimer
+def db_import(ctx: click.Context, dir_path: str) -> None:
+    """Import every `*.csv` Coinbase export in DIR_PATH (read-only w.r.t. the exchange)."""
+    repo = _open_repo(ctx)
+    result = import_dir(dir_path, repo)
+    click.echo(f"imported={result.imported} skipped={result.skipped}")
+    for warning in result.warnings:
+        click.echo(f"  warning: {warning}")