Skip to content

fix(utils): Prevent RangeError from large command output#433

Open
sentry[bot] wants to merge 1 commit into
mainfrom
seer/fix/command-output-range-error
Open

fix(utils): Prevent RangeError from large command output#433
sentry[bot] wants to merge 1 commit into
mainfrom
seer/fix/command-output-range-error

Conversation

@sentry
Copy link
Copy Markdown

@sentry sentry Bot commented May 29, 2026

This PR addresses the RangeError: Invalid string length issue (XCODEBUILDMCP-E) that caused the MCP process to crash when xcodebuild or other commands produced very large outputs.

Root Cause:
The runCommand helper in src/utils/command.ts accumulated child process stdout and stderr by concatenating chunks into a single JavaScript string (stdout += data.toString()). When the output size exceeded V8's maximum string length (~512MB on 64-bit systems), a RangeError: Invalid string length was thrown synchronously inside the data handler, leading to an uncaught exception and crashing the MCP server.

Solution:

  1. Buffer-based Accumulation: Replaced string concatenation for stdout and stderr with accumulation into Buffer[] arrays. This avoids hitting V8's string length limit during data streaming.
  2. Configurable Output Cap: Introduced a maxOutputBytes option (defaulting to 64 MiB, configurable via XCODEBUILDMCP_MAX_OUTPUT_BYTES environment variable or CommandExecOptions). Output exceeding this limit is truncated, and a truncation marker is appended.
  3. Safe String Finalization: The final output and error strings are materialized from the accumulated Buffer[] arrays using Buffer.concat().toString('utf8') only when the command settles. A defensive try/catch is in place during finalization to handle extreme edge cases, falling back to a smaller slice if concatenation still fails.
  4. Error Handling: The data handler now includes a try/catch block. Any error (including potential future RangeErrors) within the handler will now reject the command promise via handleError instead of causing an uncaught exception and crashing the process.
  5. CommandExecOptions Update: Added maxOutputBytes to CommandExecOptions for explicit control over the output limit.

Fixes XCODEBUILDMCP-E

Copy link
Copy Markdown

@github-actions github-actions Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No tests cover the new output-truncation / maxOutputBytes behaviour

The core fix introduced by this PR (buffer accumulation, truncation cap, appendChunk, finalizeStream, XCODEBUILDMCP_MAX_OUTPUT_BYTES) has no corresponding tests in src/utils/__tests__/command.test.ts, leaving the crash-prevention logic unverified.

Evidence
  • src/utils/__tests__/command.test.ts contains only two tests (delayed close settlement and detached listener count) — neither exercises large output, the cap, or truncation.
  • grep for maxOutputBytes, truncat, appendChunk, RangeError in that file returns no matches.
  • The PR description identifies a specific crash scenario (RangeError: Invalid string length) that is the sole motivation for the change, but no regression test validates it.

Identified by Warden code-review

Comment thread src/utils/command.ts
@@ -165,8 +229,16 @@ async function defaultExecutor(
if (settled) {
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No tests for the new buffer accumulation, capping, and truncation logic

The new appendChunk / finalizeStream functions and the maxOutputBytes cap are entirely untested — src/utils/__tests__/command.test.ts has no test that exercises large output, truncation, the [output truncated after N bytes] message, or the XCODEBUILDMCP_MAX_OUTPUT_BYTES env-var path. Consider adding unit tests that verify: (1) output below the cap is returned verbatim, (2) output at or above the cap is truncated with the expected message, and (3) the env-var override is respected.

Evidence
  • src/utils/__tests__/command.test.ts contains only two integration-style tests (settles after exit and does not attach stdout/stderr listeners for detached commands), neither of which sets maxOutputBytes or generates output large enough to trigger truncation.
  • appendChunk (the byte-cap logic) and finalizeStream (the concatenation + truncation-message logic) are not called or validated by any test in the suite.
  • A grep across all *.test.ts files for maxOutputBytes, appendChunk, finalizeStream, and truncat (in the context of command output) returns zero matches.
  • The XCODEBUILDMCP_MAX_OUTPUT_BYTES env-var branch inside defaultExecutor has no test path covering it.
Also found at 1 additional location
  • src/utils/command.ts:76

Identified by Warden code-review · UJ6-YUH

@cameroncooke cameroncooke marked this pull request as ready for review May 29, 2026 21:53
Copy link
Copy Markdown
Contributor

@cursor cursor Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cursor Bugbot has reviewed your changes and found 1 potential issue.

Fix All in Cursor

❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.

Reviewed by Cursor Bugbot for commit 6155c40. Configure here.

Comment thread src/utils/command.ts
const safeSlice = Math.min(totalBytes, 1 * 1024 * 1024);
const safeText = Buffer.concat(chunks, totalBytes)
.subarray(0, safeSlice)
.toString('utf8');
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Catch block in finalizeStream can itself throw unhandled

Medium Severity

The defensive catch block in finalizeStream repeats Buffer.concat(chunks, totalBytes) without its own try/catch. If the original failure was Buffer.concat itself throwing (e.g., ERR_MEMORY_ALLOCATION_FAILED when maxOutputBytes is configured very large via the env var), this second call will also throw. Since finalizeStream is called from settle(), which is invoked from event handlers and setTimeout callbacks, the unhandled throw becomes an uncaught exception — the exact crash scenario this PR aims to prevent.

Additional Locations (1)
Fix in Cursor Fix in Web

Reviewed by Cursor Bugbot for commit 6155c40. Configure here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

0 participants