ci: make go-ci test output visible in logs

[KooshaPari]

Summary

• The go-ci job redirected all test output to a file via > target/test-baseline.json, making failures completely invisible in CI logs
• Use tee to stream output to both the CI log and the artifact file
• Add if: always() to artifact upload so test results are downloadable even on failure
• Remove redundant second go test invocation (was running the full suite twice)
• Add -count=1 to disable test caching in CI for deterministic results

Test plan

• Verify go-ci job output is now visible in CI logs when tests pass
• Verify go-ci job output is visible in CI logs when tests fail
• Verify artifact is uploaded even on test failure
• Identify actual test failure from visible output and fix in follow-up

Generated with Claude Code

Summary by CodeRabbit

• Chores
  • Improved CI test execution and exit-code handling, plus streamlined test artifact uploads (now a single JSON artifact).
• Tests
  • Hardened a server-request logging/recovery test to more reliably detect and assert re-panic behavior.

[gemini-code-assist]

Note

Gemini is unable to generate a summary for this pull request due to the file types involved not being currently supported.

[coderabbitai]

Warning

Rate limit exceeded

@KooshaPari has exceeded the limit for the number of commits that can be reviewed per hour. Please wait 7 minutes and 7 seconds before requesting another review.

⌛ How to resolve this issue?

After the wait time has elapsed, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

We recommend that you space out your commits to avoid hitting the rate limit.

🚦 How do rate limits work?

CodeRabbit enforces hourly rate limits for each developer per organization.

Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout.

Please see our FAQ for further information.

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro

Run ID: 6adeb3ec-2031-4493-907d-5539ae8ce2c8

📥 Commits

Reviewing files that changed from the base of the PR and between ea056bf and aac7414.

📒 Files selected for processing (2)

• pkg/llmproxy/logging/gin_logger.go
• pkg/llmproxy/logging/gin_logger_test.go

📝 Walkthrough

Walkthrough

Updates to the PR test GitHub Actions workflow to capture Go test exit codes and produce a single JSON artifact; plus a unit-test change in gin logger recovery to directly assert and re-panic http.ErrAbortHandler behavior.

Changes

Cohort / File(s)	Summary
CI/CD Workflow .github/workflows/pr-test-build.yml	Run go test -json -count=1 ./... piped to tee and capture exit status via PIPESTATUS; upload only target/test-baseline.json with if: always() and if-no-files-found: warn; test-baseline.txt removed.
Gin logger test pkg/llmproxy/logging/gin_logger_test.go	Refactor test to invoke GinLogrusRecovery directly, wrap handler to capture and re-panic http.ErrAbortHandler, replace defer/recover approach with explicit repanicked/repanic checks and stronger assertions on panic value/type.

Sequence Diagram(s)

(omitted — changes do not introduce a new multi-component control flow requiring a sequence diagram)

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Possibly related PRs

• ci: fix workflow required-check names for feat/cliproxy-service-runtime-worktree #513 — also modifies .github/workflows/pr-test-build.yml (related CI workflow adjustments).

Suggested labels

pkg:he:service-runtime

Poem

🐇 I hopped through tests and logs tonight,

Captured exits in JSON light,
A panic caught, then let it leap,
The logger guards while I still sleep,
Hooray — small fixes, bounding bright!

🚥 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 'ci: make go-ci test output visible in logs' accurately summarizes the main change: improving test output visibility in CI logs by using tee instead of redirecting to file.

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

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment
• Commit unit tests in branch fix/go-ci-test-failure

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

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

[github-actions]

@coderabbitai full review

Automated retrigger: CodeRabbit state=FAILURE, age=24.3m (stale after 20m).

[coderabbitai]

I'll perform a full review of the PR changes now.

✅ Actions performed

Full review triggered.

[coderabbitai]

Actionable comments posted: 3

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Inline comments:
In @.github/workflows/pr-test-build.yml:
- Around line 59-60: The CI and local baseline generation diverge because
Taskfile.yml still runs two go test invocations and writes both
test-baseline.json and test-baseline.txt, while the workflow job named
go-test-baseline only uploads test-baseline.json from a single uncached run;
make them consistent by either (a) updating the workflow job go-test-baseline to
run the same Taskfile target(s) that produce both test-baseline.json and
test-baseline.txt and upload both artifacts (add path for test-baseline.txt
alongside test-baseline.json), or (b) change Taskfile.yml to mirror CI’s single
uncached go test run and only emit test-baseline.json; pick one approach and
apply it so Taskfile.yml, the go-test-baseline job, and the produced artifacts
(test-baseline.json, test-baseline.txt) match.
- Around line 49-50: The pipeline is redirecting stderr into the JSON artifact;
update the command that runs tests by removing the stderr redirection so only
stdout (the JSON events) are piped to tee. Specifically, change the line
invoking "go test -json -count=1 ./... 2>&1 | tee target/test-baseline.json" to
omit "2>&1" so "go test -json -count=1 ./... | tee target/test-baseline.json"
(leave the surrounding "set +e" behavior unchanged).

In `@pkg/llmproxy/logging/gin_logger_test.go`:
- Around line 12-52: Extract the core recovery behavior into a helper
handleRecoveredPanic(c *gin.Context, recovered interface{}) and update
GinLogrusRecovery to call that helper; then replace the current
TestGinLogrusRecoveryRepanicsErrAbortHandler with a test that directly calls
handleRecoveredPanic using a minimal test gin.Context (or a stubbed context) to
assert that http.ErrAbortHandler is re-panicked, removing the need to run
engine.ServeHTTP and keeping the test under 40 lines. Ensure GinLogrusRecovery
still uses the helper so production behavior is unchanged and the new test only
exercises handleRecoveredPanic for deterministic, framework-independent
verification.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro

Run ID: 2d35a25e-03f0-465a-8066-cae2688c2c4c

📥 Commits

Reviewing files that changed from the base of the PR and between e94432e and ea056bf.

📒 Files selected for processing (2)

• .github/workflows/pr-test-build.yml
• pkg/llmproxy/logging/gin_logger_test.go

📜 Review details

🧰 Additional context used

📓 Path-based instructions (1)

**/*.go

📄 CodeRabbit inference engine (AGENTS.md)

**/*.go: NEVER create a v2 file - refactor the original instead
NEVER create a new class if an existing one can be made generic
NEVER create custom implementations when an OSS library exists - search pkg.go.dev for existing libraries before writing code
Build generic building blocks (provider interface + registry) before application logic
Use chi for HTTP routing (NOT custom routers)
Use zerolog for logging (NOT fmt.Print)
Use viper for configuration (NOT manual env parsing)
Use go-playground/validator for validation (NOT manual if/else validation)
Use golang.org/x/time/rate for rate limiting (NOT custom limiters)
Use template strings for messages instead of hardcoded messages and config-driven logic instead of code-driven
Zero new lint suppressions without inline justification
All new code must pass: go fmt, go vet, golint
Maximum function length: 40 lines
No placeholder TODOs in committed code

Files:

• pkg/llmproxy/logging/gin_logger_test.go

🪛 GitHub Actions: pr-test-build

pkg/llmproxy/logging/gin_logger_test.go

[error] 44-51: GinLogrusRecovery: expected re-panic of http.ErrAbortHandler, but it did not

[coderabbitai]

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🌐 Web query:

Does go test -jsonguarantee JSON events on stdout while non-JSON diagnostics from thego tool may still be written to stderr?

💡 Result:

Yes.

• go test -json routes the test run through go tool test2json, which “writes a corresponding stream of JSON events to standard output.” The JSON events include the test binary’s output (its stdout+stderr are merged into TestEvent.Output). [1], [2]
• Separately, the go command’s own diagnostics (for example, failures “building the tests”) are written to the go command’s standard error, which is “reserved for printing errors building the tests.” So non-JSON diagnostics from the go tool may still appear on stderr even when stdout is a JSON event stream. [3]

Sources: [1] cmd/test2json docs, [2] Go 1.10 release notes, [3] cmd/go/internal/test docs/source.

---

Keep stderr out of test-baseline.json.

2>&1 | tee target/test-baseline.json redirects the go command's own diagnostics (e.g., build errors) into the artifact, corrupting it with non-JSON content. go test -json sends JSON events to stdout only; remove 2>&1 so stderr stays in the job log where it should be.

🔧 Proposed fix

-          go test -json -count=1 ./... 2>&1 | tee target/test-baseline.json
+          go test -json -count=1 ./... | tee target/test-baseline.json

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change

	set +e
	go test -json -count=1 ./... 2>&1 | tee target/test-baseline.json
	set +e
	go test -json -count=1 ./... | tee target/test-baseline.json

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In @.github/workflows/pr-test-build.yml around lines 49 - 50, The pipeline is
redirecting stderr into the JSON artifact; update the command that runs tests by
removing the stderr redirection so only stdout (the JSON events) are piped to
tee. Specifically, change the line invoking "go test -json -count=1 ./... 2>&1 |
tee target/test-baseline.json" to omit "2>&1" so "go test -json -count=1 ./... |
tee target/test-baseline.json" (leave the surrounding "set +e" behavior
unchanged).

[coderabbitai]

🧹 Nitpick | 🔵 Trivial

CI and local baseline generation now diverge.

Taskfile.yml:91-96 still produces both test-baseline.json and test-baseline.txt via two separate go test invocations, while this workflow now uploads only the JSON artifact from a single uncached run. That makes local reproduction materially different from CI when debugging failures.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In @.github/workflows/pr-test-build.yml around lines 59 - 60, The CI and local
baseline generation diverge because Taskfile.yml still runs two go test
invocations and writes both test-baseline.json and test-baseline.txt, while the
workflow job named go-test-baseline only uploads test-baseline.json from a
single uncached run; make them consistent by either (a) updating the workflow
job go-test-baseline to run the same Taskfile target(s) that produce both
test-baseline.json and test-baseline.txt and upload both artifacts (add path for
test-baseline.txt alongside test-baseline.json), or (b) change Taskfile.yml to
mirror CI’s single uncached go test run and only emit test-baseline.json; pick
one approach and apply it so Taskfile.yml, the go-test-baseline job, and the
produced artifacts (test-baseline.json, test-baseline.txt) match.

[coderabbitai]

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🌐 Web query:

In Gin 1.10.1, if a gin.CustomRecoverycallback re-panics, is that panic guaranteed to be observable by an outer middleware that invokes the returned handler directly instead of registering it withengine.Use?

💡 Result:

Yes — the re-panic will propagate out of the gin.CustomRecovery-returned handler to its direct caller, so an “outer” middleware that wraps and directly invokes that handler can observe (and recover) the panic.

In Gin v1.10.1, the recovery middleware does recover() in a deferred function and then calls your handle(c, err) without any additional recover around that call; so if handle panics, that panic is not caught by Gin’s recovery middleware and unwinds to the caller of the middleware handler. (raw.githubusercontent.com)

If nothing in your stack recovers it, the panic may still be recovered by Go’s net/http server boundary (logging a stack trace and closing/resetting the stream), but that’s outside Gin. (pkg.go.dev)

Citations:

• 1: https://raw.githubusercontent.com/gin-gonic/gin/v1.10.1/recovery.go
• 2: https://pkg.go.dev/net/http

---

Extract the recovery callback into a testable helper function.

The current test exceeds the 40-line function length limit (41 lines) and remains coupled to Gin's panic propagation semantics, which has introduced a failure in CI. While gin.CustomRecovery callbacks do propagate re-panics to their direct caller, routing through engine.ServeHTTP introduces additional panic recovery boundaries that prevent the outer middleware from observing the panic.

Extract the recovery logic into a standalone handleRecoveredPanic(c *gin.Context, recovered interface{}) helper and test it directly with a minimal test context instead of routing through the full HTTP server pipeline. This isolates the behavior you actually care about, eliminates framework complexity, and satisfies the maximum function length guideline.

🔧 Proposed refactor

diff --git a/pkg/llmproxy/logging/gin_logger.go b/pkg/llmproxy/logging/gin_logger.go
@@
+func handleRecoveredPanic(c *gin.Context, recovered interface{}) {
+	if err, ok := recovered.(error); ok && errors.Is(err, http.ErrAbortHandler) {
+		panic(http.ErrAbortHandler)
+	}
+
+	log.WithFields(log.Fields{
+		"panic": recovered,
+		"stack": string(debug.Stack()),
+		"path":  c.Request.URL.Path,
+	}).Error("recovered from panic")
+
+	c.AbortWithStatus(http.StatusInternalServerError)
+}
+
 func GinLogrusRecovery() gin.HandlerFunc {
-	return gin.CustomRecovery(func(c *gin.Context, recovered interface{}) {
-		if err, ok := recovered.(error); ok && errors.Is(err, http.ErrAbortHandler) {
-			panic(http.ErrAbortHandler)
-		}
-
-		log.WithFields(log.Fields{
-			"panic": recovered,
-			"stack": string(debug.Stack()),
-			"path":  c.Request.URL.Path,
-		}).Error("recovered from panic")
-
-		c.AbortWithStatus(http.StatusInternalServerError)
-	})
+	return gin.CustomRecovery(handleRecoveredPanic)
 }
diff --git a/pkg/llmproxy/logging/gin_logger_test.go b/pkg/llmproxy/logging/gin_logger_test.go
@@
 func TestGinLogrusRecoveryRepanicsErrAbortHandler(t *testing.T) {
-	// Test the recovery handler directly to avoid platform-dependent behavior
-	// in gin's ServeHTTP panic propagation (macOS vs Linux differ).
-	handler := GinLogrusRecovery()
-
 	gin.SetMode(gin.TestMode)
-	engine := gin.New()
-
-	var repanicked bool
-	var repanic interface{}
-
-	// Wrap the recovery middleware to intercept the re-panic before gin's
-	// outer recovery can swallow it.
-	engine.Use(func(c *gin.Context) {
-		defer func() {
-			if r := recover(); r != nil {
-				repanicked = true
-				repanic = r
-			}
-		}()
-		handler(c)
-	})
-	engine.GET("/abort", func(c *gin.Context) {
-		panic(http.ErrAbortHandler)
-	})
-
-	req := httptest.NewRequest(http.MethodGet, "/abort", nil)
-	recorder := httptest.NewRecorder()
-
-	engine.ServeHTTP(recorder, req)
-
-	if !repanicked {
-		t.Fatalf("expected GinLogrusRecovery to re-panic http.ErrAbortHandler, but it did not")
-	}
-	err, ok := repanic.(error)
-	if !ok {
-		t.Fatalf("expected error panic, got %T", repanic)
-	}
-	if !errors.Is(err, http.ErrAbortHandler) {
-		t.Fatalf("expected ErrAbortHandler, got %v", err)
-	}
+	c, _ := gin.CreateTestContext(httptest.NewRecorder())
+	c.Request = httptest.NewRequest(http.MethodGet, "/abort", nil)
+
+	defer func() {
+		r := recover()
+		err, ok := r.(error)
+		if !ok || !errors.Is(err, http.ErrAbortHandler) {
+			t.Fatalf("expected ErrAbortHandler, got %T (%v)", r, r)
+		}
+	}()
+
+	handleRecoveredPanic(c, http.ErrAbortHandler)
 }

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change

	func TestGinLogrusRecoveryRepanicsErrAbortHandler(t *testing.T) {
	gin.SetMode(gin.TestMode)
	// Test the recovery handler directly to avoid platform-dependent behavior
	// in gin's ServeHTTP panic propagation (macOS vs Linux differ).
	handler := GinLogrusRecovery()
	gin.SetMode(gin.TestMode)
	engine := gin.New()
	engine.Use(GinLogrusRecovery())
	var repanicked bool
	var repanic interface{}
	// Wrap the recovery middleware to intercept the re-panic before gin's
	// outer recovery can swallow it.
	engine.Use(func(c *gin.Context) {
	defer func() {
	if r := recover(); r != nil {
	repanicked = true
	repanic = r
	}
	}()
	handler(c)
	})
	engine.GET("/abort", func(c *gin.Context) {
	panic(http.ErrAbortHandler)
	})
	req := httptest.NewRequest(http.MethodGet, "/abort", nil)
	recorder := httptest.NewRecorder()
	defer func() {
	recovered := recover()
	if recovered == nil {
	t.Fatalf("expected panic, got nil")
	}
	err, ok := recovered.(error)
	if !ok {
	t.Fatalf("expected error panic, got %T", recovered)
	}
	if !errors.Is(err, http.ErrAbortHandler) {
	t.Fatalf("expected ErrAbortHandler, got %v", err)
	}
	if err != http.ErrAbortHandler {
	t.Fatalf("expected exact ErrAbortHandler sentinel, got %v", err)
	}
	}()
	engine.ServeHTTP(recorder, req)
	if !repanicked {
	t.Fatalf("expected GinLogrusRecovery to re-panic http.ErrAbortHandler, but it did not")
	}
	err, ok := repanic.(error)
	if !ok {
	t.Fatalf("expected error panic, got %T", repanic)
	}
	if !errors.Is(err, http.ErrAbortHandler) {
	t.Fatalf("expected ErrAbortHandler, got %v", err)
	}
	func TestGinLogrusRecoveryRepanicsErrAbortHandler(t *testing.T) {
	gin.SetMode(gin.TestMode)
	c, _ := gin.CreateTestContext(httptest.NewRecorder())
	c.Request = httptest.NewRequest(http.MethodGet, "/abort", nil)
	defer func() {
	r := recover()
	err, ok := r.(error)
	if !ok || !errors.Is(err, http.ErrAbortHandler) {
	t.Fatalf("expected ErrAbortHandler, got %T (%v)", r, r)
	}
	}()
	handleRecoveredPanic(c, http.ErrAbortHandler)
	}

🧰 Tools

🪛 GitHub Actions: pr-test-build

[error] 44-51: GinLogrusRecovery: expected re-panic of http.ErrAbortHandler, but it did not

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@pkg/llmproxy/logging/gin_logger_test.go` around lines 12 - 52, Extract the
core recovery behavior into a helper handleRecoveredPanic(c *gin.Context,
recovered interface{}) and update GinLogrusRecovery to call that helper; then
replace the current TestGinLogrusRecoveryRepanicsErrAbortHandler with a test
that directly calls handleRecoveredPanic using a minimal test gin.Context (or a
stubbed context) to assert that http.ErrAbortHandler is re-panicked, removing
the need to run engine.ServeHTTP and keeping the test under 40 lines. Ensure
GinLogrusRecovery still uses the helper so production behavior is unchanged and
the new test only exercises handleRecoveredPanic for deterministic,
framework-independent verification.