Skip to content

fix(health): normalize prober global status_counts#2044

Open
philluiz2323 wants to merge 1 commit into
JSONbored:mainfrom
philluiz2323:fix/prober-global-status-counts
Open

fix(health): normalize prober global status_counts#2044
philluiz2323 wants to merge 1 commit into
JSONbored:mainfrom
philluiz2323:fix/prober-global-status-counts

Conversation

@philluiz2323

Copy link
Copy Markdown
Contributor

Summary

What Changed

  • src/health-prober.mjs: both global counts loops now increment normalizeProbeStatus(row.status).
  • tests/health-prober.test.mjs: regression test with status: \"throttled\" asserting it folds into unknown.

Fixes #1942

Registry Safety

  • No secrets, PATs, wallet data, private dashboards, private URLs, or validator-local state.
  • Generated artifacts were produced by repo scripts, not hand-edited.
  • R2-only/high-churn detail artifacts are not committed.
  • Public API/OpenAPI/schema changes are intentional and documented. (None.)

Validation

  • npx vitest run tests/health-prober.test.mjs -t \"folds unrecognized probe status\"
  • git diff --check

@philluiz2323 philluiz2323 requested a review from JSONbored as a code owner June 26, 2026 23:11
@superagent-security

Copy link
Copy Markdown

Superagent didn't find any vulnerabilities or security issues in this PR.

@codecov

codecov Bot commented Jun 26, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 93.23%. Comparing base (2bcd5c1) to head (2cd147a).
⚠️ Report is 6 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #2044   +/-   ##
=======================================
  Coverage   93.23%   93.23%           
=======================================
  Files          48       48           
  Lines        7626     7626           
  Branches     2804     2802    -2     
=======================================
  Hits         7110     7110           
  Misses         90       90           
  Partials      426      426           
Files with missing lines Coverage Δ
src/health-prober.mjs 94.35% <100.00%> (ø)

... and 2 files with indirect coverage changes

🚀 New features to boost your workflow:
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@gittensory-orb

gittensory-orb Bot commented Jun 27, 2026

Copy link
Copy Markdown

Tip

🟩🟩🟩🟩🟩🟩🟩🟩🟩🟩🟩🟩

✅ Gittensory review — safe to merge

2 files · 1 AI reviewers · no blockers · readiness 73/100 · CI green · clean

✅ Approved — safe to merge

Review summary
Two-line fix routing both global `counts` accumulator loops in `runHealthProber` and `persistToKv` through `normalizeProbeStatus` before keying into the counts object, so unrecognized statuses like `"throttled"` fold into `unknown` instead of silently creating out-of-schema keys. The switch from `(counts[row.status] || 0) + 1` to `counts[normalizeProbeStatus(row.status)] += 1` is safe: the counts object is pre-initialized with all four canonical keys to `0`, and `normalizeProbeStatus` — already in use for the same purpose in `health-serving.mjs::summarizeRows` — always returns one of those keys. The regression test is thorough, covering all three downstream representations (`result.counts`, `summary.status_counts`, `meta.status_counts`) and explicitly asserting the spurious key is absent.

Signal Result Evidence
Code review ✅ No blockers 1 reviewers, synthesized
Linked issue ✅ Linked #1942
Related work ✅ No active overlap found No same-issue or scoped active PR overlap found.
Review load ✅ 20/20 Readiness component derived from cached public PR metadata and labels.
Validation evidence ❌ 5/25 Cached preflight status is hold.
Open PR queue ❌ 3/10 14 open PR(s), 9 likely reviewable, 5 unlinked.
Contributor context ✅ Confirmed Gittensor contributor philluiz2323; Gittensor profile; 740 PR(s), 171 issue(s).
Gate result ✅ Passing No configured blocker found.
Nits — 3 non-blocking
  • tests/health-prober.test.mjs: the final `assert.equal(current.summary.status_counts.throttled, undefined)` is redundant — the preceding `assert.deepEqual` with an exact four-key object already verifies no extra keys exist; harmless but worth trimming for clarity.
  • tests/health-prober.test.mjs: consider adding a parallel assertion that a canonical status (e.g. `status: "ok"`) still passes through `normalizeProbeStatus` unchanged in this same test, making it a full round-trip regression rather than only testing the normalization-to-unknown path.
  • Repository config was not parsed
Review context
  • Author: philluiz2323
  • Role context: outside_contributor
  • Public audience mode: oss maintainer
  • Lane context: Repository registration is not available in the local Gittensory cache.
  • Public profile languages: JavaScript, MDX, TypeScript, Cuda, HTML, Kotlin, Rust
  • Official Gittensor activity: 740 PR(s), 171 issue(s).
  • PR-specific overlap: none found.
Contributor next steps
  • Fix blocker.
  • Expect slower review.
  • Refresh registry data or choose a registered active repo.
Signal definitions
  • Related work = same linked issue, overlapping active PRs, or title/path similarity.
  • Review load = cached public PR metadata such as size labels, changed paths, and preflight status.
  • Open PR queue = repo-wide review pressure; it is not a PR quality failure.
  • Contributor context = public GitHub/Gittensor identity context; non-Gittensor status is not a blocker.
Review details

Generated from public PR metadata and the diff. Advisory only; deterministic signals remain authoritative.

Two-line fix routing both global `counts` accumulator loops in `runHealthProber` and `persistToKv` through `normalizeProbeStatus` before keying into the counts object, so unrecognized statuses like `"throttled"` fold into `unknown` instead of silently creating out-of-schema keys. The switch from `(counts[row.status] || 0) + 1` to `counts[normalizeProbeStatus(row.status)] += 1` is safe: the counts object is pre-initialized with all four canonical keys to `0`, and `normalizeProbeStatus` — already in use for the same purpose in `health-serving.mjs::summarizeRows` — always returns one of those keys. The regression test is thorough, covering all three downstream representations (`result.counts`, `summary.status_counts`, `meta.status_counts`) and explicitly asserting the spurious key is absent.

Nits (2)

  • tests/health-prober.test.mjs: the final `assert.equal(current.summary.status_counts.throttled, undefined)` is redundant — the preceding `assert.deepEqual` with an exact four-key object already verifies no extra keys exist; harmless but worth trimming for clarity.
  • tests/health-prober.test.mjs: consider adding a parallel assertion that a canonical status (e.g. `status: "ok"`) still passes through `normalizeProbeStatus` unchanged in this same test, making it a full round-trip regression rather than only testing the normalization-to-unknown path.

🟩 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.

  • Re-run Gittensory review

@gittensory-orb gittensory-orb Bot added gittensor Gittensor contributor context gittensor:bug Bug labels Jun 27, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

gittensor:bug Bug gittensor Gittensor contributor context

Projects

None yet

Development

Successfully merging this pull request may close these issues.

global summary status_counts skips normalizeProbeStatus (#1739 sibling): an unrecognized probe status adds a phantom key instead of folding into unknown

1 participant