From 0450c2089c50da4af7d4fc61bdd962e9545e8609 Mon Sep 17 00:00:00 2001 From: Elmehdi Aitbrahim Date: Tue, 21 Jul 2026 18:49:10 -0400 Subject: [PATCH 1/9] docs(spec): security simplification design (items 1/2) Delete the vault (already dead code) and the passphrase gate; autonomy becomes a live-read profile choice. Rails unchanged -- this changes who is asked, never what is allowed. Co-Authored-By: Claude Opus 4.8 (1M context) --- ...26-07-21-security-simplification-design.md | 223 ++++++++++++++++++ 1 file changed, 223 insertions(+) create mode 100644 docs/superpowers/specs/2026-07-21-security-simplification-design.md diff --git a/docs/superpowers/specs/2026-07-21-security-simplification-design.md b/docs/superpowers/specs/2026-07-21-security-simplification-design.md new file mode 100644 index 00000000..5baf49c4 --- /dev/null +++ b/docs/superpowers/specs/2026-07-21-security-simplification-design.md @@ -0,0 +1,223 @@ +# Security simplification — design + +**Date:** 2026-07-21 +**Status:** Approved design (pending user review) +**Workstream:** A of three. Items 1 and 2 of the 2026-07-21 requirements. + +## Context + +`keel` grew two local security mechanisms from the original spec §14: + +- an **encrypted secrets vault** (`keel/security/secrets.py`) — AES-GCM over a scrypt-derived key, + holding CDP credentials in `secrets.enc`; +- a **dangerous-action passphrase gate** (`keel/security/authz.py`) — scrypt-hashed passphrase with + rate limiting, gating `{arm_bypass, raise_caps, disable_killswitch, unlock_vault}`. + +Both are being removed. The app will be **server-hosted with real user authentication** in the +future; between here and there, a local single-user vault and passphrase buy little. The honest +assessment was already in `authz.py`'s own docstring: it "does not stop an attacker who already +holds the OS account." + +**Two findings materially shrink this work:** + +1. **The vault is already dead code.** `data/cb_client.py` and `cli.py` both load credentials via + `config.load_secrets()` from a git-ignored `.env`. The vault is reachable only through + `migrate_from_env`. Removing it deletes a *competing* credential path; it does not change how + credentials are actually loaded today. (PR #115, which would have wired it up, was closed.) +2. **PR #117 (merged) already did half of item 2.** Confirm mode now places orders via an + interactive prompt with no passphrase and no bypass token, rails running first, failing closed + on a non-TTY. What remains is the *autonomous* half. + +## Goals + +1. **Item 1 — `.env` only.** Delete the vault. Credentials come from a git-ignored `.env`. +2. **Item 2 — autonomy is a profile choice.** Confirmation before every order is the default. When + the user turns **autonomous mode** on, there is no passphrase and no per-order confirmation. The + choice is **tracked in the user's profile**, persisted in the database and read live. +3. **Delete the passphrase gate entirely**, re-gating the four halt-releasing commands with the + same interactive confirmation #117 established for orders. + +## Non-goals + +- No user authentication, accounts, or sessions — those arrive with the hosted deployment. +- **No change to the 17 hard rails.** They remain un-overridable in every mode. This work changes + *who is asked*, never *what is allowed*. +- No change to the kill-switch's fail-closed default. + +## Core safety invariants (must hold after this work) + +These are the properties the removed ceremony was standing in for. Each gets a test. + +1. **Rails run first, always.** Guards veto before any preview or placement, in every mode. +2. **Autonomy never clears a safety halt.** `resume`, `resume-entries`, `record-flow` and + `reset-hwm` require an interactive confirmation **even when autonomous mode is on**. "Trade + without asking me" and "un-stick your own drawdown breaker" are different powers. A rail that + fired because something went wrong still needs a human to clear it — otherwise a breaker can + silently reset itself and the rail stops meaning anything. +3. **Autonomy fails closed.** An absent/unreadable profile row reads as `autonomous = False`. +4. **Autonomy cannot be enabled non-interactively.** `keel autonomy on` requires a TTY and an + explicit typed confirmation, so a script, cron job or piped command can never arm it. +5. **The kill-switch still short-circuits everything**, checked first, defaulting to engaged. +6. **Paper mode never places**, regardless of the profile flag. + +--- + +## Design + +### 3.1 Delete the vault (item 1) + +Remove `keel/security/secrets.py` and `tests/security/test_secrets.py`. Drop the now-unused +`cryptography>=49.0.0` dependency from `pyproject.toml` (verified: used nowhere else in `keel`, +`packages`, or `tests`). `config.load_secrets()` is unchanged and remains the single credential +path. + +### 3.2 Delete the passphrase gate, re-gate the four halt-releasing commands + +Remove `keel/security/authz.py` and `tests/security/test_authz.py`. With both files gone the +`keel/security/` package has no remaining contents and is deleted entirely. + +Of the four declared dangerous actions, `raise_caps` and `unlock_vault` were **never used** (caps +are config-file-only; the vault is going). `arm_bypass` disappears with bypass mode. That leaves +four *commands*, all one idea — re-permitting trading after a safety halt: + +| command | what it releases | +|---|---| +| `resume` | the kill-switch | +| `resume-entries` | an armed consecutive-loss halt (rail 16) | +| `record-flow` | declares an external deposit/withdrawal so rail 11 isn't fooled | +| `reset-hwm` | rail 11's equity high-water mark, clearing a stuck drawdown halt | + +Each gains `_require_interactive_confirmation(action, detail)`: prints what is about to be +released and demands an explicit typed `yes` (not bare `y` — these are rarer and heavier than an +order confirmation), and **fails closed on a non-TTY**. `--passphrase` options and `--authz-path` +are removed from the CLI. + +**Rationale for parity with orders:** once placing a real money-spending order needs only a typed +confirmation, requiring a remembered secret to reset a high-water mark is ceremony without a +matching threat model. One rule — *dangerous actions need a human at a terminal; nothing needs a +stored secret* — is easier to reason about, and to audit, than two. + +### 3.3 Autonomy as a live-read profile choice (item 2) + +**New `profile` table** (schema 6 → 7, via the existing incremental `db.migrate` chain): + +```sql +CREATE TABLE IF NOT EXISTS profile ( + id INTEGER PRIMARY KEY CHECK (id = 1), -- single row today + autonomous INTEGER NOT NULL DEFAULT 0, + updated_ts INTEGER NOT NULL +); +``` + +`CHECK (id = 1)` keeps it a single row deliberately rather than by accident; a `user_id` column is +the obvious seam when the hosted, multi-user deployment arrives. + +**Repository API:** + +- `get_profile() -> Profile` — returns `Profile(autonomous: bool, updated_ts: int)`. **An absent + row returns `autonomous=False`**, so a fresh or damaged database fails closed. +- `set_autonomous(value: bool, now_ts: int) -> None` — upserts the single row. + +**Read live, every cycle.** `agent` calls `repo.get_profile()` on each order decision and never +caches it, so `keel autonomy off` takes effect on the **next order**, not the next restart. This +mirrors rail 14's monthly allowance, which was deliberately moved from config to the database for +exactly this property. + +**Why not `config.yaml`:** it is now a *shipped release asset* in confirm mode. Arming unattended +trading should not be a YAML line that travels between machines or gets pasted from a gist. Why +not `agent_state`: that holds operational state (kill-switch, open positions); this is a durable +user preference. + +**Mode vocabulary.** `auto_trade.mode` collapses from `paper | confirm | bypass` to +**`paper | confirm`**, and is now **validated** (an unknown value raises `ConfigError` naming the +key — today it silently falls back). `bypass_arm_ttl_sec` is removed. Effective behaviour: + +| `mode` | `profile.autonomous` | result | +|---|---|---| +| `paper` | *(ignored)* | simulated; places nothing | +| `confirm` | `false` *(default)* | live; prompts before every order | +| `confirm` | `true` | live; places without prompting | + +One switch for *is this real money*, a separate one for *do you ask me* — rather than one enum +conflating both. The shipped `config.yaml` needs no change to stay safe: autonomy is off until +someone deliberately turns it on. + +⚠️ **Breaking:** a config carrying `mode: bypass` or `bypass_arm_ttl_sec` now fails to load. That +is deliberate — failing loudly beats silently reinterpreting a config that requested autonomy. + +**Removed machinery:** `Repository.arm_bypass` / `is_bypass_armed` / `disarm_bypass`, the +`agent_state` bypass token, `agent._confirm_or_bypass`, `LoopResult.bypass_refused_reason`, and +the `keel arm-bypass` / `disarm-bypass` commands and `agent --bypass` flag. + +**Executor:** `mode: Literal["confirm", "bypass"]` becomes `Literal["confirm", "autonomous"]`. +`"autonomous"` places without consulting `confirm_fn`; `"confirm"` still requires an approving +`confirm_fn` and fails closed without one. Guard ordering is untouched. + +**Agent:** `_confirm_or_bypass` is replaced by `_effective_mode(config, repo) -> str`, which +returns `"autonomous"` only when `config.auto_trade.mode == "confirm"` **and** +`repo.get_profile().autonomous` is true; anything else is `"confirm"`. Paper mode is handled +upstream as today. + +### 3.4 CLI: `keel autonomy` + +``` +keel autonomy show # prints on/off and when it was last changed +keel autonomy on # interactive: explains the consequences, demands a typed "yes" +keel autonomy off # ungated -- de-risking is always allowed (§5 asymmetry) +``` + +`on` is a dangerous action: it prints the current mode, caps and allowlist, states plainly that +orders will be placed without asking, and requires a typed `yes` on a TTY. `off` is deliberately +ungated and needs no TTY — the asymmetry principle that runs through this project is that +*reducing* risk should never be obstructed, while *increasing* it goes through a gate. + +### 3.5 Rewrite the go-live runbook + +`docs/go-live-runbook.md` (proposed in the unmerged PR #116) documents the old +`arm-bypass`/`--passphrase` dance, which #117 already invalidated and this work removes entirely. +It is rewritten here against the real flow: `.env` → `keel migrate`/`init` → promote a rule → +`keel agent` in confirm mode → approve one tiny supervised order → optionally `keel autonomy on`. +**PR #116 is superseded and should be closed** rather than merged. + +--- + +## Components & interfaces + +| File | Change | +|---|---| +| `keel/security/` | **deleted** (both `secrets.py` and `authz.py`; package removed) | +| `tests/security/` | **deleted** | +| `pyproject.toml` | drop `cryptography` | +| `keel/data/db.py` | `SCHEMA_VERSION = 7`; `profile` table + `_migrate_v7_profile` | +| `keel/data/repository.py` | `get_profile()`, `set_autonomous()`; remove the three bypass methods | +| `keel/types.py` | `Profile` dataclass | +| `keel/agent.py` | `_effective_mode` replaces `_confirm_or_bypass`; drop `bypass_refused_reason` | +| `keel/execution/executor.py` | mode literal `confirm`/`autonomous` | +| `keel/cli.py` | `autonomy` group; `_require_interactive_confirmation`; remove authz/bypass surface | +| `packages/keel-core/keel_core/config.py` | validate `mode ∈ {paper, confirm}`; drop `bypass_arm_ttl_sec` | +| `config.yaml`, `keel/templates/config.yaml`, `keel/templates/config.live.yaml` | drop `bypass_arm_ttl_sec` + its comment (root and dev template must stay byte-identical) | +| `tests/fixtures/config_golden_*.{json,yaml}` | drop `bypass_arm_ttl_sec` | +| `docs/go-live-runbook.md` | written fresh (supersedes #116) | +| `docs/superpowers/specs/...-halal-cb-autotrade-design.md` | §14 amended to record the removal and why | + +## Testing + +Beyond updating existing tests, each safety invariant in §"Core safety invariants" gets a test: + +- a rail-vetoed order never reaches placement in **autonomous** mode (rails first); +- each of the four halt-releasing commands still demands confirmation **with `autonomous=true`**; +- absent profile row ⇒ `autonomous False`; +- `autonomy on` refuses without a TTY; `autonomy off` works without one; +- profile is re-read per cycle (flip it between two `run_once` calls, observe the change); +- `mode: paper` places nothing with `autonomous=true`; +- kill-switch engaged short-circuits before any of it; +- `mode: bypass` in a config now raises `ConfigError` naming the key; +- a v6 database migrates to v7 and gains a `profile` row default-off. + +## Migration notes for the operator + +- Delete any `secrets.enc` and `authz.json` — both are now ignored. (Neither is read; no automatic + deletion is performed, since removing files the user owns is not this tool's business.) +- Remove `bypass_arm_ttl_sec` from `config.yaml`, and change `mode: bypass` to `mode: confirm` + plus `keel autonomy on`. +- Run `keel migrate` to add the `profile` table. From abca7b3dbadfee60dacdae00560206866a935f89 Mon Sep 17 00:00:00 2001 From: Elmehdi Aitbrahim Date: Tue, 21 Jul 2026 19:04:34 -0400 Subject: [PATCH 2/9] feat(db): profile table (schema 7) carrying the user's autonomy choice get_profile() FAILS CLOSED -- an absent row reports autonomous=False, because the safe reading of 'no record' is that the user never opted into unattended trading. Single-row by CHECK(id=1); user_id is the seam for hosted multi-user. Retires the arm_bypass/is_bypass_armed/disarm_bypass token API. Co-Authored-By: Claude Opus 4.8 (1M context) --- .../2026-07-21-security-simplification.md | 78 +++++++++++++++++++ keel/data/db.py | 21 ++++- keel/data/repository.py | 58 +++++++------- packages/keel-core/keel_core/types.py | 15 +++- tests/data/test_db.py | 25 ++++++ tests/data/test_migrations.py | 2 +- tests/data/test_repository.py | 75 +++++++----------- tests/data/test_trade_outcomes.py | 4 +- 8 files changed, 194 insertions(+), 84 deletions(-) create mode 100644 docs/superpowers/plans/2026-07-21-security-simplification.md diff --git a/docs/superpowers/plans/2026-07-21-security-simplification.md b/docs/superpowers/plans/2026-07-21-security-simplification.md new file mode 100644 index 00000000..fce0f20e --- /dev/null +++ b/docs/superpowers/plans/2026-07-21-security-simplification.md @@ -0,0 +1,78 @@ +# Security simplification — Implementation Plan + +> **For agentic workers:** REQUIRED SUB-SKILL: superpowers:executing-plans. Steps use `- [ ]` checkboxes. + +**Goal:** Delete the vault and the passphrase gate; make autonomy a live-read profile choice. + +**Architecture:** A new single-row `profile` table (schema 6→7) holds `autonomous`, read live on every order decision. `auto_trade.mode` collapses to `paper|confirm`. The four halt-releasing commands swap the passphrase for an interactive typed confirmation. Rails are untouched throughout. + +**Tech Stack:** Python 3.12, click, sqlite3, pytest. + +## Global Constraints + +- **The 17 rails do not change.** This changes *who is asked*, never *what is allowed*. +- Every safety invariant in the spec's "Core safety invariants" gets a test. +- Autonomy fails closed (absent row ⇒ `False`); `autonomy on` needs a TTY; `autonomy off` never does. +- Autonomy **never** clears a safety halt. +- Full suite + `uv run ruff check keel tests packages scripts` green before each commit. + +--- + +### Task 1: `profile` table + Repository API (schema 6→7) + +**Files:** `keel/data/db.py`, `keel/data/repository.py`, `keel/types.py`, `tests/data/test_repository.py` + +**Produces:** `Profile(autonomous: bool, updated_ts: int)`; `Repository.get_profile() -> Profile`; `Repository.set_autonomous(value: bool, now_ts: int) -> None`; `db.SCHEMA_VERSION == 7`. + +- [ ] Failing tests: absent row ⇒ `autonomous False`; set/get round-trips; `set_autonomous` upserts (never a 2nd row); a v6 DB migrates to v7 and gains the table; `SCHEMA_VERSION == 7` literal tripwire. +- [ ] Run → FAIL. Implement table + `_migrate_v7_profile` + repo methods + `Profile`. Run → PASS. ruff. Commit. + +### Task 2: Delete the vault and the passphrase gate + +**Files:** delete `keel/security/`, `tests/security/`; `pyproject.toml`; `keel/cli.py` + +**Produces:** `_require_interactive_confirmation(action: str, detail: str) -> None` in `cli.py` (typed `yes`, fails closed off-TTY). + +- [ ] Failing tests: each of `resume`/`resume-entries`/`record-flow`/`reset-hwm` proceeds on typed `yes`, aborts on anything else, aborts with no TTY. +- [ ] Run → FAIL. Delete both modules + their tests + the `cryptography` dep; remove `_require_authz`, `--passphrase`, `--authz-path`, `DEFAULT_AUTHZ_PATH`; add `_require_interactive_confirmation` and wire the four commands. Run → PASS. ruff. Commit. + +### Task 3: Autonomy in the agent and executor + +**Files:** `keel/agent.py`, `keel/execution/executor.py`, `tests/test_agent.py`, `tests/execution/test_executor.py` + +**Consumes:** `Repository.get_profile()`. +**Produces:** `agent._effective_mode(config, repo) -> Literal["confirm","autonomous"]`; executor `mode: Literal["confirm","autonomous"]`. + +- [ ] Failing tests: `autonomous=True` places with no `confirm_fn`; `autonomous=False` still fails closed without one; **a rail veto never reaches placement in autonomous mode**; profile re-read per cycle (flip between two `run_once` calls); `mode: paper` places nothing even when autonomous; kill-switch short-circuits first. +- [ ] Run → FAIL. Replace `_confirm_or_bypass` with `_effective_mode`; retire `arm_bypass`/`is_bypass_armed`/`disarm_bypass` and `bypass_refused_reason`; rename the executor literal. Run → PASS. ruff. Commit. + +### Task 4: `keel autonomy on|off|show` + +**Files:** `keel/cli.py`, `tests/test_cli.py` + +- [ ] Failing tests: `show` prints off by default; `on` with typed `yes` enables and persists; `on` aborts off-TTY and on a non-`yes` answer; `off` disables **without** a TTY; **`on` does not let the four halt commands skip their confirmation**. +- [ ] Run → FAIL. Add the `autonomy` group; remove `arm-bypass`/`disarm-bypass`/`--bypass`. Run → PASS. ruff. Commit. + +### Task 5: Config — validate `mode`, drop `bypass_arm_ttl_sec` + +**Files:** `packages/keel-core/keel_core/config.py`, `config.yaml`, `keel/templates/config.yaml`, `keel/templates/config.live.yaml`, `tests/fixtures/config_golden_*`, `tests/test_config.py` + +- [ ] Failing tests: `mode: bypass` raises `ConfigError` naming the key; `paper`/`confirm` load; `bypass_arm_ttl_sec` is gone from the parsed config. +- [ ] Run → FAIL. Validate mode, drop the field, update all three configs (**root and dev template must stay byte-identical**) and the golden fixtures. Run → PASS. ruff. Commit. + +### Task 6: Docs — go-live runbook + spec §14 + +**Files:** `docs/go-live-runbook.md`, the main design spec's §14 + +- [ ] Write the runbook against the real flow: `.env` → `keel migrate`/`init` → promote a rule → `keel agent` (confirm) → one tiny supervised order → optional `keel autonomy on`. Amend §14 to record the removal and why. Commit. + +### Task 7: Verification + +- [ ] `uv run pytest -q` green; `uv run ruff check keel tests packages scripts` clean. +- [ ] Grep-prove the surface is gone: no `authz`, `save_vault`, `arm_bypass`, `bypass` outside history/docs. +- [ ] Smoke: `keel migrate` on a v6 DB; `keel autonomy show/on/off`; `keel autonomy on` piped (must refuse). + +## Self-Review + +- **Spec coverage:** §3.1→T2, §3.2→T2, §3.3→T1+T3+T5, §3.4→T4, §3.5→T6. Invariants 1,3,6→T3; 2→T4; 4→T4; 5→T3. +- **Placeholders:** none. **Type consistency:** `Profile`, `get_profile`, `set_autonomous`, `_effective_mode`, `_require_interactive_confirmation` used identically across tasks. diff --git a/keel/data/db.py b/keel/data/db.py index e9263318..de29e971 100644 --- a/keel/data/db.py +++ b/keel/data/db.py @@ -19,7 +19,7 @@ from pathlib import Path from typing import Any -SCHEMA_VERSION = 6 +SCHEMA_VERSION = 7 # Creation order matters for readability (and for backends that validate FK targets eagerly); # SQLite itself only checks FK targets at DML time, but we still declare referenced tables first. @@ -238,6 +238,13 @@ attested_at INTEGER NOT NULL ) """, + """ + CREATE TABLE IF NOT EXISTS profile ( + id INTEGER PRIMARY KEY CHECK (id = 1), + autonomous INTEGER NOT NULL DEFAULT 0, + updated_ts INTEGER NOT NULL + ) + """, ) @@ -357,12 +364,24 @@ def _migrate_v6_asset_attestations(conn: sqlite3.Connection) -> None: """ +def _migrate_v7_profile(conn: sqlite3.Connection) -> None: + """v7 adds `profile`, which carries the user's autonomy choice. Table creation is handled by + `_SCHEMA_STATEMENTS`; there is deliberately NO backfill. + + No row means `get_profile()` reports `autonomous=False`, which is the correct and safe + reading of an upgraded database: the user has never opted into unattended trading, so we must + not infer that they did. Seeding a row here -- even an explicitly `autonomous=0` one -- would + only manufacture a consent record that no human gave. + """ + + _MIGRATIONS: dict[int, Callable[[sqlite3.Connection], None]] = { 2: _migrate_v2_broker_subscriptions, 3: _migrate_v3_trade_outcomes, 4: _migrate_v4_positions, 5: _migrate_v5_candle_gap_probes, 6: _migrate_v6_asset_attestations, + 7: _migrate_v7_profile, } diff --git a/keel/data/repository.py b/keel/data/repository.py index 0f6666d1..f4358abe 100644 --- a/keel/data/repository.py +++ b/keel/data/repository.py @@ -15,7 +15,7 @@ from keel_core.subscription import BrokerSubscription, SubscriptionStatus -from keel.types import Candle, Granularity +from keel.types import Candle, Granularity, Profile _TRANSACTION_COLUMNS = ( "coinbase_id", @@ -580,42 +580,36 @@ def _position_row_to_dict(self, row: sqlite3.Row) -> dict[str, Any]: d[field] = _text_to_dec(d[field]) return d - # -- bypass arm token (Issue #60, in-process bypass hardening) --------- + # -- profile (the user's own settings) ------------------------------------ - def arm_bypass(self, now_ts: int, ttl_sec: int) -> None: - """Arm autonomous bypass mode for `ttl_sec` seconds starting at `now_ts`. + def get_profile(self) -> Profile: + """The user's profile, defaulting to NOT autonomous. - Overwrites any previous arm token outright -- there is only ever one live token, and - arming again (e.g. re-running `keel arm-bypass`) always resets the window from `now_ts` - rather than extending the old one. `agent.run_once`'s own `is_bypass_armed` check reads - this token fresh on every cycle, so it is the one place bypass mode can be authorized - from -- CLI (`keel arm-bypass`, passphrase-gated) or any other authenticated caller. - """ - self.set_state( - "bypass_arm", - {"armed_at": now_ts, "armed_until": now_ts + ttl_sec}, - ) + **Fails closed.** An absent row -- a fresh database, or one upgraded from before the + `profile` table existed -- reports `autonomous=False`. The safe reading of "no record" + is that the user never opted into unattended trading, never that they did. - def is_bypass_armed(self, now_ts: int) -> bool: - """True iff a bypass-arm token exists and `now_ts` is still inside its window. - - Freshness is a strict `now_ts < armed_until` (matching `market_feed.is_fresh`'s own - convention elsewhere in this codebase) -- the instant `armed_until` is reached, the - token is treated as expired, not one tick still-good. + Callers must re-read this per order decision rather than caching it, so that + `keel autonomy off` takes effect on the next order instead of the next restart. """ - token = self.get_state("bypass_arm") - if token is None: - return False - return now_ts < token["armed_until"] - - def disarm_bypass(self) -> None: - """Clear the bypass-arm token immediately. + row = self._conn.execute( + "SELECT autonomous, updated_ts FROM profile WHERE id = 1" + ).fetchone() + if row is None: + return Profile() + return Profile(autonomous=bool(row["autonomous"]), updated_ts=int(row["updated_ts"])) - Fail-safe direction: disarming only ever *reduces* capability, so unlike `arm_bypass` - this needs no passphrase gate at the CLI layer -- it is always safe to call, including - when nothing is currently armed (a no-op). - """ - self.set_state("bypass_arm", None) + def set_autonomous(self, value: bool, now_ts: int) -> None: + """Record the user's autonomy choice, upserting the single profile row.""" + self._conn.execute( + """ + INSERT INTO profile (id, autonomous, updated_ts) VALUES (1, ?, ?) + ON CONFLICT(id) DO UPDATE SET autonomous = excluded.autonomous, + updated_ts = excluded.updated_ts + """, + (1 if value else 0, now_ts), + ) + self._conn.commit() # -- candle gap probes ---------------------------------------------------- # A row asserts: "we asked the venue for this exact window and it returned nothing new." diff --git a/packages/keel-core/keel_core/types.py b/packages/keel-core/keel_core/types.py index 5b0b3afd..ed6fc17c 100644 --- a/packages/keel-core/keel_core/types.py +++ b/packages/keel-core/keel_core/types.py @@ -41,4 +41,17 @@ class Candle: volume: Decimal -__all__ = ["Granularity", "Side", "Candle"] +@dataclass(frozen=True) +class Profile: + """The user's own settings, as opposed to operational state or file configuration. + + `autonomous` is the single choice today: when true, the agent places rule-generated orders + without asking. It is stored in the database (not `config.yaml`) and re-read on every order + decision, so turning it off takes effect on the NEXT order rather than the next restart. + """ + + autonomous: bool = False + updated_ts: int = 0 + + +__all__ = ["Granularity", "Side", "Candle", "Profile"] diff --git a/tests/data/test_db.py b/tests/data/test_db.py index c472e9f1..d1dedf7e 100644 --- a/tests/data/test_db.py +++ b/tests/data/test_db.py @@ -101,3 +101,28 @@ def test_agent_state_table_has_key_primary_key(): columns = conn.execute("PRAGMA table_info(agent_state)").fetchall() pk_columns = {row["name"] for row in columns if row["pk"] > 0} assert pk_columns == {"key"} + + +def test_schema_version_is_7(): + """Deliberate tripwire: bump this literal consciously on every schema change.""" + from keel.data.db import SCHEMA_VERSION + + assert SCHEMA_VERSION == 7 + + +def test_a_v6_database_migrates_up_and_gains_the_profile_table(tmp_path): + from keel.data.db import SCHEMA_VERSION, connect, migrate + + conn = connect(str(tmp_path / "old.db")) + migrate(conn) + conn.execute("UPDATE schema_version SET version = 6") + conn.commit() + + migrate(conn) + assert int(conn.execute("SELECT version FROM schema_version").fetchone()["version"]) == ( + SCHEMA_VERSION + ) + named = conn.execute( + "SELECT name FROM sqlite_master WHERE type='table' AND name='profile'" + ).fetchone() + assert named is not None, "v7 must add the profile table" diff --git a/tests/data/test_migrations.py b/tests/data/test_migrations.py index bb2bd7e6..a1a3053d 100644 --- a/tests/data/test_migrations.py +++ b/tests/data/test_migrations.py @@ -46,7 +46,7 @@ def test_fresh_database_is_stamped_at_the_current_version() -> None: conn = db.connect(":memory:") db.migrate(conn) version = conn.execute("SELECT version FROM schema_version").fetchone()["version"] - assert version == db.SCHEMA_VERSION == 6 + assert version == db.SCHEMA_VERSION == 7 def test_fresh_database_gets_no_subscription_row() -> None: diff --git a/tests/data/test_repository.py b/tests/data/test_repository.py index ffb87c6c..b6725434 100644 --- a/tests/data/test_repository.py +++ b/tests/data/test_repository.py @@ -218,53 +218,6 @@ def test_set_state_overwrites_existing_key(repo): assert repo.get_state("kill_switch") is True -# -- bypass arm token (Issue #60, in-process bypass hardening) --------------- - - -def test_is_bypass_armed_false_when_never_armed(repo): - assert repo.is_bypass_armed(now_ts=1_700_000_000) is False - - -def test_is_bypass_armed_true_right_after_arm_within_ttl(repo): - repo.arm_bypass(now_ts=1_700_000_000, ttl_sec=3600) - - assert repo.is_bypass_armed(now_ts=1_700_000_001) is True - assert repo.is_bypass_armed(now_ts=1_700_003_599) is True - - -def test_is_bypass_armed_false_once_now_ts_reaches_armed_until(repo): - repo.arm_bypass(now_ts=1_700_000_000, ttl_sec=3600) - - # armed_until = 1_700_000_000 + 3600 = 1_700_003_600 -- freshness is a strict `<`. - assert repo.is_bypass_armed(now_ts=1_700_003_600) is False - assert repo.is_bypass_armed(now_ts=1_700_004_000) is False - - -def test_is_bypass_armed_false_after_disarm(repo): - repo.arm_bypass(now_ts=1_700_000_000, ttl_sec=3600) - assert repo.is_bypass_armed(now_ts=1_700_000_001) is True - - repo.disarm_bypass() - - assert repo.is_bypass_armed(now_ts=1_700_000_001) is False - - -def test_arm_bypass_overwrites_a_previous_arm(repo): - repo.arm_bypass(now_ts=1_700_000_000, ttl_sec=10) - repo.arm_bypass(now_ts=1_700_000_100, ttl_sec=3600) - - # the earlier, shorter-lived arm is gone; the new one governs. - assert repo.is_bypass_armed(now_ts=1_700_000_015) is True - assert repo.is_bypass_armed(now_ts=1_700_003_699) is True - assert repo.is_bypass_armed(now_ts=1_700_003_700) is False - - -def test_disarm_bypass_is_a_no_op_when_never_armed(repo): - repo.disarm_bypass() # must not raise - - assert repo.is_bypass_armed(now_ts=1_700_000_000) is False - - # -- rules ------------------------------------------------------------------- @@ -431,3 +384,31 @@ def test_held_products_lists_products_with_filled_live_orders(repo: Repository) ) assert repo.held_products() == ["BTC-USD", "ETH-USD"] + + +# -- profile (autonomy is a durable USER CHOICE, read live) --------------------- + + +def test_an_absent_profile_row_reads_as_NOT_autonomous(repo): + """Fails closed: a fresh or damaged database must never imply unattended trading.""" + assert repo.get_profile().autonomous is False + + +def test_set_autonomous_round_trips(repo): + repo.set_autonomous(True, now_ts=1000) + p = repo.get_profile() + assert p.autonomous is True + assert p.updated_ts == 1000 + + repo.set_autonomous(False, now_ts=2000) + p = repo.get_profile() + assert p.autonomous is False + assert p.updated_ts == 2000 + + +def test_set_autonomous_upserts_and_never_creates_a_second_row(repo): + """The table is deliberately single-row; two rows would make 'the' profile ambiguous.""" + for ts in range(1, 6): + repo.set_autonomous(ts % 2 == 0, now_ts=ts) + rows = repo._conn.execute("SELECT COUNT(*) AS n FROM profile").fetchone()["n"] + assert rows == 1 diff --git a/tests/data/test_trade_outcomes.py b/tests/data/test_trade_outcomes.py index dcf1ccb8..b1cb1558 100644 --- a/tests/data/test_trade_outcomes.py +++ b/tests/data/test_trade_outcomes.py @@ -35,11 +35,11 @@ def _outcome(**overrides: object) -> dict: return base -def test_schema_is_at_version_6() -> None: +def test_schema_is_at_version_7() -> None: conn = db.connect(":memory:") db.migrate(conn) version = conn.execute("SELECT version FROM schema_version").fetchone()["version"] - assert version == db.SCHEMA_VERSION == 6 + assert version == db.SCHEMA_VERSION == 7 def test_fresh_database_has_no_outcomes() -> None: From 0e68451f0f3ba38abad797d1d298d8ed7c3dee9a Mon Sep 17 00:00:00 2001 From: Elmehdi Aitbrahim Date: Tue, 21 Jul 2026 19:11:42 -0400 Subject: [PATCH 3/9] feat(security): delete the vault and the passphrase gate; autonomy is a profile choice The vault was already dead code: cb_client/cli load credentials from .env via config.load_secrets, and it was reachable only through migrate_from_env. Deleting it removes a competing credential path rather than changing how creds are loaded. Drops the now-unused cryptography dependency. The passphrase gate is replaced by one rule -- dangerous actions need a human at a terminal, nothing needs a stored secret. Once placing a money-spending order needs only a typed confirmation, a remembered secret to reset a high-water mark is ceremony without a matching threat model. The four halt-releasing commands (resume, resume-entries, record-flow, reset-hwm) now demand a typed 'yes' and FAIL CLOSED off a TTY. There is deliberately no env/flag override for that TTY check: any such seam would be settable from cron and would defeat the fail-closed. Autonomy moves from a config mode + armed token to a live-read profile choice. _effective_mode returns 'autonomous' only when the config is live AND the profile says so, is re-read every cycle (so 'autonomy off' binds on the NEXT order), lives inside run_once so an in-process caller cannot obtain what the CLI would refuse, and fails closed on an absent row. Rails are untouched throughout. Co-Authored-By: Claude Opus 4.8 (1M context) --- keel/agent.py | 98 +++---- keel/cli.py | 185 ++++++------ keel/execution/executor.py | 22 +- keel/execution/guards.py | 6 +- keel/security/__init__.py | 3 - keel/security/authz.py | 161 ----------- keel/security/secrets.py | 114 -------- pyproject.toml | 1 - tests/execution/test_executor.py | 59 ++-- tests/security/__init__.py | 0 tests/security/test_authz.py | 177 ------------ tests/security/test_secrets.py | 108 ------- tests/test_agent.py | 97 ++++--- tests/test_cli.py | 473 +++++++++++-------------------- uv.lock | 2 - 15 files changed, 403 insertions(+), 1103 deletions(-) delete mode 100644 keel/security/__init__.py delete mode 100644 keel/security/authz.py delete mode 100644 keel/security/secrets.py delete mode 100644 tests/security/__init__.py delete mode 100644 tests/security/test_authz.py delete mode 100644 tests/security/test_secrets.py diff --git a/keel/agent.py b/keel/agent.py index 261290b4..db818ba2 100644 --- a/keel/agent.py +++ b/keel/agent.py @@ -3,7 +3,7 @@ `run_once()` is a single cycle: poll fresh candles (`data.market_feed.poll_once`), reconstruct the `live` rules (`repo.get_rules("live")` -> real `Rule` instances, `RULE_REGISTRY` below), `strategy.engine.evaluate` them for ENTER `Signal`s, drive EXIT `Signal`s off currently-held -positions, and run every signal through `execution.executor.execute` (confirm|bypass, from +positions, and run every signal through `execution.executor.execute` (confirm|autonomous, from `config.auto_trade.mode`) -- respecting the kill-switch (`repo.get_state("kill_switch")`) throughout. `loop()` is the scheduled wrapper: call `run_once` every `interval_sec` until `stop_flag()` returns `True`. @@ -34,22 +34,17 @@ only ever deals in real `Rule` instances, never in raw JSON dicts. **Confirm mode places via a caller-supplied `confirm_fn`.** `run_once`/`loop` take an optional -to `(broker, repo, config, now_ts)` -- there is no `confirm_fn` slot for a human-in-the-loop -approval callback. So in `mode="confirm"`, every `executor.execute` call is made with -`confirm_fn=None`, which -- per `executor.execute`'s own contract -- fails closed: the order is -previewed and logged but never placed. That is the intended behavior, not an oversight: wiring -an actual interactive confirm prompt is the CLI's job (Task 9's `agent --loop --confirm`), which -can drive `run_once`/`loop` with a real `confirm_fn` some other way, or review the previewed-but- -unplaced orders out of band. `mode="bypass"` places without a prompt, still subject to every -guard rail (un-overridable, per `guards.check`). - -**Bypass requires an armed token (Issue #60).** `mode="bypass"` additionally requires -`repo.is_bypass_armed(now_ts)` -- an explicit, authenticated, time-limited arm set by -`Repository.arm_bypass` (wired to the CLI's passphrase-gated `keel arm-bypass` command). This -check is enforced by `_confirm_or_bypass` inside `run_once` itself, not only at the CLI, so a -caller invoking `run_once`/`loop` directly with `config.auto_trade.mode == "bypass"` cannot skip -it -- an unarmed or expired request fails safe by falling back to `"confirm"` (which places -nothing without a `confirm_fn`) rather than trading autonomously. +`confirm_fn`, which `executor.execute` calls with the broker preview. With `confirm_fn=None` -- +the default, and what any caller that does not supply one gets -- confirm mode **fails closed**: +the order is previewed and logged but never placed. The CLI supplies a real interactive prompt. + +**Autonomy is a profile choice, not a config mode.** `_effective_mode` returns `"autonomous"` +only when `config.auto_trade.mode == "confirm"` **and** `repo.get_profile().autonomous` is true. +The profile is read fresh every cycle and never cached, so `keel autonomy off` takes effect on +the next order rather than the next restart. The check lives inside `run_once`, not only at the +CLI, so an in-process caller cannot obtain autonomy the CLI would have refused; an absent or +unreadable profile row reads as not-autonomous. In every mode, `guards.check` runs FIRST and is +un-overridable -- autonomy changes who is asked, never what is allowed. """ from __future__ import annotations @@ -554,42 +549,32 @@ class LoopResult: enter_signals: list[Signal] = field(default_factory=list) enter_results: list[ExecutionResult] = field(default_factory=list) exit_results: list[ExecutionResult] = field(default_factory=list) - # Issue #60 (bypass-arm hardening): non-`None` iff `config.auto_trade.mode == "bypass"` was - # requested but `repo.is_bypass_armed(now_ts)` was `False` -- `mode` above then reports the - # *effective* mode actually used (always `"confirm"` in that case), and this field explains - # why the fallback happened. `None` whenever bypass wasn't requested, or was armed and ran. - bypass_refused_reason: str | None = None - - -def _confirm_or_bypass(config: Config, repo: Repository, now_ts: int) -> tuple[str, str | None]: - """The effective executor mode for this cycle, plus an optional bypass-refusal reason. - - `AutoTradeConfig.mode` defaults to `"paper"` (a Phase-1 placeholder, spec §21) which isn't - an `executor.execute` mode at all -- per the Global Constraints, confirm mode is *always* - the default, so anything other than an explicit `"bypass"`/`"confirm"` falls back to - `"confirm"` rather than raising or silently bypassing rails. - - **Issue #60 (bypass-arm hardening).** A request for `"bypass"` is only honored if - `repo.is_bypass_armed(now_ts)` is `True` -- an explicit, authenticated, time-limited arm - (`keel arm-bypass`, passphrase-gated, or any other caller that has called - `repo.arm_bypass`) recently granted it. This check lives here, inside `run_once`'s own call - path, specifically so it cannot be bypassed by a caller that invokes `agent.run_once` - in-process with `config.auto_trade.mode == "bypass"` and skips the CLI's passphrase gate - entirely -- the CLI gate (`cli._require_authz`) is defense-in-depth, this is the - un-overridable second layer. An unarmed/expired request fails safe: it falls back to - `"confirm"` (which, with no `confirm_fn`, places nothing -- see `executor.execute`'s own - contract) rather than raising or silently proceeding, and the second return value carries a - human-readable reason a caller can log or surface. + + +def _effective_mode(config: Config, repo: Repository) -> str: + """The executor mode for this cycle: `"autonomous"` or `"confirm"`. + + Two independent switches, deliberately not conflated into one enum: + + * `config.auto_trade.mode` says whether this is real money at all (`paper` never reaches an + executor mode -- it routes to the paper path upstream). + * `repo.get_profile().autonomous` says whether the user has opted out of being asked. + + `"autonomous"` is returned ONLY when the config is live (`confirm`) **and** the profile says + so. Anything else -- an unknown mode, an absent profile row, a damaged database -- yields + `"confirm"`, which with no `confirm_fn` places nothing at all. The failure direction is + always toward asking a human. + + **The profile is read here, fresh, on every cycle and never cached**, so `keel autonomy off` + takes effect on the NEXT order rather than the next restart. This mirrors rail 14's + allowance, which is re-read live for exactly the same reason. + + This lives inside `run_once`'s own call path, not only at the CLI, so a caller driving + `run_once` in-process cannot obtain autonomy the CLI would have refused. """ - mode = config.auto_trade.mode - if mode not in ("confirm", "bypass"): - return "confirm", None - if mode == "bypass" and not repo.is_bypass_armed(now_ts): - return "confirm", ( - "bypass mode requested but not armed (no active `keel arm-bypass` token, or it " - "expired) -- falling back to confirm mode; no order will be placed" - ) - return mode, None + if config.auto_trade.mode != "confirm": + return "confirm" + return "autonomous" if repo.get_profile().autonomous else "confirm" def run_once( @@ -641,15 +626,11 @@ def run_once( # for recording that the check happened, independent of whether it found anything new. repo.set_state("last_feed_ts", now_ts) - mode, bypass_refused_reason = _confirm_or_bypass(config, repo, now_ts) - # `mode: paper` is not an executor mode (see `_confirm_or_bypass`); it routes to the + mode = _effective_mode(config, repo) + # `mode: paper` is not an executor mode (see `_effective_mode`); it routes to the # PAPER path instead, which never touches the broker. Constructed once per cycle and # rehydrated from the orders table, so a per-cycle agent resumes its open positions. paper_trader = PaperTrader(repo) if config.auto_trade.mode == "paper" else None - if bypass_refused_reason is not None: - log_event( - logger, logging.WARNING, "agent.bypass_refused", reason=bypass_refused_reason - ) log_event(logger, logging.INFO, "agent.mode_resolved", mode=mode) max_age_sec = config.auto_trade.interval_sec * FEED_STALENESS_CYCLES finest = _finest_granularity(granularities) @@ -791,7 +772,6 @@ def run_once( enter_signals=enter_signals, enter_results=enter_results, exit_results=exit_results, - bypass_refused_reason=bypass_refused_reason, ) finally: unbind_cycle(cycle_token) diff --git a/keel/cli.py b/keel/cli.py index 03c2dfca..2d226cf1 100644 --- a/keel/cli.py +++ b/keel/cli.py @@ -84,7 +84,6 @@ from keel.research import deflate as deflate_mod from keel.research import ledger as trials_ledger from keel.research import matrix as matrix_mod -from keel.security import authz from keel.sim import artifact as artifact_mod from keel.sim import benchmark as benchmark_mod from keel.sim import metrics as metrics_mod @@ -104,11 +103,8 @@ DEFAULT_DB_PATH = "keel.db" DEFAULT_CONFIG_PATH = "config.yaml" -DEFAULT_AUTHZ_PATH = "authz.json" # The dangerous-action names this CLI's gated commands map to (see module docstring). -_ARM_BYPASS = "arm_bypass" -_DISABLE_KILLSWITCH = "disable_killswitch" # `rules demote` steps a rule back one lifecycle stage; `disabled` is terminal (see # `strategy.promotion`'s own `_PROMOTE_NEXT` docstring) and so is not a demote target. @@ -139,27 +135,36 @@ def wrapper(*args: Any, **kwargs: Any) -> Any: # -- passphrase resolution (no interactive hangs under CliRunner) ----------------------------- -def _resolve_passphrase(passphrase: str | None) -> str: - """`passphrase` if given; else an interactive prompt on a real TTY; else `""` (fails closed). +def _is_interactive() -> bool: + """True when a human is at a terminal. - A non-interactive invocation (tests, scripts, CI) never blocks on stdin -- it simply gets an - empty passphrase, which `authz.verify`/`authz.require` correctly reject. + 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. """ - if passphrase: - return passphrase - if sys.stdin is not None and sys.stdin.isatty(): - return click.prompt("Passphrase", hide_input=True) - return "" + return sys.stdin is not None and sys.stdin.isatty() -def _require_authz(ctx: click.Context, action: str, passphrase: str | None) -> None: - """Enforce the authz gate for `action`, aborting the command (exit 1) on denial.""" - resolved = _resolve_passphrase(passphrase) - try: - authz.require(action, resolved, path=ctx.obj["authz_path"]) - except authz.AuthzError as exc: - click.echo(f"Error: {exc}", err=True) - ctx.exit(1) +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 --------------------------------------------------------- @@ -247,12 +252,6 @@ def _print_version(ctx: click.Context, param: object, value: bool) -> None: show_default=True, help="config.yaml path.", ) -@click.option( - "--authz-path", - default=DEFAULT_AUTHZ_PATH, - show_default=True, - help="Passphrase-gate state file (see `keel.security.authz`).", -) @click.option( "--verbose", "-v", @@ -263,13 +262,12 @@ def _print_version(ctx: click.Context, param: object, value: bool) -> None: ) @click.pass_context def cli( - ctx: click.Context, db_path: str, config_path: str, authz_path: str, verbose: bool + ctx: click.Context, db_path: str, config_path: str, verbose: bool ) -> None: """keel: an offline-first, halal, guard-railed Coinbase auto-trading agent.""" ctx.ensure_object(dict) ctx.obj["db_path"] = db_path ctx.obj["config_path"] = config_path - ctx.obj["authz_path"] = authz_path ctx.obj["verbose"] = verbose @@ -1191,22 +1189,12 @@ def _print_loop_result(result: agent.LoopResult) -> None: f"products={result.products} stale={result.stale_products} " f"signals={len(result.enter_signals)} entered={entered} exited={exited}" ) - if result.bypass_refused_reason is not None: - # Issue #60: `run_once` itself refused bypass (unarmed/expired token) and fell back to - # confirm mode -- surface *why*, not just the silently-different `mode=confirm` above. - click.echo(f"[{result.ts}] bypass refused: {result.bypass_refused_reason}") @cli.command() @click.option( "--loop", is_flag=True, default=False, help="Run the scheduled loop, not one cycle." ) -@click.option( - "--confirm", "mode", flag_value="confirm", default=True, help="Confirm mode (default, safe)." -) -@click.option( - "--bypass", "mode", flag_value="bypass", help="Bypass mode (dangerous; requires --passphrase)." -) @click.option( "--interval", "interval_sec", @@ -1220,28 +1208,26 @@ def _print_loop_result(result: agent.LoopResult) -> None: default=None, help="Stop --loop after N cycles (default: run until interrupted).", ) -@click.option("--passphrase", default=None, help="Required to arm --bypass.") @click.pass_context @with_disclaimer def agent_cmd( ctx: click.Context, loop: bool, - mode: str, interval_sec: float | None, max_cycles: int | None, - passphrase: str | None, ) -> None: - """Run the agent loop (confirm mode by default; every order is still hard-rail-guarded).""" - if mode == "bypass": - _require_authz(ctx, _ARM_BYPASS, passphrase) + """Run the agent loop. Every order is hard-rail-guarded, in every mode. + Whether orders are placed at all comes from `config.auto_trade.mode` (`paper` simulates, + `confirm` is live). Whether you are ASKED comes from your profile: with autonomy off (the + default) each order needs your approval at the terminal; with `keel autonomy on` it does not. + """ config = _load_cfg(ctx) - config = replace(config, auto_trade=replace(config.auto_trade, mode=mode)) repo = _open_repo(ctx) broker = _build_broker(config) if not loop: - confirm_fn = _interactive_confirm if mode == "confirm" else None + confirm_fn = _interactive_confirm _print_loop_result( agent.run_once(broker, repo, config, now_ts=int(time.time()), confirm_fn=confirm_fn) ) @@ -1255,46 +1241,65 @@ def stop_flag(_count: list[int] = [0]) -> bool: # noqa: B006 - intentional muta _count[0] += 1 return False - confirm_fn = _interactive_confirm if mode == "confirm" else None + confirm_fn = _interactive_confirm for result in agent.loop(broker, repo, config, interval, stop_flag, confirm_fn=confirm_fn): _print_loop_result(result) -# -- arm-bypass / disarm-bypass (Issue #60, bypass-arm hardening) --------------------------- +# -- autonomy (the user's own choice, stored in their profile) ------------------------------ + + +@cli.group("autonomy") +def autonomy_group() -> None: + """Whether the agent places orders without asking you first.""" -@cli.command("arm-bypass") -@click.option("--passphrase", default=None, help="Required to arm bypass mode.") +@autonomy_group.command("show") +@click.pass_context +def autonomy_show(ctx: click.Context) -> None: + """Print the current autonomy setting.""" + profile = _open_repo(ctx).get_profile() + state = ( + "ON -- orders are placed WITHOUT asking" + if profile.autonomous + else "off -- every order asks first" + ) + click.echo(f"autonomy: {state}") + if profile.updated_ts: + click.echo(f" last changed: {profile.updated_ts}") + + +@autonomy_group.command("on") @click.pass_context @with_disclaimer -def arm_bypass_cmd(ctx: click.Context, passphrase: str | None) -> None: - """Arm autonomous bypass mode for a limited window (dangerous: requires the passphrase). - - This is the second, in-process enforcement layer `agent.run_once` itself checks - (`repo.is_bypass_armed`) before ever honoring `config.auto_trade.mode == "bypass"` -- the - existing `agent --bypass --passphrase` gate alone is not enough, since that only guards the - CLI entry point, not a caller that invokes `agent.run_once`/`agent.loop` directly. The - window lasts `config.auto_trade.bypass_arm_ttl_sec` seconds (default 3600 = 1h) from now; - call this again to reset it, or `keel disarm-bypass` to clear it early. +def autonomy_on(ctx: click.Context) -> 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. """ - _require_authz(ctx, _ARM_BYPASS, passphrase) config = _load_cfg(ctx) repo = _open_repo(ctx) - now_ts = int(time.time()) - ttl_sec = config.auto_trade.bypass_arm_ttl_sec - repo.arm_bypass(now_ts, ttl_sec) - click.echo(f"bypass ARMED: expires at {now_ts + ttl_sec} (ttl={ttl_sec}s)") + _require_interactive_confirmation( + "turn autonomy ON", + f"Orders will be placed with NO further prompt " + f"(mode={config.auto_trade.mode}, allowlist={config.allowlist}).", + ) + repo.set_autonomous(True, int(time.time())) + click.echo("autonomy ON. Run `keel autonomy off` to require confirmation again.") -@cli.command("disarm-bypass") +@autonomy_group.command("off") @click.pass_context -@with_disclaimer -def disarm_bypass_cmd(ctx: click.Context) -> None: - """Clear the bypass-arm token immediately. Always allowed (safe action; only reduces - capability -- fail-safe direction, unlike arming, needs no passphrase).""" - repo = _open_repo(ctx) - repo.disarm_bypass() - click.echo("bypass DISARMED.") +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.") # -- rules ------------------------------------------------------------------------------ @@ -2251,22 +2256,23 @@ def kill(ctx: click.Context) -> None: @cli.command() -@click.option("--passphrase", default=None, help="Required to disengage the kill-switch.") @click.pass_context @with_disclaimer -def resume(ctx: click.Context, passphrase: str | None) -> None: - """Disengage the kill-switch (dangerous: requires the authz passphrase).""" - _require_authz(ctx, _DISABLE_KILLSWITCH, passphrase) +def resume(ctx: click.Context) -> None: + """Disengage the kill-switch (dangerous: asks for confirmation).""" + _require_interactive_confirmation( + "disengage the kill-switch", + "Trading resumes immediately: the agent may place orders on its next cycle.", + ) repo = _open_repo(ctx) repo.set_state("kill_switch", False) click.echo("kill-switch disengaged: trading resumed.") @cli.command(name="resume-entries") -@click.option("--passphrase", default=None, help="Required to clear the breaker.") @click.pass_context @with_disclaimer -def resume_entries(ctx: click.Context, passphrase: str | None) -> None: +def resume_entries(ctx: click.Context) -> None: """Clear an armed consecutive-loss halt (rail 16), re-permitting new entries. This is the ONLY way to release the halt early: rail 16 reads `streak_halt_until` and never @@ -2277,7 +2283,10 @@ def resume_entries(ctx: click.Context, passphrase: str | None) -> None: re-arm the breaker on the very next loss, which is not what an operator clearing a halt means. Exits, sells and DCA are never affected by rail 16 and are unaffected here. """ - _require_authz(ctx, _DISABLE_KILLSWITCH, passphrase) + _require_interactive_confirmation( + "clear the consecutive-loss halt (rail 16)", + "New entries are re-permitted; the loss counter is reset with it.", + ) repo = _open_repo(ctx) repo.set_state("streak_halt_until", 0) repo.set_state("consecutive_losses", 0) @@ -2290,10 +2299,9 @@ def resume_entries(ctx: click.Context, passphrase: str | None) -> None: required=True, help="Signed flow in quote currency: positive for a deposit, negative for a withdrawal.", ) -@click.option("--passphrase", default=None, help="Required to rebase the high-water mark.") @click.pass_context @with_disclaimer -def record_flow(ctx: click.Context, amount: str, passphrase: str | None) -> None: +def record_flow(ctx: click.Context, amount: str) -> None: """Declare an external deposit or withdrawal so rail 11 does not mistake it for P&L. Equity is `cash + positions`, so money moving in or out shifts it -- but neither is a @@ -2312,7 +2320,10 @@ def record_flow(ctx: click.Context, amount: str, passphrase: str | None) -> None will never infer a flow on its own: guessing a withdrawal would lower the HWM and silently mask a real trading drawdown, which is the one direction a circuit breaker must not fail in. """ - _require_authz(ctx, _DISABLE_KILLSWITCH, passphrase) + _require_interactive_confirmation( + "rebase rail 11's high-water mark", + "A wrong amount here silently mis-states drawdown from now on.", + ) try: parsed = Decimal(amount) except InvalidOperation: @@ -2337,10 +2348,9 @@ def record_flow(ctx: click.Context, amount: str, passphrase: str | None) -> None @cli.command(name="reset-hwm") -@click.option("--passphrase", default=None, help="Required to reset the high-water mark.") @click.pass_context @with_disclaimer -def reset_hwm(ctx: click.Context, passphrase: str | None) -> None: +def reset_hwm(ctx: click.Context) -> None: """Reset rail 11's equity high-water mark, clearing a stuck drawdown halt. The HWM is MONOTONIC by design -- it never falls -- so any equity reading that was wrong or @@ -2352,7 +2362,10 @@ def reset_hwm(ctx: click.Context, passphrase: str | None) -> None: equity, which is the same path a fresh install takes. `drawdown_total_pct` is zeroed so the rail is not left vetoing on a stale scalar in the window before that next cycle runs. """ - _require_authz(ctx, _DISABLE_KILLSWITCH, passphrase) + _require_interactive_confirmation( + "reset rail 11's high-water mark", + "Any real, unrecovered drawdown stops being visible to the rail.", + ) repo = _open_repo(ctx) repo.set_state("equity_high_water_mark", None) repo.set_state("drawdown_total_pct", Decimal("0")) diff --git a/keel/execution/executor.py b/keel/execution/executor.py index 0d9f8966..dccc3fa5 100644 --- a/keel/execution/executor.py +++ b/keel/execution/executor.py @@ -2,7 +2,7 @@ `execute()` is the only path from a strategy `Signal` to a real order: it sizes the candidate (`execution.sizing`), runs the twelve un-overridable §14 hard rails (`execution.guards.check`) -**before** anything reaches the broker, previews the order, honors the confirm/bypass mode gate, +**before** anything reaches the broker, previews the order, honors the confirm/autonomous mode gate, places it, and writes a full audit trail to the `orders` table both before and after the broker call (so a crash mid-placement, or a broker-side rejection, still leaves a record). No path in this module calls `broker.place_order` (or even `broker.preview_order`) without `guards.check` @@ -109,15 +109,15 @@ def execute( broker: Any, repo: Repository, config: Config, - mode: Literal["confirm", "bypass"], + mode: Literal["confirm", "autonomous"], confirm_fn: ConfirmFn | None = None, now_ts: int | None = None, ) -> ExecutionResult: """Turn `signal` into a guarded order: size -> guards (veto on any violation) -> preview -> - confirm|bypass -> place -> log (before and after). + confirm|autonomous -> place -> log (before and after). `mode="confirm"` requires `confirm_fn(preview) -> bool`; a missing or rejecting `confirm_fn` - means the order is not placed (fails closed, never silently proceeds). `mode="bypass"` + means the order is not placed (fails closed, never silently proceeds). `mode="autonomous"` places without a prompt but is *not* exempt from `guards.check` -- rails run before every order in every mode, un-overridable, per the main spec §14. """ @@ -364,7 +364,7 @@ def _held_position(repo: Repository, product_id: str) -> tuple[Decimal, Decimal] return (net_qty if net_qty > 0 else Decimal("0")), avg_cost -# -- shared guard -> preview -> confirm/bypass -> place -> log pipeline ------------------------ +# -- shared guard -> preview -> confirm/autonomous -> place -> log pipeline ------------------------ def _run_order( @@ -433,8 +433,8 @@ def _run_order( preview=preview, reason="rejected at confirm gate", ) - elif mode != "bypass": - raise ValueError(f"execute: unknown mode {mode!r} -- must be 'confirm' or 'bypass'") + elif mode != "autonomous": + raise ValueError(f"execute: unknown mode {mode!r} -- must be 'confirm' or 'autonomous'") order_id = repo.insert_order(_order_row(intent, mode, now_ts)) @@ -717,7 +717,7 @@ def place_bracket( broker, repo, config, - "bypass", + "autonomous", None, now_ts, order_configuration=_bracket_order_configuration(qty, target, stop), @@ -762,7 +762,7 @@ def scale_out( ) -> ExecutionResult: """Partially close `qty` of an open position at `exit_price` (a rule-driven profit-take, e.g. "sell half at the first target") -- runs the same guard+preview+place+log pipeline as - `execute()` for a plain SELL leg, system-initiated so it proceeds in bypass mode (still + `execute()` for a plain SELL leg, system-initiated so it proceeds in autonomous mode (still subject to every guard rail, still fully logged). """ intent = OrderIntent( @@ -784,7 +784,7 @@ def scale_out( exit_price=exit_price, rule=rule_name, ) - return _run_order(intent, broker, repo, config, "bypass", None, now_ts) + return _run_order(intent, broker, repo, config, "autonomous", None, now_ts) # -- stop management: break-even roll + ATR trailing ------------------------------------------- @@ -858,7 +858,7 @@ def _roll_stop( broker, repo, config, - "bypass", + "autonomous", None, now_ts, order_configuration=_bracket_order_configuration(qty, target, new_stop), diff --git a/keel/execution/guards.py b/keel/execution/guards.py index bf613bb4..2db981fd 100644 --- a/keel/execution/guards.py +++ b/keel/execution/guards.py @@ -3,7 +3,8 @@ `check()` runs the twelve safety rails from the main spec's §14, plus three later, equally un-overridable safety-critical rails: 13/14 added by Issue #59 (USDC-funding + monthly-allowance), and 16, the consecutive-loss circuit breaker (Task 4), before any order is placed, in every -`auto_trade` mode (confirm *and* bypass) and for both rule-trading and DCA order classes. It never +`auto_trade` mode (confirm *and* autonomous) and for both rule-trading and DCA order +classes. It never short-circuits: every violated rail is collected and reported so an operator (or the executor, Task 4) sees the full picture, not just the first trip-wire. @@ -242,7 +243,8 @@ def check( ) -> GuardResult: """Run all fifteen §14 (+ Issue #59, Task 4) hard rails against `intent`. Never short-circuits. - Called before every order in every `auto_trade` mode (confirm *and* bypass) — un-overridable. + Called before every order in every `auto_trade` mode (confirm *and* autonomous) -- + un-overridable. `offline=True` (paper trading only) skips `LIVE_STATE_RAILS` — the two rails whose inputs describe the real account, which a paper rehearsal has no access to. **Every other rail still diff --git a/keel/security/__init__.py b/keel/security/__init__.py deleted file mode 100644 index 2075fe8b..00000000 --- a/keel/security/__init__.py +++ /dev/null @@ -1,3 +0,0 @@ -"""Operational security: portable encrypted secrets vault (`secrets.py`) and the dangerous-action -passphrase gate (`authz.py`, added in a later task). See main spec §14 Part A. -""" diff --git a/keel/security/authz.py b/keel/security/authz.py deleted file mode 100644 index 70c34534..00000000 --- a/keel/security/authz.py +++ /dev/null @@ -1,161 +0,0 @@ -"""Dangerous-action authorization gate (main spec §14). - -A local passphrase — stored as a **stdlib `hashlib.scrypt` hash + random salt, never -plaintext** — gates only the small set of *dangerous* actions: arming bypass/autonomous -mode, raising caps above config maxima, disabling the kill-switch/resume, and unlocking -the secrets vault. Read-only commands and confirm-mode trades need no passphrase at all. - -This is local *authorization*, not a login: on a single-user machine the real security -boundary is the OS account + full-disk encryption. The gate is defense-in-depth — it -forces intentionality and blocks casual/accidental/shoulder-surf misuse of the autonomy -capability. It does not stop an attacker who already holds the OS account. - -Failed attempts are tracked and rate-limited with an exponential backoff lockout, -persisted alongside the hash so restarts don't reset the counter. -""" - -from __future__ import annotations - -import hashlib -import hmac -import json -import os -import time -from pathlib import Path - -DANGEROUS_ACTIONS = frozenset( - {"arm_bypass", "raise_caps", "disable_killswitch", "unlock_vault"} -) - -# scrypt cost parameters (interactive-login-grade; OWASP minimum: N=2**14, r=8, p=1). -_SCRYPT_N = 2**14 -_SCRYPT_R = 8 -_SCRYPT_P = 1 -_SCRYPT_DKLEN = 32 -_SALT_BYTES = 16 - -# Rate limiting: lock out after MAX_ATTEMPTS consecutive failures, with the lockout -# window growing exponentially for each additional failure beyond the threshold. -MAX_ATTEMPTS = 5 -_LOCKOUT_BASE_SECONDS = 30.0 - -_DEFAULT_PATH = "authz.json" - - -class AuthzError(Exception): - """Raised by `require()` when an action is dangerous and authorization fails.""" - - -def _scrypt_hash( - passphrase: str, - salt: bytes, - n: int = _SCRYPT_N, - r: int = _SCRYPT_R, - p: int = _SCRYPT_P, - dklen: int = _SCRYPT_DKLEN, -) -> bytes: - return hashlib.scrypt( - passphrase.encode("utf-8"), - salt=salt, - n=n, - r=r, - p=p, - dklen=dklen, - ) - - -def _load_state(path: Path) -> dict | None: - if not path.exists(): - return None - with path.open() as f: - return json.load(f) - - -def _save_state(path: Path, state: dict) -> None: - path.parent.mkdir(parents=True, exist_ok=True) - path.write_text(json.dumps(state)) - if os.name != "nt": - os.chmod(path, 0o600) - - -def set_passphrase(passphrase: str, path: str | Path = _DEFAULT_PATH) -> None: - """Hash `passphrase` with a fresh random salt and persist it at `path`. - - Overwrites any existing state (including the rate-limit counters) — setting a new - passphrase starts a clean slate. The passphrase itself is never written to disk. - """ - path = Path(path) - salt = os.urandom(_SALT_BYTES) - digest = _scrypt_hash(passphrase, salt) - state = { - "salt": salt.hex(), - "hash": digest.hex(), - "n": _SCRYPT_N, - "r": _SCRYPT_R, - "p": _SCRYPT_P, - "dklen": _SCRYPT_DKLEN, - "failed_attempts": 0, - "locked_until": 0.0, - } - _save_state(path, state) - - -def verify(passphrase: str, path: str | Path = _DEFAULT_PATH, now: float | None = None) -> bool: - """Check `passphrase` against the hash stored at `path`. - - Returns `False` (never raises) when: no passphrase has been set yet, the passphrase - is wrong, or the gate is currently locked out from too many recent failures. `now` - defaults to the wall clock; callers (and tests) may pass an explicit epoch timestamp. - """ - path = Path(path) - now = time.time() if now is None else now - - state = _load_state(path) - if state is None: - return False - - if now < state.get("locked_until", 0.0): - return False # rate limited — still inside the lockout window - - computed = _scrypt_hash( - passphrase, - bytes.fromhex(state["salt"]), - n=state["n"], - r=state["r"], - p=state["p"], - dklen=state["dklen"], - ) - stored = bytes.fromhex(state["hash"]) - ok = hmac.compare_digest(computed, stored) - - if ok: - state["failed_attempts"] = 0 - state["locked_until"] = 0.0 - _save_state(path, state) - return True - - state["failed_attempts"] = state.get("failed_attempts", 0) + 1 - if state["failed_attempts"] >= MAX_ATTEMPTS: - excess = state["failed_attempts"] - MAX_ATTEMPTS - lockout_seconds = _LOCKOUT_BASE_SECONDS * (2**excess) - state["locked_until"] = now + lockout_seconds - _save_state(path, state) - return False - - -def require( - action: str, - passphrase: str, - path: str | Path = _DEFAULT_PATH, - now: float | None = None, -) -> None: - """Enforce the authorization gate for `action`. - - No-ops for anything outside `DANGEROUS_ACTIONS` (read-only/confirm-mode actions need - no passphrase). For a dangerous action, raises `AuthzError` unless `passphrase` - verifies — including while rate-limit locked out, even with the correct passphrase. - """ - if action not in DANGEROUS_ACTIONS: - return - if not verify(passphrase, path=path, now=now): - raise AuthzError(f"authorization denied for dangerous action {action!r}") diff --git a/keel/security/secrets.py b/keel/security/secrets.py deleted file mode 100644 index aae4da33..00000000 --- a/keel/security/secrets.py +++ /dev/null @@ -1,114 +0,0 @@ -"""Portable AES-GCM encrypted secrets vault (main spec §14 Part A). - -The CDP API key/secret (and any future remote-control token) live in a single file, -`secrets.enc`, unlocked by a master passphrase: passphrase -> **scrypt KDF** -> symmetric key -> -**AES-GCM** encrypt/decrypt of a JSON secrets blob. Deliberately *not* the OS keychain — this file -is copyable between machines, which is the portability the design calls for. - -On-disk layout (all bytes, no plaintext): - MAGIC (7 bytes) | salt (16 bytes) | nonce (12 bytes) | AES-GCM ciphertext+tag - -`MAGIC` is also passed as AES-GCM associated data, so tampering with it (or any other byte in the -file) fails authentication rather than silently decrypting garbage. - -Secrets are never logged: no function in this module writes secret values to a log, exception -message, or `repr()`. -""" - -from __future__ import annotations - -import json -import os -from pathlib import Path - -from cryptography.exceptions import InvalidTag -from cryptography.hazmat.primitives.ciphers.aead import AESGCM -from cryptography.hazmat.primitives.kdf.scrypt import Scrypt - -_MAGIC = b"HCBVLT1" -_SALT_LEN = 16 -_NONCE_LEN = 12 -_KEY_LEN = 32 - -# scrypt cost parameters for an interactive unlock (local CLI, not a server). ~16MB of memory. -_SCRYPT_N = 2**14 -_SCRYPT_R = 8 -_SCRYPT_P = 1 - -DEFAULT_VAULT_PATH = "secrets.enc" - - -class VaultError(Exception): - """Raised when a vault cannot be sealed/unlocked: wrong passphrase, tampering, or a missing - or corrupt vault file. Never includes secret values in its message. - """ - - -def _derive_key(passphrase: str, salt: bytes) -> bytes: - kdf = Scrypt(salt=salt, length=_KEY_LEN, n=_SCRYPT_N, r=_SCRYPT_R, p=_SCRYPT_P) - return kdf.derive(passphrase.encode("utf-8")) - - -def save_vault(secrets: dict, passphrase: str, path: str | Path = DEFAULT_VAULT_PATH) -> None: - """Encrypt `secrets` (a JSON-serializable dict) with a passphrase-derived key and write the - result to `path`. A fresh random salt + nonce are generated on every call, so encrypting the - same secrets twice yields different ciphertext. The file is `chmod 600`'d. - """ - path = Path(path) - salt = os.urandom(_SALT_LEN) - nonce = os.urandom(_NONCE_LEN) - key = _derive_key(passphrase, salt) - - plaintext = json.dumps(secrets).encode("utf-8") - ciphertext = AESGCM(key).encrypt(nonce, plaintext, associated_data=_MAGIC) - - path.write_bytes(_MAGIC + salt + nonce + ciphertext) - path.chmod(0o600) - - -def load_vault(passphrase: str, path: str | Path = DEFAULT_VAULT_PATH) -> dict: - """Decrypt the vault at `path` with `passphrase` and return the secrets dict. - - Raises `VaultError` for a missing file, a wrong passphrase, or a tampered/corrupt file. - """ - path = Path(path) - if not path.exists(): - raise VaultError(f"vault not found at {path}") - - blob = path.read_bytes() - if not blob.startswith(_MAGIC): - raise VaultError("not a valid secrets vault file") - - rest = blob[len(_MAGIC) :] - if len(rest) < _SALT_LEN + _NONCE_LEN: - raise VaultError("vault file is truncated or corrupt") - - salt = rest[:_SALT_LEN] - nonce = rest[_SALT_LEN : _SALT_LEN + _NONCE_LEN] - ciphertext = rest[_SALT_LEN + _NONCE_LEN :] - - key = _derive_key(passphrase, salt) - try: - plaintext = AESGCM(key).decrypt(nonce, ciphertext, associated_data=_MAGIC) - except InvalidTag as exc: - raise VaultError("wrong passphrase or tampered vault file") from exc - - return json.loads(plaintext) - - -def migrate_from_env( - env_path: str | Path = ".env", - *, - passphrase: str, - path: str | Path = DEFAULT_VAULT_PATH, -) -> None: - """Read CDP secrets out of a git-ignored `.env` file and seal them into the vault at `path`. - - Delegates to `config.load_secrets`, so it inherits the same absent/empty-file handling - (returns `{}` rather than raising, so a fresh vault with no secrets configured is still - valid). Does not delete or modify `env_path` — the caller decides when it's safe to remove it. - """ - from keel.config import load_secrets - - secrets = load_secrets(env_path) - save_vault(secrets, passphrase, path=path) diff --git a/pyproject.toml b/pyproject.toml index d873c90d..78328467 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -9,7 +9,6 @@ authors = [ requires-python = ">=3.12" dependencies = [ "click>=8.4.2", - "cryptography>=49.0.0", # pyyaml/python-dotenv are deliberately NOT listed here: nothing under `keel/` imports # them any more (config parsing and secret loading both moved into keel-core), so they # arrive transitively via the `keel-core` dependency below. Re-add them here only if diff --git a/tests/execution/test_executor.py b/tests/execution/test_executor.py index 2247e207..ffdd7592 100644 --- a/tests/execution/test_executor.py +++ b/tests/execution/test_executor.py @@ -341,7 +341,7 @@ def test_rail_violating_signal_is_vetoed_before_preview_or_place(repo): broker = NoNetworkBroker() signal = _enter_signal(product_id="DOGE-USD", setup=_setup(product_id="DOGE-USD")) - result = execute(signal, broker, repo, _config(), mode="bypass", now_ts=NOW_TS) + result = execute(signal, broker, repo, _config(), mode="autonomous", now_ts=NOW_TS) assert result.placed is False assert result.order_id is None @@ -355,7 +355,7 @@ def test_kill_switch_vetoes_even_in_bypass_mode(repo): broker = NoNetworkBroker() signal = _enter_signal() - result = execute(signal, broker, repo, _config(), mode="bypass", now_ts=NOW_TS) + result = execute(signal, broker, repo, _config(), mode="autonomous", now_ts=NOW_TS) assert result.placed is False assert any(v.startswith("kill_switch") for v in result.vetoed_by) @@ -385,7 +385,7 @@ def test_execute_fetches_the_available_quote_balance_for_a_buy_and_places(repo): broker = FakeBroker(usdc_balance=Decimal("100000")) signal = _enter_signal() - result = execute(signal, broker, repo, _config(), mode="bypass", now_ts=NOW_TS) + result = execute(signal, broker, repo, _config(), mode="autonomous", now_ts=NOW_TS) assert result.placed is True assert broker.get_accounts_calls == 1 @@ -395,7 +395,7 @@ def test_broker_balance_fetch_error_vetoes_the_buy_before_preview_or_place(repo) broker = _BrokerAccountsError() signal = _enter_signal() - result = execute(signal, broker, repo, _config(), mode="bypass", now_ts=NOW_TS) + result = execute(signal, broker, repo, _config(), mode="autonomous", now_ts=NOW_TS) assert result.placed is False assert result.order_id is None @@ -408,7 +408,7 @@ def test_insufficient_usdc_balance_vetoes_the_buy_before_preview_or_place(repo): broker = FakeBroker(usdc_balance=Decimal("10")) # entry 50000, qty ~1 -> notional ~50000 signal = _enter_signal() - result = execute(signal, broker, repo, _config(), mode="bypass", now_ts=NOW_TS) + result = execute(signal, broker, repo, _config(), mode="autonomous", now_ts=NOW_TS) assert result.placed is False assert result.preview is None @@ -432,7 +432,7 @@ def test_exit_signal_never_fetches_a_balance(repo): ts=NOW_TS, ) - result = execute(signal, broker, repo, _config(), mode="bypass", now_ts=NOW_TS) + result = execute(signal, broker, repo, _config(), mode="autonomous", now_ts=NOW_TS) assert result.placed is True assert broker.get_accounts_calls == 0 @@ -445,7 +445,7 @@ def test_bypass_mode_compliant_signal_places_without_confirm_fn(repo): broker = FakeBroker() signal = _enter_signal() - result = execute(signal, broker, repo, _config(), mode="bypass", now_ts=NOW_TS) + result = execute(signal, broker, repo, _config(), mode="autonomous", now_ts=NOW_TS) assert result.placed is True assert result.order_id is not None @@ -458,7 +458,7 @@ def test_bypass_mode_ignores_confirm_fn_if_provided(repo): signal = _enter_signal() result = execute( - signal, broker, repo, _config(), mode="bypass", confirm_fn=_reject, now_ts=NOW_TS + signal, broker, repo, _config(), mode="autonomous", confirm_fn=_reject, now_ts=NOW_TS ) # bypass mode never consults confirm_fn -- a reject-everything fn must not block it. @@ -473,7 +473,8 @@ def test_unknown_mode_raises_value_error(repo): signal = _enter_signal() with pytest.raises(ValueError, match="mode"): - execute(signal, broker, repo, _config(), mode="yolo", now_ts=NOW_TS) # type: ignore[arg-type] + # type: ignore[arg-type] + execute(signal, broker, repo, _config(), mode="yolo", now_ts=NOW_TS) # -- broker place failure -------------------------------------------------------------------- @@ -483,7 +484,7 @@ def test_broker_place_failure_is_logged_but_not_placed(repo): broker = FakeBroker(place_success=False) signal = _enter_signal() - result = execute(signal, broker, repo, _config(), mode="bypass", now_ts=NOW_TS) + result = execute(signal, broker, repo, _config(), mode="autonomous", now_ts=NOW_TS) assert result.placed is False assert result.order_id is not None # still logged (audit trail even on rejection) @@ -499,7 +500,7 @@ def test_monthly_allowance_vetoes_a_buy_over_the_live_subscription_cap(repo): broker = FakeBroker() signal = _enter_signal() - result = execute(signal, broker, repo, _config(), mode="bypass", now_ts=NOW_TS) + result = execute(signal, broker, repo, _config(), mode="autonomous", now_ts=NOW_TS) assert result.placed is False assert any(v.startswith("monthly_subscription_allowance") for v in result.vetoed_by) @@ -514,11 +515,11 @@ def test_monthly_allowance_updated_subscription_takes_effect_on_the_next_order(r broker = FakeBroker() signal = _enter_signal() - first = execute(signal, broker, repo, _config(), mode="bypass", now_ts=NOW_TS) + first = execute(signal, broker, repo, _config(), mode="autonomous", now_ts=NOW_TS) assert first.placed is False _attest(repo, free_volume_usd=Decimal("10000000")) - second = execute(signal, broker, repo, _config(), mode="bypass", now_ts=NOW_TS) + second = execute(signal, broker, repo, _config(), mode="autonomous", now_ts=NOW_TS) assert second.placed is True @@ -529,7 +530,7 @@ def test_dca_signal_sizes_via_dca_size_and_places(repo): broker = FakeBroker() signal = _dca_signal() - result = execute(signal, broker, repo, _config(), mode="bypass", now_ts=NOW_TS) + result = execute(signal, broker, repo, _config(), mode="autonomous", now_ts=NOW_TS) assert result.placed is True order = repo.get_order(result.order_id) @@ -549,7 +550,7 @@ def test_dca_signal_exempt_from_averaging_into_losers_but_bound_by_allowlist(rep ), ) - result = execute(signal, broker, repo, _config(), mode="bypass", now_ts=NOW_TS) + result = execute(signal, broker, repo, _config(), mode="autonomous", now_ts=NOW_TS) assert result.placed is False assert any(v.startswith("halal_allowlist") for v in result.vetoed_by) @@ -572,7 +573,7 @@ def _seed_open_position(repo: Repository, product_id: str, qty: Decimal, price: expected_fill=price, actual_fill=price, raw_response=None, - confirmation="bypass", + confirmation="autonomous", rule_id=None, created_at=NOW_TS - 1000, updated_at=NOW_TS - 1000, @@ -594,7 +595,7 @@ def test_exit_signal_sells_the_held_position(repo): ts=NOW_TS, ) - result = execute(signal, broker, repo, _config(), mode="bypass", now_ts=NOW_TS) + result = execute(signal, broker, repo, _config(), mode="autonomous", now_ts=NOW_TS) assert result.placed is True order = repo.get_order(result.order_id) @@ -615,7 +616,7 @@ def test_exit_signal_with_no_open_position_is_not_placed(repo): ts=NOW_TS, ) - result = execute(signal, broker, repo, _config(), mode="bypass", now_ts=NOW_TS) + result = execute(signal, broker, repo, _config(), mode="autonomous", now_ts=NOW_TS) assert result.placed is False assert result.order_id is None @@ -628,7 +629,7 @@ def test_execute_attaches_oco_bracket_after_a_stop_target_entry_fills(repo): broker = FakeBroker() signal = _enter_signal() - result = execute(signal, broker, repo, _config(), mode="bypass", now_ts=NOW_TS) + result = execute(signal, broker, repo, _config(), mode="autonomous", now_ts=NOW_TS) assert result.placed is True # entry + ONE native bracket = 2 place_order calls (was 3: entry + stop leg + target leg) @@ -844,7 +845,7 @@ def test_a_filled_order_records_the_previewed_commission_as_its_fee(repo): ) signal = _enter_signal() - result = execute(signal, broker, repo, _config(), "bypass", confirm_fn=None, now_ts=NOW_TS) + result = execute(signal, broker, repo, _config(), "autonomous", confirm_fn=None, now_ts=NOW_TS) assert result.placed is True order = repo.get_order(result.order_id) @@ -1006,7 +1007,7 @@ def test_execute_surfaces_the_bracket_order_id_it_placed(repo): """ broker = FakeBroker() - result = execute(_enter_signal(), broker, repo, _config(), mode="bypass", now_ts=NOW_TS) + result = execute(_enter_signal(), broker, repo, _config(), mode="autonomous", now_ts=NOW_TS) assert result.bracket_order_id is not None, "execute discarded the bracket's order id again" bracket = repo.get_order(result.bracket_order_id) @@ -1024,7 +1025,7 @@ def test_a_vetoed_bracket_leaves_no_bracket_order_id(repo): # DCA carries no stop, so no bracket is ever placed for it. result = execute( _enter_signal(setup=_setup(context={"order_class": "dca"})), - broker, repo, _config(), mode="bypass", now_ts=NOW_TS, + broker, repo, _config(), mode="autonomous", now_ts=NOW_TS, ) assert result.placed is True @@ -1188,7 +1189,9 @@ def test_an_exit_cancels_the_resting_bracket_before_selling(repo): rule_name="pullback_continuation", now_ts=NOW_TS, ) - result = execute(_exit_signal(), broker, repo, _config(), "bypass", None, now_ts=NOW_TS + 10) + result = execute( + _exit_signal(), broker, repo, _config(), "autonomous", None, now_ts=NOW_TS + 10 + ) assert result.placed is True assert repo.get_order(bracket_id)["status"] == "canceled" @@ -1215,7 +1218,9 @@ def cancel_order(self, order_id: str) -> bool: ) placed_before = len(broker.place_calls) - result = execute(_exit_signal(), broker, repo, _config(), "bypass", None, now_ts=NOW_TS + 10) + result = execute( + _exit_signal(), broker, repo, _config(), "autonomous", None, now_ts=NOW_TS + 10 + ) assert result.placed is False assert "bracket" in (result.reason or "").lower() @@ -1226,7 +1231,7 @@ def test_an_entry_does_not_try_to_cancel_anything(repo): """Negative control: the bracket-clearing step is EXIT-only. An entry must not touch it.""" broker = FakeBroker() - execute(_enter_signal(), broker, repo, _config(), "bypass", None, now_ts=NOW_TS) + execute(_enter_signal(), broker, repo, _config(), "autonomous", None, now_ts=NOW_TS) assert broker.cancel_calls == [] @@ -1251,7 +1256,7 @@ def get_order(self, order_id: str) -> dict: broker = _ObservingBroker() - result = execute(_enter_signal(), broker, repo, _config(), "bypass", None, now_ts=NOW_TS) + result = execute(_enter_signal(), broker, repo, _config(), "autonomous", None, now_ts=NOW_TS) order = repo.get_order(result.order_id) assert order["actual_fill"] == Decimal("50123.45") @@ -1269,7 +1274,7 @@ def get_order(self, order_id: str) -> dict: broker = _BlindBroker() - result = execute(_enter_signal(), broker, repo, _config(), "bypass", None, now_ts=NOW_TS) + result = execute(_enter_signal(), broker, repo, _config(), "autonomous", None, now_ts=NOW_TS) assert result.placed is True order = repo.get_order(result.order_id) diff --git a/tests/security/__init__.py b/tests/security/__init__.py deleted file mode 100644 index e69de29b..00000000 diff --git a/tests/security/test_authz.py b/tests/security/test_authz.py deleted file mode 100644 index 1ad2a623..00000000 --- a/tests/security/test_authz.py +++ /dev/null @@ -1,177 +0,0 @@ -"""Tests for keel.security.authz: set_passphrase, verify, require, AuthzError. - -Main spec §14 "Dangerous-action authorization gate": a passphrase (stored as a scrypt hash, -rate-limited) is required only for {arm_bypass, raise_caps, disable_killswitch, unlock_vault}. -Read-only/confirm-mode actions require nothing. -""" - -from __future__ import annotations - -import json -import os - -import pytest - -from keel.security.authz import ( - DANGEROUS_ACTIONS, - MAX_ATTEMPTS, - AuthzError, - require, - set_passphrase, - verify, -) - - -@pytest.fixture -def authz_path(tmp_path): - return tmp_path / "authz.json" - - -def test_set_passphrase_persists_hash_not_plaintext(authz_path): - set_passphrase("correct horse battery staple", path=authz_path) - - raw = authz_path.read_text() - assert "correct horse battery staple" not in raw - - state = json.loads(raw) - assert "hash" in state - assert "salt" in state - assert state["hash"] != "correct horse battery staple" - - -def test_set_passphrase_uses_a_random_salt_per_call(authz_path, tmp_path): - other_path = tmp_path / "authz2.json" - - set_passphrase("same passphrase", path=authz_path) - set_passphrase("same passphrase", path=other_path) - - state_a = json.loads(authz_path.read_text()) - state_b = json.loads(other_path.read_text()) - assert state_a["salt"] != state_b["salt"] - assert state_a["hash"] != state_b["hash"] - - -def test_verify_correct_passphrase_returns_true(authz_path): - set_passphrase("swordfish", path=authz_path) - - assert verify("swordfish", path=authz_path) is True - - -def test_verify_wrong_passphrase_returns_false(authz_path): - set_passphrase("swordfish", path=authz_path) - - assert verify("wrong-guess", path=authz_path) is False - - -def test_verify_with_no_passphrase_configured_returns_false(authz_path): - assert not authz_path.exists() - - assert verify("anything", path=authz_path) is False - - -def test_verify_resets_failed_attempts_after_success(authz_path): - set_passphrase("swordfish", path=authz_path) - - verify("wrong-1", path=authz_path) - verify("wrong-2", path=authz_path) - assert verify("swordfish", path=authz_path) is True - - state = json.loads(authz_path.read_text()) - assert state["failed_attempts"] == 0 - - -def test_require_correct_passphrase_passes_for_dangerous_action(authz_path): - set_passphrase("swordfish", path=authz_path) - - require("arm_bypass", "swordfish", path=authz_path) # must not raise - - -def test_require_wrong_passphrase_raises_authzerror(authz_path): - set_passphrase("swordfish", path=authz_path) - - with pytest.raises(AuthzError, match="arm_bypass"): - require("arm_bypass", "wrong-guess", path=authz_path) - - -@pytest.mark.parametrize("action", sorted(DANGEROUS_ACTIONS)) -def test_require_gates_every_dangerous_action(authz_path, action): - set_passphrase("swordfish", path=authz_path) - - require(action, "swordfish", path=authz_path) # correct passphrase passes - with pytest.raises(AuthzError): - require(action, "wrong-guess", path=authz_path) - - -def test_require_readonly_action_needs_no_passphrase_even_when_unset(authz_path): - assert not authz_path.exists() - - require("view_status", "", path=authz_path) # must not raise - - -def test_require_readonly_action_needs_no_passphrase_even_when_wrong(authz_path): - set_passphrase("swordfish", path=authz_path) - - require("confirm_trade", "totally-wrong", path=authz_path) # must not raise - - -def test_dangerous_actions_constant_matches_spec(): - assert DANGEROUS_ACTIONS == frozenset( - {"arm_bypass", "raise_caps", "disable_killswitch", "unlock_vault"} - ) - - -def test_require_with_no_passphrase_configured_raises_for_dangerous_action(authz_path): - assert not authz_path.exists() - - with pytest.raises(AuthzError): - require("unlock_vault", "whatever", path=authz_path) - - -def test_n_wrong_attempts_locks_out_further_attempts(authz_path): - set_passphrase("swordfish", path=authz_path) - now = 1_000_000.0 - - for i in range(MAX_ATTEMPTS): - assert verify(f"wrong-{i}", path=authz_path, now=now) is False - - # Locked out now: even the *correct* passphrase is rejected while locked. - assert verify("swordfish", path=authz_path, now=now) is False - - state = json.loads(authz_path.read_text()) - assert state["locked_until"] > now - - -def test_lockout_expires_after_backoff_window(authz_path): - set_passphrase("swordfish", path=authz_path) - now = 2_000_000.0 - - for i in range(MAX_ATTEMPTS): - verify(f"wrong-{i}", path=authz_path, now=now) - assert verify("swordfish", path=authz_path, now=now) is False - - state = json.loads(authz_path.read_text()) - locked_until = state["locked_until"] - - # Still locked just before the window elapses. - assert verify("swordfish", path=authz_path, now=locked_until - 1) is False - # Free again once the backoff window has passed. - assert verify("swordfish", path=authz_path, now=locked_until + 1) is True - - -def test_require_raises_during_lockout_even_with_correct_passphrase(authz_path): - set_passphrase("swordfish", path=authz_path) - now = 3_000_000.0 - - for i in range(MAX_ATTEMPTS): - verify(f"wrong-{i}", path=authz_path, now=now) - - with pytest.raises(AuthzError): - require("raise_caps", "swordfish", path=authz_path, now=now) - - -@pytest.mark.skipif(os.name == "nt", reason="POSIX file permissions only") -def test_authz_file_is_not_world_readable(authz_path): - set_passphrase("swordfish", path=authz_path) - - mode = authz_path.stat().st_mode & 0o777 - assert mode & 0o077 == 0 diff --git a/tests/security/test_secrets.py b/tests/security/test_secrets.py deleted file mode 100644 index f5339cda..00000000 --- a/tests/security/test_secrets.py +++ /dev/null @@ -1,108 +0,0 @@ -"""Tests for keel.security.secrets: the portable AES-GCM encrypted secrets vault. - -Main spec §14 Part A: master passphrase -> scrypt KDF -> key -> AES-GCM encrypt/decrypt a JSON -secrets blob in `secrets.enc`, copyable between machines. -""" - -from __future__ import annotations - -import stat - -import pytest - -from keel.security.secrets import VaultError, load_vault, migrate_from_env, save_vault - -SECRETS = { - "api_key": "organizations/abc/apiKeys/def", - "api_secret": "-----BEGIN EC PRIVATE KEY-----\nverysecret\n-----END EC PRIVATE KEY-----", -} - - -def test_save_then_load_vault_round_trips_secrets(tmp_path): - vault_path = tmp_path / "secrets.enc" - - save_vault(SECRETS, "correct horse battery staple", path=vault_path) - loaded = load_vault("correct horse battery staple", path=vault_path) - - assert loaded == SECRETS - - -def test_save_vault_chmods_file_600(tmp_path): - vault_path = tmp_path / "secrets.enc" - - save_vault(SECRETS, "correct horse battery staple", path=vault_path) - - mode = stat.S_IMODE(vault_path.stat().st_mode) - assert mode == 0o600 - - -def test_load_vault_with_wrong_passphrase_raises_vaulterror(tmp_path): - vault_path = tmp_path / "secrets.enc" - save_vault(SECRETS, "correct horse battery staple", path=vault_path) - - with pytest.raises(VaultError): - load_vault("wrong passphrase", path=vault_path) - - -def test_load_vault_with_tampered_file_raises_vaulterror(tmp_path): - vault_path = tmp_path / "secrets.enc" - save_vault(SECRETS, "correct horse battery staple", path=vault_path) - - raw = bytearray(vault_path.read_bytes()) - raw[-1] ^= 0xFF # flip the last byte of the ciphertext/tag - vault_path.write_bytes(bytes(raw)) - - with pytest.raises(VaultError): - load_vault("correct horse battery staple", path=vault_path) - - -def test_load_vault_missing_file_raises_vaulterror(tmp_path): - vault_path = tmp_path / "does-not-exist.enc" - - with pytest.raises(VaultError): - load_vault("any passphrase", path=vault_path) - - -def test_on_disk_blob_is_ciphertext_not_plaintext_values(tmp_path): - vault_path = tmp_path / "secrets.enc" - - save_vault(SECRETS, "correct horse battery staple", path=vault_path) - - raw = vault_path.read_bytes() - assert b"organizations/abc/apiKeys/def" not in raw - assert b"BEGIN EC PRIVATE KEY" not in raw - assert b"verysecret" not in raw - - -def test_two_saves_of_the_same_secrets_produce_different_ciphertext(tmp_path): - """Fresh salt+nonce per save (no key/nonce reuse) even for identical plaintext.""" - path_a = tmp_path / "a.enc" - path_b = tmp_path / "b.enc" - - save_vault(SECRETS, "correct horse battery staple", path=path_a) - save_vault(SECRETS, "correct horse battery staple", path=path_b) - - assert path_a.read_bytes() != path_b.read_bytes() - - -def test_migrate_from_env_seals_env_secrets_into_vault(tmp_path): - env_path = tmp_path / ".env" - env_path.write_text('CDP_API_KEY="organizations/abc/apiKeys/def"\nCDP_API_SECRET="shh-dont-log-me"\n') - vault_path = tmp_path / "secrets.enc" - - migrate_from_env(env_path=env_path, passphrase="correct horse battery staple", path=vault_path) - - loaded = load_vault("correct horse battery staple", path=vault_path) - assert loaded == { - "api_key": "organizations/abc/apiKeys/def", - "api_secret": "shh-dont-log-me", - } - - -def test_migrate_from_env_missing_file_yields_empty_vault(tmp_path): - env_path = tmp_path / ".env" - vault_path = tmp_path / "secrets.enc" - - migrate_from_env(env_path=env_path, passphrase="correct horse battery staple", path=vault_path) - - assert load_vault("correct horse battery staple", path=vault_path) == {} diff --git a/tests/test_agent.py b/tests/test_agent.py index 0a0507de..6c4f5a7d 100644 --- a/tests/test_agent.py +++ b/tests/test_agent.py @@ -152,13 +152,10 @@ def repo() -> Repository: # of each test's `now_ts`. r.set_state("withdrawals_enabled", True) r.set_state("withdrawals_attested_at", 10**12) - # `_config()` below defaults `auto_trade.mode` to "bypass" -- Issue #60 (bypass-arm - # hardening) means `run_once` now refuses that mode unarmed. Every pre-existing test in - # this module was written against "bypass just works"; arming here (a huge ttl so no test's - # `now_ts` can ever run past `armed_until`) keeps that behavior for tests that aren't - # specifically exercising the arm/disarm/expiry gate itself (those call - # `repo.disarm_bypass()` or a short ttl explicitly). - r.arm_bypass(now_ts=0, ttl_sec=10**12) + # Autonomy is a PROFILE choice now. Most tests in this module were written against "it just + # places"; opting the profile in here preserves that, and the tests that are specifically + # about the confirm/autonomous decision set the profile themselves. + r.set_autonomous(True, now_ts=0) # rail 14 now derives its cap from the attested subscription record rather than a config # default; attest a very large allowance so pre-existing tests here (none of which exercise # rail 14) aren't incidentally tripped by it. @@ -178,7 +175,7 @@ def _config(**overrides: Any) -> Config: max_per_asset_pct=Decimal("1"), ), market_data=MarketDataConfig(granularities=[Granularity.ONE_DAY], history_days=365), - auto_trade=AutoTradeConfig(mode="bypass", interval_sec=50_000), + auto_trade=AutoTradeConfig(mode="confirm", interval_sec=50_000), money_mgmt=MoneyMgmtConfig(), dca=DcaConfig(budget_usd=Decimal("50"), cadence_days=7), ) @@ -222,7 +219,7 @@ def _seed_open_position( expected_fill=price, actual_fill=price, raw_response=None, - confirmation="bypass", + confirmation="autonomous", rule_id=None, created_at=ts, updated_at=ts, @@ -315,73 +312,75 @@ def test_run_once_polls_evaluates_and_executes_a_real_dca_rule(repo): assert repo.get_state("last_feed_ts") == 90_000 -# -- run_once: bypass-arm hardening (Issue #60) -------------------------------------------------- +# -- run_once: autonomy is a live-read profile choice -------------------------------------------- -def test_bypass_without_armed_token_places_nothing_and_reports_refusal(repo): - """The core fix: `config.auto_trade.mode == "bypass"` with no armed token must not place - any order, even though a real, merged `Dca` rule would otherwise fire -- it fails safe by - falling back to confirm behavior (`confirm_fn=None` -> never placed) and surfaces why. - """ - repo.disarm_bypass() +def test_autonomy_off_places_nothing_even_though_the_rule_fires(repo): + """The default. The rule still fires and is logged, but with no `confirm_fn` the order is + previewed and never placed -- confirm mode fails closed.""" + repo.set_autonomous(False, now_ts=0) repo.insert_rule("dca", {"product_id": PRODUCT}, status="live") broker = FakeBroker(series={(PRODUCT, Granularity.ONE_DAY): [_candle(0, "100")]}) result = run_once(broker, repo, _config(), now_ts=90_000) - assert result.skipped is False assert len(result.enter_signals) == 1 # the rule still fires... assert result.enter_results[0].placed is False # ...but nothing is placed. assert broker.place_calls == [] - assert result.mode == "confirm" # fell back, fail-safe - assert result.bypass_refused_reason is not None - assert "armed" in result.bypass_refused_reason.lower() + assert result.mode == "confirm" assert repo.get_orders() == [] -def test_bypass_with_expired_token_places_nothing(repo): - repo.arm_bypass(now_ts=1_000, ttl_sec=10) # armed_until = 1_010 +def test_autonomy_on_places_without_a_confirm_fn(repo): + repo.set_autonomous(True, now_ts=0) repo.insert_rule("dca", {"product_id": PRODUCT}, status="live") broker = FakeBroker(series={(PRODUCT, Granularity.ONE_DAY): [_candle(0, "100")]}) - result = run_once(broker, repo, _config(), now_ts=90_000) # long past armed_until + result = run_once(broker, repo, _config(), now_ts=90_000) + + assert result.mode == "autonomous" + assert result.enter_results[0].placed is True + assert len(broker.place_calls) == 1 + assert repo.get_orders(mode="live", product_id=PRODUCT)[0]["side"] == "BUY" + + +def test_an_absent_profile_row_is_treated_as_NOT_autonomous(repo): + """Fails closed: a database that never recorded a choice must not imply consent.""" + repo._conn.execute("DELETE FROM profile") + repo._conn.commit() + repo.insert_rule("dca", {"product_id": PRODUCT}, status="live") + broker = FakeBroker(series={(PRODUCT, Granularity.ONE_DAY): [_candle(0, "100")]}) + + result = run_once(broker, repo, _config(), now_ts=90_000) - assert result.enter_results[0].placed is False - assert broker.place_calls == [] assert result.mode == "confirm" - assert result.bypass_refused_reason is not None + assert broker.place_calls == [] -def test_bypass_with_fresh_armed_token_places_normally(repo): - """A freshly armed token (well within ttl) lets bypass through -- still subject to every - guard, but with no confirm prompt required, exactly like bypass behaved before Issue #60.""" - repo.disarm_bypass() - repo.arm_bypass(now_ts=1_000, ttl_sec=100_000) # armed_until = 101_000 +def test_the_profile_is_re_read_every_cycle_not_cached(repo): + """`keel autonomy off` must take effect on the NEXT order, not the next restart.""" + repo.set_autonomous(True, now_ts=0) repo.insert_rule("dca", {"product_id": PRODUCT}, status="live") broker = FakeBroker(series={(PRODUCT, Granularity.ONE_DAY): [_candle(0, "100")]}) - result = run_once(broker, repo, _config(), now_ts=90_000) # inside the armed window + first = run_once(broker, repo, _config(), now_ts=90_000) + assert first.mode == "autonomous" - assert result.mode == "bypass" - assert result.bypass_refused_reason is None - assert result.enter_results[0].placed is True - assert len(broker.place_calls) == 1 - assert repo.get_orders(mode="live", product_id=PRODUCT)[0]["side"] == "BUY" + repo.set_autonomous(False, now_ts=1) + second = run_once(broker, repo, _config(), now_ts=180_000) + assert second.mode == "confirm", "the profile was cached; turning autonomy off did nothing" -def test_confirm_mode_unaffected_by_arming_state(repo): - """`mode="confirm"` never even looks at the arm token -- arm-check only gates bypass.""" - repo.disarm_bypass() +def test_paper_mode_places_nothing_even_when_autonomous(repo): + """The two switches are independent: autonomy never turns a simulation into real money.""" + repo.set_autonomous(True, now_ts=0) repo.insert_rule("dca", {"product_id": PRODUCT}, status="live") broker = FakeBroker(series={(PRODUCT, Granularity.ONE_DAY): [_candle(0, "100")]}) - config = _config(auto_trade=AutoTradeConfig(mode="confirm", interval_sec=50_000)) + config = _config(auto_trade=AutoTradeConfig(mode="paper", interval_sec=50_000)) - result = run_once(broker, repo, config, now_ts=90_000) + run_once(broker, repo, config, now_ts=90_000) - assert result.mode == "confirm" - assert result.bypass_refused_reason is None # bypass was never requested - assert result.enter_results[0].placed is False # confirm_fn=None -> fails closed, as always - assert broker.place_calls == [] + assert broker.place_calls == [], "paper mode must never reach the broker" # -- run_once: EXIT wiring on a held position --------------------------------------------------- @@ -1091,6 +1090,7 @@ def _live_ready_repo(repo): def test_confirm_APPROVED_places_the_order(repo, monkeypatch): """The change: an approved confirm-mode order is actually placed (was: never).""" + repo.set_autonomous(False, now_ts=0) # these tests are ABOUT the confirm gate _live_ready_repo(repo) _seed_rule(repo, monkeypatch, _AlwaysEnterRule(PRODUCT), status="live") broker = FakeBroker(series={(PRODUCT, Granularity.ONE_DAY): [_candle(0, "100")]}) @@ -1103,6 +1103,7 @@ def test_confirm_APPROVED_places_the_order(repo, monkeypatch): def test_confirm_DECLINED_places_nothing(repo, monkeypatch): + repo.set_autonomous(False, now_ts=0) # these tests are ABOUT the confirm gate _live_ready_repo(repo) _seed_rule(repo, monkeypatch, _AlwaysEnterRule(PRODUCT), status="live") broker = FakeBroker(series={(PRODUCT, Granularity.ONE_DAY): [_candle(0, "100")]}) @@ -1114,6 +1115,7 @@ def test_confirm_DECLINED_places_nothing(repo, monkeypatch): def test_confirm_fn_defaulting_to_None_still_fails_closed(repo, monkeypatch): """No confirm_fn (the old default) must still place nothing -- backward compatible.""" + repo.set_autonomous(False, now_ts=0) # these tests are ABOUT the confirm gate _live_ready_repo(repo) _seed_rule(repo, monkeypatch, _AlwaysEnterRule(PRODUCT), status="live") broker = FakeBroker(series={(PRODUCT, Granularity.ONE_DAY): [_candle(0, "100")]}) @@ -1124,6 +1126,7 @@ def test_confirm_fn_defaulting_to_None_still_fails_closed(repo, monkeypatch): def test_confirm_fn_sees_the_broker_preview(repo, monkeypatch): + repo.set_autonomous(False, now_ts=0) # these tests are ABOUT the confirm gate _live_ready_repo(repo) _seed_rule(repo, monkeypatch, _AlwaysEnterRule(PRODUCT), status="live") broker = FakeBroker(series={(PRODUCT, Granularity.ONE_DAY): [_candle(0, "100")]}) @@ -1140,6 +1143,7 @@ def _capture(preview): def test_a_rail_veto_means_the_confirm_prompt_is_never_reached(repo, monkeypatch): """The rails run FIRST. A vetoed order never asks the human -- confirmation is not a substitute for the hard limits.""" + repo.set_autonomous(False, now_ts=0) # these tests are ABOUT the confirm gate _live_ready_repo(repo) _seed_rule(repo, monkeypatch, _AlwaysEnterRule("DOGE-USD"), status="live") # off allowlist broker = FakeBroker(series={("DOGE-USD", Granularity.ONE_DAY): [_candle(0, "100")]}) @@ -1183,6 +1187,7 @@ def test_interactive_confirm_fails_closed_without_a_tty(monkeypatch): def test_agent_command_passes_interactive_confirm_in_CONFIRM_mode(repo, monkeypatch): """The wiring: `keel agent` (confirm) hands run_once the interactive confirm_fn; bypass hands it None.""" + repo.set_autonomous(False, now_ts=0) # these tests are ABOUT the confirm gate from click.testing import CliRunner import keel.cli as cli_module diff --git a/tests/test_cli.py b/tests/test_cli.py index 98fa2245..2be32d43 100644 --- a/tests/test_cli.py +++ b/tests/test_cli.py @@ -6,9 +6,8 @@ `keel.cli._build_broker` (the one seam that would otherwise construct a real, network-talking `CoinbaseClient`) to return it instead. -Dangerous commands (`agent --bypass`, `resume`) are gated by `keel.security.authz`; read-only -commands (`db import`, `monitor`, `rules list`, `pnl`) are not and work with no `authz.json` on -disk at all. +Halt-releasing commands (`resume`, `reset-hwm`, ...) demand a typed `yes` from a terminal; +read-only commands (`db import`, `monitor`, `rules list`, `pnl`) need no confirmation at all. """ from __future__ import annotations @@ -25,11 +24,9 @@ from keel.cli import DISCLAIMER, cli from keel.data.db import connect, migrate from keel.data.repository import Repository -from keel.security import authz from keel.types import Candle, Granularity FIXTURES_DIR = Path(__file__).parent / "fixtures" / "transactions_dir" -PASSPHRASE = "correct-horse-battery-staple" def _repo_at(db_path: Path) -> Repository: @@ -86,20 +83,6 @@ def test_db_import_runs_importer_against_temp_db(tmp_path): assert len(repo.get_transactions()) > 0 -def test_db_import_needs_no_passphrase(tmp_path): - """Read-only command: works even though no --authz-path file exists at all.""" - db_path = tmp_path / "test.db" - authz_path = tmp_path / "authz.json" - runner = CliRunner() - - result = runner.invoke( - cli, - ["--db", str(db_path), "--authz-path", str(authz_path), "db", "import", str(FIXTURES_DIR)], - ) - - assert result.exit_code == 0, result.output - assert not authz_path.exists() - # -- disclaimer ----------------------------------------------------------------------------- @@ -115,10 +98,9 @@ def test_disclaimer_shown_on_every_command(tmp_path): def test_disclaimer_shown_even_when_refused(tmp_path): db_path = tmp_path / "test.db" - authz_path = tmp_path / "authz.json" runner = CliRunner() - result = runner.invoke(cli, ["--db", str(db_path), "--authz-path", str(authz_path), "resume"]) + result = runner.invoke(cli, ["--db", str(db_path), "resume"]) assert result.exit_code != 0 assert DISCLAIMER in result.output @@ -127,74 +109,12 @@ def test_disclaimer_shown_even_when_refused(tmp_path): # -- agent --bypass gating -------------------------------------------------------------------- -def test_agent_bypass_without_passphrase_is_refused(tmp_path, valid_config_path, monkeypatch): - monkeypatch.setattr(cli_module, "_build_broker", lambda config: FakeBroker()) - db_path = tmp_path / "test.db" - authz_path = tmp_path / "authz.json" - runner = CliRunner() - - result = runner.invoke( - cli, - [ - "--db", str(db_path), - "--config", str(valid_config_path), - "--authz-path", str(authz_path), - "agent", "--bypass", - ], - ) - - assert result.exit_code != 0 - assert "denied" in result.output.lower() - - -def test_agent_bypass_with_wrong_passphrase_is_refused(tmp_path, valid_config_path, monkeypatch): - monkeypatch.setattr(cli_module, "_build_broker", lambda config: FakeBroker()) - db_path = tmp_path / "test.db" - authz_path = tmp_path / "authz.json" - authz.set_passphrase(PASSPHRASE, path=str(authz_path)) - runner = CliRunner() - - result = runner.invoke( - cli, - [ - "--db", str(db_path), - "--config", str(valid_config_path), - "--authz-path", str(authz_path), - "agent", "--bypass", "--passphrase", "wrong-passphrase", - ], - ) - - assert result.exit_code != 0 - assert "denied" in result.output.lower() - - -def test_agent_bypass_with_correct_passphrase_proceeds(tmp_path, valid_config_path, monkeypatch): - monkeypatch.setattr(cli_module, "_build_broker", lambda config: FakeBroker()) - db_path = tmp_path / "test.db" - authz_path = tmp_path / "authz.json" - authz.set_passphrase(PASSPHRASE, path=str(authz_path)) - runner = CliRunner() - - result = runner.invoke( - cli, - [ - "--db", str(db_path), - "--config", str(valid_config_path), - "--authz-path", str(authz_path), - "agent", "--bypass", "--passphrase", PASSPHRASE, - ], - ) - # No `live` rules are configured and the kill-switch defaults to engaged, so `run_once` - # fails closed immediately -- but crucially the authz gate let it get that far. - assert result.exit_code == 0, result.output - assert "skipped: kill_switch" in result.output def test_agent_confirm_mode_needs_no_passphrase(tmp_path, valid_config_path, monkeypatch): monkeypatch.setattr(cli_module, "_build_broker", lambda config: FakeBroker()) db_path = tmp_path / "test.db" - authz_path = tmp_path / "authz.json" runner = CliRunner() result = runner.invoke( @@ -202,13 +122,11 @@ def test_agent_confirm_mode_needs_no_passphrase(tmp_path, valid_config_path, mon [ "--db", str(db_path), "--config", str(valid_config_path), - "--authz-path", str(authz_path), - "agent", + "agent", ], ) assert result.exit_code == 0, result.output - assert not authz_path.exists() def test_agent_loop_bounded_by_max_cycles(tmp_path, valid_config_path, monkeypatch): @@ -232,135 +150,9 @@ def test_agent_loop_bounded_by_max_cycles(tmp_path, valid_config_path, monkeypat # -- arm-bypass / disarm-bypass (Issue #60, bypass-arm hardening) ------------------------------ -def test_arm_bypass_without_passphrase_is_refused(tmp_path, valid_config_path): - db_path = tmp_path / "test.db" - authz_path = tmp_path / "authz.json" - runner = CliRunner() - - result = runner.invoke( - cli, - [ - "--db", str(db_path), - "--config", str(valid_config_path), - "--authz-path", str(authz_path), - "arm-bypass", - ], - ) - - assert result.exit_code != 0 - assert "denied" in result.output.lower() - repo = _repo_at(db_path) - assert repo.is_bypass_armed(now_ts=0) is False - - -def test_arm_bypass_with_wrong_passphrase_is_refused(tmp_path, valid_config_path): - db_path = tmp_path / "test.db" - authz_path = tmp_path / "authz.json" - authz.set_passphrase(PASSPHRASE, path=str(authz_path)) - runner = CliRunner() - - result = runner.invoke( - cli, - [ - "--db", str(db_path), - "--config", str(valid_config_path), - "--authz-path", str(authz_path), - "arm-bypass", "--passphrase", "wrong-passphrase", - ], - ) - - assert result.exit_code != 0 - assert "denied" in result.output.lower() - repo = _repo_at(db_path) - assert repo.is_bypass_armed(now_ts=0) is False - -def test_arm_bypass_with_correct_passphrase_arms(tmp_path, valid_config_path): - db_path = tmp_path / "test.db" - authz_path = tmp_path / "authz.json" - authz.set_passphrase(PASSPHRASE, path=str(authz_path)) - runner = CliRunner() - result = runner.invoke( - cli, - [ - "--db", str(db_path), - "--config", str(valid_config_path), - "--authz-path", str(authz_path), - "arm-bypass", "--passphrase", PASSPHRASE, - ], - ) - assert result.exit_code == 0, result.output - assert "armed" in result.output.lower() - repo = _repo_at(db_path) - # `valid_config_path`'s auto_trade.bypass_arm_ttl_sec is 3600 -- armed "now" is well inside. - assert repo.is_bypass_armed(now_ts=int(time.time())) is True - - -def test_disarm_bypass_clears_the_token_no_passphrase_needed(tmp_path, valid_config_path): - db_path = tmp_path / "test.db" - authz_path = tmp_path / "authz.json" - authz.set_passphrase(PASSPHRASE, path=str(authz_path)) - runner = CliRunner() - runner.invoke( - cli, - [ - "--db", str(db_path), - "--config", str(valid_config_path), - "--authz-path", str(authz_path), - "arm-bypass", "--passphrase", PASSPHRASE, - ], - ) - repo = _repo_at(db_path) - assert repo.is_bypass_armed(now_ts=int(time.time())) is True - - result = runner.invoke( - cli, - [ - "--db", str(db_path), - "--config", str(valid_config_path), - "--authz-path", str(authz_path), - "disarm-bypass", - ], - ) - - assert result.exit_code == 0, result.output - assert "disarmed" in result.output.lower() - repo = _repo_at(db_path) - assert repo.is_bypass_armed(now_ts=int(time.time())) is False - - -def test_agent_bypass_without_arm_bypass_places_nothing_even_with_passphrase( - tmp_path, valid_config_path, monkeypatch -): - """The Issue #60 gap being closed: the CLI passphrase gate on `agent --bypass` alone is not - enough -- without a separate `arm-bypass` call, `run_once` itself refuses to trade - autonomously and the CLI surfaces that refusal.""" - monkeypatch.setattr(cli_module, "_build_broker", lambda config: FakeBroker()) - db_path = tmp_path / "test.db" - authz_path = tmp_path / "authz.json" - authz.set_passphrase(PASSPHRASE, path=str(authz_path)) - repo = _repo_at(db_path) - repo.set_state("kill_switch", False) - repo.insert_rule("dca", {"product_id": "BTC-USD"}, status="live") - runner = CliRunner() - - result = runner.invoke( - cli, - [ - "--db", str(db_path), - "--config", str(valid_config_path), - "--authz-path", str(authz_path), - "agent", "--bypass", "--passphrase", PASSPHRASE, - ], - ) - - assert result.exit_code == 0, result.output - assert "bypass" in result.output.lower() - assert "not armed" in result.output.lower() or "refused" in result.output.lower() - repo = _repo_at(db_path) - assert repo.get_orders() == [] # -- kill / resume ---------------------------------------------------------------------------- @@ -379,38 +171,20 @@ def test_kill_engages_kill_switch_no_passphrase_needed(tmp_path): def test_resume_without_passphrase_is_refused(tmp_path): db_path = tmp_path / "test.db" - authz_path = tmp_path / "authz.json" runner = CliRunner() runner.invoke(cli, ["--db", str(db_path), "kill"]) - result = runner.invoke(cli, ["--db", str(db_path), "--authz-path", str(authz_path), "resume"]) + result = runner.invoke(cli, ["--db", str(db_path), "resume"]) assert result.exit_code != 0 repo = _repo_at(db_path) assert repo.get_state("kill_switch", default=True) is True -def test_resume_with_wrong_passphrase_is_refused(tmp_path): - db_path = tmp_path / "test.db" - authz_path = tmp_path / "authz.json" - authz.set_passphrase(PASSPHRASE, path=str(authz_path)) - runner = CliRunner() - runner.invoke(cli, ["--db", str(db_path), "kill"]) - - result = runner.invoke( - cli, - ["--db", str(db_path), "--authz-path", str(authz_path), "resume", "--passphrase", "nope"], - ) - assert result.exit_code != 0 - repo = _repo_at(db_path) - assert repo.get_state("kill_switch") is True - - -def test_resume_with_correct_passphrase_disengages(tmp_path): +def test_resume_disengages_when_confirmed(tmp_path, monkeypatch): + _at_a_terminal(monkeypatch) db_path = tmp_path / "test.db" - authz_path = tmp_path / "authz.json" - authz.set_passphrase(PASSPHRASE, path=str(authz_path)) runner = CliRunner() runner.invoke(cli, ["--db", str(db_path), "kill"]) @@ -418,9 +192,9 @@ def test_resume_with_correct_passphrase_disengages(tmp_path): cli, [ "--db", str(db_path), - "--authz-path", str(authz_path), - "resume", "--passphrase", PASSPHRASE, + "resume", ], + input="yes\n", ) assert result.exit_code == 0, result.output @@ -434,7 +208,6 @@ def test_resume_with_correct_passphrase_disengages(tmp_path): def test_monitor_single_poll_needs_no_passphrase(tmp_path, valid_config_path, monkeypatch): monkeypatch.setattr(cli_module, "_build_broker", lambda config: FakeBroker()) db_path = tmp_path / "test.db" - authz_path = tmp_path / "authz.json" runner = CliRunner() result = runner.invoke( @@ -442,14 +215,12 @@ def test_monitor_single_poll_needs_no_passphrase(tmp_path, valid_config_path, mo [ "--db", str(db_path), "--config", str(valid_config_path), - "--authz-path", str(authz_path), - "monitor", + "monitor", ], ) assert result.exit_code == 0, result.output assert result.output.count("polled") == 1 - assert not authz_path.exists() def test_monitor_loop_bounded_by_max_cycles(tmp_path, valid_config_path, monkeypatch): @@ -674,22 +445,19 @@ def test_rules_seed_rows_round_trip_through_build_rule(tmp_path, valid_config_pa def test_rules_seed_needs_no_passphrase(tmp_path): db_path = tmp_path / "test.db" - authz_path = tmp_path / "authz.json" runner = CliRunner() result = runner.invoke( cli, [ "--db", str(db_path), - "--authz-path", str(authz_path), - "rules", "seed", + "rules", "seed", "--products", "BTC-USD", "--kinds", "dca", ], ) assert result.exit_code == 0, result.output - assert not authz_path.exists() # -- pnl -------------------------------------------------------------------------------------- @@ -1019,7 +787,7 @@ def _repo_at(db_path): return Repository(conn) -def test_resume_entries_clears_an_armed_streak_halt(tmp_path): +def test_resume_entries_clears_an_armed_streak_halt(tmp_path, monkeypatch): """Rail 16's violation message tells the operator to run `keel resume-entries`. Until now that command did not exist, and a test merely asserted the message MENTIONED it -- pinning a promise nothing implemented. @@ -1028,50 +796,29 @@ def test_resume_entries_clears_an_armed_streak_halt(tmp_path): setting `max_consecutive_losses: 0` does NOT release an armed halt. Without this, an operator who mis-set the cooloff waits it out or edits sqlite by hand. """ + _at_a_terminal(monkeypatch) db_path = tmp_path / "test.db" - authz_path = tmp_path / "authz.json" - authz.set_passphrase(PASSPHRASE, path=str(authz_path)) repo = _repo_at(db_path) repo.set_state("streak_halt_until", 2_000_000_000) runner = CliRunner() result = runner.invoke( cli, - ["--db", str(db_path), "--authz-path", str(authz_path), - "resume-entries", "--passphrase", PASSPHRASE], + ["--db", str(db_path), "resume-entries"], + input="yes\n", ) assert result.exit_code == 0, result.output assert _repo_at(db_path).get_state("streak_halt_until") == 0 -def test_resume_entries_is_passphrase_gated(tmp_path): - """Negative control: clearing a live-money breaker is a dangerous action, gated like - `resume` -- not something a stray shell command can do.""" - db_path = tmp_path / "test.db" - authz_path = tmp_path / "authz.json" - authz.set_passphrase(PASSPHRASE, path=str(authz_path)) - repo = _repo_at(db_path) - repo.set_state("streak_halt_until", 2_000_000_000) - runner = CliRunner() - result = runner.invoke( - cli, - ["--db", str(db_path), "--authz-path", str(authz_path), - "resume-entries", "--passphrase", "wrong-passphrase"], - ) - - assert result.exit_code != 0 - assert _repo_at(db_path).get_state("streak_halt_until") == 2_000_000_000 - - -def test_reset_hwm_clears_the_equity_high_water_mark(tmp_path): +def test_reset_hwm_clears_the_equity_high_water_mark(tmp_path, monkeypatch): """Rail 11's high-water mark is MONOTONIC, so any bad equity write is permanent: a deposit ratchets it up and a later withdrawal then reads as a drawdown that never recovers. Without this command the only remedy is hand-editing sqlite.""" + _at_a_terminal(monkeypatch) db_path = tmp_path / "test.db" - authz_path = tmp_path / "authz.json" - authz.set_passphrase(PASSPHRASE, path=str(authz_path)) repo = _repo_at(db_path) repo.set_state("equity_high_water_mark", Decimal("15000")) repo.set_state("drawdown_total_pct", Decimal("0.33")) @@ -1079,8 +826,8 @@ def test_reset_hwm_clears_the_equity_high_water_mark(tmp_path): result = runner.invoke( cli, - ["--db", str(db_path), "--authz-path", str(authz_path), - "reset-hwm", "--passphrase", PASSPHRASE], + ["--db", str(db_path), "reset-hwm"], + input="yes\n", ) assert result.exit_code == 0, result.output @@ -1089,78 +836,56 @@ def test_reset_hwm_clears_the_equity_high_water_mark(tmp_path): assert after.get_state("drawdown_total_pct") == Decimal("0") -def test_record_flow_rebases_the_high_water_mark(tmp_path): +def test_record_flow_rebases_the_high_water_mark(tmp_path, monkeypatch): """A deposit is not profit and a withdrawal is not a loss, but equity is cash + positions so both move it. Declaring the flow keeps rail 11's drawdown measuring TRADING performance.""" + _at_a_terminal(monkeypatch) db_path = tmp_path / "test.db" - authz_path = tmp_path / "authz.json" - authz.set_passphrase(PASSPHRASE, path=str(authz_path)) repo = _repo_at(db_path) repo.set_state("equity_high_water_mark", Decimal("10000")) runner = CliRunner() result = runner.invoke( cli, - ["--db", str(db_path), "--authz-path", str(authz_path), - "record-flow", "--amount", "5000", "--passphrase", PASSPHRASE], + ["--db", str(db_path), "record-flow", "--amount", "5000"], + input="yes\n", ) assert result.exit_code == 0, result.output assert _repo_at(db_path).get_state("equity_high_water_mark") == Decimal("15000") -def test_record_flow_accepts_a_negative_amount_for_a_withdrawal(tmp_path): +def test_record_flow_accepts_a_negative_amount_for_a_withdrawal(tmp_path, monkeypatch): + _at_a_terminal(monkeypatch) db_path = tmp_path / "test.db" - authz_path = tmp_path / "authz.json" - authz.set_passphrase(PASSPHRASE, path=str(authz_path)) repo = _repo_at(db_path) repo.set_state("equity_high_water_mark", Decimal("15000")) runner = CliRunner() result = runner.invoke( cli, - ["--db", str(db_path), "--authz-path", str(authz_path), - "record-flow", "--amount", "-5000", "--passphrase", PASSPHRASE], + ["--db", str(db_path), "record-flow", "--amount", "-5000"], + input="yes\n", ) assert result.exit_code == 0, result.output assert _repo_at(db_path).get_state("equity_high_water_mark") == Decimal("10000") -def test_record_flow_is_passphrase_gated(tmp_path): - """Lowering the HWM relaxes a live-money breaker, so this is a dangerous action: an - unauthenticated caller must not be able to shrink a measured drawdown.""" - db_path = tmp_path / "test.db" - authz_path = tmp_path / "authz.json" - authz.set_passphrase(PASSPHRASE, path=str(authz_path)) - repo = _repo_at(db_path) - repo.set_state("equity_high_water_mark", Decimal("15000")) - runner = CliRunner() - - result = runner.invoke( - cli, - ["--db", str(db_path), "--authz-path", str(authz_path), - "record-flow", "--amount", "-5000", "--passphrase", "wrong-passphrase"], - ) - assert result.exit_code != 0 - assert _repo_at(db_path).get_state("equity_high_water_mark") == Decimal("15000") - - -def test_record_flow_rejects_a_non_finite_amount(tmp_path): +def test_record_flow_rejects_a_non_finite_amount(tmp_path, monkeypatch): """`Decimal("nan")` parses without raising. Written into the high-water mark it poisons it permanently: every later `equity > hwm` is False, so the HWM can never re-seed.""" + _at_a_terminal(monkeypatch) db_path = tmp_path / "test.db" - authz_path = tmp_path / "authz.json" - authz.set_passphrase(PASSPHRASE, path=str(authz_path)) repo = _repo_at(db_path) repo.set_state("equity_high_water_mark", Decimal("10000")) runner = CliRunner() result = runner.invoke( cli, - ["--db", str(db_path), "--authz-path", str(authz_path), - "record-flow", "--amount", "nan", "--passphrase", PASSPHRASE], + ["--db", str(db_path), "record-flow", "--amount", "nan"], + input="yes\n", ) assert result.exit_code != 0 @@ -1223,3 +948,139 @@ def test_migrate_honours_an_explicit_db_option(tmp_path): assert int(conn.execute("SELECT version FROM schema_version").fetchone()["version"]) == ( SCHEMA_VERSION ) + + +# -- halt-releasing commands: interactive confirmation, no passphrase ---------- +# +# These four RE-PERMIT trading after a safety halt. They keep a human gate even when autonomous +# mode is on -- "trade without asking me" and "un-stick your own drawdown breaker" are different +# powers, and a breaker that can reset itself is not a breaker. +# +# There is deliberately NO env-var/flag override for the TTY check: any such seam would be +# settable from cron and would defeat the fail-closed. Tests patch the predicate instead. + +_HALT_COMMANDS = ( + ["resume"], + ["resume-entries"], + ["record-flow", "--amount", "500"], + ["reset-hwm"], +) + + +def _at_a_terminal(monkeypatch, yes: bool = True) -> None: + monkeypatch.setattr(cli_module, "_is_interactive", lambda: yes) + + +def test_halt_commands_proceed_on_a_typed_yes(tmp_path, monkeypatch): + _at_a_terminal(monkeypatch) + for args in _HALT_COMMANDS: + db = tmp_path / f"{args[0]}-yes.db" + _repo_at(db) + result = CliRunner().invoke(cli, ["--db", str(db), *args], input="yes\n") + assert result.exit_code == 0, f"{args}: {result.output}" + + +def test_halt_commands_abort_on_anything_other_than_yes(tmp_path, monkeypatch): + """A bare 'y' is not enough -- these are rarer and heavier than an order confirmation.""" + _at_a_terminal(monkeypatch) + for args in _HALT_COMMANDS: + db = tmp_path / f"{args[0]}-no.db" + _repo_at(db) + result = CliRunner().invoke(cli, ["--db", str(db), *args], input="y\n") + assert result.exit_code != 0, f"{args} should have aborted: {result.output}" + assert "aborted" in result.output.lower() + + +def test_halt_commands_fail_closed_without_a_tty(tmp_path, monkeypatch): + """A cron job or piped script must never be able to release a safety halt.""" + _at_a_terminal(monkeypatch, yes=False) + for args in _HALT_COMMANDS: + db = tmp_path / f"{args[0]}-notty.db" + _repo_at(db) + result = CliRunner().invoke(cli, ["--db", str(db), *args], input="yes\n") + assert result.exit_code != 0, f"{args} should have refused off-TTY: {result.output}" + assert "terminal" in result.output.lower() + + +def test_resume_actually_disengages_the_kill_switch_when_confirmed(tmp_path, monkeypatch): + _at_a_terminal(monkeypatch) + db = tmp_path / "resume.db" + repo = _repo_at(db) + repo.set_state("kill_switch", True) + result = CliRunner().invoke(cli, ["--db", str(db), "resume"], input="yes\n") + assert result.exit_code == 0, result.output + assert _repo_at(db).get_state("kill_switch") is False + + +# -- keel autonomy ------------------------------------------------------------ + + +def test_autonomy_is_off_by_default(tmp_path): + db = tmp_path / "a.db" + _repo_at(db) + result = CliRunner().invoke(cli, ["--db", str(db), "autonomy", "show"]) + assert result.exit_code == 0, result.output + assert "off" in result.output + + +def test_autonomy_on_requires_a_typed_yes_and_persists(tmp_path, monkeypatch, valid_config_path): + _at_a_terminal(monkeypatch) + db = tmp_path / "a.db" + _repo_at(db) + result = CliRunner().invoke( + cli, ["--db", str(db), "--config", str(valid_config_path), "autonomy", "on"], input="yes\n" + ) + assert result.exit_code == 0, result.output + assert _repo_at(db).get_profile().autonomous is True + + +def test_autonomy_on_aborts_on_a_bare_y(tmp_path, monkeypatch, valid_config_path): + _at_a_terminal(monkeypatch) + db = tmp_path / "a.db" + _repo_at(db) + result = CliRunner().invoke( + cli, ["--db", str(db), "--config", str(valid_config_path), "autonomy", "on"], input="y\n" + ) + assert result.exit_code != 0 + assert _repo_at(db).get_profile().autonomous is False + + +def test_autonomy_on_refuses_without_a_terminal(tmp_path, monkeypatch, valid_config_path): + """Arming unattended trading must not be scriptable.""" + _at_a_terminal(monkeypatch, yes=False) + db = tmp_path / "a.db" + _repo_at(db) + result = CliRunner().invoke( + cli, ["--db", str(db), "--config", str(valid_config_path), "autonomy", "on"], input="yes\n" + ) + assert result.exit_code != 0 + assert "terminal" in result.output.lower() + assert _repo_at(db).get_profile().autonomous is False + + +def test_autonomy_off_works_without_a_terminal(tmp_path, monkeypatch): + """De-risking is never obstructed: this must work from cron, a pipe, anywhere.""" + _at_a_terminal(monkeypatch, yes=False) + db = tmp_path / "a.db" + repo = _repo_at(db) + repo.set_autonomous(True, now_ts=1) + + result = CliRunner().invoke(cli, ["--db", str(db), "autonomy", "off"]) + + assert result.exit_code == 0, result.output + assert _repo_at(db).get_profile().autonomous is False + + +def test_autonomy_ON_does_not_let_halt_commands_skip_confirmation(tmp_path, monkeypatch): + """THE invariant. 'Trade without asking me' and 'un-stick your own drawdown breaker' are + different powers; a breaker that can reset itself is not a breaker.""" + _at_a_terminal(monkeypatch, yes=False) # no terminal available + for args in _HALT_COMMANDS: + db = tmp_path / f"auto-{args[0]}.db" + repo = _repo_at(db) + repo.set_autonomous(True, now_ts=1) # fully autonomous... + + result = CliRunner().invoke(cli, ["--db", str(db), *args], input="yes\n") + + assert result.exit_code != 0, f"{args} was released without a human: {result.output}" + assert "terminal" in result.output.lower() diff --git a/uv.lock b/uv.lock index 04e393d0..688dcf2d 100644 --- a/uv.lock +++ b/uv.lock @@ -398,7 +398,6 @@ version = "0.1.0" source = { editable = "." } dependencies = [ { name = "click" }, - { name = "cryptography" }, { name = "keel-broker-api" }, { name = "keel-broker-coinbase" }, { name = "keel-core" }, @@ -415,7 +414,6 @@ dev = [ [package.metadata] requires-dist = [ { name = "click", specifier = ">=8.4.2" }, - { name = "cryptography", specifier = ">=49.0.0" }, { name = "keel-broker-api", editable = "packages/keel-broker-api" }, { name = "keel-broker-coinbase", editable = "packages/keel-broker-coinbase" }, { name = "keel-core", editable = "packages/keel-core" }, From 3d79949a96e765c5e3bf5780da06440bf9b4464a Mon Sep 17 00:00:00 2001 From: Elmehdi Aitbrahim Date: Tue, 21 Jul 2026 19:13:07 -0400 Subject: [PATCH 4/9] feat(config): validate auto_trade.mode, drop bypass_arm_ttl_sec mode is now paper|confirm and an unknown value RAISES naming the key, where it previously degraded silently. A config saying 'mode: bypass' was explicitly asking for autonomy, and autonomy is now a profile choice -- quietly reinterpreting that request in either direction is worse than stopping. Co-Authored-By: Claude Opus 4.8 (1M context) --- config.yaml | 4 -- keel/templates/config.live.yaml | 4 -- keel/templates/config.yaml | 4 -- packages/keel-core/keel_core/config.py | 29 ++++++++--- tests/fixtures/config_golden_defaults.json | 1 - tests/fixtures/config_golden_full.json | 1 - tests/fixtures/config_golden_full.yaml | 1 - tests/test_config.py | 58 ++++++++++++++-------- 8 files changed, 59 insertions(+), 43 deletions(-) diff --git a/config.yaml b/config.yaml index 3d589682..31778146 100644 --- a/config.yaml +++ b/config.yaml @@ -37,10 +37,6 @@ auto_trade: mode: paper enabled: false interval_sec: 900 - # how long a `keel arm-bypass` token stays valid (seconds) before bypass mode requires - # re-arming (Issue #60) -- `keel agent --bypass` still needs a fresh armed token even with - # the CLI passphrase gate satisfied. - bypass_arm_ttl_sec: 3600 promotion: min_trades: 100 diff --git a/keel/templates/config.live.yaml b/keel/templates/config.live.yaml index a1840e98..4cc7e9a0 100644 --- a/keel/templates/config.live.yaml +++ b/keel/templates/config.live.yaml @@ -49,10 +49,6 @@ auto_trade: mode: confirm enabled: false interval_sec: 900 - # how long a `keel arm-bypass` token stays valid (seconds) before bypass mode requires - # re-arming (Issue #60) -- `keel agent --bypass` still needs a fresh armed token even with - # the CLI passphrase gate satisfied. - bypass_arm_ttl_sec: 3600 promotion: min_trades: 100 diff --git a/keel/templates/config.yaml b/keel/templates/config.yaml index 3d589682..31778146 100644 --- a/keel/templates/config.yaml +++ b/keel/templates/config.yaml @@ -37,10 +37,6 @@ auto_trade: mode: paper enabled: false interval_sec: 900 - # how long a `keel arm-bypass` token stays valid (seconds) before bypass mode requires - # re-arming (Issue #60) -- `keel agent --bypass` still needs a fresh armed token even with - # the CLI passphrase gate satisfied. - bypass_arm_ttl_sec: 3600 promotion: min_trades: 100 diff --git a/packages/keel-core/keel_core/config.py b/packages/keel-core/keel_core/config.py index b514c2fe..45fd486e 100644 --- a/packages/keel-core/keel_core/config.py +++ b/packages/keel-core/keel_core/config.py @@ -96,11 +96,6 @@ class AutoTradeConfig: mode: str = "paper" enabled: bool = False interval_sec: int = 900 - # Issue #60: how long a `keel arm-bypass` token stays valid (`Repository.arm_bypass`'s - # `ttl_sec`) before `agent.run_once` refuses `mode="bypass"` again and falls back to - # `"confirm"`. 3600s (1h) is a sensible default -- long enough for one supervised session, - # short enough that a forgotten arm doesn't grant unattended autonomy indefinitely. - bypass_arm_ttl_sec: int = 3600 @dataclass(frozen=True) @@ -135,6 +130,10 @@ class DcaConfig: _VALID_PACING_MODES = ("opportunistic", "even_daily") +#: `paper` simulates and places nothing; `confirm` is live. Whether you are ASKED is a +#: PROFILE choice (`keel autonomy on|off`), deliberately not a config mode -- config.yaml +#: ships as a release asset, and arming unattended trading should not be a YAML edit. +_VALID_AUTO_TRADE_MODES = ("paper", "confirm") @dataclass(frozen=True) @@ -278,6 +277,23 @@ class Config: ) +def _parse_auto_trade_mode(raw: dict[str, Any]) -> str: + """`auto_trade.mode`, validated against `_VALID_AUTO_TRADE_MODES`. + + Previously any unknown value silently degraded to confirm. It now raises: a config saying + `mode: bypass` was explicitly asking for autonomy, and autonomy has since become a profile + choice. Reinterpreting that request quietly -- in either direction -- is worse than stopping. + """ + mode = raw.get("mode", "paper") + if mode not in _VALID_AUTO_TRADE_MODES: + raise ConfigError( + f"auto_trade.mode: invalid value {mode!r}; must be one of " + f"{_VALID_AUTO_TRADE_MODES!r}. Autonomy is no longer a mode -- it is a profile " + f"choice: use `mode: confirm` plus `keel autonomy on`." + ) + return str(mode) + + def _parse_allowlist(raw: dict[str, Any]) -> list[str]: allowlist = raw.get("allowlist") if not allowlist or not isinstance(allowlist, list): @@ -523,10 +539,9 @@ def load_config(path: str | Path) -> Config: caps=caps, market_data=market_data, auto_trade=AutoTradeConfig( - mode=auto_trade_raw.get("mode", "paper"), + mode=_parse_auto_trade_mode(auto_trade_raw), enabled=bool(auto_trade_raw.get("enabled", False)), interval_sec=int(auto_trade_raw.get("interval_sec", 900)), - bypass_arm_ttl_sec=int(auto_trade_raw.get("bypass_arm_ttl_sec", 3600)), ), promotion=PromotionConfig( min_trades=int(promotion_raw.get("min_trades", 100)), diff --git a/tests/fixtures/config_golden_defaults.json b/tests/fixtures/config_golden_defaults.json index 40055e73..6e229176 100644 --- a/tests/fixtures/config_golden_defaults.json +++ b/tests/fixtures/config_golden_defaults.json @@ -3,7 +3,6 @@ "BTC" ], "auto_trade": { - "bypass_arm_ttl_sec": 3600, "enabled": false, "interval_sec": 900, "mode": "paper" diff --git a/tests/fixtures/config_golden_full.json b/tests/fixtures/config_golden_full.json index e89d7124..e3943abb 100644 --- a/tests/fixtures/config_golden_full.json +++ b/tests/fixtures/config_golden_full.json @@ -5,7 +5,6 @@ "PAXG" ], "auto_trade": { - "bypass_arm_ttl_sec": 7200, "enabled": true, "interval_sec": 1800, "mode": "confirm" diff --git a/tests/fixtures/config_golden_full.yaml b/tests/fixtures/config_golden_full.yaml index 95d8b602..b768bdd0 100644 --- a/tests/fixtures/config_golden_full.yaml +++ b/tests/fixtures/config_golden_full.yaml @@ -35,7 +35,6 @@ auto_trade: mode: confirm enabled: true interval_sec: 1800 - bypass_arm_ttl_sec: 7200 promotion: min_trades: 42 diff --git a/tests/test_config.py b/tests/test_config.py index 454a2486..e7df8ea6 100644 --- a/tests/test_config.py +++ b/tests/test_config.py @@ -280,28 +280,7 @@ def test_load_config_tier_missing_name_raises_configerror(write_config): # -- auto_trade.bypass_arm_ttl_sec (Issue #60, bypass-arm hardening) --------------------------- -def test_load_config_bypass_arm_ttl_sec_default_is_one_hour(valid_config_path): - config = load_config(valid_config_path) - - assert config.auto_trade.bypass_arm_ttl_sec == 3600 - - -def test_load_config_bypass_arm_ttl_sec_overridable(write_config): - text = VALID_CONFIG_YAML.replace("bypass_arm_ttl_sec: 3600", "bypass_arm_ttl_sec: 120") - path = write_config(text) - - config = load_config(path) - - assert config.auto_trade.bypass_arm_ttl_sec == 120 - - -def test_load_config_bypass_arm_ttl_sec_absent_falls_back_to_default(write_config): - text = VALID_CONFIG_YAML.replace(" bypass_arm_ttl_sec: 3600\n", "") - path = write_config(text) - - config = load_config(path) - assert config.auto_trade.bypass_arm_ttl_sec == 3600 # -- logging (engine-activity logging feature) -------------------------------------------------- @@ -477,3 +456,40 @@ def test_non_negative_int_rejects_a_negative_threshold(write_config): ) with pytest.raises(ConfigError, match="max_consecutive_losses"): load_config(write_config(text)) + + +# -- auto_trade.mode is validated (it used to fall back silently) -------------- + + +def test_mode_bypass_is_now_a_hard_error(tmp_path): + """`bypass` no longer exists. Failing loudly beats silently reinterpreting a config that + explicitly asked for autonomy -- autonomy is now a profile choice, not a config mode.""" + p = tmp_path / "c.yaml" + p.write_text( + "allowlist: [BTC]\ncaps: {max_exposure_usd: 100, max_per_asset_pct: 0.5}\n" + "auto_trade: {mode: bypass}\n" + ) + with pytest.raises(ConfigError) as exc: + load_config(str(p)) + assert "auto_trade.mode" in str(exc.value) + + +def test_paper_and_confirm_both_load(tmp_path): + for mode in ("paper", "confirm"): + p = tmp_path / f"{mode}.yaml" + p.write_text( + "allowlist: [BTC]\ncaps: {max_exposure_usd: 100, max_per_asset_pct: 0.5}\n" + f"auto_trade: {{mode: {mode}}}\n" + ) + assert load_config(str(p)).auto_trade.mode == mode + + +def test_an_unknown_mode_names_the_offending_key(tmp_path): + p = tmp_path / "c.yaml" + p.write_text( + "allowlist: [BTC]\ncaps: {max_exposure_usd: 100, max_per_asset_pct: 0.5}\n" + "auto_trade: {mode: wibble}\n" + ) + with pytest.raises(ConfigError) as exc: + load_config(str(p)) + assert "wibble" in str(exc.value) From 8c0515c7b717fab671f6fda34956757e71f7469a Mon Sep 17 00:00:00 2001 From: Elmehdi Aitbrahim Date: Tue, 21 Jul 2026 19:14:24 -0400 Subject: [PATCH 5/9] =?UTF-8?q?docs:=20rewrite=20the=20go-live=20runbook;?= =?UTF-8?q?=20record=20the=20=C2=A714=20security=20reversal?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The runbook documented the arm-bypass/passphrase dance that no longer exists. Rewritten against the real flow, and honest that its purpose is proving the plumbing (place_order has never run against the real API), not profit. Supersedes PR #116. Marks spec §14's vault + passphrase gate as BUILT THEN REMOVED, with the reasoning, so neither is re-proposed from the old text. Co-Authored-By: Claude Opus 4.8 (1M context) --- docs/go-live-runbook.md | 151 ++++++++++++++++++ .../specs/2026-07-15-keel-autotrade-design.md | 31 ++-- 2 files changed, 172 insertions(+), 10 deletions(-) create mode 100644 docs/go-live-runbook.md diff --git a/docs/go-live-runbook.md b/docs/go-live-runbook.md new file mode 100644 index 00000000..372ff003 --- /dev/null +++ b/docs/go-live-runbook.md @@ -0,0 +1,151 @@ +# Go-live runbook — the first supervised live order + +The purpose of this run is **not** profit. It is to prove the live execution path works, because +`place_order` has never been executed against the real Coinbase API. Everything up to now — +683 tests, a positive backtest, a passing verdict — says the *logic* is right. None of it says the +*plumbing* is. Treat the first order as an experiment whose result is "the order appeared on the +exchange, correctly, and we have a record of it". + +**Budget the whole thing as money you are willing to lose outright.** + +--- + +## 0. Before you start + +| | check | why | +|---|---|---| +| ☐ | `keel --version` reports `[release]`, no `DIRTY`, no `[checkout]` | a build that matches no commit is not reproducible; do not run it against funds | +| ☐ | you have a **trade-enabled** CDP key (the read-only one cannot place orders) | | +| ☐ | `.env` holds `CDP_API_KEY` / `CDP_API_SECRET`, and `.env` is git-ignored | credentials live only here — there is no vault | +| ☐ | you are at a real terminal | confirmation and every halt-releasing command fail closed off a TTY | +| ☐ | funds are in **USDC**, not USD | rail 13 vetoes a BUY that is not funded from `quote_currency`, and never draws from a bank/ACH | + +## 1. Bring the deployment up + +```bash +keel migrate # existing database: apply outstanding schema migrations +# or, on a brand-new deployment: +keel init # writes config.yaml and seeds the rule library as `candidate` +keel init-config --live # optional: the production config (mode: confirm) +``` + +Then check the config you are actually about to run: + +```bash +keel config show 2>/dev/null || grep -A3 '^auto_trade:' config.yaml +``` + +- `auto_trade.mode: confirm` — live, and asks before each order. +- `caps.max_exposure_usd` — set this to something you can lose. For a first run, small. +- `allowlist` — only assets you have screened and attested. + +## 2. Attest what the rails need + +Several rails **fail closed** until a human has attested something. This is deliberate: an +unattested venue or asset is treated as unknown, not as fine. + +```bash +keel assets list # what is attested, and what is not +keel subscription show # rail 14's spend allowance comes from this record +keel withdrawals show # rail 17: withdrawal capability is a compliance precondition +``` + +Attest anything missing (`keel assets attest`, `keel subscription attest`, +`keel withdrawals attest`) before continuing. If a rail vetoes later, its message names the +command that fixes it. + +## 3. Promote exactly one rule + +Seeded rules are `candidate` and trade nothing. Promote **one**, on **one** product: + +```bash +keel rules list +keel rules promote # candidate -> paper -> live +``` + +Promoting to `live` is what the promotion floor exists to gate. If you are deliberately +short-circuiting it for this test, know that you are — and promote a single rule, not the library. + +## 4. Run one cycle, in confirm mode, and watch + +```bash +keel agent +``` + +Autonomy is **off** by default, so this is what you should see: + +``` +Rails PASSED. Coinbase order preview: + order_total: 5.00 + ... +Place this order? [y/N]: +``` + +**Read the preview before answering.** It is the broker's own numbers, not keel's estimate. Check +the product, the side, and the total. If anything surprises you, answer `N` — declining places +nothing and costs nothing. + +The rails have already passed at this point. The prompt is an **additional** human gate, never a +replacement for them. + +## 5. Verify against reality + +Do not trust the tool's own success message alone. Check the exchange: + +```bash +keel pnl +keel rules list +``` + +- The order appears in the Coinbase UI/app with matching product, side and size. +- `orders` has a row with the real broker order id. +- The fill price is sane against the market at that moment. + +If the order did **not** appear but keel thinks it placed one, stop and investigate before +running anything else. That is the exact failure this run exists to catch. + +## 6. Halting + +```bash +keel kill # engage the kill-switch: halts all trading immediately +keel resume # release it -- asks for a typed "yes" at a terminal +``` + +The kill-switch is checked first on every cycle and **defaults to engaged**, so a damaged or +unreadable state halts trading rather than permitting it. + +Four commands re-permit trading after a halt — `resume`, `resume-entries`, `record-flow`, +`reset-hwm`. Each demands a typed `yes` from a terminal and **cannot be run from a script or cron +job**. That is deliberate: a breaker that can reset itself is not a breaker. + +## 7. Only afterwards: autonomy + +Do **not** turn this on for the first run. + +```bash +keel autonomy show +keel autonomy on # asks for a typed "yes"; requires a terminal +keel autonomy off # always allowed, works anywhere, needs no terminal +``` + +Autonomy stops keel asking before each order. It changes **who is asked, never what is allowed** — +every hard rail still runs first, and it does **not** let the agent clear a safety halt. The +setting lives in your profile in the database and is re-read on every order, so `keel autonomy off` +takes effect on the **next order**, not the next restart. + +Turn it on only once you have watched several supervised cycles behave correctly. + +--- + +## What can still go wrong + +- **`place_order` has never run against the real API.** This runbook is that test. Expect the + unexpected on the first attempt, and keep the size trivial. +- **A rail vetoes and you disagree.** Read the veto message; it names the rail and the command + that clears it. Do not work around a rail — the rails are un-overridable by design, including + in autonomous mode. +- **Confirm mode places nothing when run headless.** That is not a bug: with no TTY the + confirmation declines. Use a terminal, or turn autonomy on deliberately. +- **Equity moved because you deposited or withdrew.** Tell the tool (`keel record-flow --amount + ±N`), or rail 11 will read the movement as drawdown and veto entries on an account that lost + nothing. diff --git a/docs/superpowers/specs/2026-07-15-keel-autotrade-design.md b/docs/superpowers/specs/2026-07-15-keel-autotrade-design.md index f519cf21..8c3b6cf8 100644 --- a/docs/superpowers/specs/2026-07-15-keel-autotrade-design.md +++ b/docs/superpowers/specs/2026-07-15-keel-autotrade-design.md @@ -272,16 +272,27 @@ Enforced in `guards.py` **before every order**, **un-overridable in any mode (in losable slice (§22.3). - **No classic user authentication.** For a local single-user CLI/daemon, logins/accounts/sessions are YAGNI (OS access already opens that door). We invest instead in the two controls below. -- **Portable encrypted secrets vault** (`security/secrets.py`): the CDP key/secret (and any remote-control token) - live in an **AES-GCM-encrypted `secrets.enc`** unlocked by a **master passphrase** (passphrase → scrypt KDF → - key). One file, **copyable between machines** (explicitly *not* the machine-bound OS Keychain, so it's - transportable). `chmod 600`; secrets never logged; `.enc`/`.env` git-ignored. An optional per-machine keychain - *cache* of the derived key may be added for convenience, but the portable `.enc` is the source of truth. -- **Dangerous-action authorization gate** (`security/authz.py`): a passphrase (stored as a scrypt hash, - rate-limited) required **only** to **arm bypass/autonomous mode**, **raise caps above config maxima**, or - **disable the kill-switch / resume**. Read-only commands and confirm-mode trades need none. This is local - *authorization*, not a login — it forces intentionality and blocks casual/accidental/shoulder-surf misuse of - the autonomy capability. +- ⛔ **SUPERSEDED 2026-07-21 — the vault and the passphrase gate were BUILT and then REMOVED.** + See `2026-07-21-security-simplification-design.md`. Both are gone; do not re-propose them without + reading that spec first. What replaced them: + - **Credentials come from a git-ignored `.env`** (`config.load_secrets`). The encrypted + `secrets.enc` vault (`security/secrets.py`) was deleted. It had in fact never been on the live + path: `cb_client`/`cli` always loaded credentials from `.env`, and the vault was reachable only + via `migrate_from_env` — so it was a *competing* credential path, not the real one. + - **The scrypt passphrase gate (`security/authz.py`) was deleted.** Of its four declared + actions, `raise_caps` and `unlock_vault` were never used at all. Once placing a real, + money-spending order needs only a typed confirmation, requiring a remembered secret to reset a + high-water mark is ceremony without a matching threat model. One rule replaced it: **dangerous + actions need a human at a terminal; nothing needs a stored secret.** The four halt-releasing + commands (`resume`, `resume-entries`, `record-flow`, `reset-hwm`) demand a typed `yes` and fail + closed off a TTY, with deliberately no env/flag override (any such seam would be settable from + cron and would defeat the fail-closed). + - **Autonomy is a profile choice, not a config mode.** `auto_trade.mode` is now `paper|confirm`; + the `profile` table (schema v7) holds the user's `autonomous` flag, re-read live on every order + so `keel autonomy off` binds on the *next* order. It fails closed on an absent row, cannot be + enabled without a terminal, and **never clears a safety halt**. + - Real user authentication arrives with the future **server-hosted** deployment; a local + single-user vault + passphrase was ceremony in the meantime. - **Honest boundary:** on a single-user machine the real security boundary is the **OS account + full-disk encryption**. These controls are defense-in-depth (reduce plaintext exposure, prevent casual misuse); they do **not** stop an attacker who already holds the OS account. Stated so they aren't mistaken for more. From 58ddf1b4dbb88d8df99f6bcf8b7f5d135868d9f6 Mon Sep 17 00:00:00 2001 From: Elmehdi Aitbrahim Date: Tue, 21 Jul 2026 19:16:03 -0400 Subject: [PATCH 6/9] docs(cli): rewrite the module docstring for the new security model Removes the last descriptions of the vault, the passphrase gate and the arm-bypass token -- no live references to any of them remain anywhere in keel/, packages/, tests/ or scripts/. Co-Authored-By: Claude Opus 4.8 (1M context) --- keel/cli.py | 56 ++++++++++++++----------------- tests/conftest.py | 1 - tests/test_config.py | 1 - tests/test_subscription_record.py | 2 +- 4 files changed, 26 insertions(+), 34 deletions(-) diff --git a/keel/cli.py b/keel/cli.py index 2d226cf1..d35f64db 100644 --- a/keel/cli.py +++ b/keel/cli.py @@ -12,39 +12,31 @@ `subscription attest|set|show` (the per-venue, user-attested allowance rail 14 reads live), and a Phase-4 `insights` stub. -**Dangerous commands are gated.** Per the main spec §14 and `security.authz`, only -`{arm_bypass, raise_caps, disable_killswitch, unlock_vault}` require the passphrase gate. In this -CLI's surface that's `agent --bypass` and `arm-bypass` (both map to the `arm_bypass` action) and -`resume`, `resume-entries`, `record-flow` and `reset-hwm` (all `disable_killswitch` -- -each either releases a tripped breaker or rebases the state one reads) -- `kill` -(engaging the kill-switch) and `disarm-bypass` are -*safe* actions (they only ever reduce capability) and are always allowed; every other command -here is read-only or a local rules-table/DB mutation with no live-trading blast radius, so per -the plan ("read-only commands require no passphrase") they are not gated. There is no CLI surface -for `raise_caps`/`unlock_vault` in this task -- caps are config-file-only (no runtime override -exists in the merged `execution.guards`/`config` modules) and the vault (`security.secrets`) has -no CLI command in this task's scope. - -**`agent --bypass`'s passphrase gate is not sufficient on its own (Issue #60).** It only guards -this CLI entry point -- any other caller invoking `agent.run_once`/`agent.loop` in-process with -`config.auto_trade.mode == "bypass"` would bypass it entirely. `agent.run_once` therefore also -requires a separately armed, time-limited token (`Repository.is_bypass_armed`, set by -`arm-bypass`) before it will actually run in bypass mode -- an unarmed or expired request fails -safe to confirm-mode behavior (nothing placed) regardless of how `run_once` was invoked. Both -gates apply to the CLI path (defense in depth); only the arm-token check is un-bypassable -in-process. - -**No interactive hangs in tests.** `--passphrase` may be passed explicitly; if omitted, it is only -read via an interactive `click.prompt` when stdin is a real TTY (`_resolve_passphrase`) -- -non-interactive invocations (like `CliRunner`) get an empty passphrase, which fails the gate -closed rather than blocking on stdin. +**Dangerous commands ask a human; nothing needs a stored secret.** The former scrypt passphrase +gate is gone (see `2026-07-21-security-simplification-design.md`). Four commands re-permit trading +after a safety halt -- `resume`, `resume-entries`, `record-flow` and `reset-hwm` -- and each +demands a typed `yes` via `_require_interactive_confirmation`, **failing closed off a TTY** so a +script or cron job can never release a halt. `kill` (engaging the kill-switch) and `autonomy off` +are *safe* actions -- they only ever reduce capability -- and are always allowed, from anywhere. +Every other command here is read-only or a local rules-table/DB mutation with no live-trading +blast radius. + +**Autonomy is a profile choice, checked in-process.** `keel autonomy on` (typed `yes`, TTY +required) sets `profile.autonomous`; `agent.run_once` re-reads it every cycle via +`_effective_mode`, so the check cannot be skipped by a caller driving `run_once` in-process, and +turning autonomy off binds on the next order. It changes who is asked, never what is allowed: +`guards.check` runs first in every mode, and autonomy never releases a halt. + +**No interactive hangs in tests.** `_is_interactive()` is the single TTY predicate, with +deliberately no env-var or flag override -- any such seam would be settable from cron and would +defeat every fail-closed built on it. Tests patch the predicate. **Disclaimer.** Every command prints the halal + not-financial/religious-advice disclaimer footer (`with_disclaimer`, a decorator applied to every command's callback) -- always, even when -the command errors out or is refused by the authz gate. +the command errors out or is refused at a confirmation prompt. **No live network in tests.** `_build_broker` is the one seam that would construct a real -`CoinbaseClient` (from `.env`/vault secrets via `coinbase.rest.RESTClient`); tests monkeypatch it +`CoinbaseClient` (from `.env` secrets via `coinbase.rest.RESTClient`); tests monkeypatch it to inject a fake broker instead, exactly like `tests/test_agent.py`'s `FakeBroker`. """ @@ -368,7 +360,8 @@ def migrate_cmd(ctx: click.Context, db_override: str | None) -> None: on migrate would resurrect rules that were deliberately deleted or refuted. Runs `keel.data.db.migrate`, which steps the stored `schema_version` up incrementally and is - safe to call repeatedly. No network, no authz gate, no orders -- safe against a live database. + safe to call repeatedly. No network, no confirmation gate, no orders -- safe against a + live database. """ path = db_override or ctx.obj["db_path"] conn = connect(path) @@ -1501,7 +1494,8 @@ def rules_seed( already has a rule row of any status, so it's safe to call repeatedly (e.g. from a setup script) without piling up duplicate candidates. `--force` inserts a fresh candidate anyway. - Read-only w.r.t. the exchange: no network call, no authz gate -- it only ever writes local + Read-only w.r.t. the exchange: no network call, no confirmation gate -- it only ever + writes local `rules` rows, exactly like `rules promote`/`demote`/`disable`. """ repo = _open_repo(ctx) @@ -1892,7 +1886,7 @@ def simulate( no_trial_record: bool, skip_within_cap: bool, ) -> None: - """Simulate the deterministic engine over historical candles (read-only; no authz gate). + """Simulate the deterministic engine over historical candles (read-only; no confirmation gate). Pulls (unless `--no-fetch`) and caches ~`--years` years of candle history in the persistent DB (`--db`, never in-memory), replays the real rule set through the engine + a dollar diff --git a/tests/conftest.py b/tests/conftest.py index 82f43087..1450f90a 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -83,7 +83,6 @@ def attest_subscription( mode: paper enabled: false interval_sec: 900 - bypass_arm_ttl_sec: 3600 promotion: min_trades: 100 diff --git a/tests/test_config.py b/tests/test_config.py index e7df8ea6..ff911a4d 100644 --- a/tests/test_config.py +++ b/tests/test_config.py @@ -277,7 +277,6 @@ def test_load_config_tier_missing_name_raises_configerror(write_config): load_config(path) -# -- auto_trade.bypass_arm_ttl_sec (Issue #60, bypass-arm hardening) --------------------------- diff --git a/tests/test_subscription_record.py b/tests/test_subscription_record.py index 5a708ccf..86b1656a 100644 --- a/tests/test_subscription_record.py +++ b/tests/test_subscription_record.py @@ -66,7 +66,7 @@ def test_overdue_attestation_overrides_a_stored_active_status() -> None: def test_due_exactly_now_is_already_overdue() -> None: """Boundary: due-at is the moment it expires, not one tick still-good. - Matches `is_bypass_armed`'s strict `now_ts < armed_until` convention elsewhere. + Strict `now_ts < expiry`, the freshness convention used throughout this codebase. """ assert _record(attest_due_ts=NOW).effective_status(NOW) is SubscriptionStatus.SUSPECT From 484f1a4e3224c0b11d945b787ff297fe508355cd Mon Sep 17 00:00:00 2001 From: Elmehdi Aitbrahim Date: Tue, 21 Jul 2026 19:31:17 -0400 Subject: [PATCH 7/9] fix(security): gate rail 17's release, bound autonomy in time, honour fail-closed Addresses an independent review of this PR. Two blocking findings: 1. `keel withdrawals attest --enabled` RELEASES rail 17's entry halt and was ungated and scriptable. Its own docstring justified that with 'the confirm gate and the bypass-arm token still sit in front of it' -- this PR deletes the token, and with autonomy on the confirm gate is gone too, so cron could have re-permitted live entries with no human. Now gated like the other four; --suspended stays ungated (it only reduces capability). 2. Autonomy replaced a TTL-bounded authorization with an unbounded one -- a forgotten 'autonomy on' would grant unattended trading forever, where the old arm token expired in an hour. Adds optional 'keel autonomy on --for-hours N' (Profile.is_autonomous honours it); the default stays durable per the requirement, but 'on' now says plainly when no expiry is set, and 'show' reports remaining time or that it lapsed. profile is new on this branch, so the column lands in v7 rather than needing a v8. Also: get_profile catches sqlite3.Error so it actually implements the fail-closed contract it documents; _interactive_confirm uses the single _is_interactive predicate; disclaimer on all autonomy commands; record-flow shows the amount being confirmed; stale docstrings corrected; and config.yaml no longer implies auto_trade.enabled is a kill-switch (it is read by nothing). Co-Authored-By: Claude Opus 4.8 (1M context) --- config.yaml | 2 + ...26-07-21-security-simplification-design.md | 17 ++++- keel/agent.py | 14 ++-- keel/cli.py | 68 +++++++++++++---- keel/data/db.py | 3 + keel/data/repository.py | 37 ++++++--- keel/execution/executor.py | 3 + keel/templates/config.live.yaml | 3 +- keel/templates/config.yaml | 2 + packages/keel-core/keel_core/types.py | 16 ++++ tests/data/test_repository.py | 27 +++++++ tests/execution/test_executor.py | 7 +- .../execution/test_withdrawal_attestation.py | 9 ++- tests/test_cli.py | 76 ++++++++++++++++++- 14 files changed, 245 insertions(+), 39 deletions(-) diff --git a/config.yaml b/config.yaml index 31778146..d85bf9e1 100644 --- a/config.yaml +++ b/config.yaml @@ -35,6 +35,8 @@ market_data: auto_trade: mode: paper + # NOTE: currently UNUSED by any code path -- it is NOT a kill-switch and setting it + # true or false changes nothing. Use `keel kill` to halt trading. enabled: false interval_sec: 900 diff --git a/docs/superpowers/specs/2026-07-21-security-simplification-design.md b/docs/superpowers/specs/2026-07-21-security-simplification-design.md index 5baf49c4..2a692440 100644 --- a/docs/superpowers/specs/2026-07-21-security-simplification-design.md +++ b/docs/superpowers/specs/2026-07-21-security-simplification-design.md @@ -54,7 +54,13 @@ These are the properties the removed ceremony was standing in for. Each gets a t without asking me" and "un-stick your own drawdown breaker" are different powers. A rail that fired because something went wrong still needs a human to clear it — otherwise a breaker can silently reset itself and the rail stops meaning anything. -3. **Autonomy fails closed.** An absent/unreadable profile row reads as `autonomous = False`. +3. **Autonomy fails closed.** An absent/unreadable/damaged profile row reads as + `autonomous = False` — `get_profile` catches `sqlite3.Error` rather than relying on the caller + crashing first. +3a. **Autonomy may be time-bounded.** The removed bypass-arm token was TTL-limited so a forgotten + arm could not grant unattended trading forever. `keel autonomy on --for-hours N` restores that + bound; the default is a durable choice (per the requirement that it be tracked as a user + preference), and `autonomy on` says plainly when no expiry is set. 4. **Autonomy cannot be enabled non-interactively.** `keel autonomy on` requires a TTY and an explicit typed confirmation, so a script, cron job or piped command can never arm it. 5. **The kill-switch still short-circuits everything**, checked first, defaulting to engaged. @@ -76,13 +82,18 @@ path. Remove `keel/security/authz.py` and `tests/security/test_authz.py`. With both files gone the `keel/security/` package has no remaining contents and is deleted entirely. -Of the four declared dangerous actions, `raise_caps` and `unlock_vault` were **never used** (caps -are config-file-only; the vault is going). `arm_bypass` disappears with bypass mode. That leaves +Of the four declared dangerous actions, `raise_caps` and `unlock_vault` were **never wired to any +command** — the gate was declared for them and never applied. ⚠️ Note the premise is narrower than +it first looks: `raise_caps` was never *enforced*, but capability-raising commands do exist and +remain ungated — `keel subscription set/attest` raises rail 14's spend allowance at runtime, and +`keel assets attest` admits an asset. Neither is a regression (both were ungated before this work), +but neither should be mistaken for "caps cannot be raised at runtime". `arm_bypass` disappears with bypass mode. That leaves four *commands*, all one idea — re-permitting trading after a safety halt: | command | what it releases | |---|---| | `resume` | the kill-switch | +| `withdrawals attest --enabled` | rail 17's entry halt (⚠️ found in review — its old justification cited the confirm gate and the bypass-arm token, both of which this work removes) | | `resume-entries` | an armed consecutive-loss halt (rail 16) | | `record-flow` | declares an external deposit/withdrawal so rail 11 isn't fooled | | `reset-hwm` | rail 11's equity high-water mark, clearing a stuck drawdown halt | diff --git a/keel/agent.py b/keel/agent.py index db818ba2..012219d6 100644 --- a/keel/agent.py +++ b/keel/agent.py @@ -39,7 +39,8 @@ the order is previewed and logged but never placed. The CLI supplies a real interactive prompt. **Autonomy is a profile choice, not a config mode.** `_effective_mode` returns `"autonomous"` -only when `config.auto_trade.mode == "confirm"` **and** `repo.get_profile().autonomous` is true. +only when `config.auto_trade.mode == "confirm"` **and** +`repo.get_profile().is_autonomous(now_ts)` is true. The profile is read fresh every cycle and never cached, so `keel autonomy off` takes effect on the next order rather than the next restart. The check lives inside `run_once`, not only at the CLI, so an in-process caller cannot obtain autonomy the CLI would have refused; an absent or @@ -551,21 +552,22 @@ class LoopResult: exit_results: list[ExecutionResult] = field(default_factory=list) -def _effective_mode(config: Config, repo: Repository) -> str: +def _effective_mode(config: Config, repo: Repository, now_ts: int) -> str: """The executor mode for this cycle: `"autonomous"` or `"confirm"`. Two independent switches, deliberately not conflated into one enum: * `config.auto_trade.mode` says whether this is real money at all (`paper` never reaches an executor mode -- it routes to the paper path upstream). - * `repo.get_profile().autonomous` says whether the user has opted out of being asked. + * `repo.get_profile().is_autonomous(now_ts)` says whether the user has opted out of being + asked -- honouring any expiry the user set. `"autonomous"` is returned ONLY when the config is live (`confirm`) **and** the profile says so. Anything else -- an unknown mode, an absent profile row, a damaged database -- yields `"confirm"`, which with no `confirm_fn` places nothing at all. The failure direction is always toward asking a human. - **The profile is read here, fresh, on every cycle and never cached**, so `keel autonomy off` + **The profile is read here, fresh, once per cycle and never cached**, so `keel autonomy off` takes effect on the NEXT order rather than the next restart. This mirrors rail 14's allowance, which is re-read live for exactly the same reason. @@ -574,7 +576,7 @@ def _effective_mode(config: Config, repo: Repository) -> str: """ if config.auto_trade.mode != "confirm": return "confirm" - return "autonomous" if repo.get_profile().autonomous else "confirm" + return "autonomous" if repo.get_profile().is_autonomous(now_ts) else "confirm" def run_once( @@ -626,7 +628,7 @@ def run_once( # for recording that the check happened, independent of whether it found anything new. repo.set_state("last_feed_ts", now_ts) - mode = _effective_mode(config, repo) + mode = _effective_mode(config, repo, now_ts) # `mode: paper` is not an executor mode (see `_effective_mode`); it routes to the # PAPER path instead, which never touches the broker. Constructed once per cycle and # rehydrated from the orders table, so a per-cycle agent resumes its open positions. diff --git a/keel/cli.py b/keel/cli.py index d35f64db..922ef373 100644 --- a/keel/cli.py +++ b/keel/cli.py @@ -1,8 +1,8 @@ """keel command-line interface (P3 Task 9). Wires the merged Phase 1-3 modules into a `click` CLI: `db import` (`data.csv_import.import_dir`), -`monitor` (`data.market_feed`), `agent` (`agent.run_once`/`agent.loop`), `arm-bypass`/ -`disarm-bypass` (the `agent_state` bypass-arm token `agent.run_once` itself enforces, Issue #60), +`monitor` (`data.market_feed`), `agent` (`agent.run_once`/`agent.loop`), `autonomy on|off|show` +(the `profile` row `agent.run_once` itself re-reads each cycle), `rules list|backtest|promote|demote|disable|seed` (`data.repository` + `strategy.backtest`/ `promotion`; `seed` populates the otherwise-empty `rules` table from `agent.RULE_REGISTRY`, Issue #81), `pnl` (`analysis.pnl`), `kill`/`resume` (the `agent_state` kill-switch), @@ -814,11 +814,22 @@ def withdrawals_attest(ctx: click.Context, enabled: bool) -> None: taking physical possession whenever he desires". An asset we cannot withdraw is an asset we may not validly possess -- so rail 17 halts new ENTRIES when this is suspended or unknown. - Not passphrase-gated in either direction. `--suspended` only ever REDUCES capability, and - `--enabled` cannot itself place an order: it restores a precondition that every other rail, - the confirm gate and the bypass-arm token still sit in front of. + **Asymmetric, like `autonomy`.** `--suspended` only ever REDUCES capability and is ungated, + usable from anywhere. `--enabled` RELEASES a rail-17 entry halt, so it demands a typed `yes` + at a terminal exactly like `resume`/`resume-entries`/`record-flow`/`reset-hwm`. + + That gate used to be unnecessary for a reason that no longer holds: this command was + justified by "the confirm gate and the bypass-arm token still sit in front of it". The + bypass-arm token no longer exists, and with `keel autonomy on` the confirm gate is not there + either -- so without this, a cron line could clear a rail-17 halt and the next cycle would + place live orders with no human anywhere in the loop. """ repo = _open_repo(ctx) + if enabled: + _require_interactive_confirmation( + "attest withdrawals as ENABLED", + "This RELEASES rail 17's entry halt; the agent may place orders on its next cycle.", + ) now_ts = int(time.time()) repo.set_state("withdrawals_enabled", bool(enabled)) repo.set_state("withdrawals_attested_at", now_ts) @@ -1165,7 +1176,7 @@ def _interactive_confirm(preview: dict) -> bool: click.echo(f" {key}: {value}") else: click.echo(f" {preview!r}") - if not (sys.stdin is not None and sys.stdin.isatty()): + if not _is_interactive(): click.echo("no TTY -- declining (confirm mode fails closed).", err=True) return False return click.confirm("Place this order?", default=False) @@ -1249,23 +1260,40 @@ def autonomy_group() -> None: @autonomy_group.command("show") @click.pass_context +@with_disclaimer def autonomy_show(ctx: click.Context) -> None: """Print the current autonomy setting.""" profile = _open_repo(ctx).get_profile() + now_ts = int(time.time()) + live = profile.is_autonomous(now_ts) state = ( "ON -- orders are placed WITHOUT asking" - if profile.autonomous + 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) -> None: +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 @@ -1274,17 +1302,30 @@ def autonomy_on(ctx: click.Context) -> None: """ config = _load_cfg(ctx) repo = _open_repo(ctx) + now_ts = int(time.time()) + 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 " + f"Orders will be placed with NO further prompt, {window} " f"(mode={config.auto_trade.mode}, allowlist={config.allowlist}).", ) - repo.set_autonomous(True, int(time.time())) - click.echo("autonomy ON. Run `keel autonomy off` to require confirmation again.") + 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. @@ -2315,8 +2356,9 @@ def record_flow(ctx: click.Context, amount: str) -> None: mask a real trading drawdown, which is the one direction a circuit breaker must not fail in. """ _require_interactive_confirmation( - "rebase rail 11's high-water mark", - "A wrong amount here silently mis-states drawdown from now on.", + f"rebase rail 11's high-water mark by {amount}", + "A wrong amount or sign here silently mis-states drawdown from now on " + "(positive = deposit, negative = withdrawal).", ) try: parsed = Decimal(amount) diff --git a/keel/data/db.py b/keel/data/db.py index de29e971..3bee1bbb 100644 --- a/keel/data/db.py +++ b/keel/data/db.py @@ -242,6 +242,9 @@ CREATE TABLE IF NOT EXISTS profile ( id INTEGER PRIMARY KEY CHECK (id = 1), autonomous INTEGER NOT NULL DEFAULT 0, + -- NULL = no expiry (a durable choice). A timestamp makes autonomy LAPSE on its own, + -- restoring the time bound the removed bypass-arm token used to provide. + autonomous_until INTEGER, updated_ts INTEGER NOT NULL ) """, diff --git a/keel/data/repository.py b/keel/data/repository.py index f4358abe..9c124fba 100644 --- a/keel/data/repository.py +++ b/keel/data/repository.py @@ -589,25 +589,42 @@ def get_profile(self) -> Profile: `profile` table existed -- reports `autonomous=False`. The safe reading of "no record" is that the user never opted into unattended trading, never that they did. - Callers must re-read this per order decision rather than caching it, so that - `keel autonomy off` takes effect on the next order instead of the next restart. + Callers must re-read this each cycle rather than caching it, so that + `keel autonomy off` takes effect on the next cycle instead of the next restart. """ - row = self._conn.execute( - "SELECT autonomous, updated_ts FROM profile WHERE id = 1" - ).fetchone() + try: + row = self._conn.execute( + "SELECT autonomous, autonomous_until, updated_ts FROM profile WHERE id = 1" + ).fetchone() + except sqlite3.Error: + # A missing or damaged `profile` table must read as "no consent recorded", not + # propagate. The contract above promises fail-closed, so implement it rather than + # relying on the caller crashing before it reaches an order. + return Profile() if row is None: return Profile() - return Profile(autonomous=bool(row["autonomous"]), updated_ts=int(row["updated_ts"])) + until = row["autonomous_until"] + return Profile( + autonomous=bool(row["autonomous"]), + autonomous_until=None if until is None else int(until), + updated_ts=int(row["updated_ts"]), + ) + + def set_autonomous(self, value: bool, now_ts: int, expires_ts: int | None = None) -> None: + """Record the user's autonomy choice, upserting the single profile row. - def set_autonomous(self, value: bool, now_ts: int) -> None: - """Record the user's autonomy choice, upserting the single profile row.""" + `expires_ts=None` means the choice never lapses. Passing a timestamp makes autonomy + expire on its own -- the time bound the removed bypass-arm token used to enforce. + """ self._conn.execute( """ - INSERT INTO profile (id, autonomous, updated_ts) VALUES (1, ?, ?) + INSERT INTO profile (id, autonomous, autonomous_until, updated_ts) + VALUES (1, ?, ?, ?) ON CONFLICT(id) DO UPDATE SET autonomous = excluded.autonomous, + autonomous_until = excluded.autonomous_until, updated_ts = excluded.updated_ts """, - (1 if value else 0, now_ts), + (1 if value else 0, expires_ts, now_ts), ) self._conn.commit() diff --git a/keel/execution/executor.py b/keel/execution/executor.py index dccc3fa5..94fb4884 100644 --- a/keel/execution/executor.py +++ b/keel/execution/executor.py @@ -561,6 +561,9 @@ def _order_row(intent: OrderIntent, mode: str, now_ts: int) -> dict[str, Any]: expected_fill=intent.entry, actual_fill=None, raw_response=None, + # NOTE: rows written before 2026-07-21 carry `confirmation='bypass'` for what is now + # called `'autonomous'`. Nothing reads this column back -- it is an audit trail only -- + # so the rows are deliberately left as-written rather than rewritten by a migration. confirmation=mode, rule_id=None, created_at=now_ts, diff --git a/keel/templates/config.live.yaml b/keel/templates/config.live.yaml index 4cc7e9a0..05bb0c83 100644 --- a/keel/templates/config.live.yaml +++ b/keel/templates/config.live.yaml @@ -10,7 +10,6 @@ # 3. `caps` -- max_exposure_usd / max_per_asset_pct are your risk ceilings. # 4. `subscription` -- attest your venue tier (`keel subscription attest`); rail 14 caps spend. # 5. rules -- `keel init` seeds them as `candidate`; nothing trades until you promote. -# 6. `auto_trade.enabled` -- still false; set it true to start the scheduled loop. # # `allowlist` and `caps` are required and validated by keel.config.load_config; missing or # invalid values raise ConfigError naming the offending key rather than silently defaulting. @@ -47,6 +46,8 @@ market_data: auto_trade: # confirm = preview + explicit approval for every order (the safe live default). mode: confirm + # NOTE: currently UNUSED by any code path -- it is NOT a kill-switch and setting it + # true or false changes nothing. Use `keel kill` to halt trading. enabled: false interval_sec: 900 diff --git a/keel/templates/config.yaml b/keel/templates/config.yaml index 31778146..d85bf9e1 100644 --- a/keel/templates/config.yaml +++ b/keel/templates/config.yaml @@ -35,6 +35,8 @@ market_data: auto_trade: mode: paper + # NOTE: currently UNUSED by any code path -- it is NOT a kill-switch and setting it + # true or false changes nothing. Use `keel kill` to halt trading. enabled: false interval_sec: 900 diff --git a/packages/keel-core/keel_core/types.py b/packages/keel-core/keel_core/types.py index ed6fc17c..61907120 100644 --- a/packages/keel-core/keel_core/types.py +++ b/packages/keel-core/keel_core/types.py @@ -51,7 +51,23 @@ class Profile: """ autonomous: bool = False + #: `None` = the choice never lapses. Otherwise autonomy stops applying at this timestamp, + #: which is how the time bound of the removed bypass-arm token is preserved for anyone who + #: wants it -- a forgotten `autonomy on` need not grant unattended trading forever. + autonomous_until: int | None = None updated_ts: int = 0 + def is_autonomous(self, now_ts: int) -> bool: + """Whether autonomy actually applies at `now_ts`, honouring any expiry. + + Strict `now_ts < autonomous_until`, matching the freshness convention used throughout + this codebase: the instant the expiry is reached, autonomy is over. + """ + if not self.autonomous: + return False + if self.autonomous_until is None: + return True + return now_ts < self.autonomous_until + __all__ = ["Granularity", "Side", "Candle", "Profile"] diff --git a/tests/data/test_repository.py b/tests/data/test_repository.py index b6725434..ec942b97 100644 --- a/tests/data/test_repository.py +++ b/tests/data/test_repository.py @@ -412,3 +412,30 @@ def test_set_autonomous_upserts_and_never_creates_a_second_row(repo): repo.set_autonomous(ts % 2 == 0, now_ts=ts) rows = repo._conn.execute("SELECT COUNT(*) AS n FROM profile").fetchone()["n"] assert rows == 1 + + +def test_autonomy_can_carry_an_expiry_and_lapses_on_its_own(repo): + """The removed bypass-arm token was TIME-LIMITED so a forgotten arm could not grant + unattended trading forever. An unbounded profile flag loses that; an optional expiry + restores it without forcing it on a user who wants a durable choice.""" + repo.set_autonomous(True, now_ts=1000, expires_ts=2000) + p = repo.get_profile() + assert p.autonomous is True + assert p.autonomous_until == 2000 + + assert p.is_autonomous(now_ts=1999) is True + # strict now < expiry, like every other freshness check in this codebase + assert p.is_autonomous(now_ts=2000) is False + assert p.is_autonomous(now_ts=5000) is False + + +def test_autonomy_without_an_expiry_never_lapses(repo): + repo.set_autonomous(True, now_ts=1000) + p = repo.get_profile() + assert p.autonomous_until is None + assert p.is_autonomous(now_ts=10**12) is True + + +def test_autonomy_off_is_never_autonomous_whatever_the_expiry(repo): + repo.set_autonomous(False, now_ts=1000, expires_ts=10**12) + assert repo.get_profile().is_autonomous(now_ts=1001) is False diff --git a/tests/execution/test_executor.py b/tests/execution/test_executor.py index ffdd7592..293c00f0 100644 --- a/tests/execution/test_executor.py +++ b/tests/execution/test_executor.py @@ -2,7 +2,7 @@ `execute()` turns a `Signal` into a guarded live order: build an `OrderIntent` (sized via `execution.sizing`), run `guards.check` FIRST (un-overridable -- a violation must never reach -`preview_order`/`place_order`), preview it, honor confirm/bypass mode, place it, and log it to +`preview_order`/`place_order`), preview it, honor confirm/autonomous mode, place it, and log it to the `orders` table both before and after placement (a full audit trail even if the broker call fails). Every test here injects a **fake broker** (no network) -- `FakeBroker` below duck-types `CoinbaseClient.preview_order`/`.place_order` (+ an optional `cancel_order` for OCO) against @@ -438,7 +438,8 @@ def test_exit_signal_never_fetches_a_balance(repo): assert broker.get_accounts_calls == 0 -# -- bypass mode: compliant -> placed without a prompt -------------------------------------------- +# -- autonomous mode: compliant -> placed without a prompt +# -------------------------------------------- def test_bypass_mode_compliant_signal_places_without_confirm_fn(repo): @@ -461,7 +462,7 @@ def test_bypass_mode_ignores_confirm_fn_if_provided(repo): signal, broker, repo, _config(), mode="autonomous", confirm_fn=_reject, now_ts=NOW_TS ) - # bypass mode never consults confirm_fn -- a reject-everything fn must not block it. + # autonomous mode never consults confirm_fn -- a reject-everything fn must not block it. assert result.placed is True diff --git a/tests/execution/test_withdrawal_attestation.py b/tests/execution/test_withdrawal_attestation.py index 4e5e7cec..081738e9 100644 --- a/tests/execution/test_withdrawal_attestation.py +++ b/tests/execution/test_withdrawal_attestation.py @@ -65,15 +65,20 @@ def get_state(self, *a, **k): # -- CLI ----------------------------------------------------------------------- -def test_cli_roundtrip_and_suspension_message(tmp_path): +def test_cli_roundtrip_and_suspension_message(tmp_path, monkeypatch): db_path = tmp_path / "t.db" runner = CliRunner() + # `--enabled` RELEASES rail 17's entry halt, so it now demands a typed `yes` at a terminal + # (`--suspended` stays ungated -- it only ever reduces capability). + import keel.cli as cli_module + + monkeypatch.setattr(cli_module, "_is_interactive", lambda: True) unknown = runner.invoke(cli, ["--db", str(db_path), "withdrawals", "show"]) assert "UNKNOWN (never attested)" in unknown.output assert runner.invoke( - cli, ["--db", str(db_path), "withdrawals", "attest", "--enabled"] + cli, ["--db", str(db_path), "withdrawals", "attest", "--enabled"], input="yes\n" ).exit_code == 0 assert "ENABLED" in runner.invoke(cli, ["--db", str(db_path), "withdrawals", "show"]).output diff --git a/tests/test_cli.py b/tests/test_cli.py index 2be32d43..1dd4fc01 100644 --- a/tests/test_cli.py +++ b/tests/test_cli.py @@ -147,7 +147,6 @@ def test_agent_loop_bounded_by_max_cycles(tmp_path, valid_config_path, monkeypat assert result.output.count("skipped: kill_switch") == 3 -# -- arm-bypass / disarm-bypass (Issue #60, bypass-arm hardening) ------------------------------ @@ -1084,3 +1083,78 @@ def test_autonomy_ON_does_not_let_halt_commands_skip_confirmation(tmp_path, monk assert result.exit_code != 0, f"{args} was released without a human: {result.output}" assert "terminal" in result.output.lower() + + +def test_withdrawals_attest_ENABLED_needs_a_terminal(tmp_path, monkeypatch): + """Rail 17 halts ENTRIES when withdrawals are suspended/stale; `--enabled` releases that + halt, so it is a halt-releasing command like the other four. Its old justification was that + the confirm gate and the bypass-arm token sat in front of it -- with autonomy on, neither + does, so a cron line could re-permit live entries with no human anywhere.""" + _at_a_terminal(monkeypatch, yes=False) + db = tmp_path / "w.db" + repo = _repo_at(db) + repo.set_autonomous(True, now_ts=1) + + result = CliRunner().invoke( + cli, ["--db", str(db), "withdrawals", "attest", "--enabled"], input="yes\n" + ) + + assert result.exit_code != 0, result.output + assert "terminal" in result.output.lower() + assert _repo_at(db).get_state("withdrawals_enabled") is None + + +def test_withdrawals_attest_SUSPENDED_is_ungated_and_needs_no_terminal(tmp_path, monkeypatch): + """De-risking is never obstructed: suspending only ever halts entries.""" + _at_a_terminal(monkeypatch, yes=False) + db = tmp_path / "w2.db" + _repo_at(db) + + result = CliRunner().invoke(cli, ["--db", str(db), "withdrawals", "attest", "--suspended"]) + + assert result.exit_code == 0, result.output + assert _repo_at(db).get_state("withdrawals_enabled") is False + + +def test_withdrawals_attest_ENABLED_proceeds_on_a_typed_yes(tmp_path, monkeypatch): + _at_a_terminal(monkeypatch) + db = tmp_path / "w3.db" + _repo_at(db) + + result = CliRunner().invoke( + cli, ["--db", str(db), "withdrawals", "attest", "--enabled"], input="yes\n" + ) + + assert result.exit_code == 0, result.output + assert _repo_at(db).get_state("withdrawals_enabled") is True + + +def test_autonomy_on_for_hours_sets_an_expiry_that_lapses(tmp_path, monkeypatch, valid_config_path): + """A forgotten `autonomy on` should not be able to grant unattended trading forever.""" + _at_a_terminal(monkeypatch) + db = tmp_path / "exp.db" + _repo_at(db) + result = CliRunner().invoke( + cli, + ["--db", str(db), "--config", str(valid_config_path), "autonomy", "on", "--for-hours", "1"], + input="yes\n", + ) + assert result.exit_code == 0, result.output + profile = _repo_at(db).get_profile() + assert profile.autonomous_until is not None + assert profile.is_autonomous(profile.autonomous_until - 1) is True + assert profile.is_autonomous(profile.autonomous_until) is False + + +def test_autonomy_on_without_for_hours_warns_that_it_never_lapses( + tmp_path, monkeypatch, valid_config_path +): + _at_a_terminal(monkeypatch) + db = tmp_path / "noexp.db" + _repo_at(db) + result = CliRunner().invoke( + cli, ["--db", str(db), "--config", str(valid_config_path), "autonomy", "on"], input="yes\n" + ) + assert result.exit_code == 0, result.output + assert "NO expiry" in result.output + assert _repo_at(db).get_profile().autonomous_until is None From f03e3985c824df2b7515de47994fbeca68c045b6 Mon Sep 17 00:00:00 2001 From: Elmehdi Aitbrahim Date: Tue, 21 Jul 2026 19:38:23 -0400 Subject: [PATCH 8/9] fix(db): v8 migration for the autonomy expiry column, and review follow-ups Second review round. The expiry column was added to v7's CREATE TABLE IF NOT EXISTS, which does nothing for a database already stamped 7 -- it kept the old three-column table forever, so a recorded choice read back as OFF and 'keel autonomy off' (the DE-RISKING command) died on a missing column. Adds an idempotent v8 ALTER TABLE step. Reachable only on a developer database from this unmerged branch, but the command that reduces risk must never be the one that crashes. Also from the review: - end-to-end test that an EXPIRED profile yields mode=confirm out of run_once; the enforcement chain was previously only tested in pieces, so dropping the now_ts argument would have silently un-bounded autonomy again - get_profile logs when the profile is unreadable instead of swallowing it indistinguishably from 'the user never opted in' - --for-hours rejects 0/negative/inf/nan/overflow (0 previously wrote an already-lapsed row while printing 'autonomy ON until ...') - the halt-releasing inventory is FIVE commands, not four -- corrected in the CLI docstring and the runbook, which are what an operator actually reads - runbook: autonomy off binds on the next CYCLE, not the next order (up to 15 minutes at interval_sec 900) -- use 'keel kill' to stop immediately; and --for-hours is now documented where a supervised session is described Co-Authored-By: Claude Opus 4.8 (1M context) --- docs/go-live-runbook.md | 35 +++++++++++++++---- keel/cli.py | 16 +++++++-- keel/data/db.py | 20 ++++++++++- keel/data/repository.py | 11 ++++-- tests/data/test_db.py | 57 +++++++++++++++++++++++++++++-- tests/data/test_migrations.py | 2 +- tests/data/test_trade_outcomes.py | 4 +-- tests/test_agent.py | 24 +++++++++++++ tests/test_cli.py | 17 +++++++++ 9 files changed, 169 insertions(+), 17 deletions(-) diff --git a/docs/go-live-runbook.md b/docs/go-live-runbook.md index 372ff003..354787c7 100644 --- a/docs/go-live-runbook.md +++ b/docs/go-live-runbook.md @@ -114,9 +114,19 @@ keel resume # release it -- asks for a typed "yes" at a terminal The kill-switch is checked first on every cycle and **defaults to engaged**, so a damaged or unreadable state halts trading rather than permitting it. -Four commands re-permit trading after a halt — `resume`, `resume-entries`, `record-flow`, -`reset-hwm`. Each demands a typed `yes` from a terminal and **cannot be run from a script or cron -job**. That is deliberate: a breaker that can reset itself is not a breaker. +**Five** commands re-permit trading after a halt. Each demands a typed `yes` from a terminal and +**cannot be run from a script or cron job** — a breaker that can reset itself is not a breaker. + +| command | releases | +|---|---| +| `keel resume` | the kill-switch | +| `keel resume-entries` | a consecutive-loss halt (rail 16) | +| `keel record-flow --amount ±N` | rebases rail 11's high-water mark | +| `keel reset-hwm` | rail 11's high-water mark, clearing a stuck drawdown halt | +| `keel withdrawals attest --enabled` | rail 17's entry halt | + +The de-risking direction of each is always allowed and needs no terminal: `keel kill`, +`keel withdrawals attest --suspended`, `keel autonomy off`. ## 7. Only afterwards: autonomy @@ -129,9 +139,22 @@ keel autonomy off # always allowed, works anywhere, needs no terminal ``` Autonomy stops keel asking before each order. It changes **who is asked, never what is allowed** — -every hard rail still runs first, and it does **not** let the agent clear a safety halt. The -setting lives in your profile in the database and is re-read on every order, so `keel autonomy off` -takes effect on the **next order**, not the next restart. +every hard rail still runs first, and it does **not** let the agent clear a safety halt. + +The setting lives in your profile in the database and is re-read **at the start of every cycle**. +So `keel autonomy off` takes effect on the **next cycle**, not the next restart — but note that +with `interval_sec: 900` a cycle already in flight can still place orders for up to 15 minutes. +**If you need trading to stop immediately, use `keel kill`, not `autonomy off`.** + +**Prefer a time-bounded session:** + +```bash +keel autonomy on --for-hours 4 # lapses on its own; nothing to remember +``` + +Without `--for-hours` autonomy has **no expiry** and stays on until you turn it off — a forgotten +`autonomy on` will still be trading unattended weeks later. `keel autonomy show` tells you which +you have and how long is left. Turn it on only once you have watched several supervised cycles behave correctly. diff --git a/keel/cli.py b/keel/cli.py index 922ef373..9614ad60 100644 --- a/keel/cli.py +++ b/keel/cli.py @@ -13,8 +13,9 @@ Phase-4 `insights` stub. **Dangerous commands ask a human; nothing needs a stored secret.** The former scrypt passphrase -gate is gone (see `2026-07-21-security-simplification-design.md`). Four commands re-permit trading -after a safety halt -- `resume`, `resume-entries`, `record-flow` and `reset-hwm` -- and each +gate is gone (see `2026-07-21-security-simplification-design.md`). Five commands re-permit trading +after a safety halt -- `resume`, `resume-entries`, `record-flow`, `reset-hwm` and +`withdrawals attest --enabled` (rail 17) -- and each demands a typed `yes` via `_require_interactive_confirmation`, **failing closed off a TTY** so a script or cron job can never release a halt. `kill` (engaging the kill-switch) and `autonomy off` are *safe* actions -- they only ever reduce capability -- and are always allowed, from anywhere. @@ -93,6 +94,10 @@ "You are solely responsible for your own trading decisions." ) +#: 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 + DEFAULT_DB_PATH = "keel.db" DEFAULT_CONFIG_PATH = "config.yaml" @@ -1303,6 +1308,13 @@ def autonomy_on(ctx: click.Context, for_hours: float | None) -> None: config = _load_cfg(ctx) repo = _open_repo(ctx) now_ts = int(time.time()) + if for_hours is not None and not (0 < 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 greater than 0 and at most {_MAX_AUTONOMY_HOURS} " + f"({_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})" diff --git a/keel/data/db.py b/keel/data/db.py index 3bee1bbb..6d174236 100644 --- a/keel/data/db.py +++ b/keel/data/db.py @@ -19,7 +19,7 @@ from pathlib import Path from typing import Any -SCHEMA_VERSION = 7 +SCHEMA_VERSION = 8 # Creation order matters for readability (and for backends that validate FK targets eagerly); # SQLite itself only checks FK targets at DML time, but we still declare referenced tables first. @@ -378,6 +378,23 @@ def _migrate_v7_profile(conn: sqlite3.Connection) -> None: """ +def _migrate_v8_autonomy_expiry(conn: sqlite3.Connection) -> None: + """v8 adds `profile.autonomous_until` (NULL = the choice never lapses). + + This exists because the column was first added to v7's `CREATE TABLE IF NOT EXISTS`, which + silently does nothing for a database already stamped at 7 -- it kept the old three-column + table forever, so a recorded choice read back as "off" and `keel autonomy off` died on a + missing column. Only ever reachable on a developer database built from the unmerged branch, + but the de-risking command must never be the one that crashes. + + Idempotent: a database that got the column from the DDL (fresh, or upgraded from <=v6) is + left alone rather than hitting "duplicate column name". + """ + columns = {row["name"] for row in conn.execute("PRAGMA table_info(profile)")} + if "autonomous_until" not in columns: + conn.execute("ALTER TABLE profile ADD COLUMN autonomous_until INTEGER") + + _MIGRATIONS: dict[int, Callable[[sqlite3.Connection], None]] = { 2: _migrate_v2_broker_subscriptions, 3: _migrate_v3_trade_outcomes, @@ -385,6 +402,7 @@ def _migrate_v7_profile(conn: sqlite3.Connection) -> None: 5: _migrate_v5_candle_gap_probes, 6: _migrate_v6_asset_attestations, 7: _migrate_v7_profile, + 8: _migrate_v8_autonomy_expiry, } diff --git a/keel/data/repository.py b/keel/data/repository.py index 9c124fba..81e15dbb 100644 --- a/keel/data/repository.py +++ b/keel/data/repository.py @@ -8,6 +8,7 @@ from __future__ import annotations import json +import logging import sqlite3 import time from decimal import Decimal @@ -596,10 +597,14 @@ def get_profile(self) -> Profile: row = self._conn.execute( "SELECT autonomous, autonomous_until, updated_ts FROM profile WHERE id = 1" ).fetchone() - except sqlite3.Error: + except sqlite3.Error as exc: # A missing or damaged `profile` table must read as "no consent recorded", not - # propagate. The contract above promises fail-closed, so implement it rather than - # relying on the caller crashing before it reaches an order. + # propagate. But it must not be SILENT either: swallowing this indistinguishably + # from "the user never opted in" hides a broken migration or a corrupt database + # behind a reassuring `autonomy: off`. Fail closed AND say so. + logging.getLogger("keel").error( + "profile unreadable (%s: %s) -- treating autonomy as OFF", type(exc).__name__, exc + ) return Profile() if row is None: return Profile() diff --git a/tests/data/test_db.py b/tests/data/test_db.py index d1dedf7e..690c30d5 100644 --- a/tests/data/test_db.py +++ b/tests/data/test_db.py @@ -103,11 +103,11 @@ def test_agent_state_table_has_key_primary_key(): assert pk_columns == {"key"} -def test_schema_version_is_7(): +def test_schema_version_is_8(): """Deliberate tripwire: bump this literal consciously on every schema change.""" from keel.data.db import SCHEMA_VERSION - assert SCHEMA_VERSION == 7 + assert SCHEMA_VERSION == 8 def test_a_v6_database_migrates_up_and_gains_the_profile_table(tmp_path): @@ -126,3 +126,56 @@ def test_a_v6_database_migrates_up_and_gains_the_profile_table(tmp_path): "SELECT name FROM sqlite_master WHERE type='table' AND name='profile'" ).fetchone() assert named is not None, "v7 must add the profile table" + + +def test_a_v7_database_without_the_expiry_column_gains_it(tmp_path): + """Regression: `autonomous_until` was added to v7's CREATE TABLE, but a database already + stamped 7 runs no migration step and IF NOT EXISTS is a no-op -- so it kept the old table + forever, and `keel autonomy off` (the DE-RISKING command) died on a missing column.""" + import sqlite3 + + from keel.data.db import SCHEMA_VERSION, connect, migrate + + path = str(tmp_path / "v7.db") + conn = connect(path) + migrate(conn) + # Recreate the pre-fix v7 shape: profile without the expiry column, stamped at 7. + conn.execute("DROP TABLE profile") + conn.execute( + "CREATE TABLE profile (id INTEGER PRIMARY KEY CHECK (id = 1), " + "autonomous INTEGER NOT NULL DEFAULT 0, updated_ts INTEGER NOT NULL)" + ) + conn.execute("INSERT INTO profile (id, autonomous, updated_ts) VALUES (1, 1, 5)") + conn.execute("UPDATE schema_version SET version = 7") + conn.commit() + + migrate(conn) + + cols = [r["name"] for r in conn.execute("PRAGMA table_info(profile)")] + assert "autonomous_until" in cols, "v8 must add the expiry column to an existing v7 table" + assert int(conn.execute("SELECT version FROM schema_version").fetchone()["version"]) == ( + SCHEMA_VERSION + ) + # the recorded choice survives, and writing to it no longer raises + from keel.data.repository import Repository + + repo = Repository(conn) + assert repo.get_profile().autonomous is True + repo.set_autonomous(False, now_ts=9) # must not raise sqlite3.OperationalError + assert repo.get_profile().autonomous is False + assert isinstance(conn, sqlite3.Connection) + + +def test_migrating_a_v6_database_twice_is_idempotent(tmp_path): + """v8's ALTER must not fire twice on a DB that already got the column from the DDL.""" + from keel.data.db import SCHEMA_VERSION, connect, migrate + + conn = connect(str(tmp_path / "v6.db")) + migrate(conn) + conn.execute("UPDATE schema_version SET version = 6") + conn.commit() + migrate(conn) + migrate(conn) # must not raise "duplicate column name" + assert int(conn.execute("SELECT version FROM schema_version").fetchone()["version"]) == ( + SCHEMA_VERSION + ) diff --git a/tests/data/test_migrations.py b/tests/data/test_migrations.py index a1a3053d..e5d8e29d 100644 --- a/tests/data/test_migrations.py +++ b/tests/data/test_migrations.py @@ -46,7 +46,7 @@ def test_fresh_database_is_stamped_at_the_current_version() -> None: conn = db.connect(":memory:") db.migrate(conn) version = conn.execute("SELECT version FROM schema_version").fetchone()["version"] - assert version == db.SCHEMA_VERSION == 7 + assert version == db.SCHEMA_VERSION == 8 def test_fresh_database_gets_no_subscription_row() -> None: diff --git a/tests/data/test_trade_outcomes.py b/tests/data/test_trade_outcomes.py index b1cb1558..22075335 100644 --- a/tests/data/test_trade_outcomes.py +++ b/tests/data/test_trade_outcomes.py @@ -35,11 +35,11 @@ def _outcome(**overrides: object) -> dict: return base -def test_schema_is_at_version_7() -> None: +def test_schema_is_at_version_8() -> None: conn = db.connect(":memory:") db.migrate(conn) version = conn.execute("SELECT version FROM schema_version").fetchone()["version"] - assert version == db.SCHEMA_VERSION == 7 + assert version == db.SCHEMA_VERSION == 8 def test_fresh_database_has_no_outcomes() -> None: diff --git a/tests/test_agent.py b/tests/test_agent.py index 6c4f5a7d..6d8c517f 100644 --- a/tests/test_agent.py +++ b/tests/test_agent.py @@ -1211,3 +1211,27 @@ def _fake_run_once(broker, repo_arg, config, now_ts, confirm_fn=None): assert result.exit_code == 0, result.output assert captured["mode"] == "confirm" assert captured["confirm_fn"] is cli_module._interactive_confirm + + +def test_an_EXPIRED_autonomy_falls_back_to_confirm_and_places_nothing(repo): + """End-to-end guard on the enforcement chain run_once -> _effective_mode -> is_autonomous. + Without this, dropping the now_ts argument would silently un-bound autonomy again.""" + repo.set_autonomous(True, now_ts=1_000, expires_ts=50_000) + repo.insert_rule("dca", {"product_id": PRODUCT}, status="live") + broker = FakeBroker(series={(PRODUCT, Granularity.ONE_DAY): [_candle(0, "100")]}) + + result = run_once(broker, repo, _config(), now_ts=90_000) # well past the expiry + + assert result.mode == "confirm", "expired autonomy must fall back to asking a human" + assert broker.place_calls == [] + + +def test_autonomy_still_applies_strictly_before_its_expiry(repo): + repo.set_autonomous(True, now_ts=1_000, expires_ts=90_001) + repo.insert_rule("dca", {"product_id": PRODUCT}, status="live") + broker = FakeBroker(series={(PRODUCT, Granularity.ONE_DAY): [_candle(0, "100")]}) + + result = run_once(broker, repo, _config(), now_ts=90_000) + + assert result.mode == "autonomous" + assert len(broker.place_calls) == 1 diff --git a/tests/test_cli.py b/tests/test_cli.py index 1dd4fc01..8e137014 100644 --- a/tests/test_cli.py +++ b/tests/test_cli.py @@ -1158,3 +1158,20 @@ def test_autonomy_on_without_for_hours_warns_that_it_never_lapses( assert result.exit_code == 0, result.output assert "NO expiry" in result.output assert _repo_at(db).get_profile().autonomous_until is None + + +def test_autonomy_on_rejects_a_nonsensical_for_hours(tmp_path, monkeypatch, valid_config_path): + """0/negative would write an already-lapsed row while claiming 'autonomy ON until ...'; + inf/nan/huge would overflow int() AFTER the operator had already typed yes.""" + _at_a_terminal(monkeypatch) + for bad in ("0", "-5", "inf", "nan", "1e18"): + db = tmp_path / f"bad-{bad}.db" + _repo_at(db) + result = CliRunner().invoke( + cli, + ["--db", str(db), "--config", str(valid_config_path), + "autonomy", "on", "--for-hours", bad], + input="yes\n", + ) + assert result.exit_code != 0, f"--for-hours {bad} should be rejected: {result.output}" + assert _repo_at(db).get_profile().autonomous is False From 26963d123ab1c0f0cb491e062c919f1bcc76fa60 Mon Sep 17 00:00:00 2001 From: Elmehdi Aitbrahim Date: Tue, 21 Jul 2026 19:44:42 -0400 Subject: [PATCH 9/9] fix: reject sub-second autonomy windows; say 'next cycle', not 'next order' Third review round, all non-blocking: - --for-hours 1e-9 rounded to a zero-length window: it stored an already-lapsed row while printing 'autonomy ON until ...'. Same misleading message the last round closed for 0, just past the boundary. Minimum is now one second. - Four code docstrings still promised 'autonomy off takes effect on the next ORDER'; it is the next CYCLE, and agent.py contradicted itself in a single sentence. This is the one operational fact that matters when someone is trying to stop live trading, so they now also point at 'keel kill'. - Repository.profile_readable() lets 'autonomy show' tell a human the stored setting is UNKNOWN rather than printing a reassuring 'off'. Note that _open_repo's migrate() heals a merely missing table, so this covers damage migrate cannot -- the test lives at repository level for that reason. - logging.getLogger(__name__) per codebase convention; stale section banner. Co-Authored-By: Claude Opus 4.8 (1M context) --- keel/agent.py | 3 ++- keel/cli.py | 24 ++++++++++++++++++------ keel/data/repository.py | 15 ++++++++++++++- packages/keel-core/keel_core/types.py | 4 ++-- tests/data/test_repository.py | 13 +++++++++++++ tests/test_cli.py | 2 ++ 6 files changed, 51 insertions(+), 10 deletions(-) diff --git a/keel/agent.py b/keel/agent.py index 012219d6..d1e78c47 100644 --- a/keel/agent.py +++ b/keel/agent.py @@ -42,7 +42,8 @@ only when `config.auto_trade.mode == "confirm"` **and** `repo.get_profile().is_autonomous(now_ts)` is true. The profile is read fresh every cycle and never cached, so `keel autonomy off` takes effect on -the next order rather than the next restart. The check lives inside `run_once`, not only at the +the next cycle rather than the next restart (a cycle in flight can still place; `keel kill` is +what stops trading immediately). The check lives inside `run_once`, not only at the CLI, so an in-process caller cannot obtain autonomy the CLI would have refused; an absent or unreadable profile row reads as not-autonomous. In every mode, `guards.check` runs FIRST and is un-overridable -- autonomy changes who is asked, never what is allowed. diff --git a/keel/cli.py b/keel/cli.py index 9614ad60..365f0ccf 100644 --- a/keel/cli.py +++ b/keel/cli.py @@ -25,7 +25,7 @@ **Autonomy is a profile choice, checked in-process.** `keel autonomy on` (typed `yes`, TTY required) sets `profile.autonomous`; `agent.run_once` re-reads it every cycle via `_effective_mode`, so the check cannot be skipped by a caller driving `run_once` in-process, and -turning autonomy off binds on the next order. It changes who is asked, never what is allowed: +turning autonomy off binds on the next cycle. It changes who is asked, never what is allowed: `guards.check` runs first in every mode, and autonomy never releases a halt. **No interactive hangs in tests.** `_is_interactive()` is the single TTY predicate, with @@ -97,6 +97,9 @@ #: 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 DEFAULT_DB_PATH = "keel.db" DEFAULT_CONFIG_PATH = "config.yaml" @@ -129,7 +132,7 @@ def wrapper(*args: Any, **kwargs: Any) -> Any: return wrapper -# -- passphrase resolution (no interactive hangs under CliRunner) ----------------------------- +# -- interactive confirmation (no interactive hangs under CliRunner) ----------------------------- def _is_interactive() -> bool: @@ -1268,7 +1271,16 @@ def autonomy_group() -> None: @with_disclaimer def autonomy_show(ctx: click.Context) -> None: """Print the current autonomy setting.""" - profile = _open_repo(ctx).get_profile() + 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 = ( @@ -1308,12 +1320,12 @@ def autonomy_on(ctx: click.Context, for_hours: float | None) -> None: config = _load_cfg(ctx) repo = _open_repo(ctx) now_ts = int(time.time()) - if for_hours is not None and not (0 < for_hours <= _MAX_AUTONOMY_HOURS): + 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 greater than 0 and at most {_MAX_AUTONOMY_HOURS} " - f"({_MAX_AUTONOMY_HOURS // 24} days); got {for_hours!r}." + 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 = ( diff --git a/keel/data/repository.py b/keel/data/repository.py index 81e15dbb..26d9b669 100644 --- a/keel/data/repository.py +++ b/keel/data/repository.py @@ -602,7 +602,7 @@ def get_profile(self) -> Profile: # propagate. But it must not be SILENT either: swallowing this indistinguishably # from "the user never opted in" hides a broken migration or a corrupt database # behind a reassuring `autonomy: off`. Fail closed AND say so. - logging.getLogger("keel").error( + logging.getLogger(__name__).error( "profile unreadable (%s: %s) -- treating autonomy as OFF", type(exc).__name__, exc ) return Profile() @@ -615,6 +615,19 @@ def get_profile(self) -> Profile: updated_ts=int(row["updated_ts"]), ) + def profile_readable(self) -> bool: + """Whether the `profile` row could actually be read. + + `get_profile()` fails closed on a damaged table, which is right for the trading path but + indistinguishable from "the user never opted in". This lets a caller tell a human that + the stored setting is UNKNOWN rather than confidently reporting it as off. + """ + try: + self._conn.execute("SELECT autonomous FROM profile WHERE id = 1").fetchone() + except sqlite3.Error: + return False + return True + def set_autonomous(self, value: bool, now_ts: int, expires_ts: int | None = None) -> None: """Record the user's autonomy choice, upserting the single profile row. diff --git a/packages/keel-core/keel_core/types.py b/packages/keel-core/keel_core/types.py index 61907120..fb5e95a7 100644 --- a/packages/keel-core/keel_core/types.py +++ b/packages/keel-core/keel_core/types.py @@ -46,8 +46,8 @@ class Profile: """The user's own settings, as opposed to operational state or file configuration. `autonomous` is the single choice today: when true, the agent places rule-generated orders - without asking. It is stored in the database (not `config.yaml`) and re-read on every order - decision, so turning it off takes effect on the NEXT order rather than the next restart. + without asking. It is stored in the database (not `config.yaml`) and re-read once per + cycle, so turning it off takes effect on the NEXT cycle rather than the next restart. """ autonomous: bool = False diff --git a/tests/data/test_repository.py b/tests/data/test_repository.py index ec942b97..1cd7d963 100644 --- a/tests/data/test_repository.py +++ b/tests/data/test_repository.py @@ -439,3 +439,16 @@ def test_autonomy_without_an_expiry_never_lapses(repo): def test_autonomy_off_is_never_autonomous_whatever_the_expiry(repo): repo.set_autonomous(False, now_ts=1000, expires_ts=10**12) assert repo.get_profile().is_autonomous(now_ts=1001) is False + + +def test_profile_readable_reports_damage_that_get_profile_hides(repo): + """`get_profile` fails closed on a damaged table, which is right for the trading path but + indistinguishable from "never opted in". `profile_readable` is how a caller tells a human + the stored setting is UNKNOWN rather than confidently reporting it off.""" + assert repo.profile_readable() is True + + repo._conn.execute("DROP TABLE profile") + repo._conn.commit() + + assert repo.profile_readable() is False + assert repo.get_profile().autonomous is False # still fails closed, still no exception diff --git a/tests/test_cli.py b/tests/test_cli.py index 8e137014..cca1b64a 100644 --- a/tests/test_cli.py +++ b/tests/test_cli.py @@ -1175,3 +1175,5 @@ def test_autonomy_on_rejects_a_nonsensical_for_hours(tmp_path, monkeypatch, vali ) assert result.exit_code != 0, f"--for-hours {bad} should be rejected: {result.output}" assert _repo_at(db).get_profile().autonomous is False + +