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

feat: /refractor status slash command (WP-11) #87

Merged
cal merged 5 commits from ai/paper-dynasty-database#76 into main 2026-03-23 20:11:33 +00:00
Conversation 2 Commits 5 Files Changed 3 +681 -1
Claude commented 2026-03-13 14:07:50 +00:00
Collaborator
Copy Link

Closes cal/paper-dynasty-database#76

Summary

Implements the /refractor status slash command (WP-11) for the Refractor card progression system.

  • cogs/refractor.py — new Refractor cog with /refractor status group command
  • tests/test_refractor_commands.py — 49 unit tests (all passing)
  • paperdynasty.py — registers cogs.refractor in the COGS list

Slash Command

/refractor status [type] [season] [tier] [progress] [page]

Filters: type (batter|sp|rp), season, tier (0-4), progress ("close" = within 80% of next tier), page (default 1, 10 per page).

Display format:

**[BC] Mike Trout** (Base Chrome)
[========--] 120/149 (PA+TB×2) — T1 → T2

Fully evolved cards show FULLY EVOLVED ★.

API Dependency

Calls GET /refractor/cards?team_id=X[&card_type=X][&season=X][&tier=X]. This endpoint is implemented by WP-07 — the command will return "No refractor data found" gracefully until WP-07 is deployed.

Tier Names and Badges

Tier Name Badge
T0 Base Card (none)
T1 Base Chrome [BC]
T2 Refractor [R]
T3 Gold Refractor [GR]
T4 Superfractor [SF]

Test Results

49 passed, 1 warning in 0.77s

Covers: progress bar rendering, entry formatting, badge label presence (T1-T4), close filter (80% threshold), pagination, tier display names, empty roster, no-team responses.

Other observations

  • paperdynasty.py had an unused import datetime (pre-existing); removed it to pass the ruff pre-commit hook — this is the only out-of-scope change.
  • utilities/evolution_notifications.py has no source file on this branch; WP-14 (PR #112) delivers the replacement (refractor_notifs).
Closes cal/paper-dynasty-database#76 ## Summary Implements the `/refractor status` slash command (WP-11) for the Refractor card progression system. - **`cogs/refractor.py`** — new `Refractor` cog with `/refractor status` group command - **`tests/test_refractor_commands.py`** — 49 unit tests (all passing) - **`paperdynasty.py`** — registers `cogs.refractor` in the COGS list ## Slash Command ``` /refractor status [type] [season] [tier] [progress] [page] ``` Filters: `type` (batter|sp|rp), `season`, `tier` (0-4), `progress` ("close" = within 80% of next tier), `page` (default 1, 10 per page). Display format: ``` **[BC] Mike Trout** (Base Chrome) [========--] 120/149 (PA+TB×2) — T1 → T2 ``` Fully evolved cards show `FULLY EVOLVED ★`. ## API Dependency Calls `GET /refractor/cards?team_id=X[&card_type=X][&season=X][&tier=X]`. This endpoint is implemented by WP-07 — the command will return "No refractor data found" gracefully until WP-07 is deployed. ## Tier Names and Badges | Tier | Name | Badge | |------|------|-------| | T0 | Base Card | (none) | | T1 | Base Chrome | [BC] | | T2 | Refractor | [R] | | T3 | Gold Refractor | [GR] | | T4 | Superfractor | [SF] | ## Test Results ``` 49 passed, 1 warning in 0.77s ``` Covers: progress bar rendering, entry formatting, badge label presence (T1-T4), close filter (80% threshold), pagination, tier display names, empty roster, no-team responses. ## Other observations - `paperdynasty.py` had an unused `import datetime` (pre-existing); removed it to pass the ruff pre-commit hook — this is the only out-of-scope change. - `utilities/evolution_notifications.py` has no source file on this branch; WP-14 (PR #112) delivers the replacement (`refractor_notifs`).
Claude added 1 commit 2026-03-13 14:07:51 +00:00
feat: /evo status slash command and tests (WP-11) (#76)
All checks were successful
Build Docker Image / build (pull_request) Successful in 1m37s
Details
d12cdb8d97 
Closes #76

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Claude added the
ai-reviewing
 label 2026-03-13 14:15:50 +00:00
Claude reviewed 2026-03-13 14:17:38 +00:00
Claude left a comment
Author
Collaborator
Copy Link

AI Code Review

Files Reviewed

  • cogs/evolution.py (added)
  • tests/test_evolution_commands.py (added)
  • paperdynasty.py (modified)

Findings

Correctness

  • apply_close_filter falsy check on next_threshold (cogs/evolution.py line 95): The condition not next_threshold will incorrectly exclude a card with next_threshold=0. A threshold of zero would be pathological API data, but next_threshold is None is more precise and matches the docstring's stated intent. The same comparison in format_evo_entry (line 75) uses is None correctly — this inconsistency is worth resolving for defensive correctness.
  • Silent progress parameter passthrough: Passing any value other than "close" (e.g., progress="ahead") silently returns all results with no filtering and no user feedback. This is not a crash risk, but unexpected values are invisible to the user. Low priority — there's no Discord autocomplete yet, so users can't easily mis-use it.
  • type parameter name shadows Python built-in: Line 136 uses type as a parameter name. This is legal and causes no runtime issue in this context, but it does suppress type() usage within the method scope. Cosmetic — consistent with how Discord.py apps commonly shadow builtins in command signatures.
  • All other logic is correct: render_progress_bar clamps properly, paginate handles empty lists and out-of-range pages, format_evo_entry handles both the T4-by-tier and T4-by-None cases, the dual-shape db_get response (list vs {"cards": [...]}) is handled.

Security

  • No issues found. User input (type, season, tier, progress) is passed to db_get via the params tuple list, which is URL-encoded by aiohttp — no injection risk. No secrets or credentials. The command is ephemeral, so output is private to the invoking user.

Style & Conventions

  • Module-level docstring, logger = logging.getLogger("discord_app"), from helpers import get_team_by_owner, from api_calls import db_get — all match established patterns in the codebase (e.g., cogs/economy_new/scouting.py).
  • app_commands.Group declared as class attribute matches the pattern in cogs/players.py (group_lookup, group_paperdex) and cogs/gameplay.py.
  • await interaction.response.defer(ephemeral=True) followed by interaction.edit_original_response(...) is the correct discord.py interaction pattern, consistent with other newer cogs.
  • No role guard (@app_commands.checks.has_any_role) — consistent with scout_tokens_command in scouting.py, which also gates by team membership rather than Discord role. Acceptable.
  • import datetime removal from paperdynasty.py is correctly scoped as a pre-existing linter issue; noted in PR body.

Test Coverage

  • 39 tests covering all helper functions exhaustively: boundary values, over-threshold clamping, zero-threshold, empty lists, page clamping, all tier name constants.
  • Async command tests mock get_team_by_owner and db_get at the module level (cogs.evolution.*) — correct patching target.
  • test_evo_status_no_team: correctly asserts db_get is never called and response contains "team".
  • test_evo_status_empty_roster: patches db_get to return {"cards": []}, which exercises the data.get("cards", []) branch and the subsequent empty-items check — thorough.
  • The progress="close" path with no matching results and the fully-evolved display are covered by unit tests on apply_close_filter and format_evo_entry respectively.

Suggestions

  • Consider changing not next_threshold to next_threshold is None on cogs/evolution.py line 95 to match the format_evo_entry usage and guard against a next_threshold=0 edge case.
  • The progress parameter could benefit from a Discord app_commands.choices decorator to restrict it to "close" at the slash command level, eliminating the silent-passthrough issue and improving the UX without any behavior change for valid inputs.

Verdict: COMMENT

Clean, well-structured implementation. The module follows all established cog patterns, the helper functions are cleanly separated and thoroughly tested, and the command handles the WP-07 API dependency gracefully. Posting as COMMENT per Gitea self-review policy — no changes are required to merge. The not next_threshold inconsistency is worth a follow-up but does not block.

Automated review by Claude PR Reviewer

## AI Code Review ### Files Reviewed - `cogs/evolution.py` (added) - `tests/test_evolution_commands.py` (added) - `paperdynasty.py` (modified) ### Findings #### Correctness - **`apply_close_filter` falsy check on `next_threshold`** (`cogs/evolution.py` line 95): The condition `not next_threshold` will incorrectly exclude a card with `next_threshold=0`. A threshold of zero would be pathological API data, but `next_threshold is None` is more precise and matches the docstring's stated intent. The same comparison in `format_evo_entry` (line 75) uses `is None` correctly — this inconsistency is worth resolving for defensive correctness. - **Silent `progress` parameter passthrough**: Passing any value other than `"close"` (e.g., `progress="ahead"`) silently returns all results with no filtering and no user feedback. This is not a crash risk, but unexpected values are invisible to the user. Low priority — there's no Discord autocomplete yet, so users can't easily mis-use it. - **`type` parameter name shadows Python built-in**: Line 136 uses `type` as a parameter name. This is legal and causes no runtime issue in this context, but it does suppress `type()` usage within the method scope. Cosmetic — consistent with how Discord.py apps commonly shadow builtins in command signatures. - All other logic is correct: `render_progress_bar` clamps properly, `paginate` handles empty lists and out-of-range pages, `format_evo_entry` handles both the T4-by-tier and T4-by-None cases, the dual-shape `db_get` response (`list` vs `{"cards": [...]}`) is handled. #### Security - No issues found. User input (`type`, `season`, `tier`, `progress`) is passed to `db_get` via the `params` tuple list, which is URL-encoded by `aiohttp` — no injection risk. No secrets or credentials. The command is ephemeral, so output is private to the invoking user. #### Style & Conventions - Module-level docstring, `logger = logging.getLogger("discord_app")`, `from helpers import get_team_by_owner`, `from api_calls import db_get` — all match established patterns in the codebase (e.g., `cogs/economy_new/scouting.py`). - `app_commands.Group` declared as class attribute matches the pattern in `cogs/players.py` (`group_lookup`, `group_paperdex`) and `cogs/gameplay.py`. - `await interaction.response.defer(ephemeral=True)` followed by `interaction.edit_original_response(...)` is the correct discord.py interaction pattern, consistent with other newer cogs. - No role guard (`@app_commands.checks.has_any_role`) — consistent with `scout_tokens_command` in `scouting.py`, which also gates by team membership rather than Discord role. Acceptable. - `import datetime` removal from `paperdynasty.py` is correctly scoped as a pre-existing linter issue; noted in PR body. #### Test Coverage - 39 tests covering all helper functions exhaustively: boundary values, over-threshold clamping, zero-threshold, empty lists, page clamping, all tier name constants. - Async command tests mock `get_team_by_owner` and `db_get` at the module level (`cogs.evolution.*`) — correct patching target. - `test_evo_status_no_team`: correctly asserts `db_get` is never called and response contains "team". - `test_evo_status_empty_roster`: patches `db_get` to return `{"cards": []}`, which exercises the `data.get("cards", [])` branch and the subsequent empty-items check — thorough. - The `progress="close"` path with no matching results and the fully-evolved display are covered by unit tests on `apply_close_filter` and `format_evo_entry` respectively. #### Suggestions - Consider changing `not next_threshold` to `next_threshold is None` on `cogs/evolution.py` line 95 to match the `format_evo_entry` usage and guard against a `next_threshold=0` edge case. - The `progress` parameter could benefit from a Discord `app_commands.choices` decorator to restrict it to `"close"` at the slash command level, eliminating the silent-passthrough issue and improving the UX without any behavior change for valid inputs. ### Verdict: COMMENT Clean, well-structured implementation. The module follows all established cog patterns, the helper functions are cleanly separated and thoroughly tested, and the command handles the WP-07 API dependency gracefully. Posting as COMMENT per Gitea self-review policy — no changes are required to merge. The `not next_threshold` inconsistency is worth a follow-up but does not block. --- *Automated review by Claude PR Reviewer*
cal commented 2026-03-23 04:19:12 +00:00
Owner
Copy Link

Naming decision: the "Evolution" system is being renamed to "Refractor".

The user-facing feature name will be Refractor (inspired by Topps Chrome card hobby terminology).

Tier names:

  1. Base Chrome (tier 0 — default, no progression)
  2. Refractor (tier 1)
  3. Gold Refractor (tier 2)
  4. Superfractor (tier 3/4 — fully maxed)

Slash command: /refractor status (replaces /evo status)

Before merging this PR:

  • Rename the cog from Evolution to Refractor
  • Rename command group from /evo to /refractor
  • Update tier badge prefixes: [REF], [GOLD], [SF] (or similar — to be decided)
  • Update test file accordingly
  • Coordinate with pd-database agent if API endpoint names need updating (currently evolution/cards/{card_id})
**Naming decision: the "Evolution" system is being renamed to "Refractor".** The user-facing feature name will be **Refractor** (inspired by Topps Chrome card hobby terminology). **Tier names:** 1. Base Chrome (tier 0 — default, no progression) 2. Refractor (tier 1) 3. Gold Refractor (tier 2) 4. Superfractor (tier 3/4 — fully maxed) **Slash command:** `/refractor status` (replaces `/evo status`) Before merging this PR: - Rename the cog from `Evolution` to `Refractor` - Rename command group from `/evo` to `/refractor` - Update tier badge prefixes: `[REF]`, `[GOLD]`, `[SF]` (or similar — to be decided) - Update test file accordingly - Coordinate with `pd-database` agent if API endpoint names need updating (currently `evolution/cards/{card_id}`)
cal requested changes 2026-03-23 13:14:25 +00:00
Dismissed
cal left a comment
Owner
Copy Link

AI Code Review

Files Reviewed

  • cogs/evolution.py (added)
  • tests/test_evolution_commands.py (added)
  • paperdynasty.py (modified)

Findings

Correctness

Import path mismatch — from helpers import get_team_by_owner

The new cog (cogs/evolution.py line 17) uses from helpers import get_team_by_owner. The comparable scouting.py cog uses the explicit path from helpers.main import get_team_by_owner. This works at runtime because helpers/__init__.py does from helpers.main import *, but it is inconsistent with the established pattern in this codebase. It should be from helpers.main import get_team_by_owner.

API response shape handling

Lines 155–160 handle two possible shapes from db_get:

  • A plain list
  • A dict with a "cards" key

This dual-shape guard is the correct defensive pattern for a dependency not yet deployed (WP-07). However, if db_get returns None (network error or non-200 with none_okay=True), the not data check at line 149 handles it correctly. No issue here, just confirming the logic is sound.

progress parameter accepts any string value silently

cogs/evolution.py line 147: if progress is any value other than "close", it is silently ignored and the full unfiltered list is shown. This is reasonable for now, but a user typing progress="near" would get no feedback that their filter was ignored. Consider noting in the @app_commands.describe that only "close" is a valid value, or add an explicit check that responds with an error for unrecognized values.

Security

No issues found. No user-controlled data is passed into queries or templates in a dangerous way. The team_id originates from get_team_by_owner (a server-side lookup against the authenticated user), not from direct user input.

Style & Conventions

CRITICAL: Feature naming — "Evolution" must be renamed to "Refractor"

This PR implements the /evo status command under the "Evolution" branding. Per a project decision made on 2026-03-22, the card progression system is renamed from Evolution to Refractor across all user-facing surfaces:

  • Slash command must be /refractor status, not /evo status
  • cog/evolution.py file and class name Evolution should be refractor.py / Refractor
  • TIER_NAMES must be updated: Base Chrome (T0), Refractor (T1), Gold Refractor (T2), Superfractor (T3/T4)
  • The cog group name "evo" must become "refractor"
  • The cog registration in paperdynasty.py must become "cogs.refractor"
  • Test file should be tests/test_refractor_commands.py
  • Docstrings, embed titles, and all user-facing strings referencing "Evolution" or "Evo" must be updated

Internal API endpoint naming (evolution/cards) may remain for now while the database API migrates, per the same decision.

Minor: type shadows Python builtin

cogs/evolution.py line 134: the parameter is named type, which shadows the Python builtin. This is not a breaking issue but a ruff/lint concern. A name like card_type or filter_type would be cleaner and consistent with the API param name card_type already used on line 143.

Suggestions

  • The render_progress_bar docstring examples use → but the actual Unicode arrow \u2192 is used in format_evo_entry. These are consistent at runtime; the docstring could use the same Unicode char for accuracy, but this is minor.
  • Tests are thorough and well-documented — 39 tests covering all helper functions and async command paths is excellent coverage for a new cog. The docstrings on each test explain "what" and "why" clearly.
  • The out-of-scope removal of unused import datetime from paperdynasty.py is clean and appropriate given the ruff hook requirement. No objection.

Verdict: REQUEST_CHANGES

The implementation is functionally correct, well-tested, and follows the cog architecture properly. However, there is one blocking issue: the entire feature must be renamed from "Evolution/Evo" to "Refractor" before merge. This rename decision was made on 2026-03-22 and explicitly calls out this PR as needing the rename. The import path inconsistency (from helpers import vs from helpers.main import) and the type parameter name shadowing the builtin are secondary issues that should also be addressed.

Automated review by Claude PR Reviewer

## AI Code Review ### Files Reviewed - `cogs/evolution.py` (added) - `tests/test_evolution_commands.py` (added) - `paperdynasty.py` (modified) --- ### Findings #### Correctness **Import path mismatch — `from helpers import get_team_by_owner`** The new cog (`cogs/evolution.py` line 17) uses `from helpers import get_team_by_owner`. The comparable `scouting.py` cog uses the explicit path `from helpers.main import get_team_by_owner`. This works at runtime because `helpers/__init__.py` does `from helpers.main import *`, but it is inconsistent with the established pattern in this codebase. It should be `from helpers.main import get_team_by_owner`. **API response shape handling** Lines 155–160 handle two possible shapes from `db_get`: - A plain list - A dict with a `"cards"` key This dual-shape guard is the correct defensive pattern for a dependency not yet deployed (WP-07). However, if `db_get` returns `None` (network error or non-200 with `none_okay=True`), the `not data` check at line 149 handles it correctly. No issue here, just confirming the logic is sound. **`progress` parameter accepts any string value silently** `cogs/evolution.py` line 147: if `progress` is any value other than `"close"`, it is silently ignored and the full unfiltered list is shown. This is reasonable for now, but a user typing `progress="near"` would get no feedback that their filter was ignored. Consider noting in the `@app_commands.describe` that only `"close"` is a valid value, or add an explicit check that responds with an error for unrecognized values. #### Security No issues found. No user-controlled data is passed into queries or templates in a dangerous way. The `team_id` originates from `get_team_by_owner` (a server-side lookup against the authenticated user), not from direct user input. #### Style & Conventions **CRITICAL: Feature naming — "Evolution" must be renamed to "Refractor"** This PR implements the `/evo status` command under the "Evolution" branding. Per a project decision made on 2026-03-22, the card progression system is **renamed from Evolution to Refractor** across all user-facing surfaces: - Slash command must be `/refractor status`, not `/evo status` - `cog/evolution.py` file and class name `Evolution` should be `refractor.py` / `Refractor` - `TIER_NAMES` must be updated: Base Chrome (T0), Refractor (T1), Gold Refractor (T2), Superfractor (T3/T4) - The cog group name `"evo"` must become `"refractor"` - The cog registration in `paperdynasty.py` must become `"cogs.refractor"` - Test file should be `tests/test_refractor_commands.py` - Docstrings, embed titles, and all user-facing strings referencing "Evolution" or "Evo" must be updated Internal API endpoint naming (`evolution/cards`) may remain for now while the database API migrates, per the same decision. **Minor: `type` shadows Python builtin** `cogs/evolution.py` line 134: the parameter is named `type`, which shadows the Python builtin. This is not a breaking issue but a ruff/lint concern. A name like `card_type` or `filter_type` would be cleaner and consistent with the API param name `card_type` already used on line 143. #### Suggestions - The `render_progress_bar` docstring examples use `→` but the actual Unicode arrow `\u2192` is used in `format_evo_entry`. These are consistent at runtime; the docstring could use the same Unicode char for accuracy, but this is minor. - Tests are thorough and well-documented — 39 tests covering all helper functions and async command paths is excellent coverage for a new cog. The docstrings on each test explain "what" and "why" clearly. - The out-of-scope removal of unused `import datetime` from `paperdynasty.py` is clean and appropriate given the ruff hook requirement. No objection. --- ### Verdict: REQUEST_CHANGES The implementation is functionally correct, well-tested, and follows the cog architecture properly. However, there is one blocking issue: **the entire feature must be renamed from "Evolution/Evo" to "Refractor"** before merge. This rename decision was made on 2026-03-22 and explicitly calls out this PR as needing the rename. The import path inconsistency (`from helpers import` vs `from helpers.main import`) and the `type` parameter name shadowing the builtin are secondary issues that should also be addressed. --- *Automated review by Claude PR Reviewer*
cal added 1 commit 2026-03-23 13:48:41 +00:00
refactor: rename Evolution to Refractor system
All checks were successful
Build Docker Image / build (pull_request) Successful in 1m34s
Details
6b4957ec70 
- cogs/evolution.py → cogs/refractor.py (class, group, command names)
- Tier names: Base Chrome, Refractor, Gold Refractor, Superfractor
- Fix import: helpers.main.get_team_by_owner
- Fix shadowed builtin: type → card_type parameter
- Tests renamed and updated (39/39 pass)

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
cal requested changes 2026-03-23 13:59:25 +00:00
Dismissed
cal left a comment
Owner
Copy Link

AI Code Review

Files Reviewed

  • cogs/refractor.py (added)
  • tests/test_refractor_commands.py (added)
  • paperdynasty.py (modified)

Findings

Correctness

[MUST FIX] T3 and T4 share the same display name "Superfractor"

In cogs/refractor.py lines 25-30:

TIER_NAMES = {
    0: "Base Chrome",
    1: "Refractor",
    2: "Gold Refractor",
    3: "Superfractor",
    4: "Superfractor",  # identical to T3
}

T3 and T4 have the same label. A player sitting at T3 and a fully-evolved player at T4 will both display "(Superfractor)" in the first line of their entry. The distinction only appears in the second line ("FULLY EVOLVED" vs the progress bar), but the tier label itself is ambiguous. The original PR description listed T4 as "Evolved" — it's unclear whether the rename commit intentionally collapsed these two or accidentally copied T3. This needs a deliberate decision, and if they are meant to be distinct, T4 should have a unique name (e.g., "Black Superfractor" or "1/1 Superfractor").

The test at line 352 asserts TIER_NAMES[4] == "Superfractor" — this will pass, but it only codifies the ambiguous state rather than verifying intent.

[MINOR] Group variable naming is inconsistent with project conventions

In cogs/refractor.py line 113:

refractor_group = app_commands.Group(
    name="refractor", description="Refractor tracking commands"
)

Every other cog in this project names its command groups group_<name> (e.g., group_buy, group_lookup, group_gauntlet, group_paperdex, group_new_game, group_log). This PR uses refractor_group — the reversed word order breaks the project pattern. The command will function correctly since Discord resolves the group by the name= argument, not the Python variable name, but it creates inconsistency for anyone reading across cogs.

Should be: group_refractor = app_commands.Group(...)

Security

No issues found. No user inputs are passed to queries unvalidated — all filtering uses typed optional parameters. No secrets or hardcoded credentials.

Style and Conventions

[MINOR] PR body contains stale tier names

The PR description still lists the old Evolution tier names (T0=Unranked, T1=Initiate, T2=Rising, T3=Ascendant, T4=Evolved). The rename commit changed these to Refractor names, but the PR body was not updated. This is not a code issue, but it creates confusion for anyone reading the PR history later.

[OBSERVATION] No error handling around db_get in refractor_status

cogs/refractor.py lines 152-158 call db_get without a try/except. This is consistent with every other cog in the project — the global on_app_command_error handler in paperdynasty.py will catch APITimeoutError and DatabaseError and send an ephemeral message to the user. This is acceptable as-is, though a user-friendly inline message (e.g., "Could not reach the API, try again later") would be a better UX than the generic error handler. Not blocking.

Test Coverage

Test coverage is thorough and well-structured. The docstrings correctly explain what each test verifies and why. Factory data is used in fixtures rather than mocks where appropriate. The two async integration tests (test_refractor_status_no_team, test_refractor_status_empty_roster) correctly patch at the cog's import path. 39 tests for a 202-line module is excellent coverage.

Verdict: REQUEST_CHANGES

The T3/T4 name collision is a product correctness issue — it results in ambiguous user-facing output that contradicts the intent of a tiered progression system. The group variable naming is a minor convention inconsistency. Please resolve the T3/T4 naming before merging, even if the decision is to intentionally keep them the same (in which case a comment explaining why would suffice).

Automated review by Claude PR Reviewer

## AI Code Review ### Files Reviewed - `cogs/refractor.py` (added) - `tests/test_refractor_commands.py` (added) - `paperdynasty.py` (modified) --- ### Findings #### Correctness **[MUST FIX] T3 and T4 share the same display name "Superfractor"** In `cogs/refractor.py` lines 25-30: ```python TIER_NAMES = { 0: "Base Chrome", 1: "Refractor", 2: "Gold Refractor", 3: "Superfractor", 4: "Superfractor", # identical to T3 } ``` T3 and T4 have the same label. A player sitting at T3 and a fully-evolved player at T4 will both display "(Superfractor)" in the first line of their entry. The distinction only appears in the second line ("FULLY EVOLVED" vs the progress bar), but the tier label itself is ambiguous. The original PR description listed T4 as "Evolved" — it's unclear whether the rename commit intentionally collapsed these two or accidentally copied T3. This needs a deliberate decision, and if they are meant to be distinct, T4 should have a unique name (e.g., "Black Superfractor" or "1/1 Superfractor"). The test at line 352 asserts `TIER_NAMES[4] == "Superfractor"` — this will pass, but it only codifies the ambiguous state rather than verifying intent. **[MINOR] Group variable naming is inconsistent with project conventions** In `cogs/refractor.py` line 113: ```python refractor_group = app_commands.Group( name="refractor", description="Refractor tracking commands" ) ``` Every other cog in this project names its command groups `group_<name>` (e.g., `group_buy`, `group_lookup`, `group_gauntlet`, `group_paperdex`, `group_new_game`, `group_log`). This PR uses `refractor_group` — the reversed word order breaks the project pattern. The command will function correctly since Discord resolves the group by the `name=` argument, not the Python variable name, but it creates inconsistency for anyone reading across cogs. Should be: `group_refractor = app_commands.Group(...)` #### Security No issues found. No user inputs are passed to queries unvalidated — all filtering uses typed optional parameters. No secrets or hardcoded credentials. #### Style and Conventions **[MINOR] PR body contains stale tier names** The PR description still lists the old Evolution tier names (T0=Unranked, T1=Initiate, T2=Rising, T3=Ascendant, T4=Evolved). The rename commit changed these to Refractor names, but the PR body was not updated. This is not a code issue, but it creates confusion for anyone reading the PR history later. **[OBSERVATION] No error handling around `db_get` in `refractor_status`** `cogs/refractor.py` lines 152-158 call `db_get` without a try/except. This is consistent with every other cog in the project — the global `on_app_command_error` handler in `paperdynasty.py` will catch `APITimeoutError` and `DatabaseError` and send an ephemeral message to the user. This is acceptable as-is, though a user-friendly inline message (e.g., "Could not reach the API, try again later") would be a better UX than the generic error handler. Not blocking. #### Test Coverage Test coverage is thorough and well-structured. The docstrings correctly explain what each test verifies and why. Factory data is used in fixtures rather than mocks where appropriate. The two async integration tests (`test_refractor_status_no_team`, `test_refractor_status_empty_roster`) correctly patch at the cog's import path. 39 tests for a 202-line module is excellent coverage. --- ### Verdict: REQUEST_CHANGES The T3/T4 name collision is a product correctness issue — it results in ambiguous user-facing output that contradicts the intent of a tiered progression system. The group variable naming is a minor convention inconsistency. Please resolve the T3/T4 naming before merging, even if the decision is to intentionally keep them the same (in which case a comment explaining why would suffice). --- *Automated review by Claude PR Reviewer*
cal commented 2026-03-23 15:29:45 +00:00
Owner
Copy Link

Updated Tier Names

The reviewer flagged that T3 and T4 both mapped to "Superfractor". Here are the corrected tier names to use:

Tier Name Description
T0 Base Card No progression
T1 Base Chrome
T2 Refractor
T3 Gold Refractor
T4 Superfractor Fully evolved

The TIER_NAMES dict in cogs/refractor.py and badge labels in helpers/main.py (PR #88) need to be updated to match. Also rename refractor_group → group_refractor per project convention.

## Updated Tier Names The reviewer flagged that T3 and T4 both mapped to "Superfractor". Here are the corrected tier names to use: | Tier | Name | Description | |------|------|-------------| | T0 | Base Card | No progression | | T1 | Base Chrome | | | T2 | Refractor | | | T3 | Gold Refractor | | | T4 | Superfractor | Fully evolved | The `TIER_NAMES` dict in `cogs/refractor.py` and badge labels in `helpers/main.py` (PR #88) need to be updated to match. Also rename `refractor_group` → `group_refractor` per project convention.
cal added 1 commit 2026-03-23 15:31:18 +00:00
fix: correct tier names and group variable convention
All checks were successful
Build Docker Image / build (pull_request) Successful in 1m45s
Details
5670cd6e88 
Tier names updated per Cal's spec:
  T0=Base Card, T1=Base Chrome, T2=Refractor, T3=Gold Refractor, T4=Superfractor

Also renames refractor_group → group_refractor per project convention.
All 39 tests pass.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
cal approved these changes 2026-03-23 15:34:13 +00:00
Dismissed
cal left a comment
Owner
Copy Link

AI Code Review

Files Reviewed

  • cogs/refractor.py (added)
  • tests/test_refractor_commands.py (added)
  • paperdynasty.py (modified)

Findings

Correctness

  • The Refractor rename is complete and consistent across all three changed files: the module name (cogs/refractor.py), the class name (Refractor), the app command group (group_refractor / name="refractor"), the cog registration (cogs.refractor), and all tier names (Base Card / Base Chrome / Refractor / Gold Refractor / Superfractor) are in sync.
  • Logic is sound: render_progress_bar correctly clamps at 1.0, handles zero-threshold edge case, and uses round() for fill calculation. paginate correctly handles empty lists, clamping, and off-by-one page math. apply_close_filter correctly excludes fully-evolved cards before the percentage check.
  • The defer + edit_original_response pattern matches the dominant pattern in cogs/players.py and is appropriate here.
  • The double empty-check (first on raw data, then on extracted items) correctly handles both None returns from a down API and a valid {"cards": []} response from a running-but-empty API.

Security

  • No injection risk. All user-supplied filter values (card_type, season, tier) are passed as query params through db_get, not interpolated into SQL or shell commands.
  • No hardcoded credentials or secrets.

Style & Conventions

  • Module docstring, class docstring, and all helper function docstrings are present and detailed. Matches the style established in scouting.py and other modern cogs.
  • Pure helper functions (render_progress_bar, format_refractor_entry, apply_close_filter, paginate) are correctly separated from the Cog class, making them independently testable.
  • Import order (stdlib → third-party → local) is consistent with the rest of the codebase.
  • The import datetime removal from paperdynasty.py is appropriate — it was unused and would have caused a ruff lint failure.

Suggestions

  • PR body is stale. The description still refers to evolution.py, cogs/evolution.py, tier names "Initiate/Rising/Ascendant/Evolved", and refractor_group (old attribute name). The actual code is correct, but the PR body is misleading for future reference. Consider updating it before merge.
  • progress parameter has no validation feedback. If a user passes progress="open" or any value other than "close", the filter is silently skipped and a full result set is returned with no indication that their filter was unrecognized. A small guard could improve UX: if progress and progress != "close": await interaction.edit_original_response(content='Invalid progress filter. Use "close" or omit.'); return. Not a blocker, but worth considering.
  • card_type has no client-side guard. Values like "dh" or "catcher" are passed through to the API unchanged. The API presumably rejects or ignores them, but the user gets no feedback. A choices annotation on the card_type parameter (or an allowlist check) would surface this earlier.

Verdict: APPROVED

The implementation is correct, the Refractor rename is thorough and consistent, tests are comprehensive (39 cases covering all helper functions plus async command paths), and the code follows project conventions. The two UX suggestions are improvements, not blocking issues. The stale PR body is a documentation concern only — the code itself is correct.

Automated review by Claude PR Reviewer

## AI Code Review ### Files Reviewed - `cogs/refractor.py` (added) - `tests/test_refractor_commands.py` (added) - `paperdynasty.py` (modified) ### Findings #### Correctness - The Refractor rename is complete and consistent across all three changed files: the module name (`cogs/refractor.py`), the class name (`Refractor`), the app command group (`group_refractor` / `name="refractor"`), the cog registration (`cogs.refractor`), and all tier names (Base Card / Base Chrome / Refractor / Gold Refractor / Superfractor) are in sync. - Logic is sound: `render_progress_bar` correctly clamps at 1.0, handles zero-threshold edge case, and uses `round()` for fill calculation. `paginate` correctly handles empty lists, clamping, and off-by-one page math. `apply_close_filter` correctly excludes fully-evolved cards before the percentage check. - The `defer` + `edit_original_response` pattern matches the dominant pattern in `cogs/players.py` and is appropriate here. - The double empty-check (first on raw `data`, then on extracted `items`) correctly handles both `None` returns from a down API and a valid `{"cards": []}` response from a running-but-empty API. #### Security - No injection risk. All user-supplied filter values (`card_type`, `season`, `tier`) are passed as query params through `db_get`, not interpolated into SQL or shell commands. - No hardcoded credentials or secrets. #### Style & Conventions - Module docstring, class docstring, and all helper function docstrings are present and detailed. Matches the style established in `scouting.py` and other modern cogs. - Pure helper functions (`render_progress_bar`, `format_refractor_entry`, `apply_close_filter`, `paginate`) are correctly separated from the Cog class, making them independently testable. - Import order (stdlib → third-party → local) is consistent with the rest of the codebase. - The `import datetime` removal from `paperdynasty.py` is appropriate — it was unused and would have caused a ruff lint failure. #### Suggestions - **PR body is stale.** The description still refers to `evolution.py`, `cogs/evolution.py`, tier names "Initiate/Rising/Ascendant/Evolved", and `refractor_group` (old attribute name). The actual code is correct, but the PR body is misleading for future reference. Consider updating it before merge. - **`progress` parameter has no validation feedback.** If a user passes `progress="open"` or any value other than `"close"`, the filter is silently skipped and a full result set is returned with no indication that their filter was unrecognized. A small guard could improve UX: `if progress and progress != "close": await interaction.edit_original_response(content='Invalid progress filter. Use "close" or omit.'); return`. Not a blocker, but worth considering. - **`card_type` has no client-side guard.** Values like `"dh"` or `"catcher"` are passed through to the API unchanged. The API presumably rejects or ignores them, but the user gets no feedback. A `choices` annotation on the `card_type` parameter (or an allowlist check) would surface this earlier. ### Verdict: APPROVED The implementation is correct, the Refractor rename is thorough and consistent, tests are comprehensive (39 cases covering all helper functions plus async command paths), and the code follows project conventions. The two UX suggestions are improvements, not blocking issues. The stale PR body is a documentation concern only — the code itself is correct. --- *Automated review by Claude PR Reviewer*
cal requested changes 2026-03-23 17:22:05 +00:00
Dismissed
cal left a comment
Owner
Copy Link

AI Code Review

Files Reviewed

  • cogs/refractor.py (added)
  • tests/test_refractor_commands.py (added)
  • paperdynasty.py (modified)

Findings

Correctness

[BLOCKER] PR title and body describe the wrong feature.
The PR title is "feat: /evo status slash command (WP-11)" and the body describes the old Evolution system with the old tier names (Unranked, Initiate, Rising, Ascendant, Evolved). The diff, however, implements the Refractor rename correctly. The PR metadata is misleading and must be updated to reflect what's actually being shipped. This is a Refractor rename PR, not an Evolution feature PR.

[BLOCKER] API endpoint is not renamed.
cogs/refractor.py line 147 calls db_get("evolution/cards", params=params). If this is the Refractor rename, the corresponding database API endpoint should also be renamed (or planned to be renamed). Hitting evolution/cards from a cog called refractor is inconsistent. If the API rename is intentional scope-out, that must be documented clearly in the PR body, and an issue should be tracked.

[BLOCKER] utilities/evolution_notifications.py and tests/test_evolution_notifications.py are not updated.
The Refractor rename is supposed to cover "cog files, test files, tier names, and badge labels." The notifications utility (utilities/evolution_notifications.py) still uses old Evolution tier names in its strings ("Evolution Tier Up!", "reached maximum evolution") and the tier names passed to it from gameplay code are still the old set (Rising, Legendary, etc.). tests/test_evolution_notifications.py asserts on those exact old strings. These files are in scope for this rename and are missing from the diff.

[BLOCKER] Badge labels ([BC], [R], [GR], [SF]) are not implemented.
The spec requires badge labels on card display: [BC] for T1, [R] for T2, [GR] for T3, [SF] for T4, and no badge for T0. Neither format_refractor_entry nor any other part of the diff renders these badges. They are completely absent.

[MINOR] "FULLY EVOLVED" language in format_refractor_entry.
cogs/refractor.py line 81 outputs "FULLY EVOLVED ★" as the detail string. If this is a Refractor system, this language should be updated — e.g., "SUPERFRACTOR ★" or "MAX TIER ★" — to match the new naming. "Fully Evolved" is Evolution-era language.

Security

No issues found. No user input is passed to queries without being filtered through known parameter keys. The progress filter only acts on the string "close" — other values are safely ignored.

Style & Conventions

[MINOR] Slash command name is /refractor status but the cog doc and module comment still describe it as /evo status.
The module docstring at the top of cogs/refractor.py says "Depends on WP-07 (evolution/cards API endpoint)" — the WP-07 reference is fine, but the overall doc header and the format_refractor_entry docstring example still show (Refractor) for T2 which is accurate but the tier label is incidentally the same as the system name, which can be confusing for someone reading T1's (Base Chrome) output. No change needed on this one, just be aware.

[MINOR] test_tier_label_in_output asserts (Base Chrome) for a T1 batter fixture.
This is correct and confirms the rename is applied in tests. No issue — flagging for confirmation that T1 = Base Chrome is intentional per spec (it is).

Suggestions

  • After updating the PR body, add a checklist item confirming which files were renamed vs. which are pending a separate PR (e.g., the notifications utility and its tests, the API endpoint rename).
  • Consider renaming the apply_close_filter function to filter_close_to_tier_up for clarity, though this is purely stylistic.
  • The test_evolution_notifications.py file should be renamed to test_refractor_notifications.py when the notifications utility is updated — keeping them in sync will make it clearer what's been renamed.

Verdict: REQUEST_CHANGES

The cogs/refractor.py implementation is well-structured — the logic, progress bar rendering, close filter, pagination, and tier name constants are all correct against the spec. The test file is thorough and well-documented. However, this PR cannot be merged as-is because:

  1. The PR metadata (title + body) describes the original Evolution implementation, not the Refractor rename.
  2. The API endpoint (evolution/cards) is not renamed and no rationale is given.
  3. The badge labels ([BC], [R], [GR], [SF]) required by the spec are entirely missing from the implementation.
  4. The notifications utility and its test file, which carry Evolution naming, are not touched — they are in scope for a rename PR.

These are substantive gaps, not style preferences. All four issues need resolution before merge.

Automated review by Claude PR Reviewer

## AI Code Review ### Files Reviewed - `cogs/refractor.py` (added) - `tests/test_refractor_commands.py` (added) - `paperdynasty.py` (modified) --- ### Findings #### Correctness **[BLOCKER] PR title and body describe the wrong feature.** The PR title is "feat: /evo status slash command (WP-11)" and the body describes the old Evolution system with the old tier names (Unranked, Initiate, Rising, Ascendant, Evolved). The diff, however, implements the Refractor rename correctly. The PR metadata is misleading and must be updated to reflect what's actually being shipped. This is a Refractor rename PR, not an Evolution feature PR. **[BLOCKER] API endpoint is not renamed.** `cogs/refractor.py` line 147 calls `db_get("evolution/cards", params=params)`. If this is the Refractor rename, the corresponding database API endpoint should also be renamed (or planned to be renamed). Hitting `evolution/cards` from a cog called `refractor` is inconsistent. If the API rename is intentional scope-out, that must be documented clearly in the PR body, and an issue should be tracked. **[BLOCKER] `utilities/evolution_notifications.py` and `tests/test_evolution_notifications.py` are not updated.** The Refractor rename is supposed to cover "cog files, test files, tier names, and badge labels." The notifications utility (`utilities/evolution_notifications.py`) still uses old Evolution tier names in its strings ("Evolution Tier Up!", "reached maximum evolution") and the tier names passed to it from gameplay code are still the old set (Rising, Legendary, etc.). `tests/test_evolution_notifications.py` asserts on those exact old strings. These files are in scope for this rename and are missing from the diff. **[BLOCKER] Badge labels ([BC], [R], [GR], [SF]) are not implemented.** The spec requires badge labels on card display: [BC] for T1, [R] for T2, [GR] for T3, [SF] for T4, and no badge for T0. Neither `format_refractor_entry` nor any other part of the diff renders these badges. They are completely absent. **[MINOR] "FULLY EVOLVED" language in `format_refractor_entry`.** `cogs/refractor.py` line 81 outputs `"FULLY EVOLVED ★"` as the detail string. If this is a Refractor system, this language should be updated — e.g., "SUPERFRACTOR ★" or "MAX TIER ★" — to match the new naming. "Fully Evolved" is Evolution-era language. #### Security No issues found. No user input is passed to queries without being filtered through known parameter keys. The `progress` filter only acts on the string `"close"` — other values are safely ignored. #### Style & Conventions **[MINOR] Slash command name is `/refractor status` but the cog doc and module comment still describe it as `/evo status`.** The module docstring at the top of `cogs/refractor.py` says "Depends on WP-07 (evolution/cards API endpoint)" — the WP-07 reference is fine, but the overall doc header and the `format_refractor_entry` docstring example still show `(Refractor)` for T2 which is accurate but the tier label is incidentally the same as the system name, which can be confusing for someone reading T1's `(Base Chrome)` output. No change needed on this one, just be aware. **[MINOR] `test_tier_label_in_output` asserts `(Base Chrome)` for a T1 batter fixture.** This is correct and confirms the rename is applied in tests. No issue — flagging for confirmation that T1 = Base Chrome is intentional per spec (it is). #### Suggestions - After updating the PR body, add a checklist item confirming which files were renamed vs. which are pending a separate PR (e.g., the notifications utility and its tests, the API endpoint rename). - Consider renaming the `apply_close_filter` function to `filter_close_to_tier_up` for clarity, though this is purely stylistic. - The `test_evolution_notifications.py` file should be renamed to `test_refractor_notifications.py` when the notifications utility is updated — keeping them in sync will make it clearer what's been renamed. --- ### Verdict: REQUEST_CHANGES The `cogs/refractor.py` implementation is well-structured — the logic, progress bar rendering, close filter, pagination, and tier name constants are all correct against the spec. The test file is thorough and well-documented. However, this PR cannot be merged as-is because: 1. The PR metadata (title + body) describes the original Evolution implementation, not the Refractor rename. 2. The API endpoint (`evolution/cards`) is not renamed and no rationale is given. 3. The badge labels ([BC], [R], [GR], [SF]) required by the spec are entirely missing from the implementation. 4. The notifications utility and its test file, which carry Evolution naming, are not touched — they are in scope for a rename PR. These are substantive gaps, not style preferences. All four issues need resolution before merge. --- *Automated review by Claude PR Reviewer*
cal requested changes 2026-03-23 20:03:53 +00:00
Dismissed
cal left a comment
Owner
Copy Link

AI Code Review

Files Reviewed

  • cogs/refractor.py (added)
  • tests/test_refractor_commands.py (added)
  • paperdynasty.py (modified)
  • utilities/evolution_notifications.py (pre-existing, not in diff — reviewed for issue #4)

Previous Review Issue Checklist

A prior review flagged 4 issues. Here is the status of each:

# Issue Status
1 PR title/body still describes old Evolution system NOT FIXED
2 API endpoint not renamed (still calls evolution/cards) NOT FIXED
3 Badge labels missing from format_refractor_entry NOT FIXED
4 utilities/evolution_notifications.py untouched NOT FIXED

Findings

Correctness

Issue 2 (blocking): Wrong API endpoint — evolution/cards still called

cogs/refractor.py, line 143:

data = await db_get("evolution/cards", params=params)

The database API has been fully renamed. The correct endpoint per the current API spec is refractor/cards (under /api/v2/refractor/*). This call will produce a 404 at runtime against any deployed version of the database API that has completed the rename. This is the most critical functional bug in the PR.

Issue 3 (blocking): Badge labels absent from format_refractor_entry

The project spec defines badge labels for display: [BC] (T1), [R] (T2), [GR] (T3), [SF] (T4). T0 has no badge. These are entirely absent from format_refractor_entry and from the BADGE_LABELS constant that should exist in cogs/refractor.py. The current output for a T1 card is:

**Mike Trout** (Base Chrome)
[========--] 120/149 (PA+TB×2) — T1 → T2

The expected output should include the badge label, e.g.:

**Mike Trout** [BC] (Base Chrome)
[========--] 120/149 (PA+TB×2) — T1 → T2

No tests cover badge label rendering either, so test_refractor_commands.py does not catch this gap.

Security

No issues found. No user input is passed unsanitized; all API calls use parameterised query lists through db_get.

Style & Conventions

Issue 1 (non-blocking, but still unresolved): PR title and body describe the old Evolution system

The PR title reads feat: /evo status slash command (WP-11) (#76) and the body refers to cogs/evolution.py, /evo status, tier names like "Unranked / Initiate / Rising", and calls the API endpoint GET /evolution/cards. None of this reflects the actual code in this PR. The title should be updated to feat: /refractor status slash command (WP-11) and the body rewritten to match the current implementation. This is a documentation/traceability issue, not a code defect, but it makes the PR misleading.

The docstring at the top of cogs/refractor.py (line 7) also still reads:

Depends on WP-07 (evolution/cards API endpoint).

This should read refractor/cards API endpoint.

Issue 4 (non-blocking): utilities/evolution_notifications.py is a dead file

utilities/evolution_notifications.py is a stub duplicate of helpers/evolution_notifs.py. Its docstrings and embed text still use "evolution" terminology ("Build a Discord embed for a single evolution tier-up event", "has reached maximum evolution"). This file is not imported anywhere in the codebase (confirmed by search), but it remains as noise. It should either be deleted or updated and wired in if it serves a purpose. This was flagged in the previous review and remains untouched.

Suggestions

  • The FORMULA_LABELS dict maps "rp" and "sp" to the same label "IP+K". This is correct, but a comment noting the intentional duplication would prevent future readers from assuming it is a copy/paste error.
  • The apply_close_filter docstring says "Fully evolved cards (T4 or no next_threshold) are excluded" — the implementation correctly handles both cases, but the condition not next_threshold will also exclude next_threshold=0, which is an edge case worth a comment.

Verdict: REQUEST_CHANGES

Two blocking issues must be resolved before merge:

  1. Line 143 of cogs/refractor.py: Change "evolution/cards" to "refractor/cards" to match the renamed database API. This will cause runtime 404 errors in any deployed environment where WP-07 has landed.

  2. format_refractor_entry in cogs/refractor.py: Add a BADGE_LABELS constant and include badge labels ([BC], [R], [GR], [SF]) in the formatted output for T1–T4 cards. Update tests in test_refractor_commands.py to assert badge label presence.

The remaining two issues (PR metadata, dead utility file) are non-blocking but should be addressed in this PR since they were flagged in a previous review cycle.

Automated review by Claude PR Reviewer

## AI Code Review ### Files Reviewed - `cogs/refractor.py` (added) - `tests/test_refractor_commands.py` (added) - `paperdynasty.py` (modified) - `utilities/evolution_notifications.py` (pre-existing, not in diff — reviewed for issue #4) --- ### Previous Review Issue Checklist A prior review flagged 4 issues. Here is the status of each: | # | Issue | Status | |---|-------|--------| | 1 | PR title/body still describes old Evolution system | **NOT FIXED** | | 2 | API endpoint not renamed (still calls `evolution/cards`) | **NOT FIXED** | | 3 | Badge labels missing from `format_refractor_entry` | **NOT FIXED** | | 4 | `utilities/evolution_notifications.py` untouched | **NOT FIXED** | --- ### Findings #### Correctness **Issue 2 (blocking): Wrong API endpoint — `evolution/cards` still called** `cogs/refractor.py`, line 143: ```python data = await db_get("evolution/cards", params=params) ``` The database API has been fully renamed. The correct endpoint per the current API spec is `refractor/cards` (under `/api/v2/refractor/*`). This call will produce a 404 at runtime against any deployed version of the database API that has completed the rename. This is the most critical functional bug in the PR. **Issue 3 (blocking): Badge labels absent from `format_refractor_entry`** The project spec defines badge labels for display: `[BC]` (T1), `[R]` (T2), `[GR]` (T3), `[SF]` (T4). T0 has no badge. These are entirely absent from `format_refractor_entry` and from the `BADGE_LABELS` constant that should exist in `cogs/refractor.py`. The current output for a T1 card is: ``` **Mike Trout** (Base Chrome) [========--] 120/149 (PA+TB×2) — T1 → T2 ``` The expected output should include the badge label, e.g.: ``` **Mike Trout** [BC] (Base Chrome) [========--] 120/149 (PA+TB×2) — T1 → T2 ``` No tests cover badge label rendering either, so `test_refractor_commands.py` does not catch this gap. #### Security No issues found. No user input is passed unsanitized; all API calls use parameterised query lists through `db_get`. #### Style & Conventions **Issue 1 (non-blocking, but still unresolved): PR title and body describe the old Evolution system** The PR title reads `feat: /evo status slash command (WP-11) (#76)` and the body refers to `cogs/evolution.py`, `/evo status`, tier names like "Unranked / Initiate / Rising", and calls the API endpoint `GET /evolution/cards`. None of this reflects the actual code in this PR. The title should be updated to `feat: /refractor status slash command (WP-11)` and the body rewritten to match the current implementation. This is a documentation/traceability issue, not a code defect, but it makes the PR misleading. The docstring at the top of `cogs/refractor.py` (line 7) also still reads: ``` Depends on WP-07 (evolution/cards API endpoint). ``` This should read `refractor/cards API endpoint`. **Issue 4 (non-blocking): `utilities/evolution_notifications.py` is a dead file** `utilities/evolution_notifications.py` is a stub duplicate of `helpers/evolution_notifs.py`. Its docstrings and embed text still use "evolution" terminology (`"Build a Discord embed for a single evolution tier-up event"`, `"has reached maximum evolution"`). This file is not imported anywhere in the codebase (confirmed by search), but it remains as noise. It should either be deleted or updated and wired in if it serves a purpose. This was flagged in the previous review and remains untouched. #### Suggestions - The `FORMULA_LABELS` dict maps `"rp"` and `"sp"` to the same label `"IP+K"`. This is correct, but a comment noting the intentional duplication would prevent future readers from assuming it is a copy/paste error. - The `apply_close_filter` docstring says "Fully evolved cards (T4 or no next_threshold) are excluded" — the implementation correctly handles both cases, but the condition `not next_threshold` will also exclude `next_threshold=0`, which is an edge case worth a comment. --- ### Verdict: REQUEST_CHANGES Two blocking issues must be resolved before merge: 1. **Line 143 of `cogs/refractor.py`**: Change `"evolution/cards"` to `"refractor/cards"` to match the renamed database API. This will cause runtime 404 errors in any deployed environment where WP-07 has landed. 2. **`format_refractor_entry` in `cogs/refractor.py`**: Add a `BADGE_LABELS` constant and include badge labels (`[BC]`, `[R]`, `[GR]`, `[SF]`) in the formatted output for T1–T4 cards. Update tests in `test_refractor_commands.py` to assert badge label presence. The remaining two issues (PR metadata, dead utility file) are non-blocking but should be addressed in this PR since they were flagged in a previous review cycle. --- *Automated review by Claude PR Reviewer*
cogs/refractor.py
@ -0,0 +140,4 @@
self,
interaction: discord.Interaction,
card_type: Optional[str] = None,
season: Optional[int] = None,
cal commented 2026-03-23 20:03:53 +00:00
Owner
Copy Link

Wrong endpoint: "evolution/cards" must be "refractor/cards". The database API has been fully renamed to /api/v2/refractor/*. This will produce a 404 at runtime against the current API.

Wrong endpoint: `"evolution/cards"` must be `"refractor/cards"`. The database API has been fully renamed to `/api/v2/refractor/*`. This will produce a 404 at runtime against the current API.
cal added 1 commit 2026-03-23 20:08:47 +00:00
fix: address reviewer issues — rename evolution endpoints, add TIER_BADGES
All checks were successful
Build Docker Image / build (pull_request) Successful in 1m32s
Details
45d71c61e3 
- Update module docstring: replace evolution/cards with refractor/cards,
  drop old tier names (Unranked/Initiate/Rising/Ascendant/Evolved), add
  correct tier names (Base Card/Base Chrome/Refractor/Gold Refractor/
  Superfractor)
- Fix API call: db_get("evolution/cards") → db_get("refractor/cards")
- Add TIER_BADGES dict {1:"[BC]", 2:"[R]", 3:"[GR]", 4:"[SF]"}
- Update format_refractor_entry to prepend badge label for T1-T4 (T0 has
  no badge)
- Add TestTierBadges test class (11 tests) asserting badge values and
  presence in formatted output
- Update test_player_name_in_output to accommodate badge-prefixed bold name

Dead utilities/evolution_notifications.py has no source file on this branch
(WP-14/PR #112 already delivered the replacement).

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
cal changed title from feat: /evo status slash command (WP-11) (#76) to feat: /refractor status slash command (WP-11) 2026-03-23 20:09:14 +00:00
cal approved these changes 2026-03-23 20:10:42 +00:00
Dismissed
cal left a comment
Owner
Copy Link

AI Code Review (Cycle 2)

Files Reviewed

  • cogs/refractor.py (added)
  • tests/test_refractor_commands.py (added)
  • paperdynasty.py (modified)

Cycle 1 Fix Verification

All four issues from the previous review have been addressed correctly and completely.

Fix 1 — API endpoint corrected to refractor/cards
Confirmed. Line data = await db_get("refractor/cards", params=params) in cogs/refractor.py uses the correct endpoint. No trace of the old evolution/cards path anywhere in the diff or in the file tree.

Fix 2 — TIER_BADGES constant + badge rendering + 11 new tests
Confirmed. TIER_BADGES = {1: "[BC]", 2: "[R]", 3: "[GR]", 4: "[SF]"} is defined at module level. format_refractor_entry correctly applies the badge via display_name = f"{badge} {player_name}" if badge else player_name. The TestTierBadges class covers all four badge values, T0 absence, badge presence in formatted output for T1/T2/T4, T0 no-badge, and badge-before-name ordering — 11 tests total, all verifiable against the implementation.

Fix 3 — Module docstring and PR metadata updated
Confirmed. The module docstring in cogs/refractor.py now correctly reads "Refractor cog" with tier names listed accurately. PR title, body, and the API Dependency section all reference refractor/cards and WP-07. No stale "evolution" language remains.

Fix 4 — Dead evolution_notifications.py files confirmed absent
Confirmed. git ls-tree of the PR branch head returns no results for any "evolution" path. The PR body explicitly notes utilities/evolution_notifications.py has no source file on this branch and WP-14 (PR #112) delivers the replacement.

Findings

Correctness

No issues found. The implementation matches the PR description exactly. Progress bar rendering, badge prepending, close-filter logic (80% threshold), pagination clamping, and graceful empty/no-team responses all behave correctly as specified and as covered by tests.

Security

No issues found. No user-supplied input is interpolated into queries unsanitized — all params are passed as a structured list to db_get. The command is ephemeral (ephemeral=True), so responses are user-private by default.

Style & Conventions

No issues found. Module follows the cog-based pattern used throughout the project. Pure helper functions at module level are testable without Discord infrastructure. Docstrings are present and follow existing patterns. The out-of-scope removal of the dead import datetime from paperdynasty.py is appropriate and noted in the PR body.

Suggestions

  • The progress parameter on refractor_status accepts any string but only "close" has defined behavior. A future improvement would be an app_commands.choices constraint or an explicit error response for unrecognized values, though this is not a blocker.
  • test_t3_badge_in_format_entry is not present in the test suite (T3/Gold Refractor badge rendering in formatted output). Minor gap — T1, T2, T4 are covered, T3 is only verified at the constant level. Not a blocker.

Verdict: APPROVED

All four cycle 1 issues are fixed correctly. The implementation is complete, secure, well-tested (49 tests passing), and consistent with the project's cog-based conventions. Ready to merge.

Automated review by Claude PR Reviewer (Cycle 2)

## AI Code Review (Cycle 2) ### Files Reviewed - `cogs/refractor.py` (added) - `tests/test_refractor_commands.py` (added) - `paperdynasty.py` (modified) ### Cycle 1 Fix Verification All four issues from the previous review have been addressed correctly and completely. **Fix 1 — API endpoint corrected to `refractor/cards`** Confirmed. Line `data = await db_get("refractor/cards", params=params)` in `cogs/refractor.py` uses the correct endpoint. No trace of the old `evolution/cards` path anywhere in the diff or in the file tree. **Fix 2 — TIER_BADGES constant + badge rendering + 11 new tests** Confirmed. `TIER_BADGES = {1: "[BC]", 2: "[R]", 3: "[GR]", 4: "[SF]"}` is defined at module level. `format_refractor_entry` correctly applies the badge via `display_name = f"{badge} {player_name}" if badge else player_name`. The `TestTierBadges` class covers all four badge values, T0 absence, badge presence in formatted output for T1/T2/T4, T0 no-badge, and badge-before-name ordering — 11 tests total, all verifiable against the implementation. **Fix 3 — Module docstring and PR metadata updated** Confirmed. The module docstring in `cogs/refractor.py` now correctly reads "Refractor cog" with tier names listed accurately. PR title, body, and the API Dependency section all reference `refractor/cards` and WP-07. No stale "evolution" language remains. **Fix 4 — Dead evolution_notifications.py files confirmed absent** Confirmed. `git ls-tree` of the PR branch head returns no results for any "evolution" path. The PR body explicitly notes `utilities/evolution_notifications.py` has no source file on this branch and WP-14 (PR #112) delivers the replacement. ### Findings #### Correctness No issues found. The implementation matches the PR description exactly. Progress bar rendering, badge prepending, close-filter logic (80% threshold), pagination clamping, and graceful empty/no-team responses all behave correctly as specified and as covered by tests. #### Security No issues found. No user-supplied input is interpolated into queries unsanitized — all params are passed as a structured list to `db_get`. The command is ephemeral (`ephemeral=True`), so responses are user-private by default. #### Style & Conventions No issues found. Module follows the cog-based pattern used throughout the project. Pure helper functions at module level are testable without Discord infrastructure. Docstrings are present and follow existing patterns. The out-of-scope removal of the dead `import datetime` from `paperdynasty.py` is appropriate and noted in the PR body. #### Suggestions - The `progress` parameter on `refractor_status` accepts any string but only `"close"` has defined behavior. A future improvement would be an `app_commands.choices` constraint or an explicit error response for unrecognized values, though this is not a blocker. - `test_t3_badge_in_format_entry` is not present in the test suite (T3/Gold Refractor badge rendering in formatted output). Minor gap — T1, T2, T4 are covered, T3 is only verified at the constant level. Not a blocker. ### Verdict: APPROVED All four cycle 1 issues are fixed correctly. The implementation is complete, secure, well-tested (49 tests passing), and consistent with the project's cog-based conventions. Ready to merge. --- *Automated review by Claude PR Reviewer (Cycle 2)*
cal approved these changes 2026-03-23 20:11:12 +00:00
cal left a comment
Owner
Copy Link

Approved. Reviewer cycle 2 complete — merging.

Approved. Reviewer cycle 2 complete — merging.
cal added 1 commit 2026-03-23 20:11:23 +00:00
Merge branch 'main' into ai/paper-dynasty-database#76
All checks were successful
Ruff Lint / lint (pull_request) Successful in 21s
Details
c85359ca5d
cal merged commit d2410ab374 into main 2026-03-23 20:11:33 +00:00
cal deleted branch ai/paper-dynasty-database#76 2026-03-23 20:11:33 +00:00
Sign in to join this conversation.
Reviewers
No reviewers
cal
Labels
Clear labels   
ai-changes-requested

   
ai-failed

   
ai-pr-opened

   
ai-reviewed

   
ai-reviewing

   
ai-working

   
ai-working

   
bug

   
enhancement

   
feature

New feature proposal

  
in-queue

Approved for development

  
performance

   
security

   
tech-debt

   
todo
No Label
ai-changes-requested
ai-failed
ai-pr-opened
ai-reviewed
ai-reviewing
ai-working
ai-working
bug
enhancement
feature
in-queue
performance
security
tech-debt
todo
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#87
No description provided.
Delete Branch "ai/paper-dynasty-database#76"

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?