ci(sports): gate that display() returns a bool on every path - #264
Conversation
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) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Udr6MfaFLUPhX5Fgo67Jf5
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) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Udr6MfaFLUPhX5Fgo67Jf5
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds an AST-based checker for sports plugin ChangesSports display contract validation
Estimated code review effort: 4 (Complex) | ~40 minutes Sequence Diagram(s)sequenceDiagram
participant GitHubActions
participant ContractChecker
participant SportsPlugin
participant RegressionTest
GitHubActions->>ContractChecker: Run display contract check
ContractChecker->>SportsPlugin: Parse and inspect display()
SportsPlugin-->>ContractChecker: Return contract violations or clean result
GitHubActions->>RegressionTest: Run regression suite
RegressionTest->>ContractChecker: Exercise valid and invalid cases
ContractChecker-->>RegressionTest: Return diagnostics
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Up to standards ✅🟢 Issues
|
| Metric | Results |
|---|---|
| Complexity | 86 |
NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@scripts/check_sports_display_contract.py`:
- Around line 137-149: The return analysis in _check_function() and termination
analysis in _terminates() must be scope-aware: stop traversal at nested
functions and classes, and count only ast.Break nodes whose target is the
specific last loop rather than breaks from inner loops. Update both affected
sites in scripts/check_sports_display_contract.py (lines 137-149 and 117-122)
using a visitor or equivalent parent/loop tracking, and add tests covering a
nested helper with a non-boolean return and an inner-loop break inside while
True.
- Around line 124-128: Update _terminates() so a final ast.Match is considered
terminating only when it contains an unguarded irrefutable ast.MatchAs wildcard
case and every match_case body terminates; otherwise preserve the current
fall-through result. Add a regression case covering a valid display()
implementation with an exhaustive case _ whose body terminates.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 0b3dd62d-96be-4a9e-82fe-12c76aec189c
📒 Files selected for processing (3)
.github/workflows/module-collisions.ymlscripts/check_sports_display_contract.pyscripts/test_check_sports_display_contract.py
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) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Udr6MfaFLUPhX5Fgo67Jf5
|
Both findings were valid — and both would have failed a correct Scope-aware traversal. You're right, and the guard I'd written was worse than nothing: it looked like it handled nested scopes while doing nothing at all. for node in ast.walk(fn):
if isinstance(node, (ast.FunctionDef, ast.AsyncFunctionDef)) and node is not fn:
continue # <- no-op: walk already queued this node's children
Now Match exhaustiveness. My comment said this "cannot be told cheaply", which was just wrong — your Checked the loosening didn't blunt the gate, since three of these changes make it accept more:
The suite goes from 24 checks to 36, covering each new shape in both directions — the valid ones now pass, and |
2 participants
Follow-up to #263 (merged). That fixed the hockey blank-panel bug; this stops it coming back, and closes the biggest of the reasons nothing caught it for six months.
Why a source-level gate
Nothing caught the original for structural reasons, not bad luck:
display()and discards the result, so a wrong return type is invisible to it by construction.harness.jsonsays so explicitly ("one final, one in-progress, and one scheduled game so recent/upcoming render real game cards") — so the empty path the bug lives on is never rendered.test_*.py— only manifest checks and the harness — so the regression test added in fix(sports): stop an empty mode holding a blank panel for its duration #263 wouldn't have gated anything either.A source-level check sidesteps every one of those: it needs no data, no display and no core. Run against the migration commit that introduced the drift, it fails — so it would have caught this in February rather than August.
What it checks
Every
display()onSportsCore/SportsUpcoming/SportsRecent/SportsLivein a bundledsports.pymust return a bool on every path — including the fall-off-the-end path, which is what the original bug's worst case actually was and which a checker that only inspectsreturnnodes would miss entirely.return super().display(...)is allowed: four plugins delegate their live mode that way and the parent is itself checked. Truthy non-bools are not — the dispatcher branches onresult is True/result is False, soreturn 1lands in the same "assume success" arm that caused this.Why it's scoped to sports
25 of 43 plugins return
Nonefromdisplay(), and are right to. A clock always has content, so "nothing to show" never arises and the controller's default is correct for them. Only the sports managers have a real no-content state and a dispatcher that reads the return value. Enforcing bools fleet-wide would fail more than half the repo for no benefit.Verification
OK: 9 plugin(s) with a sports.py, all returning a bool from every display() path.super().display()delegation, which is why that's allowed.scripts/test_check_sports_display_contract.pypins 12 cases, following thetest_check_scroll_adoption.pyprecedent: valid shapes (both-paths return, try/except,super()delegation,raise, unpoliced classes), and every broken shape (bare return, fall-through, no return at all, truthy non-bool, returned variable,ifwithoutelse, handler that falls through), plus that an unparseable file fails rather than silently passing.if: always()so one PR surfaces every structural problem at once.The second commit adds the two new scripts to that workflow's
pathsfilter. Without it a PR editing only the gate would skip the very check it was changing — the workflow already carries that reasoning in a comment about itself.Still open
The remaining gap is that CI doesn't run plugin
test_*.pyat all. That can't just be switched on:test_lacrosse_plugin.pycurrently fails against live ESPN for an out-of-season date range, and several tests skip without a core onPYTHONPATH. Worth doing, but it needs triage first.There's a companion core-side change in LEDMatrix#447 that makes the harness warn when a mode draws nothing while claiming content. Independent of this one.
🤖 Generated with Claude Code
https://claude.ai/code/session_01Udr6MfaFLUPhX5Fgo67Jf5
Summary by CodeRabbit
Bug Fixes
Tests