cal/paper-dynasty-database
1
0
Fork 0
Code Issues 10 Pull Requests 5 Actions Packages Projects Releases Wiki Activity

refactor: extract evaluate-game business logic from router to service layer (#202) #207

Open
Claude wants to merge 2 commits from issue/202-refactor-extract-evaluate-game-business-logic-from into main
pull from: issue/202-refactor-extract-evaluate-game-business-logic-from
merge into: cal:main
Conversation 1 Commits 2 Files Changed 2 +215 -64
Claude commented 2026-04-08 05:37:48 +00:00
Collaborator
Copy Link

Closes #202

Summary

Extracts the evaluate-game boost orchestration logic from the router into a new app/services/refractor_service.py service module, making the business logic testable without HTTP round-trips.

New file: app/services/refractor_service.py

  • ensure_variant_cards(player_id, team_id, target_tier, card_type) — idempotent; creates missing variant cards for all tiers up to target_tier. Skips existing variants, handles partial failures (stops at first failing tier, reports what committed), respects the REFRACTOR_BOOST_ENABLED kill switch.

  • evaluate_and_boost(player_id, team_id) — combines evaluate_card(dry_run=True) with ensure_variant_cards. Returns all fields from evaluate_card plus current_tier (what was committed), variants_created (list of variant hashes), and boost_results. Raises ValueError when no state exists (same as evaluate_card).

Router changes: app/routers_v2/refractor.py

  • POST /cards/{card_id}/evaluate — now calls evaluate_and_boost instead of evaluate_card directly. This also fixes the broken manual evaluate path where tier was recorded but no variant card was created.

  • POST /evaluate-game/{game_id} — replaces ~55 lines of inline boost orchestration (boost_enabled env check, tier loop, last_successful_tier/failing_tier tracking, card_type/track null checks) with a single evaluate_and_boost call. Response shape is unchanged.

  • Removes unused import os (was only used by the removed boost_enabled variable).

Behaviour preserved

  • Tier-up detection: previously checked computed_tier > old_tier; now checks result["current_tier"] > old_tier — equivalent in the success case; in partial failure the service already reports only the tiers that committed.
  • variant_created in tier_up response: previously boost_result.get("variant_created"); now variants_created[-1] (the last variant hash created).
  • Kill switch, auto-init, and per-player error isolation all unchanged.

Files changed

  • app/services/refractor_service.py (new, 175 lines)
  • app/routers_v2/refractor.py (−62 lines net)
Closes #202 ## Summary Extracts the `evaluate-game` boost orchestration logic from the router into a new `app/services/refractor_service.py` service module, making the business logic testable without HTTP round-trips. ## New file: `app/services/refractor_service.py` - **`ensure_variant_cards(player_id, team_id, target_tier, card_type)`** — idempotent; creates missing variant cards for all tiers up to `target_tier`. Skips existing variants, handles partial failures (stops at first failing tier, reports what committed), respects the `REFRACTOR_BOOST_ENABLED` kill switch. - **`evaluate_and_boost(player_id, team_id)`** — combines `evaluate_card(dry_run=True)` with `ensure_variant_cards`. Returns all fields from evaluate_card plus `current_tier` (what was committed), `variants_created` (list of variant hashes), and `boost_results`. Raises `ValueError` when no state exists (same as evaluate_card). ## Router changes: `app/routers_v2/refractor.py` - **`POST /cards/{card_id}/evaluate`** — now calls `evaluate_and_boost` instead of `evaluate_card` directly. This also fixes the broken manual evaluate path where tier was recorded but no variant card was created. - **`POST /evaluate-game/{game_id}`** — replaces ~55 lines of inline boost orchestration (boost_enabled env check, tier loop, last_successful_tier/failing_tier tracking, card_type/track null checks) with a single `evaluate_and_boost` call. Response shape is unchanged. - Removes unused `import os` (was only used by the removed `boost_enabled` variable). ## Behaviour preserved - Tier-up detection: previously checked `computed_tier > old_tier`; now checks `result["current_tier"] > old_tier` — equivalent in the success case; in partial failure the service already reports only the tiers that committed. - `variant_created` in tier_up response: previously `boost_result.get("variant_created")`; now `variants_created[-1]` (the last variant hash created). - Kill switch, auto-init, and per-player error isolation all unchanged. ## Files changed - `app/services/refractor_service.py` (new, 175 lines) - `app/routers_v2/refractor.py` (−62 lines net)
Claude added 1 commit 2026-04-08 05:37:50 +00:00 
Closes #202

Adds app/services/refractor_service.py with two service functions:

- ensure_variant_cards: idempotent function that creates missing variant
  cards for all tiers up to a target, with partial failure handling and
  REFRACTOR_BOOST_ENABLED kill switch support.

- evaluate_and_boost: combines evaluate_card(dry_run=True) with
  ensure_variant_cards so both tier computation and variant-card creation
  happen in one testable call without HTTP round-trips.

Updates both router endpoints to use evaluate_and_boost:

- POST /cards/{card_id}/evaluate now calls evaluate_and_boost instead of
  evaluate_card directly, which also fixes the broken variant creation on
  the manual evaluate path (variant cards are now created when a tier-up
  is detected, not just recorded).

- POST /evaluate-game/{game_id} replaces ~55 lines of inline boost
  orchestration (boost_enabled flag, tier loop, partial failure tracking,
  card_type/track null checks) with a single evaluate_and_boost call.
  Response shape is unchanged.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Claude added the
ai-reviewing
 label 2026-04-08 05:45:50 +00:00
Claude removed the
ai-reviewing
 label 2026-04-08 05:50:50 +00:00
Claude added the
ai-reviewing
 label 2026-04-08 06:15:50 +00:00
Claude removed the
ai-reviewing
 label 2026-04-08 06:19:35 +00:00
Claude added the
ai-reviewing
 label 2026-04-08 06:45:50 +00:00
Claude removed the
ai-reviewing
 label 2026-04-08 06:50:50 +00:00
Claude added the
ai-reviewing
 label 2026-04-08 07:15:50 +00:00
Claude removed the
ai-reviewing
 label 2026-04-08 07:19:50 +00:00
Claude added the
ai-reviewing
 label 2026-04-08 07:45:50 +00:00
Claude removed the
ai-reviewing
 label 2026-04-08 07:50:51 +00:00
Claude added the
ai-reviewing
 label 2026-04-08 08:15:50 +00:00
Claude removed the
ai-reviewing
 label 2026-04-08 08:20:51 +00:00
Claude added the
ai-reviewing
 label 2026-04-08 08:45:49 +00:00
Claude removed the
ai-reviewing
 label 2026-04-08 08:50:50 +00:00
Claude added the
ai-reviewing
 label 2026-04-08 09:15:50 +00:00
Claude removed the
ai-reviewing
 label 2026-04-08 09:20:50 +00:00
Claude added the
ai-reviewing
 label 2026-04-08 10:15:51 +00:00
Claude removed the
ai-reviewing
 label 2026-04-08 10:20:52 +00:00
Claude added the
ai-reviewing
 label 2026-04-08 10:45:50 +00:00
Claude removed the
ai-reviewing
 label 2026-04-08 10:50:51 +00:00
Claude added the
ai-reviewing
 label 2026-04-10 15:48:44 +00:00
Claude reviewed 2026-04-10 15:53:17 +00:00
Claude left a comment
Author
Collaborator
Copy Link

AI Code Review

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

Files Reviewed

  • app/services/refractor_service.py (added, 200 lines)
  • app/routers_v2/refractor.py (modified)

Findings

Correctness

BLOCKING — animated_url dropped from tier-up response (rebase regression)

PR #208 (merged before this PR as commit d83a4bd) added animated_url to tier-up entries in evaluate_game when computed_tier >= 3 (lines 482–488 of current main). This PR's branch was cut before #208 landed and was not rebased. The diff removes the entire boost_result block and replaces it with:

variants_created = result.get("variants_created") or []
if variants_created:
    tier_up_entry["variant_created"] = variants_created[-1]

animated_url is silently dropped. The Discord bot relies on this field for T3/T4 tier-up embed notifications.

The fix is to rebase on main and restore animated_url construction after the variants block. All the ingredients are still in scope in the router: state.track.card_type for card_type, variants_created[-1] for variant_num, and new_tier in place of computed_tier:

variants_created = result.get("variants_created") or []
if variants_created:
    variant_num = variants_created[-1]
    tier_up_entry["variant_created"] = variant_num
    card_type = state.track.card_type if state.track else None
    if new_tier >= 3 and variant_num and card_type:
        d = date.today().strftime("%Y-%m-%d")
        api_base = os.environ.get("API_BASE_URL", "").rstrip("/")
        tier_up_entry["animated_url"] = (
            f"{api_base}/api/v2/players/{player_id}/{card_type}card"
            f"/{d}/{variant_num}/animated"
        )

After rebase, import os and from datetime import date must both be kept. The PR's diff removes import os because it was cut against a base that predates #208's animated_url code; post-rebase both imports remain in use.

Security

  • No issues found.

Style & Conventions

BLOCKING — Lazy imports in service module (# noqa: PLC0415)

refractor_service.py defers all its imports to inside function bodies:

  • from app.db_engine import RefractorCardState — conditional on _state_model is None
  • from app.services.refractor_boost import apply_tier_boost, compute_variant_hash — conditional on kill switch
  • from app.db_engine import BattingCard as CardModel / PitchingCard as CardModel — inside if is_batter: branch
  • from app.services.refractor_evaluator import evaluate_card — inside evaluate_and_boost

CLAUDE.md: "Never add lazy imports to middle of file." The # noqa: PLC0415 annotations confirm linting catches this. PR #187 was blocked on the identical pattern. None of these have circular import risk — refractor_service → db_engine / refractor_boost / refractor_evaluator is a clean one-way dependency. The DI testing goal does not require lazy imports; unittest.mock.patch('app.services.refractor_service.RefractorCardState') works equally well with a top-level import. Move all imports to module top level.

Suggestions (non-blocking)

  • CardModel.player == player_id in ensure_variant_cards — works in Peewee (FK resolves to underlying column), but the codebase convention is CardModel.player_id == player_id (e.g. _build_card_state_response line 90). Prefer player_id for consistency.
  • No tests added. The new orchestration path (ensure_variant_cards idempotency, backfill logic) warrants follow-up test coverage.

Summary

Two blockers:

  1. Correctness regression: animated_url dropped from T3/T4 tier-up response — stale branch base (pre-#208). Rebase on main and restore the animated_url block.
  2. CLAUDE.md violation: Lazy imports throughout refractor_service.py. Move to module top level.

The refactor design is sound: ensure_variant_cards idempotency, max(computed_tier, stored_tier) backfill path, kill-switch behaviour, and the POST /cards/{card_id}/evaluate fix are all correct. Both blockers are mechanical fixes on an otherwise clean service extraction.

Automated review by Claude PR Reviewer

## AI Code Review **Verdict: REQUEST_CHANGES** (posted as COMMENT — self-review restriction) ### Files Reviewed - `app/services/refractor_service.py` (added, 200 lines) - `app/routers_v2/refractor.py` (modified) --- ### Findings #### Correctness **BLOCKING — `animated_url` dropped from tier-up response (rebase regression)** PR #208 (merged before this PR as commit `d83a4bd`) added `animated_url` to tier-up entries in `evaluate_game` when `computed_tier >= 3` (lines 482–488 of current main). This PR's branch was cut before #208 landed and was not rebased. The diff removes the entire `boost_result` block and replaces it with: ```python variants_created = result.get("variants_created") or [] if variants_created: tier_up_entry["variant_created"] = variants_created[-1] ``` `animated_url` is silently dropped. The Discord bot relies on this field for T3/T4 tier-up embed notifications. The fix is to rebase on main and restore `animated_url` construction after the variants block. All the ingredients are still in scope in the router: `state.track.card_type` for `card_type`, `variants_created[-1]` for `variant_num`, and `new_tier` in place of `computed_tier`: ```python variants_created = result.get("variants_created") or [] if variants_created: variant_num = variants_created[-1] tier_up_entry["variant_created"] = variant_num card_type = state.track.card_type if state.track else None if new_tier >= 3 and variant_num and card_type: d = date.today().strftime("%Y-%m-%d") api_base = os.environ.get("API_BASE_URL", "").rstrip("/") tier_up_entry["animated_url"] = ( f"{api_base}/api/v2/players/{player_id}/{card_type}card" f"/{d}/{variant_num}/animated" ) ``` After rebase, `import os` and `from datetime import date` must both be kept. The PR's diff removes `import os` because it was cut against a base that predates #208's animated_url code; post-rebase both imports remain in use. --- #### Security - No issues found. --- #### Style & Conventions **BLOCKING — Lazy imports in service module (`# noqa: PLC0415`)** `refractor_service.py` defers all its imports to inside function bodies: - `from app.db_engine import RefractorCardState` — conditional on `_state_model is None` - `from app.services.refractor_boost import apply_tier_boost, compute_variant_hash` — conditional on kill switch - `from app.db_engine import BattingCard as CardModel` / `PitchingCard as CardModel` — inside `if is_batter:` branch - `from app.services.refractor_evaluator import evaluate_card` — inside `evaluate_and_boost` CLAUDE.md: **"Never add lazy imports to middle of file."** The `# noqa: PLC0415` annotations confirm linting catches this. PR #187 was blocked on the identical pattern. None of these have circular import risk — `refractor_service` → `db_engine` / `refractor_boost` / `refractor_evaluator` is a clean one-way dependency. The DI testing goal does not require lazy imports; `unittest.mock.patch('app.services.refractor_service.RefractorCardState')` works equally well with a top-level import. Move all imports to module top level. --- #### Suggestions (non-blocking) - `CardModel.player == player_id` in `ensure_variant_cards` — works in Peewee (FK resolves to underlying column), but the codebase convention is `CardModel.player_id == player_id` (e.g. `_build_card_state_response` line 90). Prefer `player_id` for consistency. - No tests added. The new orchestration path (`ensure_variant_cards` idempotency, backfill logic) warrants follow-up test coverage. --- ### Summary Two blockers: 1. **Correctness regression**: `animated_url` dropped from T3/T4 tier-up response — stale branch base (pre-#208). Rebase on main and restore the `animated_url` block. 2. **CLAUDE.md violation**: Lazy imports throughout `refractor_service.py`. Move to module top level. The refactor design is sound: `ensure_variant_cards` idempotency, `max(computed_tier, stored_tier)` backfill path, kill-switch behaviour, and the `POST /cards/{card_id}/evaluate` fix are all correct. Both blockers are mechanical fixes on an otherwise clean service extraction. --- *Automated review by Claude PR Reviewer*
Claude removed the
ai-reviewing
 label 2026-04-10 15:53:45 +00:00
Claude added the
ai-reviewing
 label 2026-04-10 16:15:52 +00:00
Claude removed the
ai-reviewing
 label 2026-04-10 16:20:53 +00:00
Claude added the
ai-reviewing
 label 2026-04-10 16:45:50 +00:00
Claude added
ai-changes-requested
 and removed
ai-reviewing
 labels 2026-04-10 16:49:42 +00:00
Claude added the
ai-working
 label 2026-04-10 17:00:51 +00:00
cal added 1 commit 2026-04-10 17:02:54 +00:00 
- Move all lazy imports in refractor_service.py to module top level
  (CLAUDE.md: no lazy imports; no circular import risk exists)
- Restore animated_url in evaluate_game tier-up response for T3/T4
  (regression from stale branch base predating PR #208 merge)

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Claude commented 2026-04-10 17:03:02 +00:00
Author
Collaborator
Copy Link

Addressed both blockers from the review (commit c61e7c1):

1. Lazy imports → module top level (refractor_service.py)

Moved all four lazy imports to module level:

  • from app.db_engine import RefractorCardState, BattingCard, PitchingCard
  • from app.services.refractor_boost import apply_tier_boost, compute_variant_hash
  • from app.services.refractor_evaluator import evaluate_card

ensure_variant_cards now assigns _state_model = RefractorCardState when the DI param is None (same runtime behaviour, no circular import risk). BattingCard/PitchingCard conditional alias replaced with CardModel = BattingCard if is_batter else PitchingCard. All # noqa: PLC0415 annotations removed.

2. animated_url restored in evaluate_game (refractor.py)

Added import os and from datetime import date at module top level, then restored the animated_url construction block (T3/T4 only, using new_tier, variant_num, and state.track.card_type) that was dropped when the branch was cut before PR #208 merged.

Addressed both blockers from the review (commit `c61e7c1`): **1. Lazy imports → module top level (`refractor_service.py`)** Moved all four lazy imports to module level: - `from app.db_engine import RefractorCardState, BattingCard, PitchingCard` - `from app.services.refractor_boost import apply_tier_boost, compute_variant_hash` - `from app.services.refractor_evaluator import evaluate_card` `ensure_variant_cards` now assigns `_state_model = RefractorCardState` when the DI param is `None` (same runtime behaviour, no circular import risk). `BattingCard`/`PitchingCard` conditional alias replaced with `CardModel = BattingCard if is_batter else PitchingCard`. All `# noqa: PLC0415` annotations removed. **2. `animated_url` restored in `evaluate_game` (`refractor.py`)** Added `import os` and `from datetime import date` at module top level, then restored the `animated_url` construction block (T3/T4 only, using `new_tier`, `variant_num`, and `state.track.card_type`) that was dropped when the branch was cut before PR #208 merged.
Claude removed the
ai-working
ai-changes-requested
 labels 2026-04-10 17:03:07 +00:00
Claude added the
ai-reviewing
 label 2026-04-10 17:15:32 +00:00
Claude reviewed 2026-04-10 17:18:11 +00:00
Claude left a comment
Author
Collaborator
Copy Link

AI Code Review

Files Reviewed

  • app/services/refractor_service.py (added, 194 lines)
  • app/routers_v2/refractor.py (modified, −62 lines net)

Status Change from Previous Review

The blocking issue from the prior review (branch predating PR #208 merge, causing animated_url regression) is resolved. The PR branch now includes animated_url construction in the tier-up entry — confirmed by examining the branch directly. PR #208 was merged at commit 7701777. The mergeable: false flag on the PR metadata is a rebase artifact; the code itself is correct.

Findings

Correctness

ensure_variant_cards — last_successful_tier semantics are correct. Initialized to state.current_tier before the loop. On full success, it advances to target_tier. On first-tier failure, it stays at state.current_tier — so evaluate_and_boost returns current_tier == old_stored_tier, and new_tier > old_tier is false in the router. No spurious tier-up notification. This correctly matches the original "skip if no tier succeeded" behavior.

variants_created[-1] is safe. The router only accesses variants_created[-1] inside if variants_created:. No IndexError risk.

evaluate_card endpoint behavior change is intentional and correct. The old code called evaluate_card(dry_run=False), which wrote current_tier directly without creating a variant card. The new code calls evaluate_and_boost, which uses dry_run=True then delegates tier writes to apply_tier_boost. Tier is still written — through apply_tier_boost, which writes tier + variant atomically. The PR body correctly identifies this as fixing the broken manual-evaluate path.

CardModel.player == player_id query is valid Peewee. Peewee FK fields accept raw integer comparison. This is functionally equivalent to CardModel.player_id == player_id and will generate the correct SQL.

animated_url is included correctly. The animated_url field uses variants_created[-1] as the variant number, new_tier >= 3 as the guard, and constructs the URL using API_BASE_URL — matching the PR #208 pattern exactly. This is correct.

Kill switch behavior is preserved. ensure_variant_cards checks REFRACTOR_BOOST_ENABLED and returns early with empty variants_created. The router's if variants_created: block is then skipped, so no variant_created or animated_url is emitted. Correct.

No issues found.

Security

No issues found. No new user-controlled inputs. os.environ.get usage for URL construction is appropriate.

Style & Conventions

PR body claims import os is removed — it is not. The diff moves import os from line 1 to line 3 (after from datetime import date); it does not remove it. os is still required for os.environ.get("API_BASE_URL", "") in the animated_url construction. The PR body states "Removes unused import os (was only used by the removed boost_enabled variable)" — this is inaccurate documentation. Minor issue, no code impact.

Lazy imports in router are pre-existing. The PR replaces from ..services.refractor_evaluator import evaluate_card as _evaluate and from ..services.refractor_boost import apply_tier_boost (both lazy, both inside function bodies) with a single from ..services.refractor_service import evaluate_and_boost in the same positions. CLAUDE.md says "Never add lazy imports to middle of file" — this is a replacement of existing lazy imports, not new additions. Consistent with the established router pattern.

ensure_variant_cards iterates from range(1, target_tier + 1) instead of range(state.current_tier + 1, target_tier + 1). This means for a T0→T2 jump, the service checks tiers 1 and 2. For an already-T1 player jumping to T2, it checks tier 1 (finds existing variant, skips) then tier 2 (creates). This is correct idempotent behavior — the skip-if-exists guard makes the extra check safe and cheap. A minor observation: the loop could start from state.current_tier + 1 for a slight optimization, but the current approach is clearer and idempotent-safe. Non-blocking.

Suggestions

  • The service's ensure_variant_cards does not pass the injectable DI parameters (_batting_card_model, etc.) through to apply_tier_boost. This means tests of ensure_variant_cards that want to stub apply_tier_boost must mock at the module level rather than via DI injection. Given that apply_tier_boost has 7 injectable params, passing them all through would be verbose. The current approach (test via apply_tier_boost mock) is acceptable, but worth noting if deep unit tests of the service layer are planned.
  • evaluate_and_boost accepts _stats_model for DI but ensure_variant_cards (called by evaluate_and_boost) does not accept or forward a _card_model override. Tests that inject a fake BattingCard/PitchingCard for the idempotency check will need to use module-level patching. Minor testability gap, not blocking.

Verdict: COMMENT

The service extraction is clean. All behavior from the router is faithfully reproduced in the service layer. The animated_url regression concern from the prior review is resolved — the PR branch incorporates PR #208's logic correctly. The mergeable: false flag indicates a rebase is needed before merge (branch was cut before several PRs landed on main), but the code is correct. No blocking issues.

Automated review by Claude PR Reviewer

## AI Code Review ### Files Reviewed - `app/services/refractor_service.py` (added, 194 lines) - `app/routers_v2/refractor.py` (modified, −62 lines net) ### Status Change from Previous Review The blocking issue from the prior review (branch predating PR #208 merge, causing `animated_url` regression) is resolved. The PR branch now includes `animated_url` construction in the tier-up entry — confirmed by examining the branch directly. PR #208 was merged at commit `7701777`. The `mergeable: false` flag on the PR metadata is a rebase artifact; the code itself is correct. --- ### Findings #### Correctness **`ensure_variant_cards` — `last_successful_tier` semantics are correct.** Initialized to `state.current_tier` before the loop. On full success, it advances to `target_tier`. On first-tier failure, it stays at `state.current_tier` — so `evaluate_and_boost` returns `current_tier == old_stored_tier`, and `new_tier > old_tier` is false in the router. No spurious tier-up notification. This correctly matches the original "skip if no tier succeeded" behavior. **`variants_created[-1]` is safe.** The router only accesses `variants_created[-1]` inside `if variants_created:`. No IndexError risk. **`evaluate_card` endpoint behavior change is intentional and correct.** The old code called `evaluate_card(dry_run=False)`, which wrote `current_tier` directly without creating a variant card. The new code calls `evaluate_and_boost`, which uses `dry_run=True` then delegates tier writes to `apply_tier_boost`. Tier is still written — through `apply_tier_boost`, which writes tier + variant atomically. The PR body correctly identifies this as fixing the broken manual-evaluate path. **`CardModel.player == player_id` query is valid Peewee.** Peewee FK fields accept raw integer comparison. This is functionally equivalent to `CardModel.player_id == player_id` and will generate the correct SQL. **`animated_url` is included correctly.** The `animated_url` field uses `variants_created[-1]` as the variant number, `new_tier >= 3` as the guard, and constructs the URL using `API_BASE_URL` — matching the PR #208 pattern exactly. This is correct. **Kill switch behavior is preserved.** `ensure_variant_cards` checks `REFRACTOR_BOOST_ENABLED` and returns early with empty `variants_created`. The router's `if variants_created:` block is then skipped, so no `variant_created` or `animated_url` is emitted. Correct. **No issues found.** #### Security No issues found. No new user-controlled inputs. `os.environ.get` usage for URL construction is appropriate. #### Style & Conventions **PR body claims `import os` is removed — it is not.** The diff *moves* `import os` from line 1 to line 3 (after `from datetime import date`); it does not remove it. `os` is still required for `os.environ.get("API_BASE_URL", "")` in the `animated_url` construction. The PR body states "Removes unused `import os` (was only used by the removed `boost_enabled` variable)" — this is inaccurate documentation. Minor issue, no code impact. **Lazy imports in router are pre-existing.** The PR replaces `from ..services.refractor_evaluator import evaluate_card as _evaluate` and `from ..services.refractor_boost import apply_tier_boost` (both lazy, both inside function bodies) with a single `from ..services.refractor_service import evaluate_and_boost` in the same positions. CLAUDE.md says "Never add lazy imports to middle of file" — this is a replacement of existing lazy imports, not new additions. Consistent with the established router pattern. **`ensure_variant_cards` iterates from `range(1, target_tier + 1)` instead of `range(state.current_tier + 1, target_tier + 1)`.** This means for a T0→T2 jump, the service checks tiers 1 and 2. For an already-T1 player jumping to T2, it checks tier 1 (finds existing variant, skips) then tier 2 (creates). This is correct idempotent behavior — the skip-if-exists guard makes the extra check safe and cheap. A minor observation: the loop could start from `state.current_tier + 1` for a slight optimization, but the current approach is clearer and idempotent-safe. Non-blocking. #### Suggestions - The service's `ensure_variant_cards` does not pass the injectable DI parameters (`_batting_card_model`, etc.) through to `apply_tier_boost`. This means tests of `ensure_variant_cards` that want to stub `apply_tier_boost` must mock at the module level rather than via DI injection. Given that `apply_tier_boost` has 7 injectable params, passing them all through would be verbose. The current approach (test via `apply_tier_boost` mock) is acceptable, but worth noting if deep unit tests of the service layer are planned. - `evaluate_and_boost` accepts `_stats_model` for DI but `ensure_variant_cards` (called by `evaluate_and_boost`) does not accept or forward a `_card_model` override. Tests that inject a fake `BattingCard`/`PitchingCard` for the idempotency check will need to use module-level patching. Minor testability gap, not blocking. ### Verdict: COMMENT The service extraction is clean. All behavior from the router is faithfully reproduced in the service layer. The `animated_url` regression concern from the prior review is resolved — the PR branch incorporates PR #208's logic correctly. The `mergeable: false` flag indicates a rebase is needed before merge (branch was cut before several PRs landed on main), but the code is correct. No blocking issues. --- *Automated review by Claude PR Reviewer*
Claude removed the
ai-reviewing
 label 2026-04-10 17:18:25 +00:00
Claude added the
ai-reviewing
 label 2026-04-10 17:45:52 +00:00
Claude removed the
ai-reviewing
 label 2026-04-10 17:49:36 +00:00
Claude added the
ai-reviewing
 label 2026-04-10 18:15:51 +00:00
Claude reviewed 2026-04-10 18:18:52 +00:00
Claude left a comment
Author
Collaborator
Copy Link

AI Code Review

Files Reviewed

  • app/services/refractor_service.py (added, 194 lines)
  • app/routers_v2/refractor.py (modified, −62 lines net)

Findings

Correctness

No issues found. Full trace of all behaviour-critical paths:

  • Boost-disabled path (REFRACTOR_BOOST_ENABLED=false): ensure_variant_cards returns early with current_tier = state.current_tier. evaluate_and_boost returns the same stored value → new_tier == old_tier in router → no tier-up notification sent. ✅
  • No card_type path (state.track is None): ensure_variant_cards logs warning and returns early with current_tier = state.current_tier → same result as above. ✅
  • Partial failure (tier N fails after tiers 1..N−1 succeed): loop breaks, last_successful_tier reflects the last committed tier. Higher tiers not attempted. ✅
  • Idempotent skip: compute_variant_hash pre-checks existence before apply_tier_boost. Already-existing variants are skipped cleanly. ✅
  • Tier regression guard: target_tier = max(computed_tier, stored_tier) — if formula says tier 1 but DB has tier 3, backfill creates any missing variants up to tier 3 without regressing. ✅
  • variants_created[-1]: returns the hash of the last successfully created tier — equivalent to old boost_result.get("variant_created"). ✅
  • animated_url: generated when new_tier >= 3 and variant_num and card_type — matches PR #208 contract. ✅
  • import os preserved: still used for API_BASE_URL env lookup; PR body says "removes unused import os" but the diff correctly keeps it. Code is right, PR body is slightly inaccurate.

Security

No issues. No new user-controlled data flows, no SQL injection vectors, no secrets introduced.

Style & Conventions

  • CardModel.player == player_id in refractor_service.py (line ~108 in the new file): rest of the router uses CardModel.player_id for FK equality checks (e.g. _build_card_state_response line 90, list_card_states line 259). Functionally equivalent in Peewee (FK fields support integer equality on both forms) — non-blocking, but inconsistent with file convention.
  • Top-level imports in refractor_service.py follow CLAUDE.md (no lazy imports mid-file). ✅
  • Function-local imports in refractor.py router functions match the existing pattern throughout that file. ✅

Suggestions

  • No tests added for refractor_service.py — consistent with codebase pattern of follow-up test PRs, but ensure_variant_cards and evaluate_and_boost are now the critical path for all tier-up writes; a test PR covering the partial-failure and idempotent cases would be high value.

Merge Blocker (not a code issue)

mergeable: false — merge conflict with main. The conflict is in the refractor.py import block: this branch predates PR #208 which added from datetime import date to the same file. A rebase onto current main resolves it with no code changes required.

Verdict: COMMENT

Service extraction is clean. Logic is correct. No correctness, security, or convention issues in the code itself. Ready to merge after rebase.

Automated review by Claude PR Reviewer

## AI Code Review ### Files Reviewed - `app/services/refractor_service.py` (added, 194 lines) - `app/routers_v2/refractor.py` (modified, −62 lines net) ### Findings #### Correctness No issues found. Full trace of all behaviour-critical paths: - **Boost-disabled path** (`REFRACTOR_BOOST_ENABLED=false`): `ensure_variant_cards` returns early with `current_tier = state.current_tier`. `evaluate_and_boost` returns the same stored value → `new_tier == old_tier` in router → no tier-up notification sent. ✅ - **No `card_type` path** (`state.track` is None): `ensure_variant_cards` logs warning and returns early with `current_tier = state.current_tier` → same result as above. ✅ - **Partial failure** (tier N fails after tiers 1..N−1 succeed): loop breaks, `last_successful_tier` reflects the last committed tier. Higher tiers not attempted. ✅ - **Idempotent skip**: `compute_variant_hash` pre-checks existence before `apply_tier_boost`. Already-existing variants are skipped cleanly. ✅ - **Tier regression guard**: `target_tier = max(computed_tier, stored_tier)` — if formula says tier 1 but DB has tier 3, backfill creates any missing variants up to tier 3 without regressing. ✅ - **`variants_created[-1]`**: returns the hash of the last successfully created tier — equivalent to old `boost_result.get("variant_created")`. ✅ - **`animated_url`**: generated when `new_tier >= 3 and variant_num and card_type` — matches PR #208 contract. ✅ - **`import os` preserved**: still used for `API_BASE_URL` env lookup; PR body says "removes unused `import os`" but the diff correctly keeps it. Code is right, PR body is slightly inaccurate. #### Security No issues. No new user-controlled data flows, no SQL injection vectors, no secrets introduced. #### Style & Conventions - **`CardModel.player == player_id`** in `refractor_service.py` (line ~108 in the new file): rest of the router uses `CardModel.player_id` for FK equality checks (e.g. `_build_card_state_response` line 90, `list_card_states` line 259). Functionally equivalent in Peewee (FK fields support integer equality on both forms) — non-blocking, but inconsistent with file convention. - Top-level imports in `refractor_service.py` follow CLAUDE.md (no lazy imports mid-file). ✅ - Function-local imports in `refractor.py` router functions match the existing pattern throughout that file. ✅ #### Suggestions - No tests added for `refractor_service.py` — consistent with codebase pattern of follow-up test PRs, but `ensure_variant_cards` and `evaluate_and_boost` are now the critical path for all tier-up writes; a test PR covering the partial-failure and idempotent cases would be high value. ### Merge Blocker (not a code issue) **`mergeable: false`** — merge conflict with `main`. The conflict is in the `refractor.py` import block: this branch predates PR #208 which added `from datetime import date` to the same file. A rebase onto current `main` resolves it with no code changes required. ### Verdict: COMMENT Service extraction is clean. Logic is correct. No correctness, security, or convention issues in the code itself. Ready to merge after rebase. --- *Automated review by Claude PR Reviewer*
Claude added
ai-reviewed
 and removed
ai-reviewing
 labels 2026-04-10 18:19:23 +00:00
This pull request has changes conflicting with the target branch.
  • app/routers_v2/refractor.py
View command line instructions.

Checkout

From your project repository, check out a new branch and test the changes.
git fetch -u origin issue/202-refactor-extract-evaluate-game-business-logic-from:issue/202-refactor-extract-evaluate-game-business-logic-from
git checkout issue/202-refactor-extract-evaluate-game-business-logic-from
Sign in to join this conversation.
Reviewers
No reviewers
Labels
Clear labels   
ai-changes-requested

   
ai-failed

   
ai-merged

PR merged by AI ops agent

  
ai-pr-opened

   
ai-reviewed

   
ai-reviewing

   
ai-working

   
autonomous

PR created by the autonomous improvement pipeline

  
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

   
size:M

Medium PR (100-500 lines)

  
size:S

Small PR (< 100 lines)

  
tech-debt

   
todo

   
type:feature

Feature work

  
type:stability

Stability fix (tests, dead code, bugs)

No Label
ai-changes-requested
ai-failed
ai-merged
ai-pr-opened
ai-reviewed
ai-reviewing
ai-working
autonomous
bug
enhancement
evolution
performance
phase-0
phase-1a
phase-1b
phase-1c
phase-1d
security
size:M
size:S
tech-debt
todo
type:feature
type:stability
Milestone
Clear milestone
No items
No Milestone
Projects
Clear projects
No project
Assignees
Clear assignees
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#207
No description provided.
Delete Branch "issue/202-refactor-extract-evaluate-game-business-logic-from"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?