fix(factory): route JSON validation error through commander writeErr on Node.js 22#444
Open
levontumanyan wants to merge 2 commits into
Open
Conversation
The test for JSON-format input validation errors was failing on Node.js 22 after the commander v14 → v15 upgrade (elastic#410). Commander 15 is ESM-only, and when loaded via dynamic import() from a CommonJS context (tsx-compiled tests), patching process.stderr.write globally becomes unreliable on Node.js 22 due to CJS/ESM boundary handling. The second test in the suite was consistently getting null for its captured output. Fix: use cmd.configureOutput().writeErr() instead of process.stderr.write() directly, which routes through commander's own output configuration layer — the same channel the test already captures via cmd.configureOutput({ writeErr }). Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
The test for JSON-format input validation errors was failing on Node.js 22 (Linux) after the commander v14 → v15 upgrade (elastic#410). Commander 15 is ESM-only, and on Linux Node 22.22.x the combination of global process.stderr.write patching in the test helper with tsx/esm caused the captured output to be unreliable — specifically the second test in the suite got an empty or non-JSON buffer, making parsed come back null. Two-part fix: 1. factory.ts: route the JSON error through cmd.configureOutput().writeErr() instead of process.stderr.write() directly, respecting commander's own output configuration layer. 2. test: remove global process.stdout/stderr.write patching from invokeWithJsonFormat and rely solely on configureOutput — which is already set up to capture output. This eliminates the global-state mutation that was the root cause of the Linux-specific failure. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
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
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.
What
Routes the JSON-format input validation error through commander's configured
writeErrinstead of writing directly toprocess.stderr.write.Why this broke
PR #410 upgraded commander v14 → v15. Commander 15 is ESM-only. The test suite runs via tsx, which compiles TypeScript to CommonJS, so commander is loaded via a dynamic
await import('commander')— a CJS→ESM boundary.On Node.js 22, this boundary causes global stream patches (
process.stderr.write = ...) to become unreliable across calls: the second test in theschema input - JSON format error outputsuite consistently got an emptyoutbuffer (→parsed = null→TypeError: Cannot read properties of null), while the first test passed fine. Node.js 24/25 handle the CJS/ESM interop differently and don't exhibit this.This is what was blocking the release PR #395 from merging.
The fix
cmd.configureOutput().writeErris the public commander API for error output — it's already what the test captures viacmd.configureOutput({ writeErr: (s) => { out += s } }). Routing through it avoids the global patch entirely and is consistent with how commander itself handles all other error output internally.Scope — follow-up needed?
This fixes only the one failing test (
factory.ts:574— input validation error in--jsonmode). There are ~10 otherprocess.stdout/stderr.writecalls infactory.tsinside action handlers (lines 516, 589, 591, 601, 603, 615, 619, 623, 631, 633, 635) that use the same pattern. They aren't currently causing CI failures — either because their tests run first in their scope, capture stdout only, or useconfigureOutputdirectly. But they carry the same theoretical risk on Node.js 22.Reviewer: is a follow-up PR to migrate those remaining calls worth doing for consistency and future-proofing, or is fixing just the failing test sufficient?