Conversation
Switched command execution from cmd.Output() buffering to pipe-based streaming decode with json.Decoder, reducing peak memory usage during large scans. Added bounded stderr tail buffering and updated tests to cover streaming execution and buffer truncation behavior.
Not up to standards ⛔🔴 Issues
|
| Category | Results |
|---|---|
| Complexity | 1 medium |
AI Reviewer: first review requested successfully. AI can make mistakes. Always validate suggestions.
TIP This summary will be updated as you push new changes.
There was a problem hiding this comment.
Code Review
This pull request refactors command execution to stream and parse output directly from stdout, reducing memory usage, and introduces a size-limited buffer for stderr. The review feedback highlights three key improvements: checking the parsing error before the stderr copy error to avoid masking the root cause, copying bytes to a temporary slice in limitedBuffer.Write to avoid fragile overlapping writes, and adding a defensive nil check in isBenignStreamClose to prevent potential nil pointer dereferences.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| if stderrCopyErr != nil && !isBenignStreamClose(stderrCopyErr) { | ||
| return nil, stderrTail.String(), stderrCopyErr | ||
| } | ||
| if parseErr != nil { | ||
| return nil, stderrTail.String(), parseErr | ||
| } | ||
| cmdOutputString := string(cmdOutput) | ||
| return &cmdOutputString, nil, nil | ||
| if waitErr != nil { | ||
| return nil, stderrTail.String(), waitErr | ||
| } |
There was a problem hiding this comment.
Checking stderrCopyErr before parseErr can mask the actual parsing error. If parseCommandOutput fails (e.g., due to invalid JSON), the process is killed, which often causes io.Copy on the stderr pipe to fail with a non-benign error (like a closed pipe). If this happens, the secondary stderrCopyErr will be returned instead of the root cause parseErr, making debugging extremely difficult. We should check and return parseErr first.
| if stderrCopyErr != nil && !isBenignStreamClose(stderrCopyErr) { | |
| return nil, stderrTail.String(), stderrCopyErr | |
| } | |
| if parseErr != nil { | |
| return nil, stderrTail.String(), parseErr | |
| } | |
| cmdOutputString := string(cmdOutput) | |
| return &cmdOutputString, nil, nil | |
| if waitErr != nil { | |
| return nil, stderrTail.String(), waitErr | |
| } | |
| if parseErr != nil { | |
| return nil, stderrTail.String(), parseErr | |
| } | |
| if stderrCopyErr != nil && !isBenignStreamClose(stderrCopyErr) { | |
| return nil, stderrTail.String(), stderrCopyErr | |
| } | |
| if waitErr != nil { | |
| return nil, stderrTail.String(), waitErr | |
| } |
| if l.buf.Len()+len(p) > l.max { | ||
| drop := l.buf.Len() + len(p) - l.max | ||
| current := l.buf.Bytes() | ||
| l.buf.Reset() | ||
| _, _ = l.buf.Write(current[drop:]) | ||
| } |
There was a problem hiding this comment.
The current implementation of limitedBuffer.Write performs an overlapping write on the same underlying array of bytes.Buffer after calling Reset(). While Go's copy is overlap-safe, relying on this behavior alongside bytes.Buffer's internal implementation details (e.g., that Reset does not reallocate or clear the slice) is fragile and non-idiomatic. Copying the remaining bytes to a temporary slice first is much safer and more maintainable.
| if l.buf.Len()+len(p) > l.max { | |
| drop := l.buf.Len() + len(p) - l.max | |
| current := l.buf.Bytes() | |
| l.buf.Reset() | |
| _, _ = l.buf.Write(current[drop:]) | |
| } | |
| if l.buf.Len()+len(p) > l.max { | |
| drop := l.buf.Len() + len(p) - l.max | |
| temp := make([]byte, l.buf.Len()-drop) | |
| copy(temp, l.buf.Bytes()[drop:]) | |
| l.buf.Reset() | |
| _, _ = l.buf.Write(temp) | |
| } |
| func isBenignStreamClose(err error) bool { | ||
| return errors.Is(err, io.EOF) || errors.Is(err, os.ErrClosed) || strings.Contains(err.Error(), "file already closed") | ||
| } |
There was a problem hiding this comment.
If err is nil, calling err.Error() will cause a nil pointer dereference panic. Although the current call sites guard against nil, adding a defensive nil check at the beginning of isBenignStreamClose makes this helper function robust and safe for future reuse.
func isBenignStreamClose(err error) bool {
if err == nil {
return false
}
return errors.Is(err, io.EOF) || errors.Is(err, os.ErrClosed) || strings.Contains(err.Error(), "file already closed")
}
Switched command execution from cmd.Output() buffering to pipe-based streaming decode with json.Decoder, reducing peak memory usage during large scans. Added bounded stderr tail buffering and updated tests to cover streaming execution and buffer truncation behavior.