From b240e5f2d7fdba1499f18e7c282ec8e14f266cef Mon Sep 17 00:00:00 2001 From: Elmehdi Aitbrahim Date: Wed, 2 Sep 2026 18:41:36 -0400 Subject: [PATCH] fix(doctor,docs): the documented path out of a halt must actually end the halt (#693, #694) Two defects, both found by following keel's own instructions during the paper-equities bootstrap, and both of which cost real operator time. #693 -- `doctor`'s `rail.kill_switch` finding offered `keel autonomy on` as the fix. That command cannot clear a kill switch; its own docstring says so. They are separate gates on purpose: autonomy decides who gets ASKED, the kill switch decides whether the agent runs at all. An operator who followed the fix line ended up in the worst of the two states -- a halted agent authorised to trade unattended. The fix line now reads `keel resume`. The old assertion in test_doctor.py pinned `keel autonomy on`, so the defect had a test defending it. It is restated here, with a comment saying what it got wrong. #694 -- the runbook's paper-equities and paper-hourly bootstraps ran `migrate`, seeded rules and fetched candles, then stopped. `migrate` creates schema and never seeds, so a fresh database has no `kill_switch` row and `get_state("kill_switch", default=True)` fails closed. Both profiles logged `skipped: kill_switch` every cycle until someone noticed. Each bootstrap now names the arming step immediately after its own fetch block, deliberately OUTSIDE the copy-pasteable fence -- same gate as `keel autonomy on` and the rail-17 release: a human at a terminal, never a scheduled job. Tests (written first, red before green): tests/commands/test_doctor_fix_lines.py -- 2 tests: every fix line names a real command, and the kill-switch fix names `keel resume` specifically tests/test_bootstrap_arms_the_profile.py -- 3 tests: both bootstrap sections exist, each names the arming step, and the step is NOT inside a copy-pasteable block Mutation-verified, four mutants, all killed: fix line reverted to `autonomy on`; fix line naming a nonexistent command; the equities arming step deleted; the arming step folded into the fence. Closes #693 Closes #694 Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01NzuKAe2RVrPt9acVAWjRyL --- docs/operator-runbook.md | 26 +++++++ keel/commands/doctor.py | 8 ++- tests/commands/test_doctor.py | 7 +- tests/commands/test_doctor_fix_lines.py | 84 ++++++++++++++++++++++ tests/test_bootstrap_arms_the_profile.py | 90 ++++++++++++++++++++++++ 5 files changed, 212 insertions(+), 3 deletions(-) create mode 100644 tests/commands/test_doctor_fix_lines.py create mode 100644 tests/test_bootstrap_arms_the_profile.py diff --git a/docs/operator-runbook.md b/docs/operator-runbook.md index d80b10e..9aab0a0 100644 --- a/docs/operator-runbook.md +++ b/docs/operator-runbook.md @@ -505,6 +505,19 @@ keel --config config.paper-hourly.yaml --db keel-paperhourly.db rules promote -- keel --config config.paper-hourly.yaml --db keel-paperhourly.db fetch ``` +**Arm it.** `keel migrate` creates schema and never seeds, so a fresh database has no +`kill_switch` row and `get_state("kill_switch", default=True)` fails closed — the profile logs +`skipped: kill_switch` on every cycle until this is run: + +> ⚠️ **At a terminal, by a human, deliberately.** Same gate as `keel autonomy on` and the rail-17 +> release: a scheduled job must never start a halted agent. Do not fold it into the block above, +> do not script it, do not pipe a `yes` into it. `keel resume` (with this profile's +> `--config`/`--db`) disengages the kill switch; `keel autonomy on` is a SEPARATE control, +> deciding who gets asked rather than whether the agent runs. Neither substitutes for the other — +> running only `autonomy on` leaves a halted agent authorised to trade unattended, which is worse +> than either state alone (#693). + + **The 2026-08-17 expansion (#351): 8 → 19 assets.** The 11 additions above — ZEC, NEAR, AVAX, UNI, FET, ICP, DOT, CRV, ALGO, BCH, DOGE — each passed a 15-minute data-health screen over 90 days (coverage ≥ 95.98%, zero zero-volume bars; results recorded in the issue), and each sits at @@ -622,6 +635,19 @@ Alpaca paper credentials); the steps, once you have them: keel --config config.paper-equities.yaml --db keel-equities.db fetch ``` +**Arm it.** `keel migrate` creates schema and never seeds, so a fresh database has no +`kill_switch` row and `get_state("kill_switch", default=True)` fails closed — the profile logs +`skipped: kill_switch` on every cycle until this is run: + +> ⚠️ **At a terminal, by a human, deliberately.** Same gate as `keel autonomy on` and the rail-17 +> release: a scheduled job must never start a halted agent. Do not fold it into the block above, +> do not script it, do not pipe a `yes` into it. `keel resume` (with this profile's +> `--config`/`--db`) disengages the kill switch; `keel autonomy on` is a SEPARATE control, +> deciding who gets asked rather than whether the agent runs. Neither substitutes for the other — +> running only `autonomy on` leaves a halted agent authorised to trade unattended, which is worse +> than either state alone (#693). + + The `rules add` form (explicit per-symbol rows, granularity stated even though ONE_DAY is the constructor default) mirrors the hourly bootstrap so the clock each row trades is visible in the row itself. `--force` is the documented bypass for a rule whose backtest diff --git a/keel/commands/doctor.py b/keel/commands/doctor.py index 9df4c8f..45ec569 100644 --- a/keel/commands/doctor.py +++ b/keel/commands/doctor.py @@ -306,7 +306,13 @@ def rail_state_findings( HALTED, "kill switch engaged", "every entry is vetoed; this is a correct state, not a fault", - "keel autonomy on", + # `keel resume`, NOT `keel autonomy on` (#693). They are separate gates on + # purpose -- autonomy is who gets ASKED, the kill switch is whether the agent + # runs at all -- and `autonomy_on` says in its own docstring that it cannot + # release a safety halt. The old line sent an operator to type a confirmation + # for unattended order placement and leave the halt in force, which is strictly + # worse than either state alone. + "keel resume", ) ) else: diff --git a/tests/commands/test_doctor.py b/tests/commands/test_doctor.py index 452701b..7bb49e9 100644 --- a/tests/commands/test_doctor.py +++ b/tests/commands/test_doctor.py @@ -232,7 +232,10 @@ def test_kill_switch_is_halted_not_broken() -> None: ) (kill,) = [f for f in findings if f.name == "rail.kill_switch"] assert kill.status == "halted" - assert "keel autonomy on" in kill.fix + # `keel resume`, not `keel autonomy on` -- this assertion pinned the wrong command until + # #693, and an operator who followed it got a still-halted agent authorised to trade + # unattended. See tests/commands/test_doctor_fix_lines.py for the standing pin. + assert "keel resume" in kill.fix def test_streak_halts_expire_on_their_own() -> None: @@ -301,7 +304,7 @@ def test_quiet_veto_log_is_ok() -> None: def test_exit_code_fails_only_on_real_faults() -> None: - halted = Finding("rail.kill_switch", "halted", "engaged", "deliberate", "keel autonomy on") + halted = Finding("rail.kill_switch", "halted", "engaged", "deliberate", "keel resume") ok = Finding("install.versions", "ok", "aligned", "six of six", "-") assert doctor_exit_code([halted, ok]) == 0 broken = Finding("attest.withdrawals", "fail", "expired", "9 days over", "attest") diff --git a/tests/commands/test_doctor_fix_lines.py b/tests/commands/test_doctor_fix_lines.py new file mode 100644 index 0000000..e00ffb1 --- /dev/null +++ b/tests/commands/test_doctor_fix_lines.py @@ -0,0 +1,84 @@ +"""Every `doctor` fix line must name a command that can produce the state it promises. + +`doctor` is what an operator runs when something is already wrong, and its `fix` field is the +one line they will act on without checking. A fix that names the wrong command does not merely +fail to help — it spends the operator's trust and their time, and on this codebase it can spend +a typed confirmation for a dangerous capability too. + +That is not hypothetical (#693). `rail.kill_switch` told the operator to run `keel autonomy on`, +which cannot clear the kill switch and says so in its own docstring. Following it meant typing +`yes` to unattended order placement and remaining halted, with nothing indicating the two were +different gates. +""" + +from __future__ import annotations + +import re +from pathlib import Path + +from keel.cli import cli +from keel.commands.doctor import rail_state_findings + +_ROOT = Path(__file__).resolve().parents[2] +_DOCTOR = _ROOT / "keel/commands/doctor.py" + +#: `keel ` as written inside a fix string. Stops at anything that is not part of a +#: command path, so `keel fetch --repair-gaps` yields `fetch` and `keel scope attest --trading` +#: yields `scope attest`. +_INVOCATION = re.compile(r'"keel ((?:[a-z][a-z-]*)(?: [a-z][a-z-]*)?)') + + +def _resolves(path: str) -> bool: + """Whether `path` (e.g. `"scope attest"`) is a real command in the CLI tree.""" + command = cli + for part in path.split(): + commands = getattr(command, "commands", None) + if not commands or part not in commands: + return False + command = commands[part] + return True + + +def test_every_command_named_in_a_fix_line_exists() -> None: + """A renamed or misremembered command in a fix line is unreachable advice. + + Scanned out of the source rather than by rendering findings, because most fix lines only + appear on the failing branch — a rendering-based scan would check the handful of states a + test happens to construct and miss the rest. + """ + named = sorted(set(_INVOCATION.findall(_DOCTOR.read_text(encoding="utf-8")))) + assert named, "no `keel ...` invocations found in doctor.py -- has the fix format changed?" + + missing = [path for path in named if not _resolves(path)] + assert not missing, ( + f"doctor names {len(missing)} command(s) that do not exist: {missing}. An operator " + "following that advice gets `No such command`." + ) + + +def test_the_kill_switch_fix_names_the_command_that_clears_it() -> None: + """**The specific failure this file was written for (#693).** + + Autonomy and the kill switch are deliberately separate controls — *who gets asked* versus + *whether the agent runs at all* — and the separation is load-bearing. A fix line that + conflates them teaches the operator they are one thing, which is the opposite of the design. + + Asserted against `rail_state_findings`' rendered output, not against the source, so it holds + whatever the string is spelled like. + """ + (finding,) = [ + f + for f in rail_state_findings( + kill_switch=True, streak_halt_until=0, drawdown_total=0, now_ts=0 + ) + if f.name == "rail.kill_switch" + ] + + assert "keel resume" in finding.fix, ( + f"the kill-switch fix says {finding.fix!r}. Only `keel resume` " + "(`trading.disengage_kill_switch`) clears it; `keel autonomy on` provably cannot, and " + "following it costs a typed confirmation for unattended trading while staying halted" + ) + assert "autonomy" not in finding.fix, ( + "naming `autonomy` here re-conflates the two gates the design keeps apart" + ) diff --git a/tests/test_bootstrap_arms_the_profile.py b/tests/test_bootstrap_arms_the_profile.py new file mode 100644 index 0000000..53062ad --- /dev/null +++ b/tests/test_bootstrap_arms_the_profile.py @@ -0,0 +1,90 @@ +"""A documented bootstrap that creates a database must also name the step that arms it (#694). + +`keel migrate` creates schema and **never seeds** — correct, and documented. So a fresh database +has no `kill_switch` row, `get_state("kill_switch", default=True)` fails closed, and the profile +skips every cycle until someone runs `keel resume`. + +Followed exactly on 2026-09-02, the equities bootstrap produced five promoted rules, 1249 cached +daily bars per symbol, and three consecutive `skipped: kill_switch` cycles. Nothing was broken; +the page was incomplete, and the failure looks like a broken profile rather than an unset flag. + +The arming step is deliberately NOT part of the copy-pasteable block: the runbook's own warning +is that releasing a halt must stay a human gesture and must never be scriptable. So this checks +that the step is *named* in the section, not that it sits inside the code fence. +""" + +from __future__ import annotations + +import re +from pathlib import Path + +_ROOT = Path(__file__).resolve().parent.parent +_RUNBOOK = _ROOT / "docs/operator-runbook.md" + +#: Sections that document standing up a profile from nothing. Keyed by their heading so a +#: failure names the section an operator would have been reading. +_BOOTSTRAP_SECTIONS = ( + "## The hourly evidence profile (paper-hourly)", + "## The equities paper profile (paper-equities)", +) + + +def _section(heading: str) -> str: + text = _RUNBOOK.read_text(encoding="utf-8") + start = text.index(heading) + following = [ + m.start() + for m in re.finditer(r"(?m)^## ", text) + if m.start() > start + ] + return text[start : following[0] if following else len(text)] + + +def test_the_bootstrap_sections_exist() -> None: + """A guard on the guard: a renamed heading would make every assertion below vacuous.""" + for heading in _BOOTSTRAP_SECTIONS: + assert heading in _RUNBOOK.read_text(encoding="utf-8"), f"missing section: {heading}" + + +def test_every_bootstrap_that_migrates_a_database_names_the_arming_step() -> None: + """`keel migrate` never seeds, so a bootstrapped profile is halted until `keel resume`. + + Checked per section rather than repository-wide: a mention of `resume` three thousand lines + away is not something the operator following these steps will see. + """ + missing = [] + for heading in _BOOTSTRAP_SECTIONS: + body = _section(heading) + if "keel migrate" not in body: + continue # this section does not create a database; nothing to arm + # `keel resume`, not the WORD "resume". The hourly section passed the first version of + # this test on the prose "must already be in force when live BUYs resume" -- a match + # that has nothing to do with arming a profile, in a section that had the same gap. + if "keel resume" not in body: + missing.append(heading) + + assert not missing, ( + "these bootstrap sections create a database with `keel migrate` and never name the step " + "that arms it, so following them exactly yields a profile that skips every cycle:\n " + + "\n ".join(missing) + ) + + +def test_the_arming_step_is_not_inside_a_copy_pasteable_block() -> None: + """The runbook's own constraint, and the reason this is a doc fix rather than a script fix. + + > The typed confirmation is deliberately human … so that a scheduled job can never release a + > §65.4 halt. Do not script this command and do not pipe a `yes` into it. + + Putting `keel resume` inside the fenced block above it would make it look like one more line + to paste — which is exactly what that warning forbids. + """ + for heading in _BOOTSTRAP_SECTIONS: + body = _section(heading) + if "keel migrate" not in body or "resume" not in body: + continue + fenced = "".join(re.findall(r"```.*?```", body, re.S)) + assert "keel resume" not in fenced and "resume\n" not in fenced, ( + f"{heading}: the arming step sits inside a copy-pasteable block. The runbook " + "requires it stay a deliberate human gesture, not one more line to paste." + )