baseball(engine): wire event-derived velocity into engine-run/outcome-sweep/action-baseline (#852 residual) - #864
Merged
Conversation
…-sweep/action-baseline (#852 residual) Box-score-migrated players had NO velocity metrics: their legacy exit_velocity/pitch_velocity scalar is dropped alongside superseded legacy GAME rows (engine-stat-rows.ts rule 1), and the canonical box-score tables carry no velocity columns at all. loaders.ts's eventDerived hook (#851) already threaded a per-field event-layer override into loadPlayerMetrics, but nothing called it. Adds src/lib/baseball/coachhelm/engine-event-derived.ts: a team-scoped, paginated read of baseball_pitch_events/baseball_batted_ball_events (#813 superseded-row filter) plus a pure per-player reducer that reuses elite-stat-events.ts's real buildHitterMetrics/buildPitcherMetrics + loaders.ts's eventDerivedVelocityFromMetrics -- never a second, drifting "average exit velocity" implementation. All-or-nothing degrade on read failure, mirroring engine-stat-rows.ts's own honesty rule. Wires it into all three engine callers: - engine-run.ts: full-history event pool -> loadAllPlayerMetrics. - outcome-sweep.ts: event rows filtered to the SAME per-action after-window as the box-score read, so a pre-action event never counts toward did-it-move measurement. - action-baseline.ts: full-history event pool -> the baseline capture. Tests: pure aggregation (mixed hitter/pitcher, zero-event absence, supersede filter, all-or-nothing degrade) plus per-caller wiring tests (event wins over legacy scalar for the same player; a zero-event player keeps their legacy velocity; event-read failure degrades every player to legacy). Extends stat-layer-manifest.ts's grandfathered-consumer allowlist for the new fixture files (legacy baseball_player_stats rows are the fallback pin, not staleness). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01H9QAYqFTKsXGsVw6wXYssa
|
Bugbot is not enabled for your account, so this pull request was not reviewed. Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs. |
|
ⓘ Qodo reviews are paused because your trial has ended. Ask your workspace admin to add credits to resume reviews. Manage billing |
|
The latest updates on your projects. Learn more about Vercel for GitHub. 1 Skipped Deployment
|
|
This pull request has been ignored for the connected project Preview Branches by Supabase. |
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. 🗂️ Base branches to auto review (3)
Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Greptile SummaryCloses the #852 velocity coverage gap for box-score-migrated players whose legacy
Confidence Score: 4/5Safe to merge; the correctness of velocity resolution (event-layer wins, zero-event fallback, all-or-nothing degrade) is fully covered by four new test files and gate-verified against the full suite. The three comments are non-blocking quality notes. The core logic is sound and well-tested. Two comments point to potential runtime cost on active teams:
Important Files Changed
Sequence Diagram%%{init: {'theme': 'neutral'}}%%
sequenceDiagram
participant Caller as Caller (engine-run / outcome-sweep / action-baseline)
participant ESR as loadEngineStatRows
participant LER as loadEngineEventRows
participant BEDB as buildEventDerivedByPlayer / eventDerivedVelocityForPlayer
participant Loaders as loadAllPlayerMetrics / loadPlayerMetrics
Caller->>ESR: teamId, [playerIds]
ESR-->>Caller: statRows (box-score, legacy-reconciled)
Caller->>LER: teamId
Note over LER: baseball_pitch_events + baseball_batted_ball_events WHERE superseded_by_run_id IS NULL
LER-->>Caller: "data: EngineEventRows | null"
alt event read succeeded
Caller->>BEDB: playerIds, pitches, battedBalls
BEDB-->>Caller: "Record<playerId, EventDerivedVelocityInput>"
else event read failed (ALL-OR-NOTHING)
Caller->>Caller: "eventDerivedByPlayer = empty, all players degrade to legacy scalar"
end
Caller->>Loaders: statRows, nowIso, eventDerivedByPlayer
Loaders-->>Caller: PlayerMetrics[] (event-layer velocity wins over legacy scalar)
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
sequenceDiagram
participant Caller as Caller (engine-run / outcome-sweep / action-baseline)
participant ESR as loadEngineStatRows
participant LER as loadEngineEventRows
participant BEDB as buildEventDerivedByPlayer / eventDerivedVelocityForPlayer
participant Loaders as loadAllPlayerMetrics / loadPlayerMetrics
Caller->>ESR: teamId, [playerIds]
ESR-->>Caller: statRows (box-score, legacy-reconciled)
Caller->>LER: teamId
Note over LER: baseball_pitch_events + baseball_batted_ball_events WHERE superseded_by_run_id IS NULL
LER-->>Caller: "data: EngineEventRows | null"
alt event read succeeded
Caller->>BEDB: playerIds, pitches, battedBalls
BEDB-->>Caller: "Record<playerId, EventDerivedVelocityInput>"
else event read failed (ALL-OR-NOTHING)
Caller->>Caller: "eventDerivedByPlayer = empty, all players degrade to legacy scalar"
end
Caller->>Loaders: statRows, nowIso, eventDerivedByPlayer
Loaders-->>Caller: PlayerMetrics[] (event-layer velocity wins over legacy scalar)
Prompt To Fix All With AIFix the following 3 code review issues. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 3
src/lib/baseball/coachhelm/action-baseline.ts:226-229
**Team-wide event fetch for a single-player baseline**
`loadEngineStatRows` on line 218 is scoped to `[playerId]`, so only this one player's box-score rows come over the wire. `loadEngineEventRows` has no equivalent `playerIds` parameter — it fetches and paginates through every non-superseded pitch/batted-ball event on the team. For a college baseball team deep in a season (thousands of pitch events), every action baseline capture now pulls the full team event history to get one player's ~200 rows. The docblock acknowledges this ("mirrors engine-run.ts's deepened-event-catalog read") but engine-run operates over all players simultaneously so the team-wide fetch is justified there; here it is not. A follow-up could add an optional `playerIds` parameter to `loadEngineEventRows` and scope the read with `.or('pitcher_id.eq.{id},batter_id.eq.{id}')` for single-player callers.
### Issue 2 of 3
src/lib/baseball/coachhelm/outcome-sweep.ts:176
**Event-read degradation is silent in all three callers**
When `loadEngineEventRows` fails (e.g. a missing RLS policy on one of the event tables, a schema drift, a PostgREST timeout), `data` is `null` and all players silently fall back to legacy scalars for velocity — exactly the pre-#852 state. This is the correct behavior, but there is no structured log or Datadog event emitted from any of the three callers (`outcome-sweep.ts` here, `action-baseline.ts`, or `engine-run.ts` where `eventRowsErr` is captured but also not logged). A persistent event-read failure would go undetected in production: coaches would see stale velocity data with no error surfaced, and the effectiveness ledger would be silently understating improvement. A single structured log call on the non-null error path — consistent with the server-side logger pattern used elsewhere — would close the observability gap.
### Issue 3 of 3
src/lib/baseball/coachhelm/outcome-sweep.ts:205-216
**Per-action O(todo × events) scan vs. the pre-grouped Map used in engine-run**
`buildEventDerivedByPlayer` in `engine-run.ts` pre-indexes events into player-keyed Maps in one O(events) pass; per-player lookup is then O(1). Here, `afterPitches` and `afterBattedBalls` are rebuilt by filtering all team event rows on every loop iteration (`O(events)` per action), and `eventDerivedVelocityForPlayer` then scans them again for the player id (`O(events)` again). For a team with 50 open actions and 5,000 pitch/batted-ball events that is ~500,000 filter operations per sweep pass. This is not harmful at current team sizes, but the outcome sweep is also triggered per-game via the postgame path (not only by the nightly Inngest cron), so the cost compounds with game frequency. Pre-grouping the after-window events by player-id — the same Map strategy `buildEventDerivedByPlayer` already implements — would reduce this to O(events + todo).
Reviews (1): Last reviewed commit: "baseball(engine): wire event-derived vel..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
+226
to
+229
| const { data: eventRows } = await loadEngineEventRows(supabase, teamId); | ||
| const eventDerived = eventRows | ||
| ? eventDerivedVelocityForPlayer(playerId, eventRows.pitches, eventRows.battedBalls) | ||
| : null; |
There was a problem hiding this comment.
loadEngineStatRows on line 218 is scoped to [playerId], so only this one player's box-score rows come over the wire. loadEngineEventRows has no equivalent playerIds parameter — it fetches and paginates through every non-superseded pitch/batted-ball event on the team. For a college baseball team deep in a season (thousands of pitch events), every action baseline capture now pulls the full team event history to get one player's ~200 rows. The docblock acknowledges this ("mirrors engine-run.ts's deepened-event-catalog read") but engine-run operates over all players simultaneously so the team-wide fetch is justified there; here it is not. A follow-up could add an optional playerIds parameter to loadEngineEventRows and scope the read with .or('pitcher_id.eq.{id},batter_id.eq.{id}') for single-player callers.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/lib/baseball/coachhelm/action-baseline.ts
Line: 226-229
Comment:
**Team-wide event fetch for a single-player baseline**
`loadEngineStatRows` on line 218 is scoped to `[playerId]`, so only this one player's box-score rows come over the wire. `loadEngineEventRows` has no equivalent `playerIds` parameter — it fetches and paginates through every non-superseded pitch/batted-ball event on the team. For a college baseball team deep in a season (thousands of pitch events), every action baseline capture now pulls the full team event history to get one player's ~200 rows. The docblock acknowledges this ("mirrors engine-run.ts's deepened-event-catalog read") but engine-run operates over all players simultaneously so the team-wide fetch is justified there; here it is not. A follow-up could add an optional `playerIds` parameter to `loadEngineEventRows` and scope the read with `.or('pitcher_id.eq.{id},batter_id.eq.{id}')` for single-player callers.
How can I resolve this? If you propose a fix, please make it concise.
| // eventRows null, so every action's `eventDerivedForPlayer` below resolves | ||
| // to no event data (legacy scalar fallback for every player this pass) -- | ||
| // never a partial event/legacy blend (mirrors loadEngineStatRows's own rule). | ||
| const { data: eventRows }: { data: EngineEventRows | null } = await loadEngineEventRows(supabase, teamId); |
There was a problem hiding this comment.
When loadEngineEventRows fails (e.g. a missing RLS policy on one of the event tables, a schema drift, a PostgREST timeout), data is null and all players silently fall back to legacy scalars for velocity — exactly the pre-#852 state. This is the correct behavior, but there is no structured log or Datadog event emitted from any of the three callers (outcome-sweep.ts here, action-baseline.ts, or engine-run.ts where eventRowsErr is captured but also not logged). A persistent event-read failure would go undetected in production: coaches would see stale velocity data with no error surfaced, and the effectiveness ledger would be silently understating improvement. A single structured log call on the non-null error path — consistent with the server-side logger pattern used elsewhere — would close the observability gap.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/lib/baseball/coachhelm/outcome-sweep.ts
Line: 176
Comment:
**Event-read degradation is silent in all three callers**
When `loadEngineEventRows` fails (e.g. a missing RLS policy on one of the event tables, a schema drift, a PostgREST timeout), `data` is `null` and all players silently fall back to legacy scalars for velocity — exactly the pre-#852 state. This is the correct behavior, but there is no structured log or Datadog event emitted from any of the three callers (`outcome-sweep.ts` here, `action-baseline.ts`, or `engine-run.ts` where `eventRowsErr` is captured but also not logged). A persistent event-read failure would go undetected in production: coaches would see stale velocity data with no error surfaced, and the effectiveness ledger would be silently understating improvement. A single structured log call on the non-null error path — consistent with the server-side logger pattern used elsewhere — would close the observability gap.
How can I resolve this? If you propose a fix, please make it concise.
Comment on lines
+205
to
+216
| const afterPitches = eventRows | ||
| ? createdAt | ||
| ? eventRows.pitches.filter((p) => !!p.measured_at && p.measured_at > createdAt) | ||
| : eventRows.pitches | ||
| : []; | ||
| const afterBattedBalls = eventRows | ||
| ? createdAt | ||
| ? eventRows.battedBalls.filter((b) => !!b.measured_at && b.measured_at > createdAt) | ||
| : eventRows.battedBalls | ||
| : []; | ||
| const eventDerivedForPlayer = eventRows | ||
| ? eventDerivedVelocityForPlayer(a.player_id!, afterPitches, afterBattedBalls) |
There was a problem hiding this comment.
buildEventDerivedByPlayer in engine-run.ts pre-indexes events into player-keyed Maps in one O(events) pass; per-player lookup is then O(1). Here, afterPitches and afterBattedBalls are rebuilt by filtering all team event rows on every loop iteration (O(events) per action), and eventDerivedVelocityForPlayer then scans them again for the player id (O(events) again). For a team with 50 open actions and 5,000 pitch/batted-ball events that is ~500,000 filter operations per sweep pass. This is not harmful at current team sizes, but the outcome sweep is also triggered per-game via the postgame path (not only by the nightly Inngest cron), so the cost compounds with game frequency. Pre-grouping the after-window events by player-id — the same Map strategy buildEventDerivedByPlayer already implements — would reduce this to O(events + todo).
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/lib/baseball/coachhelm/outcome-sweep.ts
Line: 205-216
Comment:
**Per-action O(todo × events) scan vs. the pre-grouped Map used in engine-run**
`buildEventDerivedByPlayer` in `engine-run.ts` pre-indexes events into player-keyed Maps in one O(events) pass; per-player lookup is then O(1). Here, `afterPitches` and `afterBattedBalls` are rebuilt by filtering all team event rows on every loop iteration (`O(events)` per action), and `eventDerivedVelocityForPlayer` then scans them again for the player id (`O(events)` again). For a team with 50 open actions and 5,000 pitch/batted-ball events that is ~500,000 filter operations per sweep pass. This is not harmful at current team sizes, but the outcome sweep is also triggered per-game via the postgame path (not only by the nightly Inngest cron), so the cost compounds with game frequency. Pre-grouping the after-window events by player-id — the same Map strategy `buildEventDerivedByPlayer` already implements — would reduce this to O(events + todo).
How can I resolve this? If you propose a fix, please make it concise.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
…locity-eventderived
…Size honesty (PR #864 fix-first) Two adversarial-review criticals on #864: 1. buildActionOutcomeSeed (action-baseline.ts) fired a TEAM-WIDE, unbounded, player-unscoped read of the entire pitch/batted-ball event history on every coach "convert to action" click, just to resolve ONE player's velocity scalar. loadEngineEventRows now takes an optional `playerIds` scope (`.in('pitcher_id'|'batter_id', playerIds)`, mirroring loadEngineStatRows's own `.in('player_id', playerIds)` idiom) — the single-player caller passes `[playerId]`; engine-run/outcome-sweep now pass their own already-computed roster/todo player-id lists instead of reading the whole team's history. 2. avg_exit_velocity's sampleSize was `bbCount` (every batted ball) instead of the count of rows that actually carried a non-null exit_velocity reading — inflating the honesty gate for any team whose batted-ball capture doesn't always log a radar reading. Fixed to `battedBalls.filter(b => b.exit_velocity != null).length`, and applied the same fix to the sibling avg_launch_angle metric (identical bug, same line shape). Pitcher avg_velocity was already correct. Tests: pin the DB-level player scoping (loadEngineEventRows + a buildActionOutcomeSeed integration check), and pin the sampleSize fix (10 batted balls / 4 readings -> sampleSize 4; independent launch_angle gating; hard_hit_rate's bbCount-based denominator unaffected). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
njrini99-code
pushed a commit
that referenced
this pull request
Jul 16, 2026
…ze counts (CodeRabbit #868) avg_exit_velocity/avg_launch_angle (hitting) and avg_velocity (pitching) each correctly narrow sampleSize to rows with an actual non-null reading, but still passed the FULL bbProv/pProv array (every batted ball / pitch, hand-charted or radar-read) into dominantTrust/dominantContext. A majority of hand-charted, no-reading rows could drag trustTier down to 'unverified' even when every row that fed the average was 'official' radar data. Pass the same `.filter(reading != null)` array as provenance in all three call sites. Extends the #864 sampleSize-honesty suite with mixed-trust regression tests (few official radar rows + many unverified hand-charted rows -> trustTier must reflect only the radar rows) for the batting and pitching paths. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01H9QAYqFTKsXGsVw6wXYssa
njrini99-code
added a commit
that referenced
this pull request
Jul 16, 2026
…al-audit infra, de-vibe wave 2a, public-page motion fix (#868) * devibe: remove dead files — knip batch 1/2 (mode-toggle, notifications, insight-actions) (#858) Verified dead via grep (import path + symbol name + next/dynamic scan), then git rm. No consumers found in src/, no test coverage, no dynamic imports referencing any of these paths. - src/components/baseball/coach/ModeToggle.tsx — exports JUCOModeToggle, zero importers repo-wide. Only referenced from stale docs (PHASE_5_JUCO_COACH.md, .helm/ACTIONS.md) describing a wiring into src/components/layout/header.tsx, which no longer exists. - src/components/layout/mode-toggle.tsx — exports ModeToggle/Mode, its only consumer was the dead file above. - src/components/features/notification-center.tsx — duplicate/legacy NotificationCenter; the live one is src/components/golf/calendar/NotificationCenter.tsx. .taskmaster/docs/current-state.md already flagged it "Exists but not used". - src/hooks/use-notifications.ts — duplicate/legacy useNotifications; the live hook is src/hooks/useNotifications.ts (capital N), consumed by the real NotificationCenter. - src/components/golf/coachhelm/insights/{InsightBulkActions,InsightExportModal, InsightFiltersPanel,InsightSearchBar}.tsx — not exported from the insights/ barrel (index.ts only re-exports PlayerFocusAreas/InsightsFeed/InsightListView per its "Wave 1A" comment), zero direct importers, no next/dynamic references. - src/lib/baseball/lifting/use-live-set-sync.ts — exports useLiveSetSync, zero importers; only mentioned in docs/audits (planned-but-never-wired). Gates: typecheck clean, check-cycles clean (33 known cycles, none new), no test files reference any of these paths. Co-authored-by: Fable Integrator <fable@helm.local> Co-authored-by: Claude Fable 5 <noreply@anthropic.com> * devibe: remove dead files — knip batch 2/2 (golf/travel legacy, soreness barrel, lift-programs) (#859) Verified dead via grep (import path + symbol name + next/dynamic scan), then git rm. - src/components/golf/travel/{ExpenseForm,ExpenseList,ExpenseSummary,index}.ts(x) — legacy pre-Fairway components. Superseded by src/components/fairway/pages/travel/ Fairway{ExpenseForm,ExpenseList,ExpenseSummary}.tsx, whose own header comments say they're re-skins of "the legacy golf/travel ExpenseList/ExpenseSummary" — i.e. the legacy files are explicitly documented as replaced. Zero live importers (grep for the barrel path and each symbol name comes back empty outside the legacy files themselves). - src/components/lifting/soreness/index.ts — barrel; zero importers (every other file in the same directory — BodySilhouetteFront, SorenessCheckCard, SorenessBodyMap, HighPrioritySorenessList, SorenessScheduleBuilder — IS imported directly by app code, just never through this barrel). - src/components/lifting/soreness/SorenessComplianceBoard.tsx, TeamSorenessHeatmap.tsx — only referenced from the dead barrel above; no direct importers. - src/lib/baseball/read-models/lift-programs.ts — exports getLiftProgramList/ getLiftProgramTree/getAssignContext. The live /performance/programs/[programId] page defines its own local getAssignContext (duplicated, not imported from here) — confirms this read-model was built but never wired in. Gates: typecheck clean, check-cycles clean (33 known cycles, none new). `grep` false-positive check: src/app/golf/actions/__tests__/travel.test.ts matches "ExpenseSummary" only via the substring in getExpenseSummary() (a server action, unrelated file) — ran that suite standalone to confirm (128 passed, 4 skipped, unaffected). Co-authored-by: Fable Integrator <fable@helm.local> Co-authored-by: Claude Fable 5 <noreply@anthropic.com> * devibe: remove orphaned root scaffolding (.taskmaster, .full-stack-feature, stray App Store Connect snapshots) (#860) - .taskmaster/ (9 tracked files: README, config.json, docs/current-state.md, docs/feature-checklist.md, docs/prd.txt, logs/.gitkeep, state.json, tasks/tasks.json, templates/task-template.json) — task-master scaffolding from an abandoned tool integration. Only appears elsewhere as ignore-list entries (.gitignore:76-77), never read by any script/workflow/package.json script. Zero functional references. - .full-stack-feature/ (2 tracked files: 01-requirements.md, state.json) — same pattern: only appears as ignore-list entries across .gitignore, .coderabbitignore, .coderabbit.yaml, .vercelignore, .greptile/config.json, .greptile/rules.md (all just telling other tools to skip the directory). Zero functional references. - full-snapshot.yml, full-snapshot2.yml, app-info-snapshot.yml, age-ratings-snapshot.yml — accessibility-tree/DOM snapshots of the App Store Connect web UI (not fastlane config — there is no fastlane/ directory anywhere in this repo, which uses Xcode Cloud, not fastlane). Zero script or CI references (grepped scripts/, tools/, .github/, .circleci/ — nothing reads these paths). The one doc mention (docs/operations/2026-05-28-coderabbit-fails-investigation.md) explicitly calls age-ratings-snapshot.yml "INHERITED NOISE" causing ~200 yamllint indentation errors and recommends "delete it if it's truly unused" — it is. review-gate.yml's yamllint job only lints *changed* files in a PR diff, so these aren't continuously failing CI, but they're pure accidental commits (browser-automation output) with zero purpose in the repo. - context7.json — does not exist (only context7.json.example is tracked; the real context7.json was already removed in a prior commit 6a9b565 "fix(security): stop tracking context7.json (contained leaked API key)"). Nothing to do here. Gates: typecheck clean, check-cycles clean (33 known cycles, none new). Co-authored-by: Fable Integrator <fable@helm.local> Co-authored-by: Claude Fable 5 <noreply@anthropic.com> * devibe: console triage — remove debug-leftover console.log in use-service-worker (#861) Audited the 77 console.log/debug/warn call sites in prod src (excluding tests). Two mechanisms make almost all of them deliberate, not vibe-coded leftovers, and this PR documents why nearly everything was kept: - next.config.mjs compiler.removeConsole strips console.log AND console.debug from production builds, excluding only 'error'/'warn'. So every console.log/.debug call is already dev-only/no-op in prod. - src/instrumentation.ts + src/instrumentation-client.ts both configure Sentry.consoleLoggingIntegration({ levels: ['log','warn','error'] }) — console.warn is the established, load-bearing structured-logging idiom in this codebase (forwarded to Sentry Explore → Logs), which is exactly why admin-tracer-data.ts has an explicit comment: "console.warn used (not console.log) because production build strips console.log." Reviewed every one of the 48 console.warn and 8 console.debug call sites individually: every single one has either an explicit comment justifying the log level (e.g. insight-delivery.ts's transient-fetch debug downgrade, useAdminPresence.ts's `if (process.env.NODE_ENV !== 'production')`-gated join/leave debug logs, pattern-miner.ts's documented severity policy, admin-logger.ts's PGRST205 once-only warn) or is a genuine production security/error signal (auth rate-limiting, unauthorized message/team actions, fetch-failure fallbacks). None were genuine leftovers — all kept as-is, no logger-idiom conversion performed (see below). **Deleted** (1 file, 8 statements): src/hooks/golf/use-service-worker.ts — 8 console.log calls tracing every SW lifecycle branch (register no-op, already-registered, registered, unregistered, update complete, sync unsupported, sync registered, no active worker to message, message received). Unlike every kept call site above, these had (a) no explanatory comment, (b) no dev-only guard, (c) duplicate state already exposed via the hook's own return value (`status`/`isRegistered`/ `hasUpdate`), and (d) trace literally every branch including plain early returns — the classic "log every branch while debugging a tricky SW bug" pattern (see memory: dev-SW false-offline investigation) never cleaned up. The 5 console.error calls in this same file's catch blocks are untouched (KEEP per the task rule). **Logger-idiom conversion**: grepped for a logger util first (src/lib/admin-logger.ts, server-error-logger.ts, error-logging.ts exist) — none is a general-purpose console.warn replacement; they're purpose-built for the admin_events audit trail / Sentry error classification, and console.warn already IS the repo's structured-log idiom for this class of signal (per the Sentry consoleLoggingIntegration wiring above). Converting would be redundant double-logging and risk semantic changes (async logger calls dropped into sync catch blocks) for no observability gain, so no conversions were made — warns left as-is, per the "if none, leave warns" instruction. Gates: typecheck clean, eslint --max-warnings 0 on the touched file clean, check-cycles clean (33 known cycles, none new). No test file covers this hook (grepped for use-service-worker in *.test.*/*.spec.* — zero hits). Co-authored-by: Fable Integrator <fable@helm.local> Co-authored-by: Claude Fable 5 <noreply@anthropic.com> * Build the /baseball public marketing page (was a bare redirect) (#865) Signed-out visitors used to get bounced straight to /baseball/login with zero context; they now see a real front door — hero, four editorial feature sections (roster/team-ops, stats center, recruiting pipeline, player passport) composed from the Living Annual kit in ghost/placeholder state (no fabricated screenshots or invented player data), and an honest CTA row (Sign in / Create a program / Join with a code). Signed-in visitors keep the exact prior redirect-to-dashboard behavior. - src/app/baseball/page.tsx: rewritten from a bare redirect into the full marketing page; auth check now only fires the redirect when a session exists. - src/components/baseball/marketing/BaseballMarketingMotionScope.tsx: new tiny 'use client' LazyMotion wrapper — the Living Annual atoms used here (RuledStatLine/Masthead/HairlineRule/GradeStamp) never transition off their hidden variant without a loaded feature bundle, and the page itself stays a Server Component (async session check + redirect), so this is the one client boundary. - src/app/baseball/join/page.tsx: new — the "Join with a code" CTA needed a real destination; only the dynamic /baseball/join/[code] existed. Mirrors GolfHelm's /golf/join code-entry page, themed in the Living Annual paper/ink system instead of golf's glass-orb auth chrome. - src/components/landing/Footer.tsx: generalized the shared cross-product footer's tagline off golf-only wording ("college golf") since it now also renders under a BaseballHelm hero. - src/app/baseball/__tests__/page.test.tsx: pins the redirect/no-redirect branching (coach session, player session, signed-out). Co-authored-by: Fable Integrator <fable@helm.local> Co-authored-by: Claude Fable 5 <noreply@anthropic.com> * Fix invisible names/numerals on public baseball profile pages (no LazyMotion ancestor) (#866) team/[id], player/[id] (via PlayerProfileClient), program/[id], and packet/[token] sit in the (public) route group, whose layout was a bare `<>{children}</>` — no LazyMotion anywhere upstream. team/[id] and PlayerProfileClient render Living Annual `m`-based atoms (Masthead, RuledStatLine, HairlineRule) directly; their `inkSettles`/`rulesDraw` entrance variants start at `hidden` (opacity: 0 / scaleX: 0) and only animate to `visible` once framer-motion's feature bundle is loaded via a `LazyMotion` ancestor. Without one, an `m.*` component's AnimationFeature never mounts, so the hidden variant is terminal for any visitor without `prefers-reduced-motion` on — player/team names and stat numerals stayed invisible on these live public recruiting pages. Adds PublicMotionScope (mirrors the existing AdminMotionProvider / `(dashboard)/dashboard/template.tsx` pattern already used elsewhere in the repo) and mounts it from `(public)/layout.tsx`, which stays a Server Component — the LazyMotion boundary lives in the client child. Verified via a real (unmocked) framer-motion render test: Masthead's surname text is measurably opacity: 0 forever with no wrapper, and measurably transitions off 0 once PublicMotionScope loads its feature bundle — the same computed-opacity check `toBeVisible()` uses, so it reproduces the actual bug and the actual fix rather than a mocked stand-in. program/[id] and packet/[token] don't currently render any Living Annual `m` atoms directly (packet's ScoutPacketView already carries its own LazyMotion) — the shared layout-level provider covers them defensively against regression as those pages grow. PR #865 (open, targets this same base) adds a near-identical BaseballMarketingMotionScope for the separate /baseball marketing root and explicitly flagged this (public) route group gap out of its own scope; this PR is the fix for that flagged gap. Not touching #865's files — noted in the PR body that the two wrappers could be consolidated into one shared component later. Co-authored-by: Fable Integrator <fable@helm.local> Co-authored-by: Claude Fable 5 <noreply@anthropic.com> * Add production visual-audit screenshot crawl (GHA, manual-only) (#867) New e2e/visual-audit.spec.ts mirrors baseball-route-crawler.spec.ts's proven live-DOM nav discovery (FairwaySidebar + hub-sub-nav <nav> links) and best-effort public-sample-link discovery, but captures full-page screenshots at phone (390x844) and desktop (1440x900) viewports for every discovered coach/player route plus signed-out publics, instead of asserting route health. Screenshots are data capture, not assertions — the spec only fails on a login failure or a total navigation failure. Gated behind VISUAL_AUDIT=1 (test.skip otherwise); playwright.config.ts's chromium project now ignores it and baseball-coach/baseball-player now match it, so it never runs in the ordinary e2e lane and playwright.yml/ci.yml (which name their spec files explicitly) never pick it up. New .github/workflows/visual-audit.yml runs it via workflow_dispatch against a chosen base_url (default prod), --project=baseball-coach --project=baseball-player only — verified against the installed Playwright runner source that this also runs the `setup` project's full baseball auth (both roles) as a dependency, without needing an explicit --project=setup, and without ever touching Golf's auth.setup.ts. Uploads test-results/visual-audit as visual-audit-<run_number>, if: always(). Claude-Session: https://claude.ai/code/session_01H9QAYqFTKsXGsVw6wXYssa Co-authored-by: Fable Integrator <fable@helm.local> Co-authored-by: Claude Fable 5 <noreply@anthropic.com> * db(baseball): write #379 legacy stats backfill migration (pending Nick's go) (#862) * db(baseball): write #379 legacy stats backfill migration (pending Nick's go) One-time, NOT-APPLIED migration that copies legacy baseball_player_stats 'game' rows into baseball_box_score_batting/_pitching + synthesizes shared baseball_games rows, scoped to teams with ZERO existing box-score data (teams already on the box-score adapter path are never touched). Deterministic ids (SHA-1, RFC4122-v5-shaped, own namespace) mirror #827's scripts/seed-baseball-stats.mjs detId() pattern so re-applying is a no-op and rollback can recompute — not just look up — exactly which rows are ours. Copy-only: legacy rows are never mutated. Deliberately skips recalculate_baseball_season_stats() to avoid clobbering any pre-existing season_totals-imported baseline on baseball_player_season_stats — documented as an opt-in follow-up instead. Exercised end-to-end against a disposable local Postgres 16 instance (schema mirrored from the real migrations, never any shared project) covering a two-way partial-innings player, a duplicate-row collision, an already-box-score team (excluded), and a pre-existing-scheduled-game collision (date skipped) — verified idempotent re-run and a dry-run rollback recompute+delete. See docs/baseball/legacy-backfill-runbook.md for the check-first queries, apply steps, and rollback recipe. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01H9QAYqFTKsXGsVw6wXYssa * fix(baseball): make #379 backfill's season-stats safety story true, not just written Adversarial review on PR #862 found the migration's core safety claim false: recalculate_baseball_season_stats() is described as a deliberate, manual, opt-in, per-team step, but the already-shipped save_baseball_full_box_score RPC calls it automatically on every ordinary box-score save. Since the backfilled games carry their real historical game_date (plausibly within the current season year for teams whose whole history predates #827), the very next normal game entry for an overlapping player would silently overwrite baseball_player_season_stats -- including any pre-existing season_totals baseline -- with no opt-in and no signoff. Fix, verified against a disposable local Postgres 16 instance (never any shared Supabase project): - Migration: add Step 4, seeding baseball_player_season_stats for exactly the (player_id, team_id, season_year) triples the migration's own box-score rows touch, using the identical aggregation/rate formulas recalculate_baseball_season_stats() uses -- guarded by ON CONFLICT ... DO NOTHING so a pre-existing row (e.g. a season_totals baseline) is never touched, preserving copy-only/additive-only/idempotent. Where no row existed, the eventual live recalc now lands on the same numbers already seeded (a no-op, not a surprise). - Runbook: replace the "deliberately out of scope" framing with the true story, add a pre-flight query that surfaces exactly which triples still carry pre-existing-baseline risk (Nick must review before applying), and add a diff-based season-stats rollback procedure since DO NOTHING rows have no deterministic id to recompute against. Locally reproduced the exact scenario the review described (a fresh ordinary game save via the real, unmodified RPC): the seeded player's row extended cleanly with correct math; the pre-existing baseline player's row was silently overwritten by the (unmodified) live RPC, exactly as newly documented -- confirming the fix and the doc are both now accurate. File remains WRITE-ONLY / NOT APPLIED pending Nick's go-ahead. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> --------- Co-authored-by: Fable Integrator <fable@helm.local> Co-authored-by: Claude Fable 5 <noreply@anthropic.com> * baseball(engine): wire event-derived velocity into engine-run/outcome-sweep/action-baseline (#852 residual) (#864) * baseball(engine): wire event-derived velocity into engine-run/outcome-sweep/action-baseline (#852 residual) Box-score-migrated players had NO velocity metrics: their legacy exit_velocity/pitch_velocity scalar is dropped alongside superseded legacy GAME rows (engine-stat-rows.ts rule 1), and the canonical box-score tables carry no velocity columns at all. loaders.ts's eventDerived hook (#851) already threaded a per-field event-layer override into loadPlayerMetrics, but nothing called it. Adds src/lib/baseball/coachhelm/engine-event-derived.ts: a team-scoped, paginated read of baseball_pitch_events/baseball_batted_ball_events (#813 superseded-row filter) plus a pure per-player reducer that reuses elite-stat-events.ts's real buildHitterMetrics/buildPitcherMetrics + loaders.ts's eventDerivedVelocityFromMetrics -- never a second, drifting "average exit velocity" implementation. All-or-nothing degrade on read failure, mirroring engine-stat-rows.ts's own honesty rule. Wires it into all three engine callers: - engine-run.ts: full-history event pool -> loadAllPlayerMetrics. - outcome-sweep.ts: event rows filtered to the SAME per-action after-window as the box-score read, so a pre-action event never counts toward did-it-move measurement. - action-baseline.ts: full-history event pool -> the baseline capture. Tests: pure aggregation (mixed hitter/pitcher, zero-event absence, supersede filter, all-or-nothing degrade) plus per-caller wiring tests (event wins over legacy scalar for the same player; a zero-event player keeps their legacy velocity; event-read failure degrades every player to legacy). Extends stat-layer-manifest.ts's grandfathered-consumer allowlist for the new fixture files (legacy baseball_player_stats rows are the fallback pin, not staleness). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01H9QAYqFTKsXGsVw6wXYssa * fix(baseball): bound velocity event read to player scope + fix sampleSize honesty (PR #864 fix-first) Two adversarial-review criticals on #864: 1. buildActionOutcomeSeed (action-baseline.ts) fired a TEAM-WIDE, unbounded, player-unscoped read of the entire pitch/batted-ball event history on every coach "convert to action" click, just to resolve ONE player's velocity scalar. loadEngineEventRows now takes an optional `playerIds` scope (`.in('pitcher_id'|'batter_id', playerIds)`, mirroring loadEngineStatRows's own `.in('player_id', playerIds)` idiom) — the single-player caller passes `[playerId]`; engine-run/outcome-sweep now pass their own already-computed roster/todo player-id lists instead of reading the whole team's history. 2. avg_exit_velocity's sampleSize was `bbCount` (every batted ball) instead of the count of rows that actually carried a non-null exit_velocity reading — inflating the honesty gate for any team whose batted-ball capture doesn't always log a radar reading. Fixed to `battedBalls.filter(b => b.exit_velocity != null).length`, and applied the same fix to the sibling avg_launch_angle metric (identical bug, same line shape). Pitcher avg_velocity was already correct. Tests: pin the DB-level player scoping (loadEngineEventRows + a buildActionOutcomeSeed integration check), and pin the sampleSize fix (10 batted balls / 4 readings -> sampleSize 4; independent launch_angle gating; hard_hit_rate's bbCount-based denominator unaffected). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> --------- Co-authored-by: Fable Integrator <fable@helm.local> Co-authored-by: Claude Fable 5 <noreply@anthropic.com> * Consolidate stats-upload wizard into Import Center (canonical) (#863) * Consolidate stats-upload wizard into Import Center (canonical) Audited both wizards end-to-end (§3.11 decision: Import Center is canonical). Ported the two real capability gaps before retiring the legacy path — everything else (atomic save_baseball_full_box_score RPC, player-match corrections, dedup/provenance/rollback) was already covered by Import Center's commitImport pipeline, so nothing else needed porting: - ImportWizardClient: added a "Quick box score" entry point on the choose step (preselects game_box_score + jumps straight to Upload) plus drag-and-drop onto the dropzone and a sample-values data-preview table on the detect step — the legacy wizard's two capabilities Import Center didn't have. No server-action signatures changed. - /dashboard/stats/upload is now a pure redirect into /dashboard/import, mirroring the stats -> stats-center legacy-redirect shim idiom. Sibling error.tsx/loading.tsx removed (that idiom has neither). - Retired the now-fully-orphaned StatsUploadClient/UploadHistory components (only ever imported by the old page). - Repointed the two in-app links that still pointed at the legacy route (Command Center's "Upload stats", Stats Center's header) straight at Import Center, and dropped Stats Center's redundant "Upload" button (Import Center already sat right next to it, same destination). - Test migration: extended settings-aliases-and-legacy-redirects.test.ts with the new shim, added ImportWizardClient.quick-box-score.test.tsx for the two ported capabilities, and updated the e2e assertion that pinned the retired wizard's UI strings to assert the redirect instead. nav-registry.ts (frozen) still lists /baseball/dashboard/stats/upload in stats-center's matchPrefixes and STAFF_CAPABILITY_ROUTES/GUARD_ALLOWLIST still gate it at can_manage_stats — both harmless now (a plain redirect page, still resolves on disk, destination re-enforces can_manage_imports itself) but flagging for the orchestrator in case a follow-up wants them tidied. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01H9QAYqFTKsXGsVw6wXYssa * Fix wizard-consolidation capability lockout + restore upload history (PR #863) Adversarial review (FIX_FIRST) flagged two criticals in the stats-upload -> Import Center consolidation: 1. CAPABILITY LOCKOUT — the /stats/upload redirect shim + the two repointed CTAs sent every viewer straight at Import Center's can_manage_imports gate, locking out every default staff role that holds can_manage_stats but not can_manage_imports (assistant/pitching/hitting/catching/defensive/strength coach — 6 of 11 canonical BASEBALL_STAFF_ROLE_PRESETS). Those roles could reach and interact with the old wizard before this consolidation. Fix: /stats/upload now branches on capability instead of redirecting unconditionally. can_manage_imports staff still forward to the full Import Center; can_manage_stats-only staff get the SAME ImportWizardClient rendered inline, restricted to the "Quick box score" entry point (new quickEntryOnly prop — skips the choose step and hides the "change data shape" affordance, no way to reach the full shape picker/event-level mode/source registry/rollback reserved for can_manage_imports staff). Middleware's STAFF_CAPABILITY_ROUTES already allowlists this exact route at can_manage_stats, so no middleware/nav-registry contract change was needed. Command Center's "Upload stats" and Stats Center's two CTAs are repointed from /dashboard/import back to /dashboard/stats/upload so every entry point resolves through the capability-aware router. 2. UPLOAD HISTORY DELETED — UploadHistory.tsx was the only surface reading baseball_stat_uploads (filename/status/processed counts); its deletion left every pre-consolidation upload record permanently unviewable. Fix: ported a read-only "Legacy uploads" section into ImportWizardClient (Living Annual idiom: Eyebrow/HairlineRule/EditorsLetter honest empty state, matching the existing "Recent imports" section), backed by getRecentUploads — an existing, already-demoSafe, already-team-scoped server action with zero prior callers. No server-action signature changes. Wired into both the full Import Center page and the new capability-aware /stats/upload entry point. Also extracted the roster-for-matching query (previously inlined in import/page.tsx) into a shared src/lib/baseball/import-roster.ts helper so both pages load player-matching data identically instead of drifting. Gates: typecheck clean, eslint --max-warnings 0 clean on all touched files, targeted + broader baseball vitest suites green (1178 tests), check-cycles clean (33 known cycles, none new). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01H9QAYqFTKsXGsVw6wXYssa * fix(stats-center): route import entry points by viewer capability The two Import Center entry points (header action + empty-state CTA) sent everyone through the /stats/upload shim, whose middleware gate is can_manage_stats — bouncing import-capable-but-not-stats staff (e.g. the director_ops preset) off middleware before the shim's own capability branch could forward them. The page now computes can_manage_imports server-side (same helper the shim branches on) and import-capable viewers go straight to /dashboard/import; everyone else keeps the shim path. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01H9QAYqFTKsXGsVw6wXYssa * fix(baseball-import): authorize stats-only staff for box-score import commit/preview (PR #863 round-4) previewImport/commitImport were hard-gated to can_manage_imports unconditionally, so the quickEntryOnly inline wizard at /stats/upload (rendered for the 6 can_manage_stats-only staff presets) let a stats-only coach fill out the whole form and then fail server-side on submit. Pre-consolidation, stats-only staff could upload box scores via the legacy wizard, so restore that: a 'game_box_score' request may now be authorized by can_manage_imports OR can_manage_stats; every other shape (season_totals, event_log, or omitted) keeps the original can_manage_imports-only gate. - with-baseball-action.ts: requiredCapability now also accepts a readonly array (ANY-of) or a resolver function of the action's own args, resolved once before AUTH so tags/metadata and enforcement can never disagree. Single-capability call sites (~60 existing) resolve to a one-element list and behave byte-identically to before. - imports.ts: previewImport gained an optional dataShape field (mirroring CommitImportArgs.dataShape) so the same shape-conditional gate applies at preview time too; both actions resolve the OR-gate from the exact field applyImportPlan uses for canonical-table routing, so the auth decision and the write decision can never diverge. - ImportWizardClient.tsx: pass dataShape through to previewImport, and hide the Upload step's "Back to choose" button for quickEntryOnly viewers (it routed to the full shape picker Import Center reserves for can_manage_imports staff). - New suite (imports-capability-shape-gate.test.ts) exercises the real withBaseballAction/capabilities wiring (not a passthrough mock) to prove: stats-only + game_box_score authorizes and actually writes; stats-only + season_totals still throws BaseballCapabilityError with zero side effects; no-capability staff still denied; imports-only staff unchanged across every shape. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01H9QAYqFTKsXGsVw6wXYssa --------- Co-authored-by: Fable Integrator <fable@helm.local> Co-authored-by: Claude Fable 5 <noreply@anthropic.com> * ci(visual-audit): two spaces before inline version comments (yamllint strict) Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01H9QAYqFTKsXGsVw6wXYssa * fix(migration): qualify digest() as extensions.digest — pgcrypto is not in public The 42883 failure reproduced on the CI fresh-stack replay and would have occurred identically on prod at apply time: pgcrypto lives in the extensions schema in both environments. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01H9QAYqFTKsXGsVw6wXYssa * db(baseball): manifest-based rollback + concurrency lock for #379 backfill (CodeRabbit #868) - Copy-only summary now lists Step 4's baseball_player_season_stats write (finding 1). - Add permanent, service-role-only baseball_legacy_backfill_manifest ledger (RLS enabled, anon/authenticated revoked); every Step 1-4 INSERT records its own RETURNING rows into it, same transaction, tagged with a run_tag. Rollback now joins against the manifest instead of recomputing deterministic ids from current (possibly-changed) baseball_player_stats, and the runbook's rollback + season-stats-rollback sections are rewritten around manifest-join DELETEs. Verified recalculate_baseball_season_stats() does a full from-scratch rebuild (not an incremental merge) before writing the "safe to delete" rollback caveat (finding 2). - Take an explicit LOCK TABLE ... IN SHARE ROW EXCLUSIVE MODE on all 5 read/written tables before the eligibility snapshot; runbook gains an apply- window note. Confirmed SHARE ROW EXCLUSIVE cannot self-conflict with this migration's own later INSERTs (finding 9). - Rename the two TEMP TABLEs to the required baseball_ prefix, all references (finding 10). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01H9QAYqFTKsXGsVw6wXYssa * fix(baseball): validate invite code is alphanumeric before router.push (CodeRabbit #868) The hint text promises "letters and numbers" but only length was checked, letting URI-breaking characters (?, #, /) reach router.push(`/baseball/join/${trimmed}`). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01H9QAYqFTKsXGsVw6wXYssa * fix(baseball): suppressHydrationWarning on legacy-upload created_at cell (CodeRabbit #868) toLocaleDateString() formats with the server's locale/timezone during SSR but the browser's on hydration, risking a mismatch warning. Matches this repo's existing suppressHydrationWarning-on-the-enclosing-element precedent (LocalTime.tsx, RelativeTime.tsx, Fairway calendar/announcements components). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01H9QAYqFTKsXGsVw6wXYssa * fix(baseball): hide Stats Center import actions for staff with neither capability (CodeRabbit #868) canManageImports=false conflated stats-capable staff (routed through the /stats/upload shim) with staff holding NEITHER can_manage_imports nor can_manage_stats, whom both routes would just bounce off their own middleware gate. page.tsx now Promise.all's a second hasBaseballCapability call for can_manage_stats and passes both down; StatsCenterClient renders the header "Import Center" action and the empty-state "Import a box score" CTA only when canManageImports || canManageStats holds, keeping the existing importEntryHref branch for the visible cases. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01H9QAYqFTKsXGsVw6wXYssa * fix(baseball): filter provenance to the reading-bearing rows sampleSize counts (CodeRabbit #868) avg_exit_velocity/avg_launch_angle (hitting) and avg_velocity (pitching) each correctly narrow sampleSize to rows with an actual non-null reading, but still passed the FULL bbProv/pProv array (every batted ball / pitch, hand-charted or radar-read) into dominantTrust/dominantContext. A majority of hand-charted, no-reading rows could drag trustTier down to 'unverified' even when every row that fed the average was 'official' radar data. Pass the same `.filter(reading != null)` array as provenance in all three call sites. Extends the #864 sampleSize-honesty suite with mixed-trust regression tests (few official radar rows + many unverified hand-charted rows -> trustTier must reflect only the radar rows) for the batting and pitching paths. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01H9QAYqFTKsXGsVw6wXYssa * fix(baseball): resolve capability requirement inside guarded flow + reject empty results (CodeRabbit #868) Two related fixes to withBaseballAction: - The (possibly args-conditional) requiredCapability resolver ran BEFORE Sentry.withScope/the wrapper's own try/catch even started, so a throwing resolver (e.g. a malformed/omitted argument) threw raw and unsanitized, skipping AUTH, Sentry, and logServerException entirely. Resolution now happens inside the guarded try/catch, right after AUTH resolves and before capability enforcement — a throwing resolver now produces the same sanitized BaseballActionError + Sentry-logged path as any other action failure. Still resolved exactly once, from the same args reference; tags/ breadcrumbs are set from the resolved value immediately afterward. - requiredCapability's array forms are now typed as non-empty tuples (readonly [BaseballCapability, ...BaseballCapability[]]) so `[]` is a compile-time error, and a resolver that manufactures an empty array at runtime anyway is rejected with a thrown BaseballCapabilityError (fail closed) instead of falling through to `resolvedCapabilityList[-1]` === undefined being passed to requireBaseballCapability. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01H9QAYqFTKsXGsVw6wXYssa --------- Co-authored-by: Fable Integrator <fable@helm.local> Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Closes the #852 residual velocity coverage gap: box-score-migrated players had no velocity metrics in the CoachHelm engine. Their legacy
exit_velocity/pitch_velocityscalar is dropped alongside superseded legacy GAME rows (engine-stat-rows.tsprecedence rule 1), and the canonical box-score tables (baseball_box_score_batting/_pitching) carry no velocity columns at all.loaders.ts'seventDerivedhook (#851) already threaded a per-field event-layer override intoloadPlayerMetrics/loadAllPlayerMetrics, but nothing ever called it — so those players'avg_exit_velocity/max_exit_velocity/avg_pitch_velocity/max_pitch_velocitymetrics silently went dark.Changes
New:
src/lib/baseball/coachhelm/engine-event-derived.tsloadEngineEventRows(db, teamId)— team-scoped, paginated (fetchAllRowsResult, PostgREST 1000-row cap) read ofbaseball_pitch_events/baseball_batted_ball_events, filtered to the fix(baseball): paginate elite-stat-event reads past the 1000-row PostgREST cap #813 current (non-superseded) rows only (superseded_by_run_id IS NULL). All-or-nothing: either table failing returnsdata: null, mirroringloadEngineStatRows's own honesty rule.eventDerivedVelocityForPlayer/buildEventDerivedByPlayer— pure per-player reducers that reuseelite-stat-events.ts's realbuildHitterMetrics/buildPitcherMetrics+loaders.ts'seventDerivedVelocityFromMetrics, so the honesty-gated average here is byte-identical to what the Stats Center already shows — never a second, drifting "average exit velocity" implementation.Wired into all three engine callers:
engine-run.ts— full-history event pool →loadAllPlayerMetrics's neweventDerivedByPlayerparam.outcome-sweep.ts— event rows filtered to the same per-action after-window as the box-score read (measured_at > created_at), so a pre-action event can never count toward "did it move" measurement.action-baseline.ts— full-history event pool → the baseline capture (a baseline is "current value at conversion time", matching the box-score read's own no-date-filter semantics).Manifest: extended
stat-layer-manifest.ts'sGRANDFATHERED_CONSUMERSallowlist for the 3 new wiring-test fixture files (they seed fakebaseball_player_statsrows as the legacy-fallback pin, not staleness) — required by the existingstat-layer-contract.test.tsenforcement.Test fixtures: added a real (not no-op)
.is()passthrough to the existing custom fake-client builders inoutcome-sweep-insight-resolve.test.tsandaction-baseline.test.ts, since both callers now query with.is('superseded_by_run_id', null).Tests
engine-event-derived.test.ts— pure aggregation: hitter-side/pitcher-side isolation (no cross-player leak), two-way player gets both independently, zero-event honest absence, fix(baseball): paginate elite-stat-event reads past the 1000-row PostgREST cap #813 supersede filter, all-or-nothing degrade on read failure.engine-run-event-velocity.test.ts— event-derived avg exit velocity wins over the legacy scalar for the same player; a zero-event player keeps their legacy scalar; event-read failure degrades every player back to legacy (fixed clock).outcome-sweep-event-velocity.test.ts— same win/fallback pattern, scoped to the per-action after-window (a pre-action event is proven excluded).action-baseline-event-velocity.test.ts— same win/fallback pattern for baseline capture.Gate evidence
npm run typecheck— clean.npx eslint --max-warnings 0 <touched files>— clean.npx vitest run --project unit --project integration --project rls src/lib/baseball src/lib/coachhelm/baseball src/app/baseball— 408 files / 3836 tests passed.npm run test:business— 535 files / 5251 passed, 14 skipped (no failures).node scripts/check-cycles.mjs— OK, 33 known cycles, none new.Not merging — leaving for review per batch process.
Co-Authored-By: Claude Fable 5 noreply@anthropic.com
Claude-Session: https://claude.ai/code/session_01H9QAYqFTKsXGsVw6wXYssa