Skip to content

fix(api): validate parsePositiveInt input to reject malformed numeric strings#1718

Open
galuis116 wants to merge 2 commits into
JSONbored:mainfrom
galuis116:fix/parse-positive-int-validation
Open

fix(api): validate parsePositiveInt input to reject malformed numeric strings#1718
galuis116 wants to merge 2 commits into
JSONbored:mainfrom
galuis116:fix/parse-positive-int-validation

Conversation

@galuis116

Copy link
Copy Markdown
Contributor

Add regex validation to parsePositiveInt in src/api/routes.ts, src/github/webhook.ts, and src/orb/webhook.ts to ensure the entire string consists only of digits before parsing. This prevents partial parsing of malformed strings like '123abc' which would previously return 123 instead of
ull.

Changes

  • Added /^\d+$/ regex check before parseInt in all three parsePositiveInt implementations
  • Added comprehensive unit tests covering edge cases (malformed strings, partial numeric strings, valid strings)
  • Ensures fail-closed behavior for malformed input

Fixes #1717

… strings

Add regex validation to parsePositiveInt in src/api/routes.ts, src/github/webhook.ts, and src/orb/webhook.ts to ensure the entire string consists only of digits before parsing. This prevents partial parsing of malformed strings like '123abc' which would previously return 123 instead of null.

Fixes JSONbored#1717
@galuis116 galuis116 requested a review from JSONbored as a code owner June 29, 2026 07:03
@dosubot dosubot Bot added the size:XS This PR changes 0-9 lines, ignoring generated files. label Jun 29, 2026
@gittensory-orb

gittensory-orb Bot commented Jun 29, 2026

Copy link
Copy Markdown

Caution

🟥🟥🟥🟥🟥🟥🟥🟥🟥🟥🟥🟥

🛑 Gittensory review result - reject/close recommended

Review updated: 2026-06-29 14:01:08 UTC

5 files · 1 AI reviewer · 1 blocker · readiness 55/100 · CI green · blocked

🛑 Suggested Action - Reject/Close

  • AI reviewers agree on a likely critical defect: test/unit/parse-positive-int.test.ts:7 defines a local copy of parsePositiveInt, so the new tests are disconnected from src/api/routes.ts, src/github/webhook.ts, and src/orb/webhook.ts and do not validate the production fix. — Resolve the flagged defect, or override if the AI reviewers are mistaken, then re-run the gate.

Review summary
The production changes fail closed for non-digit strings before calling parseInt in all three parsePositiveInt helpers, which matches the PR intent for malformed numeric input. The notable issue is the new test file copies the implementation under test instead of exercising any production helper, so it does not verify the changed code and can pass after a production regression.

Blockers

  • test/unit/parse-positive-int.test.ts:7 defines a local copy of parsePositiveInt, so the new tests are disconnected from src/api/routes.ts, src/github/webhook.ts, and src/orb/webhook.ts and do not validate the production fix.
Nits — 4 non-blocking
  • src/api/routes.ts:331, src/github/webhook.ts:119, and src/orb/webhook.ts:144 still duplicate the same parsing helper, which keeps the three fail-closed behaviors easy to drift apart later.
  • Export or move parsePositiveInt to a shared testable helper and have test/unit/parse-positive-int.test.ts import that production function instead of redefining it.
  • After sharing the helper, update the three call sites to use the same implementation so the regex behavior is covered once and cannot diverge by file.
  • Readiness score is below the configured threshold — Use the readiness panel as advisory maintainer context; the score does not block this PR.

Why this is blocked

  • test/unit/parse-positive-int.test.ts:7 defines a local copy of parsePositiveInt, so the new tests are disconnected from src/api/routes.ts, src/github/webhook.ts, and src/orb/webhook.ts and do not validate the production fix.
Signal Result Evidence
Code review ❌ 1 blocker 1 reviewer
Linked issue ✅ Linked #1717
Related work ⚠️ 2 scoped overlaps Top overlaps are listed below; lower-confidence bulk is hidden.
Review load ❌ 8/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 22 open PR(s), 10 likely reviewable, 12 unlinked.
Contributor context ✅ Confirmed Gittensor contributor galuis116; Gittensor profile; 1561 PR(s), 156 issue(s).
Gate result ❌ Blocking Repo-configured hard blocker found.
Review context
Contributor next steps
  • Review top overlaps.
  • Add scope summary.
  • Fix blocker.
  • Expect slower review.
  • Refresh registry data or choose a registered active repo.
  • Check active issues and PRs before submitting.
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.

🟩 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 Jun 29, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 95.58%. Comparing base (6228967) to head (4f0522a).
✅ All tests successful. No failed tests found.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1718      +/-   ##
==========================================
- Coverage   95.58%   95.58%   -0.01%     
==========================================
  Files         204      204              
  Lines       22314    22307       -7     
  Branches     8066     8063       -3     
==========================================
- Hits        21329    21322       -7     
  Misses        408      408              
  Partials      577      577              
Files with missing lines Coverage Δ
src/api/routes.ts 94.63% <ø> (-0.02%) ⬇️
src/github/webhook.ts 100.00% <ø> (ø)
src/orb/webhook.ts 100.00% <ø> (ø)
src/utils/json.ts 100.00% <100.00%> (ø)
🚀 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 added gittensor Gittensor contributor context gittensor:bug Gittensor-scored bug fix - worth 0.5x multiplier. labels Jun 29, 2026
…edback

Move parsePositiveInt from local implementations in src/api/routes.ts, src/github/webhook.ts, and src/orb/webhook.ts to a shared utility in src/utils/json.ts. Update all call sites to import from the shared location and update the test to exercise the production function directly.

Addresses Gittensory review feedback about test coverage and code duplication.
@dosubot dosubot Bot added size:M This PR changes 30-99 lines, ignoring generated files. and removed size:XS This PR changes 0-9 lines, ignoring generated files. labels Jun 29, 2026
@superagent-security superagent-security Bot removed the size:M This PR changes 30-99 lines, ignoring generated files. label Jun 29, 2026
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 - worth 0.5x multiplier. gittensor Gittensor contributor context

Projects

None yet

Development

Successfully merging this pull request may close these issues.

fix(api): fail-closed when parsePositiveInt receives malformed numeric strings

1 participant