fix: report native binary launch and crash failures - #2116
Conversation
…dows Canonicalise the sandbox temp root with fs.realpathSync so the launch-failure assertions survive Windows 8.3 short-name expansion (RUNNER~1), which the new shim-test-windows CI job would otherwise fail on. Give the cleanup hook a retry budget to tolerate a transient Windows file lock on the ~80MB fixture binary. Add a second, pure-ASCII line to the launch-failure diagnostic pointing users at what to include when reporting. Guard the tracker line, the new line, and empty stdout in assertLaunchFailure, cover child stderr passthrough in the success case, and extend ci-workflow.test.sh's full_job loop to keep shim-test-windows from being silently skipped on PR edits.
Extend argv-containment guard to launch-failure cases in run.test.js by passing the sentinel token to all three failure scenarios and adding an assertion that catches future regressions where spawnargs or the error object itself would leak to stderr. Extend secrets assertion in ci-workflow.test.sh to the newly added shim-test-windows job, preventing accidental credential references in the Windows-specific npm shim test job.
…bytes libuv's execvp falls back to /bin/sh on ENOEXEC, so a chmod-755 zero-byte file is not a launch failure on Linux: /bin/sh runs it as an empty script and exits 0, letting the old "zero-byte binary" case pass locally on macOS (real ENOEXEC) while silently masking the same case in CI. Use a directory at the bin path on POSIX instead, which reliably reports EACCES on both macOS and Linux, including as root. win32 keeps its zero-byte .exe fixture, where CreateProcess genuinely fails to launch it. Also skip the existing chmod-000 EACCES case under root, since mode 0000 does not deny exec for uid 0.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe shim runner now handles native launch failures, signals, and Windows NTSTATUS crashes with bounded diagnostics. A cross-platform test suite covers these cases. Local tests and Windows CI run the suite and include its status in workflow results. ChangesShim diagnostics and CI coverage
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant CI
participant ShimTests
participant ShimRunner
participant NativeBinary
CI->>ShimTests: Run scripts/run.test.js
ShimTests->>ShimRunner: Invoke shim with fixture and sentinel arguments
ShimRunner->>NativeBinary: Execute native binary
NativeBinary-->>ShimRunner: Return status, signal, or launch error
ShimRunner-->>ShimTests: Emit bounded diagnostics and exit status
ShimTests-->>CI: Report test result
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 Biome (2.5.5)scripts/run.jsFile contains syntax errors that prevent linting: Line 102: Illegal return statement outside of a function; Line 120: Illegal return statement outside of a function 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. Comment |
github-actions
Bot
added
the
size/L
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
.github/workflows/ci.yml (1)
146-157: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winAdd
timeout-minutesto the new blocking job.This job is now in the
resultsFAILED loop, and it spawns child processes that can hang on Windows (lockednode.execopy, endpoint-protection prompts). Without a job timeout it falls back to the 360-minute default while blocking the merge gate.♻️ Proposed tweak
shim-test-windows: needs: fast-gate runs-on: windows-latest + timeout-minutes: 15 steps:🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/ci.yml around lines 146 - 157, Add a finite timeout-minutes setting to the blocking shim-test-windows job so hung Windows child processes cannot block the merge gate for the default duration; keep the existing needs, runner, and test steps unchanged.scripts/run.js (1)
79-90: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueConsider
128 + signalexit codes for signal-terminated children.Both branches exit
1, so a Ctrl+C duringauth loginis indistinguishable from a generic failure for callers (shell loops, CI wrappers,traphandlers) that rely on the conventional130/143. The tests pin1, so this is a deliberate contract — worth confirming it's the intended one before it becomes user-visible behavior.♻️ Optional: forward conventional signal exit codes
if (e.signal) { + const codes = { SIGINT: 130, SIGTERM: 143, SIGKILL: 137 }; + const status = codes[e.signal] || 1; if (e.signal === "SIGINT" || e.signal === "SIGTERM") { - process.exit(1); + process.exit(status); } console.error( `\nlark-cli: the native binary was terminated by signal ${e.signal}.\n` + ` path: ${bin}\n\n` + `Report this error at https://github.com/larksuite/cli/issues\n` + `Please include the path and signal shown above.\n` ); - process.exit(1); + process.exit(status); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/run.js` around lines 79 - 90, Update the signal-handling branches in the child-process error flow to exit with the conventional 128 plus the terminating signal’s numeric value, including SIGINT and SIGTERM, instead of always exiting with 1. Preserve the existing diagnostic output for non-interrupt signals and update the associated tests to assert the new exit-code contract.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In @.github/workflows/ci.yml:
- Around line 146-157: Add a finite timeout-minutes setting to the blocking
shim-test-windows job so hung Windows child processes cannot block the merge
gate for the default duration; keep the existing needs, runner, and test steps
unchanged.
In `@scripts/run.js`:
- Around line 79-90: Update the signal-handling branches in the child-process
error flow to exit with the conventional 128 plus the terminating signal’s
numeric value, including SIGINT and SIGTERM, instead of always exiting with 1.
Preserve the existing diagnostic output for non-interrupt signals and update the
associated tests to assert the new exit-code contract.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: edab3e3a-944f-4a2f-bdae-8dae23b2d6a0
📒 Files selected for processing (5)
.github/workflows/ci.ymlMakefilescripts/ci-workflow.test.shscripts/run.jsscripts/run.test.js
🚀 PR Preview Install Guide🧰 CLI updatenpm i -g https://pkg.pr.new/larksuite/cli/@larksuite/cli@5ed41d67641ec6ec251c6f1b3ed267e3405bc419🧩 Skill updatenpx skills add larksuite/cli#fix/2053-no-output-after-install -y -g |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #2116 +/- ##
=======================================
Coverage 75.55% 75.55%
=======================================
Files 931 931
Lines 99362 99362
=======================================
Hits 75077 75077
Misses 18549 18549
Partials 5736 5736 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
…ckpressure Address external review of PR #2116: a crashing native binary on Windows (no POSIX signals) was silently forwarded as a numeric exit status instead of being reported as a crash, and the shim's diagnostic writes could be dropped entirely if process.exit() ran before an async pipe write to a congested stderr had flushed. Also tightens two test comments that overgeneralized Linux-specific and argv-leak-vector behavior to POSIX, adds regression coverage for both fixes, and closes a gap in the CI gate assertions that only checked the FAILED loop, not the results job's needs: array.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
scripts/run.test.js (1)
440-441: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse
execFileSyncfor thepgrepcall.
shim.pidis a number, so there is no injection risk here.execFileSyncavoids the shell and clears the OpenGrep finding.♻️ Proposed change
- const out = execSync(`pgrep -P ${shim.pid}`).toString().trim(); + const out = execFileSync("pgrep", ["-P", String(shim.pid)]) + .toString() + .trim();Update the import at Line 9 accordingly:
-const { spawnSync, spawn, execSync } = require("child_process"); +const { spawnSync, spawn, execFileSync } = require("child_process");🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/run.test.js` around lines 440 - 441, Replace the shell-based execSync invocation in the fixture PID lookup with execFileSync, passing pgrep and its arguments separately, and update the corresponding import so the call avoids shell execution while preserving the existing output parsing and fixturePid behavior.Source: Linters/SAST tools
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@scripts/run.test.js`:
- Around line 420-451: Add an immediate cleanup hook after spawning the shim in
the Promise around the test fixture, ensuring the shim is terminated on every
resolve, reject, or timeout path. Update the existing closed/error handling and
the fixture PID failure branches in the shim lifecycle logic so cleanup runs
before rejection and prevents blocked child processes from leaking.
---
Nitpick comments:
In `@scripts/run.test.js`:
- Around line 440-441: Replace the shell-based execSync invocation in the
fixture PID lookup with execFileSync, passing pgrep and its arguments
separately, and update the corresponding import so the call avoids shell
execution while preserving the existing output parsing and fixturePid behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 36cdcf7e-a1aa-4823-821c-f85f94fdf35d
📒 Files selected for processing (3)
scripts/ci-workflow.test.shscripts/run.jsscripts/run.test.js
🚧 Files skipped from review as they are similar to previous changes (1)
- scripts/ci-workflow.test.sh
liangshuo-1
left a comment
liangshuo-1
left a comment
There was a problem hiding this comment.
结论：Request changes。当前实现明显改善了诊断，但还不能保证“无 silent bad case / 无兼容性变化”。
必须修复
-
Linux 上空/纯空白可执行文件仍会静默
exit 0（scripts/run.test.js:164-177,scripts/run.js:68）
新测试明确知道 glibc/libuv 会在ENOEXEC后交给/bin/sh，却改用目录 fixture 绕开了这个真实 bad case。实测将bin/lark-cli设为0755的 0-byte 文件后，当前 HEAD 返回status=0, stdout=0B, stderr=0B，原生二进制根本没有运行，完全复现 #2053 的症状。install.js的copyFileSync(dest)也不是原子替换，中断后留下空/残缺目标文件并非不可达。建议在执行前校验至少isFile + size > 0（最好校验平台 magic），并将安装复制改为临时文件 + rename；测试应直接钉住 0-byte Linux case，而不是规避它。 -
Windows crash 的原始 NTSTATUS 被改写为 1，属于不必要的退出码 breaking change（
scripts/run.js:101,scripts/run.test.js:316）
base 会process.exit(e.status)，当前分支对0xC0000005打印诊断后改成 1。依赖%ERRORLEVEL%/ NTSTATUS 的 wrapper 将失去原始失败类型；这也与 PR 声明的 exit-status passthrough 不一致。可用process.exitCode = e.status; return同时保证 stderr flush 和原始状态透传，并把 Windows 断言改为精确状态值。 -
Windows crash 判定只覆盖
0xC0000000+，仍会让真实 crash 静默（scripts/run.js:92-104）
STATUS_BREAKPOINT (0x80000003)、STATUS_FATAL_APP_EXIT (0x40000015)等进程终止状态会直接走 silent passthrough；mock 路由验证当前两者均无诊断。因此“unexpected crashes”并未完整覆盖。请按 lark-cli 的小整数退出码契约补全异常状态分类，并在真实 Windows job 中覆盖0x40000015、0x80000003、0xC0000005，同时补0xC000013A必须保持 quiet 的负向测试。 -
auto-install 失败诊断仍可能被同一种 stderr backpressure 吞掉（
scripts/run.js:57-64）
这里仍是console.error(...)后立即process.exit(1)。我用同样的满管道 fixture 实测当前 HEAD 为code=1, captured=0B, diagnostic=false；改成process.exitCode = 1; return后诊断稳定保留。#2053 的 blocked-postinstall 场景正会进入 auto-install 路径，不能只修后面的 native-exec catch。
CI / 测试建议
shim-test-windows是 blocking job 且内部有无 timeout 的同步子进程，建议加timeout-minutes: 10~15，避免异常时按默认 360 分钟阻塞 results gate。- Windows job 当前直接执行
node scripts/run.test.js，未覆盖 issue 截图中的真实 npm-generated.cmd+ blocked postinstall 链路；关闭 #2053 前建议增加一个本地 fixture package 的 Windows npm shim E2E。
验证结果：make script-test 160 tests 通过；macOS 相关测试通过；Linux backpressure 连跑 20/20，通过 mutation 可证明旧实现必失败；Node 16 runtime case 通过；PR 的真实 windows-latest shim job 通过。当前 e2e-live/results 失败与 base 427cbd6 上相同（contact fixture 无用户），不是本 PR 引入，但在重新跑绿前仍不应合并。
2 participants
Summary
Fix the npm launcher so native binary launch failures and unexpected crashes produce actionable stderr diagnostics instead of failing silently. Issue #2053 is a Windows report, so this covers the platform's crash path explicitly rather than only POSIX signals.
Changes
0xC0000005, missing DLL). Windows has no POSIX signals, so a crash arrives as a numeric exit status that was previously forwarded silently — exactly the 安装 cli 以及 skill 后, config 和 auth 命令均无输出 #2053 symptom on the reporting platform.STATUS_CONTROL_C_EXITstays quiet, symmetric with theSIGINT/SIGTERMallowlist.SIGINTandSIGTERMinterruption while reporting other termination signals factually (never claiming "failed to launch" — the binary may have run for a while first).process.exitCode+ a natural return instead ofprocess.exit(), so a diagnostic is not dropped when stderr is a congested pipe (the AI-agent / log-wrapper case this CLI targets).Test Plan
make unit-testgo vet ./...shim-test-windows(the Windows crash-status assertion runs on a realwindows-latestrunner)Related Issues
Summary by CodeRabbit
Bug Fixes
Tests