fix(baseball): thread engine's nowIso through rolling-window loaders - #811
Conversation
Root cause of the intermittently-failing engine-run-helm-lifting test
(and a latent correctness gap that would eventually hit every rolling-
window metric the same way): loadLiftMetrics, loadReadinessMetrics,
loadPlayerMetrics' workload calc, and loadCatchingMetrics all computed
their cutoff from Date.now() — the real wall clock — instead of the
nowIso already threaded through runBaseballEngineCore for deterministic
runs. A test fixing "now" at a past date and seeding data just inside a
14-day window would pass only as long as real time hadn't yet carried
that window past the seeded date; it then fails with zero code changes
once it did (confirmed: passed on this exact branch 3 days before it
started failing, purely from the calendar advancing).
Threaded nowIso through the full call chain instead of just the one
function the failing test touches: mergeV10PlayerMetrics -> {
loadReadinessMetrics, loadLiftMetrics }, loadAllPlayerMetrics ->
loadPlayerMetrics, mergeEventPlayerMetrics -> loadCatchingMetrics, with
engine-run.ts's three call sites now passing its own nowIso through.
Each function's new nowIso param defaults to new Date().toISOString()
(matching the existing input.now ?? new Date() pattern in
effectiveness/engine.ts) so the three OTHER callers of loadPlayerMetrics
(practice-effectiveness.ts, action-baseline.ts, outcome-sweep.ts) keep
their current real-time behavior unchanged — only the deterministic
engine-run path needed fixing.
Left out of scope (same class, but a bigger architectural change and
not the cause of any current failure): generators/v10.ts's
importQualityGenerator also filters recent import runs via Date.now(),
but BaseballV10EngineInputs has no now/nowIso field for generators at
all today — adding one is a separate, larger PR.
Verified: full `npm run test:run` (unit project, 494 files/4921 tests)
and `npm run test:business` (business project, 505 files/4996 tests)
both pass clean — these are the exact commands CI's "Unit tests" and
"Business contracts" jobs run, and the ones that were failing on
PR #808 before this fix.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
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. |
|
This pull request has been ignored for the connected project Preview Branches by Supabase. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
Summary by CodeRabbit
WalkthroughThe change threads ChangesDeterministic metric windows
Estimated code review effort: 2 (Simple) | ~10 minutes 🚥 Pre-merge checks | ✅ 12✅ Passed checks (12 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ast-grep (0.44.1)ast-grep could not parse rule config: /ast-grep-rules/../git/.coderabbit/ast-grep/no-explicit-any.yml 🔧 ESLint
src/lib/baseball/coachhelm/engine-run.tsESLint skipped: missing config or dependency (missing-dependency). The ESLint configuration references a package that is not available in the sandbox. src/lib/coachhelm/baseball/loaders-events.tsESLint skipped: the ESLint configuration for this file references a package that is not available in the sandbox. src/lib/coachhelm/baseball/loaders-v10.tsESLint skipped: the ESLint configuration for this file references a package that is not available in the sandbox.
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 SummaryFixes a wall-clock leak in four rolling-window loaders that caused deterministic engine-run tests to fail silently as real time advanced past the fixed test clock. All affected loaders now accept a
Confidence Score: 5/5Safe to merge. The change is a minimal, well-scoped threading of an existing clock parameter through four loader functions with no logic mutations. Every rolling-window cutoff that previously called Date.now() in the deterministic engine path now uses Date.parse(nowIso). The three real-time callers outside the engine are unaffected — they omit the new parameter and get new Date().toISOString() as the default. The default parameter is evaluated at call time so there is no stale-clock risk. No new logic, no schema changes, no RLS surface added. No files require special attention. The generators/v10.ts wall-clock gap acknowledged in the PR description is out of scope here and warrants a separate follow-up. Important Files Changed
Sequence Diagram%%{init: {'theme': 'neutral'}}%%
sequenceDiagram
participant E as engine-run.ts
participant LA as loadAllPlayerMetrics
participant V10 as mergeV10PlayerMetrics
participant EV as mergeEventPlayerMetrics
Note over E: nowIso = context.nowIso ?? new Date().toISOString()
E->>LA: loadAllPlayerMetrics(playerIds, rows, nowIso)
LA-->>E: "pitching workload cutoff = Date.parse(nowIso) - 7d"
E->>V10: mergeV10PlayerMetrics(p, readiness, sessions, setResults, nowIso)
V10-->>E: readiness 7d + RPE 14d cutoffs driven by nowIso
E->>EV: mergeEventPlayerMetrics(base, eventInputs, nowIso)
EV-->>E: catching innings 7d cutoff driven by nowIso
%%{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 E as engine-run.ts
participant LA as loadAllPlayerMetrics
participant V10 as mergeV10PlayerMetrics
participant EV as mergeEventPlayerMetrics
Note over E: nowIso = context.nowIso ?? new Date().toISOString()
E->>LA: loadAllPlayerMetrics(playerIds, rows, nowIso)
LA-->>E: "pitching workload cutoff = Date.parse(nowIso) - 7d"
E->>V10: mergeV10PlayerMetrics(p, readiness, sessions, setResults, nowIso)
V10-->>E: readiness 7d + RPE 14d cutoffs driven by nowIso
E->>EV: mergeEventPlayerMetrics(base, eventInputs, nowIso)
EV-->>E: catching innings 7d cutoff driven by nowIso
Reviews (1): Last reviewed commit: "fix(baseball): thread engine's nowIso th..." | Re-trigger Greptile |
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ce-table + event-derived wiring (#851) loaders.ts now honestly threads stat-layer provenance instead of hardcoding baseball_player_stats everywhere, without changing any existing caller's output: - Per-row hittingSourceTable/pitchingSourceTable tags (set by the new normalizeBoxScoreBattingRow/normalizeBoxScorePitchingRow helpers) let a future caller that has normalized rows from the canonical baseball_box_score_batting/_pitching tables get an honest source_refs citation instead of the legacy default. - An optional eventDerived input (built via eventDerivedVelocityFromMetrics from elite-stat-events.ts's DerivedMetric[] output) makes exit/pitch velocity metrics prefer real event-grain data over the legacy scalar columns, per-field, whenever it exists — never blended, never fabricated. - Both inputs are additive/optional appended AFTER the existing nowIso param (preserved exactly, including its threading from #811), so engine-run.ts/outcome-sweep.ts/action-baseline.ts/practice-effectiveness.ts (Phase 4b/2, not touched here) keep working byte-for-byte unchanged — verified by running their existing test suites. registry.ts's header doc comment no longer asserts baseball_player_stats/ baseball_player_aggregates as the metrics' only source, since loaders.ts can now source some fields canonically; the file has zero remaining references to the deprecated tables, so its stat-layer-manifest.ts grandfathered entry is removed. registry.role-visibility.test.ts's source_refs fixture switches to a canonical table name for the same reason and is also removed from the manifest. engine-v10.test.ts gains new coverage for the loaders behavior above (still legitimately cites baseball_player_stats to pin the legacy-fallback path, so its manifest entry is kept, with an updated note). Pre-existing, unrelated drift found but NOT fixed here (confirmed via git-stash A/B on the base branch, present before this change): the stat-layer-contract test already fails on 3 files outside this chunk (practice-effectiveness.test.ts, player-today-self-scope.test.ts, player-today-honest-loop.test.ts reference deprecated tables without a manifest entry) and 1 stale entry (insights.ts, already migrated by #819, never removed from the manifest). Flagged for a separate fix — out of scope for the loaders/registry file list. Co-authored-by: Fable Integrator <fable@helm.local> Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
…scoped precedence (supersedes #852) (#854) * fix(baseball): CoachHelm engine Phase 4b — canonical stat reads for sweep/baseline/engine-run + deterministic import-quality clock (#379, #811 residual) Reader migration (#379 Phase 4b, the highest-blast-radius chunk): - NEW src/lib/baseball/coachhelm/engine-stat-rows.ts — the ONE consolidated per-session stat-row read for the engine. Prefers canonical baseball_box_score_batting/_pitching rows (normalized onto the loader shape via #851's normalizeBoxScoreBattingRow/normalizeBoxScorePitchingRow, with session_date joined from baseball_games and source-table provenance tags), reconciled over legacy baseball_player_stats rows per the #379 precedence rule: canonical rows replace a player's legacy GAME rows outright (never blended — the #827 seed writes the same games into both layers), legacy practice/other rows always survive (practice carve-out), and a player with zero canonical rows keeps full legacy history (fallback tier). Canonical-side read failures degrade all-or-nothing to the legacy pool; a legacy read failure remains the callers' hard error. All reads paginate past the PostgREST 1000-row cap with stable ordering. - outcome-sweep.ts / action-baseline.ts / engine-run.ts all swap their direct baseball_player_stats reads for the shared helper, so baseline capture, the outcome sweep, and the engine run measure the SAME reconciled pool (apples-to-apples did-it-move). action-baseline's old single-page .limit(1000) read is replaced by the paginated shared read. #811 residual (deterministic engine clock): - BaseballV10EngineInputs gains an optional now (ISO); engine-run threads its nowIso through it; importQualityGenerator's 14-day recency window computes from the caller-supplied nowIso instead of raw Date.now() (default preserves real-time behavior for non-engine callers). New generators/v10.test.ts pins the window against a fixed 2020 clock; engine-run-helm-lifting.test.ts pins that runBaseballEngineCore passes its own nowIso end-to-end. Provenance labels: - generators/index.ts driver() last-resort fallback label no longer hardcodes the deprecated table (loaders now cite the real per-row table); v10.ts's practice-effectiveness cite stays deliberately (its feeder still reads legacy practice rows) with an explanatory comment. Manifest (stat-layer contract kept green in both directions for this chunk): - Removed migrated entries: outcome-sweep.ts, engine-run.ts, action-baseline.ts, generators/index.ts, ai-policy-enforcement.test.ts, signal-from-insight.test.ts (fixtures moved to canonical table names). - Added: engine-stat-rows.ts + its test (the one allowed legacy-fallback read). - Updated notes: loaders.ts, generators/v10.ts, effectiveness/engine.ts and operational-rule-engine.ts (both reviewed, deliberately deferred — their cites are honest while their feeders still read layer 1), plus the three engine test entries now pinning the fallback tier. Tests: engine-stat-rows.test.ts pins the precedence rule directly; action-baseline.test.ts + outcome-sweep-insight-resolve.test.ts gain canonical-preferred, never-blended coverage alongside the existing legacy-fallback pins. Co-authored-by: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01H9QAYqFTKsXGsVw6wXYssa * fix(baseball): scope engine-stat-rows precedence to (player, date), not player alone (#379, #852 review fix) The #379 exclusion rule dropped ALL of a player's legacy stat_type='game' rows once they had ANY canonical box-score row, even for games with no canonical counterpart — a mid-season box-score import start would silently erase that player's earlier legacy-logged games from every engine caller (engine-run, outcome-sweep, action-baseline), shrinking sample_n and flipping confidence/verdicts. baseball_player_stats has no game_id, so we now correlate on the resolved canonical game date (truncated to YYYY-MM-DD on both sides) instead: a legacy game row is dropped only when that same player has canonical coverage on that exact calendar day: a same-day heuristic, not a guaranteed game-identity match, since there's no FK to lean on (documented in the module comment as an accepted double-header collision risk). Existing precedence tests encoded the bug: their legacy-row fixture dates never matched the canonical game dates, yet still asserted full drop — only possible under the old player-scoped exclusion. Realigned those fixture dates to same-day overlap (preserving each test's 100%-coverage intent) and added a mixed-coverage case (3 legacy-only + 2 canonical -> 5 rows survive) plus a same-day-collision case (legacy row on a canonically-covered date -> dropped). 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>
Root cause
PR #808 (CI guardrails) was blocked by two failing required checks — "Unit tests" and "Business contracts" — both on the same failure in
src/lib/baseball/__tests__/engine-run-helm-lifting.test.ts. That test fixes the engine's clock atnowIso: '2026-06-30T12:00:00.000Z'and seeds an RPE set result atcompleted_at: '2026-06-29', one day earlier — well inside a 14-day rolling window.The failure:
loadLiftMetrics()insrc/lib/coachhelm/baseball/loaders-v10.tscomputed its rolling-window cutoff fromDate.now()— the real wall clock — instead of thenowIsothatrunBaseballEngineCorealready threads through for deterministic runs. As real time advanced past ~14 days from the seeded date, the fixed-clock test's data aged out of a window measured against today's actual date, and the test started failing with zero code changes. Confirmed: this exact PR branch passed clean on this exact test 3 days ago and started failing today, purely from the calendar advancing.Fix
Threaded
nowIsothrough the full call chain, not just the one function the failing test touches — the same class of bug was present in three sibling loaders:loadReadinessMetrics(7-day readiness window) andloadLiftMetrics(session-due check + 14-day RPE window) inloaders-v10.ts, called viamergeV10PlayerMetricsloadPlayerMetrics's pitching-workload calc (7-day window) inloaders.ts, called vialoadAllPlayerMetricsloadCatchingMetrics's recent-innings calc (7-day window) inloaders-events.ts, called viamergeEventPlayerMetricsengine-run.ts's three call sites (loadAllPlayerMetrics,mergeV10PlayerMetrics,mergeEventPlayerMetrics) now pass through thenowIsoalready in scope there.Each new
nowIsoparameter defaults tonew Date().toISOString(), matching the existinginput.now ?? new Date()pattern already used ineffectiveness/engine.ts— so the three other callers ofloadPlayerMetrics(practice-effectiveness.ts,action-baseline.ts,outcome-sweep.ts) keep their current real-time behavior unchanged. Only the deterministic engine-run path needed fixing.Explicitly out of scope
generators/v10.ts'simportQualityGeneratorhas the same class of bug (filters recent import runs viaDate.now()), butBaseballV10EngineInputshas nonow/nowIsofield for generators at all today — threading one through would mean adding a field to a widely-shared interface used by every generator, which is a bigger, separate change. Flagging it here rather than silently leaving it.Verification
Ran the exact commands CI's "Unit tests" and "Business contracts" jobs run (no pipe-masking):
npm run test:run(vitestunitproject): 494 test files / 4921 tests passed, 0 failuresnpm run test:business(vitestbusinessproject): 505 test files / 4996 tests passed, 0 failuresnpm run typecheck: cleanBoth of these were the exact two checks failing on PR #808 before this fix.
Co-Authored-By: Claude Fable 5 noreply@anthropic.com