cal/paper-dynasty-discord
1
0
Fork 0
Code Issues 13 Pull Requests 8 Actions Packages Projects Releases Wiki Activity

feat: add /dev refractor-test integration test command #161

Merged
cal merged 10 commits from feat/refractor-integration-test into main 2026-04-10 02:23:22 +00:00
Conversation 0 Commits 10 Files Changed 6 +1185 -1
cal commented 2026-04-09 13:01:48 +00:00
Owner
Copy Link

Summary

  • Add /dev refractor-test card_id:<int> slash command that validates the full refractor chain end-to-end using synthetic game data (create game → insert plays → update season stats → evaluate refractor → render card)
  • Pure helper functions in helpers/refractor_test_data.py for stat calculations and synthetic data building (HR plays for batters, K plays for pitchers)
  • CleanupView with buttons to clean up or keep test data after each run
  • DevToolsCog conditionally loaded only when DATABASE != prod
  • 25 new tests (15 for helpers, 10 for cog)

Test plan

  • All 25 new tests pass locally
  • Ruff lint clean on all commits
  • Deploy to dev bot and run /dev refractor-test on a T0 batter card
  • Verify tier-up detection and card render
  • Test cleanup button functionality
  • Test on a pitcher card (SP/RP)

🤖 Generated with Claude Code

## Summary - Add `/dev refractor-test card_id:<int>` slash command that validates the full refractor chain end-to-end using synthetic game data (create game → insert plays → update season stats → evaluate refractor → render card) - Pure helper functions in `helpers/refractor_test_data.py` for stat calculations and synthetic data building (HR plays for batters, K plays for pitchers) - `CleanupView` with buttons to clean up or keep test data after each run - `DevToolsCog` conditionally loaded only when `DATABASE != prod` - 25 new tests (15 for helpers, 10 for cog) ## Test plan - [x] All 25 new tests pass locally - [x] Ruff lint clean on all commits - [ ] Deploy to dev bot and run `/dev refractor-test` on a T0 batter card - [ ] Verify tier-up detection and card render - [ ] Test cleanup button functionality - [ ] Test on a pitcher card (SP/RP) 🤖 Generated with [Claude Code](https://claude.com/claude-code)
cal added 8 commits 2026-04-09 13:01:49 +00:00 
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
fix: address code review — use PD_SEASON, top-level imports, fix kwargs typing
All checks were successful
Ruff Lint / lint (pull_request) Successful in 22s
Details
5b43f8dad3 
- Replace hardcoded CURRENT_SEASON = 11 with PD_SEASON from helpers.constants
- Move get_team_by_owner import to top-level (no circular dependency)
- Replace kwargs dict unpacking with explicit keyword args (fixes Pyright)
- Remove unused os import
- Add comment documenting on_timeout limitation

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Claude added the
ai-reviewing
 label 2026-04-09 13:15:51 +00:00
Claude reviewed 2026-04-09 13:18:25 +00:00
Claude left a comment
Collaborator
Copy Link

AI Code Review

Files Reviewed

  • cogs/dev_tools.py (added)
  • helpers/refractor_test_data.py (added)
  • tests/test_dev_tools.py (added)
  • tests/test_refractor_test_data.py (added)
  • paperdynasty.py (modified)
  • .gitignore (modified)

Findings

Correctness

No issues found.

  • db_delete signature matches all three cleanup calls (endpoint: str, object_id: int). Deletion order (decisions → plays → games) is correct for FK constraints.
  • calculate_plays_needed(gap=0): max(1, ceil(0/9)) = 1 — correctly generates one play to push past an at-threshold card.
  • tier_ups variable is always defined when Step 6 is reached (early returns on exception prevent scope gaps).
  • update_embed field-manipulation logic is correct: keeps field[0] (Setup), replaces everything else with Results. The embed.set_image() call in Step 6 is not cleared by update_embed (images are stored separately from fields), so the card image survives to the final Discord message.
  • "prod" not in os.getenv("DATABASE", "dev").lower() correctly gates dev-only loading; production DATABASE=Prod fails the check.
  • Batter value formula: PA + TB*2 = 1 + 4*2 = 9 ✓. Pitcher value: IP + K = 1/3 + 1 = 4/3 ✓.
  • Test mock side_effect counts match actual db_get call counts in both test_batting_card_lookup (3 calls) and test_pitching_card_fallback (4 calls).

Security

No issues found. card_id is typed as int by the slash command framework — no injection risk. No secrets or credentials introduced.

Style & Conventions

  • Dead parameter — final: bool = False in update_embed: The parameter is accepted in the closure signature but never read inside the function body. The meaningful distinction is already view is not None. Remove final to avoid confusion about what it controls.
  • Dead parameter — player_name in _execute_refractor_test: Received but never referenced inside the method. It's used in the calling refractor_test method to build the Setup embed, but passing it to _execute_refractor_test serves no purpose.

Suggestions

  • Refractor state lookup limit: params=[("team_id", team_id), ("limit", 100)] — if a team has >100 refractor entries and the target player falls beyond position 100, card_state will be None and the code assumes T0/0 progress. This could cause the tool to generate far more plays than necessary for a high-progress card. Not a blocker for a dev tool, but worth a comment noting the limitation.
  • Test assertion style in test_cleanup_calls_delete_endpoints: assert "decisions/game" in str(calls[0]) is fragile (matches substrings in call repr). Consider calls[0].args[0] == "decisions/game" for precision. Consistent with prior PR patterns in this repo, so not a blocker.

Verdict: COMMENT

Functionally correct. The test coverage is thorough (25 tests across pure helpers and cog behavior), the prod-guard pattern is clean, and the step-by-step reporting with per-step error stops is solid. Only minor dead code in two parameter lists and an optional note on the 100-item lookup limit.

Automated review by Claude PR Reviewer

## AI Code Review ### Files Reviewed - `cogs/dev_tools.py` (added) - `helpers/refractor_test_data.py` (added) - `tests/test_dev_tools.py` (added) - `tests/test_refractor_test_data.py` (added) - `paperdynasty.py` (modified) - `.gitignore` (modified) ### Findings #### Correctness No issues found. - `db_delete` signature matches all three cleanup calls (`endpoint: str, object_id: int`). Deletion order (decisions → plays → games) is correct for FK constraints. - `calculate_plays_needed(gap=0)`: `max(1, ceil(0/9)) = 1` — correctly generates one play to push past an at-threshold card. - `tier_ups` variable is always defined when Step 6 is reached (early returns on exception prevent scope gaps). - `update_embed` field-manipulation logic is correct: keeps field[0] (Setup), replaces everything else with Results. The `embed.set_image()` call in Step 6 is not cleared by `update_embed` (images are stored separately from fields), so the card image survives to the final Discord message. - `"prod" not in os.getenv("DATABASE", "dev").lower()` correctly gates dev-only loading; production `DATABASE=Prod` fails the check. - Batter value formula: `PA + TB*2 = 1 + 4*2 = 9` ✓. Pitcher value: `IP + K = 1/3 + 1 = 4/3` ✓. - Test mock `side_effect` counts match actual `db_get` call counts in both `test_batting_card_lookup` (3 calls) and `test_pitching_card_fallback` (4 calls). #### Security No issues found. `card_id` is typed as `int` by the slash command framework — no injection risk. No secrets or credentials introduced. #### Style & Conventions - **Dead parameter — `final: bool = False` in `update_embed`**: The parameter is accepted in the closure signature but never read inside the function body. The meaningful distinction is already `view is not None`. Remove `final` to avoid confusion about what it controls. - **Dead parameter — `player_name` in `_execute_refractor_test`**: Received but never referenced inside the method. It's used in the calling `refractor_test` method to build the Setup embed, but passing it to `_execute_refractor_test` serves no purpose. #### Suggestions - **Refractor state lookup limit**: `params=[("team_id", team_id), ("limit", 100)]` — if a team has >100 refractor entries and the target player falls beyond position 100, `card_state` will be `None` and the code assumes T0/0 progress. This could cause the tool to generate far more plays than necessary for a high-progress card. Not a blocker for a dev tool, but worth a comment noting the limitation. - **Test assertion style in `test_cleanup_calls_delete_endpoints`**: `assert "decisions/game" in str(calls[0])` is fragile (matches substrings in call repr). Consider `calls[0].args[0] == "decisions/game"` for precision. Consistent with prior PR patterns in this repo, so not a blocker. ### Verdict: COMMENT Functionally correct. The test coverage is thorough (25 tests across pure helpers and cog behavior), the prod-guard pattern is clean, and the step-by-step reporting with per-step error stops is solid. Only minor dead code in two parameter lists and an optional note on the 100-item lookup limit. --- *Automated review by Claude PR Reviewer*
Claude added
ai-reviewed
 and removed
ai-reviewing
 labels 2026-04-09 13:18:58 +00:00
cal added 1 commit 2026-04-09 13:34:10 +00:00
fix: remove dead parameters from PR review feedback
All checks were successful
Ruff Lint / lint (pull_request) Successful in 13s
Details
f3a83f91fd 
Remove unused `player_name` param from `_execute_refractor_test` and
unused `final` param from `update_embed` closure.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
cal reviewed 2026-04-10 02:22:11 +00:00
cal left a comment
Author
Owner
Copy Link

AI Code Review

Files Reviewed

  • cogs/dev_tools.py (added)
  • helpers/refractor_test_data.py (added)
  • tests/test_dev_tools.py (added)
  • tests/test_refractor_test_data.py (added)
  • paperdynasty.py (modified)
  • .gitignore (modified)

Findings

Correctness

No issues found.

Key checks passed:

  • The "prod" not in os.getenv("DATABASE", "dev").lower() guard correctly keeps the cog off in production for any DATABASE value containing "prod" (case-insensitive). The env var docs in CLAUDE.md confirm it is set to Dev or Prod.
  • tier_ups variable scope is safe: Step 5's except branch always returns immediately, so tier_ups is never referenced before assignment.
  • Cleanup deletion order (decisions → plays → games) is FK-correct.
  • calculate_plays_needed(gap=0) returns max(1, ceil(0 / value)) = max(1, 0) = 1, which correctly handles a card already sitting exactly at its threshold.
  • The two-step batting/pitching card lookup sets card_type_key correctly before both calls, so the fallback path carries the right type through.
  • db_delete takes (endpoint: str, object_id: int) — the three cleanup calls match this signature exactly.
  • db_get defaults none_okay=True, so the card render call with explicit none_okay=True is redundant but harmless.
  • get_team_by_owner accepts int and the code passes interaction.user.id (a Discord snowflake int) — correct.

Security

No issues found. This cog is intentionally dev-only and is never loaded when DATABASE contains "prod". No user-supplied input is interpolated into any API path — card_id is typed int by Discord's slash command framework, and all constructed endpoint paths use internal IDs fetched from the API, not raw user strings. The button ownership check (interaction.user.id != self.owner_id) correctly gates cleanup to the command invoker.

Style & Conventions

No issues found. Code follows the project's cog patterns: top-level imports only, no lazy imports, docstrings on all public functions and classes, consistent use of db_get/db_post/db_delete helpers.

Minor observation (not a blocker): the CURRENT_SEASON = PD_SEASON module-level alias in dev_tools.py adds an indirection layer with no benefit — PD_SEASON could be used directly. Purely stylistic.

Suggestions

  • The on_timeout handler clears items locally but cannot update the Discord message without a stored message reference — this is already documented in the code comment. For a dev tool the current behavior (buttons become unresponsive after 5 min) is acceptable as-is.
  • refractor/cards is fetched with limit=100. If a team has more than 100 refractor-tracked cards the lookup could miss the target player. Unlikely for a dev tool, but limit=200 or a player-ID filter would be more robust.

Verdict: APPROVED

Clean, well-structured implementation. The helper module is pure and fully unit tested (15 tests covering all branches including the zero-gap edge case). The cog is correctly gated to non-prod environments. The step-by-step failure/stop chain is sound. Button auth check is correct. The stale player_name/final parameters flagged in the previous review have been removed. Ready to merge.

Automated review by Claude PR Reviewer

## AI Code Review ### Files Reviewed - `cogs/dev_tools.py` (added) - `helpers/refractor_test_data.py` (added) - `tests/test_dev_tools.py` (added) - `tests/test_refractor_test_data.py` (added) - `paperdynasty.py` (modified) - `.gitignore` (modified) ### Findings #### Correctness No issues found. Key checks passed: - The `"prod" not in os.getenv("DATABASE", "dev").lower()` guard correctly keeps the cog off in production for any `DATABASE` value containing "prod" (case-insensitive). The env var docs in CLAUDE.md confirm it is set to `Dev` or `Prod`. - `tier_ups` variable scope is safe: Step 5's `except` branch always `return`s immediately, so `tier_ups` is never referenced before assignment. - Cleanup deletion order (decisions → plays → games) is FK-correct. - `calculate_plays_needed(gap=0)` returns `max(1, ceil(0 / value))` = `max(1, 0)` = 1, which correctly handles a card already sitting exactly at its threshold. - The two-step batting/pitching card lookup sets `card_type_key` correctly before both calls, so the fallback path carries the right type through. - `db_delete` takes `(endpoint: str, object_id: int)` — the three cleanup calls match this signature exactly. - `db_get` defaults `none_okay=True`, so the card render call with explicit `none_okay=True` is redundant but harmless. - `get_team_by_owner` accepts `int` and the code passes `interaction.user.id` (a Discord snowflake int) — correct. #### Security No issues found. This cog is intentionally dev-only and is never loaded when `DATABASE` contains "prod". No user-supplied input is interpolated into any API path — `card_id` is typed `int` by Discord's slash command framework, and all constructed endpoint paths use internal IDs fetched from the API, not raw user strings. The button ownership check (`interaction.user.id != self.owner_id`) correctly gates cleanup to the command invoker. #### Style & Conventions No issues found. Code follows the project's cog patterns: top-level imports only, no lazy imports, docstrings on all public functions and classes, consistent use of `db_get`/`db_post`/`db_delete` helpers. Minor observation (not a blocker): the `CURRENT_SEASON = PD_SEASON` module-level alias in `dev_tools.py` adds an indirection layer with no benefit — `PD_SEASON` could be used directly. Purely stylistic. #### Suggestions - The `on_timeout` handler clears items locally but cannot update the Discord message without a stored message reference — this is already documented in the code comment. For a dev tool the current behavior (buttons become unresponsive after 5 min) is acceptable as-is. - `refractor/cards` is fetched with `limit=100`. If a team has more than 100 refractor-tracked cards the lookup could miss the target player. Unlikely for a dev tool, but `limit=200` or a player-ID filter would be more robust. ### Verdict: APPROVED Clean, well-structured implementation. The helper module is pure and fully unit tested (15 tests covering all branches including the zero-gap edge case). The cog is correctly gated to non-prod environments. The step-by-step failure/stop chain is sound. Button auth check is correct. The stale `player_name`/`final` parameters flagged in the previous review have been removed. Ready to merge. --- *Automated review by Claude PR Reviewer*
Claude approved these changes 2026-04-10 02:23:08 +00:00
Claude left a comment
Collaborator
Copy Link

Approved via pd-pr

Approved via pd-pr
cal added 1 commit 2026-04-10 02:23:16 +00:00
Merge branch 'main' into feat/refractor-integration-test
All checks were successful
Ruff Lint / lint (pull_request) Successful in 31s
Details
d123437f0e
cal merged commit 662c1c5448 into main 2026-04-10 02:23:22 +00:00
cal deleted branch feat/refractor-integration-test 2026-04-10 02:23:22 +00:00
Sign in to join this conversation.
Reviewers
No reviewers
Claude
Labels
Clear labels   
ai-changes-requested

   
ai-failed

   
ai-pr-opened

   
ai-reviewed

   
ai-reviewing

   
ai-working

   
autonomous

PR created by the autonomous improvement pipeline

  
bug

   
enhancement

   
feature

New feature proposal

  
in-queue

Approved for development

  
performance

   
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-pr-opened
ai-reviewed
ai-reviewing
ai-working
autonomous
bug
enhancement
feature
in-queue
performance
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-discord#161
No description provided.
Delete Branch "feat/refractor-integration-test"

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?