test(desktop): align interceptor packaging contract - #5531
Conversation
|
@qwen-code /triage |
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Hi @tt-a1i, thanks for the fix — the underlying change (removing stale build-script assertions from the packaging contract test) looks correct and I agree with the direction.
However, the PR body doesn't follow our PR template. It's missing the required sections:
- What this PR does / Why it's needed — currently "Summary" covers part of this, but the "why" (the test was red on main because the scripts delegate to electron-builder and never reference the file directly) isn't explained.
- Reviewer Test Plan — how should a reviewer verify this? (You listed validation commands, which is helpful — just put them under the template headings so reviewers know where to look.)
- Risk & Scope — even for a small test fix, a one-liner like "risk: none, test-only change" helps reviewers move fast.
- Tested on table — which OS did you run the test on?
- 中文说明 — bilingual body is required for our review process.
Could you update the PR description to match the template? It's a quick reformat — the content is already there, it just needs the right headings. Happy to re-review once that's done. 🙏
中文说明
嗨 @tt-a1i,感谢修复——底层改动(从打包契约测试中移除过时的构建脚本断言)方向正确。
但 PR 描述没有按照我们的 PR 模板 填写,缺少以下必填章节:
- What this PR does / Why it's needed — 目前的 "Summary" 覆盖了部分内容,但没有解释"为什么"(测试在 main 上标红,因为脚本委托给 electron-builder,本身不直接引用该文件)。
- Reviewer Test Plan — 审查者如何验证?(你列出了验证命令,很有帮助——只需放到模板标题下即可。)
- Risk & Scope — 即使是小改动,写一句"风险:无,仅测试改动"也能加速审查。
- Tested on 表格 — 你在哪个操作系统上跑的测试?
- 中文说明 — 流程要求 PR 正文包含中文翻译。
请按模板重新格式化一下描述就好,内容都在,只是需要换个标题。改完我会重新审查。🙏
— Qwen Code · qwen3.7-max
|
@qwen-code /triage |
|
Thanks for the PR, @tt-a1i! Template looks good ✓ On direction: this is a straightforward test-only fix — the contract test was red on main because the build scripts no longer directly reference the interceptor file (packaging is delegated to electron-builder). No product direction concerns here. On approach: the scope is minimal and focused. You removed the four stale assertions and the now-unnecessary file reads, kept the one assertion that actually matters (electron-builder.yml includes the interceptor path), and updated the test description to match. No scope creep, no drive-by changes. Clean. Moving on to code review. 🔍 中文说明感谢贡献,@tt-a1i! 模板完整 ✓ 方向:纯测试修复——main 上的 contract 测试失败是因为构建脚本不再直接引用 interceptor 文件(打包已委托给 electron-builder),没有产品方向问题。 方案:范围最小且聚焦。移除了四个过时断言和不再需要的文件读取,保留了真正重要的断言(electron-builder.yml 包含 interceptor 路径),并更新了测试描述以匹配。没有范围蔓延,没有夹带改动。干净利落。 进入代码审查 🔍 — Qwen Code · qwen3.7-max |
|
Code review: The diff is clean — removes three stale Testing: Ran the contract test before and after the fix. Before (installed build / main)After (this PR)All clear. ✅ 中文说明代码审查: diff 干净——移除了三个过时的 测试: 在修复前后分别运行了 contract 测试。修复前测试失败(1 fail),修复后通过(1 pass)。 全部通过 ✅ — Qwen Code · qwen3.7-max |
|
This is exactly the kind of PR you want to see: a real problem (test red on main), a correct diagnosis (build scripts delegated packaging to electron-builder, so the old per-script assertions became stale), and the minimum change to fix it. The test now enforces the contract that actually matters — the electron-builder manifest includes the interceptor source path — without coupling to implementation details of how each platform script invokes the builder. The before/after confirms it works. No reservations. 中文说明这是一个标准的优质 PR:解决了真实问题(main 上测试失败),正确诊断了原因(构建脚本将打包委托给 electron-builder,旧的逐脚本断言已过时),并以最小改动修复。测试现在保护真正重要的契约——electron-builder manifest 包含 interceptor 源路径——而不是耦合到各平台脚本调用构建器的实现细节。 前后对比确认修复有效。没有顾虑。 — Qwen Code · qwen3.7-max |
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
LGTM, looks ready to ship. ✅
✅ Local build verification — safe to merge (correctness-wise)Maintainer verification of #5531 with a real local build/test run (Bun Verdict: the change is correct and introduces zero new failures. It fixes a genuinely red test by aligning the packaging contract with how the app is actually packaged. (Merge is currently 1. Target test: RED → GREEN
2. No collateral damage — broader
|
| State | pass | fail | errors |
|---|---|---|---|
main |
806 | 90 | 48 |
| #5531 | 807 | 89 | 48 |
Delta is exactly +1 pass / −1 fail with identical 48 errors — i.e. only the target test flipped. The remaining 89 failures (i18n locale parity, safe-mode permission, agent backend errors) are pre-existing and unrelated, matching the PR description.
3. Why removing the script assertions is the right fix (not hiding a gap)
All three platform build scripts package exclusively through electron-builder, which reads electron-builder.yml:
apps/electron/scripts/build-dmg.sh:165→npx electron-builder $BUILDER_ARGSapps/electron/scripts/build-linux.sh:133→npx electron-builder --linux --${ARCH}apps/electron/scripts/build-win.ps1:346→npx electron-builder --win --x64
The packaged file is declared in the manifest — apps/electron/electron-builder.yml:26:
- packages/shared/src/interceptor-request-utils.tsgit log -S "interceptor-request-utils.ts" -- apps/electron/scripts/ returns nothing: the build scripts have never referenced this filename. The three removed assertions were invalid from the day the test was added (#3778) — they asserted an implementation detail that never existed. The kept manifest-level assertion is the meaningful guard (delete line 26 and the test correctly goes red). Fixing the scripts to hard-code the filename would be architecturally wrong — that is electron-builder's job, not the scripts'.
4. Notes on the PR's other claims (non-blocking)
tscfails locally only withTS2688: Cannot find type definition file for 'bun'— that is a missingbun-typesdependency in my checkout (tsconfig.json:18sets"types": ["bun"]); the same error occurs onmain. Environmental, not caused by this PR.prettier --check:packages/desktop/is listed in the root.prettierignore:25, so the repo's format gate does not check this file (consistent with the author's "passes"). A direct prettier run does flag the keptbuilderYmlline as >80 cols, but that line is unchanged by this PR (pre-existing) — the change is format-neutral. Optional nit only.
Recommendation: approve from a correctness standpoint. 🟢
🇨🇳 中文版(点击展开)
✅ 本地构建验证 —— 就正确性而言可以合并
维护者对 #5531 做了真实本地构建/测试验证(Bun 1.3.14,macOS arm64),作为合并参考。
结论: 改动正确,且不引入任何新失败。它通过让打包契约与真实打包方式对齐,修复了一个确实变红的测试。(当前 merge 状态为 BLOCKED,原因是 review/CI 门禁,与本改动无关。)
1. 目标测试:红 → 绿
| 状态 | 结果 |
|---|---|
main(改动前) |
❌ 失败 —— 第 19 行 expect(dmgScript).toContain('interceptor-request-utils.ts') |
| #5531(改动后) | ✅ 1 pass / 0 fail |
2. 无附带影响 —— 更广的 shared 套件对比
改动前后各跑了一遍整个 packages/desktop/packages/shared/src 套件:
| 状态 | pass | fail | errors |
|---|---|---|---|
main |
806 | 90 | 48 |
| #5531 | 807 | 89 | 48 |
差异精确为 +1 pass / −1 fail,48 个 errors 完全相同 —— 即只有目标测试发生翻转。其余 89 个失败(i18n locale parity、safe-mode permission、agent backend errors)均为预先存在且无关,与 PR 描述一致。
3. 为什么删除脚本断言是正确的修法(而非掩盖缺口)
三个平台构建脚本全部只通过 electron-builder 打包,而 electron-builder 读取 electron-builder.yml:
apps/electron/scripts/build-dmg.sh:165→npx electron-builder $BUILDER_ARGSapps/electron/scripts/build-linux.sh:133→npx electron-builder --linux --${ARCH}apps/electron/scripts/build-win.ps1:346→npx electron-builder --win --x64
被打包的文件确实已在 manifest 中声明 —— apps/electron/electron-builder.yml:26:
- packages/shared/src/interceptor-request-utils.tsgit log -S "interceptor-request-utils.ts" -- apps/electron/scripts/ 无任何结果:构建脚本从未引用过这个文件名。那三条被删的断言从测试引入之日(#3778)起就是错的 —— 它们断言了一个根本不存在的实现细节。保留下来的 manifest 层断言才是有意义的守卫(删掉第 26 行,测试会正确变红)。反过来把文件名硬编码进脚本才是架构错误 —— 那是 electron-builder 的职责,不是脚本的。
4. 关于 PR 其它声明的说明(不阻塞)
tsc在本地仅因TS2688: Cannot find type definition file for 'bun'失败 —— 这是我本地 checkout 缺bun-types依赖所致(tsconfig.json:18设了"types": ["bun"]);main上报同样的错。属环境问题,非本 PR 引入。prettier --check:packages/desktop/在根.prettierignore:25中,仓库格式门禁并不检查此文件(与作者所说"passes"一致)。直接跑 prettier 确实会把保留的builderYml行标为超过 80 列,但该行未被本 PR 改动(预先存在)—— 改动对格式中性。仅为可选小建议。
建议: 就正确性而言予以批准。🟢
🤖 Verified locally by the maintainer via real Bun build/test (before/after diff). Generated with Claude Code.
Fixes #5530
What this PR does
This keeps the interceptor packaging contract test focused on the electron-builder manifest and removes stale assertions that required platform build scripts to mention the packaged source filename directly.
Why it's needed
The test was red on main because the platform build scripts now delegate packaging through electron-builder and do not need to duplicate the interceptor file name themselves.
The contract that matters is whether the app package includes the interceptor source path, so the test should assert that manifest-level behavior instead of the old script implementation detail.
Reviewer Test Plan
How to verify
Run the targeted packaging contract test and confirm it passes.
Evidence (Before & After)
Before: the contract test failed by expecting stale direct references in platform build scripts.
After:
bun test packages/desktop/packages/shared/src/__tests__/interceptor-packaging-contract.test.tspasses.Additional checks run:
npx tsc --noEmit -p packages/desktop/packages/shared/tsconfig.jsonnpx prettier --check packages/desktop/packages/shared/src/__tests__/interceptor-packaging-contract.test.tsgit diff --checkI also ran
bun test packages/desktop/packages/shared/src; this contract test now passes, while the run still has existing unrelated failures in safe-mode permission tests and i18n locale parity.Tested on
Environment (optional)
Local macOS checkout with Bun and Node test commands.
Risk & Scope
Linked Issues
Fixes #5530
AI Assistance Disclosure
I used Codex to review the changes, sanity-check the implementation against existing patterns, and help spot potential edge cases.
中文说明
What this PR does
这个 PR 让 interceptor packaging contract 测试只关注 electron-builder manifest,并移除要求平台构建脚本直接写出 packaged source 文件名的过时断言。
Why it's needed
main 上这个测试失败,是因为平台构建脚本现在通过 electron-builder 做打包,不需要自己重复引用 interceptor 文件名。
真正需要保护的契约是 app package 是否包含 interceptor source path,所以测试应该断言 manifest 层面的行为,而不是旧脚本里的实现细节。
Reviewer Test Plan
How to verify
运行定向 packaging contract 测试,确认通过。
Evidence (Before & After)
Before:contract 测试因为期待平台构建脚本里的过时直接引用而失败。
After:
bun test packages/desktop/packages/shared/src/__tests__/interceptor-packaging-contract.test.ts通过。另外还运行了上面列出的 typecheck、prettier 和
git diff --check。Tested on
Environment (optional)
本地 macOS checkout,使用 Bun 和 Node 测试命令。
Risk & Scope