fix(scoring): guard credibility against non-finite stale and unlinked counts - #1155
Conversation
… counts `inferCredibility` coerces the cached evidence counts `mergedPullRequests`, `stalePullRequests`, and `unlinkedPullRequests` with `Number(...)`, but only finiteness-guards `merged`. Because the evidence payload is a loosely-typed cache (`Record<string, JsonValue>`), any count can arrive non-numeric; a non-finite `stale` or `unlinked` propagates `NaN` through the multiplier arithmetic, and `clamp(Math.min(1, Math.max(0.25, NaN)))` cannot rescue it, so the whole credibility multiplier and the resulting estimated score become `NaN`. Extend the existing guard to all three counts so a malformed `stale`/`unlinked` degrades to the same neutral 0.8 fallback that a malformed `merged` already produced, keeping the score finite. Adds a regression test covering each newly guarded branch plus the well-formed arithmetic path.
|
Superagent didn't find any vulnerabilities or security issues in this PR. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1155 +/- ##
=======================================
Coverage 95.00% 95.00%
=======================================
Files 177 177
Lines 19827 19827
Branches 7133 7133
=======================================
Hits 18837 18837
Misses 396 396
Partials 594 594
🚀 New features to boost your workflow:
|
|
Tip 🟩🟩🟩🟩🟩🟩🟩🟩🟩🟩🟩🟩 ✅ Gittensory review — safe to merge
✅ Approved — safe to merge Review summary Nits (5)
Nits — 1 non-blocking
Review context
Contributor next steps
Signal definitions
Review detailsGenerated from public PR metadata and the diff. Advisory only; deterministic signals remain authoritative. The change expands the finiteness guard in `inferCredibility` to include stale and unlinked pull request counts, preventing NaN propagation and ensuring a fallback credibility of 0.8. A new test validates this behavior for malformed stale and unlinked values. All existing tests pass, confirming functional correctness. Nits (5)
🟩 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 Gittensory, a quiet PR intelligence layer for OSS maintainers.
|
There was a problem hiding this comment.
MERGE. fix(scoring): guard credibility against non-finite stale/unlinked counts.
- Real bug: inferCredibility only finite-guarded
merged; a non-numeric cached
stale/unlinked count coerces to NaN and clamp(NaN,…)=NaN poisons credibilityObserved,
credibilityMultiplier, estimatedMergedScore (preview.ts:331-333, 365-366). - Fix correct & well-placed: guard broadened to all three counts before the arithmetic;
Number.isFinite catches NaN+Infinity; fallback 0.8 == MIN_CREDIBILITY so the multiplier
becomes 1 — no div-by-zero / downstream regression. - Coverage complete & meaningful: every || arm tested both ways; asserts
credibilityObserved===0.8 per malformed branch + toBeCloseTo(0.87,5) on the happy path. - Already current with main; scope clean; no attribution.
Note (non-blocking): commits are authored as "plind-junior" while the PR is @dripsmvcp.
Summary
inferCredibility(insrc/scoring/preview.ts) derives a contributor credibility multiplier from cached evidence counts. It coercesmergedPullRequests,stalePullRequests, andunlinkedPullRequestswithNumber(...), but only finiteness-guardsmerged:The evidence payload is a loosely-typed cache (
Record<string, JsonValue>), so any of these fields can arrive as a non-numeric cached value (string, object, …). A non-finitestaleorunlinkedpropagatesNaNinto the arithmetic, andclampcannot rescue it (Math.min(1, Math.max(0.25, NaN))isNaN) — so the whole credibility multiplier, and throughbuildScorePreviewthegates.credibilityObservedandscoreEstimate.estimatedMergedScore, becomeNaN. The identical garbage inmergedPullRequestsis already tolerated (it returns the neutral0.8); onlystale/unlinkedwere left unguarded.Fix: extend the existing guard to all three counts, so a malformed
stale/unlinkeddegrades to the same neutral0.8fallback that a malformedmergedalready produces, keeping the score finite. Pure helper; no signature or public-surface change.No linked issue: this is a small, self-evident defensive-correctness fix to one pure scoring helper, fully described above; per
CONTRIBUTING.md/.gittensory.yml(linkedIssuePolicy: preferred) a focused fix like this does not require a linked issue.Scope
type(scope): short summaryConventional Commit format.CONTRIBUTING.mdand does not reintroduce GitHub Pages, VitePress,site/, orCNAME.Validation
git diff --checknpm run actionlintnpm run typechecknpm run test:coveragelocally — both newly guarded branches (staleandunlinkednon-finite) and the well-formed arithmetic path are covered; the changed lines are at 100% line and branch coverage and global coverage stays at/above the 97% line/branch threshold.npm 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=moderatetest/unit/scoring.test.ts).If any required check was skipped, explain why:
npm run test:ci(which wrapsgit diff --check,actionlint,db:migrations:check,typecheck,test:coverage,test:workers,build:mcp,test:mcp-pack,ui:openapi:check,ui:version-audit,ui:lint,ui:typecheck,ui:test,ui:build) ran fully green locally.npm audit --audit-level=moderatesurfaces only one pre-existing transitive advisory (undici) that this PR does not introduce — the diff changes no dependency manifest or lockfile, so thedependency-reviewjob is unaffected (that ambient advisory is tracked by the scheduledaudit.ymlworkflow + Renovate, not by PR CI).Safety
ui:openapi:checkpasses unchanged).Notes
credibilityObserved === 0.8and a finiteestimatedMergedScore, and that well-formed counts still flow through the arithmetic (so the guard does not over-fire).