refactor: eliminate duplicate code patterns (logger + server)#2950
Merged
refactor: eliminate duplicate code patterns (logger + server)#2950
Conversation
…ages #2908: Remove 10 boilerplate initGlobal*/closeGlobal* wrapper functions from global_helpers.go. Each was a one-line pass-through to the generic initGlobalLogger/closeGlobalLogger helpers. Callers now invoke the generic helpers directly. #2910: Extract rejectRequest() helper in http_helpers.go that consolidates the 3-step rejection pattern (structured log + runtime error log + HTTP error response). Update auth.go to use it. Both changes are zero-behavior-change refactors. Closes #2908 Closes #2910 Closes #2907 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Refactors internal logger and server packages to remove duplicated boilerplate in global logger lifecycle management and HTTP rejection handling, as identified by the duplicate-code analysis issues.
Changes:
- Removed type-specific
initGlobal*/closeGlobal*one-line wrappers and updated logger implementations to call the generic helpers directly. - Added a
rejectRequest()helper to consolidate the repeated “log + runtime-log + HTTP error response” rejection pattern. - Updated
authMiddlewareto userejectRequest()for unauthorized cases.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| internal/server/http_helpers.go | Introduces rejectRequest() helper to centralize rejection logging + error response behavior. |
| internal/server/auth.go | Refactors auth rejection paths to use rejectRequest(). |
| internal/logger/global_helpers.go | Removes type-specific global logger wrapper functions, leaving only generic helpers. |
| internal/logger/file_logger.go | Switches to generic global logger init/close helpers. |
| internal/logger/jsonl_logger.go | Switches to generic global logger init/close helpers. |
| internal/logger/markdown_logger.go | Switches to generic global logger init/close helpers. |
| internal/logger/server_file_logger.go | Switches to generic global logger init/close helpers. |
| internal/logger/tools_logger.go | Switches to generic global logger init/close helpers. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
47
to
56
| if authHeader == "" { | ||
| // Spec 7.1: Missing token returns 401 | ||
| logger.LogErrorMd("auth", "Authentication failed: missing Authorization header, remote=%s, path=%s", r.RemoteAddr, r.URL.Path) | ||
| logRuntimeError("authentication_failed", "missing_auth_header", r, nil) | ||
| writeErrorResponse(w, http.StatusUnauthorized, "unauthorized", "missing Authorization header") | ||
| rejectRequest(w, r, http.StatusUnauthorized, "unauthorized", "missing Authorization header", "auth", "missing_auth_header") | ||
| return | ||
| } | ||
|
|
||
| // Spec 7.1: Authorization header must contain API key directly (not Bearer scheme) | ||
| if authHeader != apiKey { | ||
| logger.LogErrorMd("auth", "Authentication failed: invalid API key, remote=%s, path=%s", r.RemoteAddr, r.URL.Path) | ||
| logRuntimeError("authentication_failed", "invalid_api_key", r, nil) | ||
| writeErrorResponse(w, http.StatusUnauthorized, "unauthorized", "invalid API key") | ||
| rejectRequest(w, r, http.StatusUnauthorized, "unauthorized", "invalid API key", "auth", "invalid_api_key") | ||
| return |
There was a problem hiding this comment.
These rejectRequest calls now log runtime errors as error_type=missing_auth_header / invalid_api_key with detail set to the human message, whereas the prior behavior was error_type=authentication_failed with detail set to the specific reason token. If downstream tooling/monitoring expects authentication_failed as the type, this is a behavior change; pass authentication_failed as the runtime error type and keep missing_auth_header/invalid_api_key as the detail (or adjust rejectRequest signature accordingly).
See below for a potential fix:
rejectRequest(w, r, http.StatusUnauthorized, "unauthorized", "missing Authorization header", "auth", "authentication_failed")
return
}
// Spec 7.1: Authorization header must contain API key directly (not Bearer scheme)
if authHeader != apiKey {
rejectRequest(w, r, http.StatusUnauthorized, "unauthorized", "invalid API key", "auth", "authentication_failed")
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Split the runtimeErrType parameter into runtimeErrType + runtimeDetail so logRuntimeError receives the same error_type/detail values as before (e.g. error_type=authentication_failed, detail=missing_auth_header). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
This was referenced Mar 31, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
Duplicate code analysis (#2907) identified 3 patterns. #2909 (withLock) was already fixed in PR #2939. This PR addresses the remaining two:
Pattern #2908: Boilerplate initGlobal*/closeGlobal* Wrappers
global_helpers.gocontained 10 one-line wrapper functions (5 init + 5 close pairs) that simply delegated to the genericinitGlobalLogger/closeGlobalLoggerhelpers. Adding a 6th logger type would require adding yet another copy-paste pair.Fix: Remove all 10 wrappers. Callers now invoke the generic helpers directly. -53 lines.
Pattern #2910: Auth/Guard Rejection Trio
Every auth rejection in
auth.gorepeated the same 3-step block: structured log → runtime error log → HTTP error response. Any change to logging or response shape required updating every site.Fix: Extract
rejectRequest()helper inhttp_helpers.gothat consolidates all three steps into one call. -6 lines, +9 lines (net: +3 with the helper).Changes
internal/logger/global_helpers.gointernal/logger/file_logger.goinitGlobalLogger/closeGlobalLoggerdirectlyinternal/logger/jsonl_logger.goinitGlobalLogger/closeGlobalLoggerdirectlyinternal/logger/markdown_logger.goinitGlobalLogger/closeGlobalLoggerdirectlyinternal/logger/server_file_logger.goinitGlobalLogger/closeGlobalLoggerdirectlyinternal/logger/tools_logger.goinitGlobalLogger/closeGlobalLoggerdirectlyinternal/server/http_helpers.gorejectRequest()helperinternal/server/auth.gorejectRequest()instead of inline trioTesting
make agent-finishedpasses (format, build, lint, all tests).Both changes are zero-behavior-change refactors.
Closes #2907
Closes #2908
Closes #2910