refactor: semantic function clustering — reduce cross-concern placement and duplication#7761
Conversation
- Move sysListServersHandler from session.go to system_tools.go - Merge strutil/session_suffix.go + json_clone.go into strutil.go - Export config.ValidateAndNormalizeIntegrityField, remove duplicate guard validation - Add doc.go note about intentional per-logger factory pattern
There was a problem hiding this comment.
Pull request overview
This PR performs targeted semantic refactors to reduce cross-concern placement and deduplicate logic across the gateway, while unifying integrity-level validation and its error messages.
Changes:
- Moved
sysListServersHandlerfrom session lifecycle code intosystem_tools.goalongside other sys-tool helpers. - Consolidated
SessionSuffixandDeepCloneJSONintointernal/strutil/strutil.goand removed the now-redundant one-function files. - Exported
config.ValidateAndNormalizeIntegrityFieldand switched the WASM guard integrity validation to delegate to it, updating tests for the unified error message format.
Show a summary per file
| File | Description |
|---|---|
| internal/strutil/strutil.go | Centralizes SessionSuffix and DeepCloneJSON into the main strutil helper file. |
| internal/strutil/session_suffix.go | Removes the standalone SessionSuffix helper file after consolidation. |
| internal/strutil/json_clone.go | Removes the standalone DeepCloneJSON helper file after consolidation. |
| internal/server/system_tools.go | Adds sysListServersHandler next to SysServer and sys tool logic. |
| internal/server/session.go | Removes sysListServersHandler from session lifecycle code. |
| internal/logger/doc.go | Documents rationale for per-type setup/error-handler functions in logger implementations. |
| internal/guard/wasm_validate.go | Delegates integrity validation to exported config helper and removes duplicate logic. |
| internal/guard/wasm_validate_test.go | Updates assertions to the new unified integrity error message format. |
| internal/guard/wasm_test.go | Updates integrity error substring expectations to the new format. |
| internal/guard/guard_test.go | Updates integrity validation error expectations to the new format. |
| internal/config/guard_policy_validation.go | Exports ValidateAndNormalizeIntegrityField and updates call sites. |
| internal/config/guard_policy_test.go | Updates tests to call the exported integrity validation helper. |
Copilot's findings
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 12/12 changed files
- Comments generated: 2
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
|
@copilot fix this failing ci check https://github.com/github/gh-aw-mcpg/actions/runs/27831905869/job/82370502865?pr=7761 |
…els() in flags.go
Fixed in the latest commit. |
…ard.AllowedIntegrityLevels removal
Fixed. |
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Four targeted refactors from the automated semantic clustering analysis of 896 functions across 23 packages.
Changes
server/session.go→server/system_tools.go: MovesysListServersHandlerout of the session lifecycle file. It has no session-management logic — it validates a session exists then delegates tocallAndLogSysTool, which belongs alongside theSysServertype and registration helpers it works with.strutilconsolidation: Deletesession_suffix.go(1 fn) andjson_clone.go(1 fn); mergeSessionSuffixandDeepCloneJSONintostrutil.gowhere the other general-purpose helpers live.Single source of truth for integrity validation: Export
config.ValidateAndNormalizeIntegrityField(was unexportedvalidateAndNormalizeIntegrityField) and remove the near-duplicateinvalidIntegrityFieldError+ inlineNormalizeIntegrityLevelcall fromguard/wasm_validate.go. Guard now delegates entirely to config:Error message format unified to
"<field> must be one of: none, unapproved, approved, merged". Affected tests updated accordingly.logger/doc.go: Add explanation of why per-typesetup*/handleError*functions are intentional rather than collapsed — each logger type has unique initialization behaviour (e.g.JSONLLoggerhas no fallback,ToolsLoggercloses its file immediately after opening).