From fd35ce57f06293c746f31b93c7c50a712c2c5e7a Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 9 Aug 2026 17:14:48 -0400 Subject: [PATCH 1/3] ci(sports): gate that display() returns a bool on every path Nothing caught the blank-panel bug this PR fixes, and the reasons are structural rather than bad luck. The safety harness calls display() and discards the result, so a wrong return type is invisible to it by construction; its fixtures deliberately seed games, so the empty path the bug lives on is never rendered; and hockey's fixture disables live mode outright because it cannot be fed from mock data. CI runs the harness and manifest checks only -- it never runs a plugin's own test_*.py -- so the regression test added here would not have gated anything either. Add a source-level gate, in the mould of the module-collision and scroll-adoption checks it now runs beside. It needs no data, no display and no core, so it sidesteps every one of those limitations, and it would have failed on the February migration commit that introduced the drift. Scoped to the bundled sports.py copies on purpose. 25 of 43 plugins return None from display() and are right to: a clock always has content, so "nothing to show" never arises and the controller's default suits them. Only the sports managers have a real no-content state and a dispatcher that reads the return value. `return super().display(...)` is allowed -- four plugins delegate their live mode that way, and the parent is itself checked. Bare returns, falling off the end, and truthy non-bools are not: the dispatcher branches on `result is True`/`is False`, so `return 1` lands in the same "assume success" arm that caused this. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01Udr6MfaFLUPhX5Fgo67Jf5 --- .github/workflows/module-collisions.yml | 11 + scripts/check_sports_display_contract.py | 223 ++++++++++++++++++ scripts/test_check_sports_display_contract.py | 138 +++++++++++ 3 files changed, 372 insertions(+) create mode 100644 scripts/check_sports_display_contract.py create mode 100644 scripts/test_check_sports_display_contract.py diff --git a/.github/workflows/module-collisions.yml b/.github/workflows/module-collisions.yml index 8c697669..d4b0e36e 100644 --- a/.github/workflows/module-collisions.yml +++ b/.github/workflows/module-collisions.yml @@ -48,3 +48,14 @@ jobs: - name: Test the scroll-adoption gate if: always() run: python scripts/test_check_scroll_adoption.py + # A sports display() that returns None reports content it does not have, + # so an out-of-season mode is never skipped and holds a blank panel for + # its whole duration. Source-level, so unlike the safety harness it needs + # no data -- and the harness seeds games, so it never renders the empty + # path this protects. + - name: Check sports display() returns a bool on every path + if: always() + run: python scripts/check_sports_display_contract.py + - name: Test the sports display()-contract gate + if: always() + run: python scripts/test_check_sports_display_contract.py diff --git a/scripts/check_sports_display_contract.py b/scripts/check_sports_display_contract.py new file mode 100644 index 00000000..5b7dee31 --- /dev/null +++ b/scripts/check_sports_display_contract.py @@ -0,0 +1,223 @@ +#!/usr/bin/env python3 +"""A bundled `sports.py` display() must return a bool on every path. + +The display controller skips a mode whose `display()` returns False, and the +sports managers' dispatcher treats anything that is not a bool as success: + + else: + # Result is None or other - assume success + return True, actual_mode + +So a display() that returns None reports content whether or not it has any. +For a sports scoreboard that is not hypothetical -- "this league has no games +right now" is a routine state (out of season, or favorite_teams_only with a +team that is not playing). The mode is then never skipped, and because the +no-games branch clears the display, what it reports as content is a blank +panel held until the display duration expires. + +That is exactly what shipped: hockey-scoreboard and lacrosse-scoreboard both +carried a stale fork of sports.py whose three display() methods had lost their +returns -- the base was annotated `-> None`. It was wrong from the monorepo +migration in February and only surfaced in August, because until the NHL +offseason those modes always had games and a wrong return value is invisible +whenever there is something to draw. + +Nothing caught it. The safety harness calls display() and discards the result, +and its fixtures deliberately seed games so the empty path never renders. This +check is the cheap part of closing that: it is a property of the source, so it +does not need data, a display, or a core to run. + +Deliberately scoped to the bundled sports.py copies. Returning None from +display() is normal and correct for most plugins -- a clock always has content, +so "nothing to show" never arises and the controller's default is right for +them. It is only the sports managers that have a real no-content state and a +dispatcher that reads the return value. + +Run: python scripts/check_sports_display_contract.py [plugin-id ...] +Exit code 0 when clean, 1 when a display() can return a non-bool. +""" + +import ast +import sys +from pathlib import Path +from typing import List, Optional, Tuple + +PLUGINS_DIR = Path(__file__).resolve().parent.parent / "plugins" + +# The manager classes whose display() the dispatcher's return-value handling +# applies to. A class that does not define display() inherits one that is +# checked here, so it needs no entry of its own. +SPORTS_CLASSES = ("SportsCore", "SportsUpcoming", "SportsRecent", "SportsLive") + + +def _is_bool_literal(node: Optional[ast.expr]) -> bool: + """True for a literal True/False, and nothing else. + + Deliberately strict. The dispatcher branches on `result is True` and + `result is False`, so a value that is merely truthy -- 1, a non-empty + string, an object -- falls through to the "assume success" path just as + None does. Accepting anything but the literals would let the original bug + back in wearing a different type. + """ + return isinstance(node, ast.Constant) and isinstance(node.value, bool) + + +def _is_super_display_call(node: Optional[ast.expr]) -> bool: + """True for `return super().display(...)`. + + A subclass that adds behaviour and then hands off to its parent is fine: + the parent is one of SPORTS_CLASSES too, so this check has already proved + that what comes back is a bool. Four plugins delegate their live mode this + way. Only `super()` qualifies -- an arbitrary `other.display()` could be + anything, and the point of this check is to not take that on trust. + """ + if not isinstance(node, ast.Call): + return False + func = node.func + return (isinstance(func, ast.Attribute) + and func.attr == "display" + and isinstance(func.value, ast.Call) + and isinstance(func.value.func, ast.Name) + and func.value.func.id == "super") + + +def _terminates(body: List[ast.stmt]) -> bool: + """Whether a statement list always exits, so control cannot fall off it. + + A function that runs off the end returns None, which is the failure this + check exists for -- and it is invisible to a scan that only inspects + `return` statements, since the offending path has none. Recursion covers + the compound statements a display() realistically ends on. + """ + if not body: + return False + last = body[-1] + + if isinstance(last, (ast.Return, ast.Raise)): + return True + + if isinstance(last, ast.If): + # An `if` without an `else` can always fall through. + return bool(last.orelse) and _terminates(last.body) and _terminates(last.orelse) + + if isinstance(last, ast.Try): + # `finally` that exits ends the function whatever happened before it. + if last.finalbody and _terminates(last.finalbody): + return True + # Otherwise every ordinary path must exit: the body (or its else), + # and each handler. + main = _terminates(last.orelse) if last.orelse else _terminates(last.body) + return main and bool(last.handlers) and all( + _terminates(h.body) for h in last.handlers) + + if isinstance(last, (ast.With, ast.AsyncWith)): + return _terminates(last.body) + + if isinstance(last, (ast.While, ast.For, ast.AsyncFor)): + # `while True:` with no break is the only loop that reliably never + # falls through; anything else may run zero times or break out. + if isinstance(last, ast.While) and _is_bool_literal(last.test) \ + and last.test.value is True and not last.orelse: + return not any(isinstance(n, ast.Break) for n in ast.walk(last)) + return False + + match_cls = getattr(ast, "Match", None) + if match_cls is not None and isinstance(last, match_cls): + # Only exhaustive when a wildcard case is present, which we cannot + # tell cheaply; treat as falling through. + return False + + return False + + +def _check_function(fn: ast.FunctionDef) -> List[str]: + """Reasons this display() can hand back something other than a bool.""" + problems = [] + + for node in ast.walk(fn): + # Skip returns belonging to a nested function -- they are not this + # function's result. + if isinstance(node, (ast.FunctionDef, ast.AsyncFunctionDef)) and node is not fn: + continue + if isinstance(node, ast.Return) and not ( + _is_bool_literal(node.value) or _is_super_display_call(node.value)): + if node.value is None: + problems.append(f"line {node.lineno}: bare `return` (yields None)") + else: + rendered = getattr(ast, "unparse", lambda n: "")(node.value) + problems.append( + f"line {node.lineno}: returns `{rendered}`, not a bool literal") + + if not _terminates(fn.body): + problems.append( + f"line {fn.lineno}: can fall off the end of the function (yields None)") + + return problems + + +def _check_plugin(sports_py: Path) -> Tuple[List[str], Optional[str]]: + """(problems, unreadable_reason) for one plugin's sports.py.""" + try: + tree = ast.parse(sports_py.read_text(encoding="utf-8")) + except (SyntaxError, UnicodeDecodeError, OSError) as exc: + return [], f"{type(exc).__name__}: {exc}" + + problems = [] + for cls in [n for n in ast.walk(tree) if isinstance(n, ast.ClassDef)]: + if cls.name not in SPORTS_CLASSES: + continue + for fn in [n for n in cls.body + if isinstance(n, (ast.FunctionDef, ast.AsyncFunctionDef)) + and n.name == "display"]: + for reason in _check_function(fn): + problems.append(f"{cls.name}.display {reason}") + return problems, None + + +def main(argv: List[str]) -> int: + if argv: + candidates = [PLUGINS_DIR / pid for pid in argv] + else: + candidates = sorted(p for p in PLUGINS_DIR.iterdir() if p.is_dir()) + + checked = 0 + failures = {} + unreadable = {} + + for plugin_dir in candidates: + sports_py = plugin_dir / "sports.py" + if not sports_py.exists(): + continue + checked += 1 + problems, reason = _check_plugin(sports_py) + if reason: + unreadable[plugin_dir.name] = reason + elif problems: + failures[plugin_dir.name] = problems + + for pid, problems in failures.items(): + detail = "; ".join(problems) + print(f"::error::{pid}/sports.py: display() must return a bool on every " + f"path so an empty mode can be skipped, but {detail}. Returning " + f"None makes the dispatcher assume success, and the mode then " + f"holds a blank panel for its whole display duration.") + + for pid, reason in unreadable.items(): + print(f"::error::{pid}/sports.py could not be parsed ({reason}). Treating " + f"that as a pass would let a malformed file skip this check.") + + if failures or unreadable: + if failures: + print(f"\nFAIL: {len(failures)} of {checked} plugin(s) with a sports.py " + f"can return a non-bool from display().") + if unreadable: + print(f"FAIL: {len(unreadable)} of {checked} plugin(s) could not be parsed.") + return 1 + + print(f"OK: {checked} plugin(s) with a sports.py, all returning a bool from " + f"every display() path.") + return 0 + + +if __name__ == "__main__": + sys.exit(main(sys.argv[1:])) diff --git a/scripts/test_check_sports_display_contract.py b/scripts/test_check_sports_display_contract.py new file mode 100644 index 00000000..86943c27 --- /dev/null +++ b/scripts/test_check_sports_display_contract.py @@ -0,0 +1,138 @@ +#!/usr/bin/env python3 +"""Regression tests for the sports display()-contract gate. + +The gate exists because hockey-scoreboard and lacrosse-scoreboard shipped a +stale fork of sports.py whose display() methods returned None. The dispatcher +treats a non-bool as success, so an empty mode reported content, was never +skipped, and held a blank panel for its whole display duration. It was wrong +from the February monorepo migration and only surfaced in August, when the NHL +offseason made "no games" persistent for the first time. + +So these pin the ways the gate could quietly stop working: + +- **The fall-off-the-end path.** The original bug's worst case had no `return` + statement at all on the offending path. A checker that only inspects `return` + nodes sees nothing wrong and passes the exact file it was written for. +- **Truthiness is not the contract.** The dispatcher branches on `result is + True` / `result is False`, so `return 1` lands in the same "assume success" + arm as `return None`. Accepting anything but the literals reintroduces the + bug in a new type. +- **Delegation must stay allowed.** Four plugins' live mode is + `return super().display(...)`, whose parent this gate has already checked. + Flagging that would make the gate unusable and invite its removal. +- **A file that cannot be parsed is not a file that passed.** Swallowing + SyntaxError and reporting no findings would let a malformed sports.py skip + the check and exit 0. + +Exit codes follow the convention in the sibling gate tests: 0 pass, 1 fail. +""" + +import sys +import tempfile +from pathlib import Path + +sys.path.insert(0, str(Path(__file__).resolve().parent)) + +from check_sports_display_contract import _check_plugin # noqa: E402 + + +def _wrap(body: str, cls: str = "SportsRecent") -> str: + return "class %s:\n def display(self, force_clear=False):\n%s\n" % ( + cls, body) + + +CLEAN = [ + ("returns on both paths", _wrap( + " if not self.games_list:\n" + " return False\n" + " return True")), + ("try/except with returns everywhere", _wrap( + " try:\n" + " return True\n" + " except Exception:\n" + " return False")), + ("delegates to super", _wrap( + " return super().display(force_clear)", cls="SportsLive")), + ("raises instead of returning", _wrap( + " if not self.ok:\n" + " raise RuntimeError('x')\n" + " return True")), + ("a class the gate does not police is ignored", _wrap( + " return", cls="SomeOtherHelper")), +] + +DIRTY = [ + # The shape that actually shipped. + ("bare return", _wrap( + " if not self.games_list:\n" + " return\n" + " return True"), "bare `return`"), + ("falls off the end", _wrap( + " if self.games_list:\n" + " return True"), "fall off the end"), + ("no return at all", _wrap( + " self._draw()"), "fall off the end"), + ("truthy non-bool", _wrap( + " return 1"), "not a bool literal"), + ("returns a variable", _wrap( + " result = self._draw()\n" + " return result"), "not a bool literal"), + # An `if` with no `else` falls through even when both its arms return. + ("if without else", _wrap( + " if a:\n" + " return True\n" + " elif b:\n" + " return False"), "fall off the end"), + # try/except where the handler falls through. + ("handler falls through", _wrap( + " try:\n" + " return True\n" + " except Exception:\n" + " self.logger.error('x')"), "fall off the end"), +] + +failures = [] + + +def check(name, cond, detail=""): + if cond: + print(" PASS %s" % name) + else: + print(" FAIL %s%s" % (name, (": " + detail) if detail else "")) + failures.append(name) + + +def main() -> int: + with tempfile.TemporaryDirectory() as tmp: + path = Path(tmp) / "sports.py" + + print("valid shapes pass") + for name, source in CLEAN: + path.write_text(source, encoding="utf-8") + problems, reason = _check_plugin(path) + check(name, not problems and reason is None, + "reported %r" % (problems or reason,)) + + print("\nbroken shapes are caught") + for name, source, expected in DIRTY: + path.write_text(source, encoding="utf-8") + problems, reason = _check_plugin(path) + check(name, bool(problems), "reported nothing") + check(" %s names the reason" % name, + any(expected in p for p in problems), + "expected %r in %r" % (expected, problems)) + + print("\nan unparseable file fails rather than passing silently") + path.write_text("class SportsRecent(:\n", encoding="utf-8") + problems, reason = _check_plugin(path) + check("syntax error is reported", reason is not None, + "returned %r" % (reason,)) + check("and is not mistaken for a clean file", not problems) + + print("\n%s" % ("FAILED: %d" % len(failures) if failures + else "All checks passed")) + return 1 if failures else 0 + + +if __name__ == "__main__": + sys.exit(main()) From fd14aed1bdbd272848b697b3fc3fbdc8abe023cf Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 9 Aug 2026 17:24:18 -0400 Subject: [PATCH 2/3] ci: run the structure workflow when its own new gate changes MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The paths filter names each checker explicitly, so the two scripts added in the previous commit would not have triggered it — a PR that edited only the gate or its tests would skip the very check it was changing. The workflow file already carries that reasoning in a comment about itself; this extends it to the new scripts. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01Udr6MfaFLUPhX5Fgo67Jf5 --- .github/workflows/module-collisions.yml | 2 ++ 1 file changed, 2 insertions(+) diff --git a/.github/workflows/module-collisions.yml b/.github/workflows/module-collisions.yml index d4b0e36e..2f948012 100644 --- a/.github/workflows/module-collisions.yml +++ b/.github/workflows/module-collisions.yml @@ -23,6 +23,8 @@ on: - 'scripts/check_module_collisions.py' - 'scripts/check_scroll_adoption.py' - 'scripts/test_check_scroll_adoption.py' + - 'scripts/check_sports_display_contract.py' + - 'scripts/test_check_sports_display_contract.py' # Without this, a PR that only edits this workflow matches no path and # the workflow never runs against its own change. - '.github/workflows/module-collisions.yml' From 8230d0651035ac3348acd113aa9422e630695393 Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 10 Aug 2026 08:16:26 -0400 Subject: [PATCH 3/3] fix(ci): scope the gate's AST traversal, and read match exhaustiveness Three false positives, all of which would have failed a correct display() and so invited the gate's removal. `ast.walk` is a flat traversal of every descendant, so skipping the nested FunctionDef node when it came round did nothing -- its children were already queued. A nested helper's `return` was therefore reported as the outer function's, and an inner loop's `break` counted as breaking an enclosing `while True`. The comment claiming otherwise was simply wrong. Recurse manually and refuse to enter nested scopes, and match a `break` only against the loop it actually binds to. A trailing `match` was treated as always falling through, on the reasoning that exhaustiveness could not be told cheaply. It can: an unguarded `case _` or bare capture is an ast.MatchAs with no sub-pattern, and if such a case exists and every case body terminates, the statement cannot fall through. A guard makes even a wildcard refutable, so that still falls through. Verified against the real plugins (still clean) and against hockey-scoreboard's pre-fix sports.py (still 8 violations), so the loosening has not blunted the check. The gate's own suite grows from 24 checks to 36, covering each new shape in both directions. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01Udr6MfaFLUPhX5Fgo67Jf5 --- scripts/check_sports_display_contract.py | 55 +++++++++++++++--- scripts/test_check_sports_display_contract.py | 56 +++++++++++++++++++ 2 files changed, 102 insertions(+), 9 deletions(-) diff --git a/scripts/check_sports_display_contract.py b/scripts/check_sports_display_contract.py index 5b7dee31..28113318 100644 --- a/scripts/check_sports_display_contract.py +++ b/scripts/check_sports_display_contract.py @@ -81,6 +81,37 @@ def _is_super_display_call(node: Optional[ast.expr]) -> bool: and func.value.func.id == "super") +# Constructs that open a new scope. A `return` inside one belongs to that +# scope, not to the display() being checked. +_NESTED_SCOPES = (ast.FunctionDef, ast.AsyncFunctionDef, ast.Lambda, ast.ClassDef) + +# Constructs a `break` binds to. A break inside one of these does not leave an +# enclosing loop. +_LOOPS = (ast.For, ast.AsyncFor, ast.While) + + +def _walk_scope(node: ast.AST, stop_at: tuple = ()) -> "object": + """ + Yield descendants of ``node`` without crossing into a nested scope. + + ``ast.walk`` is a flat traversal of every descendant, so it happily reports + a nested helper's ``return`` as the outer function's, and an inner loop's + ``break`` as breaking the outer one. Skipping the nested node when it comes + round does nothing -- its children are already queued. Recursing manually + and refusing to enter is what actually scopes the search. + """ + for child in ast.iter_child_nodes(node): + if isinstance(child, _NESTED_SCOPES) or (stop_at and isinstance(child, stop_at)): + continue + yield child + yield from _walk_scope(child, stop_at) + + +def _breaks_out_of(loop: ast.AST) -> bool: + """Whether a ``break`` in this loop's own body targets it.""" + return any(isinstance(n, ast.Break) for n in _walk_scope(loop, stop_at=_LOOPS)) + + def _terminates(body: List[ast.stmt]) -> bool: """Whether a statement list always exits, so control cannot fall off it. @@ -118,14 +149,22 @@ def _terminates(body: List[ast.stmt]) -> bool: # falls through; anything else may run zero times or break out. if isinstance(last, ast.While) and _is_bool_literal(last.test) \ and last.test.value is True and not last.orelse: - return not any(isinstance(n, ast.Break) for n in ast.walk(last)) + return not _breaks_out_of(last) return False match_cls = getattr(ast, "Match", None) if match_cls is not None and isinstance(last, match_cls): - # Only exhaustive when a wildcard case is present, which we cannot - # tell cheaply; treat as falling through. - return False + # Exhaustive when some case cannot fail to match and every case exits. + # An irrefutable case is an unguarded `case _:` or a bare capture + # (`case other:`) -- both are ast.MatchAs carrying no sub-pattern. A + # guard makes even those refutable, so the match can fall through. + irrefutable = any( + case.guard is None + and isinstance(case.pattern, ast.MatchAs) + and case.pattern.pattern is None + for case in last.cases + ) + return irrefutable and all(_terminates(case.body) for case in last.cases) return False @@ -134,11 +173,9 @@ def _check_function(fn: ast.FunctionDef) -> List[str]: """Reasons this display() can hand back something other than a bool.""" problems = [] - for node in ast.walk(fn): - # Skip returns belonging to a nested function -- they are not this - # function's result. - if isinstance(node, (ast.FunctionDef, ast.AsyncFunctionDef)) and node is not fn: - continue + # Scoped to this function: a nested helper's `return` is that helper's + # result, and flagging it would fail a perfectly correct display(). + for node in _walk_scope(fn): if isinstance(node, ast.Return) and not ( _is_bool_literal(node.value) or _is_super_display_call(node.value)): if node.value is None: diff --git a/scripts/test_check_sports_display_contract.py b/scripts/test_check_sports_display_contract.py index 86943c27..c85d2adf 100644 --- a/scripts/test_check_sports_display_contract.py +++ b/scripts/test_check_sports_display_contract.py @@ -59,6 +59,40 @@ def _wrap(body: str, cls: str = "SportsRecent") -> str: " return True")), ("a class the gate does not police is ignored", _wrap( " return", cls="SomeOtherHelper")), + # ast.walk is flat, so a guard that skips the nested FunctionDef node does + # nothing -- its children are already queued. These three shapes were all + # reported as violations until the traversal became scope-aware. + ("nested helper with a bare return", _wrap( + " def _fmt(x):\n" + " if not x:\n" + " return\n" + " return str(x)\n" + " return True")), + ("nested class whose method returns None", _wrap( + " class _Tmp:\n" + " def helper(self):\n" + " return\n" + " return True")), + ("break in an inner loop does not escape while True", _wrap( + " while True:\n" + " for g in self.games:\n" + " break\n" + " return True")), + # A trailing match is exhaustive when an unguarded irrefutable case is + # present and every case exits; treating all of them as fall-through + # failed valid code. + ("exhaustive match with a wildcard", _wrap( + " match self.mode:\n" + " case 'empty':\n" + " return False\n" + " case _:\n" + " return True")), + ("exhaustive match with a bare capture", _wrap( + " match self.mode:\n" + " case 'empty':\n" + " return False\n" + " case other:\n" + " return True")), ] DIRTY = [ @@ -89,6 +123,28 @@ def _wrap(body: str, cls: str = "SportsRecent") -> str: " return True\n" " except Exception:\n" " self.logger.error('x')"), "fall off the end"), + # The scope- and exhaustiveness-awareness above must not blunt the check. + ("a break that really does escape while True", _wrap( + " while True:\n" + " break"), "fall off the end"), + ("match with no irrefutable case", _wrap( + " match self.mode:\n" + " case 'empty':\n" + " return False"), "fall off the end"), + ("a guarded wildcard is still refutable", _wrap( + " match self.mode:\n" + " case _ if self.x:\n" + " return True"), "fall off the end"), + ("match whose wildcard case falls through", _wrap( + " match self.mode:\n" + " case 'empty':\n" + " self._log()\n" + " case _:\n" + " return True"), "fall off the end"), + ("a nested helper does not excuse the outer function", _wrap( + " def _fmt(x):\n" + " return str(x)\n" + " self._draw()"), "fall off the end"), ] failures = []