From 44764c27c2d840fa582a21dfe996f02a9aca5562 Mon Sep 17 00:00:00 2001 From: "zhangzhongbo.1105" Date: Thu, 30 Jul 2026 11:18:58 +0800 Subject: [PATCH 01/10] fix: report the OS error when the native binary cannot launch --- Makefile | 2 +- scripts/run.js | 23 ++++++++- scripts/run.test.js | 123 ++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 146 insertions(+), 2 deletions(-) create mode 100644 scripts/run.test.js diff --git a/Makefile b/Makefile index a338519e3f..4b2f524532 100644 --- a/Makefile +++ b/Makefile @@ -51,7 +51,7 @@ script-test: bash scripts/resolve-changed-from.test.sh bash scripts/ci-workflow.test.sh bash scripts/semantic-review-workflow.test.sh - $(NODE) --test scripts/e2e_domains.test.js scripts/fetch_e2e_tat.test.js scripts/install.test.js scripts/release-preflight.test.js scripts/semantic-review-verify-artifact.test.js scripts/pr-quality-summary.test.js scripts/semantic-review-publish.test.js scripts/ci-quality-summary-publish.test.js + $(NODE) --test scripts/e2e_domains.test.js scripts/fetch_e2e_tat.test.js scripts/install.test.js scripts/release-preflight.test.js scripts/semantic-review-verify-artifact.test.js scripts/pr-quality-summary.test.js scripts/semantic-review-publish.test.js scripts/ci-quality-summary-publish.test.js scripts/run.test.js # ./extension/... keeps the public plugin SDK in the default test matrix. unit-test: fetch_meta diff --git a/scripts/run.js b/scripts/run.js index a560fc02c4..f65de443fc 100755 --- a/scripts/run.js +++ b/scripts/run.js @@ -67,6 +67,27 @@ if (args[0] === "install") { try { execFileSync(bin, args, { stdio: "inherit" }); } catch (e) { - process.exit(e.status || 1); + // The binary ran and chose its own status — forward it untouched. + if (typeof e.status === "number") { + process.exit(e.status); + } + // The binary ran and was killed by a signal (Ctrl+C during `auth login`, + // for one). Not the shim's business to editorialise. + if (e.signal) { + process.exit(1); + } + // Neither: the launch itself failed. Report only what is actually known — + // permissions, file format, CPU architecture and endpoint policy all land + // here, and the errno is the only evidence. Print e.code and never + // e.message: when the child does run, e.message carries the full argv, + // which can contain values the caller passed on the command line. + const reason = typeof e.code === "string" ? e.code : "UNKNOWN"; + console.error( + `\nlark-cli: failed to launch the native binary.\n` + + ` path: ${bin}\n` + + ` error: ${reason}\n\n` + + `Report this error at https://github.com/larksuite/cli/issues\n` + ); + process.exit(1); } } diff --git a/scripts/run.test.js b/scripts/run.test.js new file mode 100644 index 0000000000..941bfd03a1 --- /dev/null +++ b/scripts/run.test.js @@ -0,0 +1,123 @@ +// Copyright (c) 2026 Lark Technologies Pte. Ltd. +// SPDX-License-Identifier: MIT + +const { describe, it } = require("node:test"); +const assert = require("node:assert/strict"); +const fs = require("fs"); +const os = require("os"); +const path = require("path"); +const { spawnSync } = require("child_process"); + +const MARKER = "failed to launch the native binary"; +const IS_WINDOWS = process.platform === "win32"; +const BIN_NAME = "lark-cli" + (IS_WINDOWS ? ".exe" : ""); + +// run.js resolves the native binary relative to its own directory +// (/../bin/lark-cli), so a fixture needs a full scripts/ + bin/ layout. +// Building that under fs.mkdtempSync keeps the developer's real ./bin/lark-cli +// untouched and keeps the location unpredictable. Never replace this with a +// fixed path such as %TEMP%\lark-cli-test — dropping a copy of the node +// executable into a predictable user-writable directory changes Windows' DLL +// search order. +function makeSandbox(t) { + const root = fs.mkdtempSync(path.join(os.tmpdir(), "lark-cli-run-")); + t.after(() => fs.rmSync(root, { recursive: true, force: true })); + fs.mkdirSync(path.join(root, "scripts")); + fs.mkdirSync(path.join(root, "bin")); + fs.copyFileSync( + path.join(__dirname, "run.js"), + path.join(root, "scripts", "run.js") + ); + // run.js runs install.js when the binary is missing. A stub that exits 0 + // without producing one lets the "installer succeeded but produced nothing" + // path reach the launch attempt instead of stopping earlier. + fs.writeFileSync( + path.join(root, "scripts", "install.js"), + "process.exit(0);\n" + ); + return { + root, + bin: path.join(root, "bin", BIN_NAME), + runJs: path.join(root, "scripts", "run.js"), + }; +} + +function runShim(sandbox, args) { + return spawnSync(process.execPath, [sandbox.runJs, ...args], { + encoding: "utf8", + }); +} + +function assertLaunchFailure(res, sandbox) { + assert.equal(res.status, 1); + assert.ok( + res.stderr.includes(MARKER), + `missing diagnostic, stderr was: ${res.stderr}` + ); + assert.ok( + res.stderr.includes(sandbox.bin), + `diagnostic omits the binary path: ${res.stderr}` + ); + const line = res.stderr.match(/^ {2}error: (.+)$/m); + assert.ok(line, `diagnostic omits an error line: ${res.stderr}`); + const reason = line[1].trim(); + assert.notEqual(reason, "", "error line is empty"); + return reason; +} + +describe("run.js launch-failure diagnostics", () => { + it("reports a zero-byte binary instead of exiting silently", (t) => { + const sandbox = makeSandbox(t); + fs.writeFileSync(sandbox.bin, ""); + if (!IS_WINDOWS) { + fs.chmodSync(sandbox.bin, 0o755); + } + + const res = runShim(sandbox, ["--version"]); + const reason = assertLaunchFailure(res, sandbox); + + // The errno is deliberately not pinned to a literal here: POSIX reports + // ENOEXEC, and finding out what Windows reports is the reason this file + // also runs on windows-latest. Only require that something specific was + // captured. + assert.notEqual(reason, "UNKNOWN", "the OS error was not captured"); + + // Endpoint protection can quarantine a zero-byte .exe, which would silently + // turn this into the missing-binary case. + assert.ok(fs.existsSync(sandbox.bin), "fixture binary vanished before launch"); + assert.equal( + fs.statSync(sandbox.bin).size, + 0, + "fixture binary is no longer zero-byte" + ); + + if (process.env.GITHUB_STEP_SUMMARY) { + fs.appendFileSync( + process.env.GITHUB_STEP_SUMMARY, + `- \`run.js\` zero-byte binary on ${process.platform}: \`${reason}\`\n` + ); + } + }); + + it( + "reports a binary that cannot be executed", + { skip: IS_WINDOWS ? "POSIX execute bit" : false }, + (t) => { + const sandbox = makeSandbox(t); + fs.writeFileSync(sandbox.bin, "#!/bin/sh\nexit 0\n"); + fs.chmodSync(sandbox.bin, 0o000); + + const res = runShim(sandbox, ["--version"]); + + assert.equal(assertLaunchFailure(res, sandbox), "EACCES"); + } + ); + + it("reports a missing binary when the installer produced nothing", (t) => { + const sandbox = makeSandbox(t); + + const res = runShim(sandbox, ["--version"]); + + assert.equal(assertLaunchFailure(res, sandbox), "ENOENT"); + }); +}); From ce16aeead9bd3f6896ff4cb0629d4612aa4de1dc Mon Sep 17 00:00:00 2001 From: "zhangzhongbo.1105" Date: Thu, 30 Jul 2026 11:33:13 +0800 Subject: [PATCH 02/10] test: guard shim exit-status passthrough and argv containment --- scripts/run.test.js | 67 +++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 67 insertions(+) diff --git a/scripts/run.test.js b/scripts/run.test.js index 941bfd03a1..137c48efe2 100644 --- a/scripts/run.test.js +++ b/scripts/run.test.js @@ -11,6 +11,8 @@ const { spawnSync } = require("child_process"); const MARKER = "failed to launch the native binary"; const IS_WINDOWS = process.platform === "win32"; const BIN_NAME = "lark-cli" + (IS_WINDOWS ? ".exe" : ""); +const SENTINEL = "--sentinel=LEAKCANARY"; +const SENTINEL_TOKEN = "LEAKCANARY"; // run.js resolves the native binary relative to its own directory // (/../bin/lark-cli), so a fixture needs a full scripts/ + bin/ layout. @@ -48,6 +50,34 @@ function runShim(sandbox, args) { }); } +// A fixture that actually launches: a copy of the running node executable. +// A #!/bin/sh script cannot be started as lark-cli.exe on Windows, which would +// make every "the binary ran" case inert there. +function installRunnableBinary(sandbox) { + fs.copyFileSync(process.execPath, sandbox.bin); + if (!IS_WINDOWS) { + fs.chmodSync(sandbox.bin, 0o755); + } +} + +// `--` stops node from claiming the sentinel as one of its own options; without +// it node bails out with "bad option" and exits 9, which would silently +// invalidate the fixture instead of exercising the intended exit status. +function runRanBinary(sandbox, code) { + return runShim(sandbox, ["-e", code, "--", SENTINEL]); +} + +function assertBinaryRan(res) { + assert.ok( + !res.stderr.includes(MARKER), + `launch-failure diagnostic on a binary that ran: ${res.stderr}` + ); + assert.ok( + !res.stderr.includes(SENTINEL_TOKEN), + `caller arguments leaked into stderr: ${res.stderr}` + ); +} + function assertLaunchFailure(res, sandbox) { assert.equal(res.status, 1); assert.ok( @@ -121,3 +151,40 @@ describe("run.js launch-failure diagnostics", () => { assert.equal(assertLaunchFailure(res, sandbox), "ENOENT"); }); }); + +describe("run.js pass-through when the binary runs", () => { + it("forwards a non-zero exit status unchanged", (t) => { + const sandbox = makeSandbox(t); + installRunnableBinary(sandbox); + + const res = runRanBinary(sandbox, "process.exit(7)"); + + assert.equal(res.status, 7); + assertBinaryRan(res); + }); + + it( + "exits 1 without a diagnostic when the binary is killed by a signal", + { skip: IS_WINDOWS ? "POSIX signals" : false }, + (t) => { + const sandbox = makeSandbox(t); + installRunnableBinary(sandbox); + + const res = runRanBinary(sandbox, "process.kill(process.pid, 'SIGTERM')"); + + assert.equal(res.status, 1); + assertBinaryRan(res); + } + ); + + it("passes stdout through and exits 0", (t) => { + const sandbox = makeSandbox(t); + installRunnableBinary(sandbox); + + const res = runRanBinary(sandbox, "console.log('shim-passthrough')"); + + assert.equal(res.status, 0); + assert.match(res.stdout, /shim-passthrough/); + assertBinaryRan(res); + }); +}); From f14ae7cda6e31e5bf9084bef058820d619ef3fdd Mon Sep 17 00:00:00 2001 From: "zhangzhongbo.1105" Date: Thu, 30 Jul 2026 11:46:59 +0800 Subject: [PATCH 03/10] ci: run npm shim tests on Windows and gate on the result --- .github/workflows/ci.yml | 17 ++++++++++++++++- scripts/ci-workflow.test.sh | 18 ++++++++++++++++++ 2 files changed, 34 insertions(+), 1 deletion(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 9d79d3d182..542c7448d5 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -143,6 +143,19 @@ jobs: - name: Run script tests run: make script-test + shim-test-windows: + needs: fast-gate + runs-on: windows-latest + steps: + - uses: actions/checkout@93cb6efe18208431cddfb8368fd83d5badbf9bfd # v5 + with: + persist-credentials: false + - uses: actions/setup-node@49933ea5288caeca8642d1e84afbd3f7d6820020 # v4 + with: + node-version: '22' + - name: Run npm shim tests on Windows + run: node --test scripts/run.test.js + deterministic-gate: needs: fast-gate runs-on: ubuntu-latest @@ -520,7 +533,7 @@ jobs: # ── Results Gate (single required check for branch protection) ───── results: if: ${{ always() }} - needs: [fast-gate, unit-test, lint, script-test, deterministic-gate, coverage, deadcode, e2e-dry-run, e2e-live, security, license-header, plugin-integration, sidecar-integration] + needs: [fast-gate, unit-test, lint, script-test, shim-test-windows, deterministic-gate, coverage, deadcode, e2e-dry-run, e2e-live, security, license-header, plugin-integration, sidecar-integration] runs-on: ubuntu-latest steps: - name: Evaluate results @@ -533,6 +546,7 @@ jobs: echo "| L2 | unit-test | ${{ needs.unit-test.result }} |" >> $GITHUB_STEP_SUMMARY echo "| L2 | lint | ${{ needs.lint.result }} |" >> $GITHUB_STEP_SUMMARY echo "| L2 | script-test | ${{ needs.script-test.result }} |" >> $GITHUB_STEP_SUMMARY + echo "| L2 | shim-test-windows | ${{ needs.shim-test-windows.result }} |" >> $GITHUB_STEP_SUMMARY echo "| L2 | deterministic-gate | ${{ needs.deterministic-gate.result }} |" >> $GITHUB_STEP_SUMMARY echo "| L2 | coverage | ${{ needs.coverage.result }} |" >> $GITHUB_STEP_SUMMARY echo "| L2 | deadcode | ${{ needs.deadcode.result }} |" >> $GITHUB_STEP_SUMMARY @@ -559,6 +573,7 @@ jobs: "${{ needs.unit-test.result }}" \ "${{ needs.lint.result }}" \ "${{ needs.script-test.result }}" \ + "${{ needs.shim-test-windows.result }}" \ "${{ needs.deterministic-gate.result }}" \ "${{ needs.coverage.result }}" \ "${{ needs.deadcode.result }}" \ diff --git a/scripts/ci-workflow.test.sh b/scripts/ci-workflow.test.sh index 6acdb0adc1..55852d6b4b 100644 --- a/scripts/ci-workflow.test.sh +++ b/scripts/ci-workflow.test.sh @@ -31,6 +31,7 @@ lint_section="$(awk ' /^ script-test:/ { exit } ' "$workflow")" script_test_section="$(job_section script-test)" +shim_test_windows_section="$(job_section shim-test-windows)" deterministic_section="$(awk ' /^ deterministic-gate:/ { in_job = 1 } in_job { print } @@ -170,6 +171,23 @@ if grep -Fq '${{ secrets.' <<<"$script_test_section"; then exit 1 fi +if ! grep -Fq "runs-on: windows-latest" <<<"$shim_test_windows_section"; then + echo "shim-test-windows must run on windows-latest so the npm shim is exercised on the platform it broke on" + exit 1 +fi +if ! grep -Fq "node --test scripts/run.test.js" <<<"$shim_test_windows_section"; then + echo "shim-test-windows must run the npm shim tests" + exit 1 +fi +if ! grep -Fq "node-version: '22'" <<<"$shim_test_windows_section"; then + echo "shim-test-windows must pin node-version so process.execPath fixtures stay reproducible" + exit 1 +fi +if ! grep -Fq '"${{ needs.shim-test-windows.result }}"' <<<"$results_section"; then + echo "shim-test-windows must sit inside the results FAILED loop; needs and the summary table alone leave it non-blocking" + exit 1 +fi + if grep -Fq "metadata-gate:" "$workflow"; then echo "metadata-gate should not run alongside deterministic-gate because both would upload the same facts artifact" exit 1 From e479585ca474e40048d1a11a30daec91167fe539 Mon Sep 17 00:00:00 2001 From: "zhangzhongbo.1105" Date: Thu, 30 Jul 2026 13:24:53 +0800 Subject: [PATCH 04/10] fix(scripts): harden shim launch-failure tests and diagnostic for Windows 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. --- scripts/ci-workflow.test.sh | 1 + scripts/run.js | 3 ++- scripts/run.test.js | 54 ++++++++++++++++++++++++++++++++++--- 3 files changed, 53 insertions(+), 5 deletions(-) diff --git a/scripts/ci-workflow.test.sh b/scripts/ci-workflow.test.sh index 55852d6b4b..247084dd02 100644 --- a/scripts/ci-workflow.test.sh +++ b/scripts/ci-workflow.test.sh @@ -202,6 +202,7 @@ for full_job in \ "$unit_test_section" \ "$lint_section" \ "$script_test_section" \ + "$shim_test_windows_section" \ "$deterministic_section" \ "$coverage_job_section" \ "$dry_run_section" \ diff --git a/scripts/run.js b/scripts/run.js index f65de443fc..2a8f603676 100755 --- a/scripts/run.js +++ b/scripts/run.js @@ -86,7 +86,8 @@ if (args[0] === "install") { `\nlark-cli: failed to launch the native binary.\n` + ` path: ${bin}\n` + ` error: ${reason}\n\n` + - `Report this error at https://github.com/larksuite/cli/issues\n` + `Report this error at https://github.com/larksuite/cli/issues\n` + + `Please include the path and error shown above.\n` ); process.exit(1); } diff --git a/scripts/run.test.js b/scripts/run.test.js index 137c48efe2..453696655a 100644 --- a/scripts/run.test.js +++ b/scripts/run.test.js @@ -22,8 +22,30 @@ const SENTINEL_TOKEN = "LEAKCANARY"; // executable into a predictable user-writable directory changes Windows' DLL // search order. function makeSandbox(t) { - const root = fs.mkdtempSync(path.join(os.tmpdir(), "lark-cli-run-")); - t.after(() => fs.rmSync(root, { recursive: true, force: true })); + // fs.realpathSync canonicalises the temp root. assertLaunchFailure checks + // res.stderr.includes(sandbox.bin), but the path run.js prints comes from + // __dirname, and Node canonicalises a main module's path. On macOS the + // difference is /var -> /private/var, a prefix extension, so substring + // containment still holds and the tests pass unnoticed. On Windows it is + // NOT a prefix extension: libuv's realpath uses GetFinalPathNameByHandle + // with FILE_NAME_NORMALIZED, which expands 8.3 short names, and + // GitHub-hosted Windows runners set TEMP to + // C:\Users\RUNNER~1\AppData\Local\Temp while the profile directory is + // runneradmin. Short-name expansion is a substitution, not a prefix + // extension, so includes() returns false and every launch-failure case + // fails on the one platform the shim-test-windows job exists to cover. Do + // not "simplify" this back to the bare mkdtempSync result. + const root = fs.realpathSync( + fs.mkdtempSync(path.join(os.tmpdir(), "lark-cli-run-")) + ); + t.after(() => + fs.rmSync(root, { + recursive: true, + force: true, + maxRetries: 5, + retryDelay: 100, + }) + ); fs.mkdirSync(path.join(root, "scripts")); fs.mkdirSync(path.join(root, "bin")); fs.copyFileSync( @@ -80,6 +102,7 @@ function assertBinaryRan(res) { function assertLaunchFailure(res, sandbox) { assert.equal(res.status, 1); + assert.equal(res.stdout, "", `stdout should stay empty: ${res.stdout}`); assert.ok( res.stderr.includes(MARKER), `missing diagnostic, stderr was: ${res.stderr}` @@ -92,6 +115,20 @@ function assertLaunchFailure(res, sandbox) { assert.ok(line, `diagnostic omits an error line: ${res.stderr}`); const reason = line[1].trim(); assert.notEqual(reason, "", "error line is empty"); + // Closure of a security review finding: a user facing EACCES with no + // pointer tends to reach for sudo / chmod 777 / disabling endpoint + // protection. Without this assertion, deleting the tracker line (and its + // follow-up sentence) from run.js leaves the suite fully green. + assert.ok( + res.stderr.includes( + "Report this error at https://github.com/larksuite/cli/issues" + ), + `diagnostic omits the issue tracker line: ${res.stderr}` + ); + assert.ok( + res.stderr.includes("Please include the path and error shown above."), + `diagnostic omits the follow-up line: ${res.stderr}` + ); return reason; } @@ -177,14 +214,23 @@ describe("run.js pass-through when the binary runs", () => { } ); - it("passes stdout through and exits 0", (t) => { + it("passes stdout and stderr through and exits 0", (t) => { const sandbox = makeSandbox(t); installRunnableBinary(sandbox); - const res = runRanBinary(sandbox, "console.log('shim-passthrough')"); + const res = runRanBinary( + sandbox, + "console.log('shim-passthrough'); console.error('shim-stderr')" + ); assert.equal(res.status, 0); assert.match(res.stdout, /shim-passthrough/); + // A shim that swallowed child stderr would be a variant of the very bug + // this branch repairs (issue #2053: launch failures with no evidence). + // This does not collide with assertBinaryRan's checks below: it asserts + // MARKER and the argv sentinel are absent from stderr, and "shim-stderr" + // is neither. + assert.match(res.stderr, /shim-stderr/); assertBinaryRan(res); }); }); From 4714205c69edafa91b4f22af227c9ce8b922ba90 Mon Sep 17 00:00:00 2001 From: "zhangzhongbo.1105" Date: Thu, 30 Jul 2026 13:39:49 +0800 Subject: [PATCH 05/10] test: extend argv and secrets hardening assertions 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. --- scripts/ci-workflow.test.sh | 5 +++++ scripts/run.test.js | 16 +++++++++++++--- 2 files changed, 18 insertions(+), 3 deletions(-) diff --git a/scripts/ci-workflow.test.sh b/scripts/ci-workflow.test.sh index 247084dd02..06e0161563 100644 --- a/scripts/ci-workflow.test.sh +++ b/scripts/ci-workflow.test.sh @@ -188,6 +188,11 @@ if ! grep -Fq '"${{ needs.shim-test-windows.result }}"' <<<"$results_section"; t exit 1 fi +if grep -Fq '${{ secrets.' <<<"$shim_test_windows_section"; then + echo "shim-test-windows must not reference secrets" + exit 1 +fi + if grep -Fq "metadata-gate:" "$workflow"; then echo "metadata-gate should not run alongside deterministic-gate because both would upload the same facts artifact" exit 1 diff --git a/scripts/run.test.js b/scripts/run.test.js index 453696655a..6cd1275003 100644 --- a/scripts/run.test.js +++ b/scripts/run.test.js @@ -129,6 +129,16 @@ function assertLaunchFailure(res, sandbox) { res.stderr.includes("Please include the path and error shown above."), `diagnostic omits the follow-up line: ${res.stderr}` ); + // The sentinel check for launch-failure cases is not redundant with the + // check in assertBinaryRan: when the launch fails, e.message contains only + // the errno, not the full argv. However, the error object also carries + // e.spawnargs and e.path, which hold the complete argument list even on + // launch failure. This assertion catches any future edit that prints + // e.spawnargs, String(e), or the whole error object. + assert.ok( + !res.stderr.includes(SENTINEL_TOKEN), + `caller arguments leaked into stderr: ${res.stderr}` + ); return reason; } @@ -140,7 +150,7 @@ describe("run.js launch-failure diagnostics", () => { fs.chmodSync(sandbox.bin, 0o755); } - const res = runShim(sandbox, ["--version"]); + const res = runShim(sandbox, ["--version", SENTINEL]); const reason = assertLaunchFailure(res, sandbox); // The errno is deliberately not pinned to a literal here: POSIX reports @@ -174,7 +184,7 @@ describe("run.js launch-failure diagnostics", () => { fs.writeFileSync(sandbox.bin, "#!/bin/sh\nexit 0\n"); fs.chmodSync(sandbox.bin, 0o000); - const res = runShim(sandbox, ["--version"]); + const res = runShim(sandbox, ["--version", SENTINEL]); assert.equal(assertLaunchFailure(res, sandbox), "EACCES"); } @@ -183,7 +193,7 @@ describe("run.js launch-failure diagnostics", () => { it("reports a missing binary when the installer produced nothing", (t) => { const sandbox = makeSandbox(t); - const res = runShim(sandbox, ["--version"]); + const res = runShim(sandbox, ["--version", SENTINEL]); assert.equal(assertLaunchFailure(res, sandbox), "ENOENT"); }); From 246e0ae4fdef7d4dfac7fd7c86ba32f4d7ce6aa4 Mon Sep 17 00:00:00 2001 From: "zhangzhongbo.1105" Date: Thu, 30 Jul 2026 14:20:12 +0800 Subject: [PATCH 06/10] test(run): pin POSIX launch-failure fixture to a directory, not zero 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. --- scripts/run.test.js | 70 ++++++++++++++++++++++++++++++++------------- 1 file changed, 50 insertions(+), 20 deletions(-) diff --git a/scripts/run.test.js b/scripts/run.test.js index 6cd1275003..56d956dd85 100644 --- a/scripts/run.test.js +++ b/scripts/run.test.js @@ -10,6 +10,8 @@ const { spawnSync } = require("child_process"); const MARKER = "failed to launch the native binary"; const IS_WINDOWS = process.platform === "win32"; +const IS_ROOT = + !IS_WINDOWS && typeof process.getuid === "function" && process.getuid() === 0; const BIN_NAME = "lark-cli" + (IS_WINDOWS ? ".exe" : ""); const SENTINEL = "--sentinel=LEAKCANARY"; const SENTINEL_TOKEN = "LEAKCANARY"; @@ -143,42 +145,70 @@ function assertLaunchFailure(res, sandbox) { } describe("run.js launch-failure diagnostics", () => { - it("reports a zero-byte binary instead of exiting silently", (t) => { + it("reports a binary that fails to launch instead of exiting silently", (t) => { const sandbox = makeSandbox(t); - fs.writeFileSync(sandbox.bin, ""); - if (!IS_WINDOWS) { - fs.chmodSync(sandbox.bin, 0o755); + + if (IS_WINDOWS) { + // On win32 a zero-byte file is not a valid PE image, so CreateProcess + // genuinely fails to start it: this really is a launch failure there. + fs.writeFileSync(sandbox.bin, ""); + } else { + // A 0-byte + executable file is NOT a launch failure on POSIX. libuv + // uses execvp, and POSIX execvp falls back to running the file through + // /bin/sh whenever execve returns ENOEXEC. /bin/sh then executes the + // empty file as an empty script and exits 0 -- execFileSync never + // throws, so this branch would never fire (measured as uid 0 in the + // Linux sandbox container: no throw, child exited 0). Do NOT + // "simplify" this back to a 0-byte file. A directory at the bin path + // passes the shim's existence check and then fails at exec with + // EACCES, and unlike chmod 0o000 that holds for uid 0 too, so this + // still proves the branch when CI runs as root. + fs.mkdirSync(sandbox.bin); } const res = runShim(sandbox, ["--version", SENTINEL]); const reason = assertLaunchFailure(res, sandbox); - // The errno is deliberately not pinned to a literal here: POSIX reports - // ENOEXEC, and finding out what Windows reports is the reason this file - // also runs on windows-latest. Only require that something specific was - // captured. - assert.notEqual(reason, "UNKNOWN", "the OS error was not captured"); - - // Endpoint protection can quarantine a zero-byte .exe, which would silently - // turn this into the missing-binary case. - assert.ok(fs.existsSync(sandbox.bin), "fixture binary vanished before launch"); - assert.equal( - fs.statSync(sandbox.bin).size, - 0, - "fixture binary is no longer zero-byte" - ); + if (IS_WINDOWS) { + // The errno is deliberately not pinned to a literal here: what Windows + // reports for a 0-byte .exe is still unverified, and finding out is + // the reason this file also runs on windows-latest. Only require that + // something specific was captured. + assert.notEqual(reason, "UNKNOWN", "the OS error was not captured"); + + // Endpoint protection can quarantine a zero-byte .exe, which would + // silently turn this into the missing-binary case. + assert.ok(fs.existsSync(sandbox.bin), "fixture binary vanished before launch"); + assert.equal( + fs.statSync(sandbox.bin).size, + 0, + "fixture binary is no longer zero-byte" + ); + } else { + // Measured with one identical directory fixture on both macOS (uid 501) + // and Linux (uid 0): EACCES both times, so this can be pinned exactly. + assert.equal(reason, "EACCES"); + } if (process.env.GITHUB_STEP_SUMMARY) { fs.appendFileSync( process.env.GITHUB_STEP_SUMMARY, - `- \`run.js\` zero-byte binary on ${process.platform}: \`${reason}\`\n` + `- \`run.js\` unlaunchable binary on ${process.platform}: \`${reason}\`\n` ); } }); it( "reports a binary that cannot be executed", - { skip: IS_WINDOWS ? "POSIX execute bit" : false }, + { + skip: IS_WINDOWS + ? "POSIX execute bit" + : IS_ROOT + ? "mode 0000 does not deny exec for uid 0, so this fixture cannot " + + "produce EACCES when running as root; the directory fixture above " + + "keeps EACCES covered" + : false, + }, (t) => { const sandbox = makeSandbox(t); fs.writeFileSync(sandbox.bin, "#!/bin/sh\nexit 0\n"); From 8d1d8df7cb5a7751defbfe07efd35d114c61fd6d Mon Sep 17 00:00:00 2001 From: "zhangzhongbo.1105" Date: Thu, 30 Jul 2026 15:41:11 +0800 Subject: [PATCH 07/10] fix: report native binary crash signals --- scripts/run.js | 16 ++++++++++++++-- scripts/run.test.js | 32 +++++++++++++++++++++++++++++++- 2 files changed, 45 insertions(+), 3 deletions(-) diff --git a/scripts/run.js b/scripts/run.js index 2a8f603676..5fb7943170 100755 --- a/scripts/run.js +++ b/scripts/run.js @@ -71,9 +71,21 @@ if (args[0] === "install") { if (typeof e.status === "number") { process.exit(e.status); } - // The binary ran and was killed by a signal (Ctrl+C during `auth login`, - // for one). Not the shim's business to editorialise. + // SIGINT and SIGTERM are the explicit quiet allowlist for intentional + // interruption (Ctrl+C during `auth login`, for one). Other signals are + // crash evidence worth reporting, but do not prove the binary failed to + // launch. Only print e.signal and the known bin path: e.message and related + // error fields can contain the caller's full argv. if (e.signal) { + if (e.signal === "SIGINT" || e.signal === "SIGTERM") { + process.exit(1); + } + 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); } // Neither: the launch itself failed. Report only what is actually known — diff --git a/scripts/run.test.js b/scripts/run.test.js index 56d956dd85..903440585e 100644 --- a/scripts/run.test.js +++ b/scripts/run.test.js @@ -241,7 +241,7 @@ describe("run.js pass-through when the binary runs", () => { }); it( - "exits 1 without a diagnostic when the binary is killed by a signal", + "exits 1 without a diagnostic when the binary receives SIGTERM", { skip: IS_WINDOWS ? "POSIX signals" : false }, (t) => { const sandbox = makeSandbox(t); @@ -250,10 +250,40 @@ describe("run.js pass-through when the binary runs", () => { const res = runRanBinary(sandbox, "process.kill(process.pid, 'SIGTERM')"); assert.equal(res.status, 1); + assert.equal(res.stdout, "", `stdout should stay empty: ${res.stdout}`); + assert.equal(res.stderr, "", `stderr should stay empty: ${res.stderr}`); assertBinaryRan(res); } ); + it( + "reports the signal when the binary crashes", + { skip: IS_WINDOWS ? "POSIX signals" : false }, + (t) => { + const sandbox = makeSandbox(t); + installRunnableBinary(sandbox); + + const res = runRanBinary(sandbox, "process.kill(process.pid, 'SIGKILL')"); + + const expected = + `\nlark-cli: the native binary was terminated by signal SIGKILL.\n` + + ` path: ${sandbox.bin}\n\n` + + `Report this error at https://github.com/larksuite/cli/issues\n` + + `Please include the path and signal shown above.\n\n`; + assert.equal(res.status, 1); + assert.equal(res.stdout, "", `stdout should stay empty: ${res.stdout}`); + assert.equal(res.stderr, expected); + assert.ok( + !res.stderr.includes(MARKER), + `crash signal was misreported as a launch failure: ${res.stderr}` + ); + assert.ok( + !res.stderr.includes(SENTINEL_TOKEN), + `caller arguments leaked into stderr: ${res.stderr}` + ); + } + ); + it("passes stdout and stderr through and exits 0", (t) => { const sandbox = makeSandbox(t); installRunnableBinary(sandbox); From 7614fa0f83c11faaa5aef15243e3e78658a85b82 Mon Sep 17 00:00:00 2001 From: "zhangzhongbo.1105" Date: Thu, 30 Jul 2026 15:57:46 +0800 Subject: [PATCH 08/10] test: cover quiet SIGINT interruption --- scripts/run.test.js | 35 ++++++++++++++++++++--------------- 1 file changed, 20 insertions(+), 15 deletions(-) diff --git a/scripts/run.test.js b/scripts/run.test.js index 903440585e..9cfd5bcaed 100644 --- a/scripts/run.test.js +++ b/scripts/run.test.js @@ -240,21 +240,26 @@ describe("run.js pass-through when the binary runs", () => { assertBinaryRan(res); }); - it( - "exits 1 without a diagnostic when the binary receives SIGTERM", - { skip: IS_WINDOWS ? "POSIX signals" : false }, - (t) => { - const sandbox = makeSandbox(t); - installRunnableBinary(sandbox); - - const res = runRanBinary(sandbox, "process.kill(process.pid, 'SIGTERM')"); - - assert.equal(res.status, 1); - assert.equal(res.stdout, "", `stdout should stay empty: ${res.stdout}`); - assert.equal(res.stderr, "", `stderr should stay empty: ${res.stderr}`); - assertBinaryRan(res); - } - ); + for (const signal of ["SIGINT", "SIGTERM"]) { + it( + `exits 1 without a diagnostic when the binary receives ${signal}`, + { skip: IS_WINDOWS ? "POSIX signals" : false }, + (t) => { + const sandbox = makeSandbox(t); + installRunnableBinary(sandbox); + + const res = runRanBinary( + sandbox, + `process.kill(process.pid, '${signal}')` + ); + + assert.equal(res.status, 1); + assert.equal(res.stdout, "", `stdout should stay empty: ${res.stdout}`); + assert.equal(res.stderr, "", `stderr should stay empty: ${res.stderr}`); + assertBinaryRan(res); + } + ); + } it( "reports the signal when the binary crashes", From a1de7026916121edb01f0f249749d72e6f233180 Mon Sep 17 00:00:00 2001 From: "zhangzhongbo.1105" Date: Thu, 30 Jul 2026 19:31:03 +0800 Subject: [PATCH 09/10] fix: keep launcher failure output concise --- scripts/run.js | 10 +++------- scripts/run.test.js | 21 ++++++++------------- 2 files changed, 11 insertions(+), 20 deletions(-) diff --git a/scripts/run.js b/scripts/run.js index 5fb7943170..6faecfe87c 100755 --- a/scripts/run.js +++ b/scripts/run.js @@ -73,7 +73,7 @@ if (args[0] === "install") { } // SIGINT and SIGTERM are the explicit quiet allowlist for intentional // interruption (Ctrl+C during `auth login`, for one). Other signals are - // crash evidence worth reporting, but do not prove the binary failed to + // crash evidence worth surfacing, but do not prove the binary failed to // launch. Only print e.signal and the known bin path: e.message and related // error fields can contain the caller's full argv. if (e.signal) { @@ -82,9 +82,7 @@ if (args[0] === "install") { } 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` + ` path: ${bin}` ); process.exit(1); } @@ -97,9 +95,7 @@ if (args[0] === "install") { console.error( `\nlark-cli: failed to launch the native binary.\n` + ` path: ${bin}\n` + - ` error: ${reason}\n\n` + - `Report this error at https://github.com/larksuite/cli/issues\n` + - `Please include the path and error shown above.\n` + ` error: ${reason}` ); process.exit(1); } diff --git a/scripts/run.test.js b/scripts/run.test.js index 9cfd5bcaed..4b44361056 100644 --- a/scripts/run.test.js +++ b/scripts/run.test.js @@ -117,19 +117,16 @@ function assertLaunchFailure(res, sandbox) { assert.ok(line, `diagnostic omits an error line: ${res.stderr}`); const reason = line[1].trim(); assert.notEqual(reason, "", "error line is empty"); - // Closure of a security review finding: a user facing EACCES with no - // pointer tends to reach for sudo / chmod 777 / disabling endpoint - // protection. Without this assertion, deleting the tracker line (and its - // follow-up sentence) from run.js leaves the suite fully green. + // Keep failure output factual and local. A launch failure is not necessarily + // a lark-cli bug, and prompting users to paste local paths into an issue adds + // noise and may expose machine-specific information. assert.ok( - res.stderr.includes( - "Report this error at https://github.com/larksuite/cli/issues" - ), - `diagnostic omits the issue tracker line: ${res.stderr}` + !res.stderr.includes("github.com/larksuite/cli/issues"), + `diagnostic should not prompt issue creation: ${res.stderr}` ); assert.ok( - res.stderr.includes("Please include the path and error shown above."), - `diagnostic omits the follow-up line: ${res.stderr}` + !res.stderr.includes("Please include"), + `diagnostic should not ask users to share local details: ${res.stderr}` ); // The sentinel check for launch-failure cases is not redundant with the // check in assertBinaryRan: when the launch fails, e.message contains only @@ -272,9 +269,7 @@ describe("run.js pass-through when the binary runs", () => { const expected = `\nlark-cli: the native binary was terminated by signal SIGKILL.\n` + - ` path: ${sandbox.bin}\n\n` + - `Report this error at https://github.com/larksuite/cli/issues\n` + - `Please include the path and signal shown above.\n\n`; + ` path: ${sandbox.bin}\n`; assert.equal(res.status, 1); assert.equal(res.stdout, "", `stdout should stay empty: ${res.stdout}`); assert.equal(res.stderr, expected); From a1d1a7d02c3a921ae17d85536b9a71e25e7d510d Mon Sep 17 00:00:00 2001 From: "zhangzhongbo.1105" Date: Fri, 31 Jul 2026 16:01:06 +0800 Subject: [PATCH 10/10] fix: surface win32 NTSTATUS crashes and preserve diagnostics under backpressure 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. --- scripts/ci-workflow.test.sh | 6 + scripts/run.js | 37 ++++++- scripts/run.test.js | 213 +++++++++++++++++++++++++++++++++--- 3 files changed, 240 insertions(+), 16 deletions(-) diff --git a/scripts/ci-workflow.test.sh b/scripts/ci-workflow.test.sh index 06e0161563..ce07ab9a58 100644 --- a/scripts/ci-workflow.test.sh +++ b/scripts/ci-workflow.test.sh @@ -188,6 +188,12 @@ if ! grep -Fq '"${{ needs.shim-test-windows.result }}"' <<<"$results_section"; t exit 1 fi +results_needs_line="$(grep -m1 '^ needs:' <<<"$results_section")" +if ! grep -Fq "shim-test-windows" <<<"$results_needs_line"; then + echo "shim-test-windows must be listed in the results job's needs: array; GitHub returns an empty string for an undeclared dependency's result, so dropping it here would silently make it non-blocking even though the FAILED loop still references it" + exit 1 +fi + if grep -Fq '${{ secrets.' <<<"$shim_test_windows_section"; then echo "shim-test-windows must not reference secrets" exit 1 diff --git a/scripts/run.js b/scripts/run.js index 6faecfe87c..789acec464 100755 --- a/scripts/run.js +++ b/scripts/run.js @@ -67,8 +67,40 @@ if (args[0] === "install") { try { execFileSync(bin, args, { stdio: "inherit" }); } catch (e) { + // Every branch below that prints a diagnostic ends with + // `process.exitCode = 1` and a plain return/fallthrough, never + // `process.exit(1)`. On POSIX, writes to a piped stderr are asynchronous + // (unlike a TTY); `process.exit()` tears the process down immediately and + // can drop a write that has not finished flushing yet. That is exactly + // what happens when the child has just filled a piped stderr (the + // AI-agent/log-wrapper case this CLI is built for): `process.exit(1)` + // right after `console.error(...)` can lose the entire diagnostic. + // Leaving `process.exitCode` set and returning naturally lets the event + // loop drain pending writes before the process actually exits, so the + // diagnostic survives; the exit code is still 1 either way. The silent + // branches (numeric-status forward, quiet SIGINT/SIGTERM) print nothing, + // so they keep using `process.exit` directly. // The binary ran and chose its own status — forward it untouched. if (typeof e.status === "number") { + // Windows has no signals: a crashing native binary (access violation + // 0xC0000005, missing DLL 0xC0000135, killed by endpoint protection) shows + // up as an NTSTATUS number here, not via e.signal. Surface the error-severity + // range (0xC0000000+) as a crash instead of forwarding it silently. Exclude + // 0xC000013A (STATUS_CONTROL_C_EXIT), the Windows Ctrl+C code, to stay + // symmetric with the quiet SIGINT/SIGTERM allowlist. A Go binary never exits + // with a code in this range deliberately. + if ( + process.platform === "win32" && + e.status >= 0xc0000000 && + e.status !== 0xc000013a + ) { + console.error( + `\nlark-cli: the native binary crashed (status 0x${(e.status >>> 0).toString(16)}).\n` + + ` path: ${bin}` + ); + process.exitCode = 1; + return; + } process.exit(e.status); } // SIGINT and SIGTERM are the explicit quiet allowlist for intentional @@ -84,7 +116,8 @@ if (args[0] === "install") { `\nlark-cli: the native binary was terminated by signal ${e.signal}.\n` + ` path: ${bin}` ); - process.exit(1); + process.exitCode = 1; + return; } // Neither: the launch itself failed. Report only what is actually known — // permissions, file format, CPU architecture and endpoint policy all land @@ -97,6 +130,6 @@ if (args[0] === "install") { ` path: ${bin}\n` + ` error: ${reason}` ); - process.exit(1); + process.exitCode = 1; } } diff --git a/scripts/run.test.js b/scripts/run.test.js index 4b44361056..60cba40e8a 100644 --- a/scripts/run.test.js +++ b/scripts/run.test.js @@ -6,7 +6,7 @@ const assert = require("node:assert/strict"); const fs = require("fs"); const os = require("os"); const path = require("path"); -const { spawnSync } = require("child_process"); +const { spawnSync, spawn, execSync } = require("child_process"); const MARKER = "failed to launch the native binary"; const IS_WINDOWS = process.platform === "win32"; @@ -15,6 +15,15 @@ const IS_ROOT = const BIN_NAME = "lark-cli" + (IS_WINDOWS ? ".exe" : ""); const SENTINEL = "--sentinel=LEAKCANARY"; const SENTINEL_TOKEN = "LEAKCANARY"; +// Single-line, easily-flippable platform gate for the win32 NTSTATUS crash +// test below (spec 6.2 row 1b). To confirm its assertions are real (not a +// vacuous skip), temporarily hardcode `true` here on a non-Windows host and +// run the suite: the test then executes for real, and it MUST fail, because +// run.js still checks the real process.platform (so its crash branch stays +// dark) and this OS truncates process.exit() codes to 8 bits before Node +// ever sees them (0xC0000005 -> 5). Restore this line to +// `IS_WINDOWS` before committing. +const IS_WIN32_CRASH_TESTABLE = IS_WINDOWS; // run.js resolves the native binary relative to its own directory // (/../bin/lark-cli), so a fixture needs a full scripts/ + bin/ layout. @@ -130,9 +139,11 @@ function assertLaunchFailure(res, sandbox) { ); // The sentinel check for launch-failure cases is not redundant with the // check in assertBinaryRan: when the launch fails, e.message contains only - // the errno, not the full argv. However, the error object also carries - // e.spawnargs and e.path, which hold the complete argument list even on - // launch failure. This assertion catches any future edit that prints + // the errno, not the full argv (measured: String(e) is + // "Error: spawnSync ENOENT" and e.path is just the bin path -- + // neither carries argv). However, the error object also carries + // e.spawnargs, which holds the complete argument list even on launch + // failure. This assertion catches any future edit that prints // e.spawnargs, String(e), or the whole error object. assert.ok( !res.stderr.includes(SENTINEL_TOKEN), @@ -150,16 +161,19 @@ describe("run.js launch-failure diagnostics", () => { // genuinely fails to start it: this really is a launch failure there. fs.writeFileSync(sandbox.bin, ""); } else { - // A 0-byte + executable file is NOT a launch failure on POSIX. libuv - // uses execvp, and POSIX execvp falls back to running the file through - // /bin/sh whenever execve returns ENOEXEC. /bin/sh then executes the - // empty file as an empty script and exits 0 -- execFileSync never - // throws, so this branch would never fire (measured as uid 0 in the - // Linux sandbox container: no throw, child exited 0). Do NOT - // "simplify" this back to a 0-byte file. A directory at the bin path - // passes the shim's existence check and then fails at exec with - // EACCES, and unlike chmod 0o000 that holds for uid 0 too, so this - // still proves the branch when CI runs as root. + // A 0-byte + executable file is NOT a launch failure on Linux/glibc. + // libuv uses execvp, and glibc's execvp falls back to running the file + // through /bin/sh whenever execve returns ENOEXEC. /bin/sh then + // executes the empty file as an empty script and exits 0 -- + // execFileSync never throws, so this branch would never fire (measured + // as uid 0 in the Linux sandbox container: no throw, child exited 0). + // This is NOT true of POSIX generally: measured on macOS (posix_spawn, + // no shell fallback), the identical 0-byte 0755 file makes + // execFileSync THROW ENOEXEC instead. Do NOT "simplify" this back to a + // 0-byte file. A directory at the bin path passes the shim's existence + // check and then fails at exec with EACCES on both platforms, and + // unlike chmod 0o000 that holds for uid 0 too, so this still proves the + // branch when CI runs as root. fs.mkdirSync(sandbox.bin); } @@ -284,6 +298,42 @@ describe("run.js pass-through when the binary runs", () => { } ); + it( + "reports a win32 NTSTATUS crash instead of forwarding the status silently", + { + skip: IS_WIN32_CRASH_TESTABLE + ? false + : "win32-only: POSIX truncates process.exit() codes to 8 bits " + + "(0xC0000005 -> 5), so the NTSTATUS crash range this branch " + + "detects cannot be reproduced outside win32 (spec 6.2 row 1b)", + }, + (t) => { + const sandbox = makeSandbox(t); + installRunnableBinary(sandbox); + + const res = runRanBinary(sandbox, "process.exit(0xC0000005)"); + + assert.equal(res.status, 1); + assert.equal(res.stdout, "", `stdout should stay empty: ${res.stdout}`); + assert.ok( + res.stderr.includes("crashed (status 0xc0000005"), + `missing win32 crash diagnostic: ${res.stderr}` + ); + assert.ok( + res.stderr.includes(sandbox.bin), + `diagnostic omits the binary path: ${res.stderr}` + ); + assert.ok( + !res.stderr.includes(MARKER), + `win32 crash was misreported as a launch failure: ${res.stderr}` + ); + assert.ok( + !res.stderr.includes(SENTINEL_TOKEN), + `caller arguments leaked into stderr: ${res.stderr}` + ); + } + ); + it("passes stdout and stderr through and exits 0", (t) => { const sandbox = makeSandbox(t); installRunnableBinary(sandbox); @@ -304,3 +354,138 @@ describe("run.js pass-through when the binary runs", () => { assertBinaryRan(res); }); }); + +// Regression for the flush-safety fix: `process.exitCode = 1` + natural +// return (not `process.exit(1)`) after the diagnostic console.error() calls. +// Writes to a piped stderr are asynchronous on POSIX; `process.exit()` can +// tear the process down before that write reaches the pipe. This is the +// exact scenario an AI agent / log wrapper hits: it captures the shim's +// stderr through a pipe, the child fills it, and the diagnostic must still +// arrive. +// +// To make this deterministic, the fixture is killed from OUTSIDE (by this +// test, via its discovered pid) while it is still blocked mid-write on a +// completely unread pipe, instead of self-signalling. Measured: if the +// fixture kills itself after its own blocking write loop returns, that +// return is proof room just opened up, so the diagnostic that follows +// almost always finds room too and the bug never reproduces -- on both +// Linux and macOS, self-signalling passed 100% regardless of +// process.exit(1) vs process.exitCode, i.e. it never actually exercised the +// backpressure path. An external kill has no such tell: the fixture can be +// stopped while genuinely full, so run.js's own diagnostic write is +// guaranteed to need the async path this fix protects. +// +// Linux-only: this exact harness reproduces the drop 100% reliably on Linux +// (matching the script-test CI job, ubuntu-latest, and the AI-agent/ +// container use case this fix targets). On macOS the underlying run.js fix +// is not in question -- a separate check confirmed the fixture genuinely +// blocks on a full, unread pipe through run.js's own execFileSync/"inherit" +// chain, same as Linux -- but child_process's "close" event was observed to +// fire as soon as the shim process exits, before this test's reader had +// drained (or in some runs, before it had even attached), independent of +// process.exit(1) vs process.exitCode. That is a difference in how Node's +// child_process "close" event interacts with unread pipe backlog once the +// writer has exited (epoll vs kqueue), not evidence about run.js's +// correctness, so it is not treated as a platform for this regression test. +describe("run.js flush safety under stderr backpressure", () => { + it( + "keeps the crash-signal diagnostic when stderr is a slow-draining pipe", + { + skip: + process.platform === "linux" + ? false + : "Linux-only: this harness's determinism relies on Linux's " + + "pipe/close semantics (see comment above); not evidence about " + + "run.js itself, which is exercised on this platform by the " + + "other POSIX signal tests in this file", + timeout: 15000, + }, + (t) => { + const sandbox = makeSandbox(t); + installRunnableBinary(sandbox); + + // Bigger than any realistic OS pipe buffer (measured ~256KB on the + // Linux CI container), so the fixture's blocking fs.writeSync loop + // genuinely fills the pipe and blocks in the kernel until a reader + // drains it -- real backpressure, not a simulation. It never gets to + // write all of this; it is killed mid-loop. + const fillBytes = 5000000; + const childCode = + "const fs = require('fs');" + + `const buf = Buffer.alloc(${fillBytes}, 0x78);` + + "let off = 0;" + + "while (off < buf.length) { off += fs.writeSync(2, buf, off, buf.length - off); }" + + "console.error('unreachable: should have been killed first');"; + + 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; + } + process.kill(fixturePid, "SIGKILL"); + + // Leave the pipe completely unread for a further stretch so that + // whatever run.js attempts to write next lands on a pipe that is + // still genuinely full, not one this test has started draining. + setTimeout(() => { + let stderr = ""; + shim.stderr.on("data", (chunk) => { + stderr += chunk; + }); + closed.then((code) => { + try { + assert.equal(code, 1); + assert.ok( + stderr.includes( + "the native binary was terminated by signal SIGKILL" + ), + `diagnostic dropped under stderr backpressure ` + + `(captured ${stderr.length} bytes): ` + + JSON.stringify(stderr.slice(-300)) + ); + assert.ok( + stderr.includes(sandbox.bin), + `diagnostic omits the binary path: ${stderr.slice(-300)}` + ); + assert.ok( + !stderr.includes(SENTINEL_TOKEN), + `caller arguments leaked into stderr under backpressure` + ); + resolve(); + } catch (err) { + reject(err); + } + }, reject); + }, 300); + }, 300); + }); + } + ); +});