diff --git a/keel/capabilities.py b/keel/capabilities.py index 84719fac..0e71d94c 100644 --- a/keel/capabilities.py +++ b/keel/capabilities.py @@ -287,8 +287,9 @@ def gate_named(name: str) -> Gate: def render_lines() -> list[str]: - """The inventory as text -- ONE renderer, so the CLI and the browser view cannot drift into - two accounts of the same safety model.""" + """The inventory as text, read by `keel.commands.capabilities` (`keel capabilities`). ONE + renderer, so a second text surface reuses it rather than writing a second account of the same + safety model; the MCP tool serves `CAPABILITIES` itself, as data.""" lines: list[str] = [] for gate in GATES: covered = [cap for cap in CAPABILITIES if cap.gate == gate.name] diff --git a/keel/commands/_common.py b/keel/commands/_common.py index 60cd60dd..d9111e24 100644 --- a/keel/commands/_common.py +++ b/keel/commands/_common.py @@ -256,10 +256,11 @@ def record_cash_posture_refutation( and not proof of a cash posture -- promoting a record on the strength of it would manufacture exactly the affirmation #666 established cannot be had. - `repo=None` is the read-only call sites (`keel balances`, `keel brokers list`), which have no - repository in hand and will not go on to trade. The CHECK still runs and still refuses for - them; only the recording is skipped, so threading a repo through every inspection command to - record a fact none of them acts on is churn nobody needs. + `repo=None` is every `_build_broker` caller with no repository in hand, none of which opens a + position: the inspections (`keel assets holdings`, `keel assets discover`), the candle fetch + behind `keel fetch`, `keel simulate` and the setup steps, and `keel orders cancel`. The CHECK + still runs and still refuses for them; only the recording is skipped, so threading a repo + through every one of them to record a fact none of them acts on is churn nobody needs. """ try: broker.verify_cash_account() diff --git a/keel/commands/doctor.py b/keel/commands/doctor.py index 316d5a36..4c947b2c 100644 --- a/keel/commands/doctor.py +++ b/keel/commands/doctor.py @@ -1809,8 +1809,10 @@ def position_watch_findings( there -- the only real way out on paper is re-promoting the rule. * `position.unprotected` -- an open tranche with a recorded `initial_stop > 0`, no resting bracket (`reconcile._has_resting_bracket`, passed in as `resting`), and no retry record. - The third clause makes it the complement of the reconcile sweep, not a duplicate. DCA - (`initial_stop` absent) is excluded for the reason the sweep skips it silently. SKIPPED + The third clause makes it the complement of the reconcile sweep, not a duplicate. DCA is + excluded for the reason the sweep skips it silently: an unattributed leg stores no + `initial_stop`, and an attributed one stores `Dca.detect`'s `0` sentinel (#973), which the + `> 0` clause reads as no stop, as `status.recorded_stop` does. SKIPPED ENTIRELY (reported `OK`) when `managed_status == "paper"` (#881): a paper fill never gets a bracket (`_paper_enter` places none), so `resting` is always False, and paper never writes an `unbracketed:` retry record either (`reconcile_unbracketed_positions` only runs diff --git a/keel/commands/positions.py b/keel/commands/positions.py index 339e0b07..4a439c02 100644 --- a/keel/commands/positions.py +++ b/keel/commands/positions.py @@ -22,9 +22,11 @@ WHAT IS ABSENT STAYS ABSENT. A product with no cached candle has no mark, and a row with no mark reports `None` for every figure that depends on one -- never zero. Zero market value renders a held position as a total loss, which is the most alarming thing this page could say, and it -would be saying it about missing data. `initial_stop` is `None` for a DCA leg or a pre-v12 -tranche, so its distance is `None` too: a distance measured against a substituted zero would -read as a position comfortably clear of a stop it does not have. +would be saying it about missing data. A tranche with no stop has no stop distance either: a +distance measured against a substituted zero would read as a position comfortably clear of a stop +it does not have. "No stop" is `status.recorded_stop`'s reading, not `initial_stop is None`: the +column is NULL on a pre-v12 tranche or an unattributed DCA leg, but `agent._open_tranche` copies +`Dca.detect`'s `stop=0` sentinel onto every attributed DCA tranche, which stores "0" (#973). NO CLOSE ACTION, EVER (#701's own refusal). This module places nothing and cancels nothing. Exits degrade to the typed-phrase friction path; a panic tap on a table row must never be the @@ -41,6 +43,7 @@ from keel import agent as agent_mod from keel.commands.doctor import ATTEST_WINDOW_APPROACHING_SEC as _DOCTOR_APPROACHING_SEC +from keel.commands.status import recorded_stop from keel.config import Config from keel.data import freshness as freshness_mod from keel.data.repository import Repository @@ -117,14 +120,18 @@ class PositionRow: market_value: Decimal | None unrealized_pnl: Decimal | None - #: The stop this tranche was SIZED against (`positions.initial_stop`). `None` means "not on - #: this row" -- a DCA leg, or a tranche predating v12 -- and NOT "no stop". + #: `positions.initial_stop` AS STORED: the stop this tranche was sized against, `None` on a + #: tranche predating v12 or an unattributed DCA leg, and `0` on an attributed DCA tranche (the + #: `Dca.detect` sentinel, which is no stop). Carried verbatim because Holdings' paper reading + #: names that 0 (`status.shown_stop`); anything that MEASURES against the stop reads it + #: through `status.recorded_stop`, as `stop_distance` does. initial_stop: Decimal | None #: `mark - initial_stop`, SIGNED: a tranche trading THROUGH its stop is the state an operator #: most needs to see, and an absolute distance would render it identically to one safely #: above. `stop_distance_pct` is that distance as a fraction OF THE MARK. Both `None` when - #: either the stop or the mark is missing. + #: the mark is missing or the tranche has no stop (`status.recorded_stop` is `None`: NULL, + #: or the zero a DCA tranche stores). stop_distance: Decimal | None stop_distance_pct: Decimal | None @@ -391,15 +398,16 @@ def _row_from_dict( qty = raw["qty"] entry_fill = raw["entry_fill"] initial_stop = raw.get("initial_stop") + stop = recorded_stop(initial_stop) market_value = None if mark is None else qty * mark unrealized = None if mark is None else qty * (mark - entry_fill) - if mark is None or initial_stop is None: + if mark is None or stop is None: stop_distance: Decimal | None = None stop_distance_pct: Decimal | None = None else: - stop_distance = mark - initial_stop + stop_distance = mark - stop stop_distance_pct = _safe_ratio(stop_distance, mark) return PositionRow( diff --git a/keel/commands/rules.py b/keel/commands/rules.py index c666b50a..10d09182 100644 --- a/keel/commands/rules.py +++ b/keel/commands/rules.py @@ -2305,6 +2305,95 @@ def rules_add(ctx: click.Context, kind: str, product: str, params_json: str | No ctx.exit(1) +def _require_rule_class(kind: str, echo_err: Callable[[str], None]) -> type: + """`kind`'s rule class, or the `unknown rule kind` refusal (echoed, then `RulesRefused`).""" + rule_cls = agent.RULE_REGISTRY.get(kind) + if rule_cls is None: + echo_err(f"Error: unknown rule kind {kind!r}; known kinds: {sorted(agent.RULE_REGISTRY)!r}") + raise RulesRefused(f"unknown rule kind {kind!r}") + return rule_cls + + +def validated_row_params( + kind: str, + supplied: dict[str, Any], + product_id: str, + *, + echo_err: Callable[[str], None] = _noop, +) -> dict[str, Any]: + """The params a `rules` row stores for `supplied`, or a refusal -- `add_rule_row`'s checks on + the params themselves, as one function so every writer of `rules` refuses the same rows. + + Builds the rule, then returns its `describe()` params (JSON-plain, `product_id` re-applied): + exactly what `agent._build_rule` reconstructs. Refuses -- `Error: ...` lines to `echo_err`, + then `RulesRefused`, nothing written -- an unknown kind, a mistyped or unknown param, one the + rule's own validation rejects, a non-finite number, and a param the row would not persist. + + That last one is the `name` refusal #961 depends on (#974): `describe()` carries no `name` + for any kind, so a `"name": "dca"` on a turtle is a dropped param and never reaches the row + whose rebuilt `rule.name` becomes `positions.rule_name`. Read by `add_rule_row` and by + `scripts/rule_manifest.py apply`, the two writers that take params from outside.""" + rule_cls = _require_rule_class(kind, echo_err) + mismatches = _param_type_mismatches(kind, rule_cls, supplied) + if mismatches: + quotable = sorted(agent.coerced_param_keys(kind)) + echo_err( + f"Error: {kind} cannot use these params:\n" + + "\n".join(f" - {problem}" for problem in mismatches) + + f"\n (a quoted value is right only for {quotable!r} -- keel converts " + f"those on the way in)" + ) + raise RulesRefused(f"{kind} cannot use these params") + + try: + rule = agent.build_rule_from_params(kind, {**supplied, "product_id": product_id}) + except TypeError as exc: + # An unknown/misspelled kwarg: the constructor names it, we name the alternatives. + echo_err( + f"Error: {kind} does not accept these params: {exc}\n" + f" accepted params: {_accepted_params(rule_cls)!r}" + ) + raise RulesRefused(f"{kind} does not accept these params") from exc + except (ValueError, ArithmeticError) as exc: + # The rule's OWN validation (e.g. `Dca`: "cadence_days must be positive"), or a value + # that is not the type the coercion boundary expects (`Decimal("x")` -> InvalidOperation, + # an `ArithmeticError`). Either way the params cannot make a rule, so no row. + echo_err(f"Error: {kind} rejected these params: {exc}") + raise RulesRefused(f"{kind} rejected these params") from exc + + nonfinite = _nonfinite_params(rule.describe()["params"]) + if nonfinite: + echo_err( + f"Error: {kind} was given a value that is not a finite number, which no backtest " + f"can report on:\n" + "\n".join(f" - {problem}" for problem in nonfinite) + ) + raise RulesRefused(f"{kind} was given a non-finite value") + + # `.describe()`'s params, JSON-plain -- the same thing `rules seed` stores, so the row is + # exactly what `agent._build_rule` reconstructs. `product_id` is re-applied because not + # every kind carries it in `describe()` (e.g. `TurtleBreakout` does not). + params: dict[str, Any] = _json_plain(rule.describe()["params"]) + params["product_id"] = product_id + + # A constructor kwarg that `describe()` does not carry would be accepted, then LOST: the row + # rebuilds without it and the backtest measures a different rule than the one asked for. + # `PullbackContinuation(granularity=ONE_DAY)` is exactly this -- it constructs, `params` has + # no `granularity` key, and `_build_rule` rebuilds it at the ONE_HOUR default, on a different + # candle series. Silently trading a rule the operator did not ask for is worse than a + # refusal, so this is a refusal. Derived from the rule's own `describe()`, so a kind that + # starts persisting a field needs no change here. + dropped = sorted(set(supplied) - set(params)) + if dropped: + echo_err( + f"Error: {kind} accepts {dropped!r} but does not persist " + f"{'them' if len(dropped) > 1 else 'it'} in the rule row, so the value would be " + f"silently lost when the rule is rebuilt for a backtest or a cycle. Refusing rather " + f"than storing a rule that is not the one you asked for." + ) + raise RulesRefused(f"{kind} does not persist {dropped!r}") + return params + + def add_rule_row( repo: Repository, config: Any, @@ -2343,10 +2432,7 @@ def add_rule_row( ) product_id = product_list[0] - rule_cls = agent.RULE_REGISTRY.get(kind) - if rule_cls is None: - echo_err(f"Error: unknown rule kind {kind!r}; known kinds: {sorted(agent.RULE_REGISTRY)!r}") - raise RulesRefused(f"unknown rule kind {kind!r}") + _require_rule_class(kind, echo_err) # "flag absent" and "flag given but empty" are different intentions, and only `is None` # tells them apart. An empty string is what a shell hands over when the proposal plumbing @@ -2384,63 +2470,7 @@ def add_rule_row( param_hint="--params", ) - mismatches = _param_type_mismatches(kind, rule_cls, supplied) - if mismatches: - quotable = sorted(agent.coerced_param_keys(kind)) - echo_err( - f"Error: {kind} cannot use these params:\n" - + "\n".join(f" - {problem}" for problem in mismatches) - + f"\n (a quoted value is right only for {quotable!r} -- keel converts " - f"those on the way in)" - ) - raise RulesRefused(f"{kind} cannot use these params") - - try: - rule = agent.build_rule_from_params(kind, {**supplied, "product_id": product_id}) - except TypeError as exc: - # An unknown/misspelled kwarg: the constructor names it, we name the alternatives. - echo_err( - f"Error: {kind} does not accept these params: {exc}\n" - f" accepted params: {_accepted_params(rule_cls)!r}" - ) - raise RulesRefused(f"{kind} does not accept these params") from exc - except (ValueError, ArithmeticError) as exc: - # The rule's OWN validation (e.g. `Dca`: "cadence_days must be positive"), or a value - # that is not the type the coercion boundary expects (`Decimal("x")` -> InvalidOperation, - # an `ArithmeticError`). Either way the params cannot make a rule, so no row. - echo_err(f"Error: {kind} rejected these params: {exc}") - raise RulesRefused(f"{kind} rejected these params") from exc - - nonfinite = _nonfinite_params(rule.describe()["params"]) - if nonfinite: - echo_err( - f"Error: {kind} was given a value that is not a finite number, which no backtest " - f"can report on:\n" + "\n".join(f" - {problem}" for problem in nonfinite) - ) - raise RulesRefused(f"{kind} was given a non-finite value") - - # `.describe()`'s params, JSON-plain -- the same thing `rules seed` stores, so the row is - # exactly what `agent._build_rule` reconstructs. `product_id` is re-applied because not - # every kind carries it in `describe()` (e.g. `TurtleBreakout` does not). - params = _json_plain(rule.describe()["params"]) - params["product_id"] = product_id - - # A constructor kwarg that `describe()` does not carry would be accepted, then LOST: the row - # rebuilds without it and the backtest measures a different rule than the one asked for. - # `PullbackContinuation(granularity=ONE_DAY)` is exactly this -- it constructs, `params` has - # no `granularity` key, and `_build_rule` rebuilds it at the ONE_HOUR default, on a different - # candle series. Silently trading a rule the operator did not ask for is worse than a - # refusal, so this is a refusal. Derived from the rule's own `describe()`, so a kind that - # starts persisting a field needs no change here. - dropped = sorted(set(supplied) - set(params)) - if dropped: - echo_err( - f"Error: {kind} accepts {dropped!r} but does not persist " - f"{'them' if len(dropped) > 1 else 'it'} in the rule row, so the value would be " - f"silently lost when the rule is rebuilt for a backtest or a cycle. Refusing rather " - f"than storing a rule that is not the one you asked for." - ) - raise RulesRefused(f"{kind} does not persist {dropped!r}") + params = validated_row_params(kind, supplied, product_id, echo_err=echo_err) existing = [ row diff --git a/keel/commands/status.py b/keel/commands/status.py index 6de2b7cb..758ec203 100644 --- a/keel/commands/status.py +++ b/keel/commands/status.py @@ -489,13 +489,17 @@ def _kind_never_brackets(rule_kind: str) -> bool: registry does not know (an empty `rule_name`, a retired kind) is NOT excused: its missing bracket stays a warning, the reading that cannot hide a real gap. - `rule_kind` is `positions.rule_name`, which is `rule.name`. No shipped path stores a name: - seed stores `describe()["params"]`, which carries no `name` for any kind, and `keel rules - add` refuses a `name` in `--params` (its dropped-param refusal, or `rsi_meanrev`'s - constructor, which takes no `name`), so a rule rebuilt from a row is named its kind -- pinned - by `test_every_kind_is_named_its_kind_and_stores_no_name_param` and - `test_add_rule_row_refuses_a_name_param_and_writes_no_row`. A hand-written row naming - another kind would be judged as that kind; a name the registry does not know WARNs.""" + `rule_kind` is `positions.rule_name`, which is `rule.name`. No path that writes `rules` + stores a name: seed stores `describe()["params"]`, which carries no `name` for any kind, and + `keel rules add` and `scripts/rule_manifest.py apply` both refuse a `name` param through + `rules.validated_row_params` (its dropped-param refusal, or `rsi_meanrev`'s constructor, + which takes no `name`; #974), so a rule rebuilt from a row is named its kind -- pinned by + `test_every_kind_is_named_its_kind_and_stores_no_name_param`, + `test_add_rule_row_refuses_a_name_param_and_writes_no_row` and + `test_apply_refuses_a_name_param_and_writes_nothing`. Keyed off the name rather than + `rules.kind` through `positions.rule_id` because unattributed tranches have no `rule_id`, and + every caller would need a rule-id-to-kind read for the attributed rest. A hand-written SQL row + naming another kind would be judged as that kind; a name the registry does not know WARNs.""" rule_cls = agent.RULE_REGISTRY.get(rule_kind) return rule_cls is not None and not rule_cls.places_bracket diff --git a/keel/data/db.py b/keel/data/db.py index de08c247..92873b1a 100644 --- a/keel/data/db.py +++ b/keel/data/db.py @@ -1096,7 +1096,7 @@ def _migrate_v21_positions_rule_id(conn: sqlite3.Connection) -> None: needed attributing: `executor.place_bracket` and the scale-out path build their `OrderIntent` from a POSITION rather than from a signal, so they had no `rules.id` to put on `orders.rule_id`. Every protective SELL was written unattributed, and - `payload._order_payload` rendered it as such. + `payload._order_row_payload` rendered it as such. That is not a cosmetic gap. Per-rule accounting reading `orders.rule_id` saw entries attributed and exits anonymous, which for a trend rule drops exactly the leg the outcome diff --git a/keel/web/api.py b/keel/web/api.py index 9ff2d739..8b4f8cb7 100644 --- a/keel/web/api.py +++ b/keel/web/api.py @@ -656,7 +656,7 @@ def read_rules(cfg: ServeConfig, _query: Query, _state: Any, now_ts: int) -> dic #: How long one deployment's admission screen stays cached (#856). 5 minutes: short enough that #: an operator who just fixed a candle gap or attested an asset sees it admitted within one -#: coffee, long enough that `/rules?budget=`'s 15s poll (`server._REFRESH_SEC`) stops re-running +#: coffee, long enough that `/rules?budget=`'s 15s poll (`main.js`'s `POLL_MS`) stops re-running #: the whole allowlist's `screen_product` against the database twenty times for every time an #: operator actually changes the budget or buffer in the address bar. _SCREEN_CACHE_TTL_SECONDS = 300 @@ -866,9 +866,11 @@ class ApiRoute: by a client that cannot render it. `""` means there is no rendered counterpart at all, which today is only `/api/config`. - `needs_database` is the JSON counterpart of `server.needs_database`: rather than serving the - setup checklist in place of a broken page, it answers `data: null` with `engine: "stopped"`, - which is the same statement in the shape a client can act on. + `needs_database` guards the whole route set rather than one page (it descends from the + `needs_database` wrapper in `server` that #540 deleted along with the rendered pages): + rather than reading tables that are not there, it answers `data: null` with + `engine: "stopped"`, which is the statement the setup checklist used to make, in the shape a + client can act on. `collection` and `sortable` are the sort surface. `collection` is the key of the one list in `data` that `?sort=` orders; `sortable` is the closed set of columns it may be ordered by, and @@ -884,10 +886,11 @@ class ApiRoute: sortable: tuple[str, ...] = field(default=()) -#: THE READ SURFACE, in full. Every one of the rendered routes in `server.ROUTES` appears here as -#: its `html_route` except `/glossary`, which gets no counterpart: it becomes an outbound -#: keeltrading.com link in #539 and `render_glossary` is deleted in #540, so an `/api/glossary` -#: would be a surface built in order to be removed -- and any client written against it would break +#: THE READ SURFACE, in full. Every one of the rendered routes that `server`'s `ROUTES` held +#: (the table #540 deleted) appears here as its `html_route` except `/glossary`, which gets no +#: counterpart: it becomes an outbound keeltrading.com link in #539 and `render_glossary` is +#: deleted in #540, so an `/api/glossary` would be a surface built in order to be removed -- and +#: any client written against it would break #: on the release that removes it. `/api/pulse` carries the landing route (`/pulse`, the client's #: default): the glance page, and the one whose body holds the halt token (never the gate token). API_ROUTES: dict[str, ApiRoute] = { diff --git a/keel/web/events.py b/keel/web/events.py index 2cab635a..bde2ccb9 100644 --- a/keel/web/events.py +++ b/keel/web/events.py @@ -91,7 +91,8 @@ #: Seconds between ticks. #: -#: Five, against `server._REFRESH_SEC`'s fifteen, and the two are answering different questions. +#: Five, against the client's fifteen-second poll (`main.js`'s `POLL_MS`), and the two are +#: answering different questions. #: Fifteen is how stale a FIGURE may get; five is how long a dead server may look alive, and that #: is the number an operator is standing in front of. It is also the heartbeat that keeps the #: connection from being reaped by anything in between -- there is nothing in between on loopback diff --git a/keel/web/gates.py b/keel/web/gates.py index ba39dd7e..20d78e98 100644 --- a/keel/web/gates.py +++ b/keel/web/gates.py @@ -12,10 +12,10 @@ **Not a bypass, and not a seam in the TTY gate.** `_is_interactive` is untouched, has no env-var or flag override, and the CLI path still requires a real terminal. This is a SECOND kind of evidence for the same fact -- a human, present, who typed something a script would not. The other -seven are refused here as firmly as they are refused to cron. +eight are refused here as firmly as they are refused to cron. **Not the whole gate either.** The typed phrase proves intent, not locality, and on its own it is -the weakest of the three checks a request must pass. `security.gated_action_permitted` decides +the weakest of the seven checks a release must pass. `security.gated_action_permitted` decides whether a request may attempt an action at all -- loopback peer, loopback bind, no declared remote origin -- and `security.gates_token` proves the caller holds this session's secret. No endpoint serves that token (#968): the console's HALT button holds `security.halt_token`, which @@ -213,10 +213,12 @@ def run_gated_action(cfg: Any, action_key: str, presented: object) -> str | None exist and asserts the refusal still returns rather than raising, which is what pins the ordering rather than merely describing it. - This is the LAST of four checks, not the only one. `server.do_POST` runs the admission gate, - the client-header gate, `security.gated_action_permitted` (loopback peer, loopback bind, no - declared remote origin) and `security.gates_token` -- never the halt token -- before anything - reaches here. The phrase proves intent; those prove locality and provenance. + This is the LAST of seven checks, not the only one. `server.do_POST` runs four on every write + -- admission (`_admitted`), `Sec-Fetch-Site` (`_sec_fetch_site_ok`), the `/api/` prefix and + the client header (`_api_client_header_ok`) -- and `server._gated_action` two more for a + release: `security.gated_action_permitted` (loopback peer, loopback bind, no declared remote + origin) and `security.gates_token` -- never the halt token. The phrase proves intent; those + prove locality and provenance. """ action = TIER1_ACTIONS.get(action_key) if isinstance(action_key, str) else None if action is None or not phrase_matches(action_key, presented): diff --git a/keel/web/server.py b/keel/web/server.py index 256a0e56..4954291e 100644 --- a/keel/web/server.py +++ b/keel/web/server.py @@ -74,11 +74,6 @@ tokens_match, ) -#: How often the live pages reload themselves. 15s matches the TUI's own poll: fast enough that -#: "is it running" is answered without a keypress, slow enough that a page being read does not -#: jump away every few seconds. -_REFRESH_SEC = 15 - #: Cap on a form body. `rfile.read(n)` with an attacker-supplied `n` is a memory-exhaustion #: primitive and there is no proxy in front of this server to impose a limit. A setup form carries #: an action key and a token. diff --git a/keel/web/static/css/keel.css b/keel/web/static/css/keel.css index f970d4ac..5f73e717 100644 --- a/keel/web/static/css/keel.css +++ b/keel/web/static/css/keel.css @@ -981,8 +981,9 @@ footer { /* Honour a reader who has asked the operating system for less motion. Written before there was * any animation to honour, so that the first one added inherited the courtesy rather than - * depending on its author remembering -- the same reasoning `server.needs_database` gives for - * guarding a whole route set instead of one page. #754's `keel-busy` is that first animation, + * depending on its author remembering -- the same reasoning the deleted `needs_database` + * wrapper in `server` gave for guarding a whole route set instead of one page. #754's + * `keel-busy` is that first animation, * and it arrived already covered, which is what the rule was for. * * Note what this does to it: `animation-duration: 0.01ms` plus `iteration-count: 1` leaves the diff --git a/keel/web/staticfiles.py b/keel/web/staticfiles.py index 62459bdb..27c35c44 100644 --- a/keel/web/staticfiles.py +++ b/keel/web/staticfiles.py @@ -37,8 +37,9 @@ #: The URL prefix static requests are served under, and the prefix the client mounts at. #: -#: **`/` since #540.** It was `/static/` from #535 to #539 only because `server.ROUTES` still owned -#: the root paths; deleting the rendered pages freed them, and the client is the application now +#: **`/` since #540.** It was `/static/` from #535 to #539 only because the rendered-page table +#: (`ROUTES` in `server`, deleted at #540) still owned the root paths; deleting the rendered pages +#: freed them, and the client is the application now #: rather than a second one parked beside it. The move was one edit per file rather than a hunt #: because `tests/web/test_client_assets.py::test_the_mount_prefix_is_spelled_the_same_everywhere` #: has pinned the three places that spell it -- here, `index.html`'s hrefs and `main.js`'s `BASE` -- diff --git a/scripts/rule_manifest.py b/scripts/rule_manifest.py index 280d6c68..f4fd3826 100644 --- a/scripts/rule_manifest.py +++ b/scripts/rule_manifest.py @@ -24,6 +24,12 @@ params from a JSON file would let an edited manifest change a LIVE rule's position size, which is exactly the kind of un-reviewed money change the promotion ladder exists to prevent. Reconcile drift deliberately, by hand. +- It **refuses an entry `keel rules add` would refuse** -- an unknown kind, params the rule + rejects, or a param the row would not persist, `name` above all (#974): `rule.name` is what a + tranche records as `positions.rule_name`, and a turtle stored as `"name": "dca"` would have its + missing bracket read as "no stop by design" (#961). The check is + `keel.commands.rules.validated_row_params`, the one `add_rule_row` runs, so the two writers + refuse the same rows in the same words. - It **refuses to create `live` rules without `--allow-live`.** Seeding straight to `live` bypasses the promotion gate (same bypass as `keel rules seed --status live` and `rules promote --force`), so it takes an explicit flag and prints a loud warning, mirroring how those commands behave. @@ -41,6 +47,7 @@ from pathlib import Path from typing import Any +from keel.commands.rules import RulesRefused, validated_row_params from keel.data.db import connect from keel.data.repository import Repository @@ -87,7 +94,8 @@ def apply( """Reconcile `db_path`'s rules against `manifest_path`. Returns a process exit code: 0 when the database matches (or was made to match) the manifest, - 1 when drift or a refused `live` insert means it does not. + 1 when drift, a refused `live` insert or an entry `validated_row_params` refuses means it does + not. A refused entry is skipped, never written; the rest of the manifest still applies. """ manifest = json.loads(manifest_path.read_text()) wanted = manifest["rules"] if isinstance(manifest, dict) else manifest @@ -98,12 +106,20 @@ def apply( to_insert: list[dict[str, Any]] = [] drift: list[str] = [] refused: list[str] = [] + invalid: list[str] = [] for entry in wanted: kind, status, params = entry["kind"], entry["status"], entry["params"] key = _key(kind, params) current = existing.get(key) + problems: list[str] = [] + try: + validated_row_params(kind, params, key[1], echo_err=problems.append) + except RulesRefused: + invalid.extend(f"{kind}:{key[1]}: {line.removeprefix('Error: ')}" for line in problems) + continue + if current is None: if status in _ARMED_STATUSES and not allow_live: refused.append(f"{kind}:{key[1]} wants status={status}") @@ -143,15 +159,17 @@ def apply( for line in refused: print(f"REFUSED: {line} -- pass --allow-live to create armed rules", file=sys.stderr) + for line in invalid: + print(f"REFUSED: {line}", file=sys.stderr) for line in drift: print(f"DRIFT: {line}", file=sys.stderr) - if not to_insert and not drift and not refused: + if not to_insert and not drift and not refused and not invalid: print(f"in sync: {len(wanted)} rule(s) match {manifest_path}") elif not write and to_insert: print("\ndry run -- nothing written. Re-run with --apply to write.") - return 1 if (drift or refused) else 0 + return 1 if (drift or refused or invalid) else 0 def main(argv: list[str] | None = None) -> int: diff --git a/tests/commands/test_build_broker_refutes_the_posture.py b/tests/commands/test_build_broker_refutes_the_posture.py index 44989ed3..26168f62 100644 --- a/tests/commands/test_build_broker_refutes_the_posture.py +++ b/tests/commands/test_build_broker_refutes_the_posture.py @@ -5,10 +5,11 @@ `doctor` still shows it green, and the only thing between that and a live entry is an exception nobody wrote down. -`repo` is OPTIONAL on `_build_broker` on purpose. Most call sites are read-only inspection -(`keel balances`, `keel brokers list`) with no repository in hand, and threading one through every -one of them to record a fact none of them will act on would be churn. The paths that matter are -the ones that go on to trade. +`repo` is OPTIONAL on `_build_broker` on purpose. Most call sites have no repository in hand and +open no position (inspection, the candle fetch, `keel orders cancel` -- the docstring of +`record_cash_posture_refutation` names them), and threading one through every one of them to +record a fact none of them will act on would be churn. The paths that matter are the ones that go +on to trade. **The refusal still propagates.** Recording it is in addition to failing closed, never instead -- `test_the_refusal_still_raises` pins that, because a refutation that swallowed the exception would @@ -17,6 +18,8 @@ from __future__ import annotations +import re + import pytest from keel_core.cash_posture import SPOT_CASH, CashPostureState, VenueCashPosture @@ -99,3 +102,28 @@ def test_nothing_attested_records_nothing_and_still_raises(repo: Repository) -> with pytest.raises(RuntimeError): record_cash_posture_refutation(_Refusing(), repo=repo, venue="coinbase", now_ts=NOW) assert repo.get_venue_cash_posture("coinbase") is None + + +def test_every_command_the_docstring_names_as_a_repoless_caller_exists() -> None: + """The docstring is where a reader learns which commands skip the recording, so every command + it names must be one `keel` has. It named `keel balances` -- a command that never existed, + whose only web reader went in #966 (#967).""" + import click + + from keel.cli import cli + + doc = record_cash_posture_refutation.__doc__ or "" + cited = re.findall(r"`keel ([a-z][a-z-]*(?: [a-z][a-z-]*)*)`", doc) + assert cited, "the docstring names its repo-less callers; this test reads that list" + + unknown = [] + for invocation in cited: + command: click.Command | None = cli + for word in invocation.split(): + if not isinstance(command, click.Group): + break + command = command.get_command(click.Context(command), word) + if command is None: + unknown.append(invocation) + break + assert unknown == [] diff --git a/tests/commands/test_positions.py b/tests/commands/test_positions.py index e1f1d112..6cae3901 100644 --- a/tests/commands/test_positions.py +++ b/tests/commands/test_positions.py @@ -181,6 +181,25 @@ def test_a_tranche_with_no_recorded_stop_reports_no_distance( assert row.stop_distance_pct is None +@pytest.mark.parametrize("sentinel", ["0", "-1"]) +def test_a_dca_tranches_zero_stop_is_no_stop_and_has_no_distance( + repo: Repository, tmp_path: Path, sentinel: str +) -> None: + """#973. `agent._open_tranche` copies `Dca.detect`'s `stop=0` sentinel onto every attributed + DCA tranche, so the live ledger stores `initial_stop` "0", not NULL. Measured against it, the + distance is the whole mark -- "100% clear of its stop" about a tranche that has none, the + reading the module docstring refuses. `status.recorded_stop` (zero or below is no stop) is + the one reading of that column; the row still carries the column as stored.""" + _open_tranche(repo, rule_name="dca", initial_stop=Decimal(sentinel)) + _mark(repo, "150") + + row = gather_positions(repo, _config(tmp_path), now_ts=NOW_TS).rows[0] + + assert row.initial_stop == Decimal(sentinel) + assert row.stop_distance is None + assert row.stop_distance_pct is None + + def test_no_mark_means_no_stop_distance_either(repo: Repository, tmp_path: Path) -> None: """The distance is measured from the CURRENT price. With no mark there is nothing to measure from, and measuring from the entry instead would quietly answer a different question.""" diff --git a/tests/commands/test_research_record.py b/tests/commands/test_research_record.py index 9a2926a2..87c200b1 100644 --- a/tests/commands/test_research_record.py +++ b/tests/commands/test_research_record.py @@ -254,7 +254,9 @@ def counting(target: Any) -> Any: # -- the rail, scanned mechanically --------------------------------------------------------------- -RECORD_MODULE_PATH = Path("keel/commands/research_record.py") +RECORD_MODULE_PATH = ( + Path(__file__).resolve().parents[2] / "keel" / "commands" / "research_record.py" +) def test_the_record_module_never_sorts_ranks_or_maxes() -> None: diff --git a/tests/conftest.py b/tests/conftest.py index 34a40733..687f8672 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -3,6 +3,7 @@ from __future__ import annotations import logging +import os from decimal import Decimal from pathlib import Path from typing import TYPE_CHECKING @@ -277,3 +278,79 @@ def _restore_keel_logger(): logger.setLevel(level) logger.propagate = propagate logger.handlers[:] = handlers + + +#: The checkout a run must leave as it found it (#964), resolved once at import -- before any +#: fixture has moved the working directory -- so the guard below watches the repository and the +#: directory pytest was started in, not wherever a test happens to be standing. +_REPO_ROOT = Path(__file__).resolve().parent.parent +_INVOCATION_CWD = Path.cwd().resolve() + + +def _logs_snapshot(root: Path) -> dict[str, tuple[int, int]] | None: + """Every file under `root/logs` with its size and mtime, or `None` when there is no `logs/`. + + Size AND mtime: a leak into an existing file (the operator ran `keel` in this checkout once) + grows it or touches it, and a leak that creates an EMPTY file -- the shape #964 reported -- + still changes the answer from `None` to a dict.""" + logs = root / "logs" + if not logs.is_dir(): + return None + return { + str(path.relative_to(logs)): (path.stat().st_size, path.stat().st_mtime_ns) + for path in sorted(logs.rglob("*")) + if path.is_file() + } + + +@pytest.fixture(autouse=True) +def _the_checkouts_logs_are_untouched(): + """Fail the test that wrote into the checkout's `logs/`, by name, the moment it does (#964). + + `logging.file` defaults to the RELATIVE `logs/keel.log`, and `configure_logging` opens it + against the working directory. Every CLI test that loads a config without a `logging:` + section used to open it in the checkout, leaving a file that the NEXT run's timeline export + read back as "the engine log could not be read (empty)" -- red on the second run, green + after a manual `rm -rf logs`, and false kills in every mutation run in between. + + Per test rather than once per session: a session check would say only that SOMETHING + leaked, and the leak is found by bisecting 8,000 tests. A `stat` per test is free. + `_run_in_tmp_path` below is what keeps it quiet; this is what notices when it stops.""" + roots = sorted({_REPO_ROOT, _INVOCATION_CWD}) + before = {root: _logs_snapshot(root) for root in roots} + yield + touched = [str(root / "logs") for root in roots if _logs_snapshot(root) != before[root]] + if touched: + pytest.fail( + f"this test wrote into {touched}: a test must log under its own tmp_path, never the " + "checkout (#964) -- the next run reads that file back as this deployment's engine log", + pytrace=False, + ) + + +@pytest.fixture(autouse=True) +def _run_in_tmp_path(tmp_path, monkeypatch): + """Every test stands in its own `tmp_path`, with `KEEL_HOME` pinned to it (#964). + + The working directory is where every relative path in a loaded config lands -- + `logging.file`'s `logs/keel.log` first among them, which `configure_logging` opens as given. + Fixing that one test at a time is a game of whack-a-mole: dozens of CLI tests load a config + with no `logging:` section, and the next one written will too. + + `KEEL_HOME` goes with it because leaving the checkout changes what `paths.state_root` picks. + The checkout holds a `config.yaml`, so it counted as a deployment root; a bare `tmp_path` + does not, and an unpinned lookup would fall through to the operator's REAL app-data + directory. Pinned, the writer (`configure_logging`, relative to the cwd) and the readers + (`resolve_under_state_root`) agree on one file inside the test's own directory. A test about + `state_root` itself unsets it or re-points it, as `tests/core/test_paths.py` already does. + + A test that reads a repository file opens it from `__file__`, never from the cwd.""" + monkeypatch.chdir(tmp_path) + monkeypatch.setenv("KEEL_HOME", str(tmp_path)) + # `python -c` children used to import keel from the cwd (the repo root); from `tmp_path` they + # would import whatever the venv's editable install points at -- the MAIN checkout, in a + # worktree that borrows its venv. Pin the tree under test, keeping any PYTHONPATH already set. + monkeypatch.setenv( + "PYTHONPATH", + os.pathsep.join(filter(None, [str(_REPO_ROOT), os.environ.get("PYTHONPATH")])), + ) diff --git a/tests/test_capabilities.py b/tests/test_capabilities.py index 76bf557b..4ba4d423 100644 --- a/tests/test_capabilities.py +++ b/tests/test_capabilities.py @@ -343,3 +343,26 @@ def test_no_row_or_gate_claims_a_browser_can_release_a_halt() -> None: assert cli in "\n".join(render_lines()) assert "terminal" in cap.increases, cap.key assert "gates token" in cap.increases, cap.key + + +def test_render_lines_names_the_surfaces_that_read_it() -> None: + """`render_lines` justified itself as the one renderer shared by "the CLI and the browser + view". The browser view went with the Gates page in #956, and the docstring went on naming + it (#967). It now names the modules that import it, and this keeps that list honest in both + directions: every importer named, and at least one.""" + from pathlib import Path + + keel_dir = Path(_ROOT) / "keel" + importers = set() + for path in sorted(keel_dir.rglob("*.py")): + for node in ast.walk(ast.parse(path.read_text())): + if ( + isinstance(node, ast.ImportFrom) + and node.module == "keel.capabilities" + and any(alias.name == "render_lines" for alias in node.names) + ): + importers.add(".".join(path.relative_to(_ROOT).with_suffix("").parts)) + + assert importers, "render_lines has a reader; this test reads the import graph to find it" + doc = render_lines.__doc__ or "" + assert sorted(m for m in importers if f"`{m}`" not in doc) == [] diff --git a/tests/test_cli.py b/tests/test_cli.py index 08b41b39..57681ad9 100644 --- a/tests/test_cli.py +++ b/tests/test_cli.py @@ -40,6 +40,11 @@ FIXTURES_DIR = Path(__file__).parent / "fixtures" / "transactions_dir" +#: The checkout's own `config.yaml`, which the simulate tests seed candles for and the telemetry +#: test loads. Named explicitly: every test now runs in its own `tmp_path` (#964), so a bare +#: invocation no longer falls through to the checkout's config. +REPO_CONFIG = Path(__file__).resolve().parent.parent / "config.yaml" + def _repo_at(db_path: Path) -> Repository: conn = connect(str(db_path)) @@ -638,16 +643,28 @@ def test_rules_list_shows_rule_needs_no_passphrase(tmp_path): assert "candidate" in result.output -def test_rules_backtest_with_no_candles_reports_zero_trades(tmp_path): +def test_rules_backtest_with_no_candles_reports_zero_trades(tmp_path, valid_config_path): db_path = tmp_path / "test.db" repo = _repo_at(db_path) rule_id = repo.insert_rule("pullback_continuation", {"product_id": "BTC-USD"}) runner = CliRunner() - result = runner.invoke(cli, ["--db", str(db_path), "rules", "backtest", str(rule_id)]) + result = runner.invoke( + cli, + [ + "--db", + str(db_path), + "--config", + str(valid_config_path), + "rules", + "backtest", + str(rule_id), + ], + ) assert result.exit_code == 0, result.output assert "n_trades=0" in result.output + assert "fee_pct=1.2000%" in result.output def test_rules_backtest_states_the_fee_rate_it_priced_fills_at(tmp_path, valid_config_path): @@ -705,14 +722,18 @@ def test_rules_backtest_prices_fills_at_the_configs_taker_rate(tmp_path, write_c assert "fee_pct=3.0000%" in result.output -def test_rules_backtest_unknown_rule_id_errors(tmp_path): +def test_rules_backtest_unknown_rule_id_errors(tmp_path, valid_config_path): db_path = tmp_path / "test.db" _repo_at(db_path) runner = CliRunner() - result = runner.invoke(cli, ["--db", str(db_path), "rules", "backtest", "999"]) + result = runner.invoke( + cli, + ["--db", str(db_path), "--config", str(valid_config_path), "rules", "backtest", "999"], + ) assert result.exit_code != 0 + assert "no rule with id 999" in result.output def test_rules_promote_stays_when_floor_not_cleared(tmp_path, valid_config_path): @@ -1356,7 +1377,9 @@ def test_rules_seed_respects_products_and_kinds_options(tmp_path, valid_config_p assert rows[0]["params"]["product_id"] == "BTC-USD" -def test_rules_seed_unknown_kind_errors(tmp_path): +def test_rules_seed_unknown_kind_errors(tmp_path, valid_config_path): + """Needs a real `--config`: `rules seed` loads config first, so without one the command + exits nonzero on 'file not found' and this passes whatever seed does with a bad kind.""" db_path = tmp_path / "test.db" runner = CliRunner() @@ -1365,6 +1388,8 @@ def test_rules_seed_unknown_kind_errors(tmp_path): [ "--db", str(db_path), + "--config", + str(valid_config_path), "rules", "seed", "--products", @@ -1375,6 +1400,7 @@ def test_rules_seed_unknown_kind_errors(tmp_path): ) assert result.exit_code != 0 + assert "unknown rule kind" in result.output def test_rules_seed_rows_round_trip_through_build_rule(tmp_path, valid_config_path): @@ -1542,6 +1568,8 @@ def _boom(config): # pragma: no cover -- only invoked if the test fails its own [ "--db", str(db_path), + "--config", + str(REPO_CONFIG), "simulate", "--no-fetch", "--years", @@ -1660,6 +1688,8 @@ def test_simulate_reports_per_product_slippage_beside_the_results(tmp_path, monk [ "--db", str(db_path), + "--config", + str(REPO_CONFIG), "simulate", "--no-fetch", "--years", @@ -1712,7 +1742,18 @@ def test_simulate_prints_floor_cap_and_anchor_beside_every_result(tmp_path, monk result = CliRunner().invoke( cli, - ["--db", str(db_path), "simulate", "--no-fetch", "--years", "1", "--out", str(out_path)], + [ + "--db", + str(db_path), + "--config", + str(REPO_CONFIG), + "simulate", + "--no-fetch", + "--years", + "1", + "--out", + str(out_path), + ], ) assert result.exit_code == 0, result.output @@ -1752,6 +1793,8 @@ def test_simulate_no_fetch_does_not_refetch_and_reuses_cached_db(tmp_path, monke [ "--db", str(db_path), + "--config", + str(REPO_CONFIG), "simulate", "--no-fetch", "--years", @@ -1776,6 +1819,8 @@ def test_simulate_artifact_flag_writes_html_next_to_markdown(tmp_path, monkeypat [ "--db", str(db_path), + "--config", + str(REPO_CONFIG), "simulate", "--no-fetch", "--years", @@ -1818,6 +1863,8 @@ def _boom(config): # pragma: no cover -- only invoked if the test fails its own [ "--db", str(db_path), + "--config", + str(REPO_CONFIG), "simulate", "--no-fetch", "--skip-within-cap", @@ -1856,6 +1903,8 @@ def test_simulate_no_fetch_default_runs_full_tier_matrix(tmp_path, monkeypatch): [ "--db", str(db_path), + "--config", + str(REPO_CONFIG), "simulate", "--no-fetch", "--years", @@ -1901,7 +1950,10 @@ def test_loading_config_binds_the_venue_for_telemetry(tmp_path: Path) -> None: assert telemetry.current_venue() is None - result = CliRunner().invoke(cli, ["--db", str(tmp_path / "keel.db"), "subscription", "show"]) + result = CliRunner().invoke( + cli, + ["--db", str(tmp_path / "keel.db"), "--config", str(REPO_CONFIG), "subscription", "show"], + ) assert result.exit_code == 0, result.output assert telemetry.current_venue() == DEFAULT_VENUE diff --git a/tests/test_paper_equities_profile.py b/tests/test_paper_equities_profile.py index dd6a8f2e..a00a25f9 100644 --- a/tests/test_paper_equities_profile.py +++ b/tests/test_paper_equities_profile.py @@ -1009,6 +1009,9 @@ def test_load_alpaca_secrets_reads_environment_then_env_file(tmp_path, monkeypat (tmp_path / ".env").write_text( "ALPACA_API_KEY_ID=file-key-id\nALPACA_API_SECRET_KEY=file-secret\n" ) + # The `.env` is found by standing in the folder, not through `KEEL_HOME` (which the suite + # pins to `tmp_path`, #964, and would keep finding this file after the move below). + monkeypatch.delenv("KEEL_HOME", raising=False) monkeypatch.chdir(tmp_path) monkeypatch.delenv("ALPACA_API_KEY_ID", raising=False) monkeypatch.delenv("ALPACA_API_SECRET_KEY", raising=False) diff --git a/tests/test_rule_manifest.py b/tests/test_rule_manifest.py index 1e81143d..f56696c0 100644 --- a/tests/test_rule_manifest.py +++ b/tests/test_rule_manifest.py @@ -18,10 +18,14 @@ if str(REPO_ROOT) not in sys.path: sys.path.insert(0, str(REPO_ROOT)) +import pytest # noqa: E402 + +from keel.commands.rules import RulesRefused, add_rule_row # noqa: E402 from keel.data.db import connect, migrate # noqa: E402 from keel.data.repository import Repository # noqa: E402 from keel.strategy.rules.dca import Dca # noqa: E402 from scripts.rule_manifest import apply, export # noqa: E402 +from tests.commands.test_rules_services import _config # noqa: E402 DCA = { "kind": "dca", @@ -131,6 +135,66 @@ def test_rules_match_on_kind_and_product(tmp_path: Path) -> None: assert {r["params"]["product_id"] for r in _rules(db)} == {"BTC-USD", "ETH-USD"} +TURTLE_NAMED_DCA = { + "kind": "turtle_breakout", + "status": "candidate", + "params": {"product_id": "BTC-USD", "name": "dca"}, +} + + +@pytest.mark.parametrize( + "entry", + [ + TURTLE_NAMED_DCA, + {**DCA, "params": {**DCA["params"], "name": "dca"}}, + { + "kind": "rsi_meanrev", + "status": "candidate", + "params": {"product_id": "BTC-USD", "name": "dca"}, + }, + ], + ids=lambda entry: entry["kind"], +) +def test_apply_refuses_a_name_param_and_writes_nothing( + tmp_path: Path, capsys: pytest.CaptureFixture[str], entry: dict +) -> None: + """#974. `rule.name` is what `positions.rule_name` records, and #961 reads a stop-less kind's + name as "no stop by design". A manifest that stored `"name": "dca"` on a turtle would make that + turtle's missing bracket read as deliberate -- the one direction that hides a gap. `keel rules + add` refuses a `name` param; `apply` is the other writer of `rules`, so it refuses the same + way, with the same words, and inserts nothing for that entry.""" + db = _db(tmp_path) + manifest = _manifest(tmp_path, [entry]) + + assert apply(db, manifest, write=True) == 1 + assert _rules(db) == [] + + err: list[str] = [] + with pytest.raises(RulesRefused): + add_rule_row( + Repository(connect(":memory:")), + _config(), + kind=entry["kind"], + product=entry["params"]["product_id"], + params_json=json.dumps({k: v for k, v in entry["params"].items() if k != "product_id"}), + now_ts=0, + echo_err=err.append, + ) + stderr = capsys.readouterr().err + assert "REFUSED" in stderr + assert err and all(line.removeprefix("Error: ") in stderr for line in err), (err, stderr) + + +def test_a_refused_entry_does_not_stop_the_valid_ones(tmp_path: Path) -> None: + """The refusal is per entry, as `--allow-live`'s is: the rest of the manifest still applies, + and the exit code says the database does not match the file.""" + db = _db(tmp_path) + manifest = _manifest(tmp_path, [TURTLE_NAMED_DCA, DCA]) + + assert apply(db, manifest, write=True) == 1 + assert [(r["kind"], r["params"]) for r in _rules(db)] == [("dca", DCA["params"])] + + def test_committed_manifest_is_valid(tmp_path: Path) -> None: """The checked-in manifest must actually rebuild -- a stale or malformed one is worthless.""" committed = REPO_ROOT / "deploy" / "live-rules.json" diff --git a/tests/test_subprocess_imports_tree_under_test.py b/tests/test_subprocess_imports_tree_under_test.py new file mode 100644 index 00000000..3641fa3a --- /dev/null +++ b/tests/test_subprocess_imports_tree_under_test.py @@ -0,0 +1,25 @@ +"""A `python -c` child must import keel from the tree under test (#978 review F2). + +`conftest._run_in_tmp_path` moves every test's cwd to `tmp_path`, so a child no longer finds the +repo root through `''` on `sys.path`; it would import whatever the venv's editable install points +at -- the main checkout, in a worktree that borrows its venv -- and the four `-c` tests would +silently check a different tree's code.""" + +from __future__ import annotations + +import subprocess +import sys +from pathlib import Path + +_REPO_ROOT = Path(__file__).resolve().parent.parent + + +def test_a_python_dash_c_child_imports_keel_from_this_repo() -> None: + out = subprocess.run( + [sys.executable, "-c", "import keel; print(keel.__file__)"], + capture_output=True, + text=True, + check=True, + ).stdout.strip() + + assert Path(out).resolve().is_relative_to(_REPO_ROOT) diff --git a/tests/web/test_timeline_export.py b/tests/web/test_timeline_export.py index c0b9c142..391ce8b5 100644 --- a/tests/web/test_timeline_export.py +++ b/tests/web/test_timeline_export.py @@ -20,13 +20,43 @@ from pathlib import Path from typing import Any +import pytest + from keel.data.db import connect, migrate from keel.data.repository import Repository -from tests.web.test_api import _get, _session, deployment, running # noqa: F401 +from tests.conftest import VALID_CONFIG_YAML +from tests.web.test_api import _get, _session, running # noqa: F401 EXPORT = "/api/timeline/export.csv" +def _config_with_a_pinned_log(tmp_path: Path) -> Path: + """A config whose `logging.file` is ABSOLUTE, inside this test's own directory (#964). + + Left at the default, `logs/keel.log` resolves against whatever the state root is, so these + tests read whichever engine log happened to sit there -- and an empty one, left by an earlier + run, turned the first line of every export into `# NOTE: the engine log could not be read + (empty)`. Pinned to a file nothing writes, the log is `missing`: the export's settled no-log + answer, which puts no note above the header.""" + config_path = tmp_path / "config.yaml" + config_path.write_text( + VALID_CONFIG_YAML + f"\nlogging:\n file: {tmp_path / 'pinned-engine.log'}\n" + ) + return config_path + + +@pytest.fixture +def deployment(tmp_path: Path) -> tuple[str, str]: + """`test_api.deployment`, with the engine log pinned (see `_config_with_a_pinned_log`). + `running` there asks for `deployment` by name, so this module's own fixture is the one it + binds.""" + db_path = tmp_path / "keel.db" + conn = connect(str(db_path)) + migrate(conn) + conn.close() + return str(db_path), str(_config_with_a_pinned_log(tmp_path)) + + def _csv(server: Any, path: str = EXPORT) -> tuple[int, dict[str, str], str]: status, headers, body = _get(server, path, cookie=_session(server)) return status, {k.lower(): v for k, v in headers.items()}, body @@ -261,7 +291,7 @@ def test_the_export_names_the_chain_status_beside_the_hash(running: Any) -> None def test_the_export_filters_by_the_composite_kind(tmp_path: Path) -> None: """Console PR4: the Audit page opens on `kind=keel`, and its CSV link carries that kind -- so the file has to hold exactly what the screen does: no `trade` and no `flow` rows.""" - from tests.web.test_api import VALID_CONFIG_YAML, _bind + from tests.web.test_api import _bind db_path = tmp_path / "keel.db" conn = connect(str(db_path)) @@ -301,24 +331,21 @@ def test_the_export_filters_by_the_composite_kind(tmp_path: Path) -> None: attested_at=1_700_000_200, ) conn.close() - config_path = tmp_path / "config.yaml" - config_path.write_text(VALID_CONFIG_YAML) + config_path = _config_with_a_pinned_log(tmp_path) for server in _bind(str(db_path), str(config_path)): _status, _headers, everything = _csv(server, EXPORT + "?scope=all") status, _headers, body = _csv(server, EXPORT + "?scope=all&kind=keel") def kinds(text: str) -> list[str]: - # Past any `# NOTE` line above the header (a log gap or a broken chain), then by name. - rows = [row for row in csv.reader(io.StringIO(text)) if not row[0].startswith("# NOTE")] - kind_at = rows[0].index("kind") + rows = list(csv.reader(io.StringIO(text))) + kind_at = rows[0].index("kind") # the pinned log is missing: no `# NOTE` above it return [row[kind_at] for row in rows[1:]] every_kind = kinds(everything) keel_only = kinds(body) assert status == 200 - # Cycle (`system`) rows come from whatever engine log the config resolves relative to the - # working directory (#964), so they may or may not appear; the seeded stores always do. - assert {"attestation", "flow", "trade"} <= set(every_kind), "the fixture reached the file" - assert "attestation" in keel_only - assert set(keel_only) <= {"system", "attestation"}, keel_only + # The engine log is pinned to a file nothing writes (#964), so there are no cycle (`system`) + # rows to filter, and each seeded store reaches the file exactly once. + assert sorted(every_kind) == ["attestation", "flow", "trade"], "the fixture reached the file" + assert keel_only == ["attestation"], keel_only diff --git a/tests/web/test_web_prose_stays_true.py b/tests/web/test_web_prose_stays_true.py new file mode 100644 index 00000000..f5d5a8d5 --- /dev/null +++ b/tests/web/test_web_prose_stays_true.py @@ -0,0 +1,209 @@ +"""Counts and names in `keel/web/` prose, checked against the code they describe (#967). + +Each of these was a sentence that had been true once and was still being read: a refresh constant +whose pages were deleted at #540, a "seven" that became eight when a capability row was added, +and a "four checks" that missed `Sec-Fetch-Site`. A phrase ban would catch only the exact wording +that went stale. These tests derive the number or the name from the code instead, so the next +edit that changes the code fails here until the prose is changed with it. +""" + +from __future__ import annotations + +import ast +import importlib +import inspect +import re +import textwrap +from pathlib import Path + +from keel.capabilities import BROWSER, CAPABILITIES +from keel.web import gates, server + +WEB = Path(server.__file__).resolve().parent +KEEL = WEB.parent + +_NUMBER_WORDS = { + "two": 2, + "three": 3, + "four": 4, + "five": 5, + "six": 6, + "seven": 7, + "eight": 8, + "nine": 9, + "ten": 10, + "eleven": 11, + "twelve": 12, +} + +_PRIVATE_CONSTANT = re.compile(r"_[A-Z][A-Z0-9_]*") + + +def _module_constants(tree: ast.Module) -> list[str]: + names: list[str] = [] + for node in tree.body: + if isinstance(node, ast.Assign): + names += [t.id for t in node.targets if isinstance(t, ast.Name)] + elif isinstance(node, ast.AnnAssign) and isinstance(node.target, ast.Name): + names.append(node.target.id) + return [name for name in names if _PRIVATE_CONSTANT.fullmatch(name)] + + +def test_every_private_constant_in_keel_web_has_a_reader_in_its_module() -> None: + """A private constant nothing reads is a comment pretending to be configuration. + `server._REFRESH_SEC` described pages that reload themselves; #540 removed the pages and the + constant stayed, still answering "how often does the console refresh" with a number that + drives nothing (the client polls on `main.js`'s own `POLL_MS`).""" + dead: dict[str, list[str]] = {} + for path in sorted(WEB.glob("*.py")): + tree = ast.parse(path.read_text()) + loaded = { + node.id + for node in ast.walk(tree) + if isinstance(node, ast.Name) and isinstance(node.ctx, ast.Load) + } + unread = [name for name in _module_constants(tree) if name not in loaded] + if unread: + dead[path.name] = unread + + assert dead == {} + + +def test_a_cited_private_server_constant_exists() -> None: + """Prose elsewhere in keel that names `server._SOMETHING` is naming a number a reader will + go and look up. `events.HEARTBEAT_SEC` and `api._SCREEN_CACHE_TTL_SECONDS` both justified + themselves against `server._REFRESH_SEC` after it stopped meaning anything.""" + cited = { + (path.relative_to(KEEL.parent).as_posix(), match.group(1)) + for path in sorted(KEEL.rglob("*.py")) + for match in re.finditer(r"`server\.(_[A-Z][A-Z0-9_]*)`", path.read_text()) + } + missing = sorted((where, name) for where, name in cited if not hasattr(server, name)) + + assert missing == [] + + +_FILE_SUFFIXES = {"py", "js", "css", "html", "json", "yaml", "md"} + + +def _cited_web_references() -> list[tuple[str, int, str, str]]: + """Every backticked `module.name` in `keel/**/*.py` whose module is a `keel.web` module. + + A bare stem is read as the SIBLING first (a `server.X` in `keel/web/` is `keel.web.server`); + outside `keel/web/` it is read only where no other keel module shares the stem (`server` is + also `keel.mcp.server`, so a citation there is not this test's to judge). `api.js` and + `payload.py` name files, not attributes, and are skipped by their suffix.""" + web_stems = {path.stem for path in WEB.glob("*.py") if path.stem != "__init__"} + others: dict[str, int] = {} + for path in KEEL.rglob("*.py"): + if path.parent != WEB: + others[path.stem] = others.get(path.stem, 0) + 1 + found: list[tuple[str, int, str, str]] = [] + for path in sorted(KEEL.rglob("*.py")): + sibling = path.parent == WEB + for number, line in enumerate(path.read_text().splitlines(), 1): + for match in re.finditer(r"`(\w+)\.(\w+)(?:\(\))?`", line): + module, name = match.groups() + if module not in web_stems or name in _FILE_SUFFIXES: + continue + if not sibling and others.get(module): + continue + found.append((path.relative_to(KEEL.parent).as_posix(), number, module, name)) + return found + + +def _resolves(module: object, name: str) -> bool: + """A module-level name, or a method of a class the module defines: the prose says + `server._gated_action` for `KeelHandler._gated_action` and `server.shutdown()` for the + `shutdown` that `KeelServer` inherits, and both are findable from there.""" + if hasattr(module, name): + return True + classes = [ + value + for value in vars(module).values() + if inspect.isclass(value) and value.__module__ == getattr(module, "__name__", "") + ] + return any(hasattr(cls, name) for cls in classes) + + +def test_a_cited_web_module_attribute_exists() -> None: + """Prose that says `server.needs_database` or `server.ROUTES` sends a reader to a name to + look up. Both were deleted with the renderer (#540) and were still cited, as was + `payload._order_payload` (now `_order_row_payload`). Only the `keel.web` modules are + resolved: elsewhere `config.quote_currency`, `orders.rule_id` and `agent.enter_evaluated` + name columns, settings and event kinds, which no import can check.""" + cited = _cited_web_references() + assert cited, "the scan found no citations at all; the pattern has stopped matching" + missing = [ + f"{where}:{number} `{module}.{name}`" + for where, number, module, name in cited + if not _resolves(importlib.import_module(f"keel.web.{module}"), name) + ] + + assert missing == [] + + +def _stated_count(text: str, pattern: str) -> int: + match = re.search(pattern, " ".join(text.split())) + assert match, f"the sentence matching {pattern!r} is gone; re-point this test at its successor" + return _NUMBER_WORDS[match.group(1).lower()] + + +def test_the_gates_module_counts_the_actions_it_refuses_from_the_inventory() -> None: + """`gates`' docstring says how many capability increases the browser gate refuses outright. + That is every action in `keel.capabilities` that no `web` row mirrors -- eight of the ten + since the inventory grew, while the docstring still said seven.""" + browser_mirrored = {cap.mirrors for cap in CAPABILITIES if cap.gate == BROWSER.name} + terminal_only = [ + cap + for cap in CAPABILITIES + if cap.gate != BROWSER.name and (cap.module, cap.function) not in browser_mirrored + ] + + stated = _stated_count(gates.__doc__ or "", r"The other (\w+) are refused") + + assert len(terminal_only) == stated + + +def _refusing_guards(method: object, *, stop_at: str | None = None) -> list[ast.If]: + """The top-level `if not ...: return` guards of a handler method, in order, up to (not + including) the statement whose source mentions `stop_at`.""" + tree = ast.parse(textwrap.dedent(inspect.getsource(method))) # type: ignore[arg-type] + function = tree.body[0] + assert isinstance(function, ast.FunctionDef) + guards: list[ast.If] = [] + for statement in function.body: + if isinstance(statement, ast.Expr) and isinstance(statement.value, ast.Constant): + continue # the docstring, which names `_gated_action` itself + if stop_at is not None and stop_at in ast.unparse(statement): + return guards + if isinstance(statement, ast.If) and isinstance(statement.test, ast.UnaryOp): + if isinstance(statement.test.op, ast.Not): + guards.append(statement) + assert stop_at is None, f"{stop_at!r} was never reached" + return guards + + +def test_the_phrase_check_names_its_place_among_every_check_before_it() -> None: + """`run_gated_action` is the LAST check a release passes. Its docstring says how many come + first and which; both are read here from the two handler methods that run them, so a check + added to `do_POST` (as `Sec-Fetch-Site` was) fails this until the docstring counts it.""" + handler = server.KeelHandler + before_dispatch = _refusing_guards(handler.do_POST, stop_at="_gated_action") + in_gated_action = _refusing_guards(handler._gated_action) + checks = len(before_dispatch) + len(in_gated_action) + 1 # + the phrase itself + + doc = inspect.getdoc(gates.run_gated_action) or "" + assert _stated_count(doc, r"LAST of (\w+) checks") == checks + # the module docstring counts the same checks: it once said "three" three paragraphs above + # the "seven" here, so a reader got two numbers for one gate + assert _stated_count(gates.__doc__ or "", r"weakest of the (\w+) checks") == checks + + called = { + node.func.attr if isinstance(node.func, ast.Attribute) else node.func.id + for guard in [*before_dispatch, *in_gated_action] + for node in ast.walk(guard.test) + if isinstance(node, ast.Call) and isinstance(node.func, ast.Attribute | ast.Name) + } - {"startswith", "get", "tokens_match"} + unnamed = sorted(name for name in called if name not in doc) + assert unnamed == [], f"run_gated_action's docstring does not name {unnamed}"