Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions src/utils/CommandExecutor.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,14 @@ export interface CommandExecOptions {
cwd?: string;
onStdout?: (chunk: string) => void;
onStderr?: (chunk: string) => void;
/**
* Maximum number of bytes to accumulate per stream (stdout/stderr) before
* truncating. Prevents `RangeError: Invalid string length` when very large
* outputs (e.g. verbose xcodebuild logs) exceed V8's maximum string length
* (~512MB on 64-bit). Defaults to XCODEBUILDMCP_MAX_OUTPUT_BYTES env var or
* 64 MiB.
*/
maxOutputBytes?: number;
}

/**
Expand Down
136 changes: 115 additions & 21 deletions src/utils/command.ts
Original file line number Diff line number Diff line change
Expand Up @@ -69,8 +69,70 @@

const childProcess = spawn(executable, args, spawnOpts);

let stdout = '';
let stderr = '';
// Accumulate child process output as raw Buffers (not concatenated strings)
// so we never trip V8's max-string-length limit (~512MB on 64-bit), which
// previously surfaced as an uncaught `RangeError: Invalid string length`
// thrown synchronously from the 'data' handler when xcodebuild emitted
// very large verbose logs.

Check warning on line 76 in src/utils/command.ts

View workflow job for this annotation

GitHub Actions / warden: code-review

[UJ6-YUH] No tests for the new buffer accumulation, capping, and truncation logic (additional location)

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.
const DEFAULT_MAX_OUTPUT_BYTES = 64 * 1024 * 1024; // 64 MiB per stream
const envCap = Number.parseInt(process.env.XCODEBUILDMCP_MAX_OUTPUT_BYTES ?? '', 10);

Check warning on line 78 in src/utils/command.ts

View check run for this annotation

@sentry/warden / warden: code-review

No test coverage for new output truncation and buffer accumulation logic

The new `appendChunk`, `finalizeStream`, and `maxOutputBytes` logic — including `XCODEBUILDMCP_MAX_OUTPUT_BYTES` env parsing — has no unit tests in `src/utils/__tests__/command.test.ts`, leaving the critical crash-prevention path unverified.
const maxOutputBytes =
opts?.maxOutputBytes ??
(Number.isFinite(envCap) && envCap > 0 ? envCap : DEFAULT_MAX_OUTPUT_BYTES);

const stdoutChunks: Buffer[] = [];
const stderrChunks: Buffer[] = [];
let stdoutBytes = 0;
let stderrBytes = 0;
let stdoutTruncated = false;
let stderrTruncated = false;

const appendChunk = (
chunks: Buffer[],
currentBytes: number,
truncated: boolean,
data: Buffer,
): { bytes: number; truncated: boolean } => {
if (truncated) {
return { bytes: currentBytes, truncated: true };
}
const remaining = maxOutputBytes - currentBytes;
if (data.byteLength <= remaining) {
chunks.push(data);
return { bytes: currentBytes + data.byteLength, truncated: false };
}
if (remaining > 0) {
chunks.push(data.subarray(0, remaining));
}
return { bytes: maxOutputBytes, truncated: true };
};

const finalizeStream = (
chunks: Buffer[],
totalBytes: number,
truncated: boolean,
): string => {
try {
const text = Buffer.concat(chunks, totalBytes).toString('utf8');
return truncated
? `${text}\n[output truncated after ${totalBytes} bytes]`
: text;
} catch (err) {
// Defensive: if the concatenated string still somehow exceeds V8's
// string limit, fall back to a heavily truncated slice rather than
// crashing the MCP process.
log(
'error',
`Failed to finalize captured output (${totalBytes} bytes): ${(err as Error).message}`,
);
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.

return `${safeText}
[output truncated after ${safeSlice} bytes due to size]`;
}
};

const streamClosers: Array<() => void> = [];
const streamDetachers: Array<() => void> = [];
Expand Down Expand Up @@ -114,6 +176,8 @@
detachStreamListeners();

const success = code === 0;
const stdout = finalizeStream(stdoutChunks, stdoutBytes, stdoutTruncated);
const stderr = finalizeStream(stderrChunks, stderrBytes, stderrTruncated);
const response: CommandResponse = {
success,
output: stdout,
Expand Down Expand Up @@ -143,7 +207,7 @@

const attachStream = (
stream: NodeJS.ReadableStream | null | undefined,
onChunk: (chunk: string) => void,
onChunk: (chunk: Buffer) => void,
): void => {
if (!stream) {
return;
Expand All @@ -162,11 +226,19 @@
};

const handleData = (data: Buffer | string): void => {
if (settled) {

Check warning on line 229 in src/utils/command.ts

View workflow job for this annotation

GitHub Actions / warden: code-review

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.

Check warning on line 229 in src/utils/command.ts

View check run for this annotation

@sentry/warden / warden: code-review

[JDM-NLA] No test coverage for new output truncation and buffer accumulation logic (additional location)

The new `appendChunk`, `finalizeStream`, and `maxOutputBytes` logic — including `XCODEBUILDMCP_MAX_OUTPUT_BYTES` env parsing — has no unit tests in `src/utils/__tests__/command.test.ts`, leaving the critical crash-prevention path unverified.
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

return;
}
const chunk = data.toString();
onChunk(chunk);
try {
const buf = Buffer.isBuffer(data) ? data : Buffer.from(data);
onChunk(buf);
} catch (err) {
// Any failure inside the data handler (including a future
// RangeError) must reject the promise rather than escape as an
// uncaught exception (which previously crashed the MCP process via
// mechanism=auto.node.onuncaughtexception).
handleError(err as Error);
}
};

stream.on('data', handleData);
Expand Down Expand Up @@ -212,25 +284,47 @@
}

attachStream(childProcess.stdout, (chunk) => {
stdout += chunk;
opts?.onStdout?.(chunk);
emitTranscript?.({
kind: 'transcript',
fragment: 'process-line',
stream: 'stdout',
line: chunk,
});
const result = appendChunk(stdoutChunks, stdoutBytes, stdoutTruncated, chunk);
stdoutBytes = result.bytes;
if (!stdoutTruncated && result.truncated) {
log(
'warning',
`stdout exceeded maxOutputBytes (${maxOutputBytes}); truncating further output`,
);
}
stdoutTruncated = result.truncated;
if (opts?.onStdout || emitTranscript) {
const text = chunk.toString('utf8');
opts?.onStdout?.(text);
emitTranscript?.({
kind: 'transcript',
fragment: 'process-line',
stream: 'stdout',
line: text,
});
}
});

attachStream(childProcess.stderr, (chunk) => {
stderr += chunk;
opts?.onStderr?.(chunk);
emitTranscript?.({
kind: 'transcript',
fragment: 'process-line',
stream: 'stderr',
line: chunk,
});
const result = appendChunk(stderrChunks, stderrBytes, stderrTruncated, chunk);
stderrBytes = result.bytes;
if (!stderrTruncated && result.truncated) {
log(
'warning',
`stderr exceeded maxOutputBytes (${maxOutputBytes}); truncating further output`,
);
}
stderrTruncated = result.truncated;
if (opts?.onStderr || emitTranscript) {
const text = chunk.toString('utf8');
opts?.onStderr?.(text);
emitTranscript?.({
kind: 'transcript',
fragment: 'process-line',
stream: 'stderr',
line: text,
});
}
});

childProcess.once('error', handleError);
Expand Down
Loading