Conversation
logRuntimeError has no conceptual tie to authentication — its only caller is rejectRequest in http_helpers.go. Moving it co-locates the function with both its caller and the other HTTP utility helpers (writeErrorResponse, rejectRequest, withResponseLogging). Also removes the unused 'log' and 'time' imports from auth.go, and moves TestLogRuntimeError (plus the stringPtr helper) to http_helpers_test.go for consistent test-to-source colocation. Follows the recommendation in #3113 (semantic function clustering analysis). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
lpcox
deleted the
repo-assist/refactor-logRuntimeError-2026-04-03-a530417244456718
branch
There was a problem hiding this comment.
Pull request overview
Refactors the internal/server package by relocating the logRuntimeError helper from auth-specific code into the shared HTTP helpers module, keeping logging utilities closer to their actual usage (rejectRequest) and the rest of the HTTP helper surface.
Changes:
- Moved
logRuntimeErrorfrominternal/server/auth.gotointernal/server/http_helpers.go. - Updated the helper’s internal debug logger usage to align with
http_helpers.go’s logger (logHelpers). - Moved the associated unit test from
auth_test.gotohttp_helpers_test.goto keep tests co-located with the helper.
Show a summary per file
| File | Description |
|---|---|
| internal/server/http_helpers.go | Adds logRuntimeError alongside other HTTP utilities; introduces time import. |
| internal/server/http_helpers_test.go | Adds TestLogRuntimeError and a local stringPtr helper. |
| internal/server/auth.go | Removes logRuntimeError and cleans up unused imports. |
| internal/server/auth_test.go | Removes TestLogRuntimeError and stringPtr now that the helper moved. |
Copilot's findings
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 4/4 changed files
- Comments generated: 1
| // Spec 9.2: Log to stdout with timestamp, server name, request ID, error details | ||
| log.Printf("[ERROR] timestamp=%s server=%s request_id=%s error_type=%s detail=%s path=%s method=%s", | ||
| timestamp, server, requestID, errorType, detail, r.URL.Path, r.Method) |
There was a problem hiding this comment.
logRuntimeError claims to log "to stdout per spec", but it uses the package-level standard logger (log.Printf). By default, the standard logger writes to stderr and also prepends its own timestamp/prefix unless log.SetFlags(0) is called, which can make the emitted line diverge from the intended spec format. Consider using a dedicated logger configured with os.Stdout and flags=0 (or update the comment if stdout/format is not required).
🤖 This PR was created by Repo Assist, an automated AI assistant.
Summary
Moves
logRuntimeErrorfrominternal/server/auth.gotointernal/server/http_helpers.go, where it logically belongs.Motivation
logRuntimeErroris a logging helper with no conceptual tie to authentication. Its only non-test caller isrejectRequestinhttp_helpers.go. Moving it co-locates the function with both its caller and the rest of the server's HTTP utility helpers (writeErrorResponse,rejectRequest,withResponseLogging).This follows the quick-win recommendation in #3113 (semantic function clustering analysis).
Changes
auth.go: removedlogRuntimeError, cleaned up now-unused"log"and"time"importshttp_helpers.go: addedlogRuntimeError, updated debug log to uselogHelpersinstead oflogAuth, added"time"importauth_test.go: removedTestLogRuntimeErrorandstringPtrhelperhttp_helpers_test.go: addedTestLogRuntimeErrorandstringPtrhelper for consistent test-to-source colocationNo functional behaviour changes — the function body is identical; only the package-level debug logger variable name changes from
logAuthtologHelpers, which doesn't affect emitted log content.Test Status
Go 1.25.0 is required but the network-isolated CI environment cannot download the toolchain. Syntactic correctness verified with
gofmt -eon all four changed files — all pass cleanly. Logic is identical to the original; no branches changed.