fixing cloud agents log streaming early exit based on actual logs being received

[real-danm]

Any log line received from the upstream sdk or user code prefixed with ERROR: would stop the log streaming and not show the full output of the message. This ERROR: check had previously been used to close the writer once the upstream sent the message; this has been solved on the server side by correctly closing the writer when an error is found.
also adds some tests to simulate various failure and writer close modes.

Summary by CodeRabbit

• Bug Fixes

  • Log streaming no longer interrupts on lines prefixed with "ERROR:" — log lines are streamed continuously and only writer or transport failures terminate the stream.

• Tests

  • Added comprehensive tests for log streaming covering writer failures, context cancellation, HTTP errors, and mid-stream connection closures.

_{✏️ Tip: You can customize this high-level summary in your review settings.}

[coderabbitai]

📝 Walkthrough

Walkthrough

Removed a runtime check that scanned log lines for an "ERROR:" prefix; log lines are now written to the provided writer unconditionally. A new test suite for Client.StreamLogs was added covering writer errors, context cancellation, non-OK responses, and server disconnects.

Changes

Cohort / File(s)	Summary
Log Streaming Simplification pkg/cloudagents/logs.go	Removed strings import and the runtime check that searched for "ERROR:" prefixes; writer now receives log lines unconditionally, with errors only propagated from writer failures.
Test Coverage for StreamLogs pkg/cloudagents/logs_test.go	Added comprehensive tests for Client.StreamLogs covering: reader closed early, context cancellation, partial/failing writer behavior, non-OK HTTP responses with APIError, and server closing connection mid-stream.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~22 minutes

Poem

🐰 I nibble logs that once were checked,
Now lines flow on, no prefixes wrecked.
Tests hop round each broken stream,
Catching drops and frantic beams.
A rabbit cheers: the pipeline's decked! 🥕

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title directly describes the main bug fix: removing the premature 'ERROR:' prefix check that caused log streaming to exit early. This matches the core change in logs.go and the test additions.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing touches

• 📝 Docstrings were successfully generated.

---

🧹 Recent nitpick comments

pkg/cloudagents/logs_test.go (1)

19-44: Make writer-closure test deterministic (avoid io.Pipe + sleep).
Using an io.Pipe without a reader plus a fixed sleep makes the test timing-dependent and can block on Write. Consider a deterministic writer that returns io.ErrClosedPipe after N writes by extending failingWriter, and remove the goroutine/sleep.

♻️ Proposed refactor

 type failingWriter struct {
 	failAfter int
 	written   int
+	err       error
 }
 
 func (fw *failingWriter) Write(p []byte) (n int, err error) {
 	fw.written++
 	if fw.written > fw.failAfter {
-		return 0, fmt.Errorf("writer closed")
+		if fw.err != nil {
+			return 0, fw.err
+		}
+		return 0, fmt.Errorf("writer closed")
 	}
 	return len(p), nil
 }
 
 func TestStreamLogs_WriterClosesEarly(t *testing.T) {
-	_, pw := io.Pipe()
+	writer := &failingWriter{failAfter: 1, err: io.ErrClosedPipe}
 	// ...
-	go func() {
-		time.Sleep(100 * time.Millisecond)
-		pw.Close()
-	}()
-
-	err := client.StreamLogs(context.Background(), "deploy", "test-agent", pw, "us-west")
+	err := client.StreamLogs(context.Background(), "deploy", "test-agent", writer, "us-west")
 	// ...
 }
 
 func TestStreamLogs_WriterReturnsError(t *testing.T) {
 	// ...
-	writer := &failingWriter{failAfter: 2}
+	writer := &failingWriter{failAfter: 2, err: fmt.Errorf("writer closed")}
 	err := client.StreamLogs(context.Background(), "deploy", "test-agent", writer, "us-west")
 	// ...
 }

Also applies to: 91-102, 121-122

📜 Recent review details

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 86d254c and a18a01e.

📒 Files selected for processing (1)

• pkg/cloudagents/logs_test.go

🧰 Additional context used

🧬 Code graph analysis (1)

pkg/cloudagents/logs_test.go (2)

pkg/cloudagents/client.go (1)

• Client (34-44)

pkg/cloudagents/logs.go (1)

• APIError (27-30)

⏰ 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). (2)

• GitHub Check: test
• GitHub Check: Analyze (go)

🔇 Additional comments (3)

pkg/cloudagents/logs_test.go (3)

56-89: Good cancellation assertion.
Using errors.Is(err, context.Canceled) keeps the test robust to wrapped errors.

---

132-157: Solid non-OK response coverage.
Validates error handling when the server returns a non-200 status.

---

159-190: Nice coverage for mid-stream disconnects.
Asserts scanner error/EOF when the server drops the connection.

_{✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.}

---

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Actionable comments posted: 1

🤖 Fix all issues with AI agents

In `@pkg/cloudagents/logs_test.go`:
- Around line 188-190: The test currently checks error text with
strings.Contains(err.Error(), "scanner error") and "unexpected EOF" but only
calls t.Logf when neither matches, so unexpected errors don't fail the test;
update the assertion: replace the t.Logf call with t.Errorf (or t.Fatalf) to
fail the test and include the actual err and expected substrings in the message,
e.g. reference the existing err variable and strings.Contains checks so the test
fails on unexpected error text; alternatively, if any error is acceptable,
remove the strings.Contains conditional entirely and assert that err != nil (or
adjust the test to explicitly document acceptance).

🧹 Nitpick comments (1)

pkg/cloudagents/logs_test.go (1)

19-54: Potential test flakiness: discarded pipe reader may cause writes to block.

The test creates an io.Pipe() but discards the reader (pr). Writes to a PipeWriter without an active reader will block until the pipe is closed. This means the test relies on timing behavior where pw.Close() unblocks the write after 100ms.

If the intent is to test writer closure, consider using io.Pipe() with a goroutine that reads and discards data, or use a custom writer that closes after a threshold (similar to failingWriter). This would make the test more deterministic.

♻️ Suggested improvement

 func TestStreamLogs_WriterClosesEarly(t *testing.T) {
-	_, pw := io.Pipe()
+	pr, pw := io.Pipe()
+
+	// Drain the pipe to prevent blocking writes
+	go func() {
+		io.Copy(io.Discard, pr)
+	}()

 	server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {

📜 Review details

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 49e93af and 86d254c.

📒 Files selected for processing (2)

• pkg/cloudagents/logs.go
• pkg/cloudagents/logs_test.go

💤 Files with no reviewable changes (1)

• pkg/cloudagents/logs.go

🧰 Additional context used

🧬 Code graph analysis (1)

pkg/cloudagents/logs_test.go (2)

pkg/cloudagents/client.go (1)

• Client (34-44)

pkg/cloudagents/logs.go (1)

• APIError (27-30)

⏰ 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). (1)

• GitHub Check: test

🔇 Additional comments (3)

pkg/cloudagents/logs_test.go (3)

56-89: LGTM!

The test correctly validates context cancellation behavior using errors.Is() for proper error unwrapping.

---

91-130: LGTM!

The failingWriter provides a clean, deterministic way to test write failure scenarios. The test correctly validates error handling when the writer fails.

---

132-157: LGTM!

The test properly validates error handling for non-OK HTTP responses with the APIError payload structure.

_{✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.}

[yoonsio]

nice find!

[coderabbitai]

Caution

Docstrings generation - FAILED

No docstrings were generated.