Review: Bug: teams/save appends instead of replacing (CI blocker)

Verdict: NEEDS_REFINEMENT

Template Completeness

• [x] Lineage — standalone, correctly documented as unplanned discovery
• [x] Repo — forgejo_admin/basketball-api
• [x] What Broke / Expected Behavior — substitutes for User Story (appropriate for bug)
• [ ] User Story — absent (acceptable for bug type; What Broke + Expected Behavior serve the same purpose)
• [x] Context — embedded in "What Broke" section with root cause analysis
• [x] File Targets — specific files, line numbers, and all three branches documented
• [x] Test Expectations — run command provided, pass/fail counts specified
• [x] Acceptance Criteria — 5 criteria, all verifiable
• [x] Constraints — scope bounded ("2-line fix"), future design intent documented
• [x] Related — upstream blockers (#170, #172) and migration reference
• [x] Repro Steps — clear numbered steps with expected vs actual
• [x] Environment — CI context and prod impact noted

File Targets

• [x] src/basketball_api/routes/admin.py line 697 — verified: admin_teams_save() function starts at line 697
• [x] src/basketball_api/routes/admin.py lines 749-753 (temp team branch) — verified: player.teams.append(real_team) at line 752, no preceding clear()
• [x] src/basketball_api/routes/admin.py lines 770-773 (existing team branch) — verified: player.teams.append(team) at line 772, no preceding clear()
• [x] src/basketball_api/routes/admin.py lines 732-735 (unassign branch) — verified: player.teams.clear() at line 734, already correct as ticket states
• [x] tests/test_admin_teams.py line 290 — verified: test_temp_team_dedup exists, asserts len(player1.teams) == 1 at line 316
• [x] alembic/versions/019_player_teams_junction.py — verified: migration file exists

Repo Placement

Dependencies

• Blocks: Issue #170 (jersey sync) and PR #172 are blocked by this CI failure — documented in scope.
• Board item #393 ("Baby Betty contradictory jersey state") shares the arch:stripe-webhook label and is also a teams-related bug in backlog — may be related but no explicit dependency documented.
• No items currently in in_progress or next_up that conflict with this fix (the two in_progress items are Phase 14: Billing Tiers and Phase 15: Production Port).

Acceptance Criteria

• [x] player.teams.clear() insertion — grep-verifiable after fix
• [x] test_temp_team_dedup passes — pytest tests/test_admin_teams.py::TestTeamsSave::test_temp_team_dedup -v
• [x] All 18 test_admin_teams.py tests pass — pytest tests/test_admin_teams.py -v
• [x] Full suite (555 tests) — python -m pytest
• [x] CI pipeline succeeds — observable post-merge

Blast Radius

• Source code: Only admin.py uses player.teams.append() in production code. The two locations identified in the ticket are the only append sites. No other routes or services have this pattern.
• Test fixtures: 5 test files use player.teams.append() in fixture setup — these are correct (building test state, not simulating save behavior).
• No downstream consumers: The /admin/teams/save endpoint is called only by the SPA draft board. No other services consume it.
• Idempotency concern: Adding clear() before append() means re-saving the same assignments will clear+re-add. The cids dedup check (lines 750-751, 770-771) becomes redundant after clear() but harmless. Consider whether updated counter logic needs adjustment — currently it only increments when real_team.id not in cids, but after clear() the player will always have 0 teams, so updated will always increment. This is a minor behavioral change but likely acceptable.

Issues Found

• Label mismatch: arch:stripe-webhook is incorrect. This bug is in the /admin/teams/save route (admin.py). There is no Stripe or webhook code involved. The correct architecture label should reflect the admin/draft-board component (e.g., arch:admin-api or arch:draft-board). Board item #393 has the same mislabel.

Recommendation