fix(desktop): realign the release test with the new signing step - #11522
Conversation
The desktop release test pins the shape of the macOS signing step, and it
still described the allowlist that step used to be: the ripgrep and Node
section markers, the `find -exec … {} +` form, and `"$node_bin"` as the
signed path. Signing now discovers the Mach-O files under the staged
runtime instead of naming them, so every one of those assertions was
false and the step failed on all four platforms of release
0.3.0-preview.0 — after the runtime had been built.
Rewrite the assertions against what the step does now, keeping the
properties the old list guaranteed by construction: discovery spans the
whole runtime, only the Node.js branch passes entitlements, and the step
verifies what it signed. The empty-runtime guard replaces the ripgrep
warning it retired.
The reason this landed red is the second change. The job that runs this
test only fires when packages/desktop-shell, the update-manifest script,
or ci.yml change — but the file it asserts on is desktop-release.yml,
which is not in that list. A workflow-only edit therefore passes a green
Desktop Shell job in seconds without running the test that guards it.
Add the workflow to the filter.
|
✅ Qwen Triage finished — CI landed green on ✅ Qwen Triage 已完成 —— |
|
Thanks for the PR! Template looks good ✓ Problem: observed, and I checked both halves independently rather than taking the description's word for it. The Direction: CI and release-pipeline maintenance — nothing about what gets published or how changes, and the signing step itself is untouched. The general lesson you draw (a test that reads a file outside its own package needs that file inside its trigger) is the right one, and it is worth more than the immediate fix. Size: not applicable. No core paths — Approach: the scope feels right and I would not cut anything. The one tradeoff you already named is real: adding the release workflow to the filter means a workflow-only edit now buys a Rust compile plus Risk: no elevated risk signals — neither file matches the revert-correlated paths. Moving on to code review. 🔍 中文说明感谢贡献！ 模板完整 ✓ 问题：已观测到的真实故障，而且我独立核实了两部分，没有只采信 PR 描述。 方向：属于 CI 与发布流水线维护——发布什么、如何发布都没有变化，签名步骤本身也未被触碰。你提炼的通用结论（读取自身包之外文件的测试，必须把该文件纳入自己的触发条件）是对的，其价值超出这次修复本身。 规模：不适用。未触及核心路径—— 方案：范围合理，我不会砍掉任何部分。你已经点出的那个取舍确实存在：把发布 workflow 加入过滤条件后，一次只改 workflow 的提交要在两个 OS 上付一次 Rust 编译加 风险：无升级风险信号——两个文件都不匹配与回滚相关的高风险路径。 进入代码审查 🔍 — Qwen Code · qwen3.8-max-2026-09-02 Reviewed at |
Code reviewMy independent read before opening the diff: update the stale assertions to describe discovery-based signing, and add the release workflow to the job's path filter. That is what the PR does, and I did not find a simpler path — restructuring the signing logic into a testable script would be the "real" fix but is a much larger change than this warrants. I verified the central claim statically rather than trusting it. Every new assertion matches
I also checked the assertions the PR did not touch in the same function, since a stale one left behind would mean the realignment is incomplete: Two things worth saying about the entitlement check. It is stronger than what it replaces — the old one scoped The filter change is correct: the new alternative sits inside the existing Non-blocking — a property quietly stopped being pinned. The assertion you removed carried the message "ripgrep codesign failures must fail the signing step". Under the loop form that property now rests entirely on Non-blocking — the file mode change. Covered in the gate comment; it is harmless but unrelated and unexplained. Test evidenceThis is an unattended CI run, so per the gate rules I did not build or execute anything from this PR — the evidence below is the PR's own CI, read through the API, plus the static verification above. Final CI results for
One row per check name (latest run); skipped checks omitted; failures sort first. / 每个检查名一行（取最新一次运行），省略 skipped，失败项排在最前。 No check has failed at the time of writing; six are still running. Not verified, and no sandboxed lane can verify it: the filter half of the fix. This PR's own CI cannot exercise it, precisely because the diff matches the filter through two other paths — the job would run with or without the new alternative. Neither Everything else in the description that I could check without executing code checks out, with the one factual correction noted in the gate comment about how many platforms failed. 中文说明代码审查在看 diff 之前我自己的判断是：把过时断言改成描述"枚举式签名"，并把发布 workflow 加进该 job 的路径过滤条件。PR 正是这么做的，我也没找到更简的路径——把签名逻辑重构成可测脚本才是"根本解法"，但相对这里的收益来说改动大得多。 我没有采信 PR 的说法，而是静态核实了它的核心主张。每一条新断言都能对上 我也检查了同一函数中 PR 没有改动的断言，因为若留下过时断言，"重新对齐"就是不完整的： 关于 entitlements 这条检查有两点值得说。它比被替换的那条更强——旧断言把 过滤条件改动正确：新增分支位于既有 非阻塞——有一条性质悄悄不再被固定。 被删掉的断言带有"ripgrep codesign failures must fail the signing step"这条消息。改成循环写法后，该性质完全依赖第 365 行的 非阻塞——文件模式变更。 已在 Gate 评论中说明；无害，但与目标无关且未加解释。 测试证据这是无人值守的 CI 运行，按 gate 规则我没有构建或执行本 PR 的任何代码——下面的证据来自通过 API 读取的 PR 自身 CI，加上上述静态核实。 写作时没有检查失败，六项仍在运行。其中关键的是 有一项未验证，且没有任何沙箱通道能验证：过滤条件那一半修复。 本 PR 自身的 CI 无法检验它，恰恰因为 diff 通过另外两条路径已命中过滤条件——无论新增分支在不在，该 job 都会运行。 描述中其他能在不执行代码前提下核对的部分都成立，唯一的事实更正见 Gate 评论中关于失败平台数量的说明。 — Qwen Code · qwen3.8-max-2026-09-02 Reviewed at |
|
Confidence: 4/5 — everything I could check without running the code checks out, including the failure it claims to fix; two non-blocking nits. Going back to my own proposal: I would have written the same two changes, and I would not have thought to make the entitlement check a count across the whole step instead of a scoped The reason I am comfortable despite the whole PR being assertions over shell text: I could confirm the failure was real and specific. A release run died on My two reservations are both small and neither blocks. The dropped "codesign failures must fail the step" property is the one I would actually act on — it is a one-line assert, and it is the property that keeps a signing step from silently producing unsigned binaries, which is the whole reason this test exists. The file mode change I would just like explained or reverted, because an unexplained diff in a PR whose subject is precision about text is the sort of thing that erodes trust in the rest of it. Six months from now I would thank whoever wrote this rather than curse them. The comment explaining why the step enumerates instead of listing is the useful kind, and the filter change means the next person to edit the release workflow will actually hear about it. CI is still running, so I am not approving in this pass — approval is deferred until CI lands green on 中文说明Confidence: 4/5 —— 在不执行代码的前提下能核对的部分全部成立，包括它声称要修的那次故障；有两处非阻塞小问题。 回到我自己的方案：我会写出同样的两处改动，但我不会想到把 entitlements 检查做成"统计整个步骤中的出现次数"，而不是限定范围的 尽管整个 PR 都是对 shell 文本的断言，我仍然放心，原因是：我能确认那次故障真实且具体。一次发布运行死在 我保留的两点都很小，且都不构成阻塞。真正值得动手的是被舍弃的"codesign 失败必须让步骤失败"这条性质——它只需一行断言，而它守护的正是"签名步骤不会静默产出未签名二进制"，也就是这个测试存在的全部理由。文件模式变更我只是希望它被解释或还原：一个以"对文本的精确性"为主题的 PR 里出现未加说明的 diff，会让人对其余部分的信任打折扣。 六个月后回看，我会感谢写这段代码的人，而不是抱怨他。解释"为什么该步骤改为枚举而非逐个点名"的注释是有用的那类注释；过滤条件的改动意味着下一个修改发布 workflow 的人真的会收到反馈。 CI 仍在运行，因此本轮我不做批准——批准推迟到 CI 在 — Qwen Code · qwen3.8-max-2026-09-02 Reviewed at |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
LGTM, looks ready to ship — CI landed green after the review. ✅
chiga0
left a comment
chiga0
left a comment
There was a problem hiding this comment.
Tier: Standard (CI trigger + test assertions update — no persisted format, no wire protocol, no auth)
Scope: .github/workflows/ci.yml (trigger filter) · packages/desktop-shell/scripts/test-release.js (signing-step assertions). Generated/vendored files: none. Test files: reviewed as primary subject matter here.
What I checked
1. Each new assertion against the actual workflow content (desktop-release.yml @ head)
All seven new / changed assertions were verified against the workflow source at the current head SHA:
| Assertion | Match |
|---|---|
file -b --mime-type "$file" = 'application/x-mach-binary' |
✓ line 381 |
done < <(find "$runtime_dir" -type f -print0) |
✓ line 384 |
--entitlements src-tauri/NodeEntitlements.plist "$file" |
✓ line 396 |
entitlementFlags count ['--entitlements'] between step boundaries |
✓ exactly 1 in slice |
codesign --verify --strict "$file" |
✓ line 407 |
No Mach-O binary found under $runtime_dir |
✓ line 386 (now exit 1, was a warning) |
Node.js runtime binary not found at $node_bin |
✓ line 403 |
2. Removed assertions and their guarantee coverage
Each removed assertion's safety property is covered by a replacement or by set -euo pipefail:
- ripgrep/Node section markers → replaced by Mach-O discovery regex
ripgrep must not inherit the app entitlements→ replaced by entitlementFlags count (broader)find -exec … {} +(codesign failure bails the step) →set -euo pipefail+ per-codesigncall in loop achieves the sameRipgrep vendor directory not foundwarning → replaced byNo Mach-O binary founderror (stronger:exit 1)
3. entitlementFlags slice boundary safety
Both indexOf targets ("name: 'Sign bundled vendor binaries (macOS)'" and "name: 'Refresh bundled runtime checksums after signing") appear exactly once in the workflow; the slice is deterministic.
4. CI trigger filter correctness
The added pattern \.github/workflows/desktop-release\.yml in the grep -Eq call correctly matches the path .github/workflows/desktop-release.yml and only that file. Verified against the actual workflow path.
5. Cross-check
One existing review: qwen-code-ci-bot approved with "LGTM, looks ready to ship — CI landed green". No substantive findings to confirm or refute.
Coverage
No execution rung run (no working tree). The change has no runtime surface beyond running the JS test suite against the workflow file it reads — that requires an installed desktop package. Not a material gap for a test-assertion update whose output is deterministic on the workflow text.
Approval blockers: none.
Reviewed with AI assistance.
qwen-code-dev-bot
left a comment
qwen-code-dev-bot
left a comment
There was a problem hiding this comment.
Approved at head 4864d94c.
Required CI is green here — Test (ubuntu-latest, Node 22.x), Lint & Static, Integration Tests (no-AK, No Sandbox), web-shell E2E Smoke and both Desktop Shell lanes completed successfully; only review-pr is still running.
The part worth stating is that the retargeted assertions really ran at this commit rather than merely sitting in a diff: Desktop Shell (ubuntu-22.04) shows Detect desktop-shell changes → success and Run desktop release tests → success, not skipped, so node scripts/test-release.js evaluated every new pattern against the workflow as it now stands on main.
The new pins are the right replacements for what the allowlist used to guarantee by construction: discovery keyed on application/x-mach-binary, find "$runtime_dir" -type f -print0 so coverage is the whole staged runtime rather than a hand-picked subtree, the Node entitlement attached to the loop variable (so it cannot leak to the other branch), and codesign --verify --strict in the same step. The strongest of them is the --entitlements count over the step slice — it is a deepStrictEqual against a single occurrence, so adding the flag to the non-Node branch fails even though the Node branch is untouched, which is exactly the property the old "ripgrep must not inherit entitlements" assertion expressed while the two paths were separate blocks. The empty-runtime error replaces the retired ripgrep warning, and I confirmed that guard is reachable only when the array is empty, since the zero-count branch exits before the loops.
The paths addition is the more valuable half of this PR. A test that reads desktop-release.yml was outside every trigger that could change it, which is how a stale string comparison survived a merge and then cost six minutes per platform on release day; the filter now names the file it guards, and the cost is disclosed — a workflow-only edit pays for a Rust compile, with the release test still Linux-only.
No new Critical found. Two nits, neither a gate: the file mode flips 100755 → 100644, which is harmless because both call sites use node scripts/test-release.js, but it makes this script the odd one out beside prepare-runtime.js and smoke-packaged.js; and these assertions pin shell text, so the next edit to the signing step must touch this test too — a fair price for the only executable check that step has.
|
Released in v0.23.3. |
4 participants
What this PR does
Realigns the desktop release test with the macOS signing step it guards, and closes the gap that let them drift apart.
The test pins the shape of that step, and it still described the allowlist the step used to be: the ripgrep and Node section markers, the
find -exec … {} +form, and the Node binary as the signed path. Signing now discovers the Mach-O files under the staged runtime rather than naming them, so those assertions describe code that no longer exists. The new ones assert what the step does now while keeping every property the old list guaranteed by construction — discovery spans the whole runtime, only the Node.js branch passes entitlements, and the step verifies what it signed. A guard against an empty runtime replaces the ripgrep warning that was retired.The second change is why this reached a release at all. The job that runs this test fires only when the desktop package, the update-manifest script, or the main CI workflow change — but the file the test reads is the desktop release workflow, which is not in that list. A workflow-only edit therefore gets a green desktop job in seconds without running the test that guards it. The release workflow is now in the filter.
Why it's needed
Release 0.3.0-preview.0 failed on all four platforms in this test, after each runner had already built the CLI bundle and staged the runtime — roughly six minutes per platform spent to discover a stale string comparison. The release that preceded it merged with this job green, because the job never looked.
The pairing is the point: a test that reads a file outside its own package needs that file inside its trigger, or it becomes a test that only runs when it cannot fail.
Reviewer Test Plan
How to verify
Run the desktop release tests against the current workflow and confirm they pass, then confirm they still bite: change the signed path in the signing step, or drop the verification loop, and the corresponding assertion should fail. The entitlements assertion is the one worth poking hardest — it counts the flag across the whole step, so adding
--entitlementsto the non-Node branch must fail even though the Node branch is untouched.For the filter, open a PR that touches only the desktop release workflow and confirm the desktop job now runs the release tests instead of reporting done in under twenty seconds.
Evidence (Before & After)
N/A — CI and test-only change, no user-visible surface.
Tested on
The rewritten assertions were run against the workflow as it stands on main and all pass; the surrounding suite needs an installed package to run in full.
Environment (optional)
N/A
Risk & Scope
Linked Issues
Follows #11518.
中文说明
这个 PR 做了什么
让桌面发布测试与它所守护的 macOS 签名步骤重新对齐，并堵上导致两者脱节的缺口。
该测试固定了签名步骤的形态，而它描述的仍是那个步骤过去的白名单形式：ripgrep 与 Node 的分节标记、
find -exec … {} +的写法，以及把 Node 二进制作为被签名路径。现在签名改为枚举暂存运行时下的 Mach-O 文件而不是逐个点名，因此这些断言描述的是已经不存在的代码。新的断言针对该步骤当前的行为，同时保留旧清单原本靠结构保证的全部性质——枚举覆盖整个运行时、只有 Node.js 分支传入 entitlements、步骤会验证自己签过的东西。此外用"运行时中没有任何原生二进制"的硬失败，替代了已被移除的 ripgrep 警告。第二处改动才是它为什么会一路走到发布的原因。运行该测试的 job 只在桌面包、更新清单脚本或主 CI workflow 变更时触发——但测试读取的文件是桌面发布 workflow，而它不在这个列表里。因此一次只改 workflow 的提交会在几秒内拿到绿色的桌面 job，却根本没跑守护它的测试。现在发布 workflow 已加入过滤条件。
为什么需要
0.3.0-preview.0 发布在四个平台上全部失败于这个测试，而此时每个 runner 都已经构建完 CLI bundle 并暂存好运行时——每个平台约六分钟，只为发现一处过时的字符串比较。而它前一个 PR 是带着这个 job 的绿色合入的，因为这个 job 压根没看。
这两处改动是配套的：一个读取自身包之外文件的测试，必须把那个文件纳入自己的触发条件，否则它就变成一个"只在不可能失败时才运行"的测试。
评审验证计划
如何验证
针对当前 workflow 运行桌面发布测试，确认通过；然后确认它仍然有咬合力：修改签名步骤中被签名的路径，或删掉验证循环，对应断言应当失败。最值得多试的是 entitlements 那条——它统计整个步骤中该 flag 的出现次数，因此即使 Node 分支原封不动，只要给非 Node 分支加上
--entitlements也必须失败。过滤条件方面，提一个只改动桌面发布 workflow 的 PR，确认桌面 job 现在会真正运行发布测试，而不是二十秒内就报完成。
证据（Before & After）
N/A——仅 CI 与测试改动，无用户可见界面。
测试平台
重写后的断言已针对 main 上现有的 workflow 全部跑通；整套测试的其余部分需要已安装的依赖才能完整运行。
环境（可选）
N/A
风险与范围
关联 Issue
承接 #11518。