Skip to content

fix(ci): build before tests and protect desktop source workspace - #74

Closed
cangen185-sudo wants to merge 1 commit into
starsstreaming:mainfrom
cangen185-sudo:fix/ci-linux-workspace-install
Closed

cangen185-sudo wants to merge 1 commit into
starsstreaming:mainfrom
cangen185-sudo:fix/ci-linux-workspace-install

Conversation

@cangen185-sudo

@cangen185-sudo cangen185-sudo commented Oct 2, 2026 •

Copy link
Copy Markdown
Collaborator

问题与修复结果

将 CI 的完整 build 放在 typecheck/test 之前,并把可跨平台安装的 desktop 开发源码与 Windows 发布产物分开:源码 workspace 为 private,聚合 staging 为 public + os: ["win32"]。非 Windows 的 per-host install/uninstall 在调用宿主脚本前拒绝执行。

本 PR 已以当前 main 35730da179021eaaa1f6f9f3094bb5fefb536cce 为基线重新整理为一个提交:428144bd2086b150a6aabdcd873f86bf3c19c9f6。差异精简为 8 个文件、42 行新增与 7 行删除,解决原有分支冲突。

与维护者意见及最新上游的关系

  • 快捷方式问题采用产品修复方向:main 已经通过 fix(ci): restore workspace installs and isolate Windows process tests #94 在私有 ASCII 别名上完成 COM 读写,再发布原 Unicode 名称。本 PR 保留该实现和全部 Windows 快捷方式回归,不重新改名,也不按 locale 跳过测试。
  • fix(ci): restore workspace installs and isolate Windows process tests #94 已采用长路径 TMP/TEMP 运行器及 desktop 测试文件串行执行。本 PR 保留这些行为;npm test -w beauticode-desktop 继续通过运行器固定 Windows 长路径临时目录。
  • 当前 main 的 Windows/Node 22 CI 仍在旧 PID claim 的测试中因真实 PowerShell 探测超时、触发 fail-open 而失败。恢复确定的 execFile seam,并补充探测不可用时保留 live PID 的回归;注释说明 syncBuiltinESMExports() 用于同步被测源码的 named ESM import。
  • 按建议运行了一次 npm install --package-lock-only;未产生 lockfile 差异。没有重写根 typecheck 脚本。

实際验证

  • 两个新增 workspace/平台守卫检查先失败后通过;相关 contract、route、process-liveness 测试 11/11 通过。
  • 本地 Windows / Node.js 24.16.0:干净 npm ci、build、typecheck 通过;完整 npm test 404 通过、10 按既有条件跳过、0 失败。
  • desktop 包 62/62 通过,包含真实 Windows 快捷方式创建和限制 COM Unicode Save 的模拟回归;staging 仍为 public + Windows-only。
  • 使用 npm 官方 registry 的 npm audit --audit-level=high 报告 0 vulnerabilities;相关 JavaScript 语法及 git diff --check 通过。
  • 首次复用旧 checkout 时发现 5 个已无源文件的、未跟踪的旧 dist JS 产物被打包;已逐文件记录哈希并移到可恢复备份后完成最终全套。没有将这些本地产物或备份夹带进提交。
  • 新提交的远端 Windows/Ubuntu、Node 22/24 矩阵以最新 Actions 为准;本地结果不代替远端 CI。

范围

没有更新版本、发布 npm/EXE、安装到真实宿主或改变正在运行的客户端。Codex 辅助完成审查意见复核、修复、验证和 PR 更新。

Summary by CodeRabbit

  • 功能调整
    • 桌面端安装和卸载命令现仅支持 Windows;在 Linux 或 macOS 上执行时会报错。状态与健康检查等命令不受影响。
    • Linux 可安装的是私有源码工作区;可发布的桌面分发包仅面向 Windows。
  • 文档
    • README 已说明桌面分发包的生成方式及 Windows 平台限制。

@qodo-code-review

Copy link
Copy Markdown

ⓘ Qodo reviews are paused because your trial has ended. Ask your workspace admin to add credits to resume reviews. Manage billing

@coderabbitai

coderabbitai Bot commented Oct 2, 2026 •

Copy link
Copy Markdown

Review in Change Stack →

Note

Currently processing new changes in this PR. This may take a few minutes, please wait...

⚙️ Run configuration
  • Configuration used: Organization UI
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: 2f46351a-86d9-4c34-988b-d7b49e322996
📥 Commits

Reviewing files that changed from the base of the PR and between 6146622 and 428144b.

📒 Files selected for processing: 8
  • .github/workflows/ci.yml
  • packages/beauticode-desktop/README.md
  • packages/beauticode-desktop/package.json
  • packages/beauticode-desktop/src/index.mjs
  • packages/beauticode-desktop/test/contract.test.mjs
  • packages/beauticode-desktop/test/route.test.mjs
  • packages/core/test/process-liveness.test.js
  • scripts/pack-desktop-aggregate.mjs
 ________________________________________________
< Name's Rabbit. CodeRabbit. Licensed to review. >
 ------------------------------------------------
  \
   \   (\__/)
       (•ㅅ•)
       /   づ

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Advanced

Run ID: 560b4bf6-0cb3-4d4c-b8ed-f425cecced8e

📥 Commits

Reviewing files that changed from the base of the PR and between 899c2d0 and 6146622.

⛔ Files ignored due to path filters (1)
  • package-lock.json is excluded by !**/package-lock.json
📒 Files selected for processing (13)
  • .github/workflows/ci.yml
  • packages/adapter-codex/test/codex-lifecycle.test.js
  • packages/adapter-workbuddy/test/wb-runner-log.test.js
  • packages/beauticode-desktop/README.md
  • packages/beauticode-desktop/package.json
  • packages/beauticode-desktop/src/index.mjs
  • packages/beauticode-desktop/src/quick-shortcuts.mjs
  • packages/beauticode-desktop/test/all.test.mjs
  • packages/beauticode-desktop/test/contract.test.mjs
  • packages/beauticode-desktop/test/pack.test.mjs
  • packages/beauticode-desktop/test/route.test.mjs
  • packages/core/test/process-liveness.test.js
  • scripts/pack-desktop-aggregate.mjs

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

📝 Walkthrough

本次变更调整桌面工作区与发布包的平台限制,更新宿主命令和快捷方式脚本行为,并修改 CI 执行顺序及多项测试。

Changes: 桌面工作区与发布包

Layer / File(s) Summary
工作区配置与宿主命令
packages/beauticode-desktop/package.json, packages/beauticode-desktop/README.md, packages/beauticode-desktop/src/index.mjs, packages/beauticode-desktop/test/all.test.mjs, packages/beauticode-desktop/test/contract.test.mjs, packages/beauticode-desktop/test/route.test.mjs
工作区清单标记为私有并移除操作系统限制。非 Windows 平台执行宿主安装或卸载命令时会报错。README 和测试更新了相关说明与断言;测试路径比较统一处理路径分隔符。
快捷方式脚本错误处理
packages/beauticode-desktop/src/quick-shortcuts.mjs
PowerShell 脚本静默显示进度;发生异常时将异常消息写入标准错误,并以状态码 1 退出。
Windows 发布包清单
scripts/pack-desktop-aggregate.mjs, packages/beauticode-desktop/test/pack.test.mjs
打包流程将生成的清单设置为非私有且仅面向 Windows。打包测试验证 os 字段为 ["win32"]。

CI 执行顺序

Layer / File(s) Summary
调整 CI 步骤顺序
.github/workflows/ci.yml
CI 工作流现在先运行构建,再运行类型检查和测试。

适配器测试路径

Layer / File(s) Summary
调整适配器测试路径
packages/adapter-codex/test/codex-lifecycle.test.js, packages/adapter-workbuddy/test/wb-runner-log.test.js
Codex 测试使用系统临时目录构造路径。Workbuddy 失败场景测试改用包含实际 NUL 字符的路径。

进程存活测试

Layer / File(s) Summary
模拟进程启动时间探测
packages/core/test/process-liveness.test.js
测试模拟平台启动时间探测,验证早于启动时间的声明无法识别当前 PID;另覆盖探测失败时当前 PID 仍被视为存活。

Priority: ➖ Normal

Estimated code review effort: 3 (Moderate) | ~20 minutes

Change: Bug fix

Suggested reviewers: starsstreaming

Merge Risk: 🟡 Moderate · up to 61466

The latest Windows run failed an enabled shortcut-save test, so the full matrix has not passed. Diagnose the failure or obtain a passing Windows run before relying on this validation.

Security architecture risk: 🔵 Low · up to 61466

The inspected changes preserve Windows-only host installation and release packaging while allowing cross-platform development. No introduced security concern was established, but incomplete coverage prevents a minimal-risk assessment.

Retained concerns
No architecture-level concerns identified.

Security review details

Security Blast Radius

  • inferred — The demonstrated expansion is cross-platform development installation, not automatic host modification. The desktop package retains a postinstall hook, but that hook only prints instructions; host mutation requires an explicit command and remains Windows-gated in the inspected CLI paths.

Trust Boundaries and Controls

  • observed — The new platform check is a compatibility control, not an authorization or sandbox boundary: library callers can supply a platform override. Normal command dispatch defaults to process.platform, and staging separately restores npm's Windows eligibility metadata. No new privilege elevation is present in the inspected command-dispatch change.

Resilience and Maintainability Implications

  • inferred — The added guard creates no intermediate mutable state: unsupported commands terminate before child execution, while Windows commands retain existing behavior. Installer identity, cleanup, recovery, and concurrency guarantees remain delegated to the unchanged child implementations; their completeness is not established by this review.
🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 20.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 5 functions across 10 files. (3 skipped: … Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
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.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed 标题准确概括了主要变更:将跨平台开发工作区与 Windows 发布包分离。标题简洁、具体,并与变更内容一致。
Full details: Docstring Coverage

Explanation

Docstring coverage is 20.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 5 functions across 10 files. (3 skipped: 3 unsupported.)

  • Fix all pre-merge checks with AI
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create a new PR
  • Autopilot · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Autopilot is currently an internal CodeRabbit preview.

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.

@starsstreaming

Copy link
Copy Markdown
Owner

Review — this is the highest-priority fix in the repo right now

Reviewed locally against main @ e04637f (after #75/#76/#77 landed), with an actual npm ci / build / typecheck / test run on the branch, not just a diff read.

Verdict: very worth merging, and it should go first — but it cannot go green as-is.

I verified the two problems this fixes are real and are the only reason main's CI is red:

platform failing step on main root cause
ubuntu npm ci packages/beauticode-desktop/package.json declares os: ["win32"], cached into package-lock.json → EBADPLATFORM
windows npm test CI runs test before build; packages/beauticode-desktop/test/* needs built workspace deps

main (run 37013812476, sha 7c2726e9) still fails on exactly those two steps. Your PR fixes both. Ubuntu 22+24 already go fully green on it, and it merges cleanly — no conflicts with main or with #75/#76/#77.


Before merge

1. packages/beauticode-desktop/test/quick-shortcuts.test.mjs:7 — the Windows job is still red

This is the one thing blocking the goal of "merge to get CI green again".

not ok 47 - quick shortcut survives an npm package move by pointing at the stable runtime
  location: 'packages\\beauticode-desktop\\test\\quick-shortcuts.test.mjs:7:1'
  error: 'Unable to save shortcut "C:\\Users\\runneradmin\\AppData\\Local\\Temp\\'
       + 'beauticode-quick-shortcut-Bu7p50\\Programs\\beautiCode Cursor (??).lnk".'

Root cause — not your code, and not the test either:

packages/beauticode-desktop/src/quick-shortcuts.mjs:29 hardcodes a non-ASCII suffix:

const shortcutPath = path.join(programsDir, `beautiCode ${NAMES[host]} (背景).lnk`);

That name goes to WScript.Shell's CreateShortcut/Save over OLE automation, which marshals strings through the process ANSI code page. On a non-CJK Windows (the windows-latest runner), 背景 degrades to ?? and Save() fails outright.

Two facts worth noting:

  • The line came from main commit 6c50bab, not from this PR. Your PR only added try { … } catch { [Console]::Error.WriteLine(…); exit 1 }.
  • Your change converted a false pass into a real failure — previously Save() threw, the outer code printed created anyway, and the test passed. That is strictly better behaviour, but it means CI must decide about this test now.

I could not reproduce it here: this machine is ACP=936 / zh-CN, and the same Chinese filename saves with exit 0 and intact UTF-8 bytes. So it is environment-specific and cannot be validated from a CJK dev box.

Please pick one and say which in the PR:

  • (a) Fix the product. Either drop (背景) from the filename, or make it ASCII in filename position and keep the display name Unicode. Note the shipped feature is genuinely broken on en-US Windows today, so this is a real bug, not just a test problem.
  • (b) Gate the test on the code page / locale and file the product bug separately. Acceptable, but then CI stays partly blind to this feature on non-CJK runners.

My recommendation is (a) — the shortcut name is cosmetic, and the bug currently affects a large share of users.

2. packages/beauticode-desktop/test/pack.test.mjs + package.json:"test" — confirm the harness swap is intentional

You changed the package's test script to:

"test": "node ../../scripts/test-runner.mjs --test test/*.test.mjs"

That is the right call — scripts/test-runner.mjs pins TMP/TEMP to the long-form path to avoid Windows 8.3 short-name flakiness, and it matches how other packages are wired. Two asks:

  • Confirm the long-path pin is what actually makes this package's tests deterministic (it is what the pack.test.mjs ENOENT failure suggests), and mention it in the PR description.
  • If scripts/test-runner.mjs was not previously used by beauticode-desktop, note the behaviour change for anyone running npm test -w beauticode-desktop locally.

3. Minor / non-blocking

  • package-lock.json consistency. I could not reproduce this on the merge result here (hasInstallScript is absent from that lock entry on both main and the branch), and your ubuntu jobs go green, so the lock is consistent as far as npm ci is concerned. Please just re-run npm install --package-lock-only once before merging and commit any resulting diff, so the lock does not drift on the first install after merge.
  • Branch is stale. The branch is 11 commits behind main (and now also behind fix(installer): preserve unowned DSH plugin directories #75/fix(doubao): honor verified custom installation paths #76/fix(desktop-cdp): cap JSON responses while streaming #77). It still merges cleanly — I checked the one risky overlap, the artifacts/…/bg-canvas*.png deletions from main, and the merge resolution correctly keeps them deleted and pack.test.mjs's assert.rejects(fs.access(...)) still passes. A rebase before merge is still advisable so CI runs against the final base.
  • packages/core/test/process-liveness.test.js now mocks child_process.execFile and calls syncBuiltinESMExports(). Legitimate and it replaces a genuinely flaky uptime-based test, but it reaches into Node internals — a short comment on why the seam is needed would help future readers.
  • packages/beauticode-desktop/src/index.mjs platform guard in runHostCommand is correctly placed before resolveRoute/spawnSync and honours an injected platform. Verified the one real caller (bin/beauticode-desktop.mjs:19) goes through defaultOptions(), which defaults platform to process.platform, so real Windows users are unaffected.

What I verified locally (branch 6146622, 4 commits, 14 files)

npm ci           ✅
npm run build    ✅
npm run typecheck ✅  exit 0
npm test         ✅  63 + 80 + 108 + 45 + 24 + 4 + 2 + 57 pass, 0 fail
                     (one pre-existing # SKIP in adapter-codex: the Windows Task Scheduler case)

CI step-level results for run 37007224772 agree: ubuntu 22+24 fully green, windows 22+24 now clear build and typecheck and fail only at npm test.

I also ran the full merged tree (this branch + #75/#76/#77) end-to-end: green, 0 failures.

Also checked, so you don't have to: your branch does not duplicate anything in #75/#76/#77 (disjoint files), and all four merge together with zero conflicts and a green suite.


Correction to an earlier assumption (worth stating so nobody re-does it)

The root typecheck script is not broken on current main. I deleted packages/adapter-desktop-cdp/dist to simulate a fresh clone and ran the root npm run typecheck → exit 0. main's package.json already builds adapter-desktop-cdp before typechecking adapter-cursor (landed in 1a0c994).

So the value of this PR is the CI step reordering (npm run build before typecheck/test) plus the workspace/release split — not repairing the typecheck script. If the PR description or commit message claims the latter, it is worth correcting.


Once item 1 is resolved I'm happy to merge this immediately — it is the unblocker for the whole matrix.

@cangen185-sudo
cangen185-sudo force-pushed the fix/ci-linux-workspace-install branch from 6146622 to 428144b Compare October 14, 2026 11:27
@cangen185-sudo cangen185-sudo changed the title fix(ci): separate cross-platform workspace from Windows release fix(ci): build before tests and protect desktop source workspace Oct 14, 2026
@cangen185-sudo

Copy link
Copy Markdown
Collaborator Author

@starsstreaming 已按你的意见重新核对当前 main,并将 #74 整理到 35730da 之上;新 head 为 428144bd2086b150a6aabdcd873f86bf3c19c9f6,保留为一个提交,当前差异只有 8 个文件。

  1. 快捷方式采用产品修复方向。fix(ci): restore workspace installs and isolate Windows process tests #94 已实现 ASCII 临时别名读写 COM、保留原 Unicode 名称,我直接保留最新上游实现和所有 Windows 回归,没有再改名或跳过测试。本地真实快捷方式创建及“COM 只接受 ASCII”模拟均通过。
  2. 长路径 test-runner 与串行 desktop 测试已经由 fix(ci): restore workspace installs and isolate Windows process tests #94 引入,本次保留;已在 PR 描述说明 npm test -w beauticode-desktop 的实际行为。
  3. npm install --package-lock-only 已运行,lockfile 无差异;没有修改根 typecheck 实现。保留 CI 的 build-before-typecheck/test 顺序、private source/public Windows staging 契约和 per-host 平台守卫。
  4. 当前 main 的 Windows/Node 22 CI 仍因 PID 探测超时 fail-open 导致旧 claim 测试失败。本次保留确定的 execFile mock,新增 probe unavailable 回归,并补上 syncBuiltinESMExports() 同步 named ESM import 的注释。

本地 Windows/Node 24.16 完成干净 npm ci、build、typecheck 和完整 npm test:404 passed、10 skipped、0 failed;desktop 包 62/62。npm 官方 registry 的 audit 为 0 vulnerabilities,语法及 diff 检查通过。旧 checkout 的 5 个无源 dist 产物已按文件哈希移入可恢复备份,随后全套通过。

新提交已经推送到我的 fork,最新跨平台 CI 尚待完成。PR 描述已按最终差异重写,可再复核;未执行合并、发布或本机宿主安装。Codex 辅助完成本次更新与验证。

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