Skip to content

fix(github): default octokit to native fetch to prevent empty release lists - #28

Merged
dopry merged 1 commit into
dopry:nextfrom
jjeff:fix/native-fetch-for-releases
Jun 26, 2026
Merged

fix(github): default octokit to native fetch to prevent empty release lists#28
dopry merged 1 commit into
dopry:nextfrom
jjeff:fix/native-fetch-for-releases

Conversation

@jjeff

@jjeff jjeff commented Jun 22, 2026

Copy link
Copy Markdown
Contributor

Fixes #27.

Problem

PecansGitHubBackend hard-wires node-fetch@2 as octokit's request.fetch. In some runtimes (reproduced on DigitalOcean App Platform against a repo with ~390 releases), node-fetch@2 fails to parse the large, gzipped, multi-page listReleases response, so octokit.paginate() silently resolves to []. Pecans caches zero releases and every download/update returns nothing — with no error logged. (Error responses such as a 404 still parse, so a bad token surfaces loudly while a valid token silently returns an empty list.)

Native fetch (undici) parses the same response correctly. This package already requires Node >=22, so globalThis.fetch is always available.

Change

  • Default octokit's request.fetch to globalThis.fetch.
  • Add an optional fetch to PecansGitHubBackendOpts so callers/tests can override the implementation.
  • Inject node-fetch in the integration-test backend helper so nock continues to intercept HTTP (nock v13 does not intercept undici). Production keeps the native default.

Asset-streaming downloads still use node-fetch (they rely on the Node-stream .body) and are intentionally left unchanged.

Verification

  • Full suite green with the CI-equivalent fake credentials: 631 passed (24 files), fully hermetic via nock.
  • tsc --noEmit clean.
  • Reproduced the original failure and confirmed the fix out-of-band: with native fetch, releases() returns all 392 releases (99 non-draft, stable latest 5.0.15); with node-fetch@2 in the affected runtime it returned empty.

Follow-up (not in this PR)

The injected node-fetch@2 is likely the source of long-standing deprecation warnings; a separate change could drop node-fetch@2 entirely and/or bump @octokit/rest from ^20.

🤖 Generated with Claude Code

Copilot AI review requested due to automatic review settings June 22, 2026 15:32

Copilot AI 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.

Pull request overview

This PR fixes a production-impacting issue in the GitHub backend where forcing Octokit to use node-fetch@2 could cause octokit.paginate() to silently return an empty release list for large, multi-page listReleases responses. It updates the backend to default to the runtime’s native globalThis.fetch (available on Node >=22 per this repo’s engine constraint), while still allowing tests/callers to inject an alternate fetch implementation.

Changes:

  • Default Octokit’s request.fetch to globalThis.fetch to avoid node-fetch@2 parsing failures on large release listings.
  • Add an optional fetch override to PecansGitHubBackendOpts so callers/tests can supply a custom fetch implementation.
  • Update integration tests to inject node-fetch so nock (v13) can continue intercepting Octokit HTTP calls.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

File Description
test/integration/integration.spec.ts Injects node-fetch into the backend in tests so nock can intercept Octokit requests when production defaults to native fetch.
src/backends/github.ts Adds opts.fetch and uses it (or globalThis.fetch) for Octokit requests to prevent silent empty release lists in some environments.

@dopry dopry left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Just remove node-fetch so it uses the built in. the comments are unnecessary

@dopry
dopry force-pushed the fix/native-fetch-for-releases branch 2 times, most recently from 81a4a8b to f42f7d4 Compare June 26, 2026 18:43
@dopry
dopry requested a review from Copilot June 26, 2026 18:43

Copilot AI 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.

Pull request overview

Copilot reviewed 6 out of 8 changed files in this pull request and generated 1 comment.

Comments suppressed due to low confidence (1)

package.json:24

  • The PR description mentions keeping node-fetch for asset streaming and injecting it in tests because nock v13 can’t intercept native fetch, but this PR removes node-fetch entirely and upgrades nock to v14 (which changes that constraint). Please update the PR description/issue text to match the actual approach taken here (native fetch everywhere + nock v14), or reintroduce the described node-fetch injection if that’s still desired.
  "dependencies": {
    "@octokit/rest": "^20.0.1",
    "@octokit/webhooks": "^12.0.3",
    "body-parser": "^1.19.0",
    "express": "^4.18.1",
    "express-useragent": "^1.0.15",
    "semver": "^7.3.7",
    "ts-node": "^10.8.1",
    "urljoin.js": "^1.0.0",
    "uuid": "^8.3.2"

Comment thread src/backends/github.ts
on user is claiming node-fetch@2 as octokit's `request.fetch` fails to parse
large, gzipped, multi-page `listReleases` response, so `octokit.paginate()`
silently resolves to an empty array. Pecans then caches zero releases and
every download 404/500s — with no error logged (error responses like 404
still parse, so a bad token surfaces loudly while a valid token silently
returns nothing).

regardless nock 14 and our current node.js support native fetch so
we can remove the dependency.
@dopry
dopry force-pushed the fix/native-fetch-for-releases branch from f42f7d4 to ec1a340 Compare June 26, 2026 18:59
@dopry
dopry merged commit 0f6eac6 into dopry:next Jun 26, 2026
1 check passed
github-actions Bot pushed a commit that referenced this pull request Jul 21, 2026
# [2.0.0-next.18](v2.0.0-next.17...v2.0.0-next.18) (2026-07-21)

* feat!: unified ReleaseService resolution pipeline on discrete os/arch/pkg (Phase 7 PR B) ([#44](#44)) ([8a2f06a](8a2f06a))

### Bug Fixes

* **github:** default octokit to native fetch to prevent empty release lists ([#28](#28)) ([0f6eac6](0f6eac6))
* honor route params in downloads, dead code removal, small fixes (Phase 2) ([#31](#31)) ([9fd8f79](9fd8f79))
* semantic-release trusted publishing ([4f1ee88](4f1ee88))
* semantic-release trusted publishing ([#53](#53)) ([3d4d4ce](3d4d4ce))
* working generic refresh webhook; document raw as the backend-private asset slot (Phase 7 PR A) ([#43](#43)) ([23d1d9f](23d1d9f))

### chore

* esm only ([#24](#24)) ([1b8a546](1b8a546))

### Features

* dependency modernization — Express 5, octokit 22, remove UA autodetection (Phase 5) ([#41](#41)) ([8ea329a](8ea329a))
* modernize packaging and dev tooling (Phase 3) ([#32](#32)) ([3345ed7](3345ed7))
* recognize .msix / .msixbundle assets and 'msix' package format ([#46](#46)) ([4f848b9](4f848b9)), closes [#26](#26)
* typed HTTP errors with router-scoped error handling (Phase 6) ([#42](#42)) ([e3b7b54](e3b7b54))

### BREAKING CHANGES

* @dopry/pecans is now ESM-only. require('@dopry/pecans')
is no longer supported; use import (Node >= 22.12).

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_017zXcPSv6FFTPgD59T4KudM

* refactor: import model types with import type in runtime modules

Follows up on Copilot review: PecansRelease/PecansReleases (and other
names used only in type positions) are classes, so tsc accepts plain
imports, but with verbatimModuleSyntax they would stay in the emitted
JS as runtime imports. Convert the type-only usages to import type to
keep the runtime module graph minimal and cycle-free.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_017zXcPSv6FFTPgD59T4KudM
* Pecans no longer exposes a versions property.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_015zUyj4PpSog9RkrYxzFRhM

* feat!: remove the Versions and resolveReleaseAssetForVersion adapters

The deprecation shims this PR introduced are dropped instead of carried
to 3.0: route handlers and consumers resolve through ReleaseService /
resolveAssetForRelease directly. The table-driven specs that pinned the
legacy composite-id resolution semantics are migrated onto the pipeline
(via platformToQuery) so the behavioral pins survive the adapter
removal; unique Versions coverage moved into service.spec.

Pre-existing deprecations (GitHubBackend, PecansSettings.timeout,
PecansReleaseDTO.channel) keep their 3.0 schedule.
* Versions, VersionFilterOpts, PlatformQuery, and
resolveReleaseAssetForVersion are no longer exported.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_015zUyj4PpSog9RkrYxzFRhM
* getArchFromUserAgent, getOsFromUserAgent, and
getPlatformFromUserAgent now take the internal UserAgentDetails type
instead of express-useragent's Details, and getArchFromUserAgent
defaults Windows and Linux to '64' (32-bit desktops are effectively
extinct; the function is not used internally).

pecans consumed exactly four booleans from the unmaintained
express-useragent package; src/utils/userAgent.ts derives them from the
User-Agent header directly, with mobile exclusions the old library
handled via separate flags (iOS UAs contain 'like Mac OS X', Android
UAs contain 'Linux'). The middleware attaches the same req.useragent
shape. Unit specs cover the parser; the Phase 1 UA-driven download
contract tests pass unchanged.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_015zUyj4PpSog9RkrYxzFRhM

* fix: validate update-route params through getStringParam consistently

Review feedback: handleUpdateOSX truthiness-checked req.params directly
but read values through getStringParam, and handleUpdateWin had no
version guard at all; a missing tag would have produced a '>=undefined'
range. Validate once through the helper and reuse the validated values.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_015zUyj4PpSog9RkrYxzFRhM

* fix: exclude Macintosh+Mobile webview UAs from macOS detection

Review feedback (partial): a Mobile token alongside Macintosh indicates
an iPad-class webview masquerading as a Mac; genuine macOS browsers
never send it. Fixture + test added. Note true iPadOS desktop-mode UAs
are byte-identical to Mac Safari and undetectable by any parser.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_015zUyj4PpSog9RkrYxzFRhM

* fix: short-circuit dlfilename when the filename param is absent

Review feedback: an undefined filename passed into queryReleases matches
every release (the predicate treats undefined as no-filter), which would
serve an arbitrary asset instead of a 404. Unreachable via the current
route but guarded for consistency with the update handlers.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_015zUyj4PpSog9RkrYxzFRhM

* feat: remove user-agent platform autodetection
* selecting a platform is now the client's
responsibility. GET / is no longer a download route, and the platform
segment is required on /download, /download/version/:tag, and
/download/channel/:channel (a missing platform returns 400). The
user-agent parser, its middleware, and the getPlatformFromUserAgent /
getArchFromUserAgent / getOsFromUserAgent helpers are removed.

Autodetection only ever served bare browser links - Squirrel update
clients and /dl/* always send explicit platforms - and reliable device
detection is better handled client-side where UA Client Hints are
available. Reverting this commit restores the feature wholesale if
anyone misses it.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_015zUyj4PpSog9RkrYxzFRhM

* fix: remove imports orphaned by the autodetection removal

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_015zUyj4PpSog9RkrYxzFRhM

* fix: validate tag ranges early in validateReqQueryTag

Review feedback: validRange's result was discarded, so invalid tags only
failed deep in release matching with a generic 'Invalid Range Specified'
error. Invalid ranges now throw UnsupportedTagError at the parameter
boundary ('latest' stays allowed), and the error message no longer says
'channel' for tags (copy-paste from UnsupportedChannelError).

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_015zUyj4PpSog9RkrYxzFRhM
* build output moves from dist/cjs + dist/mjs to a tsup
bundle (dist/index.js CJS, dist/index.mjs ESM). Deep imports into dist
paths no longer resolve; all models (PecansRelease, PecansAsset,
PecansReleases, ...) are now exported from the package root instead.

- replace the dual-tsc + fixup.sh build with tsup (CJS + ESM + d.ts +
  sourcemaps, node22 target); consolidate four tsconfigs into one
  typecheck-only tsconfig.json
- add a proper exports map with types for both module systems; verified
  with publint and arethetypeswrong (all green: node10/node16/bundler)
- declare debug and qs as real dependencies - both are imported directly
  but were only present transitively via express, which broke the ESM
  bundle (inlined CJS require calls)
- guard the run-directly check with typeof require so the ESM build is
  importable; node dist/index.js still starts the server
- ts-node out of runtime dependencies; dev now runs tsx watch; start
  runs the compiled dist; drop nodemon
- ESLint 9 flat config + prettier (replaces the stale mocha-era
  .eslintrc); fix the 19 findings it surfaced (unused imports/vars,
  case-block declarations, error causes, no-useless-assignment)
- add explicit @types/node; add lint/format/typecheck scripts
- package.json: type commonjs, sideEffects false, canonical repo url

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_015zUyj4PpSog9RkrYxzFRhM

* fix: rank .tar.gz assets by their full extension in resolveForVersion

path.extname reports '.gz' for .tar.gz filenames, so the sort fallback
ranked them at prefs.indexOf(-1) - ahead of every genuine preference -
whenever .tgz and .tar.gz assets coexisted. Use getSupportedExt, which
handles the double extension, matching the Phase 2 fix to
PecansAsset.satisfiesExtensions. Regression test added.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_015zUyj4PpSog9RkrYxzFRhM

* fix: throw Error from configure(), correct Listening typo

Review feedback: the default switch case in configure() threw a raw
string (no stack trace); the startup log said 'Lisening'. The test that
pinned the typo is updated to match.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_015zUyj4PpSog9RkrYxzFRhM

* fix: throw on invalid os in getDownloadExtensionsByOs

Review feedback: the switch had no default, so an invalid OperatingSystem
cast in at runtime silently returned undefined against the declared
SupportedFileExtension[] return type. Fail loudly instead; edge-case test
updated to pin the throw.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_015zUyj4PpSog9RkrYxzFRhM
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

PecansGitHubBackend silently returns empty release list (node-fetch@2 fails to parse large listReleases responses)

3 participants