Skip to content

Commit ada303c

Browse files
eaitbrahimclaude
andauthored
fix(web): drop reset-hwm to Tier 2, and bind every phrase to its own action (#790) (#791)
* fix(web): drop reset-hwm to Tier 2, and bind every phrase to its own action (#790) Eleven findings on the gate table. Three were about what Tier 1 should CONTAIN rather than how it was written, and the first is the one that mattered. ── THE TIER BOUNDARY ARGUED AGAINST ITSELF ──────────────────────────────────── `reset_high_water_mark` is four writes: it clears the mark, zeroes BOTH drawdown scalars and empties `equity_history` -- the rolling window the weekly peak is computed from. `record_declared_flow` shifts the mark by a signed amount the operator states. So Tier 1 admitted the action that ERASES the drawdown evidence while Tier 2 refused the one that adjusts it, on the stated grounds that the adjusting one "can, with the wrong sign, mask a real trading drawdown". Dropped to Tier 2. The alternative -- keeping it and rewording the criterion -- would have been fitting the rule to the answer. Tier 1 is two actions now: `resume` and `resume-entries`. `resume`'s own entry says what it really is. `guards.check` reads `repo.get_state("kill_switch", default=True)`, so an unset kill-switch is ENGAGED: on a never-resumed deployment this moves a fail-closed default to permissive rather than releasing a brake a rail applied. It stays, because `keel kill` re-engages in one gesture, and that is what bounds it. And the ordering is written down: the browser has no halt today, so a release route shipping before one would let an operator start from a phone and not stop from it. Stage 2b ships the ungated one-click halt in the SAME change as the first release route, not after it. ── A PHRASE THAT RELEASED EVERY ACTION ──────────────────────────────────────── Every test read the expected phrase back out of the table, so the suite was self-consistent under any content. A `phrase_matches` that ignored `action_key` and tested the presented phrase against every row passed all twelve -- making any Tier 1 phrase a master key, which is the closure this module calls its safety property, defeated. Nothing pinned the phrase literals either: `RESUME TRADING` -> `RESUME NOW` shipped green, as did swapping two actions' phrases. Bound now, and asserted as a cross product: every phrase against every OTHER action. The literals are pinned, and no two phrases may share a FIRST WORD -- the near miss is an operator pattern-matching under stress, for whom a future `RESUME ENTRIES` beside `RESUME TRADING` would be genuinely confusable. ── AND THE SAME BUG I HAD FIXED ONE PR EARLIER ──────────────────────────────── `secrets.compare_digest` raises `TypeError` on a non-ASCII `str` and on a non-`str`. I found and fixed exactly this in `tokens_match` during #786's review, then wrote the same pattern here without the guard. The phrase comes off an operator's keyboard through a JSON body, so a smart quote or a non-breaking space from a phone delivered the correct verdict -- refuse -- as a 500. ── THE REST ─────────────────────────────────────────────────────────────────── `function` named a `click.Command`. `getattr(keel.cli, "resume")()` would enter Click standalone mode, re-parse the SERVER's `sys.argv`, run the TTY gate with no terminal and `sys.exit` inside a request thread. It is `operation` now, naming the state service module-qualified, pinned by literal and checked to resolve to something callable -- and two actions may not reach one operation, which would release the kill-switch when the operator asked to clear a streak halt. `TIER1_ACTIONS` is a `MappingProxyType`; the rows were already frozen and that is asserted rather than assumed. `action` and `detail` are IMPORTED from `keel.commands.trading` rather than paraphrased. That module holds the wording so "neither re-words them", and the drift was already visible: the CLI printed "Any real, unrecovered drawdown stops being visible to the rail" where this module said "re-seeding the high-water mark". A test asserts both strings are the module's own. All seven non-Tier-1 actions are named individually now; `scope-attest` and `posture-attest` were missing, so either could have been added for free. And the module no longer describes wiring that does not exist in the present tense. ── TWO TESTS THAT WERE THEATRE ──────────────────────────────────────────────── The timing test grepped the source for `compare_digest`; a mutant replaced the call and left the word in a trailing comment. `==` and `compare_digest` return the same values for every input, so no black-box test can tell them apart -- it reads the AST now and requires the RETURNED value to be that call. Writing it turned up the same class of bug in my own test: `ast.walk` is breadth-first, so `returns[-1]` is not the last return in the source. It passed by luck and failed the moment an import edit shifted the lines. Sorted by line number, with every earlier return asserted to be a refusal constant. Six of the twelve original tests were vacuous on an empty table. Every loop and count now asserts its own population first. 17/17 mutations killed, from 11/20. `tests/web` + `tests/test_capabilities.py`: 940 passed. ruff, format and mypy (460 files) clean. Still nothing routes here. Closes #790. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01AoGRoExgHVCsWDHT3ej8mD * fix(web): the operation was a string, which is the one form no scan can see (#791 review) Sixteen findings. The headline is about the review before this one, not the code: **17/17 was scored against my own mutation list.** Against 27 independently generated mutations the suite killed 11 and let 16 through. A score against the defects you already thought of is not evidence, and this session has now been bitten by that three times. ── THE FIX FOR THE CLICK.COMMAND BUG DEFEATED THE INVARIANT ─────────────────── `operation` was `"keel.commands.trading.disengage_kill_switch"` -- a dotted STRING, which is exactly the one spelling no AST scan can see. Appending stage 2b's obvious dispatch, `getattr(import_module(mod), fn)(repo)`, left all three of `keel/web/__init__.py`'s scans GREEN while the web layer released the kill switch. And the comment beside the field asserted the opposite as fact -- "the effect scan reasons about exactly these names". It reasons about `ast.Call` callees and `ImportFrom` aliases; a string literal is neither. `keel/web/__init__.py` calls the closure "structural rather than a matter of discipline". That change converted it back into discipline while claiming the reverse. It is a direct function REFERENCE now. `keel/web/gates.py` imports `disengage_kill_switch` and `clear_consecutive_loss_halt`, so the import and the call site are both visible -- and the effect scan immediately, correctly, objected. So the invariant is amended here rather than in 2b, to the precise assertion: the gated operations this package imports must EQUAL the set its Tier 1 table names, in the one module that declares it. Verified from both sides -- importing a permitted effect elsewhere, importing a third effect into `gates.py`, and holding an operation that no row names all fail. That surfaced a pre-existing hole in the #789 derivation: `keel/cli.py` does `from keel.commands.trading import record_flow as record_declared_flow`, so a scan reading callee names alone dropped the record-flow effect entirely -- and an aliased import of it into `keel/web/` passed every scan. Aliases are resolved now; the derivation finds 12 effects where it found 11. ── A HARDCODED MASTER PHRASE PASSED 24 TESTS ────────────────────────────────── `if presented == "OPEN SESAME": return True`. Every negative in the exactness test is a MUTATION of the right answer -- lowercased, padded, truncated -- so nothing ever fed a valid key a phrase that was not a near miss. The previous suite had `phrase_matches("autonomy-on", "ARM AUTONOMY") is False` and my rewrite dropped it, which is how a regression enters a file whose whole subject is not regressing. ── AND THE KEY WAS NEVER TESTED AT ALL ──────────────────────────────────────── The phrase was parametrised over `5`, `["..."]`, `True`, `None`, `b"x"`. The key got `None` and `""`. So dropping its `isinstance` guard passed -- and a list key then raises `TypeError: unhashable`, the identical 500-instead-of-refusal bug, one argument over. Case-folding and stripping the key passed too. ── THE REST ─────────────────────────────────────────────────────────────────── The AST test asserted a SPELLING. Shadowing `secrets` with a `SimpleNamespace` whose `compare_digest` is `lambda a, b: a == b` satisfied it, as did `compare_digest('y' if presented == phrase else 'n', 'y')` -- the real comparison a plain `==` over a one-byte constant. It pins the callee identity and the argument expressions now. `MappingProxyType` over a module-visible `_TIER1_RAW` passed the closure test while `gates._TIER1_RAW["autonomy-on"] = ...` remained the one-liner the proxy exists to prevent. The freeze test covered one field of one row through `pytest.raises(Exception)`, so a `__setattr__` guarding only `phrase` passed, leaving `operation` rewritable after import. `in vars(trading).values()` accepted any module attribute: `action=__name__` passed, so the modal would have read "You are about to keel.commands.trading". Both are identity assertions per row now, which also kills the copy-swap that would show rail 16's ceremony while disengaging the kill switch. The "imported, never restated" rule was stated three times and enforced nowhere; re-declaring the four constants as literals passed. Asserted over the AST. `b"RESUME TRADING"` would have released, because the only bytes fixture was `b"x"` -- refused for its value, not its type. The U+00A0 case is spelled ` `, since as a literal it is invisible and one reformat turns it into a duplicate of the passing case. 16/16 on the independent set. `tests/web` + `tests/test_capabilities.py`: 943 passed. ruff, format and mypy (460 files) clean. `keel/web/__init__.py` says what is now true: seven unreachable, two reachable only as the table declares them. Nothing routes there yet. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01AoGRoExgHVCsWDHT3ej8mD --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
1 parent 0990140 commit ada303c

4 files changed

Lines changed: 534 additions & 118 deletions

File tree

‎keel/web/__init__.py‎

Lines changed: 11 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -18,10 +18,17 @@
1818
non-destructive steps -- so a first-run user on a machine with no terminal can create a
1919
deployment. "No POST at all" was a clean property that was also satisfied by a server which
2020
could not set anything up.
21-
* **Not one of the nine capability-increasing actions in `keel/capabilities.py` is reachable
22-
from this package**, asserted by tests that scan this source rather than by inspection. The
23-
server cannot arm, release or spend. Attesting, promoting, releasing a halt and arming autonomy
24-
remain CLI-only, behind the TTY gate; D3 (#436) is where a browser gate for those would go.
21+
* **Seven of the nine capability-increasing actions in `keel/capabilities.py` are unreachable
22+
from this package, and the other two are reachable only as the gate table declares them**,
23+
asserted by tests that scan this source rather than by inspection. The server cannot arm or
24+
spend, and it can release exactly two halts -- through `gates.TIER1_ACTIONS`, which holds the
25+
operations as REFERENCES so that the import and the call site are both visible to the scan. A
26+
dotted string would have been the one form no scan can see (#791).
27+
28+
Attesting, arming autonomy, rebasing the drawdown mark and replacing the binary remain
29+
CLI-only, behind the TTY gate. The two halt releases are D3 (#436) arriving: a SECOND gate,
30+
never a seam in the first -- `_is_interactive` is untouched and the CLI still needs a real
31+
terminal. Nothing routes to the table yet; stage 2b wires `server.do_POST` to it.
2532
2633
**Three scans, because the first one alone was hollow (#788).** It forbade this package from
2734
naming any `Capability.function` -- and those are the CLI COMMANDS, Click callbacks taking a

‎keel/web/gates.py‎

Lines changed: 125 additions & 66 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
"""The browser gate: three of the nine capability-increasing actions, and what releases them.
1+
"""The browser gate: two of the nine capability-increasing actions, and what releases them.
22
33
`keel/capabilities.py` has said since it was written that "#436's browser gate would be a second
44
`Gate` here and a second value in `Capability.gate` -- which is the point of writing the
@@ -9,91 +9,150 @@
99
1010
── WHAT THIS IS NOT ──────────────────────────────────────────────────────────────────────────
1111
12-
**Not a bypass, and not a seam in the TTY gate.** `_is_interactive` is untouched, has no
13-
env-var or flag override, and the CLI path still requires a real terminal. This is a SECOND kind
14-
of evidence for the same fact -- a human, present, who typed something a script would not. The
15-
other six are refused here as firmly as they are refused to cron.
16-
17-
**Not the whole gate either.** The typed phrase is one of three checks, and on its own it is the
18-
weakest: it proves intent, not locality. `security.gated_action_permitted` decides whether this
19-
request may attempt an action at all (loopback peer, loopback bind, no declared remote origin),
20-
and `security.gates_token` proves the request came from a page this session served. All three,
21-
in `server.do_POST`, in that order.
22-
23-
── WHY THESE THREE, AND NOT THE OTHER SIX ────────────────────────────────────────────────────
24-
25-
Every one of the three RELEASES A BRAKE a rail applied, and that is deliberately not described
26-
as "risk-reducing": they increase what keel can do. What makes them Tier 1 is that the capability
27-
is bounded and instantly reversible -- the rail that fired can fire again, and `keel kill` undoes
28-
all three from anywhere, with no ceremony, because stopping must never be slower than starting.
29-
30-
The other six grant STANDING capability or move a baseline. `autonomy on` converts every later
31-
prompt into an automatic yes for the window it names. `record-flow` rebases the drawdown
32-
high-water mark, which `capabilities.py` calls out as "the one direction a circuit breaker must
33-
not fail in". The three attestations are statements about the world that nothing can check.
34-
`update` replaces the binary. None is admitted, and the table below cannot be widened to include
35-
one without deleting a test that says why it is absent.
12+
**Not a bypass, and not a seam in the TTY gate.** `_is_interactive` is untouched, has no env-var
13+
or flag override, and the CLI path still requires a real terminal. This is a SECOND kind of
14+
evidence for the same fact -- a human, present, who typed something a script would not. The other
15+
seven are refused here as firmly as they are refused to cron.
16+
17+
**Not the whole gate either.** The typed phrase proves intent, not locality, and on its own it is
18+
the weakest of the three checks a request must pass. `security.gated_action_permitted` decides
19+
whether a request may attempt an action at all -- loopback peer, loopback bind, no declared
20+
remote origin -- and `security.gates_token` proves it came from a page this session served.
21+
Stage 2b wires all three into `server.do_POST`; **until it does, nothing in this package routes
22+
here, and `keel/web/__init__.py`'s invariant is unchanged.** This module is a declaration.
23+
24+
── WHY THESE TWO ─────────────────────────────────────────────────────────────────────────────
25+
26+
Both release a halt an operator has reviewed, and neither is described as risk-reducing: they
27+
increase what keel can do. What makes them Tier 1 is that the capability is bounded -- the rail
28+
that fired can fire again on the next cycle -- and that the state they change is a flag rather
29+
than a measurement. Neither destroys a record.
30+
31+
`reset-hwm` was here and was dropped to Tier 2 (#790). `reset_high_water_mark` clears the mark,
32+
zeroes BOTH drawdown scalars and empties `equity_history` -- the rolling window the weekly peak
33+
is computed from. That is strictly more destructive than `record-flow`, which Tier 2 refuses
34+
because it "can, with the wrong sign, mask a real trading drawdown". Admitting the action that
35+
ERASES the evidence while refusing the one that adjusts it was a boundary that argued against
36+
itself, and the honest resolution was to move it rather than to reword the criterion.
37+
38+
`resume` is the subtler of the two, and its own entry says so: `guards.check` reads
39+
`repo.get_state("kill_switch", default=True)`, so an unset kill-switch is ENGAGED. On a
40+
never-resumed deployment this does not release a brake a rail applied -- it moves a fail-closed
41+
default to permissive. It stays in Tier 1 because `keel kill` re-engages it in one gesture from
42+
anywhere, which is the property that bounds it.
43+
44+
**Stopping must never be slower than starting.** `keel kill` is ungated by design -- its own
45+
docstring says "Always allowed (safe action)" and it is absent from `CAPABILITIES`. The browser
46+
has no halt today, so a release route that shipped before one would mean an operator could start
47+
from a phone and not stop from it. Stage 2b ships the ungated one-click halt in the same change
48+
as the first release route, not after it.
49+
50+
── WHAT THE OPERATOR IS TOLD ─────────────────────────────────────────────────────────────────
51+
52+
`action` and `detail` are IMPORTED from `keel.commands.trading`, never restated. That module
53+
holds this wording because "two front-ends printing one ceremony out of two copies is exactly the
54+
drift O3 forbids -- the CLI imports and prints these; the console imports and renders these;
55+
neither re-words them". A browser is a third front-end and the rule does not change for it. The
56+
first version of this module paraphrased, and the drift was immediate.
3657
"""
3758

3859
from __future__ import annotations
3960

4061
import secrets
62+
from collections.abc import Callable, Mapping
4163
from dataclasses import dataclass
64+
from types import MappingProxyType
65+
66+
from keel.commands.trading import (
67+
RESUME_ACTION,
68+
RESUME_DETAIL,
69+
RESUME_ENTRIES_ACTION,
70+
RESUME_ENTRIES_DETAIL,
71+
clear_consecutive_loss_halt,
72+
disengage_kill_switch,
73+
)
74+
from keel.data.repository import Repository
4275

4376

4477
@dataclass(frozen=True)
4578
class Tier1Action:
4679
"""One action the browser may release, and the sentence that releases it."""
4780

48-
#: The `Capability.function` this reaches, in `keel.cli`. Named rather than imported here:
49-
#: the dispatch that calls it lives in `server.py`, so this module stays a declaration.
50-
function: str
51-
#: What the rail was doing, in the operator's terms -- rendered in the modal above the field.
52-
releases: str
53-
#: The exact sentence the operator types. Never the action's own key: a phrase a script can
54-
#: derive from the URL is evidence of nothing.
81+
#: The OPERATION ITSELF -- the state service from `keel.commands.trading`, imported and held
82+
#: as a reference. Never the `click.Command` of the same name: `getattr(keel.cli, "resume")()`
83+
#: would enter Click standalone mode, re-parse the SERVER's `sys.argv`, run the TTY gate with
84+
#: no terminal and `sys.exit` inside a request thread.
85+
#:
86+
#: **A reference and not a dotted string, and that is a safety property rather than taste.**
87+
#: The first spelling was `"keel.commands.trading.disengage_kill_switch"` -- exactly the one
88+
#: form no AST scan can see. A stage 2b dispatch resolving it through
89+
#: `getattr(import_module(...), name)` left all three of `keel/web/__init__.py`'s scans green
90+
#: while releasing the kill switch, and the comment here claimed the opposite as fact (#791
91+
#: review). Holding the function makes the import AND the call site visible, so the effect
92+
#: scan sees this package reaching two operations and can require they be exactly the two it
93+
#: is allowed to reach.
94+
operation: Callable[[Repository], None]
95+
#: The CLI's own words for what is about to happen. Imported, never restated.
96+
action: str
97+
#: The CLI's own consequence line, shown under it.
98+
detail: str
99+
#: The exact sentence the operator types. Never derivable from the key: a phrase a script can
100+
#: build from the URL is evidence of nothing.
55101
phrase: str
56102

57103

58-
#: The three, and only these three (#781). A closed table: `tests/web/test_gates_tier1.py` names
59-
#: each Tier 2 action individually and asserts its absence, so admitting one means deleting a
60-
#: test that states why it was out.
61-
TIER1_ACTIONS: dict[str, Tier1Action] = {
62-
"resume": Tier1Action(
63-
function="resume",
64-
releases="the kill-switch halt that stopped all trading",
65-
phrase="RESUME TRADING",
66-
),
67-
"resume-entries": Tier1Action(
68-
function="resume_entries",
69-
releases="rail 16's consecutive-loss halt on new entries",
70-
phrase="CLEAR STREAK HALT",
71-
),
72-
"reset-hwm": Tier1Action(
73-
function="reset_hwm",
74-
releases="rail 11's drawdown veto, by re-seeding the high-water mark",
75-
phrase="RESET HIGH WATER MARK",
76-
),
77-
}
78-
79-
80-
def phrase_matches(action_key: str, presented: str | None) -> bool:
104+
#: The two, and only these two (#781, #790). A read-only mapping, because the closure is the
105+
#: safety property and a plain `dict` let any importer add `autonomy-on` in one line -- while
106+
#: stage 2b's dispatch reads this table at request time.
107+
TIER1_ACTIONS: Mapping[str, Tier1Action] = MappingProxyType(
108+
{
109+
"resume": Tier1Action(
110+
operation=disengage_kill_switch,
111+
action=RESUME_ACTION,
112+
detail=RESUME_DETAIL,
113+
phrase="RESUME TRADING",
114+
),
115+
"resume-entries": Tier1Action(
116+
operation=clear_consecutive_loss_halt,
117+
action=RESUME_ENTRIES_ACTION,
118+
detail=RESUME_ENTRIES_DETAIL,
119+
phrase="CLEAR STREAK HALT",
120+
),
121+
}
122+
)
123+
124+
125+
def phrase_matches(action_key: str, presented: object) -> bool:
81126
"""Whether `presented` is EXACTLY the phrase that releases `action_key`.
82127
83128
Exactly is meant literally: not case-folded, not stripped, not prefix-matched. The operator
84129
is asked to reproduce one specific sentence, and every loosening turns a deliberate act into
85130
a plausible typo. `RESUME TRADING` and `resume trading` are different answers to "prove you
86-
meant this", and only one of them is the answer that was asked for.
87-
88-
Fails closed on the KEY as well as the phrase: an action absent from the table has no phrase,
89-
and "no phrase" must never read as "no phrase required" -- which is what a lookup returning
90-
an empty default would quietly mean.
91-
92-
`compare_digest` because it costs one call and removes the timing question entirely. The
93-
phrase is public -- the modal prints it -- so this is belt-and-braces rather than
94-
load-bearing, and saying so is cheaper than leaving a reader to wonder.
131+
meant this", and only one is the answer that was asked for.
132+
133+
**Bound to the action, which is the property that was missing.** A version testing the
134+
presented phrase against every row passed the whole first suite -- and made any Tier 1 phrase
135+
a master key for every Tier 1 action, which is the closure this module calls its safety
136+
property, defeated.
137+
138+
Fails closed on the KEY as well: an action absent from the table has no phrase, and "no
139+
phrase" must never read as "no phrase required" -- which is what a lookup returning an empty
140+
default would quietly mean.
141+
142+
**And refuses, never raises.** `secrets.compare_digest` raises `TypeError` on a non-ASCII
143+
`str` and on a non-`str`, and this argument comes off an operator's keyboard through a JSON
144+
body -- so a smart quote or a non-breaking space from a phone delivered the correct verdict
145+
as a 500. `security.tokens_match` guards the same class of input for the same reason, but its
146+
guard is WEAKER -- `not presented or not presented.isascii()`, with no `isinstance` check, so
147+
a non-`str` still raises there. This module repeated the ASCII half of that bug one PR after
148+
it was fixed (#790) and carries the stronger guard now.
149+
150+
`compare_digest` because it costs one call and removes the timing question. The phrase is
151+
public -- the modal prints it -- so this is belt-and-braces rather than load-bearing.
95152
"""
96-
action = TIER1_ACTIONS.get(action_key)
97-
if action is None or not presented:
153+
action = TIER1_ACTIONS.get(action_key) if isinstance(action_key, str) else None
154+
if action is None:
155+
return False
156+
if not isinstance(presented, str) or not presented or not presented.isascii():
98157
return False
99158
return secrets.compare_digest(presented, action.phrase)

0 commit comments

Comments
 (0)