cal/major-domo-v2
1
0
Fork 0
Code Issues 6 Pull Requests 2 Actions Packages Projects Releases Wiki Activity

perf: parallelize schedule_service API calls with asyncio.gather #103

Merged
cal merged 7 commits from ai/major-domo-v2-88 into next-release 2026-03-20 15:16:41 +00:00
Conversation 0 Commits 7 Files Changed 6 +775 -143
Claude commented 2026-03-20 15:02:34 +00:00
Collaborator
Copy Link

Fixes #88

Summary

  • Replaced sequential for week in week_range: await self.get_week_schedule(...) loops with asyncio.gather() in get_team_schedule(), get_recent_games(), and get_upcoming_games()
  • get_team_schedule(): up to 18 sequential HTTP requests → all concurrent
  • get_recent_games(): 2 sequential requests → concurrent
  • get_upcoming_games(): up to 18 sequential requests with early exit → all concurrent (early exit removed — unnecessary when all weeks fetch in parallel)
  • Added 16 new tests in test_services_schedule.py (first coverage for this service)

Test plan

  • python -m pytest tests/test_services_schedule.py -v — 16/16 pass
  • python -m pytest --tb=short -q — full suite passes (18 pre-existing failures in unrelated chart/scorebug tests)
  • /simplify review — refactored test helpers to use existing GameFactory/TeamFactory

🤖 Generated with Claude Code

Fixes #88 ## Summary - Replaced sequential `for week in week_range: await self.get_week_schedule(...)` loops with `asyncio.gather()` in `get_team_schedule()`, `get_recent_games()`, and `get_upcoming_games()` - `get_team_schedule()`: up to 18 sequential HTTP requests → all concurrent - `get_recent_games()`: 2 sequential requests → concurrent - `get_upcoming_games()`: up to 18 sequential requests with early exit → all concurrent (early exit removed — unnecessary when all weeks fetch in parallel) - Added 16 new tests in `test_services_schedule.py` (first coverage for this service) ## Test plan - [x] `python -m pytest tests/test_services_schedule.py -v` — 16/16 pass - [x] `python -m pytest --tb=short -q` — full suite passes (18 pre-existing failures in unrelated chart/scorebug tests) - [x] `/simplify` review — refactored test helpers to use existing `GameFactory`/`TeamFactory` 🤖 Generated with [Claude Code](https://claude.com/claude-code)
Claude added 1 commit 2026-03-20 15:02:35 +00:00
perf: parallelize schedule_service week fetches with asyncio.gather (#88)
All checks were successful
Build Docker Image / build (pull_request) Successful in 1m20s
Details
b480120731 
Closes #88

Replaced sequential for-loops in get_team_schedule(), get_recent_games(),
and get_upcoming_games() with asyncio.gather() to fire all per-week HTTP
requests concurrently. Also adds import asyncio which was missing.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
cal added 1 commit 2026-03-20 15:05:45 +00:00
refactor: use GameFactory/TeamFactory in schedule service tests
All checks were successful
Build Docker Image / build (pull_request) Successful in 58s
Details
0992acf718 
Replace custom _make_game/_make_team helpers with existing test
factories for consistency with the rest of the test suite.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
cal changed title from perf: parallelize schedule_service week fetches with asyncio.gather (#88) to perf: parallelize schedule_service API calls with asyncio.gather 2026-03-20 15:06:08 +00:00
cal changed target branch from main to next-release 2026-03-20 15:06:10 +00:00
cal reviewed 2026-03-20 15:08:45 +00:00
cal left a comment
Owner
Copy Link

AI Code Review

Files Reviewed

  • services/schedule_service.py (modified)
  • tests/test_services_schedule.py (added)
  • bot.py (modified — maintenance tree + duplicate attribute bug)
  • commands/admin/management.py (modified — type: ignore comment)
  • tests/test_bot_maintenance_tree.py (added)

Findings

Correctness

schedule_service.py — all three parallelization changes are correct.

get_team_schedule: Sequential loop over week_range replaced with asyncio.gather(*[...]). The gather result is a tuple of lists in the same order as the coroutines, so the downstream filter loop is correct.

get_recent_games: List comprehension [(10 - offset) for offset in range(weeks_back) if (10 - offset) > 0] correctly filters non-positive week numbers before constructing the gather. Semantically equivalent to the old if week <= 0: break guard, but without the early-exit behaviour — which is fine because the filter is applied before any I/O.

get_upcoming_games: Early-exit (if upcoming_games and len(upcoming_games) >= 20: break) correctly removed. It was an optimisation that no longer makes sense when all weeks are fetched in a single parallel batch. The weeks_ahead parameter in the signature still exists but is now ignored — the implementation always fetches all 18 weeks regardless of its value. This is a latent behaviour mismatch between the docstring/signature and the implementation (see Suggestions below).

bot.py — pre-existing duplicate attribute assignment confirmed still present.

This was flagged in the PR #84 review and is a merge artifact from PR #83. Lines 121 and 123 of bot.py both assign self.maintenance_mode: bool = False. This PR's diff adds the MaintenanceAwareTree class and the tree_cls kwarg but does not introduce the duplicate — it was already in next-release. The duplicate is harmless at runtime (second assignment overwrites first with the same value) but is dead code. The test_maintenance_mode_default_is_false test passes despite this because it only asserts the final value, not the number of assignments.

bot.py — dead decorator block in setup_hook still present.

The old @self.tree.interaction_check / maintenance_check decorator block (lines 129–143 in the current bot.py) is still in setup_hook. It was supposed to be removed as part of the fix introduced in PR #83, and was flagged as still-present dead code in the PR #84 review. This PR does not touch setup_hook and so does not remove it. It generates a RuntimeWarning (unawaited coroutine) whenever the bot starts. This is a pre-existing issue, not introduced by this PR.

test_services_schedule.py — correctness of test logic verified.

test_filters_by_team_case_insensitive: mock.side_effect = [week1, week2] with weeks=2 triggers asyncio.gather over two coroutines. With AsyncMock, side_effect as a list pops one value per call, which works correctly even inside gather. Result count assertion (len(result) == 2) is correct.

test_skips_negative_weeks assertion comment says "only 10 valid weeks" (weeks 10 down to 1). 10 - 0 = 10, 10 - 14 = -4 — the filter removes offsets 10–14, leaving offsets 0–9, which is 10 weeks. Correct.

test_returns_only_incomplete_games asserts len(result) == 18 because mock.return_value = [completed, upcoming] is called for all 18 weeks, returning 1 incomplete game each time. This is a valid whitebox assertion but tightly couples the test to the implementation detail of always fetching exactly 18 weeks. If that constant changes, the assertion will break. Low risk, worth noting.

test_bot_maintenance_tree.py — test_maintenance_on_message_has_no_emoji is overly strict.

The test checks unicodedata.category(ch) != "So" (Other Symbol) for each character in the maintenance message. This correctly catches most emoji. However, the project's "no emoji in bot messages" rule (from discord-app-v2/CLAUDE.md) specifically applies to EmbedTemplate.success/error/warning/info/loading() title parameters — it is a pattern about double-emoji in embeds, not a blanket ban on all Unicode symbols in all messages. The current test would falsely fail if the message were changed to include a legitimate typographic symbol (e.g., a copyright sign, ©, category So). This is a minor over-constraint, not a blocking issue.

_patch_discord_app_commands autouse fixture is a no-op.

The fixture at tests/test_bot_maintenance_tree.py:68 yields immediately with no setup or teardown. The docstring says it stubs the parent CommandTree.__init__, but the body does nothing. The tests pass because interaction_check is called as an unbound method (tree_cls.interaction_check(MagicMock(), interaction)) without ever constructing a MaintenanceAwareTree instance, so __init__ is never invoked. The fixture is not harmful but is misleading — it promises patching it does not perform.

Security

No security issues. The getattr(bot, "maintenance_mode", False) safe-access is correct and avoids AttributeError during startup races. Admin check uses guild_permissions.administrator, which is the appropriate guild-level flag.

Style & Conventions

All modified files follow the project's double-quote, trailing-comma style (enforced by the linter in previous PRs). asyncio import is at module level, not lazy-imported mid-file, consistent with CLAUDE.md. Docstrings are present on all new methods and test classes. @logged_command and self.logger are not applicable here (schedule_service and tests, not a cog command).

Suggestions

  1. weeks_ahead parameter in get_upcoming_games is now ignored. The signature is get_upcoming_games(self, season: int, weeks_ahead: int = 6) but the body always gathers range(1, 19). Either remove the parameter (breaking change for callers) or restore the behaviour by using weeks_ahead in the gather range. The PR body notes the early exit was "unnecessary when all weeks fetch in parallel" — but weeks_ahead was a different concept (how far ahead to look, not an optimisation gate). Worth a follow-up to reconcile the signature and implementation.

  2. Dead code in bot.py — duplicate self.maintenance_mode assignment and stale @self.tree.interaction_check block in setup_hook. These are pre-existing issues from PR #83 that survived the merge. A targeted cleanup PR removing lines 123 (self.maintenance_mode: bool = False duplicate) and the entire maintenance_check inner function block (lines 129–143) would be appropriate.

  3. _patch_discord_app_commands fixture. Either remove it (the tests work without it) or give it a real body that patches discord.app_commands.CommandTree.__init__ to a no-op, matching its stated intent.

  4. test_returns_only_incomplete_games magic number. Consider replacing the literal 18 with a named constant or deriving it from the implementation's range(1, 19) to make the coupling explicit.

Verdict: COMMENT

The schedule service parallelization is correct and well-tested. The 16 new tests cover the parallelization paths, edge cases, and the group_games_by_series grouping logic. No security issues. The suggestions above (particularly the weeks_ahead parameter mismatch) are worth addressing in a follow-up, but none of them block merging. Posting as COMMENT per convention — Gitea does not allow self-approval.

Automated review by Claude PR Reviewer

## AI Code Review ### Files Reviewed - `services/schedule_service.py` (modified) - `tests/test_services_schedule.py` (added) - `bot.py` (modified — maintenance tree + duplicate attribute bug) - `commands/admin/management.py` (modified — type: ignore comment) - `tests/test_bot_maintenance_tree.py` (added) --- ### Findings #### Correctness **schedule_service.py — all three parallelization changes are correct.** `get_team_schedule`: Sequential loop over `week_range` replaced with `asyncio.gather(*[...])`. The gather result is a tuple of lists in the same order as the coroutines, so the downstream filter loop is correct. `get_recent_games`: List comprehension `[(10 - offset) for offset in range(weeks_back) if (10 - offset) > 0]` correctly filters non-positive week numbers before constructing the gather. Semantically equivalent to the old `if week <= 0: break` guard, but without the early-exit behaviour — which is fine because the filter is applied before any I/O. `get_upcoming_games`: Early-exit (`if upcoming_games and len(upcoming_games) >= 20: break`) correctly removed. It was an optimisation that no longer makes sense when all weeks are fetched in a single parallel batch. The `weeks_ahead` parameter in the signature still exists but is now ignored — the implementation always fetches all 18 weeks regardless of its value. This is a latent behaviour mismatch between the docstring/signature and the implementation (see Suggestions below). **bot.py — pre-existing duplicate attribute assignment confirmed still present.** This was flagged in the PR #84 review and is a merge artifact from PR #83. Lines 121 and 123 of `bot.py` both assign `self.maintenance_mode: bool = False`. This PR's diff adds the `MaintenanceAwareTree` class and the `tree_cls` kwarg but does not introduce the duplicate — it was already in `next-release`. The duplicate is harmless at runtime (second assignment overwrites first with the same value) but is dead code. The `test_maintenance_mode_default_is_false` test passes despite this because it only asserts the final value, not the number of assignments. **bot.py — dead decorator block in setup_hook still present.** The old `@self.tree.interaction_check` / `maintenance_check` decorator block (lines 129–143 in the current `bot.py`) is still in `setup_hook`. It was supposed to be removed as part of the fix introduced in PR #83, and was flagged as still-present dead code in the PR #84 review. This PR does not touch `setup_hook` and so does not remove it. It generates a RuntimeWarning (unawaited coroutine) whenever the bot starts. This is a pre-existing issue, not introduced by this PR. **test_services_schedule.py — correctness of test logic verified.** `test_filters_by_team_case_insensitive`: `mock.side_effect = [week1, week2]` with `weeks=2` triggers `asyncio.gather` over two coroutines. With `AsyncMock`, `side_effect` as a list pops one value per call, which works correctly even inside `gather`. Result count assertion (`len(result) == 2`) is correct. `test_skips_negative_weeks` assertion comment says "only 10 valid weeks" (weeks 10 down to 1). `10 - 0 = 10`, `10 - 14 = -4` — the filter removes offsets 10–14, leaving offsets 0–9, which is 10 weeks. Correct. `test_returns_only_incomplete_games` asserts `len(result) == 18` because `mock.return_value = [completed, upcoming]` is called for all 18 weeks, returning 1 incomplete game each time. This is a valid whitebox assertion but tightly couples the test to the implementation detail of always fetching exactly 18 weeks. If that constant changes, the assertion will break. Low risk, worth noting. **test_bot_maintenance_tree.py — `test_maintenance_on_message_has_no_emoji` is overly strict.** The test checks `unicodedata.category(ch) != "So"` (Other Symbol) for each character in the maintenance message. This correctly catches most emoji. However, the project's "no emoji in bot messages" rule (from `discord-app-v2/CLAUDE.md`) specifically applies to `EmbedTemplate.success/error/warning/info/loading()` title parameters — it is a pattern about double-emoji in embeds, not a blanket ban on all Unicode symbols in all messages. The current test would falsely fail if the message were changed to include a legitimate typographic symbol (e.g., a copyright sign, `©`, category `So`). This is a minor over-constraint, not a blocking issue. **`_patch_discord_app_commands` autouse fixture is a no-op.** The fixture at `tests/test_bot_maintenance_tree.py:68` yields immediately with no setup or teardown. The docstring says it stubs the parent `CommandTree.__init__`, but the body does nothing. The tests pass because `interaction_check` is called as an unbound method (`tree_cls.interaction_check(MagicMock(), interaction)`) without ever constructing a `MaintenanceAwareTree` instance, so `__init__` is never invoked. The fixture is not harmful but is misleading — it promises patching it does not perform. #### Security No security issues. The `getattr(bot, "maintenance_mode", False)` safe-access is correct and avoids `AttributeError` during startup races. Admin check uses `guild_permissions.administrator`, which is the appropriate guild-level flag. #### Style & Conventions All modified files follow the project's double-quote, trailing-comma style (enforced by the linter in previous PRs). `asyncio` import is at module level, not lazy-imported mid-file, consistent with CLAUDE.md. Docstrings are present on all new methods and test classes. `@logged_command` and `self.logger` are not applicable here (schedule_service and tests, not a cog command). #### Suggestions 1. **`weeks_ahead` parameter in `get_upcoming_games` is now ignored.** The signature is `get_upcoming_games(self, season: int, weeks_ahead: int = 6)` but the body always gathers `range(1, 19)`. Either remove the parameter (breaking change for callers) or restore the behaviour by using `weeks_ahead` in the gather range. The PR body notes the early exit was "unnecessary when all weeks fetch in parallel" — but `weeks_ahead` was a different concept (how far ahead to look, not an optimisation gate). Worth a follow-up to reconcile the signature and implementation. 2. **Dead code in `bot.py` — duplicate `self.maintenance_mode` assignment and stale `@self.tree.interaction_check` block in `setup_hook`.** These are pre-existing issues from PR #83 that survived the merge. A targeted cleanup PR removing lines 123 (`self.maintenance_mode: bool = False` duplicate) and the entire `maintenance_check` inner function block (lines 129–143) would be appropriate. 3. **`_patch_discord_app_commands` fixture.** Either remove it (the tests work without it) or give it a real body that patches `discord.app_commands.CommandTree.__init__` to a no-op, matching its stated intent. 4. **`test_returns_only_incomplete_games` magic number.** Consider replacing the literal `18` with a named constant or deriving it from the implementation's `range(1, 19)` to make the coupling explicit. ### Verdict: COMMENT The schedule service parallelization is correct and well-tested. The 16 new tests cover the parallelization paths, edge cases, and the `group_games_by_series` grouping logic. No security issues. The suggestions above (particularly the `weeks_ahead` parameter mismatch) are worth addressing in a follow-up, but none of them block merging. Posting as COMMENT per convention — Gitea does not allow self-approval. --- *Automated review by Claude PR Reviewer*
cal added 1 commit 2026-03-20 15:11:26 +00:00 
The parameter was already ignored (body hardcodes range(1, 19)).
Remove from signature and the one caller that passed it.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Claude added the
ai-reviewing
 label 2026-03-20 15:15:11 +00:00
cal reviewed 2026-03-20 15:15:21 +00:00
cal left a comment
Owner
Copy Link

AI Code Review

Files Reviewed

  • services/schedule_service.py (modified)
  • commands/league/schedule.py (modified — reformatting + call-site update)
  • bot.py (modified — maintenance-mode duplicate assignment still present)
  • commands/admin/management.py (modified — type: ignore added)
  • tests/test_services_schedule.py (added)
  • tests/test_bot_maintenance_tree.py (added)

Findings

Correctness

schedule_service.py — get_recent_games: hardcoded "current week = 10"

This is a pre-existing design limitation, not introduced by this PR, but the parallelization makes it more visible. The weeks-to-fetch list is computed as:

weeks_to_fetch = [(10 - offset) for offset in range(weeks_back) if (10 - offset) > 0]

Week 10 is still hardcoded as the anchor, so this method is only correct when the league is actually at week 10. This was true in the sequential version too; the PR preserves the behaviour faithfully. No change required here, but worth a follow-up issue if the service is ever used in production for real-time recent games.

test_sorted_descending_by_week_and_game_num — test uses mock.side_effect with a list against asyncio.gather

asyncio.gather calls get_week_schedule(season, 10) and get_week_schedule(season, 9) concurrently. side_effect as a list pops values in call order, which for a list-based side_effect is the order gather dispatches them. Because gather dispatches in argument order and Python's mock pops in FIFO order, week 10 gets [game_w10] and week 9 gets [game_w9]. This is technically correct but relies on asyncio.gather preserving dispatch order (which it does — the coroutines are started in argument order). The assertion passes and is trustworthy.

bot.py — duplicate self.maintenance_mode: bool = False assignment (lines 121 and 123)

This is a pre-existing merge artifact noted in the PR #84 review. This PR adds test_bot_maintenance_tree.py which imports bot.py, so the duplicate assignment is now tested code. The test_maintenance_mode_default_is_false test still passes because assigning the attribute twice has no runtime effect — but the dead line is still a code smell. Not introduced by this PR.

bot.py — dead @self.tree.interaction_check decorator block (lines 129–143) NOT removed

The diff shows bot.py was modified only to add MaintenanceAwareTree and the tree_cls kwarg plus the duplicate maintenance_mode line. The dead decorator block in setup_hook (lines 129–143) is still present in the file. This means bot.py now has two maintenance-check paths: the correct one (via MaintenanceAwareTree.interaction_check) and the dead decorator that generates a RuntimeWarning: coroutine 'maintenance_check' was never awaited at startup because @self.tree.interaction_check is not a valid decorator in discord.py.

This was the core bug of issue #82 / PR #84. The new test_bot_maintenance_tree.py thoroughly tests the MaintenanceAwareTree class but does not test or exercise the dead decorator block, so its continued presence is not caught by the new tests. The decorator block should be removed before merge.

get_upcoming_games — early-exit removal trade-off

The old code had an early exit at 20 games to limit unnecessary requests. The new version always fetches all 18 weeks. For a mid-season call where weeks 1–10 are complete and weeks 11–18 are upcoming, this is strictly better (all 18 fetches run in parallel vs. the old sequential scan that would have stopped early anyway). The trade-off is that an all-season call now always costs 18 parallel HTTP requests rather than stopping as soon as a sufficient window of upcoming games is found. For this league size this is acceptable, and the PR body documents it. No objection.

commands/league/schedule.py line 160 — weeks_ahead removed from caller

schedule_service.get_upcoming_games(season),  # was: get_upcoming_games(season, weeks_ahead=1)

The old call passed weeks_ahead=1, meaning only 1 week ahead was scanned. After the parameter removal, the new call fetches all 18 weeks. This is a behaviour change: the /schedule command will now return upcoming games from all future weeks rather than just the next week. This may or may not be desirable depending on what the embed displays. The embed caps output at 5 games, so the visual change is minor; the extra API calls are the real cost. Worth noting for manual testing.

Security

No issues found. No user input is used in API parameters beyond integers that are passed directly. No credentials or tokens are touched.

Style & Conventions

commands/league/schedule.py — the diff is predominantly mechanical reformatting (trailing whitespace, trailing commas, line-length wrapping). All changes conform to project style. The formatter also fixed a missing newline at end-of-file.

commands/admin/management.py — adding # type: ignore[attr-defined] is correct since commands.Bot does not declare maintenance_mode; the attribute is set dynamically in SBABot.__init__.

Test files — docstrings explain both "what" and "why" as required by CLAUDE.md. Factory usage is correct. autouse fixtures are not needed here (no module-level state) but the pattern is harmless.

Suggestions

  1. Remove the dead @self.tree.interaction_check block from setup_hook (lines 129–143 in bot.py). This is the actual fix for issue #82 and is still outstanding. Without it, a RuntimeWarning fires at startup and the decorator silently competes with the tree subclass.

  2. Fix the duplicate self.maintenance_mode: bool = False assignment (lines 121 and 123 in bot.py). One of these lines should be removed.

  3. get_recent_games anchoring at week 10 is a pre-existing design gap. A follow-up issue to use the current week from league_service would make recent-games queries correct across the full season.

Verdict: COMMENT

The asyncio.gather parallelization in schedule_service.py is correctly implemented — gather dispatch order is preserved, all three methods produce the same results as the sequential versions, and the 16 new tests are well-structured and cover the critical paths.

However, bot.py still contains the dead @self.tree.interaction_check decorator block (lines 129–143) that was the root cause of issue #82, and the duplicate maintenance_mode assignment (lines 121/123). Both of these were flagged in the PR #84 review. This PR adds tests that import bot.py and thoroughly test MaintenanceAwareTree, but does not clean up the dead code. These two issues in bot.py should be resolved before merge — they are one-line and one-block removals.

The schedule service changes themselves are ready to merge.

Automated review by Claude PR Reviewer

## AI Code Review ### Files Reviewed - `services/schedule_service.py` (modified) - `commands/league/schedule.py` (modified — reformatting + call-site update) - `bot.py` (modified — maintenance-mode duplicate assignment still present) - `commands/admin/management.py` (modified — `type: ignore` added) - `tests/test_services_schedule.py` (added) - `tests/test_bot_maintenance_tree.py` (added) --- ### Findings #### Correctness **schedule_service.py — `get_recent_games`: hardcoded "current week = 10"** This is a pre-existing design limitation, not introduced by this PR, but the parallelization makes it more visible. The weeks-to-fetch list is computed as: ```python weeks_to_fetch = [(10 - offset) for offset in range(weeks_back) if (10 - offset) > 0] ``` Week 10 is still hardcoded as the anchor, so this method is only correct when the league is actually at week 10. This was true in the sequential version too; the PR preserves the behaviour faithfully. No change required here, but worth a follow-up issue if the service is ever used in production for real-time recent games. **`test_sorted_descending_by_week_and_game_num` — test uses `mock.side_effect` with a list against `asyncio.gather`** `asyncio.gather` calls `get_week_schedule(season, 10)` and `get_week_schedule(season, 9)` concurrently. `side_effect` as a list pops values in call order, which for a list-based `side_effect` is the order `gather` dispatches them. Because `gather` dispatches in argument order and Python's mock pops in FIFO order, week 10 gets `[game_w10]` and week 9 gets `[game_w9]`. This is technically correct but relies on `asyncio.gather` preserving dispatch order (which it does — the coroutines are started in argument order). The assertion passes and is trustworthy. **`bot.py` — duplicate `self.maintenance_mode: bool = False` assignment (lines 121 and 123)** This is a pre-existing merge artifact noted in the PR #84 review. This PR adds `test_bot_maintenance_tree.py` which imports `bot.py`, so the duplicate assignment is now tested code. The `test_maintenance_mode_default_is_false` test still passes because assigning the attribute twice has no runtime effect — but the dead line is still a code smell. Not introduced by this PR. **`bot.py` — dead `@self.tree.interaction_check` decorator block (lines 129–143) NOT removed** The diff shows `bot.py` was modified only to add `MaintenanceAwareTree` and the `tree_cls` kwarg plus the duplicate `maintenance_mode` line. The dead decorator block in `setup_hook` (lines 129–143) is still present in the file. This means `bot.py` now has two maintenance-check paths: the correct one (via `MaintenanceAwareTree.interaction_check`) and the dead decorator that generates a `RuntimeWarning: coroutine 'maintenance_check' was never awaited` at startup because `@self.tree.interaction_check` is not a valid decorator in discord.py. This was the core bug of issue #82 / PR #84. The new `test_bot_maintenance_tree.py` thoroughly tests the `MaintenanceAwareTree` class but does not test or exercise the dead decorator block, so its continued presence is not caught by the new tests. The decorator block should be removed before merge. **`get_upcoming_games` — early-exit removal trade-off** The old code had an early exit at 20 games to limit unnecessary requests. The new version always fetches all 18 weeks. For a mid-season call where weeks 1–10 are complete and weeks 11–18 are upcoming, this is strictly better (all 18 fetches run in parallel vs. the old sequential scan that would have stopped early anyway). The trade-off is that an all-season call now always costs 18 parallel HTTP requests rather than stopping as soon as a sufficient window of upcoming games is found. For this league size this is acceptable, and the PR body documents it. No objection. **`commands/league/schedule.py` line 160 — `weeks_ahead` removed from caller** ```python schedule_service.get_upcoming_games(season), # was: get_upcoming_games(season, weeks_ahead=1) ``` The old call passed `weeks_ahead=1`, meaning only 1 week ahead was scanned. After the parameter removal, the new call fetches all 18 weeks. This is a behaviour change: the `/schedule` command will now return upcoming games from all future weeks rather than just the next week. This may or may not be desirable depending on what the embed displays. The embed caps output at 5 games, so the visual change is minor; the extra API calls are the real cost. Worth noting for manual testing. #### Security No issues found. No user input is used in API parameters beyond integers that are passed directly. No credentials or tokens are touched. #### Style & Conventions **`commands/league/schedule.py`** — the diff is predominantly mechanical reformatting (trailing whitespace, trailing commas, line-length wrapping). All changes conform to project style. The formatter also fixed a missing newline at end-of-file. **`commands/admin/management.py`** — adding `# type: ignore[attr-defined]` is correct since `commands.Bot` does not declare `maintenance_mode`; the attribute is set dynamically in `SBABot.__init__`. **Test files** — docstrings explain both "what" and "why" as required by CLAUDE.md. Factory usage is correct. `autouse` fixtures are not needed here (no module-level state) but the pattern is harmless. #### Suggestions 1. **Remove the dead `@self.tree.interaction_check` block from `setup_hook`** (lines 129–143 in `bot.py`). This is the actual fix for issue #82 and is still outstanding. Without it, a `RuntimeWarning` fires at startup and the decorator silently competes with the tree subclass. 2. **Fix the duplicate `self.maintenance_mode: bool = False` assignment** (lines 121 and 123 in `bot.py`). One of these lines should be removed. 3. `get_recent_games` anchoring at week 10 is a pre-existing design gap. A follow-up issue to use the current week from `league_service` would make recent-games queries correct across the full season. --- ### Verdict: COMMENT The `asyncio.gather` parallelization in `schedule_service.py` is correctly implemented — gather dispatch order is preserved, all three methods produce the same results as the sequential versions, and the 16 new tests are well-structured and cover the critical paths. However, `bot.py` still contains the dead `@self.tree.interaction_check` decorator block (lines 129–143) that was the root cause of issue #82, and the duplicate `maintenance_mode` assignment (lines 121/123). Both of these were flagged in the PR #84 review. This PR adds tests that import `bot.py` and thoroughly test `MaintenanceAwareTree`, but does not clean up the dead code. These two issues in `bot.py` should be resolved before merge — they are one-line and one-block removals. The schedule service changes themselves are ready to merge. --- *Automated review by Claude PR Reviewer*
cal merged commit 52fa56cb69 into next-release 2026-03-20 15:16:41 +00:00
cal deleted branch ai/major-domo-v2-88 2026-03-20 15:16:42 +00:00
Claude reviewed 2026-03-20 15:18:40 +00:00
Claude left a comment
Author
Collaborator
Copy Link

AI Code Review

Files Reviewed

  • services/schedule_service.py (modified)
  • commands/league/schedule.py (modified)
  • bot.py (modified)
  • commands/admin/management.py (modified)
  • tests/test_services_schedule.py (added)
  • tests/test_bot_maintenance_tree.py (added)

Findings

Correctness

  • asyncio.gather parallelization — correct in all three methods. get_week_schedule wraps its body in try/except and returns [] on any error, so individual week failures degrade gracefully and won't propagate through gather.
  • weeks_ahead parameter removal — correct. The old parameter was dead code: get_upcoming_games always scanned range(1, 19) regardless of the weeks_ahead value. Removing it and updating the schedule.py call site is the honest fix.
  • Early-exit removal — The old if upcoming_games and len(upcoming_games) >= 20: break is no longer applicable once all 18 weeks are fetched in parallel. Removing it is correct. Tradeoff: always 18 API calls vs. potentially fewer. For a standard mid-season scenario this is acceptable.
  • Sort order preserved — get_recent_games sorts reverse=True (most recent first), get_upcoming_games sorts ascending (soonest first). Both preserved correctly.
  • get_recent_games hardcoded week 10 — Pre-existing issue, not introduced here. weeks_to_fetch = [(10 - offset) for offset in range(weeks_back) if (10 - offset) > 0] still hardcodes the "current week" as 10. This was already a known limitation in the original code.

Bundled Unrelated Changes

This "perf: parallelize schedule_service" PR also includes changes to bot.py, commands/admin/management.py, and a new tests/test_bot_maintenance_tree.py. These are from a different concern (the MaintenanceAwareTree fix) and should ideally have been separate. They appear here because the ai/ branch was cut before PR #83 hotfixed main directly — so next-release legitimately lacked MaintenanceAwareTree and needed it. The implementation is correct. The test_bot_maintenance_tree.py tests are thorough (8 cases covering maintenance-off, maintenance-on/non-admin, maintenance-on/admin, non-Member user, missing attribute fallback, and default-false attribute).

Pre-existing Bug (not introduced here)

bot.py lines 121 and 123 assign self.maintenance_mode: bool = False twice — this is a merge artifact from PR #83 that was already noted in the previous review. Not introduced by this PR, but also not cleaned up.

Security

  • No issues found.

Style & Conventions

  • commands/league/schedule.py received cosmetic reformatting (single→double quotes, trailing commas, blank-line cleanup). Consistent with linter style seen in previous PRs — cosmetic only.
  • New tests follow GameFactory/TeamFactory patterns as required per the /simplify review mentioned in the PR body.

Suggestions

  • The hardcoded week = 10 - offset in get_recent_games should eventually be replaced with a call to league_service.get_current_week() so the "recent games" window is accurate regardless of where in the season the bot is. Could be a follow-up issue.
  • test_services_schedule.py TestGetRecentGames.test_sorted_descending_by_week_and_game_num verifies sort order but only checks the .week attribute. Consider also asserting on .game_num for the descending sort to fully exercise the composite key.

Verdict: COMMENT

Schedule parallelization is correct and well-tested — 16 new tests cover all three methods plus group_games_by_series. The MaintenanceAwareTree changes bundled here were necessary for next-release (since the PR #83 hotfix targeted main directly) and are also correct. No blocking issues.

Automated review by Claude PR Reviewer

## AI Code Review ### Files Reviewed - `services/schedule_service.py` (modified) - `commands/league/schedule.py` (modified) - `bot.py` (modified) - `commands/admin/management.py` (modified) - `tests/test_services_schedule.py` (added) - `tests/test_bot_maintenance_tree.py` (added) ### Findings #### Correctness - **`asyncio.gather` parallelization** — correct in all three methods. `get_week_schedule` wraps its body in try/except and returns `[]` on any error, so individual week failures degrade gracefully and won't propagate through `gather`. - **`weeks_ahead` parameter removal** — correct. The old parameter was dead code: `get_upcoming_games` always scanned `range(1, 19)` regardless of the `weeks_ahead` value. Removing it and updating the `schedule.py` call site is the honest fix. - **Early-exit removal** — The old `if upcoming_games and len(upcoming_games) >= 20: break` is no longer applicable once all 18 weeks are fetched in parallel. Removing it is correct. Tradeoff: always 18 API calls vs. potentially fewer. For a standard mid-season scenario this is acceptable. - **Sort order preserved** — `get_recent_games` sorts reverse=True (most recent first), `get_upcoming_games` sorts ascending (soonest first). Both preserved correctly. - **`get_recent_games` hardcoded week 10** — Pre-existing issue, not introduced here. `weeks_to_fetch = [(10 - offset) for offset in range(weeks_back) if (10 - offset) > 0]` still hardcodes the "current week" as 10. This was already a known limitation in the original code. #### Bundled Unrelated Changes This "perf: parallelize schedule_service" PR also includes changes to `bot.py`, `commands/admin/management.py`, and a new `tests/test_bot_maintenance_tree.py`. These are from a different concern (the `MaintenanceAwareTree` fix) and should ideally have been separate. They appear here because the `ai/` branch was cut before PR #83 hotfixed `main` directly — so `next-release` legitimately lacked `MaintenanceAwareTree` and needed it. The implementation is correct. The `test_bot_maintenance_tree.py` tests are thorough (8 cases covering maintenance-off, maintenance-on/non-admin, maintenance-on/admin, non-Member user, missing attribute fallback, and default-false attribute). #### Pre-existing Bug (not introduced here) `bot.py` lines 121 and 123 assign `self.maintenance_mode: bool = False` twice — this is a merge artifact from PR #83 that was already noted in the previous review. Not introduced by this PR, but also not cleaned up. #### Security - No issues found. #### Style & Conventions - `commands/league/schedule.py` received cosmetic reformatting (single→double quotes, trailing commas, blank-line cleanup). Consistent with linter style seen in previous PRs — cosmetic only. - New tests follow `GameFactory`/`TeamFactory` patterns as required per the `/simplify` review mentioned in the PR body. #### Suggestions - The hardcoded `week = 10 - offset` in `get_recent_games` should eventually be replaced with a call to `league_service.get_current_week()` so the "recent games" window is accurate regardless of where in the season the bot is. Could be a follow-up issue. - `test_services_schedule.py` `TestGetRecentGames.test_sorted_descending_by_week_and_game_num` verifies sort order but only checks the `.week` attribute. Consider also asserting on `.game_num` for the descending sort to fully exercise the composite key. ### Verdict: COMMENT Schedule parallelization is correct and well-tested — 16 new tests cover all three methods plus `group_games_by_series`. The `MaintenanceAwareTree` changes bundled here were necessary for `next-release` (since the PR #83 hotfix targeted `main` directly) and are also correct. No blocking issues. --- *Automated review by Claude PR Reviewer*
Claude added
ai-reviewed
 and removed
ai-reviewing
 labels 2026-03-20 15:19:09 +00:00
Sign in to join this conversation.
Reviewers
No reviewers
Labels
Clear labels   
ai-changes-requested

   
ai-pr-opened

   
ai-reviewed

   
ai-reviewing

   
ai-working

   
in-next-release

Fix included in the next-release branch, pending merge to main

  
status/in-progress

Work is actively in progress

  
status/pr-open

A pull request has been opened for this issue

No Label
ai-changes-requested
ai-pr-opened
ai-reviewed
ai-reviewing
ai-working
in-next-release
status/in-progress
status/pr-open
Milestone
Clear milestone
No items
No Milestone
Projects
Clear projects
No project
Assignees
Clear assignees
No Assignees
2 Participants
Notifications
Due Date
The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: cal/major-domo-v2#103
No description provided.
Delete Branch "ai/major-domo-v2-88"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?