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

Merge next-release into main #81

Merged
cal merged 34 commits from next-release into main 2026-03-17 16:44:45 +00:00
Conversation 0 Commits 34 Files Changed 42 +3245 -2596
cal commented 2026-03-11 18:06:38 +00:00
Owner
Copy Link

This PR merges the next-release branch into main, incorporating all changes since the last release. It includes dependency pinning, workflow improvements, and various fixes. No conflicts with recent hot-patches as the hotfix for stale roster cache is already included in next-release.

This PR merges the next-release branch into main, incorporating all changes since the last release. It includes dependency pinning, workflow improvements, and various fixes. No conflicts with recent hot-patches as the hotfix for stale roster cache is already included in next-release.
cal added 33 commits 2026-03-11 18:06:40 +00:00 
The else branch in descriptive_text() caught both "away leading" and
"tied" cases, always overwriting the tied text. Changed to if/elif/else
so tied scores display correctly.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Reviewed-on: #52
Prevents users from managing injuries for players not on their team.
Admins bypass the check; org affiliates (MiL/IL) are recognized.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
/trade add-player hardcoded from_roster=MAJOR_LEAGUE for all players,
causing incorrect transaction records when trading MiL/IL players. This
cascaded into false roster validation errors for other teams' /dropadd
commands because pre-existing transaction processing misidentified which
roster a player was leaving from.

Now looks up the player's actual team and calls roster_type() to determine
the correct source roster. Same fix applied to /trade supplementary.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Codebase audit identified ~50 lazy imports. Moved 42 unnecessary ones to
top-level imports — only keeping those justified by circular imports,
init-order dependencies, or optional dependency guards. Updated test mock
patch targets where needed. See #57 for remaining DI candidates.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Branch from next-release for normal work, main only for hotfixes.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Reviewed-on: #58
TransactionBuilder cached roster data indefinitely via _roster_loaded flag,
causing validation to use stale counts when builders persisted across multiple
/ilmove invocations. Now validate_transaction() always fetches fresh roster
data. Also adds dependency injection for roster_service to improve testability.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Simplify review found that force_refresh=True on every validate_transaction()
call caused redundant API fetches on every embed render and button press.

Instead, invalidate the roster cache after successful submit_transaction() so
the next operation fetches fresh data. This preserves the cache for normal
interaction flows while still preventing stale data after submissions.

Also adds type annotation for roster_svc DI parameter.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Reviewed-on: #60
- Add `maintenance_mode: bool = False` flag to `SBABot.__init__`
- Register a global `@tree.interaction_check` that blocks non-admin users
  from all commands when maintenance mode is active
- Update `admin_maintenance` command to set `self.bot.maintenance_mode`
  and log the state change, replacing the no-op placeholder comment

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Reviewed-on: #62
fix: update roster labels to use Minor League and Injured List (#59)
All checks were successful
Build Docker Image / build (pull_request) Successful in 1m10s
Details
346e36bfc6 
- Add section headers (Active Roster, Minor League, Injured List) to the
  main summary embed in _create_roster_embeds so each roster section is
  clearly labeled for users
- Fix incorrect docstring in team_service.get_team_roster that had the
  shortil/longil mapping backwards

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
refactor: extract duplicate command hash logic into _compute_command_hash (#31)
All checks were successful
Build Docker Image / build (pull_request) Successful in 1m6s
Details
2242140dcd 
Both _should_sync_commands and _save_command_hash contained an identical
~35-line block building the command data list and computing the SHA-256 hash.
Extracted into a new synchronous helper method _compute_command_hash() that
both callers now delegate to.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
perf: reuse persistent aiohttp.ClientSession in GiphyService (#26)
All checks were successful
Build Docker Image / build (pull_request) Successful in 1m5s
Details
e9dd318e5a 
Both get_disappointment_gif and get_gif previously created a new
ClientSession per call. Replace with a lazily-initialised shared
session stored on the instance, eliminating per-call TCP handshake
and DNS overhead.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
test: implement test_validate_transaction_exception_handling (#35)
All checks were successful
Build Docker Image / build (pull_request) Successful in 1m6s
Details
ebec9a720b 
Removes the @pytest.mark.skip and implements the test using a
patched RosterValidation that raises on the first call, triggering
the except clause in validate_transaction. Verifies the method
returns is_legal=False with a descriptive error message.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Adds next-release branch trigger and replaces separate dev/production
build steps with the shared docker-tags action for tag resolution.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Reviewed-on: #63
Reviewed-on: #65
Reviewed-on: #66
Reviewed-on: #67
Reviewed-on: #68
fix: prefix trade validation errors with team abbreviation
All checks were successful
Build Docker Image / build (pull_request) Successful in 1m5s
Details
8ee35e564c 
Roster-level errors, warnings, and suggestions now display as
"[BSG] Too many ML players" so users know which team each issue
applies to.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Trade validation now automatically fetches the current week from
league_service and validates against next week's projected roster
(including pending transactions), matching /dropadd behavior.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
The TransactionBuilder cached pre-existing transactions on first load
and never refreshed them. This meant transactions submitted by other
sessions (or newly visible after API fixes) were invisible for the
lifetime of the builder session, causing incorrect roster projections.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Add centralized `is_admin(interaction)` helper that includes the
`isinstance(interaction.user, discord.Member)` guard, preventing
AttributeError in DM contexts.

Use it in `can_edit_player_image()` which previously accessed
`guild_permissions.administrator` directly without the isinstance
guard. Update the corresponding test to mock the user with
`spec=discord.Member` so the isinstance check passes.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Reviewed-on: #64
Reviewed-on: #71
Read all spreadsheet data (plays, box score, pitching decisions) before any
database writes so formula errors like #N/A don't leave the DB in a partial
state. Also preserve SheetsException detail through the error chain and show
users the specific cell/error instead of a generic failure message.

Closes #78

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Reviewed-on: #80
Merge pull request 'fix: re-fetch existing transactions on each validation' (#72) from fix/refresh-existing-transactions-on-validation into next-release
All checks were successful
Build Docker Image / build (push) Successful in 1m24s
Details
Build Docker Image / build (pull_request) Successful in 57s
Details
541380acdf 
Reviewed-on: #72
Claude added the
ai-reviewing
 label 2026-03-11 18:15:47 +00:00
Claude reviewed 2026-03-11 18:18:55 +00:00
Claude left a comment
Collaborator
Copy Link

AI Code Review

Files Reviewed

  • .gitea/workflows/docker-build.yml (modified — multi-channel release support)
  • CLAUDE.md (modified — branching workflow docs)
  • bot.py (modified — maintenance mode, _compute_command_hash() refactor)
  • utils/permissions.py (modified — is_admin() helper, reformatting)
  • commands/admin/management.py (modified — cosmetic reformatting only)
  • commands/injuries/management.py (modified — team ownership verification)
  • commands/league/submit_scorecard.py (modified — read-all-before-writes, SheetsException handler)
  • services/roster_service.py (modified — lazy import moved to top-level)
  • services/giphy_service.py (modified — persistent aiohttp session)
  • services/trade_builder.py, services/transaction_builder.py (modified — previously reviewed)
  • tests/test_injury_ownership.py (added)
  • tests/test_models_play.py (added)

Findings

Correctness

Scorecard read-all-before-writes (#80): Correct and well-motivated. All three spreadsheet reads (read_playtable_data, read_box_score, read_pitching_decisions) now happen before any DB writes in Phase 6. WP/LP validation also moved before writes, so a missing pitcher no longer leaves orphaned play rows. The new SheetsException catch block rolls back correctly using rollback_state — GAME_PATCHED triggers both wipe and play deletion; PLAYS_POSTED triggers only play deletion. Logic is sound.

Maintenance mode (#28): self.maintenance_mode: bool = False is set on the bot in __init__, and tree.interaction_check reads it. The /admin-maintenance command should toggle self.bot.maintenance_mode. I didn't see the toggle logic in this diff (it lives in management.py), but checking the current file should confirm it was wired up correctly.

is_admin() helper (#55): Clean implementation. The isinstance(interaction.user, discord.Member) guard correctly handles DM contexts. Used in _verify_team_ownership appropriately.

_compute_command_hash() refactor: Removes duplication between _should_sync_commands and _save_command_hash — straightforward and correct.

Injury ownership check (#18): _verify_team_ownership is correctly implemented for /injury set-new and /injury clear. Admin bypass works (isinstance guard + administrator check). The player.team_id guard (returns True if no team data) is reasonable defensive coding. is_same_organization handles MiL affiliates correctly.

Style & Conventions

Redundant lazy import in _verify_team_ownership (commands/injuries/management.py ~line 92): The PR adds from services.team_service import team_service at the top level (line 22), but _verify_team_ownership also contains from services.team_service import team_service as a mid-method lazy import. CLAUDE.md explicitly states "Never add lazy imports to middle of file." Since the top-level import was added in this PR, the inline import inside _verify_team_ownership is redundant and should be removed. The pre-existing lazy imports in injury_roll (line 510 and 700) are out-of-scope for this PR.

Stale TODO comment (commands/injuries/management.py ~line 515): injury_roll still has # TODO: Add team ownership verification placeholder comments from before the fix was implemented. These weren't introduced by this PR but were in scope of the ownership work — worth cleaning up.

Security

No issues found. The is_admin() guard is properly placed before bypassing ownership checks. No hardcoded credentials, no user input passed to dangerous APIs, no injection vectors.

Test Coverage

  • tests/test_injury_ownership.py (245 lines) — thorough coverage of _verify_team_ownership: admin bypass, owner passes, non-owner blocked, no-team case, no-team-id case. Uses model_construct to skip Pydantic validation (correct pattern for this codebase).
  • tests/test_models_play.py — added for the play model.
  • Scorecard changes have no dedicated unit tests (hard to unit test async spreadsheet reads), but the logic is straightforward and integration-tested via real runs.

Suggestions

  1. Remove redundant lazy import: In _verify_team_ownership, delete from services.team_service import team_service — the top-level import added in this same PR already provides it.
  2. Clean up stale TODO in injury_roll: Remove the three-line "# Note / # For now / # TODO" block at line 515 since the ownership infrastructure now exists (and could be applied to injury_roll in a follow-up if desired).

Verdict: COMMENT

This is a solid release merge. The scorecard read-all-before-writes fix is well-designed and correctly implemented. The injury ownership check is thorough with good test coverage. The maintenance mode wiring is clean. The one convention issue (redundant lazy import inside _verify_team_ownership) violates CLAUDE.md but is minor — the top-level import works correctly, the inline one is just dead weight. Not blocking.

Automated review by Claude PR Reviewer

## AI Code Review ### Files Reviewed - `.gitea/workflows/docker-build.yml` (modified — multi-channel release support) - `CLAUDE.md` (modified — branching workflow docs) - `bot.py` (modified — maintenance mode, `_compute_command_hash()` refactor) - `utils/permissions.py` (modified — `is_admin()` helper, reformatting) - `commands/admin/management.py` (modified — cosmetic reformatting only) - `commands/injuries/management.py` (modified — team ownership verification) - `commands/league/submit_scorecard.py` (modified — read-all-before-writes, SheetsException handler) - `services/roster_service.py` (modified — lazy import moved to top-level) - `services/giphy_service.py` (modified — persistent aiohttp session) - `services/trade_builder.py`, `services/transaction_builder.py` (modified — previously reviewed) - `tests/test_injury_ownership.py` (added) - `tests/test_models_play.py` (added) --- ### Findings #### Correctness **Scorecard read-all-before-writes (#80):** Correct and well-motivated. All three spreadsheet reads (`read_playtable_data`, `read_box_score`, `read_pitching_decisions`) now happen before any DB writes in Phase 6. WP/LP validation also moved before writes, so a missing pitcher no longer leaves orphaned play rows. The new `SheetsException` catch block rolls back correctly using `rollback_state` — `GAME_PATCHED` triggers both wipe and play deletion; `PLAYS_POSTED` triggers only play deletion. Logic is sound. **Maintenance mode (#28):** `self.maintenance_mode: bool = False` is set on the bot in `__init__`, and `tree.interaction_check` reads it. The `/admin-maintenance` command should toggle `self.bot.maintenance_mode`. I didn't see the toggle logic in this diff (it lives in `management.py`), but checking the current file should confirm it was wired up correctly. **`is_admin()` helper (#55):** Clean implementation. The `isinstance(interaction.user, discord.Member)` guard correctly handles DM contexts. Used in `_verify_team_ownership` appropriately. **`_compute_command_hash()` refactor:** Removes duplication between `_should_sync_commands` and `_save_command_hash` — straightforward and correct. **Injury ownership check (#18):** `_verify_team_ownership` is correctly implemented for `/injury set-new` and `/injury clear`. Admin bypass works (isinstance guard + administrator check). The `player.team_id` guard (returns True if no team data) is reasonable defensive coding. `is_same_organization` handles MiL affiliates correctly. #### Style & Conventions **Redundant lazy import in `_verify_team_ownership`** (`commands/injuries/management.py` ~line 92): The PR adds `from services.team_service import team_service` at the top level (line 22), but `_verify_team_ownership` also contains `from services.team_service import team_service` as a mid-method lazy import. CLAUDE.md explicitly states "Never add lazy imports to middle of file." Since the top-level import was added in this PR, the inline import inside `_verify_team_ownership` is redundant and should be removed. The pre-existing lazy imports in `injury_roll` (line 510 and 700) are out-of-scope for this PR. **Stale TODO comment** (`commands/injuries/management.py` ~line 515): `injury_roll` still has `# TODO: Add team ownership verification` placeholder comments from before the fix was implemented. These weren't introduced by this PR but were in scope of the ownership work — worth cleaning up. #### Security No issues found. The `is_admin()` guard is properly placed before bypassing ownership checks. No hardcoded credentials, no user input passed to dangerous APIs, no injection vectors. #### Test Coverage - `tests/test_injury_ownership.py` (245 lines) — thorough coverage of `_verify_team_ownership`: admin bypass, owner passes, non-owner blocked, no-team case, no-team-id case. Uses `model_construct` to skip Pydantic validation (correct pattern for this codebase). - `tests/test_models_play.py` — added for the play model. - Scorecard changes have no dedicated unit tests (hard to unit test async spreadsheet reads), but the logic is straightforward and integration-tested via real runs. #### Suggestions 1. **Remove redundant lazy import:** In `_verify_team_ownership`, delete `from services.team_service import team_service` — the top-level import added in this same PR already provides it. 2. **Clean up stale TODO in `injury_roll`:** Remove the three-line "# Note / # For now / # TODO" block at line 515 since the ownership infrastructure now exists (and could be applied to `injury_roll` in a follow-up if desired). --- ### Verdict: COMMENT This is a solid release merge. The scorecard read-all-before-writes fix is well-designed and correctly implemented. The injury ownership check is thorough with good test coverage. The maintenance mode wiring is clean. The one convention issue (redundant lazy import inside `_verify_team_ownership`) violates CLAUDE.md but is minor — the top-level import works correctly, the inline one is just dead weight. Not blocking. --- *Automated review by Claude PR Reviewer*
cal added 1 commit 2026-03-17 16:28:21 +00:00
Merge main into next-release
All checks were successful
Build Docker Image / build (push) Successful in 3m26s
Details
Build Docker Image / build (pull_request) Successful in 58s
Details
910a27e356 
Resolve conflict in views/trade_embed.py: keep main's hotfix
(emoji-stripped UI, inline validation errors) and apply next-release's
import refactor (lazy imports hoisted to top-level).

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
cal merged commit 0a2df0f36e into main 2026-03-17 16:44:45 +00:00
cal removed the
ai-reviewing
 label 2026-03-23 15:32:41 +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#81
No description provided.
Delete Branch "next-release"

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?