fix(practice): assistant coaches enter the conflict check; slot-expansion docstring corrected (8.1)

[JoelA510]

Phase 8 task 8.1 (docs/PHASE_8_PLAN.md §8.1). Two defects in the practice scheduler — fixed in the core modules and in the shipped Edge Function path (review rounds 1–2) — one raised-not-fixed item, and the reachability finding that turned into the port.

Closes #362.

What changed

Defect 1 — assistant coaches were invisible to the conflict check.

Core. packages/core/src/practiceScheduling.js exports listTeamCoachIds(team): head coach plus assistants, deduplicated, empty ids dropped; read as assistantCoachIds first, then assistant_coach_ids (the precedence teamSnapshot.js already applies). A missing or null list is "no assistants" (the sibling if (team.coachId) contract); a list that is present and not an array throws a TypeError rather than reading as conflict-free. Every team.coachId site consults every coach:

• practiceScheduling.js — booking, release, the multi-team priority (busiest coach, computed once per team alongside coachIds), the overlap check, and the unavailableSlotIds hard constraint (unioned over all coaches). Preferred days/slots are scoring hints and still follow the head coach — 8.2's one-coach-model work, left open on purpose.
• autoScheduler.js — checkHardConstraints no longer returns valid on !team.coachId; unavailability and overlap loop over team.coachIds, precomputed once per run.
• practiceMetrics.js — coachConflicts is one entry per overlapping pair: coachIds lists every coach the pair shares, coachId the first. computeFitness charges an overlap once whether the pair shares one coach or two; evaluationPipeline.js emits one error issue naming all of them ("Coaches h, s have overlapping practices"). coachLoad still counts each coach. Stated in the JSDoc above the first @param.

Shipped path (rounds 1–2). The app POSTs to supabase/functions/auto-scheduler/index.ts, a Deno port of these modules, and PracticeSchedulingPage.normalizeTeam dropped assistants before the request.

• supabase/functions/_shared/engines/practice-coaches.ts: listTeamCoachIds (same contract as core: camelCase then snake, null/undefined → none, present non-array → throw) and checkHardConstraints; in _shared so a Deno test reaches the seam without importing the serve() module.
• supabase/functions/_shared/schemas/auto-scheduler.ts: the request schema, moved out of index.ts onto the import-map zod so it is testable and composes the one shared TeamSchema (index.ts composed it with its own esm.sh zod, which does not type-check across two zod instances; index.ts used z nowhere else, so that import is gone). assistantCoachIds is z.array(z.string()).nullable().optional() in _shared/schemas/scoring.ts.
• index.ts: every coachId-only site (buildState, mutation add/remove, greedy assign, priority sort) loops team.coachIds, resolved once at input preparation.
• _shared/engines/scoring-engine.ts: mirrors practiceMetrics.js — each coach's assignments sorted, every overlapping pair visited, pairs merged across coaches; a precomputed team.coachIds is honoured, else the helper; reason and issue built after the merge, naming every shared coach.
• frontend/src/pages/PracticeSchedulingPage.jsx normalizeTeam forwards assistantCoachIds: team.assistantCoachIds ?? team.assistant_coach_ids ?? []; exported for its vitest case (3 cases).

Defect 2 — practiceSlotExpansion.js docstring. Chosen: correct the docstring, not implement the adjustment (stated in the commit); implementing it is 8.9. Guarded behaviourally: expanding with and without sunset / lighting / daylight input gives identical output, an honoured input (fewer phases) changes it, and the declared parameter shape names no such input (a fake ({ slots, sunset }) fails the same predicate).

Raised, not fixed: #361 — practice_slots.day_of_week CHECK forbids Friday while the league runs 19 Friday rows. Migration is its own PR with revert + smoke scripts under docs/sql/.

Claims verified

Claim	Result	Actual
practiceScheduling.js and autoScheduler.js consult only team.coachId	HELD (those two files)	every coach path keyed on coachId at the lines listed
No other coach path exists	DID NOT HOLD	practiceMetrics.js; auto-scheduler/index.ts; _shared/engines/scoring-engine.ts; page normalizeTeam; PracticeOverridePanel.jsx (head-coach display names, manual-override UI — untouched). All but the last fixed here.
Corpus: 215 assignments / 132 teams / 196 people	HELD	derived from loadSeason2026() at run time
"Roughly 83 co-coach assignments unseen"	HELD, exactly	non-slot-1 assignments = 83; teams with >1 coach = 82 (81 with two, 1 with three; 0 without a head)
practiceSlotExpansion.js:13 daylight claim; no sunset/daylight/lighting input	HELD	only match was line 13; function takes { slots, seasonPhases }
Migration CHECK at line 501 ('mon','tue','wed','thu'); 19 Friday rows	HELD	enum on line 29 already includes fri; CSV Friday rows = 19; 8.0 loader asserts 19
Assistant spelling the check must read	resolved from the repo	engine/typedef/loader: assistantCoachIds; DB column and mock client: assistant_coach_ids; both helpers read camelCase first, then snake; the request schema declares camelCase (what the page sends) and the snake spelling arrives via passthrough.

Failing-then-passing evidence

Vitest. Six suites with the new tests and core sources reverted to origin/main: 11 failed | 137 passed. Layer isolation: with only autoScheduler.js reverted, 2 failed | 18 passed — the two "relocate cannot seat…" tests, built where fitness favours the clash. Item-4 comparator change (round 1): the processing-order fixture (read off which slot each team receives, since assignments is returned sorted by id) passed identically before and after.

Deno (executed via npx --yes deno@2, 2.9.6, DENO_NO_PACKAGE_JSON=1, remote imports through the proxy):

• deno check on index.ts, practice-coaches.ts, scoring-engine.ts, schemas/auto-scheduler.ts: exactly one error, pre-existing — TS2339 Property 'catch' does not exist on type 'void', now index.ts:566 (:644 on origin/main, same line of code). No new errors.
• deno test _shared/tests/practice-coaches_test.ts: 14 passed after round 2. With the four round-1 sources restored (--no-check): 7 failed | 7 passed — every round-2 control red: snake spelling in the helper; TeamSchema null; request-schema null; issue naming both coaches; three-team merge (T2/T3 = [h, s]); assistant_coach_ids-only row; precomputed coachIds. With scoring-engine.ts + scoring.ts at origin/main (round 1): 2 failed | 6 passed. The three checkHardConstraints cases have no red "before" — the seam is new — and are proven by their in-test controls.
• Pre-existing red, untouched: _shared/tests/scoring-engine_test.ts "coach conflict detection" (line 49) asserts result.coachConflicts[0].reason.includes('Time overlap'). It fails on origin/main and still fails: tests/unit/scoring-engine.test.ts (Vitest, green on main) asserts the same field contains 'overlapping practices', so the two pre-existing tests contradict each other and the main wording is kept. Not made to pass naturally.

Positive controls proven to fail: no-head-coach teams; "distinct assistants" controls; fitness control (two-coach pair equals one-coach pair, both below no-clash); pipeline control (one issue each, zero without a clash); optimizer oracle on a shared-assistant pair and a head/assistant crossover; normalizeTeam control (no raw assistant_coach_ids key survives); docstring test's fake ({ slots, sunset }) and phase-count change; request schema still rejects a non-array; singular rendering for a one-coach pair.

Deliberately left open

• Coach preferences remain head-coach-keyed (8.2).
• PracticeOverridePanel.jsx manual-override check compares head-coach display names.
• The pre-existing deno check error at index.ts:566 and the contradictory pre-existing scoring-engine tests — outside 8.1.
• practice_slots.day_of_week CHECK forbids Friday; the league runs 19 Friday practice rows #361.
• packages/core/package.json main points at a nonexistent src/index.js; no index to register listTeamCoachIds in.

Verification

Test-verified (executed, output captured): npm run test — main baseline 2216 / 34 / 6 → 2242 / 34 / 6 (160 files, +26 tests). Deno suite 14/14 after, 7 red before. All before/after and isolation runs above.

Executed: npm run lint — 0 errors, 1 warning (the useVirtualizer baseline). npm run typecheck — 0 errors. npm run frontend:build — clean. npm run check:advisors — PASS. npm run check:bundle — PASS; both re-run after each Edge Function edit. prettier --check on the TypeScript files — clean. deno check as above. /code-review at high before opening (6 findings, all addressed).

Statically reviewed only: the index.ts booking loops in buildState, the mutation add/remove and the greedy seed — they call the tested seam but the module runs serve() at top level and cannot be imported by a test; the Edge Function needs a deploy to take effect.

🤖 Generated with Claude Code

https://claude.ai/code/session_011ppMMPnorTKrb9ZVaGTpS9

[chatgpt-codex-connector]

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.

[vercel]

The latest updates on your projects. Learn more about Vercel for GitHub.

Project	Deployment	Actions	Updated
squadlogic	Ready	Preview	Sep 5, 2026 7:03am UTC