fix: address review feedback (#187)

- Move lazy imports to top level in card_storage.py and players.py (CLAUDE.md violation)
- Use os.environ.get() for S3_BUCKET/S3_REGION to allow dev/prod bucket separation
- Fix test patch targets from app.db_engine to app.services.card_storage (required after top-level import move)
- Fix assert_called_once_with field name: MockBatting.player → MockBatting.player_id

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

app/routers_v2/players.py

@@ -40,6 +40,7 @@ from ..db_engine import (
 )
 from ..db_helpers import upsert_players
 from ..dependencies import oauth2_scheme, valid_token
+from ..services.card_storage import backfill_variant_image_url
 from ..services.refractor_boost import compute_variant_hash
 
 # ---------------------------------------------------------------------------
@@ -925,8 +926,6 @@ async def get_batter_card(
                 (CardModel.player_id == player_id) & (CardModel.variant == variant)
             )
             if card_row.image_url is None:
-                from app.services.card_storage import backfill_variant_image_url
-
                 background_tasks.add_task(
                     backfill_variant_image_url,
                     player_id=player_id,

app/services/card_storage.py

@@ -23,9 +23,6 @@ backfill_variant_image_url(player_id, variant, card_type, cardset_id, png_path)
 
 Design notes
 ------------
-- backfill_variant_image_url uses a lazy import for BattingCard/PitchingCard
-  to avoid circular imports at module load time (db_engine imports routers
-  indirectly via the app module in some paths).
 - S3 credentials are resolved from the environment by boto3 at call time;
   no credentials are hard-coded here.
 - The cache-bust ?d= param matches the card-creation pipeline convention so
@@ -33,14 +30,17 @@ Design notes
 """
 
 import logging
+import os
 from datetime import date
 
 import boto3
 
+from app.db_engine import BattingCard, PitchingCard
+
 logger = logging.getLogger(__name__)
 
-S3_BUCKET = "paper-dynasty"
-S3_REGION = "us-east-1"
+S3_BUCKET = os.environ.get("S3_BUCKET", "paper-dynasty")
+S3_REGION = os.environ.get("S3_REGION", "us-east-1")
 
 
 def get_s3_client():
@@ -150,9 +150,6 @@ def backfill_variant_image_url(
     Returns:
         None
     """
-    # Lazy import to avoid circular imports at module load time
-    from app.db_engine import BattingCard, PitchingCard
-
     try:
         # 1. Read PNG from disk
         with open(png_path, "rb") as f:

tests/test_card_storage.py

@@ -207,7 +207,7 @@ class TestBackfillVariantImageUrl:
 
         with (
             patch("app.services.card_storage.get_s3_client", return_value=mock_s3),
-            patch("app.db_engine.BattingCard") as MockBatting,
+            patch("app.services.card_storage.BattingCard") as MockBatting,
         ):
             MockBatting.get.return_value = mock_card
 
@@ -220,7 +220,7 @@ class TestBackfillVariantImageUrl:
             )
 
             MockBatting.get.assert_called_once_with(
-                MockBatting.player == 42, MockBatting.variant == 1
+                MockBatting.player_id == 42, MockBatting.variant == 1
             )
             assert mock_card.image_url is not None
             mock_card.save.assert_called_once()
@@ -235,7 +235,7 @@ class TestBackfillVariantImageUrl:
 
         with (
             patch("app.services.card_storage.get_s3_client", return_value=mock_s3),
-            patch("app.db_engine.PitchingCard") as MockPitching,
+            patch("app.services.card_storage.PitchingCard") as MockPitching,
         ):
             MockPitching.get.return_value = mock_card
 
@@ -248,7 +248,7 @@ class TestBackfillVariantImageUrl:
             )
 
             MockPitching.get.assert_called_once_with(
-                MockPitching.player == 99, MockPitching.variant == 2
+                MockPitching.player_id == 99, MockPitching.variant == 2
             )
             assert mock_card.image_url is not None
             mock_card.save.assert_called_once()
@@ -263,7 +263,7 @@ class TestBackfillVariantImageUrl:
 
         with (
             patch("app.services.card_storage.get_s3_client", return_value=mock_s3),
-            patch("app.db_engine.BattingCard") as MockBatting,
+            patch("app.services.card_storage.BattingCard") as MockBatting,
         ):
             MockBatting.get.return_value = MagicMock()
 
@@ -293,7 +293,7 @@ class TestBackfillVariantImageUrl:
 
         with (
             patch("app.services.card_storage.get_s3_client", return_value=mock_s3),
-            patch("app.db_engine.BattingCard"),
+            patch("app.services.card_storage.BattingCard"),
         ):
             # Must not raise
             backfill_variant_image_url(
@@ -314,7 +314,7 @@ class TestBackfillVariantImageUrl:
 
         with (
             patch("app.services.card_storage.get_s3_client"),
-            patch("app.db_engine.BattingCard"),
+            patch("app.services.card_storage.BattingCard"),
         ):
             # Must not raise
             backfill_variant_image_url(
@@ -336,7 +336,7 @@ class TestBackfillVariantImageUrl:
 
         with (
             patch("app.services.card_storage.get_s3_client", return_value=mock_s3),
-            patch("app.db_engine.BattingCard") as MockBatting,
+            patch("app.services.card_storage.BattingCard") as MockBatting,
         ):
             MockBatting.get.return_value = mock_card