capture go build / go mod tidy output to outputcapture #2540
Conversation
WalkthroughThe pull request adds line-oriented streaming utilities and integrates them into build output capture. A new LineWriter (io.Writer) is added in both pkg/util/utilfn and tsunami/util to buffer bytes and invoke a per-line callback, with Flush support and a 128 KiB max-line behavior. tsunami/util also receives StreamToLines, StreamToLinesChan, LineOutput, and ReadLineWithTimeout. tsunami/build/build.go is updated to embed and use LineWriter inside OutputCapture so build/go mod subprocess stdout/stderr can be captured per-line. A golangci.yml lint exclusion change is included. Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
pkg/util/utilfn/streamtolines.go(1 hunks)tsunami/build/build.go(3 hunks)tsunami/util/streamtolines.go(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
tsunami/build/build.go (1)
tsunami/util/streamtolines.go (2)
LineWriter(89-92)NewLineWriter(95-99)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Build for TestDriver.ai
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Analyze (go)
| func (lw *LineWriter) Write(p []byte) (n int, err error) { | ||
| streamToLines_processBuf(&lw.lineBuf, p, lw.lineFn) | ||
| return len(p), nil | ||
| } | ||
|
|
||
| // Flush outputs any remaining buffered data as a final line. | ||
| // Should be called when the input stream is complete (e.g., at EOF). | ||
| func (lw *LineWriter) Flush() { | ||
| if len(lw.lineBuf.buf) > 0 && !lw.lineBuf.inLongLine { | ||
| lw.lineFn(lw.lineBuf.buf) | ||
| lw.lineBuf.buf = nil | ||
| } |
There was a problem hiding this comment.
Guard utilfn.LineWriter with a mutex.
This copy of LineWriter is hit by the same concurrent Stdout/Stderr writes. Without locking, shared lineBuf is racy and corrupts captured lines. Please add the same synchronization as suggested in the util package version so both stay in sync.
Apply the identical mutex changes shown on the util version (import sync, add a mutex field, and lock in Write/Flush).
🤖 Prompt for AI Agents
In pkg/util/utilfn/streamtolines.go around lines 102 to 113, the LineWriter is
not concurrency-safe: add synchronization matching the util package variant by
importing "sync", adding a mutex field (e.g., mu sync.Mutex) to the LineWriter
struct, and surrounding the body of Write and Flush with mu.Lock()/defer
mu.Unlock() so accesses to lw.lineBuf are serialized; ensure the Mutex is used
in both methods and no other behavior changes are introduced.
| func (oc *OutputCapture) Write(p []byte) (n int, err error) { | ||
| if oc == nil { | ||
| return os.Stdout.Write(p) | ||
| } | ||
| return oc.lineWriter.Write(p) | ||
| } | ||
|
|
||
| func (oc *OutputCapture) Flush() { | ||
| if oc == nil || oc.lineWriter == nil { | ||
| return | ||
| } | ||
| oc.lineWriter.Flush() | ||
| } |
There was a problem hiding this comment.
Preserve zero-value usability of OutputCapture.
OutputCapture is exported and previously worked when callers used a zero value (&build.OutputCapture{} or an embedded struct). After this change, lineWriter stays nil and Write now panics when the build code routes stdout/stderr through it. Please keep zero-value usage safe—e.g., lazily initialize lineWriter inside Write/Flush (with the same callback) or move the initialization into a private helper invoked whenever needed.
One option:
func (oc *OutputCapture) Write(p []byte) (n int, err error) {
if oc == nil {
return os.Stdout.Write(p)
}
+ oc.ensureLineWriter()
return oc.lineWriter.Write(p)
}
func (oc *OutputCapture) Flush() {
- if oc == nil || oc.lineWriter == nil {
+ if oc == nil {
return
}
+ oc.ensureLineWriter()
oc.lineWriter.Flush()
}
+
+func (oc *OutputCapture) ensureLineWriter() {
+ if oc.lineWriter != nil {
+ return
+ }
+ if oc.lines == nil {
+ oc.lines = make([]string, 0)
+ }
+ oc.lineWriter = util.NewLineWriter(func(line []byte) {
+ oc.lock.Lock()
+ defer oc.lock.Unlock()
+ oc.lines = append(oc.lines, string(line))
+ })
+}Make sure to set up the callback exactly once so existing constructors remain compatible.
| func (lw *LineWriter) Write(p []byte) (n int, err error) { | ||
| streamToLines_processBuf(&lw.lineBuf, p, lw.lineFn) | ||
| return len(p), nil | ||
| } | ||
|
|
||
| // Flush outputs any remaining buffered data as a final line. | ||
| // Should be called when the input stream is complete (e.g., at EOF). | ||
| func (lw *LineWriter) Flush() { | ||
| if len(lw.lineBuf.buf) > 0 && !lw.lineBuf.inLongLine { | ||
| lw.lineFn(lw.lineBuf.buf) | ||
| lw.lineBuf.buf = nil | ||
| } |
There was a problem hiding this comment.
LineWriter must be concurrency-safe.
OutputCapture wires both Stdout and Stderr of exec.Cmd to the same LineWriter. Those pipes are copied in parallel goroutines, so Write and Flush can run concurrently. Because lineBuf is mutated without synchronization, we’ll hit races and corrupt buffered data (lines interleaving or panics). Protect the state with a mutex (and mirror that in Flush) before wiring this through build output.
Apply this diff as a starting point:
@@
-import (
- "bytes"
- "context"
- "io"
- "time"
-)
+import (
+ "bytes"
+ "context"
+ "io"
+ "sync"
+ "time"
+)
@@
type LineWriter struct {
+ mu sync.Mutex
lineBuf lineBuf
lineFn func([]byte)
}
@@
func (lw *LineWriter) Write(p []byte) (n int, err error) {
+ lw.mu.Lock()
+ defer lw.mu.Unlock()
streamToLines_processBuf(&lw.lineBuf, p, lw.lineFn)
return len(p), nil
}
@@
func (lw *LineWriter) Flush() {
+ lw.mu.Lock()
+ defer lw.mu.Unlock()
if len(lw.lineBuf.buf) > 0 && !lw.lineBuf.inLongLine {
lw.lineFn(lw.lineBuf.buf)
lw.lineBuf.buf = nil
}
}Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In tsunami/util/streamtolines.go around lines 102 to 113, the LineWriter is not
concurrency-safe: its Write and Flush mutate lw.lineBuf concurrently when stdout
and stderr pipes are copied in parallel. Add a sync.Mutex field to LineWriter
and use it to guard all mutations/read of lw.lineBuf in Write and Flush (lock at
start, defer unlock) so buffered data and inLongLine are accessed atomically;
ensure any early returns still unlock via defer and that Flush mirrors the same
locking to prevent races and corruption.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
tsunami/build/build.go (3)
49-56: Critical: Zero-valueOutputCapturewill panic inWritemethod.The previous review comment about preserving zero-value usability has not been addressed. If a caller creates
&OutputCapture{}(without usingMakeOutputCapture()),lineWriterwill be nil, and line 55 will panic when dereferencing it.Apply lazy initialization as suggested in the previous review:
func (oc *OutputCapture) Write(p []byte) (n int, err error) { if oc == nil { return os.Stdout.Write(p) } + oc.ensureLineWriter() oc.lock.Lock() defer oc.lock.Unlock() return oc.lineWriter.Write(p) }Add the helper method:
func (oc *OutputCapture) ensureLineWriter() { if oc.lineWriter != nil { return } if oc.lines == nil { oc.lines = make([]string, 0) } oc.lineWriter = util.NewLineWriter(func(line []byte) { oc.lines = append(oc.lines, string(line)) }) }
58-65: UpdateFlushto use lazy initialization for consistency.Once lazy initialization is added to
Write, update this method similarly to ensure consistent behavior.func (oc *OutputCapture) Flush() { - if oc == nil || oc.lineWriter == nil { + if oc == nil { return } + oc.ensureLineWriter() oc.lock.Lock() defer oc.lock.Unlock() oc.lineWriter.Flush() }
346-355: Critical: Incorrect logic causes panic whenverboseis true butocis nil.The condition at line 346 uses OR (
||) instead of AND (&&). Whenverboseis true butocis nil:
- Line 347 panics on
oc.Printf- Lines 348-349 set stdout/stderr to nil (likely causing silent output loss)
- Line 355 panics on
oc.Flush()Apply this fix:
- if oc != nil || verbose { + if oc != nil { oc.Printf("Running go mod tidy") tidyCmd.Stdout = oc tidyCmd.Stderr = oc + } else if verbose { + tidyCmd.Stdout = os.Stdout + tidyCmd.Stderr = os.Stderr } if err := tidyCmd.Run(); err != nil { return fmt.Errorf("failed to run go mod tidy: %w", err) } - oc.Flush() + if oc != nil { + oc.Flush() + }
🧹 Nitpick comments (1)
tsunami/build/build.go (1)
67-76: Simplify nil check inPrintf.The check for
lineWriter == nilis unnecessary sincePrintfdoesn't uselineWriter—it appends directly tooc.lines.-func (oc *OutputCapture) Printf(format string, args ...interface{}) { - if oc == nil || oc.lineWriter == nil { - log.Printf(format, args...) - return - } +func (oc *OutputCapture) Printf(format string, args ...interface{}) { + if oc == nil { + log.Printf(format, args...) + return + } line := fmt.Sprintf(format, args...) oc.lock.Lock() defer oc.lock.Unlock() oc.lines = append(oc.lines, line) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tsunami/build/build.go(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
tsunami/build/build.go (1)
pkg/util/utilfn/streamtolines.go (2)
LineWriter(89-92)NewLineWriter(95-99)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Analyze (go)
- GitHub Check: Build for TestDriver.ai
🔇 Additional comments (1)
tsunami/build/build.go (1)
38-47: LGTM!The initialization of
lineWriterwith the callback is well-designed. The callback appends tooc.lineswithout explicit locking because it's invoked within the critical section ofWrite/Flushmethods that already hold the lock.
| if oc != nil || opts.Verbose { | ||
| oc.Printf("Running: %s", strings.Join(buildCmd.Args, " ")) | ||
| buildCmd.Stdout = os.Stdout | ||
| buildCmd.Stderr = os.Stderr | ||
| buildCmd.Stdout = oc | ||
| buildCmd.Stderr = oc | ||
| } | ||
|
|
||
| if err := buildCmd.Run(); err != nil { | ||
| return fmt.Errorf("failed to build application: %w", err) | ||
| } | ||
| oc.Flush() |
There was a problem hiding this comment.
Critical: Incorrect logic causes panic when verbose is true but oc is nil.
Same issue as in createGoMod: the OR condition at line 679 causes panics at lines 680 and 688 when verbose is true but oc is nil.
Apply this fix:
- if oc != nil || opts.Verbose {
+ if oc != nil {
oc.Printf("Running: %s", strings.Join(buildCmd.Args, " "))
buildCmd.Stdout = oc
buildCmd.Stderr = oc
+ } else if opts.Verbose {
+ buildCmd.Stdout = os.Stdout
+ buildCmd.Stderr = os.Stderr
}
if err := buildCmd.Run(); err != nil {
return fmt.Errorf("failed to build application: %w", err)
}
- oc.Flush()
+ if oc != nil {
+ oc.Flush()
+ }🤖 Prompt for AI Agents
In tsunami/build/build.go around lines 679 to 688, the condition uses "oc != nil
|| opts.Verbose" which leads to dereferencing a nil oc when verbose is true;
change the guard so all uses of oc (oc.Printf, buildCmd.Stdout, buildCmd.Stderr,
and oc.Flush) only execute when oc != nil (e.g., use oc != nil && opts.Verbose
for the print if you need verbosity tied to opts, or simply check oc != nil
before assigning Stdout/Stderr and calling Printf), and ensure oc.Flush() is
called only when oc != nil to avoid the panic.
No description provided.