fix: ngrok Windows build + close CI error-swallowing gap (v0.18.0.1)#1024
Merged
fix: ngrok Windows build + close CI error-swallowing gap (v0.18.0.1)#1024
Conversation
… Windows @ngrok/ngrok has a native .node addon that causes `bun build --outfile` to fail with "cannot write multiple output files without an output directory". Externalize it alongside the existing runtime deps (playwright, diff, bun:sqlite), matching the exact pattern used for every other dynamic import in server.ts. Adds a policy comment explaining when to extend the externals list so the next native dep doesn't repeat this failure. Two community contributors independently converged on this fix: - @tomasmontbrun-hash (#1019) - @scarson (#1013) Also fixes issues #1010 and #960. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…st failures
Shell operator precedence trap in both the build and test scripts:
cmd1 && cmd2 && ... && rm -f .*.bun-build || true
bun test ... && bun run slop:diff 2>/dev/null || true
The trailing `|| true` was intended to suppress cleanup errors, but it
applies to the entire `&&` chain — so ANY failure (including the
build-node-server.sh failure that broke Windows installs since v0.15.12)
silently exits 0. CI ran the build, the build failed, and CI reported green.
Wrap the cleanup/slop-diff commands in subshells so `|| true` only scopes to
the intended step:
... && (rm -f .*.bun-build || true)
bun test ... && (bun run slop:diff 2>/dev/null || true)
Verified: `bash -c 'false && echo A && rm -f X || true'` exits 0 (old,
broken), `bash -c 'false && echo A && (rm -f X || true)'` exits 1 (new,
correct).
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Two assertions: 1. `node --check` passes on the built `server-node.mjs` (valid ES module syntax). This catches regressions where the post-processing steps (perl regex replacements) corrupt the bundle. 2. No inlined `@ngrok/ngrok` module identifiers (ngrok_napi, platform- specific binding packages). Verifies the --external flag actually kept it external. Skips gracefully when `browse/dist/server-node.mjs` is missing — the dist dir is gitignored, so a fresh clone + `bun test` without a prior build is a valid state, not a failure. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Mirror the existing Playwright verification step. Since @ngrok/ngrok is
now externalized in server-node.mjs (resolved at runtime from node_modules),
confirm the platform-specific native binary (@ngrok/ngrok-win32-x64-msvc et
al.) is installed at setup time rather than surfacing the failure later
when the user runs /pair-agent.
Same fallback pattern: if `node -e "require('@ngrok/ngrok')"` fails, fall
back to `npm install --no-save @ngrok/ngrok` to pull the missing binary.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Fixes shipped in this version: - Externalize @ngrok/ngrok so the Node server bundle builds on Windows (PRs #1019, #1013; issues #1010, #960) - Shell precedence fix so build/test failures no longer exit 0 in CI - Build validation test for server-node.mjs - Windows setup verifies @ngrok/ngrok native binary is loadable Credit: @tomasmontbrun-hash (#1019), @scarson (#1013). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This was referenced Apr 16, 2026
E2E Evals: ✅ PASS0/0 tests passed | $0 total cost | 12 parallel runners
12x ubicloud-standard-2 (Docker: pre-baked toolchain + deps) | wall clock ≈ slowest suite |
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
Fixes a bug where
./setupon Windows (and fresh Linux installs) has been failing silently since v0.15.12 with:Root cause:
@ngrok/ngrokhas a native.nodeaddon that makes Bun want to emit a separate chunk, which conflicts with--outfile. Fix: externalize it like we already do forplaywright,diff, andbun:sqlite.Then this PR keeps going. The reason the bug shipped in the first place is that
package.json'sbuildandtestscripts had a shell precedence trap: the trailing|| trueapplied to the entire&&chain, so CI happily reported green while the build failed. Fixed that too.Credit
Two community contributors independently tracked this down and proposed the identical one-line fix:
Also closes #1010 and #960.
Thanks for the sharp diagnoses. The externalize fix is exactly what both PRs proposed — I'm landing it alongside the CI-masking fixes that let this slip through in the first place.
What's in this PR
browse/scripts/build-node-server.sh— externalize@ngrok/ngrok, plus a policy comment explaining when to add future externals.package.json— wrap trailing cleanup in subshells so|| trueonly scopes to cleanup, not the whole build/test chain.browse/test/build.test.ts— new regression test. Validatesserver-node.mjspassesnode --checkand confirms@ngrok/ngrokactually stayed external (not inlined). Skips gracefully whenbrowse/dist/is absent.setup— Windows now verifies Node can load@ngrok/ngrokat install time (mirroring the existing Playwright check), so the failure surfaces at./setupinstead of at/pair-agenttime.Test plan
bash browse/scripts/build-node-server.sh— emits singleserver-node.mjs, 0.33 MBnode --check browse/dist/server-node.mjs— passesnode -e "require('@ngrok/ngrok')"— loads cleanly on darwin-arm64bun test browse/test/build.test.ts— 2 passbun test— full free-tier suite passescmd && rm || trueexits 0 when cmd fails; newcmd && (rm || true)exits 1 (precedence fix verified)Reviews
/plan-ceo-review— SELECTIVE EXPANSION mode, 6 proposals, 5 accepted, 0 deferred. CLEAR./codex review(outside voice) — 3 substantive findings, all accepted into scope (same|| truebug in test script, graceful-skip in test, policy comment)./plan-eng-review— 1 issue (Windows setup check for ngrok), accepted. 0 critical gaps. CLEAR.🤖 Generated with Claude Code