refactor: semantic function clustering improvements (Finding 1–4) by Copilot · Pull Request #8507 · github/gh-aw-mcpg

Copilot · 2026-07-02T22:31:39Z

Addresses all 4 actionable findings from the automated semantic analysis of 146 non-test Go source files (#8499).

Finding 1 — `PositiveInteger` delegates to `TimeoutMinimum`

internal/config/validation_rules.go — eliminates duplicated < 1 guard; matches the pattern already used by TimeoutPositive. Error Message and Suggestion text preserved via override:

// Before: independent < 1 guard
if value < 1 { return &ValidationError{...} }

// After: delegates, then customises the returned error
if err := TimeoutMinimum(value, 1, fieldName, jsonPath); err != nil {
    err.Message    = fmt.Sprintf("%s must be a positive integer (>= 1), got %d", ...)
    err.Suggestion = fmt.Sprintf("Use a positive integer (>= 1) for %s", ...)
    return err
}

Finding 2 — `internal/strutil` → `internal/util`

Package renamed to util: the package contains duration formatting, cryptographic randomness, JSON deep-clone, and collection utilities — none of which are string-specific. All ~26 import sites updated (production, tests, AGENTS.md, CONTRIBUTING.md).

Finding 3 — Logger factory functions inlined

file_logger.go, markdown_logger.go, jsonl_logger.go, tools_logger.go, observed_url_domains_logger.go — 10 named functions (setupX / handleXError, 2 per logger × 5 loggers) that were each referenced exactly once as struct field values are inlined as anonymous functions directly in their loggerFactory var declarations. Tests updated to use factory.setup(...) / factory.onError(...).

Finding 4 — Route arg-key constants in `proxy/router.go`

Seven constants (argOwner, argRepo, argPullNumber, argIssueNumber, argMethod, argResourceID, argDiscussionNumber) replace ~20 scattered bare string literals across the route table and MatchRoute, providing a single source of truth and typo protection.

Finding 1: Refactor PositiveInteger to delegate to TimeoutMinimum, preserving message/suggestion text by overriding after delegation. Finding 2: Rename internal/strutil package to internal/util to better reflect its general-purpose nature (not purely string utilities). Update all ~26 import sites across production code, tests, and documentation. Finding 3: Inline setup*/handle*Error named functions into logger factory var declarations for file, markdown, jsonl, tools, and observed-url-domains loggers. Update tests to call factory.setup/factory.onError instead. Finding 4: Add string constants (argOwner, argRepo, argPullNumber, etc.) in internal/proxy/router.go, replacing ~20 repeated bare string literals. Closes #8499

Copilot

Pull request overview

Refactors several small areas identified by semantic analysis to reduce duplication, improve naming/discoverability, and tighten up routing/logging internals without changing overall gateway behavior.

Changes:

Refactors PositiveInteger to delegate to TimeoutMinimum while preserving its specific error message/suggestion wording.
Renames internal/strutil to internal/util and updates imports/usages across the codebase.
Inlines single-use logger factory helpers and centralizes proxy router arg keys as constants.

Show a summary per file

File	Description
internal/config/validation_rules.go	Refactors `PositiveInteger` to reuse `TimeoutMinimum` and then override message/suggestion text.
internal/config/guard_policy_validation.go	Updates imports/usages for the `internal/util` rename.
internal/util/util.go	Renames package declaration to `util`.
internal/util/util_test.go	Renames package declaration to `util` in tests.
internal/util/truncate.go	Renames package declaration to `util`.
internal/util/truncate_test.go	Renames package declaration to `util` in tests.
internal/util/random.go	Renames package declaration to `util`.
internal/util/random_test.go	Renames package declaration to `util` in tests.
internal/util/format_duration.go	Renames package + updates comment wording to match `util`.
internal/util/format_duration_test.go	Renames package declaration to `util` in tests.
internal/util/collections.go	Renames package declaration to `util`.
internal/util/collections_test.go	Renames package declaration to `util` in tests.
internal/urlutil/domains.go	Updates `SortedSetKeys` import/usage for `internal/util`.
internal/tracing/provider.go	Updates randomness helper import/usage for `internal/util`.
internal/server/session.go	Updates truncation helper import/usage for `internal/util`.
internal/server/guard_init.go	Updates `CopyTrimmedStringIntMap` import/usage for `internal/util`.
internal/server/difc_log.go	Updates map extraction helpers import/usage for `internal/util`.
internal/server/copy_tool_call_limits_test.go	Updates `CopyTrimmedStringIntMap` test usage to `internal/util`.
internal/server/circuit_breaker.go	Updates duration formatting helper import/usage for `internal/util`.
internal/sanitize/sanitize.go	Updates truncation helper import/usage for `internal/util`.
internal/proxy/router.go	Centralizes common arg keys as constants and updates route arg maps; updates helper imports to `internal/util`.
internal/proxy/response_transform.go	Updates `DeepCloneJSON` import/usage for `internal/util`.
internal/proxy/response_transform_test.go	Updates `DeepCloneJSON` test usage for `internal/util`.
internal/proxy/response_transform_coverage_test.go	Updates `DeepCloneJSON` coverage test usage for `internal/util`.
internal/proxy/proxy_test.go	Updates `DeepCloneJSON` test usage for `internal/util`.
internal/proxy/handler.go	Updates truncation helper import/usage for `internal/util`.
internal/proxy/graphql.go	Updates map extraction + truncation helper imports/usages for `internal/util`.
internal/middleware/jqschema.go	Updates random query ID helper import/usage for `internal/util`.
internal/mcp/http_transport.go	Updates truncation helper import/usage for `internal/util`.
internal/logger/tools_logger.go	Inlines factory setup/onError functions in the logger factory declaration.
internal/logger/rpc_format.go	Updates truncation helper import/usage for `internal/util`.
internal/logger/observed_url_domains_logger.go	Inlines factory setup/onError functions; updates sorting helper import/usage for `internal/util`.
internal/logger/observed_url_domains_logger_test.go	Updates tests to call `observedURLDomainsLoggerFactory.setup(...)` directly.
internal/logger/markdown_logger.go	Inlines factory setup/onError functions in the logger factory declaration.
internal/logger/markdown_logger_close_coverage_test.go	Updates tests to call `markdownLoggerFactory.setup/onError(...)` directly.
internal/logger/logger.go	Updates duration formatting helper import/usage for `internal/util`.
internal/logger/log_cleanup_test.go	Updates truncation helper import/usage for `internal/util`.
internal/logger/jsonl_logger.go	Inlines factory setup/onError functions in the logger factory declaration.
internal/logger/file_logger.go	Inlines factory setup/onError functions in the logger factory declaration.
internal/logger/fallback_handlers_test.go	Updates tests to use `*.onError(...)` factory functions.
internal/guard/wasm_payload.go	Updates slice conversion helper import/usage for `internal/util`.
internal/githubhttp/collaborator.go	Updates map extraction helper import/usage for `internal/util`.
internal/difc/sink_server_ids.go	Updates deduplication helper import/usage for `internal/util`.
internal/auth/header.go	Updates random agent ID helper import/usage for `internal/util`.
CONTRIBUTING.md	Updates documentation references from `strutil` → `util`.
AGENTS.md	Updates documentation references from `strutil` → `util`.

Review details

Tip

Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Files reviewed: 46/46 changed files
Comments generated: 9
Review effort level: Low

@@ -12,7 +12,7 @@ func TestHandleFileLoggerError_UsesFallback(t *testing.T) {
 	var logger *FileLogger
 	var err error
 	_ = captureStdLog(t, func() {
-		logger, err = handleFileLoggerError(assert.AnError, "/tmp/logs", "test.log")
+		logger, err = fileLoggerFactory.onError(assert.AnError, "/tmp/logs", "test.log")


@@ -26,7 +26,7 @@ func TestHandleMarkdownLoggerError_UsesFallback(t *testing.T) {
 	var logger *MarkdownLogger
 	var err error
 	_ = captureStdLog(t, func() {
-		logger, err = handleMarkdownLoggerError(assert.AnError, "/tmp/logs", "test.md")
+		logger, err = markdownLoggerFactory.onError(assert.AnError, "/tmp/logs", "test.md")


@@ -39,7 +39,7 @@ func TestHandleToolsLoggerError_UsesFallback(t *testing.T) {
 	var logger *ToolsLogger
 	var err error
 	_ = captureStdLog(t, func() {
-		logger, err = handleToolsLoggerError(assert.AnError, "/tmp/logs", "tools.json")
+		logger, err = toolsLoggerFactory.onError(assert.AnError, "/tmp/logs", "tools.json")


@@ -75,7 +75,7 @@ func TestMarkdownLogger_Close_FooterWriteError(t *testing.T) {
 // Close() should return nil.
 func TestMarkdownLogger_Close_NilLogFile(t *testing.T) {
 	tmpDir := t.TempDir()
-	ml, err := handleMarkdownLoggerError(os.ErrNotExist, tmpDir, "test.md")
+	ml, err := markdownLoggerFactory.onError(os.ErrNotExist, tmpDir, "test.md")


 	// Create a directory at the target file path so atomicWriteFile's Rename fails.
 	targetDir := filepath.Join(tmpDir, observedURLDomainsFileName)
 	require.NoError(t, os.MkdirAll(targetDir, 0755))

-	l, err := setupObservedURLDomainsLogger(nil, tmpDir, observedURLDomainsFileName)
+	l, err := observedURLDomainsLoggerFactory.setup(nil, tmpDir, observedURLDomainsFileName)


 - `internal/sanitize/` - Sensitive data redaction utilities (`SanitizeString`, `SanitizeJSON`, `TruncateSecret`) for safe log output
 - `internal/server/` - HTTP server (routed/unified modes)
- `internal/strutil/` - String and formatting utilities
+- `internal/util/` - String and formatting utilities


    ├── sanitize/              # Sensitive data redaction utilities for logging
    ├── server/                # HTTP server (routed/unified modes)
-    ├── strutil/               # String and formatting utility helpers (deduplication, trimming, duration formatting, JSON deep-clone)
+    ├── util/               # String and formatting utility helpers (deduplication, trimming, duration formatting, JSON deep-clone)


 - **`internal/sanitize/`** - Sensitive data redaction utilities (`SanitizeString`, `SanitizeJSON`, `TruncateSecret`) for safe log output
 - **`internal/server/`** - HTTP server with routed and unified modes
- **`internal/strutil/`** - String and formatting utility helpers (deduplication, trimming, duration formatting, JSON deep-clone)
+- **`internal/util/`** - String and formatting utility helpers (deduplication, trimming, duration formatting, JSON deep-clone)


lpcox · 2026-07-02T22:50:43Z

@copilot address review feedback

Copilot · 2026-07-02T22:53:10Z

@copilot address review feedback

Done in the latest commit. Changes made:

Renamed the three test functions in fallback_handlers_test.go to TestFileLoggerFactory_OnError_UsesFallback, TestMarkdownLoggerFactory_OnError_UsesFallback, and TestToolsLoggerFactory_OnError_UsesFallback.
Updated the comment in markdown_logger_close_coverage_test.go to reference markdownLoggerFactory.onError.
Renamed TestSetupObservedURLDomainsLogger_WriteToFileFails → TestObservedURLDomainsLoggerFactory_Setup_WriteToFileFails and updated the assertion message to reference observedURLDomainsLoggerFactory.setup.
Updated AGENTS.md and both CONTRIBUTING.md entries for internal/util/ to accurately reflect that the package includes string/formatting, randomness, and JSON deep-clone utilities.

lpcox · 2026-07-02T23:04:09Z

@copilot resolve the merge conflicts in this pull request

…ntic-function-clustering # Conflicts: # internal/config/validation_rules.go

Copilot · 2026-07-02T23:08:02Z

@copilot resolve the merge conflicts in this pull request

Merged origin/main into the branch. The only conflict was in internal/config/validation_rules.go — the PositiveInteger function. Main had introduced a newValidationError helper and used it directly in PositiveInteger, while our PR delegates to TimeoutMinimum (Finding 1). Resolved by keeping the delegation approach. Build and tests pass.

Initial plan

931d991

Copilot AI assigned Copilot and lpcox Jul 2, 2026

Copilot started work on behalf of lpcox July 2, 2026 22:33 View session

Copilot AI linked an issue Jul 2, 2026 that may be closed by this pull request

[refactor] Semantic Function Clustering Analysis: Code Organization and Refactoring Opportunities #8499

Closed

4 tasks

GitHub Advanced Security started work on behalf of lpcox July 2, 2026 22:33 View session

GitHub Advanced Security finished work on behalf of lpcox July 2, 2026 22:34

Copilot AI changed the title ~~[WIP] Refactor semantic function clustering analysis for code organization~~ refactor: semantic function clustering improvements (Finding 1–4) Jul 2, 2026

lpcox marked this pull request as ready for review July 2, 2026 22:43

Copilot AI review requested due to automatic review settings July 2, 2026 22:43

Copilot finished work on behalf of lpcox July 2, 2026 22:43

Copilot AI requested a review from lpcox July 2, 2026 22:43

Copilot started reviewing on behalf of lpcox July 2, 2026 22:43 View session

Copilot AI reviewed Jul 2, 2026

View reviewed changes

GitHub Advanced Security started work on behalf of lpcox July 2, 2026 22:49 View session

GitHub Advanced Security finished work on behalf of lpcox July 2, 2026 22:50

Copilot started work on behalf of lpcox July 2, 2026 22:51 View session

fix: update stale test names and doc descriptions per review feedback

b6d76d5

GitHub Advanced Security started work on behalf of lpcox July 2, 2026 22:53 View session

Copilot finished work on behalf of lpcox July 2, 2026 22:53

GitHub Advanced Security finished work on behalf of lpcox July 2, 2026 22:54

Copilot started work on behalf of lpcox July 2, 2026 23:04 View session

Merge remote-tracking branch 'origin/main' into copilot/refactor-sema…

de8f9a5

…ntic-function-clustering # Conflicts: # internal/config/validation_rules.go

GitHub Advanced Security started work on behalf of lpcox July 2, 2026 23:08 View session

Copilot finished work on behalf of lpcox July 2, 2026 23:08

GitHub Advanced Security finished work on behalf of lpcox July 2, 2026 23:09

lpcox merged commit d3050f7 into main Jul 2, 2026
24 checks passed

lpcox deleted the copilot/refactor-semantic-function-clustering branch July 2, 2026 23:11

Copilot AI mentioned this pull request Jul 3, 2026

fix(middleware): resolve CompileToolResponseFilter/WithVars structural duplication #8602

Closed

github-actions Bot mentioned this pull request Jul 4, 2026

[ca] CLI Version Updates: Claude Code 2.1.201, MCP Gateway v0.3.33 github/gh-aw#43333

Closed

Copilot AI mentioned this pull request Jul 4, 2026

[ca] CLI Version Updates: Claude Code 2.1.201, MCP Gateway v0.3.33 github/gh-aw#43358

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

refactor: semantic function clustering improvements (Finding 1–4) - #8507

refactor: semantic function clustering improvements (Finding 1–4)#8507
lpcox merged 4 commits into
mainfrom
copilot/refactor-semantic-function-clustering

Copilot AI commented Jul 2, 2026 •

edited

Loading

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

lpcox commented Jul 2, 2026

Uh oh!

Copilot AI commented Jul 2, 2026

Uh oh!

lpcox commented Jul 2, 2026

Uh oh!

Copilot AI commented Jul 2, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Uh oh!

Conversation

Copilot AI commented Jul 2, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Finding 1 — PositiveInteger delegates to TimeoutMinimum

Finding 2 — internal/strutil → internal/util

Finding 3 — Logger factory functions inlined

Finding 4 — Route arg-key constants in proxy/router.go

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Review details

Uh oh!

Uh oh!

lpcox commented Jul 2, 2026

Uh oh!

Copilot AI commented Jul 2, 2026

Uh oh!

lpcox commented Jul 2, 2026

Uh oh!

Copilot AI commented Jul 2, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Copilot AI commented Jul 2, 2026 •

edited

Loading

Finding 1 — `PositiveInteger` delegates to `TimeoutMinimum`

Finding 2 — `internal/strutil` → `internal/util`

Finding 4 — Route arg-key constants in `proxy/router.go`