fix(scripts): bound gittensor-impact-card's fetches with a request timeout#7280
Conversation
…meout Two of the script's three outbound fetches (the two api.gittensor.io calls via fetchJson, and the gt-logo.svg fetch) had no AbortSignal.timeout at all, unlike every other external-fetch script in this repo -- a hung connection would block the README-card regeneration job indefinitely. Closes JSONbored#7231
…card test fetchImpl.mock.calls[0] is typed as any[] | undefined under this project's noUncheckedIndexedAccess, which array-destructuring assignment can't accept without a non-null assertion.
|
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 #7280 +/- ##
=======================================
Coverage 93.81% 93.81%
=======================================
Files 704 704
Lines 69458 69458
Branches 18896 18896
=======================================
Hits 65163 65163
Misses 3302 3302
Partials 993 993
Flags with carried forward coverage won't be shown. Click here to find out more. |
|
Tip ✅ LoopOver review result - approve/merge recommendedReview updated: 2026-07-19 12:22:29 UTC
Review summary Nits — 5 non-blocking
Decision drivers
Context & advisory signals — never blocks the verdict
Review context
Contributor next steps
Signal definitions
🧪 Chat with LoopOverAsk LoopOver a question about this PR directly in a comment — grounded only in the same cached, public-safe facts shown above, never a new claim.
Full command reference: https://loopover.ai/docs/loopover-commands 🧪 Experimental — new and may change. 🟩 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 LoopOver, a quiet PR intelligence layer for OSS maintainers.
|
Summary
scripts/gittensor-impact-card.mjs'sfetchJson(used for both theapi.gittensor.io/repos/:repo/impactand.../prscalls) and the standalonegt-logo.svgfetch had no request timeout at all — unlike every other external-fetch script in this repo (scripts/check-mcp-release-due.mjs'sGITHUB_REQUEST_TIMEOUT_MS,scripts/smoke-observability-metrics.mjs,src/github/client.ts'sGITHUB_FETCH_TIMEOUT_MS, etc.). A hungapi.gittensor.io/gittensor.ioconnection would block the README-card regeneration job (.github/workflows/gittensor-impact.yml) indefinitely.GITTENSOR_IMPACT_CARD_FETCH_TIMEOUT_MSconstant (10_000, exported) and passedsignal: AbortSignal.timeout(...)on all three call sites (both viafetchJson, plus the extractedfetchGtLogoSvghelper for the logo fetch, which previously wasn't even wrapped in a named function).fetchImplparameter (defaulting to globalfetch) so they're independently unit-testable without a real network call — matching this repo's established pattern for testable fetch adapters (e.g.scripts/load-test-worker.mjs).if (import.meta.url === pathToFileURL(...).href)) every other script in this repo already has around itsmain()invocation (e.g.scripts/check-mcp-release-due.mjs) — this script was missing it, so simply importing the module (as the new test file needs to, to reachfetchJson/fetchGtLogoSvg) previously ranmain()immediately and crashed viaprocess.exit(1). This is a pure test-enablement fix: CLI usage (node scripts/gittensor-impact-card.mjs <owner/repo> <out-file.svg>) and output are byte-identical — confirmed by running the script directly post-change and seeing the sameUsage: ...message on missing args.render/bucketWeekly/sparkline/meterpure helpers.Scope
type(scope): short summaryConventional Commit format.CONTRIBUTING.mdand does not reintroduce GitHub Pages, VitePress,site/, orCNAME.Closes #7231
Validation
git diff --checknpm run actionlintnode --check scripts/gittensor-impact-card.mjs— clean.npm run typecheck— the whole-repotsc --noEmitreliably OOMs on this shared, memory-constrained sandbox regardless of diff size (reproduced on a clean checkout too, documented pattern from prior PRs in this repo's history). A first push of this PR was closed by CI on a realtscerror (fetchImpl.mock.calls[0]destructured directly, but this project'snoUncheckedIndexedAccesstypes an array index asT | undefined, which a destructuring assignment can't accept without a non-null assertion) — my first scopedtscverification omitted that compiler option, so it missed the error CI's full-flag run caught. Root-caused and fixed (fetchImpl.mock.calls[0]!), then re-verified via a standalone scopedtsc --noEmitinvocation using this project's exacttsconfig.jsoncompiler options this time (--strict --exactOptionalPropertyTypes --noUncheckedIndexedAccess --noImplicitOverride --noFallthroughCasesInSwitch --forceConsistentCasingInFileNames), small enough to avoid the OOM — clean. The new.d.mtscompanion deliberately typesfetchImplas a narrow customImpactCardFetchshape rather thantypeof fetch, since the latter (in this repo's Cloudflare-Workers-typed environment) is overloaded in a way a plainly-typedvi.fn()mock can't satisfy — the same pitfall a previous PR in this repo (feat(scripts): add load-testing tooling for the Worker's key endpoints #7184) hit and fixed the same way.npx vitest run test/unit/gittensor-impact-card-script.test.ts— 7/7 passing: success path, non-2xx throw, theAbortSignalis passed on every request, a genuinely-hangingfetchImplstill resolves via the configured timeout (verified by stubbingAbortSignal.timeoutitself rather than waiting out a real 10s, since Node'sAbortSignal.timeoutschedules an internal timervi.useFakeTimers()doesn't intercept), the logo fetch's body-text return, and the documented default constant value.scripts/**is incodecov.yml'signorelist (confirmed — matches the issue's own note), so this PR carries no Codecov patch-coverage obligation; the new dedicated test suite is the local coverage proxy.npm run test:workers/npm run build:mcp/npm run test:mcp-pack/npm run ui:openapi:check(not applicable — this PR only touches one standalone script + its test + its.d.mts; no Worker route, MCP, UI, or OpenAPI surface changed)npm audit --audit-level=moderate(no dependency changes)If any required check was skipped, explain why:
npm run typecheckOOMs on this specific sandbox under current memory pressure regardless of diff size; a scopedtsc --noEmitagainst the new test file (with the same strict flags) is the local proxy, and CI's isolated runner performs the real whole-repotsc --noEmit.Safety
GETrequests against already-public endpoints; no auth/cookie/CORS/session changes.)UI Evidencesection below with JPG/JPEG or PNG screenshots. (N/A — no UI changes; the generated README card's visual output is byte-for-byte unchanged, only the network layer underneath it gained a timeout.)