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." + )