fix(dev_tools): use unified cards/{id} endpoint for /dev refractor-test #168

Merged

cal merged 2 commits from fix/refractor-test-unified-cards-lookup into main 2026-04-11 17:22:22 +00:00

Conversation 1 Commits 2 Files Changed 2 +268 -144

cal commented 2026-04-11 17:15:32 +00:00

Owner

Copy Link

The previous two-step battingcards→pitchingcards fallback caused card ID
collisions. Example: /dev refractor-test card_id:494 resolved to Cameron
Maybin (battingcards row 494, 2018 Marlins) instead of the intended
pitcher Grayson Rodriguez (pitchingcards row 494, 2023 Orioles).

Changes

• Single db_get("cards", object_id=card_id) lookup; cards.id is
  globally unique across the card-instance table
• Card type derived from player.pos_1: SP → sp, RP/CP → rp,
  else → batter
• team_id sourced from card["team"]["id"] (the card instance's
  owning team); get_team_by_owner fallback removed
• TestRefractorTestSetup rewritten for the single-endpoint contract
  with a unified_card_response fixture factory and pos_1 derivation
  coverage (SP/RP/CP/batter)

Semantic shift

card_id now refers to the globally-unique card-instance ID from the
cards table, not a batting/pitching template ID. For a dev-only
command this is acceptable — instance IDs are already discoverable
via /refractor status.

Tests

• python -m pytest tests/test_dev_tools.py tests/test_refractor_test_data.py
• 29/29 passing; red/green TDD cycle followed

Out of scope

• SENTINEL_PITCHER_ID = 3 opposing-player default — pre-existing, unchanged

Paired with the spec PR in cal/paper-dynasty-umbrella
(fix/refractor-test-unified-cards-lookup).

Spec: docs/superpowers/specs/2026-04-11-refractor-test-unified-cards-lookup-design.md

The previous two-step battingcards→pitchingcards fallback caused card ID collisions. Example: `/dev refractor-test card_id:494` resolved to Cameron Maybin (battingcards row 494, 2018 Marlins) instead of the intended pitcher Grayson Rodriguez (pitchingcards row 494, 2023 Orioles). ## Changes - Single `db_get("cards", object_id=card_id)` lookup; `cards.id` is globally unique across the card-instance table - Card type derived from `player.pos_1`: `SP` → `sp`, `RP`/`CP` → `rp`, else → `batter` - `team_id` sourced from `card["team"]["id"]` (the card instance's owning team); `get_team_by_owner` fallback removed - `TestRefractorTestSetup` rewritten for the single-endpoint contract with a `unified_card_response` fixture factory and pos_1 derivation coverage (SP/RP/CP/batter) ## Semantic shift `card_id` now refers to the globally-unique card-instance ID from the `cards` table, not a batting/pitching template ID. For a dev-only command this is acceptable — instance IDs are already discoverable via `/refractor status`. ## Tests - `python -m pytest tests/test_dev_tools.py tests/test_refractor_test_data.py` - 29/29 passing; red/green TDD cycle followed ## Out of scope - `SENTINEL_PITCHER_ID = 3` opposing-player default — pre-existing, unchanged Paired with the spec PR in `cal/paper-dynasty-umbrella` (`fix/refractor-test-unified-cards-lookup`). Spec: `docs/superpowers/specs/2026-04-11-refractor-test-unified-cards-lookup-design.md`

cal added 1 commit 2026-04-11 17:15:33 +00:00

fix(dev_tools): use unified cards/{id} endpoint for /dev refractor-test ... 1c47928356

The previous two-step battingcards→pitchingcards fallback caused card
ID collisions — e.g. card 494 resolving to Cameron Maybin (batting)
instead of the intended pitcher Grayson Rodriguez. The unified cards
endpoint is keyed on globally-unique card instance IDs and carries
player, team, and variant in a single response.

- Single db_get("cards", object_id=card_id) lookup
- Card type derived from player.pos_1 (SP→sp, RP/CP→rp, else→batter)
- team_id sourced from card["team"]["id"] (no get_team_by_owner fallback)
- TestRefractorTestSetup rewritten for the single-endpoint contract

Spec: docs/superpowers/specs/2026-04-11-refractor-test-unified-cards-lookup-design.md

Claude added the

ai-reviewing

label 2026-04-11 17:15:38 +00:00

Claude reviewed 2026-04-11 17:18:09 +00:00

Claude left a comment

Collaborator

Copy Link

AI Code Review

Files Reviewed

• cogs/dev_tools.py (modified)
• tests/test_dev_tools.py (modified)

Findings

Correctness

No issues found. The core fix is correct:

• db_get("cards", object_id=card_id) replaces the two-step batting/pitching fallback — cards.id is globally unique so no template-ID collision.
• card["player"]["player_id"] is the correct field path for the unified endpoint (differs from the old card["player"]["id"] on template endpoints).
• (card.get("team") or {}).get("id") safely handles team: null cards and correctly gates on a hard error instead of silently falling back to get_team_by_owner.
• card_type_default is always assigned before the if card_state: branch, so it's never used uninitialized.
• AUTH_TOKEN is {"Authorization": "Bearer ..."} (dict) — correct type for aiohttp.ClientSession(headers=...), consistent with api_calls.py usage throughout.

Security

No issues found. card_id is an int parameter from Discord's app_command layer — no injection risk. AUTH_TOKEN is not user-controlled.

Style & Conventions

No issues found. Logger rename to "discord_app" matches the named logger pattern used in other cogs. Structured debug logging added for state-lookup is appropriate for a dev tool.

Suggestions

1. Dead mock in test_successful_batter_flow (unchanged test, broken by this PR's implementation change). The render step now uses aiohttp.ClientSession directly instead of db_get. The existing mock mock_get.return_value = {"image_url": "https://s3.example.com/card.png"} is dead — it's never consumed. The aiohttp call will throw a ClientConnectorError at test time, which is silently caught by the except Exception handler. The test still passes because it only asserts "✅" (from game creation, not render). The render step's success path is effectively untested. Consider either mocking aiohttp in that test or removing the dead mock and adding a comment that the render step is covered by exception-handling only.

2. No ClientTimeout on render call. Every other HTTP call in api_calls.py wraps with ClientTimeout(total=timeout). The new render path omits it — the session could hang indefinitely if the render endpoint is slow. Acceptable for a dev-only tool but worth noting for consistency.

3. embed.set_image() silently removed. The old render path extracted an image URL and called embed.set_image(url=img_url) on success, displaying the freshly-rendered card in the embed. The new path drops this entirely. The PR body doesn't mention the behavior change. Not a blocker for a dev tool, but worth documenting.

Verdict: COMMENT

Core fix is correct, well-motivated (the Cameron Maybin vs Grayson Rodriguez collision example makes the bug clear), and the setup-phase test coverage is thorough (SP/RP/CP/batter derivation, no-team guard, single-endpoint assertion, no-fallback assertion). The suggestions above are non-blocking.

---

Automated review by Claude PR Reviewer

## AI Code Review ### Files Reviewed - `cogs/dev_tools.py` (modified) - `tests/test_dev_tools.py` (modified) ### Findings #### Correctness No issues found. The core fix is correct: - `db_get("cards", object_id=card_id)` replaces the two-step batting/pitching fallback — `cards.id` is globally unique so no template-ID collision. - `card["player"]["player_id"]` is the correct field path for the unified endpoint (differs from the old `card["player"]["id"]` on template endpoints). - `(card.get("team") or {}).get("id")` safely handles `team: null` cards and correctly gates on a hard error instead of silently falling back to `get_team_by_owner`. - `card_type_default` is always assigned before the `if card_state:` branch, so it's never used uninitialized. - `AUTH_TOKEN` is `{"Authorization": "Bearer ..."}` (dict) — correct type for `aiohttp.ClientSession(headers=...)`, consistent with `api_calls.py` usage throughout. #### Security No issues found. `card_id` is an `int` parameter from Discord's app_command layer — no injection risk. `AUTH_TOKEN` is not user-controlled. #### Style & Conventions No issues found. Logger rename to `"discord_app"` matches the named logger pattern used in other cogs. Structured debug logging added for state-lookup is appropriate for a dev tool. #### Suggestions 1. **Dead mock in `test_successful_batter_flow`** (unchanged test, broken by this PR's implementation change). The render step now uses `aiohttp.ClientSession` directly instead of `db_get`. The existing mock `mock_get.return_value = {"image_url": "https://s3.example.com/card.png"}` is dead — it's never consumed. The aiohttp call will throw a `ClientConnectorError` at test time, which is silently caught by the `except Exception` handler. The test still passes because it only asserts `"✅"` (from game creation, not render). The render step's success path is effectively untested. Consider either mocking aiohttp in that test or removing the dead mock and adding a comment that the render step is covered by exception-handling only. 2. **No `ClientTimeout` on render call**. Every other HTTP call in `api_calls.py` wraps with `ClientTimeout(total=timeout)`. The new render path omits it — the session could hang indefinitely if the render endpoint is slow. Acceptable for a dev-only tool but worth noting for consistency. 3. **`embed.set_image()` silently removed**. The old render path extracted an image URL and called `embed.set_image(url=img_url)` on success, displaying the freshly-rendered card in the embed. The new path drops this entirely. The PR body doesn't mention the behavior change. Not a blocker for a dev tool, but worth documenting. ### Verdict: COMMENT Core fix is correct, well-motivated (the Cameron Maybin vs Grayson Rodriguez collision example makes the bug clear), and the setup-phase test coverage is thorough (SP/RP/CP/batter derivation, no-team guard, single-endpoint assertion, no-fallback assertion). The suggestions above are non-blocking. --- *Automated review by Claude PR Reviewer*

cal reviewed 2026-04-11 17:18:21 +00:00

cal left a comment

Author

Owner

Copy Link

AI Code Review

Files Reviewed

• cogs/dev_tools.py (modified)
• tests/test_dev_tools.py (modified)

Findings

Correctness

No issues found.

• Single db_get("cards", object_id=card_id) call with correct keyword arg — matches db_get signature in api_calls.py.
• card["player"]["player_id"] field name correctly matches the unified endpoint's response shape (distinct from the old template tables that used "id").
• (card.get("team") or {}).get("id") safely handles both team=None and a missing "team" key without raising.
• pos_1 derivation (or "" guard + .upper()) correctly handles None and mixed-case values. SP → sp, RP/CP → rp, else → batter matches spec.
• card_type_default is read in the else: card_type = card_type_default branch — the Pyright false-positive is a known non-issue per review scope.
• Render step correctly uses get_req_url(api_ver=2) and AUTH_TOKEN header dict — both verified exports of api_calls.py.
• aiohttp is imported at the top of the file, not lazily mid-function.
• get_team_by_owner fully removed from both usage site and the import line.

Security

No issues found. This cog is only loaded when DATABASE != prod. No user-controlled data reaches SQL or shell. AUTH_TOKEN is sourced from environment, not hardcoded.

Style & Conventions

No issues found. Logger name changed to "discord_app" which aligns with the named-logger pattern used across the project.

Suggestions

Non-blocking observations only:

1. Stale @app_commands.describe string (cogs/dev_tools.py line 111): Still says "The batting or pitching card ID to test" — could be updated to reflect the card-instance ID semantic in a follow-up. Low priority for a dev-only command.

2. Dead mock_get in test_successful_batter_flow (tests/test_dev_tools.py line 427): mock_get.return_value targets the old db_get-based render path. The render step now uses aiohttp directly, so this mock is inert. The test still passes because the aiohttp call is caught by the non-fatal except handler. Consider patching aiohttp.ClientSession here in a follow-up to make the happy-path render assertion meaningful.

Verdict: APPROVED

Clean, targeted fix. The unified endpoint lookup eliminates the template-ID collision correctly, the pos_1 derivation is well-guarded, team extraction handles the None case properly, and the new test suite meaningfully covers all four pos_1 branches plus both error paths. All new imports are verified present in api_calls.py.

(Note: Gitea single-user repos block the APPROVED review state — merge with pd-pr merge --no-approve.)

---

Automated review by Claude PR Reviewer

## AI Code Review ### Files Reviewed - `cogs/dev_tools.py` (modified) - `tests/test_dev_tools.py` (modified) ### Findings #### Correctness No issues found. - Single `db_get("cards", object_id=card_id)` call with correct keyword arg — matches `db_get` signature in `api_calls.py`. - `card["player"]["player_id"]` field name correctly matches the unified endpoint's response shape (distinct from the old template tables that used `"id"`). - `(card.get("team") or {}).get("id")` safely handles both `team=None` and a missing `"team"` key without raising. - `pos_1` derivation (`or ""` guard + `.upper()`) correctly handles None and mixed-case values. SP → sp, RP/CP → rp, else → batter matches spec. - `card_type_default` is read in the `else: card_type = card_type_default` branch — the Pyright false-positive is a known non-issue per review scope. - Render step correctly uses `get_req_url(api_ver=2)` and `AUTH_TOKEN` header dict — both verified exports of `api_calls.py`. - `aiohttp` is imported at the top of the file, not lazily mid-function. - `get_team_by_owner` fully removed from both usage site and the import line. #### Security No issues found. This cog is only loaded when `DATABASE != prod`. No user-controlled data reaches SQL or shell. `AUTH_TOKEN` is sourced from environment, not hardcoded. #### Style & Conventions No issues found. Logger name changed to `"discord_app"` which aligns with the named-logger pattern used across the project. #### Suggestions Non-blocking observations only: 1. **Stale `@app_commands.describe` string** (`cogs/dev_tools.py` line 111): Still says `"The batting or pitching card ID to test"` — could be updated to reflect the card-instance ID semantic in a follow-up. Low priority for a dev-only command. 2. **Dead `mock_get` in `test_successful_batter_flow`** (`tests/test_dev_tools.py` line 427): `mock_get.return_value` targets the old `db_get`-based render path. The render step now uses `aiohttp` directly, so this mock is inert. The test still passes because the aiohttp call is caught by the non-fatal `except` handler. Consider patching `aiohttp.ClientSession` here in a follow-up to make the happy-path render assertion meaningful. ### Verdict: APPROVED Clean, targeted fix. The unified endpoint lookup eliminates the template-ID collision correctly, the pos_1 derivation is well-guarded, team extraction handles the None case properly, and the new test suite meaningfully covers all four pos_1 branches plus both error paths. All new imports are verified present in `api_calls.py`. *(Note: Gitea single-user repos block the APPROVED review state — merge with `pd-pr merge --no-approve`.)* --- *Automated review by Claude PR Reviewer*

Claude removed the

ai-reviewing

label 2026-04-11 17:18:26 +00:00

cal added 1 commit 2026-04-11 17:19:52 +00:00

docs(dev_tools): update refractor-test card_id describe string ... c8424e6cb1

Review follow-up: the @app_commands.describe string still referenced
"batting or pitching card ID" after the switch to the unified cards
endpoint. Update to clarify that card_id is now a card-instance ID.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

cal referenced this pull request 2026-04-11 17:21:47 +00:00

test(dev_tools): refractor-test render step mock is a dead mock #169

cal commented 2026-04-11 17:22:18 +00:00

Author

Owner

Copy Link

Merging after pr-pipeline review. Two reviewer notes:

1. Docstring update (stale @app_commands.describe string) — fixed in commit c8424e6 on this branch.

2. Dead mock in test_successful_batter_flow — pre-existing weakness, deferred to a follow-up issue:
   #169

Pairing spec merged in cal/paper-dynasty-umbrella#7.

Merging after pr-pipeline review. Two reviewer notes: 1. Docstring update (stale @app_commands.describe string) — fixed in commit c8424e6 on this branch. 2. Dead mock in test_successful_batter_flow — pre-existing weakness, deferred to a follow-up issue: https://git.manticorum.com/cal/paper-dynasty-discord/issues/169 Pairing spec merged in cal/paper-dynasty-umbrella#7.

cal merged commit 46c0d1ae1d into main 2026-04-11 17:22:22 +00:00

cal deleted branch fix/refractor-test-unified-cards-lookup 2026-04-11 17:22:22 +00:00

cal referenced this issue from a commit 2026-04-11 17:22:24 +00:00

Merge pull request 'fix(dev_tools): use unified cards/{id} endpoint for /dev refractor-test' (#168) from fix/refractor-test-unified-cards-lookup into main

Sign in to join this conversation.

Reviewers

No reviewers

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

  

tests

Test coverage and test quality

  

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

tests

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

Subscribe

Due Date

The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: cal/paper-dynasty-discord#168

No description provided.