fix: report native binary launch and crash failures - #2116
Open
Emrys1105 wants to merge 10 commits into
Open
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.
📝 WalkthroughWalkthroughThe shim runner now preserves bounded diagnostics for native launch failures, signals, and Windows NTSTATUS crashes. A new cross-platform test suite validates 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 Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
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@a1d1a7d02c3a921ae17d85536b9a71e25e7d510d🧩 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.19% 75.46% +0.26%
==========================================
Files 914 928 +14
Lines 96783 98479 +1696
==========================================
+ Hits 72778 74314 +1536
- Misses 18407 18511 +104
- Partials 5598 5654 +56 ☔ 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
Comment on lines
+420
to
+451
| return new Promise((resolve, reject) => { | ||
| const shim = spawn( | ||
| process.execPath, | ||
| [sandbox.runJs, "-e", childCode, "--", SENTINEL], | ||
| { stdio: ["ignore", "ignore", "pipe"] } | ||
| ); | ||
|
|
||
| // Attach the close listener up front. With the fixed shim, run.js's | ||
| // own process stays alive until its diagnostic write actually | ||
| // flushes, but that is exactly what this test must not assume -- | ||
| // attaching this late could miss an early "close". | ||
| const closed = new Promise((res) => shim.on("close", res)); | ||
| shim.on("error", reject); | ||
|
|
||
| setTimeout(() => { | ||
| // run.js's execFileSync gives the fixture no pid of its own to | ||
| // the caller (it is a grandchild); discover it as the shim's | ||
| // child process instead. | ||
| let fixturePid = null; | ||
| try { | ||
| const out = execSync(`pgrep -P ${shim.pid}`).toString().trim(); | ||
| fixturePid = out ? Number(out.split("\n")[0]) : null; | ||
| } catch (err) { | ||
| reject( | ||
| new Error(`could not discover the fixture pid: ${err.message}`) | ||
| ); | ||
| return; | ||
| } | ||
| if (!fixturePid) { | ||
| reject(new Error("fixture pid not found under the shim")); | ||
| return; | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Kill the shim on every exit path.
The test rejects at Line 443 and Line 449 without stopping shim. In those paths the fixture stays blocked on a full, unread pipe, and the shim stays alive waiting for it. The leaked processes can keep the test runner or the CI job alive after the failure. Register a cleanup hook right after spawn so any exit path terminates the shim.
🛡️ Proposed cleanup hook
const shim = spawn(
process.execPath,
[sandbox.runJs, "-e", childCode, "--", SENTINEL],
{ stdio: ["ignore", "ignore", "pipe"] }
);
+ t.after(() => {
+ if (shim.exitCode === null && shim.signalCode === null) {
+ shim.kill("SIGKILL");
+ }
+ });📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| return new Promise((resolve, reject) => { | |
| const shim = spawn( | |
| process.execPath, | |
| [sandbox.runJs, "-e", childCode, "--", SENTINEL], | |
| { stdio: ["ignore", "ignore", "pipe"] } | |
| ); | |
| // Attach the close listener up front. With the fixed shim, run.js's | |
| // own process stays alive until its diagnostic write actually | |
| // flushes, but that is exactly what this test must not assume -- | |
| // attaching this late could miss an early "close". | |
| const closed = new Promise((res) => shim.on("close", res)); | |
| shim.on("error", reject); | |
| setTimeout(() => { | |
| // run.js's execFileSync gives the fixture no pid of its own to | |
| // the caller (it is a grandchild); discover it as the shim's | |
| // child process instead. | |
| let fixturePid = null; | |
| try { | |
| const out = execSync(`pgrep -P ${shim.pid}`).toString().trim(); | |
| fixturePid = out ? Number(out.split("\n")[0]) : null; | |
| } catch (err) { | |
| reject( | |
| new Error(`could not discover the fixture pid: ${err.message}`) | |
| ); | |
| return; | |
| } | |
| if (!fixturePid) { | |
| reject(new Error("fixture pid not found under the shim")); | |
| return; | |
| } | |
| return new Promise((resolve, reject) => { | |
| const shim = spawn( | |
| process.execPath, | |
| [sandbox.runJs, "-e", childCode, "--", SENTINEL], | |
| { stdio: ["ignore", "ignore", "pipe"] } | |
| ); | |
| t.after(() => { | |
| if (shim.exitCode === null && shim.signalCode === null) { | |
| shim.kill("SIGKILL"); | |
| } | |
| }); | |
| // Attach the close listener up front. With the fixed shim, run.js's | |
| // own process stays alive until its diagnostic write actually | |
| // flushes, but that is exactly what this test must not assume -- | |
| // attaching this late could miss an early "close". | |
| const closed = new Promise((res) => shim.on("close", res)); | |
| shim.on("error", reject); | |
| setTimeout(() => { | |
| // run.js's execFileSync gives the fixture no pid of its own to | |
| // the caller (it is a grandchild); discover it as the shim's | |
| // child process instead. | |
| let fixturePid = null; | |
| try { | |
| const out = execSync(`pgrep -P ${shim.pid}`).toString().trim(); | |
| fixturePid = out ? Number(out.split("\n")[0]) : null; | |
| } catch (err) { | |
| reject( | |
| new Error(`could not discover the fixture pid: ${err.message}`) | |
| ); | |
| return; | |
| } | |
| if (!fixturePid) { | |
| reject(new Error("fixture pid not found under the shim")); | |
| return; | |
| } |
🧰 Tools
🪛 OpenGrep (1.26.0)
[ERROR] 440-440: Dynamic command passed to child_process.exec/execSync. Use child_process.execFile or spawn with an argument array instead.
(coderabbit.command-injection.exec-js)
🤖 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 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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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