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

fix: replace decorator misuse with MaintenanceAwareTree subclass (#82) #84

Closed
Claude wants to merge 1 commits from ai/major-domo-v2-82 into main
pull from: ai/major-domo-v2-82
merge into: cal:main
Conversation 0 Commits 1 Files Changed 1 +20 -16
Claude commented 2026-03-17 17:31:55 +00:00
Collaborator
Copy Link

Closes #82

Problem

setup_hook used @self.tree.interaction_check as a decorator, but CommandTree.interaction_check is an async method meant to be overridden via subclassing — not a decorator factory. Python was calling the coroutine with the decorated function as its argument, producing a RuntimeWarning: coroutine 'CommandTree.interaction_check' was never awaited and leaving maintenance mode as a complete no-op.

Fix

  • Added MaintenanceAwareTree(discord.app_commands.CommandTree) subclass before SBABot that overrides interaction_check with the maintenance check logic
  • Passed tree_cls=MaintenanceAwareTree to super().__init__ in SBABot.__init__
  • Removed the now-obsolete @self.tree.interaction_check decorator block from setup_hook

The interaction_check override accesses bot.maintenance_mode via interaction.client, which is the standard pattern for tree-level checks that need bot state.

Files Changed

  • bot.py — new MaintenanceAwareTree class, tree_cls kwarg, removed decorator block

Test Results

945 passed, 2 skipped

Closes #82 ## Problem `setup_hook` used `@self.tree.interaction_check` as a decorator, but `CommandTree.interaction_check` is an async method meant to be overridden via subclassing — not a decorator factory. Python was calling the coroutine with the decorated function as its argument, producing a `RuntimeWarning: coroutine 'CommandTree.interaction_check' was never awaited` and leaving maintenance mode as a complete no-op. ## Fix - Added `MaintenanceAwareTree(discord.app_commands.CommandTree)` subclass before `SBABot` that overrides `interaction_check` with the maintenance check logic - Passed `tree_cls=MaintenanceAwareTree` to `super().__init__` in `SBABot.__init__` - Removed the now-obsolete `@self.tree.interaction_check` decorator block from `setup_hook` The `interaction_check` override accesses `bot.maintenance_mode` via `interaction.client`, which is the standard pattern for tree-level checks that need bot state. ## Files Changed - `bot.py` — new `MaintenanceAwareTree` class, `tree_cls` kwarg, removed decorator block ## Test Results 945 passed, 2 skipped
Claude added 1 commit 2026-03-17 17:31:57 +00:00
fix: replace decorator misuse with MaintenanceAwareTree subclass (#82)
All checks were successful
Build Docker Image / build (pull_request) Successful in 1m4s
Details
f7b2e452e0 
Closes #82

`@self.tree.interaction_check` in `setup_hook` called the coroutine as
a decorator factory instead of overriding it — producing an unawaited
coroutine warning and leaving maintenance mode as a no-op.

Replace with `MaintenanceAwareTree(discord.app_commands.CommandTree)` that
overrides `interaction_check`, and pass `tree_cls=MaintenanceAwareTree`
to `super().__init__`. Remove the now-obsolete decorator block from
`setup_hook`.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Claude added the
ai-reviewing
 label 2026-03-17 17:45:50 +00:00
Claude reviewed 2026-03-17 17:47:57 +00:00
Claude left a comment
Author
Collaborator
Copy Link

AI Code Review

Files Reviewed

  • bot.py (modified)

Findings

Correctness

  • Not mergeable — rebase required. This PR reports mergeable: false. Branch ai/major-domo-v2-82 was cut from commit 0a2df0f (PR #81), before PR #83 (fix/maintenance-mode-interaction-check) landed on main. PR #83 already added the MaintenanceAwareTree class and tree_cls=MaintenanceAwareTree to main, creating a direct conflict with the identical additions in this PR.
  • Duplicate class would result on merge. The MaintenanceAwareTree class definition (lines 67–103 in current main) and the tree_cls=MaintenanceAwareTree kwarg (line 118) already exist. Merging as-is would produce a duplicate class at module scope.
  • The underlying fix logic is correct: getattr(bot, "maintenance_mode", False) is the right safe-access pattern for interaction.client, the admin permission check is sound, and the ephemeral response is appropriate.

Security

  • No issues found.

Style & Conventions

  • # type: ignore[assignment] on bot = interaction.client is correct — interaction.client is typed as discord.Client, narrowing to SBABot requires the ignore.
  • Implementation follows discord.py's documented CommandTree subclassing pattern.

Suggestions

  • After rebasing on current main, the only change needed is removing the stale @self.tree.interaction_check decorator block from setup_hook (lines 129–143 in current main). The class addition and tree_cls kwarg are already in place — this becomes a one-hunk, ~16-line removal.
  • Pre-existing on main (not introduced by this PR): self.maintenance_mode: bool = False is assigned twice in SBABot.__init__ (lines 121 and 123) — apparent merge artifact from PR #83. Worth a follow-up cleanup commit.

Verdict: REQUEST_CHANGES

The fix is conceptually correct and the logic is sound, but the PR cannot be merged in its current state (mergeable: false). Rebase ai/major-domo-v2-82 on current main, resolve the conflict (drop the MaintenanceAwareTree class addition since it's already present), and the result should be a clean removal of the dead decorator block that merges immediately.

Automated review by Claude PR Reviewer

## AI Code Review ### Files Reviewed - `bot.py` (modified) ### Findings #### Correctness - **Not mergeable — rebase required.** This PR reports `mergeable: false`. Branch `ai/major-domo-v2-82` was cut from commit `0a2df0f` (PR #81), before PR #83 (`fix/maintenance-mode-interaction-check`) landed on `main`. PR #83 already added the `MaintenanceAwareTree` class and `tree_cls=MaintenanceAwareTree` to `main`, creating a direct conflict with the identical additions in this PR. - **Duplicate class would result on merge.** The `MaintenanceAwareTree` class definition (lines 67–103 in current `main`) and the `tree_cls=MaintenanceAwareTree` kwarg (line 118) already exist. Merging as-is would produce a duplicate class at module scope. - The underlying fix logic is correct: `getattr(bot, "maintenance_mode", False)` is the right safe-access pattern for `interaction.client`, the admin permission check is sound, and the ephemeral response is appropriate. #### Security - No issues found. #### Style & Conventions - `# type: ignore[assignment]` on `bot = interaction.client` is correct — `interaction.client` is typed as `discord.Client`, narrowing to `SBABot` requires the ignore. - Implementation follows discord.py's documented `CommandTree` subclassing pattern. #### Suggestions - After rebasing on current `main`, the only change needed is removing the stale `@self.tree.interaction_check` decorator block from `setup_hook` (lines 129–143 in current `main`). The class addition and `tree_cls` kwarg are already in place — this becomes a one-hunk, ~16-line removal. - Pre-existing on `main` (not introduced by this PR): `self.maintenance_mode: bool = False` is assigned twice in `SBABot.__init__` (lines 121 and 123) — apparent merge artifact from PR #83. Worth a follow-up cleanup commit. ### Verdict: REQUEST_CHANGES The fix is conceptually correct and the logic is sound, but the PR cannot be merged in its current state (`mergeable: false`). Rebase `ai/major-domo-v2-82` on current `main`, resolve the conflict (drop the `MaintenanceAwareTree` class addition since it's already present), and the result should be a clean removal of the dead decorator block that merges immediately. --- *Automated review by Claude PR Reviewer*
Claude added
ai-changes-requested
 and removed
ai-reviewing
 labels 2026-03-17 17:48:34 +00:00
cal closed this pull request 2026-03-17 17:49:27 +00:00
All checks were successful
Build Docker Image / build (pull_request) Successful in 1m4s
Details

Pull request closed

Please reopen this pull request to perform a merge.
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
1 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#84
No description provided.
Delete Branch "ai/major-domo-v2-82"

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?