fix: 统一 Node 版本文档为 >=24 <26 + Windows 安装器 pnpm 进度 spinner#1022
Conversation
- README (en/ja/zh), SETUP (en/zh): Node.js 20+ → 24+, >=20.0.0 → >=24.0.0 - F113 doc: fnm/Node.js 20 → 24, brew install node@20 → node@24 - desktop/service-manager.js: error message >=20 → >=24 - desktop/scripts/build-mac.sh: fallback version v22.12.0 → v24.16.0 - cli-spawn.ts comment: v20 → v24 - scripts/install.ps1: replace Tee-Object with live spinner for pnpm install progress, re-emit captured output on failure so real errors are visible to the user Co-Authored-By: Claude <noreply@anthropic.com>
|
Maintainer triage:
Directionally this PR is welcome, but merge is not cleared yet because The failing tests are all in
Because this PR changes
Once the public tests pass, we can continue code review. No merge clearance yet. [砚砚/gpt-5.5🐾] |
…ch-Object spinner
The pnpm install pipeline changed from Tee-Object to ForEach-Object
for live spinner progress display. Update test assertions to accept
either pipeline type while preserving the core invariants:
- pnpm must be invoked directly (not via Invoke-Pnpm wrapper)
- $global:LASTEXITCODE must be read/written explicitly
- ErrorActionPreference must be downgraded around pnpm capture
Also fix catch-block extraction: the function now has nested bare
catch {} blocks for spinner operations which broke greedy regex
matching. Use simpler positional checks instead.
Co-Authored-By: Claude <noreply@anthropic.com>
zts212653
left a comment
There was a problem hiding this comment.
Thanks for getting CI green. I re-reviewed the current HEAD e4044be3.
Requesting changes for one blocking test-guard regression, not for the spinner direction itself.
Blocking finding:
packages/api/test/install-script-error-classification.test.js no longer proves that the real Invoke-PnpmInstallWithCapturedOutput catch path preserves the $global:LASTEXITCODE -eq 0 success escape hatch.
The new tests use body.indexOf('} catch {') and then search from that point onward. In the new implementation, the first } catch { is the small spinner capability probe (try { [void][Console]::CursorVisible ... } catch {}), not the pnpm pipeline catch block. From that point, afterCatch includes the normal success return (Ok = $global:LASTEXITCODE -eq 0) before the real pnpm catch block. That means a future edit could remove the $global:LASTEXITCODE -eq 0 check from the real pipeline catch path and these tests would still pass.
This is exactly the invariant that previously blocked this PR and protects the Windows / Node 24 pnpm exit-code plumbing related to clowder-ai#987, so it needs to stay precise.
Please tighten the tests so they anchor to the actual multi-line pnpm pipeline catch block, for example by slicing from the multi-line catch index you already detect, or by extracting the catch block around the # Two distinct scenarios reach this catch: comment / following finally. Also fix the extra duplicated regex argument in the second assert.match; the intended message string is currently ignored.
After that, I can re-review. The production direction still looks reasonable: direct $pnpmCommand @CommandArgs 2>&1 | ForEach-Object is preserved, $ErrorActionPreference remains scoped around capture, and the new failure re-emit behavior is aligned with the issue.
[砚砚/gpt-5.5🐾]
Reviewer feedback: the previous test used body.indexOf('} catch {')
which matched the spinner capability probe (bare catch {}) instead of
the actual pnpm pipeline catch block. A future edit could remove
$global:LASTEXITCODE from the real catch and the tests would still
pass — invariant regression.
Fix: anchor both DEP0169 tolerance tests to the "Two distinct
scenarios reach this catch:" comment, then extract the catch block
between that and the following '} finally {'. This precisely targets
the pnpm pipeline catch path.
Also fix duplicated regex argument in the second assert.match call
(the intended message string was being ignored).
Verified: removing $global:LASTEXITCODE -eq 0 from the catch block
causes both tests to fail as expected.
Co-Authored-By: Claude <noreply@anthropic.com>
zts212653
left a comment
There was a problem hiding this comment.
Re-reviewed current HEAD c28e9b9.
The previous blocking point is fixed: the tests now anchor to the real pnpm pipeline catch block via the Two distinct scenarios marker and slice to the following finally, so they no longer accidentally pass by scanning from the spinner capability probe catch or the normal success path. The duplicated assert.match argument is also gone.
I also rechecked the production path against the #987/#1014 exit-code invariants:
Invoke-PnpmInstallWithCapturedOutputstill resolves pnpm up front and calls& $pnpmCommand @CommandArgs 2>&1 | ForEach-Objectdirectly.$global:LASTEXITCODEis explicitly initialized before the native call and read on both success and catch paths.$ErrorActionPreferenceis downgraded only around captured pnpm execution and restored infinally.- Failure output is captured and re-emitted, which matches the visibility goal from
clowder-ai#1021.
All GitHub checks are green at this HEAD. Approving review. Merge/intake decision is still maintainer-owned because this touches installer/runtime plumbing.
[砚砚/gpt-5.5🐾]
…1022) * fix: update Node 20 → 24 in docs/runtime + add install spinner - README (en/ja/zh), SETUP (en/zh): Node.js 20+ → 24+, >=20.0.0 → >=24.0.0 - F113 doc: fnm/Node.js 20 → 24, brew install node@20 → node@24 - desktop/service-manager.js: error message >=20 → >=24 - desktop/scripts/build-mac.sh: fallback version v22.12.0 → v24.16.0 - cli-spawn.ts comment: v20 → v24 - scripts/install.ps1: replace Tee-Object with live spinner for pnpm install progress, re-emit captured output on failure so real errors are visible to the user Co-Authored-By: Claude <noreply@anthropic.com> * fix(test): update install-script-error-classification tests for ForEach-Object spinner The pnpm install pipeline changed from Tee-Object to ForEach-Object for live spinner progress display. Update test assertions to accept either pipeline type while preserving the core invariants: - pnpm must be invoked directly (not via Invoke-Pnpm wrapper) - $global:LASTEXITCODE must be read/written explicitly - ErrorActionPreference must be downgraded around pnpm capture Also fix catch-block extraction: the function now has nested bare catch {} blocks for spinner operations which broke greedy regex matching. Use simpler positional checks instead. Co-Authored-By: Claude <noreply@anthropic.com> * fix(test): anchor catch-block invariants to DEP0169 comment marker Reviewer feedback: the previous test used body.indexOf('} catch {') which matched the spinner capability probe (bare catch {}) instead of the actual pnpm pipeline catch block. A future edit could remove $global:LASTEXITCODE from the real catch and the tests would still pass — invariant regression. Fix: anchor both DEP0169 tolerance tests to the "Two distinct scenarios reach this catch:" comment, then extract the catch block between that and the following '} finally {'. This precisely targets the pnpm pipeline catch path. Also fix duplicated regex argument in the second assert.match call (the intended message string was being ignored). Verified: removing $global:LASTEXITCODE -eq 0 from the catch block causes both tests to fail as expected. Co-Authored-By: Claude <noreply@anthropic.com> --------- Co-authored-by: Claude <noreply@anthropic.com> Co-authored-by: Lysander Su <773678591@qq.com>
关联 Issue
Closes #1021
变更摘要
1. Node 版本文档统一(9 处)
项目代码和安装器已要求 Node >=24 <26,但文档和部分运行时代码仍写 Node 20,导致新用户按文档安装后版本不匹配。
README.mdREADME.ja-JP.mdREADME.zh-CN.mdSETUP.mdSETUP.zh-CN.mddocs/features/F113-multi-platform-one-click-deploy.mdnode@20→ 24 /node@24desktop/service-manager.jsdesktop/scripts/build-mac.shpackages/api/src/utils/cli-spawn.ts2. Windows 安装器 pnpm 进度优化
scripts/install.ps1的Invoke-PnpmInstallWithCapturedOutput函数改造:Tee-Object,进度行(packages/xxx、.../xxx)原地刷新显示旋转动画 + 当前包名,非进度行(错误/警告)正常打印到新行capturedOutput完整输出,避免 "The real error is above" 但上方无内容ForEach-Object闭包中值类型变量不可变的问题测试
install.ps1,验证 spinner 实时显示和错误输出可见🤖 Generated with Claude Code