diff --git a/.github/workflows/module-collisions.yml b/.github/workflows/module-collisions.yml index 8c697669..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' @@ -48,3 +50,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..28113318 --- /dev/null +++ b/scripts/check_sports_display_contract.py @@ -0,0 +1,260 @@ +#!/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") + + +# 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. + + 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 _breaks_out_of(last) + return False + + match_cls = getattr(ast, "Match", None) + if match_cls is not None and isinstance(last, match_cls): + # 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 + + +def _check_function(fn: ast.FunctionDef) -> List[str]: + """Reasons this display() can hand back something other than a bool.""" + problems = [] + + # 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: + 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..c85d2adf --- /dev/null +++ b/scripts/test_check_sports_display_contract.py @@ -0,0 +1,194 @@ +#!/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")), + # 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 = [ + # 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"), + # 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 = [] + + +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())