fix: surface real Cerebras request errors instead of a generic fallback#139
Conversation
cancel() ran unconditionally right after every Chat() call, so ctx.Err() was always non-nil by the time the interrupt check ran — every real failure (bad request, auth error, network issue) was misclassified as "interrupted" and the detailed term.PrintError message never printed. Users only ever saw "cerebras backend request failed" with no reason.
Sigilix OverviewEffort: 2/5 (small) Quality gates
Summary — latest pushFixes a bug in RunCerebrasAgent where an unconditional cleanup Important files
Confidence: 4/5The core logic fix is a minimal, high-impact reorder that correctly separates cleanup from error detection, and the new test validates the exact regression, though the global stdout patching in the test is a minor fragility.
Suggested labels:
|
✅ QualityMax Pipeline
Powered by QualityMax — AI-Powered Test Automation |
![sigilix[bot]](https://avatars.githubusercontent.com/in/3558711?s=60&v=4){kind=small}
| out, _ := io.ReadAll(r) | ||
| return string(out) |
There was a problem hiding this comment.
captureStdout ignores io.ReadAll error, masking pipe-read failures
The captureStdout helper discards the error from io.ReadAll(r) on line 31. If the in-memory pipe read ever fails, the helper returns an empty string and the caller's assertions fail with a misleading "expected the detailed Cerebras error to be printed" message instead of surfacing the actual test-setup failure.
Example:
input: os.Pipe succeeds but io.ReadAll returns an error
actual: captureStdout returns "", test fails with "expected the detailed Cerebras error to be printed, got: \"\""
expected: test fails with "reading captured stdout: <underlying error>"
Current:
out, _ := io.ReadAll(r)
return string(out)Proposed:
out, err := io.ReadAll(r)
if err != nil {
t.Fatalf("reading captured stdout: %v", err)
}
return string(out)| out, _ := io.ReadAll(r) | |
| return string(out) | |
| out, err := io.ReadAll(r) | |
| if err != nil { | |
| t.Fatalf("reading captured stdout: %v", err) | |
| } | |
| return string(out) |
More Info
- Threat model: A rare pipe-read failure in the test helper produces a confusing assertion failure, making test debugging harder without changing production behavior.
- Specific code citations:
internal/agent/cerebras_agent_test.goline 31:out, _ := io.ReadAll(r)discards the returned error. - Existing protections: None; the error is explicitly ignored with
_. - Proposed mitigation: Capture the error and fail the test explicitly if the pipe read fails:
out, err := io.ReadAll(r); if err != nil { t.Fatalf("reading captured stdout: %v", err) }. - Alternative mitigations considered: Use
t.Cleanupto close the writer, but that still requires checking the read error. - Severity calibration: Score 2 because the failure mode is bounded to test diagnostics and in-memory pipe reads essentially never fail in practice.
Prompt To Fix With AI
This is a comment left during a code review.
Path: internal/agent/cerebras_agent_test.go
Line: 31-32
Comment:
**captureStdout ignores io.ReadAll error, masking pipe-read failures**
The `captureStdout` helper discards the error from `io.ReadAll(r)` on line 31. If the in-memory pipe read ever fails, the helper returns an empty string and the caller's assertions fail with a misleading "expected the detailed Cerebras error to be printed" message instead of surfacing the actual test-setup failure.
Example:
input: os.Pipe succeeds but io.ReadAll returns an error
actual: captureStdout returns "", test fails with "expected the detailed Cerebras error to be printed, got: \"\""
expected: test fails with "reading captured stdout: <underlying error>"
Threat model:
A rare pipe-read failure in the test helper produces a confusing assertion failure, making test debugging harder without changing production behavior.
Specific code citations:
`internal/agent/cerebras_agent_test.go` line 31: `out, _ := io.ReadAll(r)` discards the returned error.
Existing protections:
None; the error is explicitly ignored with `_`.
Proposed mitigation:
Capture the error and fail the test explicitly if the pipe read fails: `out, err := io.ReadAll(r); if err != nil { t.Fatalf("reading captured stdout: %v", err) }`.
Alternative mitigations considered:
Use `t.Cleanup` to close the writer, but that still requires checking the read error.
Severity calibration:
Score 2 because the failure mode is bounded to test diagnostics and in-memory pipe reads essentially never fail in practice.
How can I resolve this? If you propose a fix, please make it concise.
golangci-lint's errcheck flagged the unchecked httptest w.Write in the new regression test, which failed the CI build-and-test job.
1 participant
Summary
RunCerebrasAgent(internal/agent/cerebras_agent.go),cancel()was called unconditionally right after everyChat()call for cleanup. But the very next line checkedctx.Err() != nilto detect a genuine user-initiated interrupt (Ctrl-C viaAgent.CancelCurrent) — sincecancel()had just run,ctx.Err()was always non-nil at that point, so every real failure (bad model, auth error, malformed request, network issue, etc.) took the silent "interrupted" branch and returnedfalsewith zero diagnostic output. The caller then only ever saw the generic top-level fallback:cerebras backend request failed.ctx.Err()before the cleanupcancel()call, so the interrupt check reflects whether the context was actually canceled externally during the request, not by our own post-hoc cleanup.Test plan
TestRunCerebrasAgentSurfacesRequestErrorDetail, which spins up anhttptestserver returning HTTP 400 and asserts the detailed error message is printed. Confirmed it fails on the old code and passes with the fix.qmax-code --backend cerebras -p "hey".go build ./...andgo test ./...pass.