pal-e-docs

Review: Basketball-API client with Keycloak auth

review-597-2026-03-28 Review

review needs-refinement
Project

Verdict: NEEDS_REFINEMENT

Template Completeness

  • [x] Type — Feature
  • [x] Lineage — depends on #4 (scaffold)
  • [x] Repo — forgejo_admin/westside-ai-assistant
  • [x] User Story — clear who/what/why for Marcus as admin
  • [x] Context — explains Keycloak client credentials flow, caching, and typed functions
  • [x] File Targets — specific files to create and modify, plus do-not-touch boundaries
  • [x] Acceptance Criteria — 6 testable conditions
  • [x] Test Expectations — 4 unit tests plus run command
  • [x] Constraints — httpx, internal URL, Keycloak token URL, client-only
  • [x] Checklist — PR, tests, no unrelated changes
  • [x] Related — project page and architecture doc linked

Traceability

  • [x] story:read-ops — read operations for program data
  • [x] story:write-ops — write operations for program data
  • [x] arch:A3 — Basketball-API Client component in domain model
  • [x] Forgejo issue — forgejo_admin/westside-ai-assistant#5, open

File Targets

  • [x] app/basketball.py — to be created. Repo currently only has README.md (scaffold from #4 not yet implemented). Path is consistent with #4's app/ convention. Parent dir will exist after #4 completes.
  • [x] requirements.txt — to be modified. Does not exist yet; depends on #4 creating it. Dependency documented in Lineage.
  • [x] Do-not-touch boundaries clearly stated (basketball-api repo, app/groupme.py).
Targets are specific enough for an agent, but the agent will need the exact HTTP paths for each basketball-api endpoint (see Acceptance Criteria below).

Repo Placement

OK. Issue filed on forgejo_admin/westside-ai-assistant, work targets that repo. basketball-api is consumed, not modified — correctly noted in Constraints. Single-repo scope.

Dependencies

  • [x] #4 (FastAPI scaffold + GroupMe webhook handler) — pending. Board item #596, column: backlog, state: open. This ticket cannot execute until #4 merges. Documented in Lineage.
  • [ ] Keycloak client provisioning — UNDOCUMENTED. The ticket references a westside-ai-bot client in the westside-basketball realm with admin role and service-accounts-enabled. No board item or Forgejo issue covers creating this Keycloak client. Either #1 (Provision infrastructure in pal-e-services) includes it, or a separate ticket is needed. The issue body should state which ticket provisions the Keycloak client.

Acceptance Criteria

6 AC items are testable and specific. However, the AC references function names that don't precisely match basketball-api's actual endpoint paths. An agent implementing blindly from the AC would guess URLs incorrectly. Specific issues:
  • Missing tenant context: Multiple basketball-api endpoints require tenant_id or tenant_slug parameters (e.g., GET /api/teams?tenant_id=X, GET /tenants/{tenant_slug}/roster, POST /api/roster/{tenant_slug}/check-in/{player_id}). The AC and Context sections don't mention how the client resolves tenant identity — hardcoded slug, env var, or per-call parameter.
  • assign_tryout_number is a bulk operation: The actual endpoint POST /tryouts/admin/{tenant_slug}/assign-numbers bulk-assigns numbers to ALL unassigned players. The function name implies single-player assignment.
  • Inconsistent route prefixes: get_roster lives at GET /tenants/{tenant_slug}/roster, checkin_player at POST /api/roster/{tenant_slug}/check-in/{player_id}, and assign_tryout_numbers at POST /tryouts/admin/{tenant_slug}/assign-numbers. These are three different route prefix patterns vs. the /api/ and /admin/ patterns used by most other endpoints.

Blast Radius

Low. This is a new client module in a new repo. No existing code is modified. basketball-api is consumed read-only. The only risk is incorrect endpoint URLs causing runtime failures, which is why the endpoint mapping gaps matter. Rollback is trivial (revert one file).

Decomposition Assessment

1 file to create, 1 file to modify, 6 AC, 4 test expectations, single repo. All work is in one module (app/basketball.py) plus one test file. Fits within the three-thing limit (1 client class, 1 test file, 1 requirements update). Estimated agent time: 3-4 minutes. No decomposition needed.

Recommendation

  • [BODY] Add a Constraints bullet documenting tenant resolution strategy: hardcoded slug, env var, or per-call parameter.
  • [BODY] Add an endpoint reference table in Context or Constraints listing each function's exact HTTP method + full URL path. Verified paths from basketball-api codebase:
  • [BODY] Rename assign_tryout_number to assign_tryout_numbers (plural) and note it is a bulk trigger, not a per-player operation.
  • [BODY] Add Lineage note clarifying which ticket provisions the Keycloak client westside-ai-bot (likely #1 or needs its own ticket).