Skip to content

fix: reject mismatched updater manifests before release - #30952

Closed
sechan-netizen wants to merge 1 commit into
stablyai:mainfrom
sechan-netizen:fix/release-manifest-version-gate
Closed

sechan-netizen wants to merge 1 commit into
stablyai:mainfrom
sechan-netizen:fix/release-manifest-version-gate

Conversation

@sechan-netizen

Copy link
Copy Markdown

ELI5

A release can contain the right ZIP files while its updater manifest still points at an older version. The release gate now rejects that release before it becomes visible to users.

What Changed

  • Verify every updater manifest's version matches the release tag.
  • Verify versioned ZIP, DMG, DEB, and RPM references in manifests match the release version.
  • Add regression coverage for a v1.4.230 manifest that still points to Orca-1.4.229-arm64-mac.zip.

Why

The existing gate only checked that every manifest-referenced asset existed. A stale asset could remain in the release and let the gate pass, while new clients received a 404 for the old filename under the new tag. Version consistency catches the mismatch before publication.

Linked Issue

Reported from the auto-update failure:
Cannot download .../v1.4.230/Orca-1.4.229-arm64-mac.zip, status 404

Visual Proof

N/A — release validation only; no UI changes.

Testing

  • node --check config/scripts/verify-release-required-assets.mjs
  • node --check config/scripts/verify-release-required-assets.test.mjs
  • Smoke-verified that a current 1.4.230 manifest passes and a stale 1.4.229 manifest fails, including when the stale ZIP is still present.

AI Disclosure

Not applicable — internal contributor.

Review

Agent skill upstream boundary

  • Not applicable, or this change copies or mechanically translates no upstream skill-installer source, tests, fixtures, registry entries, path tables, comments, or documentation.

Notes

The existing draft-only publication gate remains the enforcement point. This change adds manifest/version integrity checks to that gate and does not alter updater behavior for valid releases.

Checklist

  • This PR is small and focused
  • I explained what changed and why
  • Before/after screenshots or videos are not applicable
  • Self-reviewed for correctness, security, and performance
  • Cross-platform, SSH/remote, and path/shortcut impact considered (release metadata only)
  • pnpm lint, pnpm typecheck, pnpm test, and pnpm build pass (targeted validation run; full CI will cover the repository)

@coderabbitai

coderabbitai Bot commented Oct 20, 2026 •

Copy link
Copy Markdown
Contributor

Review in Change Stack →

📝 Walkthrough

Release verification now compares each manifest version with the release tag without its leading v. It also compares semantic versions in referenced asset names with the tag’s base version. Verification errors include manifest and asset version mismatches alongside existing missing, upload-state, and empty-asset errors. Tests cover manifest version extraction and rejection of a stale uploaded asset.

Priority: ➖ Normal

Severity of issue fixed: Low

Merge Risk: 🔵 Low · up to 09a0c

The release gate has a narrow regression-coverage gap: its new test would not catch a future failure to reject a stale ZIP reference. This is bounded merge risk; an asset-only test would close it.

Pre-merge checks: Passed 3 · Failed 2

Failed Failed checks: 2 warnings

Check name Status Explanation Resolution
Description check Warning The description explains the problem, changes, rationale, testing, and non-UI impact. However, the required Linked Issue section does not include an actual issue reference or Fixes # entry. Add the linked issue reference in the required format, such as "Fixes #123", and ensure it points to the issue addressed by this pull request.
Docstring Coverage Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 5 functions across 2 files. Write docstrings for the functions missing them to satisfy the coverage threshold.
Passed Passed checks: 3 passed
Check name Status Explanation
Title check Passed The title clearly and concisely describes the main change: rejecting updater manifests with versions that do not match the release.
Linked Issues check Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check Passed Check skipped because no linked issues were found for this pull request.
  • Fix all pre-merge checks with AI
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Actionable comments posted: 1


ℹ️ Review info
⚙️ Run configuration
  • Configuration used: Repository UI
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: 1466fb09-3961-4828-b891-01bd7d4f2373
📥 Commits

Reviewing files that changed from the base of the PR and between 907694d and 09a0cc5.

📒 Files selected for processing: 2
  • config/scripts/verify-release-required-assets.mjs
  • config/scripts/verify-release-required-assets.test.mjs

Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 9 remain after this review.

Comment on lines +143 to +169

it('rejects a stale manifest version even when the stale asset is still present', async () => {
const tag = 'v1.4.230'
const required = getRequiredReleaseAssetNames(tag)
const staleAsset = 'Orca-1.4.229-arm64-mac.zip'
const release = releaseWithAssets(tag, [...required, staleAsset])
const fetchMock = vi.fn(async (url) => {
if (url.endsWith('/assets/3')) {
return jsonResponse(
[
'version: 1.4.229',
'files:',
` - url: ${staleAsset}`,
`path: ${staleAsset}`
].join('\n')
)
}
return url.endsWith('/releases?per_page=100')
? jsonResponse([release])
: jsonResponse('version: 1.4.230\n')
})
vi.stubGlobal('fetch', fetchMock)

await expect(
verifyRequiredReleaseAssets({ repo: 'stablyai/orca', tag, token: 'token' })
).rejects.toThrow('Manifest version mismatch: latest-mac.yml=1.4.229 (expected 1.4.230)')
})

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🎯 Functional Correctness | 🟠 Major | ⚡ Quick win

🔎 Supported by static analysis

🏁 Script executed:

sed -n '1,210p' config/scripts/verify-release-required-assets.test.mjs
sed -n '45,180p' config/scripts/verify-release-required-assets.mjs

Repository: stablyai/orca

Length of output: 10018


🏁 Script executed:

set -o pipefail
printf '%s\n' '--- helper and verifier context ---'
sed -n '1,115p' config/scripts/verify-release-required-assets.mjs
printf '%s\n' '--- current test ---'
sed -n '115,190p' config/scripts/verify-release-required-assets.test.mjs
printf '%s\n' '--- full PR diff for the test ---'
git diff 907694dcbb23b1d2f60805c102b0f2a0ff999e75f 09a0cc5d0e8677c0ba6c078c1b34ca246de3d5e5 -- config/scripts/verify-release-required-assets.test.mjs
printf '%s\n' '--- references to asset-version validation/tests ---'
rg -n -F -- 'Manifest asset version mismatch' config/scripts
rg -n -F -- 'extractAssetBaseVersion' config/scripts

Repository: stablyai/orca

Length of output: 8301


Make the stale-asset test isolate asset-version validation.

The test sets the manifest version to 1.4.229, so it fails on the manifest-version check. It does not verify rejection when the manifest version matches the release tag. Change the manifest version to 1.4.230 and assert the asset-version mismatch.

Suggested fix
-  it('rejects a stale manifest version even when the stale asset is still present', async () => {
+  it('rejects a stale referenced asset even when the stale asset is still present', async () => {
...
-            'version: 1.4.229',
+            'version: 1.4.230',
...
-    ).rejects.toThrow('Manifest version mismatch: latest-mac.yml=1.4.229 (expected 1.4.230)')
+    ).rejects.toThrow(
+      'Manifest asset version mismatch: latest-mac.yml -> Orca-1.4.229-arm64-mac.zip'
+    )
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
it('rejects a stale manifest version even when the stale asset is still present', async () => {
const tag = 'v1.4.230'
const required = getRequiredReleaseAssetNames(tag)
const staleAsset = 'Orca-1.4.229-arm64-mac.zip'
const release = releaseWithAssets(tag, [...required, staleAsset])
const fetchMock = vi.fn(async (url) => {
if (url.endsWith('/assets/3')) {
return jsonResponse(
[
'version: 1.4.229',
'files:',
` - url: ${staleAsset}`,
`path: ${staleAsset}`
].join('\n')
)
}
return url.endsWith('/releases?per_page=100')
? jsonResponse([release])
: jsonResponse('version: 1.4.230\n')
})
vi.stubGlobal('fetch', fetchMock)
await expect(
verifyRequiredReleaseAssets({ repo: 'stablyai/orca', tag, token: 'token' })
).rejects.toThrow('Manifest version mismatch: latest-mac.yml=1.4.229 (expected 1.4.230)')
})
it('rejects a stale referenced asset even when the stale asset is still present', async () => {
const tag = 'v1.4.230'
const required = getRequiredReleaseAssetNames(tag)
const staleAsset = 'Orca-1.4.229-arm64-mac.zip'
const release = releaseWithAssets(tag, [...required, staleAsset])
const fetchMock = vi.fn(async (url) => {
if (url.endsWith('/assets/3')) {
return jsonResponse(
[
'version: 1.4.230',
'files:',
` - url: ${staleAsset}`,
`path: ${staleAsset}`
].join('\n')
)
}
return url.endsWith('/releases?per_page=100')
? jsonResponse([release])
: jsonResponse('version: 1.4.230\n')
})
vi.stubGlobal('fetch', fetchMock)
await expect(
verifyRequiredReleaseAssets({ repo: 'stablyai/orca', tag, token: 'token' })
).rejects.toThrow(
'Manifest asset version mismatch: latest-mac.yml -> Orca-1.4.229-arm64-mac.zip'
)
})

OrcaWin added a commit that referenced this pull request Oct 26, 2026
## ELI5

Orca can remember one release's ZIP filename and later follow “latest” into a newer release where that file does not exist. Resolve “latest” once, then keep the manifest and download on that concrete release.

## What Changed

- Replace the default-channel moving-feed fallback with a validated stable tag resolved from GitHub's `/releases/latest` redirect.
- Reuse the existing Electron `net.request` HEAD handling and platform manifest/asset readiness probes. `net.fetch` cancels manual redirects in real Electron, so it is not used for redirect discovery.
- Return a retryable check failure when a ready concrete fallback cannot be resolved, and reject fallback results belonging to an expired check.
- Update existing feed expectations and installer fixtures; cover stable fallback, Atom outages, unavailable fallback, late completions, redirect validation, and platform manifest selection.

## Why

The normal path already pins a tag, but Atom outages and no-newer results still installed a `/releases/latest/download` provider. That provider retains relative filenames while GitHub's redirect can move between checking and downloading. Correct release manifests alone cannot prevent the mismatch.

Credit to @brennanb2025 for the original tag-pinning fix in #1797, and @nwparker for the attempt-ownership guard in #23160, which this change extends across the fallback lookup. Also reviewed @sechan-netizen's #30952 and its review thread: its release-manifest validation is a complementary safeguard and remains separate because this race reproduces with correct manifests. No commits from that PR are copied here.

This PR adopts @brennanb2025's audited #34075 lint fix (splitting three files that already exceeded main's line limits), with Brennan retained as a co-author. Independent byte-level audits preserve the moved runtime bodies and test groups; the follow-up replaces five test-only assertions with typed calls and emits identical JavaScript. The standalone older PR remains unmerged after repeated daemon-test CI/order sensitivity; the combined tree here passed all CI, including that exact daemon test file (16 tests) in run 40398604134, job 121839803430.

## Linked Issue

Fixes #34034

Related reports: #15921 and #22621. The controlled race matches their URL signature and retry behavior; the reporter's original feed selection cannot be established without its logs.

## Visual Proof

N/A — updater feed selection only; no presentation change. Verified the HTTP behavior using the production fallback resolver and electron-updater 6.8.10 with unmodified v1.4.240/v1.4.241 manifests and a controlled local release server:

| Scenario | Result |
| --- | --- |
| Old moving provider: read 240 manifest, advance latest to 241, download | `v1.4.241/Orca-1.4.240-arm64-mac.zip`: 404 |
| Retry the old download without a new check | Same 404 |
| Fresh check | Correct 241 ZIP: 200 |
| Fixed fallback: resolve 240 tag, advance latest to 241, download | Correct 240 ZIP under tag 240: 200 |

No installer was executed and no app window was opened.

## Testing

- [x] I manually tested these changes locally
- [x] Automated tests added/updated

`ORCA_BACKGROUND_LAUNCH=1 pnpm check:changed --maxWorkers=2`: all 26 checks pass on the combined branch, including formatting, lint, typechecks, affected tests, and ratchets. The earlier updater-only run also passed all 20 selected checks.

The controller regression run against the original implementation produces five failures with passing controls. The fixed implementation passes. Resolver tests exercise macOS arm64, Windows x64, Linux x64/arm64 manifest selection, invalid redirects, missing assets, and recovery after network failure; these platform variants use mocked transport.

A separate background Electron 43.8.1 smoke on macOS arm64 uses the production resolver and real Electron networking against GitHub, resolving the ready `v1.4.241` tag. The controlled before/after reproduction also exercises the production resolver and actual GenericProvider. Neither proof exercises native installation.

All non-skipped GitHub CI checks pass for combined head `c04a6ca95604e064fb4f91adb1a57ea3dcf9adf0`: all five full unit shards, relay integration, Linux/Windows package checks, macOS updater regression tests, static analysis/typechecks, and the required aggregate check. Final independent review recommends merging that exact head as low risk with no code blockers.

## AI Disclosure

## Review

Independent sub-agent review found no actionable issues and rated the updater change low risk; 57 relevant tests passed independently. The prerequisite received two independent refactor audits, with identical moved runtime/test bodies and a merge-preservation gate reporting zero unexplained deletions and zero unrelated files. The five typed mock changes produce identical JavaScript. Also self-reviewed the feed-selection paths, timeout ownership, transport cleanup, and release-channel filtering. The added lookup runs only on the existing fallback path, uses HEAD for redirect discovery, retains five-second request deadlines, and reuses manifest/asset probes. No new polling, subprocess, persistent cache, or per-workspace work is introduced. Native Windows/Linux networking and installer execution were not exercised locally.

Review disposition for [the DMG-only manifest comment](#34076 (comment)): an independent executable comparison of the original and fixed implementations returned the same `ready` Atom result for a DMG-only manifest; the old fallback already handed that manifest directly to MacUpdater, which cannot select its required ZIP. This is an existing release-integrity limitation, not introduced or worsened by tag pinning. It remains outside this race fix; a complete integrity change must validate the native files list and architecture selection rather than merely finding any ZIP-like name.

## Agent skill upstream boundary

- [x] Not applicable; no upstream skill source or fixtures copied.

## Notes

Desktop updater implementation is shared with headless serve. No IPC/RPC contract, remote version requirement, mobile client code, filesystem path, or workspace-kind assumption changes. Existing RC/perf selection remains in place. If fallback discovery also fails, checks use the existing retry/error flow instead of launching a moving feed.

## Checklist

- [x] This PR is focused on consistent release selection
- [x] Problem, behavior, and mechanism explained
- [x] Visual proof N/A with network verification recorded
- [x] Self-reviewed for correctness, security, and performance
- [x] Cross-platform, remote, mobile, and folder-workspace impact considered
- [x] Required changed-code checks pass


Co-authored-by: Brennan Benson <79079362+brennanb2025@users.noreply.github.com>
@OrcaWin

OrcaWin commented Oct 26, 2026

Copy link
Copy Markdown
Collaborator

Closing this related updater proposal as part of the cleanup following merged #34076. Thank you for investigating this failure pattern; your work is credited in the merged PR.

The reproduced URL mismatch was fixed in the client by pinning the fallback release. This PR's additional publication-time manifest checks were not included in that merge; they are separate release-integrity hardening.

Please reopen if the problem recurs on a build containing the fix, or if a concrete malformed-manifest case demonstrates that these additional checks are still needed. Include the release tag, manifest, and failing URL so the remaining cause can be distinguished from the fixed redirect race.

@OrcaWin OrcaWin closed this Oct 26, 2026
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.

2 participants