refactor: centralize outlier functions and deduplicate shared scaffolding - #9654
Merged
Conversation
8 tasks
…extract jsonFileSink, centralize ParseServerIDFromToolName
Copilot
AI
changed the title
[WIP] Refactor semantic function clustering analysis based on findings
refactor: centralize outlier functions and deduplicate shared scaffolding
Jul 19, 2026
lpcox
marked this pull request as ready for review
July 19, 2026 23:32
Contributor
There was a problem hiding this comment.
Pull request overview
Centralizes misplaced helpers and deduplicates JSON-backed logger state.
Changes:
- Moves agent ID generation and tool-name parsing into focused modules.
- Introduces shared
jsonFileSinklogger scaffolding. - Updates callers, tests, and logger namespace exceptions.
Show a summary per file
| File | Description |
|---|---|
internal/auth/id.go |
Houses agent ID generation. |
internal/auth/header.go |
Removes ID generation concerns. |
internal/util/toolname.go |
Adds canonical server ID parser. |
internal/util/toolname_test.go |
Tests tool-name parsing. |
internal/middleware/jqschema.go |
Uses the canonical parser. |
internal/middleware/jqschema_coverage_test.go |
Updates parser references. |
internal/logger/fileutil.go |
Adds shared JSON sink state. |
internal/logger/tools_logger.go |
Embeds the shared sink. |
internal/logger/tools_logger_test.go |
Updates logger construction. |
internal/logger/observed_url_domains_logger.go |
Embeds the shared sink. |
internal/logger/observed_url_domains_logger_test.go |
Updates sink-related tests. |
internal/logger/logger_namespace_test.go |
Moves the API-key namespace exception. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 12/12 changed files
- Comments generated: 2
- Review effort level: Medium
Comment on lines
+602
to
+603
| // TestParseServerIDFromToolName exercises all three branches of | ||
| // util.ParseServerIDFromToolName (formerly the unexported helper in jqschema.go): |
Collaborator
There was a problem hiding this comment.
Done in 3ffc166 — removed the duplicate TestParseServerIDFromToolName block and the now-unused util import. The canonical table in internal/util/toolname_test.go covers these cases and TestAuditObservedURLDomains covers the middleware call site.
| // GenerateRandomAgentID generates a cryptographically random agent ID. | ||
| // Per spec §7.3, the gateway SHOULD generate a random agent ID on startup | ||
| // if none is provided. Returns a 32-byte hex-encoded string (64 chars). | ||
| func GenerateRandomAgentID() (string, error) { |
Collaborator
There was a problem hiding this comment.
Done in 3ffc166 — moved the ID-generation failure/recovery tests and the errorReader helper into a new internal/auth/id_test.go, and dropped the now-unused crypto/rand and errors imports from header_test.go.
Remove the redundant embedded field `jsonFileSink` from the selector; the `useFallback` field is promoted through the embedded struct, so `globalObservedURLDomainsLogger.useFallback` is equivalent and satisfies staticcheck QF1008. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
- Move GenerateRandomAgentID failure/recovery tests and the errorReader helper out of internal/auth/header_test.go into a new id_test.go, so the header-focused test file no longer owns ID-generation test code. Drop the now-unused crypto/rand and errors imports from header_test.go. - Remove the duplicate TestParseServerIDFromToolName block (and the unused util import) from internal/middleware/jqschema_coverage_test.go; the canonical table in internal/util/toolname_test.go already covers these cases, and TestAuditObservedURLDomains covers the middleware call site. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
This was referenced Jul 19, 2026
This was referenced Jul 20, 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.
Addresses findings from semantic function clustering analysis: four functions sat in wrong files, and two logger types duplicated the same three-field state struct.
Changes
internal/auth/id.go(new)GenerateRandomAgentIDout ofheader.go(which only does header parsing/validation) into a dedicated filelogAPIKeylogger with it; updates namespace test exception list fromheader.go→id.gofmtandutilimports fromheader.gointernal/logger/fileutil.go—jsonFileSinkbase structToolsLoggerandObservedURLDomainsLoggerboth carried identicallogDir string,fileName string,useFallback boolfieldsjsonFileSinkstruct with awriteJSON(data, perm)helper:internal/util/toolname.go(new) —ParseServerIDFromToolNameparseServerIDFromToolNamewas a generic string utility embedded injqschema.go(jq filtering logic)util.ParseServerIDFromToolNamewith a dedicatedtoolname_test.go;jqschema.goand its coverage test updated to call the canonical locationSession ID extraction (no change needed)
extractSessionIDFromRequestinserver/session.gois already a minimal one-line wrapper overauth.ExtractSessionIDFromHeaders— no duplication to fix