fix(eval): state the coverage an EvalScoreRecord already determines - #9861
Conversation
|
Warning ⏸️ LoopOver review result - manual review recommendedReview updated: 2026-07-29 14:52:19 UTC
Review summary Nits — 3 non-blocking
Decision drivers
Context & advisory signals — never blocks the verdict
Review context
Contributor next steps
Signal definitions
🧪 Chat with LoopOverAsk LoopOver a question about this PR directly in a comment — grounded only in the same cached, public-safe facts shown above, never a new claim.
Full command reference: https://loopover.ai/docs/loopover-commands 🧪 Experimental — new and may change. 🟩 Safe / merged · 🟦 Advisory · 🟨 Held for review · 🟥 Blocked / closed 💰 Earn for open-source contributions like this. Gittensor lets GitHub contributors earn for the work they already do — register to start earning →. Checked by LoopOver, a quiet PR intelligence layer for OSS maintainers.
|
|
Superagent didn't find any vulnerabilities or security issues in this PR. |
loopover-orb
Bot
added
the
gittensor:bug
![loopover-orb[bot]](https://avatars.githubusercontent.com/in/4139483?s=60&v=4){kind=small}
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #9861 +/- ##
===========================================
- Coverage 91.64% 79.05% -12.60%
===========================================
Files 916 282 -634
Lines 112753 58432 -54321
Branches 27084 8565 -18519
===========================================
- Hits 103338 46194 -57144
- Misses 8126 11955 +3829
+ Partials 1289 283 -1006
Flags with carried forward coverage won't be shown. Click here to find out more.
|
`buildEvalScoreRecordsFromRulePrecision` hardcoded `coverage: null` while asserting `abstained: 0` two lines below. JSONbored#9215 defines coverage as `decided / (decided + abstained)`, so with abstained structurally zero the value is fully determined -- a validator re-deriving the record computed 1 and disagreed with the published field, which is exactly what an independently re-derivable record format exists to prevent. Compute it through a new exported `evalScoreCoverage(decided, abstained)`, so the JSONbored#9265 benchmark_run emitter states coverage by calling the same definition rather than restating the formula, and `decided === 0` still yields null (0/0 is undefined, never a masked zero). Extends the doc comment's existing per-field null-vs-zero justifications to cover the third field.
1 participant
Summary
buildEvalScoreRecordsFromRulePrecisionhardcodedcoverage: nullwhile assertingabstained: 0two lines below. #9215 definescoverage = decided / (decided + abstained),so with
abstainedstructurally0the value is fully determined: any rule that decidedanything covered all of it. Publishing
nulltold a consumer "this record does not stateits coverage" about a quantity the record itself pins down, and a validator re-deriving the
score per the #9215 contract computed
1and disagreed with the published field — the onething an independently re-derivable record format exists to prevent.
Root cause. The doc comment justifies
recall: nullandabstained: 0explicitly, perfield.
coveragewas filled in asnullby symmetry withrecalland never reasoned about— an omission in a justification block rather than a logic error, which is why it survived
review.
Changes.
evalScoreCoverage(decided, abstained): number | nullinsrc/review/eval-score-records.ts—total > 0 ? decided / total : null, mirroring theguard-the-denominator-else-null shape
PublicRulePrecisionRow.precisionuses below itssample floor. Exported so benchmark: leaderboard artifacts as spec-conformant EvalScoreRecords #9265's
benchmark_runemitter states coverage by calling thisrather than restating the formula.
coverage: evalScoreCoverage(row.decided, 0), passing the sameliteral
0the siblingabstainedfield publishes so the two cannot drift into a recordwhose coverage contradicts its own abstention count.
coveragejustification in the established per-fieldnull-vs-zero style.
decided === 0still yieldsnull—0/0is genuinely undefined, never a masked zero.recordDigestcovers the whole record, so digests change foroutcome_confirmed_precisionrecords. No migration is needed: records are built on read (
routes.ts), never persisted,and no golden-corpus or fixture file pins a digest.
Closes #9643
Scope
type(scope): short summaryConventional Commit formatCONTRIBUTING.mdand does not reintroduce GitHub Pages, VitePress,site/, orCNAMEEvalScoreRecord.coverageis published as null #9643)Validation
git diff --check— cleannpm run actionlintnpm run typecheck— exit 0 (root + packages)npm run test:coverage— see note belownpm run test:workersnpm run build:mcpnpm run test:mcp-packnpm run ui:openapi:checknpm run ui:lintnpm run ui:typechecknpm run ui:buildnpm audit --audit-level=moderate— 0 vulnerabilitiesTargeted runs that passed in full:
npx vitest run test/unit/eval-score-records.test.ts— 27/27eval-score-records,public-eval-scores-route,benchmark-eval-records,threshold-backtest-run,public-rule-precision) — 74/74fails without the change.
db:migrations:immutable:check, db:schema-drift:check, cf-typegen:check,
coverage-boltons:check, selfhost:env-reference:check, miner:env-reference:check,
selfhost:validate-observability, docs:drift-check, manifest:drift-check,
engine-parity:drift-check, branding-drift:check, engines-nvmrc:check,
release-manifest:sync:check, command-reference:check, ui:openapi:check,
workspace-dep-ranges:check, server-manifest:check, dispatch-gate-reasons:check,
ui-derived-types:check, contract:api-schemas:check)
npm run test:coverageran to completion locally (1013 s, 1320 files): 1258 files / 25084tests passed; 58 files / 207 tests failed, exit 1. Those 58 files fail identically on
unmodified
main— verified with a baseline run over the same file set at the same basecommit (57 failed / 206 tests). The one-file delta,
test/unit/miner-cross-repo-evaluation.test.ts, passes 60/60 with these changes applied inisolation and references nothing this PR touches; it is a load-sensitive flake. The failures
are environmental to a Windows dev box (
spawn claude ENOENT,spawn codex ENOENT, missingsqlite temp state, Windows path handling in several of the repo's own scripts). No failure
anywhere in the run log references
evalScoreCoverage,eval-score-records, orpublic-eval-scores. The unchecked boxes above were not run to completion for the samereason — they are not skipped as unnecessary, and CI is the authoritative signal for them.
Safety
coveragefield was already typednumber | null;ui:openapi:checkis cleanUI Evidencesection — n/a, no visible changeUI Evidence
Not applicable — backend-only change to a published JSON field with no visible UI, frontend,
docs, or extension surface.
.loopover.yml'sscreenshotTableGateis scoped toapps/loopover-ui/src/{components,routes}/**andstyles.css; this PR touches none of them.Notes
Risks and limitations.
coverage: null(eval-score-records.test.tslines 75/79,
public-eval-scores-route.test.tsline 59). They are corrected to1, whichis the behavioural change this issue asks for, not a weakened test.
deriveBenchmarkLeaderboardranks ties bycoverage ?? -1, sonull → 1could inprinciple reorder it. It filters to
workUnit.kind === "benchmark_run"first and these areoutcome_confirmed_precisionrecords, so they never reach the ranking — no ordering change.benchmark-eval-records.tsreadscoveragestraight offreport.coverage.coverageratherthan re-deriving it, so there is no existing duplicate formula to unify. Retrofitting it to
call the helper is deliberately out of scope here: the Deliverables do not ask for it and
the value originates in
packages/loopover-engine.recordDigestacross the deploy will see it change once, by design.