feat(WP-10): pack opening hook — evolution_card_state initialization #107

Merged

cal merged 1 commits from feature/wp10-pack-opening-hook into card-evolution 2026-03-18 20:32:01 +00:00

Conversation 0 Commits 1 Files Changed 6 +1165 -47

cal commented 2026-03-18 18:41:41 +00:00

Owner

Copy Link

Closes #75.

Summary

• Add app/services/evolution_init.py with initialize_card_evolution and _determine_card_type
• Hook into POST /api/v2/cards to initialize evolution state on pack opening
• 16 new tests (all passing); also fixed pre-existing lint and import issues in cards.py and test_evolution_models.py

What was built

app/services/evolution_init.py (new)

_determine_card_type(player) — pure function, no DB, maps pos_1 to track type:

• pos_1 contains SP → 'sp'
• pos_1 contains RP or CP → 'rp'
• anything else → 'batter'

initialize_card_evolution(player_id, team_id, card_type) — get_or_create pattern:

• Looks up the EvolutionTrack matching card_type
• Calls EvolutionCardState.get_or_create(player_id, team_id, defaults={...})
• New state: current_tier=0, current_value=0.0, fully_evolved=False
• Duplicate call for same (player_id, team_id) is a no-op — existing progress is never reset
• Returns None on any failure (missing track seed, DB error); never raises

app/routers_v2/cards.py (modified)

Added WP-10 hook after Card.bulk_create and before the 200 response:

# WP-10: initialize evolution state for each new card (fire-and-forget)
for x in cards.cards:
    try:
        this_player = Player.get_by_id(x.player_id)
        card_type = _determine_card_type(this_player)
        initialize_card_evolution(x.player_id, x.team_id, card_type)
    except Exception:
        logging.exception("evolution hook: unexpected error ...")

Also fixed 3 pre-existing ruff violations in cards.py (unused lc_id, bare f-string, unused e in except).

tests/test_evolution_init.py (new, 16 tests)

Unit (TestDetermineCardType) — 7 tests, no DB:

• SP → 'sp', RP → 'rp', CP → 'rp'
• 1B, C, DH, CF → 'batter'

Integration (TestInitializeCardEvolution) — 9 tests against in-memory SQLite:

• First card creates state with current_tier=0, current_value=0.0, fully_evolved=False
• Duplicate card is a no-op; simulated partial progress (current_tier=2) is preserved
• Two different players on same team each get their own row
• SP player gets sp track, RP/CP player gets rp track
• Missing track returns None without raising
• End-to-end: _determine_card_type + initialize_card_evolution correctly routes all three types

Test results

16 passed (test_evolution_init.py)
12 passed (test_evolution_models.py — 3 were previously broken due to wrong import, also fixed here)

No regressions in any other test file.

Design decisions

• Fire-and-forget: The outer try/except in the router ensures evolution failures can never break pack opening. The inner try/except in the service catches DB-level surprises.
• No AI/Gauntlet exclusion: Unlike Paperdex, all teams get an evolution state. The spec does not exclude them and back-filling would be complex.
• No evolution_progress rows: Out of scope for WP-10 per spec. Progress accumulation is WP-07/WP-08.
• get_or_create not insert_or_ignore: Peewee's get_or_create returns the existing instance, making the "duplicate is a no-op" guarantee testable and explicit.

🤖 Generated with Claude Code

Closes #75. ## Summary - Add `app/services/evolution_init.py` with `initialize_card_evolution` and `_determine_card_type` - Hook into `POST /api/v2/cards` to initialize evolution state on pack opening - 16 new tests (all passing); also fixed pre-existing lint and import issues in `cards.py` and `test_evolution_models.py` ## What was built ### `app/services/evolution_init.py` (new) **`_determine_card_type(player)`** — pure function, no DB, maps `pos_1` to track type: - `pos_1` contains `SP` → `'sp'` - `pos_1` contains `RP` or `CP` → `'rp'` - anything else → `'batter'` **`initialize_card_evolution(player_id, team_id, card_type)`** — get_or_create pattern: - Looks up the `EvolutionTrack` matching `card_type` - Calls `EvolutionCardState.get_or_create(player_id, team_id, defaults={...})` - New state: `current_tier=0`, `current_value=0.0`, `fully_evolved=False` - Duplicate call for same `(player_id, team_id)` is a no-op — existing progress is never reset - Returns `None` on any failure (missing track seed, DB error); never raises ### `app/routers_v2/cards.py` (modified) Added WP-10 hook after `Card.bulk_create` and before the 200 response: ```python # WP-10: initialize evolution state for each new card (fire-and-forget) for x in cards.cards: try: this_player = Player.get_by_id(x.player_id) card_type = _determine_card_type(this_player) initialize_card_evolution(x.player_id, x.team_id, card_type) except Exception: logging.exception("evolution hook: unexpected error ...") ``` Also fixed 3 pre-existing ruff violations in `cards.py` (unused `lc_id`, bare f-string, unused `e` in except). ### `tests/test_evolution_init.py` (new, 16 tests) **Unit (`TestDetermineCardType`)** — 7 tests, no DB: - SP → `'sp'`, RP → `'rp'`, CP → `'rp'` - 1B, C, DH, CF → `'batter'` **Integration (`TestInitializeCardEvolution`)** — 9 tests against in-memory SQLite: - First card creates state with `current_tier=0`, `current_value=0.0`, `fully_evolved=False` - Duplicate card is a no-op; simulated partial progress (`current_tier=2`) is preserved - Two different players on same team each get their own row - SP player gets `sp` track, RP/CP player gets `rp` track - Missing track returns `None` without raising - End-to-end: `_determine_card_type` + `initialize_card_evolution` correctly routes all three types ## Test results ``` 16 passed (test_evolution_init.py) 12 passed (test_evolution_models.py — 3 were previously broken due to wrong import, also fixed here) ``` No regressions in any other test file. ## Design decisions - **Fire-and-forget**: The outer `try/except` in the router ensures evolution failures can never break pack opening. The inner `try/except` in the service catches DB-level surprises. - **No AI/Gauntlet exclusion**: Unlike `Paperdex`, all teams get an evolution state. The spec does not exclude them and back-filling would be complex. - **No `evolution_progress` rows**: Out of scope for WP-10 per spec. Progress accumulation is WP-07/WP-08. - **`get_or_create` not `insert_or_ignore`**: Peewee's `get_or_create` returns the existing instance, making the "duplicate is a no-op" guarantee testable and explicit. 🤖 Generated with [Claude Code](https://claude.ai/claude-code)

cal added 1 commit 2026-03-18 18:41:42 +00:00

feat(WP-10): pack opening hook — evolution_card_state initialization ... 264c7dc73c

Closes #75.

New file app/services/evolution_init.py:
- _determine_card_type(player): pure fn mapping pos_1 to 'batter'/'sp'/'rp'
- initialize_card_evolution(player_id, team_id, card_type): get_or_create
  EvolutionCardState with current_tier=0, current_value=0.0, fully_evolved=False
- Safe failure: all exceptions caught and logged, never raises
- Idempotent: duplicate calls for same (player_id, team_id) are no-ops
  and do NOT reset existing evolution progress

Modified app/routers_v2/cards.py:
- Add WP-10 hook after Card.bulk_create in the POST endpoint
- For each card posted, call _determine_card_type + initialize_card_evolution
- Wrapped in try/except so evolution failures cannot block pack opening
- Fix pre-existing lint violations (unused lc_id, bare f-string, unused e)

New file tests/test_evolution_init.py (16 tests, all passing):
- Unit: track assignment for batter / SP / RP / CP positions
- Integration: first card creates state with zeroed fields
- Integration: duplicate card is a no-op (progress not reset)
- Integration: different players on same team get separate states
- Integration: card_type routes to correct EvolutionTrack
- Integration: missing track returns None gracefully

Fix tests/test_evolution_models.py: correct PlayerSeasonStats import/usage

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

Claude added the

ai-reviewing

label 2026-03-18 18:45:48 +00:00

Claude approved these changes 2026-03-18 18:48:44 +00:00

Claude left a comment

Collaborator

Copy Link

AI Code Review

Files Reviewed

• app/services/evolution_init.py (added)
• app/routers_v2/cards.py (modified)
• tests/test_evolution_init.py (added)
• tests/test_evolution_models.py (modified — import fix)

Findings

Correctness

• _determine_card_type: Substring matching via .upper() is correct for all known position strings. "SP" in pos, "RP" in pos, "CP" in pos are unambiguous for the baseball position value space.
• get_or_create(player_id=player_id, team_id=team_id): Valid Peewee syntax — the raw FK column accessors player_id/team_id resolve against the UNIQUE index on (EvolutionCardState.player, EvolutionCardState.team). Consistent with the ProcessedGame pattern from PR #106.
• Double try/except structure: Inner catches DoesNotExist + unexpected errors on track lookup; outer catches get_or_create errors. Never raises. Fire-and-forget guarantee is solid.
• WP-10 hook placement: Correctly runs after Card.bulk_create (and after the atomic block), before the 200 response. Since bulk_create is inside db.atomic(), any bulk-create failure aborts the function before the hook runs — no phantom evolution states for uninserted cards.
• test_evolution_models.py fix: Import reorder (PlayerSeasonStats moved to head of import block) + docstring update (PlayerSeasonStats → BattingSeasonStats) reflecting the next-release rename. Minimal, correct.

Security

• No security issues. player_id and team_id flow from an authenticated, validated request body through internal Peewee ORM calls — no user-controlled strings reach raw SQL.
• [REDACTED] log pattern preserved throughout cards.py.
• No credentials exposed.

Style & Conventions

• New app/services/ package has a proper __init__.py — import paths work correctly.
• Importing _determine_card_type (private by convention) across modules is slightly unconventional, but the PR documents this explicitly and the function is genuinely shared between router and tests. Acceptable.
• Docstrings on both functions are thorough and follow the existing project pattern.
• Pre-existing lint fixes (bare f-string, unused lc_id) are clean and scoped.

Suggestions

• N+1 queries in WP-10 hook: The hook calls Player.get_by_id(x.player_id) per card in a separate loop. Since player_ids is already collected in the preceding loop, a single {p.player_id: p for p in Player.select().where(Player.player_id << player_ids)} dict would replace N queries with 1. Similarly, initialize_card_evolution re-fetches EvolutionTrack per call (3 possible values). Both are acceptable for fire-and-forget semantics and small pack sizes — noting for future optimization if pack sizes grow.
• raise HTTPException(status_code=200) at the end of v1_cards_post is a pre-existing issue (not introduced here); noted for a follow-up cleanup.

Verdict: APPROVED

Clean, correct implementation. The service layer is well-factored, the error-handling guarantees are sound, and the 16 tests cover all meaningful paths including idempotency, cross-player isolation, track routing, and the safe-failure (missing seed) path. No blocking issues.

---

Automated review by Claude PR Reviewer

## AI Code Review ### Files Reviewed - `app/services/evolution_init.py` (added) - `app/routers_v2/cards.py` (modified) - `tests/test_evolution_init.py` (added) - `tests/test_evolution_models.py` (modified — import fix) ### Findings #### Correctness - **`_determine_card_type`**: Substring matching via `.upper()` is correct for all known position strings. `"SP" in pos`, `"RP" in pos`, `"CP" in pos` are unambiguous for the baseball position value space. - **`get_or_create(player_id=player_id, team_id=team_id)`**: Valid Peewee syntax — the raw FK column accessors `player_id`/`team_id` resolve against the UNIQUE index on `(EvolutionCardState.player, EvolutionCardState.team)`. Consistent with the `ProcessedGame` pattern from PR #106. - **Double try/except structure**: Inner catches `DoesNotExist` + unexpected errors on track lookup; outer catches `get_or_create` errors. Never raises. Fire-and-forget guarantee is solid. - **WP-10 hook placement**: Correctly runs after `Card.bulk_create` (and after the atomic block), before the 200 response. Since `bulk_create` is inside `db.atomic()`, any bulk-create failure aborts the function before the hook runs — no phantom evolution states for uninserted cards. - **`test_evolution_models.py` fix**: Import reorder (`PlayerSeasonStats` moved to head of import block) + docstring update (`PlayerSeasonStats` → `BattingSeasonStats`) reflecting the `next-release` rename. Minimal, correct. #### Security - No security issues. `player_id` and `team_id` flow from an authenticated, validated request body through internal Peewee ORM calls — no user-controlled strings reach raw SQL. - `[REDACTED]` log pattern preserved throughout `cards.py`. - No credentials exposed. #### Style & Conventions - New `app/services/` package has a proper `__init__.py` — import paths work correctly. - Importing `_determine_card_type` (private by convention) across modules is slightly unconventional, but the PR documents this explicitly and the function is genuinely shared between router and tests. Acceptable. - Docstrings on both functions are thorough and follow the existing project pattern. - Pre-existing lint fixes (bare f-string, unused `lc_id`) are clean and scoped. #### Suggestions - **N+1 queries in WP-10 hook**: The hook calls `Player.get_by_id(x.player_id)` per card in a separate loop. Since `player_ids` is already collected in the preceding loop, a single `{p.player_id: p for p in Player.select().where(Player.player_id << player_ids)}` dict would replace N queries with 1. Similarly, `initialize_card_evolution` re-fetches `EvolutionTrack` per call (3 possible values). Both are acceptable for fire-and-forget semantics and small pack sizes — noting for future optimization if pack sizes grow. - **`raise HTTPException(status_code=200)`** at the end of `v1_cards_post` is a pre-existing issue (not introduced here); noted for a follow-up cleanup. ### Verdict: APPROVED Clean, correct implementation. The service layer is well-factored, the error-handling guarantees are sound, and the 16 tests cover all meaningful paths including idempotency, cross-player isolation, track routing, and the safe-failure (missing seed) path. No blocking issues. --- *Automated review by Claude PR Reviewer*

Claude added

ai-reviewed

and removed

ai-reviewing

labels 2026-03-18 18:49:05 +00:00

cal reviewed 2026-03-18 20:15:07 +00:00

cal left a comment

Author

Owner

Copy Link

AI Code Review

Files Reviewed

• app/routers_v2/cards.py (modified)
• app/services/evolution_init.py (added)
• migrations/2026-03-17_add_evolution_tables.sql (modified)
• tests/test_evolution_init.py (added)
• tests/test_evolution_models.py (modified)
• tests/test_evolution_state_api.py (added)

---

Findings

Correctness

The core logic is correct.

Hook placement relative to bulk_create: The evolution hook loops over cards.cards (the raw input payload), not the returned ORM instances from bulk_create. This is fine — player_id and team_id come directly from the input, and the hook only needs those two values.

Hook runs outside the atomic block: Card.bulk_create runs inside with db.atomic(). The evolution hook runs after that block closes, so an evolution init failure cannot roll back the card insert. This is correct and intentional per the fire-and-forget design.

EvolutionTrack.get() with no uniqueness constraint on card_type: The EvolutionTrack model defines card_type = CharField() with no unique constraint. If two tracks were seeded with the same card_type, EvolutionTrack.get(EvolutionTrack.card_type == card_type) would silently return the first matching row — Peewee's get() uses result[0] internally and does not raise on multiple results. The broad except Exception in the outer layer does not catch this because no exception is raised; the wrong track would be silently assigned. In practice this is a seeding concern (the spec defines exactly three tracks), but the assumption is unenforced at the DB level.

_determine_card_type substring matching: The .upper() call is correct. The in operator checks for substring presence, so any pos_1 value containing "SP" as a substring would route to sp. With the tightly controlled two-to-three character position strings in this codebase (SP, RP, CP, 1B, etc.) this is not a real risk, but the assumption should hold.

test_evolution_models.py docstring drift: The diff updates the section header comment and the class docstring to say BattingSeasonStats, but the Python import, fixture, and class under test remain PlayerSeasonStats (correctly — the ORM class name has not changed, only the SQL table name was renamed in the migration). This creates a comment-vs-code mismatch that could confuse a future reader but does not affect correctness.

Security

No issues. The hook receives player_id and team_id from an already-authenticated, already-validated payload. No new endpoints are exposed. The service is not callable from any public route.

Style & Conventions

The new code follows all established patterns:

• Peewee get_or_create with a defaults dict matches the existing Paperdex.get_or_create pattern directly above the hook in cards.py.
• Module-level logger = logging.getLogger(__name__) is consistent with season_stats.py and other services.
• Double try/except structure (inner for get_or_create, outer for EvolutionTrack.get) with explicit DoesNotExist handling before the broad except Exception fallback is well-reasoned.
• The _determine_card_type leading underscore correctly signals internal-but-exported.

The lc_id / last_card removal is a clean, correct fix — that variable was fetched before bulk_create and only used in the commented-out sheets.post_new_cards call.

Suggestions (non-blocking)

EvolutionTrack.card_type unique constraint: Consider adding a unique constraint on evolution_track.card_type in the migration (or a follow-on migration). As written, duplicate seed data causes silent wrong-track assignment rather than a loud error. A DB-level constraint would enforce the three-track invariant without any code change.

Comment drift in test_evolution_models.py: The section header # BattingSeasonStats and the class docstring saying Tests for BattingSeasonStats should be reverted to PlayerSeasonStats since the Python class name did not change. The rename only applied to the SQL table name.

test_evolution_state_api.py skip scope: The _skip_no_pg mark is applied per-test function, which is correct. Just confirming the Gitea Actions CI runner either has POSTGRES_HOST set or the skip fires cleanly — the module-level guard pattern used here is the right approach regardless.

---

Verdict: APPROVED

The implementation is correct, safe, and well-tested. The fire-and-forget wrapper, get-or-create idempotency, and track-type routing all behave as specified in WP-10. The 16 unit and integration tests cover the key behavioral requirements including idempotency, cross-player isolation, and the missing-track safe-failure path. The pre-existing lint fixes are welcome.

The card_type uniqueness observation is worth a follow-up but does not block this PR — the seeding data is controlled and the spec is unambiguous about exactly three tracks.

---

Automated review by Claude PR Reviewer

## AI Code Review ### Files Reviewed - `app/routers_v2/cards.py` (modified) - `app/services/evolution_init.py` (added) - `migrations/2026-03-17_add_evolution_tables.sql` (modified) - `tests/test_evolution_init.py` (added) - `tests/test_evolution_models.py` (modified) - `tests/test_evolution_state_api.py` (added) --- ### Findings #### Correctness The core logic is correct. **Hook placement relative to `bulk_create`**: The evolution hook loops over `cards.cards` (the raw input payload), not the returned ORM instances from `bulk_create`. This is fine — `player_id` and `team_id` come directly from the input, and the hook only needs those two values. **Hook runs outside the atomic block**: `Card.bulk_create` runs inside `with db.atomic()`. The evolution hook runs after that block closes, so an evolution init failure cannot roll back the card insert. This is correct and intentional per the fire-and-forget design. **`EvolutionTrack.get()` with no uniqueness constraint on `card_type`**: The `EvolutionTrack` model defines `card_type = CharField()` with no unique constraint. If two tracks were seeded with the same `card_type`, `EvolutionTrack.get(EvolutionTrack.card_type == card_type)` would silently return the first matching row — Peewee's `get()` uses `result[0]` internally and does not raise on multiple results. The broad `except Exception` in the outer layer does not catch this because no exception is raised; the wrong track would be silently assigned. In practice this is a seeding concern (the spec defines exactly three tracks), but the assumption is unenforced at the DB level. **`_determine_card_type` substring matching**: The `.upper()` call is correct. The `in` operator checks for substring presence, so any `pos_1` value containing `"SP"` as a substring would route to `sp`. With the tightly controlled two-to-three character position strings in this codebase (`SP`, `RP`, `CP`, `1B`, etc.) this is not a real risk, but the assumption should hold. **`test_evolution_models.py` docstring drift**: The diff updates the section header comment and the class docstring to say `BattingSeasonStats`, but the Python import, fixture, and class under test remain `PlayerSeasonStats` (correctly — the ORM class name has not changed, only the SQL table name was renamed in the migration). This creates a comment-vs-code mismatch that could confuse a future reader but does not affect correctness. #### Security No issues. The hook receives `player_id` and `team_id` from an already-authenticated, already-validated payload. No new endpoints are exposed. The service is not callable from any public route. #### Style & Conventions The new code follows all established patterns: - Peewee `get_or_create` with a `defaults` dict matches the existing `Paperdex.get_or_create` pattern directly above the hook in `cards.py`. - Module-level `logger = logging.getLogger(__name__)` is consistent with `season_stats.py` and other services. - Double try/except structure (inner for `get_or_create`, outer for `EvolutionTrack.get`) with explicit `DoesNotExist` handling before the broad `except Exception` fallback is well-reasoned. - The `_determine_card_type` leading underscore correctly signals internal-but-exported. The `lc_id` / `last_card` removal is a clean, correct fix — that variable was fetched before `bulk_create` and only used in the commented-out `sheets.post_new_cards` call. #### Suggestions (non-blocking) **`EvolutionTrack.card_type` unique constraint**: Consider adding a unique constraint on `evolution_track.card_type` in the migration (or a follow-on migration). As written, duplicate seed data causes silent wrong-track assignment rather than a loud error. A DB-level constraint would enforce the three-track invariant without any code change. **Comment drift in `test_evolution_models.py`**: The section header `# BattingSeasonStats` and the class docstring saying `Tests for BattingSeasonStats` should be reverted to `PlayerSeasonStats` since the Python class name did not change. The rename only applied to the SQL table name. **`test_evolution_state_api.py` skip scope**: The `_skip_no_pg` mark is applied per-test function, which is correct. Just confirming the Gitea Actions CI runner either has `POSTGRES_HOST` set or the skip fires cleanly — the module-level guard pattern used here is the right approach regardless. --- ### Verdict: APPROVED The implementation is correct, safe, and well-tested. The fire-and-forget wrapper, get-or-create idempotency, and track-type routing all behave as specified in WP-10. The 16 unit and integration tests cover the key behavioral requirements including idempotency, cross-player isolation, and the missing-track safe-failure path. The pre-existing lint fixes are welcome. The `card_type` uniqueness observation is worth a follow-up but does not block this PR — the seeding data is controlled and the spec is unambiguous about exactly three tracks. --- *Automated review by Claude PR Reviewer*

cal merged commit f5b24cf8f2 into card-evolution 2026-03-18 20:32:01 +00:00

cal deleted branch feature/wp10-pack-opening-hook 2026-03-18 20:32:01 +00:00

cal referenced this issue from a commit 2026-03-18 20:32:01 +00:00

Merge pull request 'feat(WP-10): pack opening hook — evolution_card_state initialization' (#107) from feature/wp10-pack-opening-hook into card-evolution

Sign in to join this conversation.

Reviewers

No reviewers

Claude

Labels

Clear labels   

ai-changes-requested

  

ai-failed

  

ai-merged

PR merged by AI ops agent

  

ai-pr-opened

  

ai-reviewed

  

ai-reviewing

  

ai-reviewing

  

ai-working

  

bug

  

enhancement

  

evolution

Card Evolution system

  

performance

  

phase-0

Render Pipeline Optimization

  

phase-1a

Schema & Data Foundation

  

phase-1b

API & Formula Engine

  

phase-1c

Bot Integration

  

phase-1d

Deployment

  

security

  

tech-debt

  

todo

No Label

ai-changes-requested

ai-failed

ai-merged

ai-pr-opened

ai-reviewed

ai-reviewing

ai-reviewing

ai-working

bug

enhancement

evolution

performance

phase-0

phase-1a

phase-1b

phase-1c

phase-1d

security

tech-debt

todo

Milestone

Clear milestone

No items

No Milestone

Projects

Clear projects

No project

Assignees

Clear assignees

No Assignees

2 Participants

Notifications

Subscribe

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

No description provided.