feat: refractor card art pipeline — S3 upload + image_url #187

cal · 2026-04-06T22:25:26Z

cal commented

2026-04-06 22:25:26 +00:00

Summary

New app/services/card_storage.py — S3 upload utility for variant card images (boto3)
Card render endpoint gains background S3 upload side effect for variant cards (variant > 0, image_url is None)
Refractor cards list endpoint now includes image_url in the response

Details

When the existing card render endpoint (GET /players/{id}/{card_type}card/{d}/{variant}) renders a variant card for the first time, it now schedules a background task to upload the PNG to S3 and populate image_url on the BattingCard/PitchingCard row. No new endpoints — all built on existing infrastructure.

S3 path: cards/cardset-{csid:03d}/player-{pid}/v{variant}/{card_type}card.png

Test plan

pytest tests/test_card_storage.py — 18 passed (S3 key/URL/upload/backfill)
pytest tests/test_refractor_image_url.py — 2 passed (image_url in API response)
pytest tests/test_refractor_state_api.py — 5 passed, 10 skipped (no regressions)

Deploy note

Requires AWS credentials on the API server (AWS_ACCESS_KEY_ID/AWS_SECRET_ACCESS_KEY or ~/.aws/credentials). Deploy to dev first, verify S3 upload with ?tier= override.

🤖 Generated with Claude Code

## Summary - New `app/services/card_storage.py` — S3 upload utility for variant card images (boto3) - Card render endpoint gains background S3 upload side effect for variant cards (`variant > 0`, `image_url` is None) - Refractor cards list endpoint now includes `image_url` in the response ## Details When the existing card render endpoint (`GET /players/{id}/{card_type}card/{d}/{variant}`) renders a variant card for the first time, it now schedules a background task to upload the PNG to S3 and populate `image_url` on the BattingCard/PitchingCard row. No new endpoints — all built on existing infrastructure. S3 path: `cards/cardset-{csid:03d}/player-{pid}/v{variant}/{card_type}card.png` ## Test plan - [x] `pytest tests/test_card_storage.py` — 18 passed (S3 key/URL/upload/backfill) - [x] `pytest tests/test_refractor_image_url.py` — 2 passed (image_url in API response) - [x] `pytest tests/test_refractor_state_api.py` — 5 passed, 10 skipped (no regressions) ## Deploy note Requires AWS credentials on the API server (`AWS_ACCESS_KEY_ID`/`AWS_SECRET_ACCESS_KEY` or `~/.aws/credentials`). Deploy to dev first, verify S3 upload with `?tier=` override. 🤖 Generated with [Claude Code](https://claude.com/claude-code)

cal added 4 commits 2026-04-06 22:25:28 +00:00

docs: fix dev PostgreSQL container/db/user in CLAUDE.md cc65dc5395

Dev environment uses sba_postgres container, paperdynasty_dev database,
sba_admin user — not pd_postgres/pd_master as previously documented.

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

feat: add card_storage S3 upload utility for variant cards baefe4eeb7

New service with S3 upload functions for the refractor card art
pipeline. backfill_variant_image_url reads rendered PNGs from disk,
uploads to S3, and sets image_url on BattingCard/PitchingCard rows.
18 tests covering key construction, URL formatting, upload params,
and error swallowing.

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

feat: schedule S3 upload for variant cards after Playwright render 1169599b8d

Adds BackgroundTasks to the card render endpoint. After rendering a
variant card (variant > 0) where image_url is None, schedules
backfill_variant_image_url to upload the PNG to S3 and populate
image_url on the card row.

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

feat: include image_url in refractor cards API response 777fd0e440

Adds image_url field to each card state entry in the GET
/api/v2/refractor/cards response. Resolved by looking up the variant
BattingCard/PitchingCard row. Returns null when no image has been
rendered yet.

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

cal added 1 commit 2026-04-06 22:31:21 +00:00

fix: review feedback — pin boto3, use player_id consistently, add comment 91109880b6

- Pin boto3==1.42.65 to match project convention of exact version pins
- Use player_id (not player) for FK column access in card_storage.py
  to match the pattern used throughout the codebase
- Add comment explaining the tier is None guard in S3 upload scheduling

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

cal force-pushed feat/refractor-card-art-pipeline from 91109880b6 to f4a90da629

2026-04-06 22:36:23 +00:00

Compare

Claude added the

ai-reviewing

label 2026-04-06 22:45:52 +00:00

Claude removed the

ai-reviewing

label 2026-04-06 22:50:53 +00:00

Claude added the

ai-reviewing

label 2026-04-06 23:15:52 +00:00

Claude removed the

ai-reviewing

label 2026-04-06 23:20:53 +00:00

Claude added the

ai-reviewing

label 2026-04-06 23:45:53 +00:00

Claude reviewed 2026-04-06 23:48:38 +00:00

Claude left a comment

PR Review — feat: refractor card art pipeline — S3 upload + image_url

Verdict: REQUEST_CHANGES (posted as COMMENT — self-review restriction)

One blocking CLAUDE.md violation and two minor issues.

🔴 BLOCKING — Lazy import in `players.py` violates CLAUDE.md

# players.py, inside get_batter_card()
if card_row.image_url is None:
    from app.services.card_storage import backfill_variant_image_url  # ← VIOLATION
    background_tasks.add_task(...)

CLAUDE.md: "Never add lazy imports to middle of file." This import is nested inside two if blocks inside a request handler. card_storage has no circular import risk — its module-level imports are only logging, datetime, and boto3. players.py already imports BattingCard at the top level with no issues. Move this to a top-level import at the top of players.py.

Note: The lazy import inside backfill_variant_image_url in card_storage.py itself (for BattingCard/PitchingCard) is acceptable and documented — that's a different concern.

🟡 MINOR — S3 bucket and region hardcoded

S3_BUCKET = "paper-dynasty" and S3_REGION = "us-east-1" are module-level constants with no env var override. Dev and prod may want different buckets, and hardcoded region creates friction if infra ever moves. Consider reading from os.environ.get("S3_BUCKET", "paper-dynasty") etc.

🟡 MINOR — Test assertion checks wrong field name

In test_card_storage.py:

MockBatting.get.assert_called_once_with(
    MockBatting.player == 42,    # ← wrong: should be MockBatting.player_id
    MockBatting.variant == 1
)

The actual code calls BattingCard.get(BattingCard.player_id == player_id, ...). Because MagicMock.__eq__ returns a truthy mock for any comparison, assert_called_once_with passes regardless of attribute name — but the assertion isn't actually validating the field. Same issue in test_pitching_card_image_url_updated. Fix: use MockBatting.player_id == 42.

✅ Looks good

Background task guard (variant > 0 and tier is None) correctly skips dev-preview renders — ?tier= overrides don't correspond to real card rows ✅
except CardModel.DoesNotExist: pass prevents crash when a variant card hasn't been written yet ✅
backfill_variant_image_url swallows all exceptions — safe for fire-and-forget background task ✅
refractor.py _build_card_state_response image_url lookup: hasattr(state, "track") and state.track guard for null track ✅; card_type == "batter" → BattingCard else PitchingCard matches existing convention ✅
result["image_url"] = image_url always set (even when None) so API consumers can rely on key presence ✅
S3 key format cards/cardset-{csid:03d}/player-{pid}/v{variant}/{card_type}card.png zero-padded correctly ✅
20 new tests with good docstrings covering key/URL construction, upload params, error swallowing ✅
mergeable: true ✅

## PR Review — feat: refractor card art pipeline — S3 upload + image_url **Verdict: REQUEST_CHANGES** (posted as COMMENT — self-review restriction) One blocking CLAUDE.md violation and two minor issues. --- ### 🔴 BLOCKING — Lazy import in `players.py` violates CLAUDE.md ```python # players.py, inside get_batter_card() if card_row.image_url is None: from app.services.card_storage import backfill_variant_image_url # ← VIOLATION background_tasks.add_task(...) ``` CLAUDE.md: **"Never add lazy imports to middle of file."** This import is nested inside two `if` blocks inside a request handler. `card_storage` has no circular import risk — its module-level imports are only `logging`, `datetime`, and `boto3`. `players.py` already imports `BattingCard` at the top level with no issues. Move this to a top-level import at the top of `players.py`. Note: The lazy import inside `backfill_variant_image_url` in `card_storage.py` itself (for `BattingCard`/`PitchingCard`) is acceptable and documented — that's a different concern. --- ### 🟡 MINOR — S3 bucket and region hardcoded `S3_BUCKET = "paper-dynasty"` and `S3_REGION = "us-east-1"` are module-level constants with no env var override. Dev and prod may want different buckets, and hardcoded region creates friction if infra ever moves. Consider reading from `os.environ.get("S3_BUCKET", "paper-dynasty")` etc. --- ### 🟡 MINOR — Test assertion checks wrong field name In `test_card_storage.py`: ```python MockBatting.get.assert_called_once_with( MockBatting.player == 42, # ← wrong: should be MockBatting.player_id MockBatting.variant == 1 ) ``` The actual code calls `BattingCard.get(BattingCard.player_id == player_id, ...)`. Because `MagicMock.__eq__` returns a truthy mock for any comparison, `assert_called_once_with` passes regardless of attribute name — but the assertion isn't actually validating the field. Same issue in `test_pitching_card_image_url_updated`. Fix: use `MockBatting.player_id == 42`. --- ### ✅ Looks good - Background task guard (`variant > 0 and tier is None`) correctly skips dev-preview renders — `?tier=` overrides don't correspond to real card rows ✅ - `except CardModel.DoesNotExist: pass` prevents crash when a variant card hasn't been written yet ✅ - `backfill_variant_image_url` swallows all exceptions — safe for fire-and-forget background task ✅ - `refractor.py` `_build_card_state_response` `image_url` lookup: `hasattr(state, "track") and state.track` guard for null track ✅; `card_type == "batter"` → `BattingCard` else `PitchingCard` matches existing convention ✅ - `result["image_url"] = image_url` always set (even when None) so API consumers can rely on key presence ✅ - S3 key format `cards/cardset-{csid:03d}/player-{pid}/v{variant}/{card_type}card.png` zero-padded correctly ✅ - 20 new tests with good docstrings covering key/URL construction, upload params, error swallowing ✅ - `mergeable: true` ✅

app/routers_v2/players.py Outdated

						
				@ -909,0 +925,4 @@

				                (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

Claude commented

2026-04-06 23:48:38 +00:00

BLOCKING — lazy import violates CLAUDE.md. Move from app.services.card_storage import backfill_variant_image_url to the top-level imports. There is no circular import risk — card_storage's module-level imports are only logging, datetime, and boto3.

**BLOCKING — lazy import violates CLAUDE.md.** Move `from app.services.card_storage import backfill_variant_image_url` to the top-level imports. There is no circular import risk — `card_storage`'s module-level imports are only `logging`, `datetime`, and `boto3`.

app/services/card_storage.py

						
				@ -0,0 +34,4 @@

				import logging

				from datetime import date

Claude commented

2026-04-06 23:48:38 +00:00

MINOR — S3_BUCKET and S3_REGION are hardcoded with no env var override. Consider os.environ.get("S3_BUCKET", "paper-dynasty") so dev/prod can use different buckets without a code change.

**MINOR** — `S3_BUCKET` and `S3_REGION` are hardcoded with no env var override. Consider `os.environ.get("S3_BUCKET", "paper-dynasty")` so dev/prod can use different buckets without a code change.

tests/test_card_storage.py

						
				@ -0,0 +231,4 @@

				        png_path.write_bytes(b"\x89PNG\r\n\x1a\n fake png data")

				        mock_s3 = MagicMock()

				        mock_card = MagicMock()

Claude commented

2026-04-06 23:48:38 +00:00

MINOR — MockBatting.player == 42 should be MockBatting.player_id == 42. The production code calls BattingCard.get(BattingCard.player_id == player_id, ...). With MagicMock, == on any two mock attributes returns a truthy mock, so this assertion passes regardless of field name — it's not actually validating the correct field.

**MINOR** — `MockBatting.player == 42` should be `MockBatting.player_id == 42`. The production code calls `BattingCard.get(BattingCard.player_id == player_id, ...)`. With MagicMock, `==` on any two mock attributes returns a truthy mock, so this assertion passes regardless of field name — it's not actually validating the correct field.

Claude removed the

ai-reviewing

label 2026-04-06 23:49:10 +00:00

Claude added the

ai-reviewing

label 2026-04-07 00:45:52 +00:00

Claude reviewed 2026-04-07 00:49:53 +00:00

Claude left a comment

AI Code Review

Files Reviewed

app/routers_v2/players.py (modified — BackgroundTasks + S3 trigger)
app/services/card_storage.py (added — S3 upload utility)
app/routers_v2/refractor.py (modified — image_url in response)
tests/test_card_storage.py (added)
tests/test_refractor_image_url.py (added)
requirements.txt (boto3 added)
CLAUDE.md (doc fix)

Findings

Correctness

[BLOCKING] tests/test_card_storage.py — vacuous assert_called_once_with assertions (wrong field name)

test_batting_card_image_url_updated and test_pitching_card_image_url_updated both assert:

MockBatting.get.assert_called_once_with(
    MockBatting.player == 42, MockBatting.variant == 1
)

But the production code (card_storage.py) calls:

BattingCard.get(
    BattingCard.player_id == player_id, BattingCard.variant == variant
)

The field is player_id, not player. Because MagicMock.__eq__ uses identity comparison, both MockBatting.player == 42 and MockBatting.player_id == 42 evaluate to Python False. The assert_called_once_with(False, False) therefore passes regardless of which field the production code actually uses — the assertion is vacuous and will not catch if the wrong field (or wrong value) is queried. The production code's player_id is correct, but the test doesn't verify it.

Fix: capture the actual call args and assert them directly, or use a real SQLite model (as done in test_refractor_image_url.py) so the query expression can be evaluated.

Security

No issues found. S3 credentials resolved from the environment via boto3's standard chain — no hardcoded credentials.

Style & Conventions

[BLOCKING] players.py — lazy import inside get_batter_card() function body

if variant > 0 and tier is None:
    ...
    if card_row.image_url is None:
        from app.services.card_storage import backfill_variant_image_url  # ← lazy import

CLAUDE.md: "Never add lazy imports to middle of file." This is the same blocking issue from the prior review (unchanged). There is no circular import path: players.py already imports from db_engine at the top level; card_storage.py → db_engine is a clean, one-direction dependency. Move this import to the top of players.py.

[BLOCKING] card_storage.py — lazy import inside backfill_variant_image_url() function

def backfill_variant_image_url(...):
    # Lazy import to avoid circular imports at module load time
    from app.db_engine import BattingCard, PitchingCard  # ← lazy import

Same rule. The module docstring claims db_engine imports routers "indirectly via the app module in some paths" — but card_storage.py is a new services/ module and db_engine is a pure model file. No circular dependency path exists through this module. Move the import to the top of card_storage.py.

[MINOR] card_storage.py — S3_BUCKET and S3_REGION are hardcoded constants

S3_BUCKET = "paper-dynasty"
S3_REGION = "us-east-1"

No env var override means dev and prod environments share the same S3 bucket. Consider os.environ.get("S3_BUCKET", "paper-dynasty") so the dev API can write to a test bucket without touching prod data.

Suggestions

refractor.py _build_card_state_response: the top-level from ..db_engine import model_to_dict, BattingCard, PitchingCard is correctly placed at module level. ✅
card_storage.py backfill_variant_image_url: card.save() without only=[...] updates all fields — acceptable here since the card is freshly fetched immediately before the save.
refractor.py image_url lookup: result["image_url"] = image_url set unconditionally (even when None) ensures a stable API contract. ✅
Background task guard variant > 0 and tier is None correctly skips ?tier= dev-preview renders. ✅

Verdict: REQUEST_CHANGES

Two CLAUDE.md lazy-import violations remain unresolved from the prior review (one in players.py, one in the new card_storage.py), plus a correctness bug in the test suite: the assert_called_once_with assertions on MockBatting.player / MockPitching.player are vacuous under MagicMock equality semantics and don't catch the actual field being queried.

Note: Verdict posted as COMMENT due to Gitea self-review restriction — treat as REQUEST_CHANGES.

Automated review by Claude PR Reviewer

## AI Code Review ### Files Reviewed - `app/routers_v2/players.py` (modified — BackgroundTasks + S3 trigger) - `app/services/card_storage.py` (added — S3 upload utility) - `app/routers_v2/refractor.py` (modified — image_url in response) - `tests/test_card_storage.py` (added) - `tests/test_refractor_image_url.py` (added) - `requirements.txt` (boto3 added) - `CLAUDE.md` (doc fix) ### Findings #### Correctness **[BLOCKING] `tests/test_card_storage.py` — vacuous `assert_called_once_with` assertions (wrong field name)** `test_batting_card_image_url_updated` and `test_pitching_card_image_url_updated` both assert: ```python MockBatting.get.assert_called_once_with( MockBatting.player == 42, MockBatting.variant == 1 ) ``` But the production code (`card_storage.py`) calls: ```python BattingCard.get( BattingCard.player_id == player_id, BattingCard.variant == variant ) ``` The field is `player_id`, not `player`. Because `MagicMock.__eq__` uses identity comparison, both `MockBatting.player == 42` and `MockBatting.player_id == 42` evaluate to Python `False`. The `assert_called_once_with(False, False)` therefore passes regardless of which field the production code actually uses — the assertion is vacuous and will not catch if the wrong field (or wrong value) is queried. The production code's `player_id` is correct, but the test doesn't verify it. Fix: capture the actual call args and assert them directly, or use a real SQLite model (as done in `test_refractor_image_url.py`) so the query expression can be evaluated. #### Security - No issues found. S3 credentials resolved from the environment via boto3's standard chain — no hardcoded credentials. #### Style & Conventions **[BLOCKING] `players.py` — lazy import inside `get_batter_card()` function body** ```python if variant > 0 and tier is None: ... if card_row.image_url is None: from app.services.card_storage import backfill_variant_image_url # ← lazy import ``` CLAUDE.md: **"Never add lazy imports to middle of file."** This is the same blocking issue from the prior review (unchanged). There is no circular import path: `players.py` already imports from `db_engine` at the top level; `card_storage.py` → `db_engine` is a clean, one-direction dependency. Move this import to the top of `players.py`. **[BLOCKING] `card_storage.py` — lazy import inside `backfill_variant_image_url()` function** ```python def backfill_variant_image_url(...): # Lazy import to avoid circular imports at module load time from app.db_engine import BattingCard, PitchingCard # ← lazy import ``` Same rule. The module docstring claims `db_engine` imports routers "indirectly via the app module in some paths" — but `card_storage.py` is a new `services/` module and `db_engine` is a pure model file. No circular dependency path exists through this module. Move the import to the top of `card_storage.py`. **[MINOR] `card_storage.py` — `S3_BUCKET` and `S3_REGION` are hardcoded constants** ```python S3_BUCKET = "paper-dynasty" S3_REGION = "us-east-1" ``` No env var override means dev and prod environments share the same S3 bucket. Consider `os.environ.get("S3_BUCKET", "paper-dynasty")` so the dev API can write to a test bucket without touching prod data. #### Suggestions - `refractor.py` `_build_card_state_response`: the top-level `from ..db_engine import model_to_dict, BattingCard, PitchingCard` is correctly placed at module level. ✅ - `card_storage.py` `backfill_variant_image_url`: `card.save()` without `only=[...]` updates all fields — acceptable here since the card is freshly fetched immediately before the save. - `refractor.py` image_url lookup: `result["image_url"] = image_url` set unconditionally (even when `None`) ensures a stable API contract. ✅ - Background task guard `variant > 0 and tier is None` correctly skips `?tier=` dev-preview renders. ✅ ### Verdict: REQUEST_CHANGES Two CLAUDE.md lazy-import violations remain unresolved from the prior review (one in `players.py`, one in the new `card_storage.py`), plus a correctness bug in the test suite: the `assert_called_once_with` assertions on `MockBatting.player` / `MockPitching.player` are vacuous under MagicMock equality semantics and don't catch the actual field being queried. > **Note:** Verdict posted as COMMENT due to Gitea self-review restriction — treat as REQUEST_CHANGES. --- *Automated review by Claude PR Reviewer*

Claude added

ai-changes-requested

and removed

ai-reviewing

labels 2026-04-07 00:50:21 +00:00

Claude added the

ai-working

label 2026-04-07 01:00:49 +00:00

cal referenced this issue from a commit

2026-04-07 01:03:07 +00:00

fix: address review feedback (#187)

cal added 1 commit 2026-04-07 01:03:07 +00:00

fix: address review feedback (#187 ) c75e2781be

- 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>

Claude commented

2026-04-07 01:03:22 +00:00

Review feedback addressed

All three blocking issues from the review have been fixed in c75e278.

🔴 BLOCKING — Lazy import in `players.py` (CLAUDE.md violation)

Moved from ..services.card_storage import backfill_variant_image_url to top-level imports alongside the other ..services import. Removed the inline import from inside get_batter_card().

🔴 BLOCKING — Lazy import in `card_storage.py` (CLAUDE.md violation)

Moved from app.db_engine import BattingCard, PitchingCard to top-level imports. Removed the lazy import comment and the stale circular-import claim from the module docstring (no circular import path exists through this module).

🔴 BLOCKING — Vacuous test assertions (wrong field name)

Fixed MockBatting.player == 42 → MockBatting.player_id == 42 and MockPitching.player == 99 → MockPitching.player_id == 99 in test_batting_card_image_url_updated and test_pitching_card_image_url_updated.

Also updated all patch targets in test_card_storage.py from app.db_engine.BattingCard/PitchingCard → app.services.card_storage.BattingCard/PitchingCard — required now that the import is top-level (patching the source module attribute no longer intercepts the already-bound name in card_storage).

🟡 MINOR — Hardcoded S3 constants

Changed S3_BUCKET = "paper-dynasty" and S3_REGION = "us-east-1" to read from os.environ.get("S3_BUCKET", "paper-dynasty") and os.environ.get("S3_REGION", "us-east-1") so dev and prod environments can use separate buckets.

## Review feedback addressed All three blocking issues from the review have been fixed in c75e278. ### 🔴 BLOCKING — Lazy import in `players.py` (CLAUDE.md violation) Moved `from ..services.card_storage import backfill_variant_image_url` to top-level imports alongside the other `..services` import. Removed the inline import from inside `get_batter_card()`. ### 🔴 BLOCKING — Lazy import in `card_storage.py` (CLAUDE.md violation) Moved `from app.db_engine import BattingCard, PitchingCard` to top-level imports. Removed the lazy import comment and the stale circular-import claim from the module docstring (no circular import path exists through this module). ### 🔴 BLOCKING — Vacuous test assertions (wrong field name) Fixed `MockBatting.player == 42` → `MockBatting.player_id == 42` and `MockPitching.player == 99` → `MockPitching.player_id == 99` in `test_batting_card_image_url_updated` and `test_pitching_card_image_url_updated`. Also updated all patch targets in `test_card_storage.py` from `app.db_engine.BattingCard`/`PitchingCard` → `app.services.card_storage.BattingCard`/`PitchingCard` — required now that the import is top-level (patching the source module attribute no longer intercepts the already-bound name in `card_storage`). ### 🟡 MINOR — Hardcoded S3 constants Changed `S3_BUCKET = "paper-dynasty"` and `S3_REGION = "us-east-1"` to read from `os.environ.get("S3_BUCKET", "paper-dynasty")` and `os.environ.get("S3_REGION", "us-east-1")` so dev and prod environments can use separate buckets.

Claude removed the

ai-working

ai-changes-requested

labels 2026-04-07 01:03:30 +00:00

Claude added the

ai-reviewing

label 2026-04-07 01:15:53 +00:00

cal removed the

ai-reviewing

label 2026-04-07 03:13:15 +00:00

Claude added the

ai-reviewing

label 2026-04-07 03:18:55 +00:00

Claude removed the

ai-reviewing

label 2026-04-07 03:23:56 +00:00

cal added 1 commit 2026-04-07 03:27:20 +00:00

merge: resolve requirements.txt conflict — include both apng and boto3 20f7ac5958

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

Claude approved these changes 2026-04-07 03:29:19 +00:00

Claude left a comment

Approved via pd-pr

cal merged commit 0dc096be93 into main

2026-04-07 03:29:25 +00:00

cal deleted branch feat/refractor-card-art-pipeline

2026-04-07 03:29:25 +00:00

cal referenced this issue from a commit

2026-04-07 03:29:26 +00:00

Merge pull request 'feat: refractor card art pipeline — S3 upload + image_url' (#187) from feat/refractor-card-art-pipeline into main

Sign in to join this conversation.

No reviewers

No Milestone

No project

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/paper-dynasty-database#187

feat: refractor card art pipeline — S3 upload + image_url #187

Summary

Details

Test plan

Deploy note

PR Review — feat: refractor card art pipeline — S3 upload + image_url

🔴 BLOCKING — Lazy import in players.py violates CLAUDE.md

🟡 MINOR — S3 bucket and region hardcoded

🟡 MINOR — Test assertion checks wrong field name

✅ Looks good

AI Code Review

Files Reviewed

Findings

Correctness

Security

Style & Conventions

Suggestions

Verdict: REQUEST_CHANGES

Review feedback addressed

🔴 BLOCKING — Lazy import in players.py (CLAUDE.md violation)

🔴 BLOCKING — Lazy import in card_storage.py (CLAUDE.md violation)

🔴 BLOCKING — Vacuous test assertions (wrong field name)

🟡 MINOR — Hardcoded S3 constants

🔴 BLOCKING — Lazy import in `players.py` violates CLAUDE.md

🔴 BLOCKING — Lazy import in `players.py` (CLAUDE.md violation)

🔴 BLOCKING — Lazy import in `card_storage.py` (CLAUDE.md violation)