fix(cli): stall on some forge cli builds

[dbeal-eth]

When forge outputs large amounts of data (verbose builds, many warnings),
the child process buffers would fill causing the process to block.

This fix actively consumes stdout/stderr streams and displays them
on build failure to improve debuggability.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

[claude]

PR Review: Fix for Forge Build Process Stalling

Thank you for addressing this important issue! This PR fixes a critical bug where the forge build process could stall when producing large amounts of output. Here is my detailed review:

---

Strengths

1. Root Cause Properly Identified: The fix correctly addresses the buffer filling issue by consuming stdout/stderr streams, which prevents the child process from blocking.

2. Good Test Coverage: The addition of an e2e test (build-foundry-compile-error.sh) is excellent - it verifies that compilation errors are properly captured and displayed.

3. Improved Error Visibility: On build failure, users now see both stdout and stderr from forge, making debugging much easier.

4. Code Formatting Improvements: The reformatting of several functions (diff, alter, fetch, etc.) improves readability.

---

Critical Bug Found

Location: packages/cli/src/index.ts:221 and packages/cli/src/index.ts:224

The current code has a critical bug:

• Array.slice() returns a NEW array without modifying the original
• This means the data is consumed (preventing the stall) but NEVER stored in the arrays
• On error, forgeStdout.join() and forgeStderr.join() will always be EMPTY STRINGS

Fix Required:
Replace slice(-999).push() with proper array mutation:

• Use forgeStdout.push() directly
• Then use if (forgeStdout.length > 1000) forgeStdout.shift() to limit size

---

Other Issues

1. Test Script Uses Deprecated Syntax - The test uses [contract.Broken] but should use [deploy.Broken] per CLAUDE.md

2. Trace Test Update - The test expectation now includes value display. This appears unrelated to the main fix and should be mentioned in the PR description.

---

Performance Considerations

After fixing the array mutation bug, consider limiting by total BYTES instead of CHUNKS, since each chunk could be large. Suggest using a 1MB limit and tracking cumulative size.

---

Security Considerations

No security concerns identified. The code properly sanitizes output and limits buffer size.

---

Recommendations

1. MUST FIX: Replace .slice(-999).push() with proper array operations
2. SHOULD FIX: Update test to use modern [deploy.] syntax
3. CONSIDER: Implement byte-based buffer limiting instead of chunk-based
4. CONSIDER: Add JSDoc comments explaining the buffer consumption strategy

---

Summary

This PR solves a real problem but has a critical bug that prevents the error output from being captured and displayed. Once the array mutation issue is fixed, this will be a solid improvement to the developer experience.

Recommendation: Request changes to fix the array.slice() bug before merging.

Great work identifying and tackling this tricky issue with child process buffers!

[dbeal-eth]

merging because positive feedback from client, good AI review