feat: enforce FA lock deadline after week 14 #122

cal · 2026-03-31T21:07:56Z

cal commented

2026-03-31 21:07:56 +00:00

Summary

Enforces fa_lock_week (14) in TransactionBuilder.add_move() — blocks /dropadd from adding players FROM Free Agency when current_week >= 14
Dropping players TO FA remains allowed after the deadline
Consolidates two league_service.get_current_state() calls into a single shared fetch, eliminating a redundant API round-trip on every FA pickup

Test plan

test_add_move_from_fa_blocked_after_deadline — verifies FA pickups blocked at week 15
test_drop_to_fa_allowed_after_deadline — verifies drops to FA still work after deadline
All 983 existing tests pass (41 transaction builder tests)
Manual test: attempt /dropadd with an FA player in prod after deployment

🤖 Generated with Claude Code

## Summary - Enforces `fa_lock_week` (14) in `TransactionBuilder.add_move()` — blocks `/dropadd` from adding players FROM Free Agency when `current_week >= 14` - Dropping players TO FA remains allowed after the deadline - Consolidates two `league_service.get_current_state()` calls into a single shared fetch, eliminating a redundant API round-trip on every FA pickup ## Test plan - [x] `test_add_move_from_fa_blocked_after_deadline` — verifies FA pickups blocked at week 15 - [x] `test_drop_to_fa_allowed_after_deadline` — verifies drops to FA still work after deadline - [x] All 983 existing tests pass (41 transaction builder tests) - [ ] Manual test: attempt `/dropadd` with an FA player in prod after deployment 🤖 Generated with [Claude Code](https://claude.com/claude-code)

cal added 1 commit 2026-03-31 21:07:57 +00:00

feat: enforce FA lock deadline — block signing FA players after week 14 6016afb999

The fa_lock_week config existed but was never enforced. Now /dropadd blocks
adding players FROM Free Agency when current_week >= fa_lock_week (14).
Dropping players TO FA remains allowed after the deadline.

Also consolidates two league_service.get_current_state() calls into one
shared fetch at the top of add_move() to eliminate a redundant API round-trip.

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

cal reviewed 2026-03-31 21:11:35 +00:00

cal left a comment

Review — feat: enforce FA lock deadline after week 14

transaction_builder.py

is_fa_pickup detection: Correct asymmetry. from_roster == FREE_AGENCY and to_roster != FREE_AGENCY precisely targets FA pickups only; drops to FA have from_roster != FREE_AGENCY and bypass the check entirely.

API call consolidation: Clean. needs_current_state correctly unifies the two previously separate fetch paths — FA lock check and pending-transaction week resolution — into one call.

Fallback on API failure (current_week = 1): Permissive fail. Week 1 never triggers the lock, so a get_current_state() outage won't accidentally block all FA activity. Correct default.

current_week is not None guard: Safe. current_week is only None when needs_current_state was False, meaning is_fa_pickup was also False, so the block can never fire incorrectly on non-FA moves.

Pending transaction next_week derivation: (current_week + 1) if current_week else 1 preserves prior behavior exactly. No regression.

tests

autouse fixture (both test classes): Correct approach — all existing tests run at week 10 (safely below the deadline) without modification. No existing tests impacted.

test_add_move_from_fa_blocked_after_deadline: Week 15 → FA pickup → success=False, error message contains "Free agency is closed", move_count == 0. Solid.

test_drop_to_fa_allowed_after_deadline: Verifies ML→FA is not blocked. Minor gap: this test runs at week 10, so it doesn't prove drops are allowed specifically after the deadline. Not a blocker — the logic clearly excludes drops from the check path.

Edge cases verified

Scenario	Behavior
API failure → week 1 fallback	FA pickup allowed (permissive)
`current_state` returns `None`	Same fallback, same result
Drop to FA at week 15+	`is_fa_pickup=False`, no check fires
Pickup at exactly week 14	`>=` correctly blocks (inclusive deadline)
`next_week` pre-supplied by caller	FA lock still checked independently

Verdict: APPROVED

Logic is correct, consolidation is clean, failure modes are permissive. Approved for merge.

## Review — feat: enforce FA lock deadline after week 14 ### transaction_builder.py **`is_fa_pickup` detection**: Correct asymmetry. `from_roster == FREE_AGENCY and to_roster != FREE_AGENCY` precisely targets FA pickups only; drops to FA have `from_roster != FREE_AGENCY` and bypass the check entirely. **API call consolidation**: Clean. `needs_current_state` correctly unifies the two previously separate fetch paths — FA lock check and pending-transaction week resolution — into one call. **Fallback on API failure (`current_week = 1`)**: Permissive fail. Week 1 never triggers the lock, so a `get_current_state()` outage won't accidentally block all FA activity. Correct default. **`current_week is not None` guard**: Safe. `current_week` is only `None` when `needs_current_state` was `False`, meaning `is_fa_pickup` was also `False`, so the block can never fire incorrectly on non-FA moves. **Pending transaction `next_week` derivation**: `(current_week + 1) if current_week else 1` preserves prior behavior exactly. No regression. ### tests **`autouse` fixture (both test classes)**: Correct approach — all existing tests run at week 10 (safely below the deadline) without modification. No existing tests impacted. **`test_add_move_from_fa_blocked_after_deadline`**: Week 15 → FA pickup → `success=False`, error message contains "Free agency is closed", `move_count == 0`. Solid. **`test_drop_to_fa_allowed_after_deadline`**: Verifies ML→FA is not blocked. Minor gap: this test runs at week 10, so it doesn't prove drops are allowed specifically *after* the deadline. Not a blocker — the logic clearly excludes drops from the check path. ### Edge cases verified | Scenario | Behavior | |---|---| | API failure → week 1 fallback | FA pickup allowed (permissive) | | `current_state` returns `None` | Same fallback, same result | | Drop to FA at week 15+ | `is_fa_pickup=False`, no check fires | | Pickup at exactly week 14 | `>=` correctly blocks (inclusive deadline) | | `next_week` pre-supplied by caller | FA lock still checked independently | ### Verdict: APPROVED Logic is correct, consolidation is clean, failure modes are permissive. Approved for merge.

Claude approved these changes 2026-03-31 21:12:07 +00:00

Claude left a comment

LGTM — FA lock enforcement is clean, tests cover both directions, and the consolidated API call is a nice efficiency win.

Claude added the

ai-reviewing

label 2026-03-31 21:15:46 +00:00

Claude reviewed 2026-03-31 21:17:43 +00:00

Claude left a comment

AI Code Review

Files Reviewed

services/transaction_builder.py (modified)
tests/test_services_transaction_builder.py (modified)

Findings

Correctness

FA pickup detection is correct. is_fa_pickup = (from_roster == FREE_AGENCY and to_roster != FREE_AGENCY) correctly identifies pickups while allowing drops to FA (from_roster != FREE_AGENCY, to_roster == FREE_AGENCY) to pass through.
Shared fetch consolidation is correct. needs_current_state covers both the FA lock check and the next_week is None case for pending transactions, eliminating the redundant API call.
Failsafe is permissive but reasonable. On get_current_state() failure, current_week = 1 means the FA lock is NOT triggered (1 < 14). This allows FA pickups when the week can't be determined, which is the safe choice for service availability.
Minor: redundant current_week is not None guard on the FA check block. When is_fa_pickup = True, needs_current_state is always True, so current_week is always set (either to current_state.week or the 1 fallback). The is not None check can never be True while is_fa_pickup is True. Not a bug, just dead logic.
Minor: FA lock check now runs before the duplicate-player check. Previously, adding an already-tracked player from FA returned "already has a move". Now it returns "Free agency is closed" when locked. Behavior change is harmless but worth noting.

Security

No issues found.

Style & Conventions

Two informative comments were removed from the pending transactions block:
- # This prevents duplicate claims that would need to be resolved at freeze time
- # Only applies to /dropadd (scheduled moves), not /ilmove (immediate moves)
  These aren't blocking, but they explained non-obvious behavior. Consider preserving them.

Test Coverage

test_add_move_from_fa_blocked_after_deadline uses week=15, which verifies blocking after the deadline. However, the boundary case week=14 (the first blocked week per current_week >= fa_lock_week) is not tested. Consider adding test_add_move_from_fa_blocked_at_deadline with week=14.
test_drop_to_fa_allowed_after_deadline uses the autouse fixture at week=10, not a post-deadline week. The test name implies it proves drops work after the deadline, but the FA lock check is never triggered for drops regardless of week (because is_fa_pickup = False). The assertion is correct, but a comment noting this would clarify why week=10 is sufficient.
autouse fixtures on both test classes correctly prevent the production league_service.get_current_state() from being called during existing tests — good isolation.

Suggestions

Add a week=14 boundary test to confirm the first locked week.
Update test_drop_to_fa_allowed_after_deadline name or add a comment clarifying that the FA lock is direction-based, not week-based for drops.

Verdict: COMMENT

Clean, correct implementation. The consolidation logic is sound, the permissive fallback is appropriate, and the autouse fixture approach correctly protects existing tests. The two notes above (boundary test at week=14 and misleading test name) are suggestions, not blockers.

Automated review by Claude PR Reviewer

## AI Code Review ### Files Reviewed - `services/transaction_builder.py` (modified) - `tests/test_services_transaction_builder.py` (modified) ### Findings #### Correctness - **FA pickup detection is correct.** `is_fa_pickup = (from_roster == FREE_AGENCY and to_roster != FREE_AGENCY)` correctly identifies pickups while allowing drops to FA (`from_roster != FREE_AGENCY, to_roster == FREE_AGENCY`) to pass through. - **Shared fetch consolidation is correct.** `needs_current_state` covers both the FA lock check and the `next_week is None` case for pending transactions, eliminating the redundant API call. - **Failsafe is permissive but reasonable.** On `get_current_state()` failure, `current_week = 1` means the FA lock is NOT triggered (1 < 14). This allows FA pickups when the week can't be determined, which is the safe choice for service availability. - **Minor: redundant `current_week is not None` guard** on the FA check block. When `is_fa_pickup = True`, `needs_current_state` is always `True`, so `current_week` is always set (either to `current_state.week` or the `1` fallback). The `is not None` check can never be `True` while `is_fa_pickup` is `True`. Not a bug, just dead logic. - **Minor: FA lock check now runs before the duplicate-player check.** Previously, adding an already-tracked player from FA returned `"already has a move"`. Now it returns `"Free agency is closed"` when locked. Behavior change is harmless but worth noting. #### Security - No issues found. #### Style & Conventions - Two informative comments were removed from the pending transactions block: - `# This prevents duplicate claims that would need to be resolved at freeze time` - `# Only applies to /dropadd (scheduled moves), not /ilmove (immediate moves)` These aren't blocking, but they explained non-obvious behavior. Consider preserving them. #### Test Coverage - **`test_add_move_from_fa_blocked_after_deadline`** uses `week=15`, which verifies blocking after the deadline. However, the boundary case `week=14` (the first blocked week per `current_week >= fa_lock_week`) is not tested. Consider adding `test_add_move_from_fa_blocked_at_deadline` with `week=14`. - **`test_drop_to_fa_allowed_after_deadline`** uses the autouse fixture at `week=10`, not a post-deadline week. The test name implies it proves drops work *after* the deadline, but the FA lock check is never triggered for drops regardless of week (because `is_fa_pickup = False`). The assertion is correct, but a comment noting this would clarify why week=10 is sufficient. - `autouse` fixtures on both test classes correctly prevent the production `league_service.get_current_state()` from being called during existing tests — good isolation. #### Suggestions - Add a `week=14` boundary test to confirm the first locked week. - Update `test_drop_to_fa_allowed_after_deadline` name or add a comment clarifying that the FA lock is direction-based, not week-based for drops. ### Verdict: COMMENT Clean, correct implementation. The consolidation logic is sound, the permissive fallback is appropriate, and the `autouse` fixture approach correctly protects existing tests. The two notes above (boundary test at `week=14` and misleading test name) are suggestions, not blockers. --- *Automated review by Claude PR Reviewer*

Claude added

ai-reviewed

and removed

ai-reviewing

labels 2026-03-31 21:18:22 +00:00

cal merged commit c8cb80c5f3 into main

2026-03-31 21:20:42 +00:00

cal deleted branch feature/fa-lock-enforcement

2026-03-31 21:20:42 +00:00

cal referenced this issue from a commit

2026-03-31 21:20:44 +00:00

Merge pull request 'feat: enforce FA lock deadline after week 14' (#122) from feature/fa-lock-enforcement into main