Refactor internal helper placement and naming to resolve semantic clustering outliers#7170
Merged
Merged
Conversation
Closed
9 tasks
Copilot
AI
changed the title
[WIP] Refactor semantic function clustering to fix outliers and duplicates
Refactor internal helper placement and naming to resolve semantic clustering outliers
Jun 7, 2026
Contributor
There was a problem hiding this comment.
Pull request overview
This PR refactors several internal helpers to better align them with their owning domains (server/guard/config/logger/mcp), reduces duplicated validation logic, and resolves a session-method naming collision in the MCP connection layer.
Changes:
- Moved session-ID truncation logic from
internal/strutilintointernal/serverand updated all server logging call sites and tests. - Moved WASM guard discovery helpers into
internal/guardand updated server initialization/tests to use guard-owned discovery. - Consolidated duplicated validation and expansion helpers (config/guard), simplified DIFC filtered-item metadata extraction, and renamed MCP SDK session validation helper to
requireSDKSession.
Show a summary per file
| File | Description |
|---|---|
| internal/strutil/truncate.go | Removes server-specific TruncateSessionID from shared string utilities. |
| internal/strutil/truncate_test.go | Removes TruncateSessionID tests now that behavior is owned by server. |
| internal/server/unified_wasm_guards_dir_test.go | Updates WASM guard discovery tests to exercise guard.FindServerWASMGuardFile. |
| internal/server/session.go | Switches session logging to use truncateSessionID in server package. |
| internal/server/session_util.go | Adds server-owned truncateSessionID helper for safe session logging. |
| internal/server/session_util_test.go | Adds focused unit tests for server-owned session truncation. |
| internal/server/session_auto_init.go | Updates auto-init logging to use server-owned session truncation. |
| internal/server/routed.go | Updates cache/log output to use server-owned session truncation. |
| internal/server/middleware.go | Updates OTEL/logging code paths to use server-owned session truncation. |
| internal/server/http_helpers.go | Updates request-body logging to use server-owned session truncation. |
| internal/server/guard_init.go | Delegates per-server WASM guard discovery/root-dir lookup to internal/guard. |
| internal/server/difc_log.go | Inlines author/number extraction into buildFilteredItemLogEntry and removes helpers. |
| internal/server/difc_log_test.go | Adds edge-case coverage for author-login and number extraction in the inlined logic. |
| internal/server/difc_log_helpers_test.go | Removes tests for helper functions that were deleted. |
| internal/mcp/sdk_method_dispatch_test.go | Updates comments to reflect requireSDKSession rename. |
| internal/mcp/pagination.go | Renames SDK session validation calls to requireSDKSession. |
| internal/mcp/pagination_test.go | Updates test comments to match requireSDKSession rename. |
| internal/mcp/connection.go | Renames requireSession to requireSDKSession and moves tag snapshot type out to logging file. |
| internal/mcp/connection_test.go | Updates tests to call requireSDKSession. |
| internal/mcp/connection_logging.go | Relocates AgentTagsSnapshot + context extraction helper alongside logging consumers. |
| internal/logger/rpc_helpers_test.go | Updates tests/benchmarks to call unexported extractErrorMessage. |
| internal/logger/log_cleanup.go | Unexports ExtractErrorMessage to extractErrorMessage. |
| internal/guard/wasm.go | Adds guard-owned WASM discovery helpers (GetWASMGuardsRootDir, FindServerWASMGuardFile). |
| internal/guard/wasm_validate.go | Delegates string-array and allow-only repo validation to canonical config validators. |
| internal/guard/label_agent.go | Moves label-agent helper functions into label-agent file alongside RunLabelAgent. |
| internal/guard/guard.go | Removes label-agent helpers now relocated to label_agent.go. |
| internal/config/guard_policy_validation.go | Adds canonical reusable validators (ValidateStringArrayField, IsValidAllowOnlyReposValue). |
| internal/config/expand.go | Centralizes expandMapInPlace and expandTracingVariables into a shared expansion module. |
| internal/config/config_tracing.go | Removes duplicated tracing-variable expansion helper. |
| internal/config/config_stdin.go | Removes duplicated map expansion helper; continues calling shared helper. |
Copilot's findings
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 30/30 changed files
- Comments generated: 0
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.
This PR resolves the semantic clustering findings across
internal/by relocating outlier functions/types to their owning domains and removing duplicate validation logic betweenconfigandguard. It also resolves an ambiguous transport-vs-server session method name collision.Config expansion logic consolidation
expandMapInPlaceandexpandTracingVariablesintointernal/config/expand.goso${VAR}expansion behavior lives in one place.config_stdin.goandconfig_tracing.go.Guard/config validator consolidation
internal/config/guard_policy_validation.go:ValidateStringArrayFieldIsValidAllowOnlyReposValueinternal/guard/wasm_validate.goto delegate to config validators instead of reimplementing equivalent checks.WASM guard discovery ownership moved to
guardinternal/guard/wasm.go:GetWASMGuardsRootDirFindServerWASMGuardFileinternal/server/guard_init.goand related tests to call guard-owned helpers.DIFC filtered-item map helper outliers removed
buildFilteredItemLogEntry(internal/server/difc_log.go) per issue recommendation.difc_log_test.go.Logger cleanup API scope
ExtractErrorMessagetoextractErrorMessage(internal/logger/log_cleanup.go) since usage is internal/test-only.MCP tag snapshot type placement
AgentTagsSnapshotandGetAgentTagsSnapshotFromContextfromconnection.gotoconnection_logging.goto colocate declarations with actual logging consumers.Label-agent helper placement
emptyAgentLabelsResultandApplyLabelAgentResultfrominternal/guard/guard.gotointernal/guard/label_agent.goalongsideRunLabelAgent.Session ID truncation ownership
internal/strutilintointernal/server/session_util.goastruncateSessionID.TruncateSessionIDfromstrutiland moved tests to server package.Naming collision removal
(*mcp.Connection).requireSession()torequireSDKSession()to distinguish SDK transport session validation fromserver.requireSession(ctx)lifecycle behavior.