fix(test): bump runCli maxBuffer to fit growing manifest#1842
Open
Benjamin-eecs wants to merge 2 commits into
Open
fix(test): bump runCli maxBuffer to fit growing manifest#1842Benjamin-eecs wants to merge 2 commits into
Benjamin-eecs wants to merge 2 commits into
Conversation
opencli list -f json crossed 1 MB at 1030 entries on v1.8.2 so the execFile default overflows and the helper returns code ERR_CHILD_PROCESS_STDIO_MAXBUFFER instead of the real exit code. Closes jackwener#1841
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
This PR updates the E2E CLI helper to handle large stdout payloads from opencli commands by increasing the child process maxBuffer and making it configurable.
Changes:
- Extend
runClioptions to acceptmaxBuffer. - Set a higher default
maxBuffer(16 MiB) to preventERR_CHILD_PROCESS_STDIO_MAXBUFFER. - Pass
maxBufferthrough to the underlyingexeccall.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+31
to
+34
| // `opencli list -f json` already weighs ~1 MB at 1030 entries on v1.8.2 and | ||
| // grows with every new adapter. The execFile default maxBuffer is 1 MB; past | ||
| // it stdout overflows and the helper returns code | ||
| // 'ERR_CHILD_PROCESS_STDIO_MAXBUFFER' instead of the actual exit code. |
| export async function runCli( | ||
| args: string[], | ||
| opts: { timeout?: number; env?: Record<string, string> } = {}, | ||
| opts: { timeout?: number; env?: Record<string, string>; maxBuffer?: number } = {}, |
| // grows with every new adapter. The execFile default maxBuffer is 1 MB; past | ||
| // it stdout overflows and the helper returns code | ||
| // 'ERR_CHILD_PROCESS_STDIO_MAXBUFFER' instead of the actual exit code. | ||
| const maxBuffer = opts.maxBuffer ?? 16 * 1024 * 1024; |
Pulls the runCli stdout cap out of the function body so future tuning lands in one place.
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.
Description
tests/e2e/helpers.tsusespromisify(execFile)without passingmaxBuffer, so it inherits Node's 1 MB default. On v1.8.2node dist/src/main.js list -f json | wc -cmeasures 1,067,516 bytes, which is just past the default.execFilerejects withcode: 'ERR_CHILD_PROCESS_STDIO_MAXBUFFER', the catch block returns that string as the resultcode, and the E2E assertionsexpect(code).toBe(0)fail with the literal'ERR_CHILD_PROCESS_STDIO_MAXBUFFER'. The failure repros on both ubuntu and macos in theE2E Headed Chromeworkflow onec3eddec chore(release): 1.8.2 (#1830), and affectstests/e2e/management.test.ts > list shows all registered commandsplustests/e2e/output-formats.test.ts > list -f json produces valid output. yaml / csv / md formats stayed under 1 MB so they passed, but the underlying helper has no headroom for any future adapter additions in any format.Pass an explicit
maxBufferof 16 MB toexecFile, with an optionalopts.maxBufferoverride for callers that need a different bound. 16 MB gives roughly 15x headroom over the current manifest size and matches the order-of-magnitude that other Node CLIs default to when wrapping their own subprocesses. The default behavior of every existingrunClicaller is preserved; only the silent overflow ceiling moves.Related issue: closes #1841.
Type of Change
Checklist
Screenshots / Output
Pre-fix repro on
upstream/mainv1.8.2:Post-fix on this branch:
Full local check:
npx tsc --noEmitclean.tests/e2e/management.test.ts+tests/e2e/output-formats.test.ts16/16.cli-manifest.jsonuntouched.