fix(baseball): CoachHelm engine loaders/registry — #379 Phase 4a

[njrini99-code]

Summary

Implements the design chunk "CoachHelm engine Phase 4a — loaders + registry" from the #379 stats-layer reconciliation design, building on #827 (seed/demo reconciliation) and #828 (shared legacy-stat-adapters.ts), both already on batch/bbh-finish-0714.

Base-branch verification (per task instructions)

• git log origin/batch/bbh-finish-0714 --oneline | grep -i clock → found dcaa59fb fix(baseball): thread engine's nowIso through rolling-window loaders (#811), folded in via ff54f8a5 merge main (post #808/#811) into batch/bbh-finish-0714. No merge needed — the base branch already has fix(baseball): thread engine's nowIso through rolling-window loaders #811's nowIso threading through loadLiftMetrics/loadReadinessMetrics/loadPlayerMetrics/loadCatchingMetrics. Confirmed nowIso stays the 3rd positional param with its existing default in loadPlayerMetrics/loadAllPlayerMetrics — every new parameter is appended after it.

What changed (exactly the chunk's 5 files, nothing else)

• src/lib/coachhelm/baseball/loaders.ts
  • Added optional per-row hittingSourceTable / pitchingSourceTable tags on BoxScoreRow, honestly cited in source_refs instead of the hardcoded 'baseball_player_stats' literal. Two new pure normalizers — normalizeBoxScoreBattingRow / normalizeBoxScorePitchingRow — map canonical baseball_box_score_batting/_pitching rows into the loader's existing internal shape (same math, different input), ready for the Phase 4b callers to adopt.
  • Added an optional eventDerived input (appended after nowIso, so it's fully backward compatible) that lets exit/pitch velocity metrics prefer real event-grain data from elite-stat-events.ts over the legacy scalar columns — per field, never blended, never fabricated. eventDerivedVelocityFromMetrics() extracts avg_exit_velocity / avg_velocity from that module's DerivedMetric[] output (no max_* event metric exists yet, so max fields honestly keep falling back to the legacy scalar).
  • Math is untouched for every existing caller: all 4 current call sites (engine-run.ts, outcome-sweep.ts, action-baseline.ts, practice-effectiveness.ts — none touched by this PR, all Phase 4b/2 concerns) call with the old 3-arg signature and get byte-identical output — verified by running their existing test suites unchanged (39/39 passing).
• src/lib/coachhelm/baseball/metrics/registry.ts — header doc comment no longer asserts baseball_player_stats/baseball_player_aggregates as the metrics' only source (loaders.ts can now source some fields canonically); zero remaining references to the deprecated tables in this file. Metric IDs/metadata (direction, fidelity, thresholds) are unchanged.
• src/lib/coachhelm/baseball/engine-v10.test.ts — new describe('loaders #379 reconciliation...') block covering: legacy-fallback behavior unchanged, event-derived override wins per-field, box-score-normalizer source-table threading, eventDerivedVelocityFromMetrics extraction (present + absent cases), and loadAllPlayerMetrics's nowIso positional preservation + per-player event map threading.
• src/lib/coachhelm/baseball/metrics/registry.role-visibility.test.ts — fixture table: literal switched from 'baseball_player_stats' to 'baseball_box_score_batting' (unrelated to the test's actual assertions).
• src/lib/baseball/stat-layer-manifest.ts — removed the now-stale registry.ts entry (file no longer references a deprecated table) and the registry.role-visibility.test.ts entry (fixture migrated); updated loaders.ts's note to describe the new additive capabilities; restored engine-v10.test.ts's entry (still legitimately pins the legacy-fallback path in its new tests).

Gates

• npm run typecheck → clean, exit 0.
• npx eslint --max-warnings 0 on all 5 touched files → clean, exit 0.
• npx vitest run --project unit src/lib/coachhelm/baseball/engine-v10.test.ts src/lib/coachhelm/baseball/metrics/registry.role-visibility.test.ts → 27/27 passing.
• Backward-compat sanity (not required by the file list, run anyway since loaders.ts is shared): full src/lib/coachhelm/baseball suite (57/57), plus the 4 external caller test files (action-baseline.test.ts, engine-run-coach-triage.test.ts, outcome-sweep-insight-resolve.test.ts, practice-effectiveness.test.ts, upload-stats-csv.test.ts) — all 39/39 passing unchanged.
• src/lib/baseball/__tests__/stat-layer-contract.test.ts (business project) — ran before/after via git stash A/B on this exact branch: identical failure set both times (3 pre-existing offending files + 1 pre-existing stale entry, none touched by this PR — see Deferred below). This PR introduces zero new offenders and zero new stale entries to that contract test.

Deferred (pre-existing, out of scope for this chunk)

stat-layer-contract.test.ts was already red on batch/bbh-finish-0714 before this PR (confirmed via git stash):

• New unlisted offenders: src/app/baseball/actions/__tests__/practice-effectiveness.test.ts, src/contracts/baseball/access/player-today-self-scope.test.ts, src/contracts/baseball/product-trust/player-today-honest-loop.test.ts reference a deprecated table without a manifest entry.
• Stale entry: src/app/baseball/actions/insights.ts no longer references a deprecated table at all (it appears to have migrated in fix(baseball): migrate discover.ts/insights.ts actions onto withBaseballAction (#394) #819 — fix(baseball): migrate discover.ts/insights.ts actions onto withBaseballAction (#394) — without removing its manifest entry in that commit).

Neither is in this chunk's file list (loaders.ts, registry.ts, its two test files, the manifest); fixing them belongs to whichever chunk owns insights.ts/practice-effectiveness.ts/player-today.ts (Phase 2/3 in the #379 design) or a standalone manifest-hygiene fix. Flagging here so it isn't lost.

🤖 Generated with Claude Code

[cursor]

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-code-review]

ⓘ Qodo reviews are paused because your trial has ended. Ask your workspace admin to add credits to resume reviews. Manage billing

[vercel]

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

1 Skipped Deployment

Project	Deployment	Actions	Updated (UTC)
helmv3	Ignored		Jul 15, 2026 8:16am

[supabase]

This pull request has been ignored for the connected project qmnssrrolpinvwjjnufo because there are no changes detected in supabase directory. You can change this behaviour in Project Integrations Settings ↗︎.

---

Preview Branches by Supabase.
Learn more about Supabase Branching ↗︎.

[coderabbitai]

Important

Review skipped

Auto reviews are disabled on base/target branches other than the default branch.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 382be660-7a66-4cdb-a097-ef7f806078d5

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

• 🔍 Trigger review

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch task/379-engine-4a

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[greptile-apps]

Greptile Summary

Phase 4a of the #379 stats-layer reconciliation: wires honest per-row source provenance and optional event-derived velocity overrides into loaders.ts, without touching any existing call site's output. All math is unchanged for the four unmigrated callers; the new inputs are strictly additive and backward-compatible.

• loaders.ts gains two box-score normalizers, source-table routing helpers, and an event-derived velocity path that wins per-field over legacy scalars without blending.
• registry.ts doc comment no longer asserts the deprecated flat tables as the only source; metric IDs, directions, and thresholds are untouched.
• engine-v10.test.ts adds 6 new cases covering legacy-fallback preservation, per-field event-derived override, normalizer source threading, and absence handling.
• stat-layer-manifest.ts correctly removes registry.ts and registry.role-visibility.test.ts entries while keeping loaders.ts and engine-v10.test.ts grandfathered.

Confidence Score: 4/5

Safe to merge for Phase 4a. No existing caller behavior changes. The two concerns are forward-looking footguns for Phase 4b adopters, not current defects.

The additive design is solid and tests thoroughly cover the new paths. Two concerns matter for Phase 4b: the source-table routing helpers pick the first tagged row via .find() so a mixed batch silently produces wrong provenance with no guard; and normalizeBoxScorePitchingRow produces rows without pitch_velocity, so a Phase 4b caller adopting the normalizer without wiring eventDerived loses velocity metrics silently. Neither is reachable today.

src/lib/coachhelm/baseball/loaders.ts — specifically hittingSourceTableFor/pitchingSourceTableFor (first-match assumption) and normalizeBoxScorePitchingRow (missing pitch_velocity callout in JSDoc).

Important Files Changed

Filename	Overview
src/lib/coachhelm/baseball/loaders.ts	Core change: adds optional hittingSourceTable/pitchingSourceTable tagging, two box-score normalizers, and an eventDerived velocity path. Backward-compatible; all existing callers unaffected. Two concerns: first-match source table resolution silently mislabels mixed batches, and normalizeBoxScorePitchingRow drops pitch_velocity silently.
src/lib/coachhelm/baseball/engine-v10.test.ts	Adds new describe block with solid coverage. The metric() helper's source_refs table was changed to baseball_box_score_batting, creating a semantic mismatch with the new legacy-fallback test cases in the same file.
src/lib/coachhelm/baseball/metrics/registry.ts	Doc-comment-only change: removes hardcoded deprecated table references from the header. No functional code changed; metric IDs, directions, fidelity levels, and thresholds are all untouched.
src/lib/coachhelm/baseball/metrics/registry.role-visibility.test.ts	Fixture table in the local ref() helper changed from baseball_player_stats to baseball_box_score_batting. The test's actual assertions (player_eligible boolean checks) are unaffected.
src/lib/baseball/stat-layer-manifest.ts	Removes registry.ts and registry.role-visibility.test.ts entries, updates loaders.ts note, and keeps engine-v10.test.ts grandfathered. Manifest hygiene is correct.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    subgraph Callers["Phase 4a Callers (unchanged)"]
        C1["engine-run.ts / outcome-sweep.ts / action-baseline.ts / practice-effectiveness.ts"]
    end
    subgraph Phase4b["Phase 4b Callers (future)"]
        C2["normalizeBoxScoreBattingRow() / normalizeBoxScorePitchingRow()"]
        C3["eventDerivedVelocityFromMetrics() from elite-stat-events.ts"]
    end
    subgraph Loader["loadPlayerMetrics / loadAllPlayerMetrics"]
        SR["hittingSourceTableFor / pitchingSourceTableFor"]
        EV_PATH["eventDerived velocity fields"]
        LEG_PATH["Legacy scalar fallback"]
    end
    subgraph Output["source_refs emitted"]
        T1["baseball_player_stats"]
        T2["baseball_box_score_batting / _pitching"]
        T3["baseball_batted_ball_events / baseball_pitch_events"]
    end
    C1 -->|"3-arg call, no tags"| Loader
    C2 -->|"tagged rows"| SR
    C3 -->|"EventDerivedVelocityInput"| EV_PATH
    SR -->|"no tag"| T1
    SR -->|"tag found"| T2
    EV_PATH -->|"value present"| T3
    EV_PATH -->|"null"| LEG_PATH
    LEG_PATH --> T1

Loading

%%{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"}}}%%
flowchart TD
    subgraph Callers["Phase 4a Callers (unchanged)"]
        C1["engine-run.ts / outcome-sweep.ts / action-baseline.ts / practice-effectiveness.ts"]
    end
    subgraph Phase4b["Phase 4b Callers (future)"]
        C2["normalizeBoxScoreBattingRow() / normalizeBoxScorePitchingRow()"]
        C3["eventDerivedVelocityFromMetrics() from elite-stat-events.ts"]
    end
    subgraph Loader["loadPlayerMetrics / loadAllPlayerMetrics"]
        SR["hittingSourceTableFor / pitchingSourceTableFor"]
        EV_PATH["eventDerived velocity fields"]
        LEG_PATH["Legacy scalar fallback"]
    end
    subgraph Output["source_refs emitted"]
        T1["baseball_player_stats"]
        T2["baseball_box_score_batting / _pitching"]
        T3["baseball_batted_ball_events / baseball_pitch_events"]
    end
    C1 -->|"3-arg call, no tags"| Loader
    C2 -->|"tagged rows"| SR
    C3 -->|"EventDerivedVelocityInput"| EV_PATH
    SR -->|"no tag"| T1
    SR -->|"tag found"| T2
    EV_PATH -->|"value present"| T3
    EV_PATH -->|"null"| LEG_PATH
    LEG_PATH --> T1

Loading

Prompt To Fix All With AI

Fix the following 3 code review issues. Work through them one at a time, proposing concise fixes.

---

### Issue 1 of 3
src/lib/coachhelm/baseball/loaders.ts:455-459
**First-match source resolution silently mislabels mixed batches**

`hittingSourceTableFor` / `pitchingSourceTableFor` call `.find()` and return the first tagged row's table. When a Phase 4b caller accidentally passes a mixed batch — some rows normalized from `baseball_box_score_batting` (tagged), some from the legacy fetch (untagged) — every stat's `source_ref` will claim `baseball_box_score_batting`, even though a portion of the underlying data still came from `baseball_player_stats`. The "honest source refs" invariant the loader promises would be silently violated.

The helper's contract (all rows in a batch should share the same source) is nowhere documented or asserted. A cheap guard — logging a warning or throwing when more than one distinct `hittingSourceTable` is found among the rows — would surface the misuse immediately during Phase 4b migration rather than producing quietly wrong provenance in production.

### Issue 2 of 3
src/lib/coachhelm/baseball/loaders.ts:174-200
**`normalizeBoxScorePitchingRow` silently drops pitch velocity metrics**

The normalizer's input shape has no `pitch_velocity` field (correctly — `baseball_box_score_pitching` is a box-score aggregate, not pitch-event grain). As a result, rows produced by this function never populate `BoxScoreRow.pitch_velocity`, so `pvRows = pitchRows.filter(r => r.pitch_velocity != null)` will always be empty for a migrated caller. Unless `eventDerived.avgPitchVelocity` is explicitly supplied, `avg_pitch_velocity` and `max_pitch_velocity` are **silently absent** for that player.

A Phase 4b caller who adopts `normalizeBoxScorePitchingRow` without also wiring up `eventDerived` would see pitch velocity disappear from the loader output — different behavior from the legacy path — with no error or warning. The JSDoc doesn't mention this. A one-line note would prevent the surprise.

### Issue 3 of 3
src/lib/coachhelm/baseball/engine-v10.test.ts:68-72
**`metric()` fixture table contradicts the new legacy-fallback test cases**

The helper now hard-codes `table: 'baseball_box_score_batting'` as the source ref for inputs to the generator tests. But the new loader tests added in this same file (lines 139–145) explicitly verify that an unmigrated caller gets `'baseball_player_stats'`. A developer reading the file sees both: the generator fixtures claim canonical-table provenance, while the loader tests document the opposite for today's reality. Since generators don't validate the source ref table the inconsistency is harmless at runtime, but it makes the file's intent harder to read and could mislead someone debugging a provenance chain.

_{Reviews (1): Last reviewed commit: "fix(baseball): CoachHelm engine loaders/..." | Re-trigger Greptile}

[greptile-apps]

First-match source resolution silently mislabels mixed batches

hittingSourceTableFor / pitchingSourceTableFor call .find() and return the first tagged row's table. When a Phase 4b caller accidentally passes a mixed batch — some rows normalized from baseball_box_score_batting (tagged), some from the legacy fetch (untagged) — every stat's source_ref will claim baseball_box_score_batting, even though a portion of the underlying data still came from baseball_player_stats. The "honest source refs" invariant the loader promises would be silently violated.

The helper's contract (all rows in a batch should share the same source) is nowhere documented or asserted. A cheap guard — logging a warning or throwing when more than one distinct hittingSourceTable is found among the rows — would surface the misuse immediately during Phase 4b migration rather than producing quietly wrong provenance in production.

Prompt To Fix With AI

This is a comment left during a code review.
Path: src/lib/coachhelm/baseball/loaders.ts
Line: 455-459

Comment:
**First-match source resolution silently mislabels mixed batches**

`hittingSourceTableFor` / `pitchingSourceTableFor` call `.find()` and return the first tagged row's table. When a Phase 4b caller accidentally passes a mixed batch — some rows normalized from `baseball_box_score_batting` (tagged), some from the legacy fetch (untagged) — every stat's `source_ref` will claim `baseball_box_score_batting`, even though a portion of the underlying data still came from `baseball_player_stats`. The "honest source refs" invariant the loader promises would be silently violated.

The helper's contract (all rows in a batch should share the same source) is nowhere documented or asserted. A cheap guard — logging a warning or throwing when more than one distinct `hittingSourceTable` is found among the rows — would surface the misuse immediately during Phase 4b migration rather than producing quietly wrong provenance in production.

How can I resolve this? If you propose a fix, please make it concise.

[greptile-apps]

normalizeBoxScorePitchingRow silently drops pitch velocity metrics

The normalizer's input shape has no pitch_velocity field (correctly — baseball_box_score_pitching is a box-score aggregate, not pitch-event grain). As a result, rows produced by this function never populate BoxScoreRow.pitch_velocity, so pvRows = pitchRows.filter(r => r.pitch_velocity != null) will always be empty for a migrated caller. Unless eventDerived.avgPitchVelocity is explicitly supplied, avg_pitch_velocity and max_pitch_velocity are silently absent for that player.

A Phase 4b caller who adopts normalizeBoxScorePitchingRow without also wiring up eventDerived would see pitch velocity disappear from the loader output — different behavior from the legacy path — with no error or warning. The JSDoc doesn't mention this. A one-line note would prevent the surprise.

Prompt To Fix With AI

This is a comment left during a code review.
Path: src/lib/coachhelm/baseball/loaders.ts
Line: 174-200

Comment:
**`normalizeBoxScorePitchingRow` silently drops pitch velocity metrics**

The normalizer's input shape has no `pitch_velocity` field (correctly — `baseball_box_score_pitching` is a box-score aggregate, not pitch-event grain). As a result, rows produced by this function never populate `BoxScoreRow.pitch_velocity`, so `pvRows = pitchRows.filter(r => r.pitch_velocity != null)` will always be empty for a migrated caller. Unless `eventDerived.avgPitchVelocity` is explicitly supplied, `avg_pitch_velocity` and `max_pitch_velocity` are **silently absent** for that player.

A Phase 4b caller who adopts `normalizeBoxScorePitchingRow` without also wiring up `eventDerived` would see pitch velocity disappear from the loader output — different behavior from the legacy path — with no error or warning. The JSDoc doesn't mention this. A one-line note would prevent the surprise.

How can I resolve this? If you propose a fix, please make it concise.