Skip to content

fix(miner): use precision-safe numeric comparison in the CLI update-check semver comparator#3053

Merged
loopover-orb[bot] merged 1 commit into
JSONbored:mainfrom
joaovictor91123:fix/miner-semver-prerelease-precision
Jul 4, 2026
Merged

fix(miner): use precision-safe numeric comparison in the CLI update-check semver comparator#3053
loopover-orb[bot] merged 1 commit into
JSONbored:mainfrom
joaovictor91123:fix/miner-semver-prerelease-precision

Conversation

@joaovictor91123

Copy link
Copy Markdown
Contributor

Summary

comparePrerelease in both packages/gittensory-miner/lib/update-check.js and packages/gittensory-mcp/bin/gittensory-mcp.js (two independent, byte-identical copies used by each CLI's own npm-registry upgrade-nudge check) compares numeric semver prerelease identifiers via Number(leftId) !== Number(rightId):

if (leftNumeric && rightNumeric) {
  if (Number(leftId) !== Number(rightId)) return Number(leftId) < Number(rightId) ? -1 : 1;
}

Number() loses precision past Number.MAX_SAFE_INTEGER (2^53-1 = 9007199254740991). Two distinct digit strings past that width can round to the exact same float64 value — e.g. "9007199254740993" (2^53+1) is not representable and rounds down to 9007199254740992 (2^53), the same value "9007199254740992" itself produces. Number(leftId) !== Number(rightId) then reads false, so the comparator wrongly reports two genuinely different, orderable numeric identifiers as equal.

Fix

Compare numeric identifiers as decimal strings instead: with no leading zeros (semver's own rule for numeric identifiers), a longer digit string is unconditionally the larger number, and equal-length strings compare lexicographically — never through Number(). Applied identically to both copies so the two CLIs' upgrade-nudge comparisons stay byte-identical, per the existing "mirrors gittensory-mcp" comments already in both files.

This is the exact same fix already applied to compareMcpSemver's comparePrerelease in src/services/mcp-compatibility.ts in the recently-merged #3049 — that fix's own doc comment explicitly calls out avoiding Number() for this reason. These two CLI-package copies are separate, independent implementations (not touched by #3049, which only fixed the src/services/ MCP-compatibility service) that had the identical latent gap.

Why no linked issue

Small, self-evident, mechanical fix mirroring an already-merged sibling fix (#3049) in a different pair of files; this repo's linkedIssuePolicy is preferred, not required, for a change this scoped.

Scope

  • The PR title follows type(scope): short summary Conventional Commit format, for example fix(api): restore profile access checks.
  • This PR is focused and does not mix unrelated backend, UI, MCP, docs, dependency, and deploy changes.
  • This follows CONTRIBUTING.md and does not reintroduce GitHub Pages, VitePress, site/, or CNAME.
  • I linked an issue, or this is small enough that the summary explains why an issue is not needed.

Validation

  • git diff --check
  • npm run actionlint
  • npm run typecheck
  • npm run test:coverage locally — see note below on packages/** scope.
  • npm run test:workers
  • npm run build:mcp
  • npm run test:mcp-pack
  • npm run ui:openapi:check
  • npm run ui:lint
  • npm run ui:typecheck
  • npm run ui:build
  • npm audit --audit-level=moderate
  • New or changed behavior has unit/integration tests for new branches, fallback paths, and sanitizer boundaries

If any required check was skipped, explain why:

  • This PR touches only packages/** (plus a test/unit/miner-cli.test.ts regression test), no src/** API/OpenAPI/UI surface, so ui:openapi:check, ui:lint, ui:typecheck, and ui:build are not applicable. Per codecov.yml, coverage is collected by vitest over src/** only — packages/** is outside the Codecov patch-coverage gate entirely — but I still added a real regression test (test/unit/miner-cli.test.ts, exercising packages/gittensory-miner/lib/update-check.js's exported compareSemver) proving the exact precision bug and its fix, and ran the full test/unit/miner-cli.test.ts suite (23 tests) clean. packages/gittensory-mcp/bin/gittensory-mcp.js has no existing unit-test entry points for its internal (non-exported) comparePrerelease/compareSemver — consistent with the file's current test coverage, I verified it only via npm run build:mcp (node --check, syntax/build validity) plus a manual read-through confirming byte-parity with the now-fixed, tested update-check.js copy.
  • npm run build:miner and npm run build:mcp both ran clean (node --check on every changed file).
  • npm run test:mcp-pack hits a Windows-only spawnSync("npm", ...) resolution failure locally (no shell: true, npm resolves to npm.cmd on Windows) — unrelated to this diff, expected to run on the Linux CI runner.
  • The full unsharded test:coverage suite could not be run clean in my local Windows dev environment for unrelated reasons: several test files depend on tooling not present there (Docker daemon, Python, sentry-cli), and generated-file "stale" checks (openapi.json, cf-typegen, selfhost-env-reference) false-positive on this checkout's CRLF line endings vs. the repo's LF convention — confirmed unrelated to this diff by reproducing them identically on a clean, unmodified main.
  • npm run actionlint, npm run typecheck, npm run test:workers, and npm audit all ran clean.

Safety

  • No secrets, wallet details, hotkeys, coldkeys, user PATs, private keys, raw trust scores, private rankings, or private maintainer evidence are exposed.
  • Public GitHub text stays sanitized, low-noise, and does not imply compensation guarantees or optimization tactics.
  • Auth, cookie, CORS, GitHub App, Cloudflare, or session changes include negative-path tests. — N/A, no such changes.
  • API/OpenAPI/MCP behavior is updated and tested where needed. — N/A, no request/response schema changes; this is an internal CLI-only version comparator.
  • UI changes use live API data or real empty/error/loading states, not production mock/demo fallbacks. — N/A, no UI changes.
  • Visible UI changes include a UI Evidence section below. — N/A, no UI changes.
  • Public docs/changelogs are updated where needed. — N/A, no docs/changelog changes.

UI Evidence

N/A — no UI/frontend/docs changes.

Notes

  • New regression test: "compares numeric prerelease identifiers as decimal strings, not via Number() (precision loss past 2^53-1)" in test/unit/miner-cli.test.ts.

@superagent-security

Copy link
Copy Markdown
Contributor

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

@loopover-orb loopover-orb Bot added the gittensor:bug Gittensor-scored bug fix — scores a 0.05x multiplier. label Jul 4, 2026
@loopover-orb

loopover-orb Bot commented Jul 4, 2026

Copy link
Copy Markdown

Warning

🟨🟨🟨🟨🟨🟨🟨🟨🟨🟨🟨🟨

⏸️ Gittensory review result - manual review recommended

Review updated: 2026-07-04 10:41:08 UTC

3 files · 1 AI reviewer · no blockers · readiness 73/100 · CI green · clean

⏸️ Suggested Action - Manual Review

Review summary
This change fixes the reachable precision bug in the prerelease comparator by replacing float64 numeric coercion with length-plus-lexicographic decimal comparison, matching the already-fixed service-side implementation. The logic is correct for valid semver numeric identifiers, and the added regression exercises the exact precision-loss case that `Number()` mishandled. I do not see a must-fix defect in the visible diff.

Nits — 5 non-blocking
  • nit: packages/gittensory-mcp/bin/gittensory-mcp.js:3485 changes the MCP CLI copy without a sibling regression test for that CLI path, so future drift between the two byte-identical copies could slip through even though the miner test covers the same helper shape.
  • nit: packages/gittensory-miner/lib/update-check.js:75 still relies on the parser accepting only valid semver numeric identifiers with no leading zeroes; that is fine for npm package versions, but a short comment or fixture for invalid `01`-style inputs would make the assumption explicit.
  • Add the same precision-loss regression around the MCP CLI comparator if that helper is exposed through the existing MCP CLI test harness, or extract the shared comparator so one test covers both CLIs by construction.
  • Consider matching the concise service-side comment in `src/services/mcp-compatibility.ts` instead of duplicating the long incident explanation in both generated CLI copies, unless these files intentionally carry full standalone context.
  • Readiness score is below the configured threshold — Use the readiness panel as advisory maintainer context; the score does not block this PR.
Signal Result Evidence
Code review ✅ No blockers 1 reviewer
Linked issue ⚠️ Missing No linked issue or no-issue rationale found.
Related work ✅ No active overlap found No same-issue or scoped active PR overlap found.
Change scope ✅ 20/20 Low review scope from cached public metadata (no linked issue context).
Validation posture ❌ 5/25 Preflight is holding this PR: the review lane is unavailable, so it is not ready for automated review.
Contributor workload ✅ 10/10 Author activity: 82 registered-repo PR(s), 44 merged, 8 issue(s).
Contributor context ✅ Confirmed Gittensor contributor joaovictor91123; Gittensor profile; 82 PR(s), 8 issue(s).
Gate result ✅ Passing No configured blocker found.
Review context
  • Author: joaovictor91123
  • Role context: outside_contributor
  • Public audience mode: oss maintainer
  • Lane context: Repository registration is not available in the local Gittensory cache.
  • Public profile languages: not available
  • Official Gittensor activity: 82 PR(s), 8 issue(s).
  • PR-specific overlap: none found.
Contributor next steps
  • Explain no-issue PR.
  • Await review-lane availability.
  • Refresh registry data or choose a registered active repo.
  • Link the issue being solved, or explicitly explain why this is a no-issue PR.
Signal definitions
  • Related work = same linked issue, overlapping active PRs, or title/path similarity.
  • Change scope = cached public metadata such as size labels, draft state, and review-burden hints.
  • Validation posture = whether the PR provides enough public validation/test evidence for maintainer review.
  • Contributor workload = public contributor activity and cleanup pressure, not a repo-wide quality failure.
  • Contributor context = public GitHub/Gittensor identity context; non-Gittensor status is not a blocker.

🟩 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

@codecov

codecov Bot commented Jul 4, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 96.10%. Comparing base (54f6e76) to head (b878710).
⚠️ Report is 1 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #3053   +/-   ##
=======================================
  Coverage   96.10%   96.10%           
=======================================
  Files         261      261           
  Lines       28895    28895           
  Branches    10520    10520           
=======================================
  Hits        27769    27769           
  Misses        492      492           
  Partials      634      634           
🚀 New features to boost your workflow:
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@loopover-orb loopover-orb Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Gittensory approves — the gate is satisfied and CI is green.

@loopover-orb
loopover-orb Bot merged commit 83a1b50 into JSONbored:main Jul 4, 2026
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

gittensor:bug Gittensor-scored bug fix — scores a 0.05x multiplier.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant