Skip to content

fix(upstream): validate issue number shape before Number() constructor in parseGitHubIssueUrl#1720

Open
galuis116 wants to merge 3 commits into
JSONbored:mainfrom
galuis116:fix/parse-github-issue-url-validation
Open

fix(upstream): validate issue number shape before Number() constructor in parseGitHubIssueUrl#1720
galuis116 wants to merge 3 commits into
JSONbored:mainfrom
galuis116:fix/parse-github-issue-url-validation

Conversation

@galuis116

Copy link
Copy Markdown
Contributor

Add regex validation to parseGitHubIssueUrl in src/upstream/ruleset.ts to ensure the issue number consists only of digits before Number() conversion. This prevents partial parsing of malformed issue numbers like '123abc' which could lead to unexpected behavior.

Changes

  • Added /^\d+$/ regex check before Number() conversion in parseGitHubIssueUrl
  • Added comprehensive unit tests covering edge cases (malformed URLs, partial numeric strings, valid URLs)
  • Ensures fail-closed behavior for malformed input

Fixes #1719

…r in parseGitHubIssueUrl

Add regex validation to parseGitHubIssueUrl in src/upstream/ruleset.ts to ensure the issue number consists only of digits before Number() conversion. This prevents partial parsing of malformed issue numbers like '123abc' which could lead to unexpected behavior.

Fixes JSONbored#1719
@galuis116 galuis116 requested a review from JSONbored as a code owner June 29, 2026 07:08
@dosubot dosubot Bot added the size:XS This PR changes 0-9 lines, ignoring generated files. label Jun 29, 2026
@superagent-security superagent-security Bot removed the size:XS This PR changes 0-9 lines, ignoring generated files. label Jun 29, 2026
@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 (53a37ab).
⚠️ Report is 1 commits behind head on main.
✅ All tests successful. No failed tests found.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #1720   +/-   ##
=======================================
  Coverage   95.58%   95.58%           
=======================================
  Files         204      204           
  Lines       22314    22315    +1     
  Branches     8066     8067    +1     
=======================================
+ Hits        21329    21330    +1     
  Misses        408      408           
  Partials      577      577           
Files with missing lines Coverage Δ
src/upstream/ruleset.ts 98.51% <100.00%> (+<0.01%) ⬆️
🚀 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 29, 2026

Copy link
Copy Markdown

Caution

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

🛑 Gittensory review result - reject/close recommended

Review updated: 2026-06-29 12:39:50 UTC

2 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-github-issue-url.test.ts:4 reimplements parseGitHubIssueUrl locally instead of importing or exercising the production function, so this is a fabricated test that will not catch regressions in src/upstream/ruleset.ts. — Resolve the flagged defect, or override if the AI reviewers are mistaken, then re-run the gate.

Review summary
The production change in src/upstream/ruleset.ts:1085 adds the right fail-closed guard for non-digit issue numbers before converting with Number(). The implementation itself is narrowly scoped and matches the PR description, but the new unit test is not wired to the production parser. As written, the test suite can pass while parseGitHubIssueUrl in src/upstream/ruleset.ts regresses, so the PR does not actually cover the behavior it claims to protect.

Blockers

  • test/unit/parse-github-issue-url.test.ts:4 reimplements parseGitHubIssueUrl locally instead of importing or exercising the production function, so this is a fabricated test that will not catch regressions in src/upstream/ruleset.ts.
Nits — 4 non-blocking
  • nit: src/upstream/ruleset.ts:1086 still accepts digit strings that exceed JavaScript's safe integer range; consider Number.isSafeInteger(number) if this parser must preserve exact issue identifiers.
  • test/unit/parse-github-issue-url.test.ts:4 should import the real parser if it can be exported safely, or exercise the public ruleset path that calls parseGitHubIssueUrl so the src/upstream/ruleset.ts:1085 branch is hit.
  • src/upstream/ruleset.ts:1086 can be tightened to `Number.isSafeInteger(number)` alongside `Number.isInteger(number)` if oversized numeric path segments should fail closed.
  • 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-github-issue-url.test.ts:4 reimplements parseGitHubIssueUrl locally instead of importing or exercising the production function, so this is a fabricated test that will not catch regressions in src/upstream/ruleset.ts.
Signal Result Evidence
Code review ❌ 1 blocker 1 reviewer
Linked issue ✅ Linked #1719
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; size label size:XS.
Validation evidence ❌ 5/25 Cached preflight status is hold.
Open PR queue ❌ 3/10 21 open PR(s), 9 likely reviewable, 12 unlinked.
Contributor context ✅ Confirmed Gittensor contributor galuis116; Gittensor profile; 1550 PR(s), 157 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

@gittensory-orb gittensory-orb Bot added gittensor Gittensor contributor context gittensor:bug Gittensor-scored bug fix - worth 0.5x multiplier. labels Jun 29, 2026
…nteger

Export parseGitHubIssueUrl to allow proper test coverage and update the numeric validation to use Number.isSafeInteger instead of Number.isInteger for safer bounds checking. This addresses Gittensory feedback about test coverage and numeric validation.

Addresses Gittensory review feedback on PR JSONbored#1720.
@dosubot dosubot Bot added the size:XS This PR changes 0-9 lines, ignoring generated files. label Jun 29, 2026
@superagent-security superagent-security Bot removed the size:XS This PR changes 0-9 lines, ignoring generated files. label Jun 29, 2026
The previous refactor left a stray `export export function` which is a
parse error and broke every Vitest suite. Reduce to a single `export`.
@dosubot dosubot Bot added the size:XS This PR changes 0-9 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 size:XS This PR changes 0-9 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

fix(upstream): validate issue number shape before Number() constructor in parseGitHubIssueUrl

1 participant