feat(desktop): migrate Nightly to GitHub Releases - #4317
Conversation
93be1f8 to
69eaca6
Compare
Generated-by: Codex
Generated-by: Codex
Generated-by: Codex
Generated-by: Codex
69eaca6 to
6af9173
Compare
jackwener
left a comment
There was a problem hiding this comment.
Reviewed the latest head 6af9173a8. I am not approving: the package check is red and this PR caused it. You said approve only if there is no P0/P1, so I am reporting instead. The cause is small and the fix is two lines — details inline.
First, the part that matters most, and it is good news. This migration removes the rsync/SSH publish path entirely — there is no rsync, no NIGHTLIES_RSYNC_*, and no StrictHostKeyChecking left in desktop-nightly.yml. That resolves the P1 I raised on #4254 (host key verification disabled on the Nightlies transport) by deleting the transport rather than patching it, and it also retires the --delete guard P3 I raised on #4271, #4280 and #4282, since those rsync steps no longer exist. Replacing an unverified SSH upload with GitHub Releases plus per-asset attestation (Attest every GitHub Nightly asset subject, add-attestation) is a materially stronger integrity story than what it replaces. Worth stating plainly: this PR closes my longest-standing open finding.
The blocker is a Windows path-separator bug in two new assertions. The package job fails on "Package the Windows installer and ZIP" with:
expected: /\/dev-mac\.yml$/u
actual: 'D:\a\maka\maka\apps\desktop\release\dev-mac.yml'
The packaging is correct — the right file is produced in the right place. The assertion is what is wrong: it requires a literal forward slash before the filename, and on a Windows runner the separator is a backslash, so it can never match. Both new assertFile callbacks have the same defect, so both the macOS-wrapper and Windows-wrapper tests fail. It would pass on a Linux-only run, which is presumably why it slipped through.
138 tests pass and only these 2 fail, so nothing else in the change is implicated.
I will re-review as soon as that is green.
Automated review notice: This comment was posted by an automated review agent operated by jackwener. It is not an independent human review and does not replace one.
hqhq1025
left a comment
There was a problem hiding this comment.
I found one P1 and do not recommend merging this head yet.
The Windows release gate is failing because the two new wrapper assertions in scripts/desktop-nightly.test.mjs require POSIX / separators. On the Windows runner, the real paths end in \\dev-mac.yml and \\dev.yml, so both assertions fail even though the package metadata names are correct. This is already captured by the exact-head inline comment on the first assertion; I independently reproduced both failures and am not duplicating the same inline finding. Comparing basename(path) with the expected filename would make the contract platform-neutral.
I found no additional P0-P3 issues in the GitHub prerelease publisher, tag/source binding, exact asset/digest checks, dev update-channel configuration, or downloaded-update attestation path.
Validation completed on 6af9173a84ab11f33a10a7ff25faa028d43cccdc: clean install, full build, full typecheck, release contracts (141/141 on Linux), focused Nightly publisher/update tests, app update service and attestation tests, lint, formatting, diff check, and a clean merge tree against 6e6af952a6335fa904c1da7037ccc6269b36928a. The hosted package check fails with the two Windows path assertions; the hosted test check was still running at final refresh.
I could not execute signed/notarized macOS packaging, a real Nightly publication, or verify the repository's Immutable Releases setting from this environment.
Automated review notice: This comment was posted by an automated review agent operated by hqhq1025. It is not an independent human review and does not replace one.
Generated-by: Codex
zhiiw
left a comment
There was a problem hiding this comment.
Reviewed at exact head 6af9173a (verified unchanged at review time).
Not approving: the package check is red on this head. The Windows packaging job runs check:release, and the two new wrapper tests fail there with a POSIX-only path assertion — see the inline [P1]. The same failure reproduces on my Windows machine, so this is deterministic, not flake.
What I verified before the gate:
- The migration removes the rsync chain cleanly: the SSH transport, feed-advance read, and dual-directory staging are gone; publication is now draft-prerelease → exact-asset verification → publish, with the draft made non-draft only after the complete verified asset set is present (the feed-last equivalent holds).
- Fail-closed properties are real: tag/source drift rejected via
remoteProductTagCommit; a pre-existing draft with unexpected or changed assets is rejected byassertAssetSubset(name+size+digest); the final state must match the local bytes exactly, checked twice (PATCH response and a fresh re-view). - The channel split is coherent: packaged Nightlies use the GitHub
devchannel (dev-mac.yml/dev.yml), formal releases keeplatest*(the wrappers select bybuildVersion === manifest.version), and attestation URLs now resolve to the release assets for every channel. - Executed on a real Windows machine: the nightly script suites 16/18 — the 2 failures are exactly the inline P1.
Automated review notice: This comment was posted by an automated review agent operated by zhiiw. It is not an independent human review and does not replace one.
简体中文
不批:本 head 的 package 检查红——Windows 打包作业跑 check:release,两个新 wrapper 测试的 POSIX 路径断言在那里挂(详见行内 P1;我本机 Windows 确定性复现)。闸前已核实:rsync 链整体拆除干净;发布序列为 draft → 精确资产校验 → 转正式(feed-last 语义保持);fail-closed 属性真实(tag/源漂移拒、草稿资产子集校验 name+size+digest、终态双重核验);channel 划分一致(nightly 走 dev 频道、正式发布保留 latest*,wrapper 按 buildVersion===manifest.version 选择)。本机套件 16/18,2 个失败即行内这条。
M4n5ter
left a comment
There was a problem hiding this comment.
English
Approved at d284fca.
I found no unresolved blocking correctness, security, recovery, or design issues. The previous Windows path-separator failure is correctly fixed with basename(), and the focused Nightly suites pass 18/18.
One non-blocking simplification suggestion: .nightly-input is validated and then copied unchanged into .nightly-stage/release. The workflow could validate, attest, add the bundle, and publish directly from the downloaded directory. This would remove one redundant asset representation and its rm/mkdir/copyFile path without weakening the exact-set, metadata, attestation, or final digest checks.
The publication authority, exact tag/source binding, draft-before-publish sequence, GitHub dev update channel, differential metadata, and downloaded-update attestation all look coherent.
Real signed/notarized publication and two consecutive Nightly updates remain rollout validation tasks; GitHub Immutable Releases must also be enabled before distribution. These do not block approval of this code change.
Automated review notice: This review was produced by an AI-assisted review agent operated by M4n5ter. It does not replace independent human review.
简体中文
已审查最新 head d284fca,批准。
没有发现未解决的 correctness、安全、恢复或设计阻塞问题。此前 Windows 路径分隔符导致的失败已通过 basename() 正确修复,Nightly 定向测试 18/18 通过。
有一个非阻塞的简化建议:.nightly-input 通过验证后,被原样复制到 .nightly-stage/release。可以直接在下载目录完成验证、attestation、加入 bundle 和发布,从而删除一套重复的资产表示以及对应的 rm/mkdir/copyFile 路径,同时保留 exact-set、metadata、attestation 和最终 digest 校验。
发布 authority、精确 tag/source 绑定、先 draft 后公开、GitHub dev 更新通道、差分更新 metadata 和下载后 attestation 验证均保持一致。
真实签名/notarization 发布和连续两个 Nightly 的自动/差分更新仍需在 rollout 阶段验证;正式分发前也必须开启 GitHub Immutable Releases。这些不阻塞本次代码审批。
自动化审查说明: 本审查由 M4n5ter 操作的 AI 辅助审查代理生成,不能替代独立人工审查。
jackwener
left a comment
There was a problem hiding this comment.
Re-reviewed at head d284fca86. The P1 is resolved — approving. All six checks are terminal green, including the package job on Windows that was failing.
The fix is correct and does not trade strength for portability, which is the thing I check first when a red test turns green:
- if (path.endsWith('.yml')) assert.match(path, /\/dev-mac\.yml$/u);
+ if (path.endsWith('.yml')) assert.equal(basename(path), 'dev-mac.yml');basename removes the path-syntax dependency entirely, and exact equality on the filename is stricter than the suffix match it replaces — the old regex would have accepted weird-dev-mac.yml, this will not. No assertion was deleted, no test skipped, and the commit touches three lines in one test file and nothing else. Both callbacks were fixed the same way.
The rest of the change is unchanged from 6af9173a8, so my earlier review stands. Restating the part that matters: this migration removes the rsync/SSH publish path entirely, which resolves the #4254 P1 about disabled host key verification by deleting the transport rather than patching it, and retires the --delete guard P3 I raised on #4271, #4280 and #4282 along with it. GitHub Releases with per-asset attestation is a stronger integrity story than what it replaces.
Automated review notice: This comment was posted by an automated review agent operated by jackwener. It is not an independent human review and does not replace one.
zhiiw
left a comment
There was a problem hiding this comment.
Re-reviewed at the current head d284fca8 (verified unchanged at review time; all checks completed/success on this head, including the previously red package at 22m18s and test at 24m34s).
The delta from my earlier review (6af9173a) is one test-only commit (+3/−3): both wrapper assertions now compare basename(path) instead of a forward-slash-anchored regex — exactly the platform-neutral fix, no production change. The previously failing suite passes on the same Windows machine that failed before (8/8), and the hosted Windows packaging job is green again.
The earlier verification stands unchanged: the rsync chain is cleanly removed, publication remains draft → exact-asset verification → publish with fail-closed tag/asset/digest checks, and the dev/latest channel split is coherent.
Automated review notice: This comment was posted by an automated review agent operated by zhiiw. It is not an independent human review and does not replace one.
简体中文
增量只有一个 test-only 提交(+3/−3):两处断言改用 basename 比较,正是行内 P1 建议的修法,生产代码未动。先前失败的套件在之前失败的同一台 Windows 机器上 8/8 过;本 head 的 package(之前红)与 test 全绿。此前对迁移主体的核实结论全部保留。
hqhq1025
left a comment
There was a problem hiding this comment.
Reviewed exact head d284fca8664aa6fb69e091788fc9968a0c1279cf. The previous P1 is fixed: both Nightly wrapper assertions now compare basename(path) with dev-mac.yml / dev.yml, so the contract is independent of platform path separators. The fix delta is limited to that test file, and the current Windows package job now passes.
I found no remaining P0-P3 issues in the complete current change. The GitHub prerelease publisher still binds the exact tag and source commit, validates the exact remote asset set and digests before publishing, keeps Nightly packages on the dev update channel, and verifies downloaded-update attestation from GitHub Release assets.
Validation on this head: clean install; build:test; full production build; full workspace typecheck; complete release gate 141/141; focused Nightly suites 18/18; full lint, format, ASF-header, and diff checks; clean merge tree against current main; and all current hosted checks terminal green. I could not independently execute signed/notarized macOS packaging, perform a real Nightly publication, or verify the repository Immutable Releases setting.
Review notice: This review was prepared by an automated review agent operated by hqhq1025 and is published at the direction of AstroHan, who has read these findings and is the human accountable for them.
M4n5ter
left a comment
There was a problem hiding this comment.
Approved at exact head d284fca8664aa6fb69e091788fc9968a0c1279cf. I found no P0–P3 issue in the current source.
The previous Windows-only release-test failure is closed by comparing basename() with dev-mac.yml / dev.yml; all exact-head hosted checks are now green, including the complete Windows packaging and update job. The Nightly publisher binds the exact npm version, source commit, protected tag, nine release assets, and remote SHA-256 records before one draft-to-prerelease mutation. Packaged Nightlies use the GitHub dev channel and verify downloaded bytes against the Nightly workflow's Sigstore identity.
The migration also removes the SSH/rsync credentials and transport, cutover marker, mutable dual-feed advancement, and Apache Nightlies compatibility path without introducing another distribution authority. Both prior review threads are resolved, and the PR is mergeable and clean against current main.
Automated review notice: This comment was posted by an automated review agent operated by M4n5ter. It is not an independent human review and does not replace one.
`check:release` conflicted on its single line: main added `scripts/desktop-nightly-release.test.mjs` (#4317) and `scripts/qualify-released-cli-state-root.test.mjs`, while this branch removed the `npm run check:asf-npm` chain. Both suites stay; the duplicate chain stays removed. Generated-by: Claude Code
Summary
v<version>tag, with the macOS and Windows installers, blockmaps,dev-mac.yml,dev.yml, and offline Sigstore bundledevupdate channel so the app continues to detect, download, verify, and install newer Nightlies automatically, including differential blockmap lookupVerification
npm run check:release— 141 tests passednode --test apps/desktop/dist/main/__tests__/app-update-attestation.test.js apps/desktop/dist/main/__tests__/app-update-service.test.js— 18 tests passednpm run lintnpm run format:checkactionlint .github/workflows/desktop-nightly.ymlgit diff --checkbuild:main,build:preload,build:overlay, andbuild:rendererpassed on the rebased exact headdev*.ymlwhile the platform wrappers requiredlatest*.yml; both wrappers now select metadata from the resolved build identity and have direct Nightly coverageRollout
apache/makabefore distributing Desktop Nightlies.DESKTOP_NIGHTLY_ENABLEDtotrue, dispatch a fresh npm Nightly frommain, and verify the Desktop workflow publishes one non-Latest prerelease with the exact tag and nine assets.Each Nightly remains a separate versioned prerelease. Reusing one mutable prerelease would conflict with immutable publication, updater version history, differential blockmaps, and rollback. Retention is intentionally left as a later operator policy rather than adding a second cleanup mechanism in this PR.
AI use
Select exactly one:
Tool(s) and scope: Codex implemented and simplified the migration, added tests and operational documentation, ran the verification, and coordinated independent adversarial review plus a Simplify audit. The commits include the required
Generated-by: Codextrailer.Checklist
Lint, format, release checks, affected Desktop builds, and affected suites pass. The unchecked item preserves the repository-wide typecheck caveat recorded earlier for unrelated
packages/uisources.Does this PR entail a change in behavior?