feat: enforce trade deadline in /trade commands #121

cal · 2026-03-30T21:44:05Z

cal commented

2026-03-30 21:44:05 +00:00

Summary

Adds is_past_trade_deadline property to the Current model (offseason-aware, inclusive of deadline week)
Guards /trade initiate, Submit button, and _finalize_trade against trades after the deadline
Fails closed: if the API returns no current data, trades are blocked

Tests

4 new tests (1 model-level, 3 command-level) covering past-deadline block, at-deadline allow, and None/fail-closed
All 981 tests pass

Test plan

python -m pytest — all 981 pass
Optionally deploy to dev and test with a past-deadline trade_deadline value

🤖 Generated with Claude Code

## Summary - Adds `is_past_trade_deadline` property to the `Current` model (offseason-aware, inclusive of deadline week) - Guards `/trade initiate`, Submit button, and `_finalize_trade` against trades after the deadline - Fails closed: if the API returns no current data, trades are blocked ## Tests - 4 new tests (1 model-level, 3 command-level) covering past-deadline block, at-deadline allow, and None/fail-closed - All 981 tests pass ## Test plan - [x] `python -m pytest` — all 981 pass - [ ] Optionally deploy to dev and test with a past-deadline `trade_deadline` value 🤖 Generated with [Claude Code](https://claude.com/claude-code)

cal added 1 commit 2026-03-30 21:44:06 +00:00

feat: enforce trade deadline in /trade commands b872a05397

Add is_past_trade_deadline property to Current model and guard /trade initiate,
submit, and finalize flows. All checks fail-closed (block if API unreachable).
981 tests pass.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

Claude approved these changes 2026-03-30 21:44:36 +00:00

Claude left a comment

All 981 tests pass. Trade deadline enforcement is correct — fails closed on missing API data, guards all three entry points (/trade initiate, Submit button, _finalize_trade), and the model-level property handles offseason edge cases cleanly. Approving.

Claude added the

ai-reviewing

label 2026-03-30 21:45:47 +00:00

cal merged commit 8e02889fd4 into main

2026-03-30 21:46:19 +00:00

cal deleted branch feature/trade-deadline-enforcement

2026-03-30 21:46:19 +00:00

cal referenced this issue from a commit

2026-03-30 21:46:21 +00:00

Merge pull request 'feat: enforce trade deadline in /trade commands' (#121) from feature/trade-deadline-enforcement into main

Claude reviewed 2026-03-30 21:47:19 +00:00

Claude left a comment

AI Code Review

Files Reviewed

models/current.py (modified — added is_past_trade_deadline property)
commands/transactions/trade.py (modified — deadline guard in trade_initiate)
views/trade_embed.py (modified — deadline guards in submit_button and _finalize_trade)
tests/test_commands_trade_deadline.py (added)
tests/test_models.py (modified — added test_is_past_trade_deadline)

Findings

Correctness

is_past_trade_deadline logic is correct: week > trade_deadline (allowing trading on the deadline week itself), with an offseason bypass (week > 18 → False). Matches the PR description exactly.
All three guard sites (trade_initiate, submit_button, _finalize_trade) implement the same fail-closed pattern correctly.
In _finalize_trade, the old next_week = current.week + 1 if current else 1 fallback is removed — the 1 fallback was a silent lie that would have applied trades to week 1 if the API was unavailable. The new return on not current is strictly safer.
league_service was already imported in views/trade_embed.py (already used by _finalize_trade), so no missing import.
Test message assertion (call_kwargs[0][0]) correctly matches the positional string arg passed to followup.send. ✓

Security

No issues. Fail-closed design is the right choice — trades are blocked, not silently allowed, when league state is unavailable.

Style & Conventions

Imports and structure follow project conventions.
Raw-string ephemeral responses ("❌ ...") are consistent with other quick error messages in trade.py.
The test_trade_initiate_allowed_at_deadline_week test correctly patches clear_trade_builder and asserts it was called — this verifies the deadline check was passed, not just that no error was sent. Good approach.

Suggestions

Untested view-level guards: The submit_button and _finalize_trade deadline checks are not covered by the new tests — all 3 command-level tests target trade_initiate. These paths follow the same pattern and are likely correct, but a race-condition scenario (deadline passes between initiation and submission) is exactly the kind of edge case worth covering.
Offseason semantics: is_past_trade_deadline returns False during the offseason (week > 18), effectively re-opening trades. This is presumably intentional (off-season trading window), but it may be worth a comment in the property docstring clarifying why the offseason bypasses the deadline, so future readers don't mistake it for a bug.

Verdict: COMMENT

Clean, correct implementation. The fail-closed approach and the three-layer guard (initiate → submit → finalize) are all sound. Minor gap: the view-level deadline checks in submit_button and _finalize_trade have no corresponding tests, but the logic is straightforward and the pattern is identical to the tested trade_initiate guard. Mergeable as-is.

Automated review by Claude PR Reviewer

## AI Code Review ### Files Reviewed - `models/current.py` (modified — added `is_past_trade_deadline` property) - `commands/transactions/trade.py` (modified — deadline guard in `trade_initiate`) - `views/trade_embed.py` (modified — deadline guards in `submit_button` and `_finalize_trade`) - `tests/test_commands_trade_deadline.py` (added) - `tests/test_models.py` (modified — added `test_is_past_trade_deadline`) ### Findings #### Correctness - `is_past_trade_deadline` logic is correct: `week > trade_deadline` (allowing trading on the deadline week itself), with an offseason bypass (`week > 18` → `False`). Matches the PR description exactly. - All three guard sites (`trade_initiate`, `submit_button`, `_finalize_trade`) implement the same fail-closed pattern correctly. - In `_finalize_trade`, the old `next_week = current.week + 1 if current else 1` fallback is removed — the `1` fallback was a silent lie that would have applied trades to week 1 if the API was unavailable. The new `return` on `not current` is strictly safer. - `league_service` was already imported in `views/trade_embed.py` (already used by `_finalize_trade`), so no missing import. - Test message assertion (`call_kwargs[0][0]`) correctly matches the positional string arg passed to `followup.send`. ✓ #### Security - No issues. Fail-closed design is the right choice — trades are blocked, not silently allowed, when league state is unavailable. #### Style & Conventions - Imports and structure follow project conventions. - Raw-string ephemeral responses (`"❌ ..."`) are consistent with other quick error messages in `trade.py`. - The `test_trade_initiate_allowed_at_deadline_week` test correctly patches `clear_trade_builder` and asserts it was called — this verifies the deadline check was passed, not just that no error was sent. Good approach. #### Suggestions - **Untested view-level guards**: The `submit_button` and `_finalize_trade` deadline checks are not covered by the new tests — all 3 command-level tests target `trade_initiate`. These paths follow the same pattern and are likely correct, but a race-condition scenario (deadline passes between initiation and submission) is exactly the kind of edge case worth covering. - **Offseason semantics**: `is_past_trade_deadline` returns `False` during the offseason (week > 18), effectively re-opening trades. This is presumably intentional (off-season trading window), but it may be worth a comment in the property docstring clarifying *why* the offseason bypasses the deadline, so future readers don't mistake it for a bug. ### Verdict: COMMENT Clean, correct implementation. The fail-closed approach and the three-layer guard (initiate → submit → finalize) are all sound. Minor gap: the view-level deadline checks in `submit_button` and `_finalize_trade` have no corresponding tests, but the logic is straightforward and the pattern is identical to the tested `trade_initiate` guard. Mergeable as-is. --- *Automated review by Claude PR Reviewer*

Claude added

ai-reviewed

and removed

ai-reviewing

labels 2026-03-30 21:47:44 +00:00

Sign in to join this conversation.

No reviewers