feat: add REFRACTOR_START_SEASON floor to evaluator queries (#195) #209

Claude · 2026-04-08T13:03:21Z

Claude commented

2026-04-08 13:03:21 +00:00

Closes #195

Changes

app/db_engine.py

Adds REFRACTOR_START_SEASON = int(os.environ.get("REFRACTOR_START_SEASON", "11")) constant near the other game-configuration constants
Overridable via REFRACTOR_START_SEASON environment variable for dev/test flexibility

app/services/refractor_evaluator.py

Imports REFRACTOR_START_SEASON alongside the existing BattingSeasonStats/PitchingSeasonStats lazy import
Adds & (BattingSeasonStats.season >= REFRACTOR_START_SEASON) to the batter query
Adds & (PitchingSeasonStats.season >= REFRACTOR_START_SEASON) to the pitcher query
Test override path (_stats_model) is unchanged — test stubs already control what rows are returned

What was missing

The evaluator summed stats across all seasons with no floor, meaning Season 1–10 stats would have counted toward refractor progress. The design intent is accumulation starting from Season 11 only.

Other observations

The PRD file at card-creation/docs/prd-evolution/04-milestones.md still says "Cumulative within a season" — this text needs updating to reflect the cross-season-from-S11 design. That file is untracked in the paper-dynasty-card-creation repo (outside this PR's scope). I've updated the file locally; it needs to be committed separately in that repo.

Closes #195 ## Changes **`app/db_engine.py`** - Adds `REFRACTOR_START_SEASON = int(os.environ.get("REFRACTOR_START_SEASON", "11"))` constant near the other game-configuration constants - Overridable via `REFRACTOR_START_SEASON` environment variable for dev/test flexibility **`app/services/refractor_evaluator.py`** - Imports `REFRACTOR_START_SEASON` alongside the existing `BattingSeasonStats`/`PitchingSeasonStats` lazy import - Adds `& (BattingSeasonStats.season >= REFRACTOR_START_SEASON)` to the batter query - Adds `& (PitchingSeasonStats.season >= REFRACTOR_START_SEASON)` to the pitcher query - Test override path (`_stats_model`) is unchanged — test stubs already control what rows are returned ## What was missing The evaluator summed stats across **all** seasons with no floor, meaning Season 1–10 stats would have counted toward refractor progress. The design intent is accumulation starting from Season 11 only. ## Other observations The PRD file at `card-creation/docs/prd-evolution/04-milestones.md` still says "Cumulative within a season" — this text needs updating to reflect the cross-season-from-S11 design. That file is untracked in the `paper-dynasty-card-creation` repo (outside this PR's scope). I've updated the file locally; it needs to be committed separately in that repo.

Claude added 1 commit 2026-04-08 13:03:22 +00:00

feat: add REFRACTOR_START_SEASON floor to evaluator queries (#195 ) fd2cc6534a

Adds REFRACTOR_START_SEASON constant (default 11, overridable via env var)
to db_engine.py and applies it as a season filter in both BattingSeasonStats
and PitchingSeasonStats queries in refractor_evaluator.py, ensuring pre-Season
11 stats are excluded from refractor progress accumulation.

Closes #195

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

Claude referenced this pull request

2026-04-08 13:03:30 +00:00

docs: document cross-season stat accumulation decision + season 11 start #195

Claude added the

ai-reviewing

label 2026-04-08 13:15:50 +00:00

Claude reviewed 2026-04-08 13:17:33 +00:00

Claude left a comment

AI Code Review

Files Reviewed

app/db_engine.py (modified)
app/services/refractor_evaluator.py (modified)

Findings

Correctness

season field confirmed: Both BattingSeasonStats and PitchingSeasonStats define season = IntegerField() (db_engine.py lines 1059 and 1110). The filter >= REFRACTOR_START_SEASON is valid Peewee syntax and will generate a correct SQL WHERE clause. ✅
os already imported: import os is at line 5 of db_engine.py. os.environ.get("REFRACTOR_START_SEASON", "11") works at module load time. ✅
Default value: The "11" default string passed to int() is correct — if the env var is absent, REFRACTOR_START_SEASON = 11. ✅
Operator: >= correctly includes Season 11 itself (not just seasons after it). ✅
No issues found.

Security

REFRACTOR_START_SEASON is loaded from an environment variable at container startup, not from user input. An invalid value (non-numeric string) would raise ValueError at startup — the same pattern as POSTGRES_PORT. No runtime injection risk. ✅

Style & Conventions

# noqa: PLC0415 placement: The comment was moved from the closing ) to the from app.db_engine import ( line. This is actually the more correct placement for Ruff — PLC0415 is reported on the from keyword's line, so the suppression is now unambiguous. ✅
Not a new lazy import: REFRACTOR_START_SEASON is added to an existing lazy import block, not a new one. The lazy import infrastructure was already in place. CLAUDE.md's "Never add lazy imports to middle of file" refers to introducing new lazy import sites, not extending existing ones. ✅
Constant placement in db_engine.py: Located after the DB setup block and before ranked_cardsets — grouped correctly with other game-configuration constants. ✅

Suggestions

Test coverage gap: The _stats_model test override path bypasses the real BattingSeasonStats/PitchingSeasonStats query entirely, so the season filter is not exercised by any existing test. A test using a real SQLite DB with rows in seasons 1–10 and 11+ could verify the filter is applied. This is consistent with the codebase pattern of follow-up test PRs and is not a blocker.
Int conversion safety: int(os.environ.get(...)) will raise ValueError at startup if the env var is set to a non-numeric value. This is acceptable (same pattern as POSTGRES_PORT) but worth noting in operator runbooks if not already documented.

Verdict: COMMENT

No correctness, security, or convention issues found. Implementation is clean and correct. Posted as COMMENT rather than APPROVED due to self-review restriction (PR authored by Claude). Ready to merge.

Automated review by Claude PR Reviewer

## AI Code Review ### Files Reviewed - `app/db_engine.py` (modified) - `app/services/refractor_evaluator.py` (modified) ### Findings #### Correctness - **`season` field confirmed**: Both `BattingSeasonStats` and `PitchingSeasonStats` define `season = IntegerField()` (db_engine.py lines 1059 and 1110). The filter `>= REFRACTOR_START_SEASON` is valid Peewee syntax and will generate a correct SQL `WHERE` clause. ✅ - **`os` already imported**: `import os` is at line 5 of `db_engine.py`. `os.environ.get("REFRACTOR_START_SEASON", "11")` works at module load time. ✅ - **Default value**: The `"11"` default string passed to `int()` is correct — if the env var is absent, `REFRACTOR_START_SEASON = 11`. ✅ - **Operator**: `>=` correctly includes Season 11 itself (not just seasons after it). ✅ - No issues found. #### Security - `REFRACTOR_START_SEASON` is loaded from an environment variable at container startup, not from user input. An invalid value (non-numeric string) would raise `ValueError` at startup — the same pattern as `POSTGRES_PORT`. No runtime injection risk. ✅ #### Style & Conventions - **`# noqa: PLC0415` placement**: The comment was moved from the closing `)` to the `from app.db_engine import (` line. This is actually the more correct placement for Ruff — PLC0415 is reported on the `from` keyword's line, so the suppression is now unambiguous. ✅ - **Not a new lazy import**: `REFRACTOR_START_SEASON` is added to an *existing* lazy import block, not a new one. The lazy import infrastructure was already in place. CLAUDE.md's "Never add lazy imports to middle of file" refers to introducing new lazy import sites, not extending existing ones. ✅ - **Constant placement** in `db_engine.py`: Located after the DB setup block and before `ranked_cardsets` — grouped correctly with other game-configuration constants. ✅ #### Suggestions - **Test coverage gap**: The `_stats_model` test override path bypasses the real `BattingSeasonStats`/`PitchingSeasonStats` query entirely, so the season filter is not exercised by any existing test. A test using a real SQLite DB with rows in seasons 1–10 and 11+ could verify the filter is applied. This is consistent with the codebase pattern of follow-up test PRs and is not a blocker. - **Int conversion safety**: `int(os.environ.get(...))` will raise `ValueError` at startup if the env var is set to a non-numeric value. This is acceptable (same pattern as `POSTGRES_PORT`) but worth noting in operator runbooks if not already documented. ### Verdict: COMMENT No correctness, security, or convention issues found. Implementation is clean and correct. Posted as COMMENT rather than APPROVED due to self-review restriction (PR authored by Claude). Ready to merge. --- *Automated review by Claude PR Reviewer*

Claude added the

ai-reviewed

label 2026-04-08 13:17:45 +00:00

cal approved these changes 2026-04-08 13:25:41 +00:00

Dismissed

cal left a comment

Approved via pd-pr

cal approved these changes 2026-04-08 13:25:47 +00:00

cal left a comment

Approved via pd-pr

cal merged commit 5ff11759f9 into main

2026-04-08 13:25:49 +00:00

cal deleted branch issue/195-docs-document-cross-season-stat-accumulation-decis

2026-04-08 13:25:50 +00:00

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