feat: typed HTTP errors with router-scoped error handling (Phase 6) - #42
Merged
Conversation
Adds src/errors.ts: an HttpError hierarchy (BadRequestError 400,
NotFoundError 404) that the existing Unsupported{Platform,Channel,Tag}
Error classes now extend, plus an errorHandler middleware translating
HttpErrors into their status codes (JSON or text via content
negotiation) with a logged 500 fallback.
The middleware registers at the end of the pecans router, so composed
host apps get correct status codes without extra wiring; it is also
exported for host apps to use on their own routes, and main() installs
it as the app-level fallback.
Client errors that previously surfaced as 500s now return 4xx:
- 400: unknown/invalid platform, invalid tag or semver range, missing
update parameters, missing platform segment, unsupported filetype
- 404: unknown channel, version/release not found, missing RELEASES
asset, /notes for an unknown version (previously crashed on
undefined), no matching release/asset
Squirrel contract responses (204/200 shapes, RELEASES output) are
unchanged; only error paths moved. Moving the error classes into
src/errors.ts also dissolves the pecans <-> versions circular import.
The classes are re-exported from the package root as before; the update
handlers gain early semver-range validation so a bad version fails at
the parameter boundary.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_015zUyj4PpSog9RkrYxzFRhM
There was a problem hiding this comment.
Pull request overview
This PR introduces a typed HTTP error hierarchy and a router-scoped Express error handler so Pecans’ client-caused failures return appropriate 4xx status codes instead of defaulting to 500s. It centralizes error translation/formatting in src/errors.ts, installs the middleware at the end of pecans.router, and updates unit/integration tests to match the new status-code behavior.
Changes:
- Add
HttpError/BadRequestError/NotFoundError+errorHandler()middleware, and migrate existing “Unsupported*” errors onto this hierarchy. - Update Pecans route handlers to throw typed errors (and forward to
next) instead of writing ad-hoc 4xx/5xx responses. - Update unit + integration tests to assert the new 400/404 behaviors and add dedicated middleware/hierarchy tests.
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| src/errors.ts | Introduces typed HTTP errors and the shared Express error-handling middleware. |
| src/pecans.ts | Switches many failure paths to typed errors and installs router-scoped errorHandler(). |
| src/versions.ts | Emits NotFoundError instead of generic Error when resolve finds no versions. |
| src/index.ts | Re-exports errors and installs errorHandler() as app-level fallback outside pecans.router. |
| test/unit/errors.spec.ts | Adds unit coverage for error types and errorHandler() behavior. |
| test/unit/pecans.spec.ts | Updates unit assertions to expect typed errors via next(err) and updated messages. |
| test/unit/versions.spec.ts | Updates imports to new error module location. |
| test/integration/update-win.spec.ts | Pins new 404 behavior for update failure paths. |
| test/integration/update-mac.spec.ts | Pins new 400 behavior for invalid platform/version/missing params. |
| test/integration/download.spec.ts | Pins new 400/404 behavior for invalid route/platform/no asset cases. |
| test/integration/dl.spec.ts | Pins new 404 behavior for unknown channel cases. |
| test/integration/api.spec.ts | Pins new 404 behavior for unknown release notes version. |
Review feedback: the update handlers validated the :version param with validRange, but build their filter as '>=' + version - a range-shaped input like '>=1.0.0' passed validation, produced the invalid combined range '>=>=1.0.0', and 500ed. Squirrel clients report their installed version, so validate with semver.valid (specific version) and 400 on anything else. Pinned on both mac and windows update routes. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015zUyj4PpSog9RkrYxzFRhM
Review feedback:
- errorHandler serves plain text to Accept: */* clients (matching the
pre-typed-errors middleware) and JSON only when explicitly requested
- /notes{/:version} honored its path param in the route pattern but read
only the query string, silently serving the LATEST release's notes for
/notes/99.0.0; the path param now wins, 404s for unknown versions, and
400s for non-semver values
- drop a trailing space in the Unrecognized OS message
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_015zUyj4PpSog9RkrYxzFRhM
Review feedback: - security: error messages embed user-controlled url values (platform, version, tag); the text/html branch reflected them under an html content type (reflected XSS in a browser), and the default branch had the same flaw because res.send(string) defaults to text/html. The handler now serves text/plain or JSON only, with the default branch forcing text/plain. This flaw predates the typed-error work - the old inline middleware also had a text/html branch. - /notes/:version now accepts the 'latest' keyword like ?version=latest, and the error message matches the actual validRange check Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015zUyj4PpSog9RkrYxzFRhM
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 12 out of 12 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (2)
src/pecans.ts:304
serveAsset()is async; calling it withoutawaitmeans errors from the backend (or fromserveAssetitself) will not be caught by this try/catch, so they won’t reachnext(err)and won’t be translated by the router’serrorHandler(). This can surface as unhandled promise rejections under load or network failure.
throw new NotFoundError("No Matching Assets Found");
}
const asset = matchingAssets[0];
this.serveAsset(req, res, release, asset);
} catch (e) {
src/pecans.ts:466
- This returns the Promise from
serveAsset()without awaiting it, so any rejection from the backend can bypass this handler’s try/catch and won’t be forwarded tonext(err)(Express 4 doesn’t automatically handle async promise rejections). Awaiting here ensures failures are consistently routed througherrorHandler()instead of becoming unhandled rejections.
if (!asset)
throw new NotFoundError(
`No download available for platform ${platform} for version ${release.version} (${channel})`,
);
// Call analytic middleware, then serve
return this.serveAsset(req, res, release, asset);
} catch (err) {
next(err);
}
Review feedback: serveAsset() was fire-and-forgotten in dlfilename and dl, and returned-without-await in handleDownload (the return-vs-return- await pitfall), so a backend rejection orphaned the promise - the request hung and the error never reached errorHandler. All three sites now await. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015zUyj4PpSog9RkrYxzFRhM
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
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Phase 6 of the modernization roadmap: client errors stop masquerading as 500s.
The hierarchy (
src/errors.ts)HttpErrorbase carrying astatusCode;BadRequestError(400) andNotFoundError(404); the existingUnsupported{Platform,Channel,Tag}Errorclasses now extendBadRequestError.errorHandler()middleware translatesHttpErrors into their status codes (JSON{error, code}or plain text via content negotiation) and logs a 500 fallback for everything else, with aheadersSentguard.pecans.routergets correct status codes with zero extra wiring. Also exported for host apps' own routes, andmain()installs it as the app-level fallback.src/errors.tsdissolves the long-standingpecans.ts ↔ versions.tscircular import; everything stays re-exported from the package root.Status-code changes (error paths only)
/notesfor an unknown version (previously crashed onundefined.notes)The update handlers also gain early semver-range validation so a bad
:versionfails at the parameter boundary with attribution, instead of deep in release matching.Squirrel contracts unchanged: 204/200 semantics, update JSON shape, and byte-exact RELEASES output are untouched — only failure paths moved, and Squirrel clients treat any non-2xx as "no update" either way. The Phase 1 contract tests for success paths pass unmodified; the ~14 tests that pinned 500s (several explicitly annotated "until typed error handling lands") now pin their intended 4xx codes.
Verification
npm test: 31 files / 699 passed (5 new error-module tests); lint, typecheck, format, build all green.🤖 Generated with Claude Code
https://claude.ai/code/session_015zUyj4PpSog9RkrYxzFRhM
Generated by Claude Code