Skip to content

fix: working generic refresh webhook; document raw as the backend-private asset slot (Phase 7 PR A) - #43

Merged
dopry merged 5 commits into
nextfrom
claude/pecans-phase-7a-backend-abstraction
Jul 11, 2026
Merged

fix: working generic refresh webhook; document raw as the backend-private asset slot (Phase 7 PR A)#43
dopry merged 5 commits into
nextfrom
claude/pecans-phase-7a-backend-abstraction

Conversation

@dopry

@dopry dopry commented Jul 11, 2026

Copy link
Copy Markdown
Owner

Summary

First of the three Phase 7 PRs (backend abstraction → resolution pipeline → HTTP split). This one clarifies the backend↔core contract and fixes a broken endpoint; HTTP behavior of all download/update routes is unchanged (Phase 1 contract suite passes untouched).

Backend contract: raw is the backend-private slot, typed via TRaw

An asset is only ever served by the backend that created it — serveAsset/getAssetStream are abstract Backend methods that core always delegates to — so per-backend serving state is that backend's internal detail, not public model surface. The PR encodes that instead of adding new public fields:

  • PecansAssetDTO.raw is documented as the backend-private slot: the backend that normalizes an asset may stash whatever it needs there, and core and other consumers must not interpret it. This is the pattern the codebase has had since 1.x, now stated as a rule.
  • PecansAssetDTO<TRaw = unknown> / PecansAsset<TRaw = unknown> / Backend<TRaw = unknown>: a backend declares its payload type once (PecansGitHubBackend extends Backend<GithubReleaseAsset>) and gets fully typed raw reads back in serveAsset/getAssetStream — no subclassing of the model needed. At the neutral default raw is unknown, so consumers must narrow instead of dot-accessing what used to be any. Type-level change only; zero runtime difference. (Type-level break: code reading asset.raw.foo against the neutral type no longer compiles — that's the hint safety this buys.)
  • The GitHub backend's reads of its own raw gain clear missing-field errors (Asset 123 has no browser_download_url in its raw payload) instead of crashing with Cannot read properties of undefined on malformed assets.

An earlier iteration of this PR added public downloadUrl/apiUrl fields to the DTO; review discussion concluded that was itself a leaky abstraction (core never needs them, and they would grow the already-serialized /api/versions asset JSON), so it was reverted in favor of the documented-opaque-raw contract.

Fix: generic refresh webhook could never authenticate

The base Backend.getRefreshWebhookMiddleware compared the stored secret hash against req.params.secret — which is never populated on use()-mounted middleware, so any request with a configured refreshSecret failed. Now:

  • POST-only on the watched path (matching the GitHub webhook middleware); other methods fall through, so crawlers following a shared ?secret= link can't trigger refreshes (Copilot review finding).
  • The secret arrives via an X-Pecans-Secret header or ?secret= query parameter (the middleware has no route params to read).
  • Comparison is constant-time (crypto.timingSafeEqual over sha256 digests).
  • Mismatch → 403 via a new ForbiddenError (exported, slots into the Phase 6 error hierarchy); success → 200 {"refreshed": true}.
  • No refreshSecret configured → pass-through (endpoint stays disabled), unchanged.

The GitHub backend's override (signed-payload verification via @octokit/webhooks) is untouched. Documented in a new README "Cache-refresh webhook" section.

Verification

  • npm test: 31 files / 717 passed (new coverage: webhook middleware unit + e2e tests through the router's error handler, POST-only gate, raw-passthrough and missing-url error pins)
  • lint, typecheck, format, build green
  • packed tarball smoke-tested from CJS and ESM, plus a strict-mode TS consumer check: TRaw inference works, neutral raw is unknown (dot access fails to compile), PecansGitHubBackend still satisfies the neutral Backend slot

🤖 Generated with Claude Code

https://claude.ai/code/session_015zUyj4PpSog9RkrYxzFRhM

…fresh webhook (Phase 7A)

Backend abstraction, first of three Phase 7 PRs:

- PecansAssetDTO gains explicit downloadUrl (public redirect target) and
  apiUrl (authenticated fetch target); the GitHub backend populates them
  in normalizeAsset, and serveAsset/getAssetStream read them instead of
  reaching into the GitHub-shaped raw payload. raw stays populated and a
  constructor fallback keeps pre-existing DTO producers working (compat
  shim removed in 3.0).
- Fix the base Backend.getRefreshWebhookMiddleware: it compared the
  stored hash against req.params.secret, which is never populated on
  use()-mounted middleware, so the endpoint could never authenticate.
  The secret now arrives via X-Pecans-Secret header or ?secret= query,
  is compared in constant time, 403s (new ForbiddenError) on mismatch,
  and responds 200 {refreshed: true} on success. Documented in README.

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

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 makes the backend↔core contract more explicit by adding explicit asset URLs to PecansAssetDTO, and fixes the generic (non-GitHub) cache-refresh webhook authentication so it can actually be used when mounted via use().

Changes:

  • Add downloadUrl (public redirect target) and apiUrl (authenticated fetch target) to PecansAssetDTO, with a documented compatibility fallback to the GitHub-shaped raw payload (planned removal in 3.0).
  • Update the GitHub backend to populate and consume these explicit URLs (including erroring when missing), reducing downstream coupling to GitHub REST shapes.
  • Fix the base Backend.getRefreshWebhookMiddleware to authenticate via X-Pecans-Secret header or ?secret= query param using constant-time comparison, returning 403 via a new ForbiddenError, and returning 200 { refreshed: true } on success; add docs + broaden test harness to support non-GitHub backends.

Reviewed changes

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

Show a summary per file
File Description
test/unit/PecansAsset.spec.ts Adds unit coverage for downloadUrl/apiUrl propagation and raw-payload fallback behavior.
test/unit/github.spec.ts Updates GitHub backend tests to validate redirects/fetches use explicit URLs with raw fallbacks and missing-URL errors.
test/unit/backend-complete.spec.ts Updates base-backend webhook middleware tests for header/query secret auth, 403 behavior, and 200 JSON success response.
test/integration/webhook.spec.ts Adds integration tests covering the generic refresh webhook behavior end-to-end (non-GitHub backend).
test/harness.ts Generalizes configurePecansTestApp to accept any Backend, enabling non-GitHub integration testing.
src/models/PecansAsset.ts Introduces downloadUrl/apiUrl fields on asset DTO/model with compatibility fallback from raw.
src/errors.ts Adds ForbiddenError (HTTP 403) to the typed error hierarchy.
src/backends/github.ts Populates and uses explicit asset URLs in normalizeAsset, serveAsset, and getAssetStream with raw fallbacks.
src/backends/backend.ts Fixes refresh webhook secret extraction for use()-mounted middleware, adds constant-time verification, and responds 200 JSON on refresh.
README.md Documents the cache-refresh webhook for GitHub and non-GitHub backends (header/query secret, 200/403 responses).

… asset

The base PecansAsset model no longer falls back to the GitHub-shaped
raw payload for downloadUrl/apiUrl - raw has always been a private
contract between a backend and itself (in 1.x its only reader was the
GitHub backend's serveAsset), so the neutral model should not know
GitHub's field names. The GitHub backend keeps its own raw fallbacks
for legacy raw-only DTOs it is asked to serve (removed in 3.0).

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

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 10 out of 10 changed files in this pull request and generated no new comments.

Reverts the downloadUrl/apiUrl DTO fields added earlier in this PR.
Assets are only ever served by the backend that created them (serveAsset
and getAssetStream are abstract Backend methods that core delegates to),
so per-backend serving state does not belong on the public model - any
backend can stash what it needs in raw, now documented as opaque outside
the creating backend. This also avoids growing /api/versions JSON output,
which already serializes assets. The GitHub backend keeps the clear
missing-url errors instead of crashing on undefined raw fields.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_015zUyj4PpSog9RkrYxzFRhM
@dopry dopry changed the title feat: backend abstraction — explicit asset URLs, working generic refresh webhook (Phase 7 PR A) fix: working generic refresh webhook; document raw as the backend-private asset slot (Phase 7 PR A) Jul 11, 2026
@dopry
dopry requested a review from Copilot July 11, 2026 23:38

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 10 out of 10 changed files in this pull request and generated 1 comment.

Comment thread src/backends/backend.ts
…ok to POST

- PecansAssetDTO<TRaw = unknown> / PecansAsset<TRaw = unknown> /
  Backend<TRaw = unknown>: backends declare their private raw payload
  type once (PecansGitHubBackend extends Backend<GithubReleaseAsset>)
  and get typed raw reads in serveAsset/getAssetStream; everyone else
  sees unknown and must narrow instead of dot-accessing an any.
  Type-level change only - no runtime difference.
- getRefreshWebhookMiddleware only handles POST on the watched path
  (matching the GitHub webhook middleware); other methods fall through
  so crawlers following a shared ?secret= link can't trigger refreshes
  (Copilot review).

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

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 11 out of 11 changed files in this pull request and generated 2 comments.

Comment thread src/backends/backend.ts Outdated
Comment thread src/backends/github.ts
- serveAsset's missing-Location error now includes the asset id, API
  URL, and HTTP status (Copilot review).
- The base serveAsset doc claimed an LRU cache that has never existed;
  it now states that backends must override it (Copilot review).

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

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 11 out of 11 changed files in this pull request and generated no new comments.

@dopry
dopry merged commit 23d1d9f into next Jul 11, 2026
3 checks passed
@dopry
dopry deleted the claude/pecans-phase-7a-backend-abstraction branch July 11, 2026 23:59
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.

3 participants