diff --git a/keel/agent.py b/keel/agent.py index 6bd9f01d..6541a631 100644 --- a/keel/agent.py +++ b/keel/agent.py @@ -60,7 +60,7 @@ from collections.abc import Callable from dataclasses import dataclass, field from decimal import Decimal -from typing import Any +from typing import Any, Literal from keel_core.products import quote_currency_of from keel_core.telemetry import bind_cycle, log_event, new_cycle_id, unbind_cycle @@ -309,9 +309,16 @@ def _open_tranche( rails vetoed. Neither is an error here -- the tranche is real either way, and reconciliation simply has no bracket to resolve back to it until one is placed. """ - entry_fill = None if order is None else order["actual_fill"] - qty = None if order is None else order["qty"] - if entry_fill is None or qty is None: + # `order is None` is folded into the one guard below rather than tested separately, so the + # checker can narrow `order` for the `order["fee"]` read after it. Testing only the two + # extracted values left `order` typed `dict | None` all the way down, even though a `None` + # order forces `entry_fill`/`qty` to `None` and returns here. + if order is None: + entry_fill = qty = None + else: + entry_fill = order["actual_fill"] + qty = order["qty"] + if order is None or entry_fill is None or qty is None: log_event( logger, logging.WARNING, @@ -478,6 +485,11 @@ def _seed_paper_account_if_needed( repo.set_state("equity_state_mode", "paper") if paper_trader.get_cash() is None: funding = config.paper.starting_equity_usd + # Declared up front: inferring from the first branch pins `seed` to `Decimal`, and the + # mark-to-market fallback below can legitimately yield `None` (unreadable broker). The + # `is None` check in that branch still returns before `seed_cash`, so this only widens + # the declaration to match what the two branches actually produce. + seed: Decimal | None if funding > 0: seed = funding else: @@ -593,7 +605,10 @@ def _handle_exits( repo: Repository, broker: Any, config: Config, - mode: str, + # Same `Literal` pair as `_effective_mode`'s return and `executor.execute`'s parameter: this + # only ever receives the former and only ever forwards to the latter, so a bare `str` here + # was the one gap that let an unchecked value through the middle of that path. + mode: Literal["confirm", "autonomous"], now_ts: int, confirm_fn: executor.ConfirmFn | None = None, ) -> list[ExecutionResult]: @@ -769,9 +784,17 @@ class LoopResult: drawdown_weekly_pct: Decimal | None = None -def _effective_mode(config: Config, repo: Repository, now_ts: int) -> str: +def _effective_mode( + config: Config, repo: Repository, now_ts: int +) -> Literal["confirm", "autonomous"]: """The executor mode for this cycle: `"autonomous"` or `"confirm"`. + The return type is the same `Literal` pair `executor.execute` accepts, not a bare `str`: + those two values are all this can return (see the fail-toward-`"confirm"` note below), and + the wider annotation meant the two `execute(...)` call sites in this module were passing an + unchecked `str` into a `Literal` parameter. `config.auto_trade.mode` stays a `str` -- it has + a third value, `"paper"`, which never reaches an executor mode at all. + Two independent switches, deliberately not conflated into one enum: * `config.auto_trade.mode` says whether this is real money at all (`paper` never reaches an diff --git a/keel/analysis/levels.py b/keel/analysis/levels.py index 5369ef99..4e8775d2 100644 --- a/keel/analysis/levels.py +++ b/keel/analysis/levels.py @@ -91,7 +91,11 @@ def _cluster_pivots( clusters: list[list[tuple[int, Decimal]]] = [[ordered[0]]] for pivot in ordered[1:]: current = clusters[-1] - avg = sum(price for _, price in current) / len(current) + # `Decimal(0)` start: a bare `sum()` starts from an `int` 0, which types the running + # average as `Decimal | float` and carries that widening into `Level.price` below. Every + # cluster holds at least the pivot that opened it, so the start value is never actually + # summed -- this pins the type without changing a single computed number. + avg = sum((price for _, price in current), Decimal(0)) / len(current) if abs(pivot[1] - avg) <= avg * tolerance: current.append(pivot) else: @@ -99,7 +103,7 @@ def _cluster_pivots( levels = [] for cluster in clusters: - avg_price = sum(price for _, price in cluster) / len(cluster) + avg_price = sum((price for _, price in cluster), Decimal(0)) / len(cluster) touches = _distinct_touches([ts for ts, _ in cluster], min_separation_sec) levels.append(Level(price=avg_price, kind=kind, touches=touches, angular=False)) return levels diff --git a/keel/cli.py b/keel/cli.py index 76fcd589..d1575c7d 100644 --- a/keel/cli.py +++ b/keel/cli.py @@ -1816,14 +1816,18 @@ def _build_account_metrics( per_asset_pnl[trade.asset] = per_asset_pnl.get(trade.asset, Decimal("0")) + ( trade.pnl or Decimal("0") ) + # `exit_ts` is optional on the trade type because an OPEN trade has none; `closed_trades` + # has already excluded those, so every entry here carries one. Written as a filter rather + # than left implicit so the average is taken over exactly the trades that can contribute a + # duration -- matching the defensive `trade.pnl or Decimal("0")` two lines up, and keeping + # a stray `None` from reaching `Decimal(None - entry_ts)`. + hold_spans = [ + Decimal(t.exit_ts - t.entry_ts) / Decimal(3600) + for t in closed_trades + if t.exit_ts is not None + ] avg_hold_hours = ( - sum( - (Decimal(t.exit_ts - t.entry_ts) / Decimal(3600) for t in closed_trades), - Decimal("0"), - ) - / len(closed_trades) - if closed_trades - else Decimal("0") + sum(hold_spans, Decimal("0")) / len(hold_spans) if hold_spans else Decimal("0") ) return { diff --git a/keel/commands/insights.py b/keel/commands/insights.py index d9ceccdf..52b8e8e8 100644 --- a/keel/commands/insights.py +++ b/keel/commands/insights.py @@ -310,12 +310,17 @@ def _match_trade(entries_for_rule: list[Any], opened_at: int, closed_at: int) -> def _journal_entry_from_outcome( row: dict[str, Any], trades_by_rule: dict[str, list[Any]], repo: Repository ) -> JournalEntry: + # `outcome` is resolved through a separate `matched_outcome` local, and declared `str` here, + # so that `JournalEntry.outcome` (a `str`) receives a value the checker agrees is one. Read + # straight off `matched` -- which `_match_trade` types `Any` -- it stayed `str | None` all + # the way to the constructor even though the fallback below leaves no `None` path. + outcome: str if row["is_dca"]: r_multiple = None outcome = "dca" else: r_multiple = None - outcome = None + matched_outcome: str | None = None rule_name = row["rule_name"] if rule_name: if rule_name not in trades_by_rule: @@ -323,10 +328,11 @@ def _journal_entry_from_outcome( matched = _match_trade(trades_by_rule[rule_name], row["opened_at"], row["closed_at"]) if matched is not None: r_multiple = matched.r_multiple - outcome = matched.outcome - if outcome is None: + matched_outcome = matched.outcome + if matched_outcome is None: pnl = row["pnl_net"] - outcome = "win" if pnl > 0 else "loss" if pnl < 0 else "scratch" + matched_outcome = "win" if pnl > 0 else "loss" if pnl < 0 else "scratch" + outcome = matched_outcome return JournalEntry( closed_at=row["closed_at"], diff --git a/keel/commands/rules.py b/keel/commands/rules.py index 3da0daa0..e37c30cc 100644 --- a/keel/commands/rules.py +++ b/keel/commands/rules.py @@ -461,13 +461,19 @@ def rules_seed( seeded: list[str] = [] skipped: list[str] = [] for kind in kind_list: - rule_cls = agent.RULE_REGISTRY[kind] for product in product_list: label = f"{kind}:{product}" if not force and (kind, product) in existing_keys: skipped.append(label) continue - rule = rule_cls(product_id=product) + # Via `build_rule_from_params` rather than `RULE_REGISTRY[kind](product_id=...)`: + # that function is documented as THE `(kind, params)` -> `Rule` boundary, and this + # was the one caller reaching around it. With `product_id` as the only param none + # of its coercion tables apply, so it calls the very same constructor -- but a rule + # kind that later needs coercion for a seeded default gets it here for free. Its + # unknown-kind `ValueError` cannot fire: `kind_list` is checked against the registry + # above and exits non-zero before this loop. + rule = agent.build_rule_from_params(kind, {"product_id": product}) params = _json_plain(rule.describe()["params"]) params["product_id"] = product repo.insert_rule(kind, params, status=status, now_ts=now_ts) @@ -515,7 +521,11 @@ def _declared_choices(rule_cls: type) -> dict[str, tuple[Any, ...]]: exception: an un-checkable param is the status quo, a crashing `rules add` is not. """ try: - hints = get_type_hints(rule_cls.__init__) + # `type: ignore[misc]`: mypy rejects reading `__init__` off a value because a subclass + # could carry an incompatible one -- which is precisely what is being introspected here. + # Reaching for the constructor of whatever concrete rule class was passed IS the job of + # this function (see the docstring), so the unsoundness it warns about is the intent. + hints = get_type_hints(rule_cls.__init__) # type: ignore[misc] except (NameError, TypeError): # an unresolvable forward ref, or a slot wrapper __init__ return {} diff --git a/keel/commands/tui.py b/keel/commands/tui.py index 4ea8bb64..adee9132 100644 --- a/keel/commands/tui.py +++ b/keel/commands/tui.py @@ -251,8 +251,13 @@ def _autonomy_lines(report: StatusReport) -> list[ScreenLine]: else: lines.append(ScreenLine("autonomy: off", "muted")) if a.autonomous and not a.live: - lapsed_text = f" (was ON but LAPSED at {_human_dt(a.autonomous_until)})" - lines.append(ScreenLine(lapsed_text, "muted")) + # An expiry is what makes this branch reachable (autonomy recorded ON, its deadline + # passed), so `autonomous_until` is set here in practice. Guarded anyway because the + # failure is silent rather than loud: `_human_dt(None)` does not raise -- + # `time.localtime(None)` means "now" -- so a missing deadline would render as having + # lapsed at this very instant, which reads as fact. + lapsed_at = _human_dt(a.autonomous_until) if a.autonomous_until is not None else "unknown" + lines.append(ScreenLine(f" (was ON but LAPSED at {lapsed_at})", "muted")) elif a.live and a.autonomous_until is not None: lines.append(ScreenLine(f" lapses at {_human_dt(a.autonomous_until)}", "muted")) return lines @@ -313,9 +318,12 @@ def _available_lines(available: AvailableBalance | None) -> list[ScreenLine]: if available is None: return [] if available.amount is not None: + # `updated_ts` is a separate field from `amount` and can be absent while the amount is + # present; same silent-"now" hazard as the autonomy line above, and on a freshness + # stamp specifically, where a wrong value is worse than an admitted missing one. + as_of = _human_dt(available.updated_ts) if available.updated_ts is not None else "unknown" text = ( - f"live account: {available.amount:,.2f} {available.quote} available " - f"({_human_dt(available.updated_ts)})" + f"live account: {available.amount:,.2f} {available.quote} available ({as_of})" ) return [ScreenLine(text, "ok")] return [ScreenLine(f"live account: unavailable -- {available.error}", "warn")] diff --git a/keel/sim/portfolio_sim.py b/keel/sim/portfolio_sim.py index b32efb5f..3abf0cdc 100644 --- a/keel/sim/portfolio_sim.py +++ b/keel/sim/portfolio_sim.py @@ -561,7 +561,11 @@ def _process_rule_signals( account: SimAccount, config: Config, latest_price: dict[str, Decimal], - held: dict[str, _Held], + # Keyed by (asset, rule_name) -- one slot per RULE per asset, matching `run()`'s own `held` + # and `_process_dca_signals`. This said `dict[str, _Held]`, which no caller ever passed and + # which contradicted both the `(asset, signal.rule_name)` membership test and the assignment + # under the same key in this function's body. + held: dict[tuple[str, str], _Held], now_ts: int, monthly_volume_cap: Decimal | None = None, ) -> bool: diff --git a/keel/sim/report.py b/keel/sim/report.py index a5dea1ae..955de428 100644 --- a/keel/sim/report.py +++ b/keel/sim/report.py @@ -401,7 +401,7 @@ def _would_have_traded_gaps(telemetry: SimTelemetry) -> list[GapItem]: def _data_coverage_gaps(coverage: dict) -> list[GapItem]: - gaps = [] + gaps: list[GapItem] = [] if not coverage: return gaps diff --git a/keel/strategy/backtest.py b/keel/strategy/backtest.py index 1997abec..35cbc626 100644 --- a/keel/strategy/backtest.py +++ b/keel/strategy/backtest.py @@ -51,7 +51,7 @@ from dataclasses import dataclass from decimal import Decimal -from keel.strategy.rules.base import Rule, Setup, Trade +from keel.strategy.rules.base import Rule, Setup, Trade, TradeOutcome from keel.strategy.stats import BacktestResult, summarize from keel.types import Candle, Granularity, Side @@ -181,6 +181,10 @@ def _close_trade( risk = (entry_fill - position.setup.stop) * qty r_multiple = pnl / risk if risk != 0 else None + # Annotated rather than inferred: without it the three branches widen `outcome` to plain + # `str`, which `Trade.outcome` then rejects. Naming the alias also catches a typo in one of + # these literals here, at the assignment, instead of at the constructor call below. + outcome: TradeOutcome if pnl > 0: outcome = "win" elif pnl < 0: diff --git a/keel/strategy/rules/base.py b/keel/strategy/rules/base.py index c65703be..77f05a90 100644 --- a/keel/strategy/rules/base.py +++ b/keel/strategy/rules/base.py @@ -71,6 +71,13 @@ class Signal: rule_id: int | None = None +#: How a trade ended. Named rather than spelled out at each use so the producer +#: (`backtest._closed_trade`, which picks the branch) and the consumer (`Trade.outcome`) cannot +#: drift: an inline `Literal[...]` repeated in both places lets one side gain a value the other +#: silently rejects, and the mismatch only shows up as an `arg-type` error at the constructor. +type TradeOutcome = Literal["win", "loss", "open", "scratch"] + + @dataclass class Trade: """A backtest/paper fill pair (entry + optional exit).""" @@ -85,7 +92,7 @@ class Trade: r_multiple: Decimal | None mfe: Decimal mae: Decimal - outcome: Literal["win", "loss", "open", "scratch"] + outcome: TradeOutcome class Rule(ABC): @@ -106,6 +113,13 @@ class Rule(ABC): name: str params: dict + #: The product this rule instance trades. Declared here because it was already a de-facto + #: part of the interface: every concrete rule takes it as its first constructor argument and + #: stores it (`PullbackContinuation`, `Dca` and `TurtleBreakout` assign it; `RsiMeanReversion` + #: carries it as a dataclass field), and `sim.portfolio_sim`/`sim.report` read + #: `rule.product_id` off rules they hold only as this base type. Annotation only -- no + #: default, exactly like `name`/`params`, so nothing about construction changes. + product_id: str promotion_class: str = "default" rule_id: int | None = None #: Why the last `detect()` call declined, or `None` if it fired (or never recorded one). diff --git a/keel/strategy/rules/turtle_breakout.py b/keel/strategy/rules/turtle_breakout.py index 460e2de1..b4396931 100644 --- a/keel/strategy/rules/turtle_breakout.py +++ b/keel/strategy/rules/turtle_breakout.py @@ -161,12 +161,18 @@ def __init__( # Rule interface # ------------------------------------------------------------------ - def _decline(self, gate: str, **numbers: object) -> None: + def _decline(self, gate: str, **numbers: object) -> Setup | None: """Record WHY this bar declined on `last_rejection`, and return `None` for `detect()`. Returning `None` (rather than setting and letting the caller `return None`) keeps each decline a single line, so the reason can never drift from the branch that produced it. + The return type is `Setup | None` rather than the `None` this always returns, because + all 8 call sites are `return self._decline(...)` and mypy rejects using the value of a + `-> None` call at all (`func-returns-value`) -- the annotation names what the CALLER + returns, which is what makes the one-line idiom above type-check. It never returns a + `Setup`; declining is the only thing it does. + This only ever writes an attribute -- it does NOT log. `detect()` runs once per bar in `strategy.backtest` and `sim.portfolio_sim`, so logging here would emit millions of events in a sim; `engine.evaluate` reads the attribute and folds it into the one @@ -346,11 +352,16 @@ def _prior_breakout_won(self, daily: list[Candle]) -> bool: warmup = max(entry_lookback, adx_period, atr_period) + 1 won = False - pos_entry: Decimal | None = None - pos_stop: Decimal | None = None + # (entry, stop) as ONE optional pair rather than two independently-optional locals: + # they are set together on entry and cleared together on exit, and every read of the + # stop happens on a bar where the entry is also live. As two variables that coupling + # was invisible -- narrowing `pos_entry is None` told the checker nothing about + # `pos_stop`, so the protective-stop comparison below read a `Decimal | None`. Pairing + # them makes "stopped without an entry" unrepresentable instead of merely unreachable. + position: tuple[Decimal, Decimal] | None = None for i in range(warmup, len(tail)): c = tail[i] - if pos_entry is None: + if position is None: # entry mirrors detect(): close > prior-entry_lookback Donchian high, ADX>thr, # optional MACD>0, valid 2N stop. if not float(c.close) > donchian_high(tail[:i], entry_lookback): @@ -370,15 +381,16 @@ def _prior_breakout_won(self, daily: list[Candle]) -> bool: stop_px = entry_px - stop_mult * atr_i if stop_px >= entry_px: continue - pos_entry, pos_stop = entry_px, stop_px + position = (entry_px, stop_px) else: # manage: 2N stop first (protective), then the asymmetric channel-low exit. + pos_entry, pos_stop = position if c.low <= pos_stop: won = False # stopped out below entry - pos_entry = pos_stop = None + position = None elif float(c.close) <= donchian_low(tail[:i], exit_lookback): won = c.close > pos_entry - pos_entry = pos_stop = None + position = None self._filter_cache = (last_ts, won) return won diff --git a/keel/strategy/stats.py b/keel/strategy/stats.py index 907600dc..46007d76 100644 --- a/keel/strategy/stats.py +++ b/keel/strategy/stats.py @@ -43,6 +43,28 @@ class BacktestResult: avg_mae: Decimal +def _closed_pnl(trade: Trade) -> Decimal: + """`trade`'s realised P&L, with the closed-trade invariant stated instead of assumed. + + `Trade.pnl` is `Decimal | None` because an OPEN trade has no realised P&L yet. Every other + outcome is produced by a close path that sets it (`backtest._closed_trade`; `paper` does the + same), and `summarize` filters to `outcome != "open"` before computing any aggregate -- so + within those aggregates `pnl` is never `None`. Nothing in the type system said so, which left + every `sum()` below summing `Decimal | None`. + + Asserting it here rather than at each call site buys two things: the aggregates come out + typed `Decimal` instead of `Decimal | None`, and a violation surfaces as a named error + naming the offending outcome, rather than as a `TypeError: unsupported operand type(s) for + +: 'decimal.Decimal' and 'NoneType'` raised from inside a generator with no trade in hand. + """ + if trade.pnl is None: + raise ValueError( + f"trade with outcome={trade.outcome!r} has pnl=None; only an open trade may " + "omit realised P&L, and open trades are excluded from these aggregates" + ) + return trade.pnl + + def summarize(trades: list[Trade]) -> BacktestResult: """Aggregate `trades` into a `BacktestResult`. @@ -71,12 +93,14 @@ def summarize(trades: list[Trade]) -> BacktestResult: losses = [t for t in closed if t.outcome == "loss"] win_rate = len(wins) / n_trades - avg_win = (sum((t.pnl for t in wins), Decimal(0)) / len(wins)) if wins else Decimal(0) - avg_loss = (sum((t.pnl for t in losses), Decimal(0)) / len(losses)) if losses else Decimal(0) - expectancy = sum((t.pnl for t in closed), Decimal(0)) / n_trades + avg_win = (sum((_closed_pnl(t) for t in wins), Decimal(0)) / len(wins)) if wins else Decimal(0) + avg_loss = ( + (sum((_closed_pnl(t) for t in losses), Decimal(0)) / len(losses)) if losses else Decimal(0) + ) + expectancy = sum((_closed_pnl(t) for t in closed), Decimal(0)) / n_trades - gross_profit = sum((t.pnl for t in wins), Decimal(0)) - gross_loss = abs(sum((t.pnl for t in losses), Decimal(0))) + gross_profit = sum((_closed_pnl(t) for t in wins), Decimal(0)) + gross_loss = abs(sum((_closed_pnl(t) for t in losses), Decimal(0))) if gross_loss > 0: profit_factor = gross_profit / gross_loss elif gross_profit > 0: @@ -90,7 +114,7 @@ def summarize(trades: list[Trade]) -> BacktestResult: streak = 0 max_losing_streak = 0 for t in closed: - running += t.pnl + running += _closed_pnl(t) peak = max(peak, running) max_drawdown = max(max_drawdown, peak - running) if t.outcome == "loss": diff --git a/pyproject.toml b/pyproject.toml index b9b7de34..e01f60ed 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -88,8 +88,25 @@ dev = [ [tool.mypy] python_version = "3.14" files = ["packages", "keel", "tests"] +# Implied by `--strict`, but mypy rejects it in a per-module section ("Per-module sections +# should only specify per-module flags"), so it is stated here rather than in the broker block +# below. It is already mypy's own default, so this line changes no behaviour today -- it is +# here so the setting stays explicit if that default ever flips. +warn_redundant_casts = true # New packages are strict from birth. +# +# ⚠️ These are the flags `--strict` bundles, listed one by one INSTEAD of `strict = true`, +# because `strict` is not actually a per-module setting: mypy applies it GLOBALLY no matter +# which `module` pattern the section carries (a section naming a module that does not exist +# turns it on for the whole run just the same). Written as `strict = true` here, this section +# silently type-checked `keel.*`, `keel_core.*` and `tests.*` under full strict mode too -- +# invisible only because all three carry `ignore_errors` below and so never printed what it +# found. The bug surfaces the moment a module is ungated: dropping `keel.*` from the exempt +# list turned up 131 errors where default mode finds 45. Keep this list expanded; re-collapsing +# it to `strict = true` re-breaks the scoping. If a future mypy adds a strict-implied flag, +# add it here as well -- `tests/test_packaging.py::test_broker_strict_flags_match_mypy_strict` +# fails the build when this list and mypy's own `--strict` bundle drift apart. [[tool.mypy.overrides]] module = [ "keel_broker_api.*", @@ -97,14 +114,38 @@ module = [ "keel_broker_fake.*", "keel_broker_robinhood.*", ] -strict = true +disallow_any_generics = true +disallow_subclassing_any = true +disallow_untyped_calls = true +disallow_untyped_defs = true +disallow_incomplete_defs = true +check_untyped_defs = true +disallow_untyped_decorators = true +# `warn_redundant_casts` belongs to the bundle too but is global-only; see `[tool.mypy]` above. +warn_unused_ignores = true +warn_return_any = true +no_implicit_reexport = true +strict_equality = true +extra_checks = true -# Legacy code and the existing suite are exempt until their modules move out -# (monorepo spec steps 4-7). Tighten one package at a time, never all at once. +# The suite is exempt until its modules move out (monorepo spec steps 4-7). `keel.*` came off +# this list once its default-mode errors were fixed: it is CHECKED but not yet `strict`, which +# is the next tightening step for it (and the reason it is not simply added to the strict block +# above). Tighten one package at a time, never all at once. [[tool.mypy.overrides]] -module = ["keel.*", "tests.*"] +module = ["tests.*"] ignore_errors = true +# The Coinbase SDK ships no `py.typed`, so every module importing it is an `import-untyped` +# error the moment its importer is checked. `keel/commands/_common.py` and `keel/data/cb_client.py` +# import it directly (the broker-port migration has not removed those yet -- see the note on the +# `keel-broker-coinbase` dependency above). Scoped to the SDK, NOT to our modules: our own code +# stays checked, only the untyped third-party import is waved through, exactly as the broker +# adapter already has to do. +[[tool.mypy.overrides]] +module = "coinbase.*" +ignore_missing_imports = true + # keel-core moved in the previous plan but predates strict mode; it is tightened # when its remaining consumers migrate. [[tool.mypy.overrides]] diff --git a/tests/test_packaging.py b/tests/test_packaging.py index 5d7aaf4f..24c7eec4 100644 --- a/tests/test_packaging.py +++ b/tests/test_packaging.py @@ -75,17 +75,29 @@ def test_the_dev_only_fake_venue_is_not_a_runtime_dependency_of_anything(): assert "keel-broker-fake" not in deps, f"{name} must not depend on keel-broker-fake" +#: The flag that identifies a strict override. `strict = true` cannot be used in a per-module +#: section -- mypy applies it GLOBALLY whatever `module` the section names -- so the strict +#: packages spell the bundle out flag by flag instead, and this is the one that best marks the +#: intent. Detecting the old `strict = true` spelling as well keeps this honest if a section is +#: ever added that way: the marker rule below should still cover it (the scoping bug is a +#: separate problem from the PEP 561 one). +_STRICT_MARKER = "disallow_untyped_defs" + + +def _mypy_overrides() -> list[dict]: + return tomllib.loads((_ROOT / "pyproject.toml").read_text())["tool"]["mypy"]["overrides"] + + def _strict_modules() -> list[str]: """Import packages the root `[tool.mypy]` config checks in strict mode. - Read from the config rather than listed here, so that tightening a package (moving it into a - `strict = true` override) automatically brings it under the marker rule below instead of - requiring someone to remember this file. + Read from the config rather than listed here, so that tightening a package (giving it the + strict flag block) automatically brings it under the marker rule below instead of requiring + someone to remember this file. """ - overrides = tomllib.loads((_ROOT / "pyproject.toml").read_text())["tool"]["mypy"]["overrides"] modules: list[str] = [] - for override in overrides: - if not override.get("strict"): + for override in _mypy_overrides(): + if not (override.get("strict") or override.get(_STRICT_MARKER)): continue entry = override["module"] for pattern in [entry] if isinstance(entry, str) else entry: @@ -111,6 +123,69 @@ def test_strictly_typed_packages_ship_a_py_typed_marker(module): """ candidates = [*(_ROOT / "packages").glob(f"*/{module}/py.typed"), _ROOT / module / "py.typed"] assert any(p.is_file() for p in candidates), ( - f"{module} is type-checked with `strict = true` but ships no py.typed marker; " + f"{module} is type-checked strictly but ships no py.typed marker; " f"create an empty one beside its `__init__.py` so installers can see the annotations" ) + + +def test_the_strict_module_list_is_not_empty(): + """`_strict_modules()` must actually find the strict packages. + + It discovers them by reading the mypy config, which means a change to how strictness is + SPELLED there silently empties the parametrization above -- `pytest` then reports one + skipped `[NOTSET]` case instead of four passing ones, and the py.typed rule stops being + enforced without anything going red. That is exactly what happened when the broker section + moved off `strict = true` onto the expanded flag list. A guard whose coverage can vanish + quietly needs a guard of its own. + """ + assert _strict_modules(), ( + "no strictly-typed modules found in [tool.mypy] overrides -- if the way strictness is " + f"configured changed, update `_STRICT_MARKER` ({_STRICT_MARKER!r}) to match" + ) + + +def test_broker_strict_flags_match_mypy_strict(): + """The expanded flag list must stay equal to what `--strict` actually turns on. + + The broker packages spell `--strict` out flag by flag because `strict = true` is not a + per-module setting (mypy applies it globally regardless of the `module` pattern, which is + how `keel.*` came to be checked under full strict mode while appearing exempt). The cost of + expanding it is that the list is now a COPY of mypy's, and a copy drifts: a future mypy that + adds a flag to the bundle would tighten every other project and quietly leave these four + behind. + + So the bundle is derived from the installed mypy rather than hard-coded here -- diffing a + default `Options` against a `--strict` one -- and compared with the config. `implicit_reexport` + is the one flag whose sense is inverted (`--strict` clears it; the config sets its `no_` + form), and `warn_redundant_casts` is excluded because it is global-only and already mypy's + default, so it never appears in the diff. + """ + import contextlib + import io + + from mypy.main import process_options + + def options(extra: list[str]): + with contextlib.redirect_stdout(io.StringIO()), contextlib.redirect_stderr(io.StringIO()): + _, opts = process_options([*extra, "x.py"], server_options=False) + return opts + + default, strict = options([]), options(["--strict"]) + expected = set() + for name in (n for n in dir(default) if not n.startswith("_")): + value = getattr(strict, name, None) + if isinstance(value, bool) and getattr(default, name, None) != value: + # `--strict` CLEARS implicit_reexport; the config states that as `no_implicit_reexport`. + expected.add(name if value else f"no_{name}") + + broker_override = next( + o for o in _mypy_overrides() if o.get(_STRICT_MARKER) and "keel_broker_api.*" in o["module"] + ) + configured = {k for k, v in broker_override.items() if k != "module" and v is True} + + assert configured == expected, ( + "the broker strict-flag list has drifted from mypy's own `--strict` bundle.\n" + f" missing from pyproject.toml: {sorted(expected - configured)}\n" + f" no longer implied by --strict: {sorted(configured - expected)}\n" + "Update the [[tool.mypy.overrides]] block for the broker packages to match." + )