Skip to content

Refactor: Move TruncateSessionID to sanitize package#776

Closed
Copilot wants to merge 4 commits intomainfrom
copilot/refactor-semantic-function-clustering
Closed

Refactor: Move TruncateSessionID to sanitize package#776
Copilot wants to merge 4 commits intomainfrom
copilot/refactor-semantic-function-clustering

Conversation

Copy link
Contributor

Copilot AI commented Feb 7, 2026

Refactoring Plan

High Priority (Issue #1) ✅

  • Move TruncateSessionID() from internal/auth/header.go to internal/logger/sanitize/sanitize.go
  • Add tests for TruncateSessionID() in sanitize_test.go
  • Update all imports in internal/server/sdk_logging.go from auth.TruncateSessionID to sanitize.TruncateSessionID
  • Run tests to verify no functionality broken
  • Fix linting error: Update TestTruncateSessionID in header_test.go to call sanitize.TruncateSessionID
  • Sync header_test.go with main branch format to prevent merge conflicts

Medium Priority (Issue #2) ✅

  • Add documentation comment to runDockerInspect() explaining it's intentionally placed in validation package

Verification ✅

  • Run make test-unit to verify all tests pass
  • Run make build to ensure compilation works
  • Run make lint to verify code quality
Original prompt

This section details on the original issue you should resolve

<issue_title>[refactor] Semantic Function Clustering - High-Priority Refactoring Opportunities</issue_title>
<issue_description># 🔧 Semantic Function Clustering Analysis

Analysis performed on repository: github/gh-aw-mcpg
Analysis date: 2026-02-05
Total Go files analyzed: 62 files (39 non-test files)

Executive Summary

The codebase demonstrates excellent overall organization with clear package boundaries and minimal code duplication. The analysis identified 2 high-priority refactoring opportunities and several minor organizational improvements. Most patterns follow Go idioms and domain-driven design principles.

Key Findings:

  • Strong package cohesion - Most files serve a single, clear purpose
  • Minimal duplication - Most "similar" functions serve different domains
  • ⚠️ 1 misplaced function - TruncateSessionID in auth package (used only by logger)
  • ⚠️ 1 outlier function - runDockerInspect in validation package (Docker operation)
  • Well-organized helpers - Domain-specific helpers properly placed

Function Inventory Summary

Packages Analyzed

Package Files Primary Purpose Organization Quality
internal/auth 1 Authentication header parsing ✅ Good (1 outlier)
internal/cmd 8 CLI commands and flags ✅ Excellent
internal/config 8 Configuration loading/validation ⚠️ Good (1 outlier)
internal/difc 5 DIFC security labels ✅ Excellent
internal/guard 4 Security guards ✅ Excellent
internal/launcher 3 Backend process management ✅ Excellent
internal/logger 13 Logging framework ✅ Excellent
internal/mcp 2 MCP protocol types ✅ Excellent
internal/middleware 1 Request middleware ✅ Excellent
internal/server 10 HTTP server implementation ✅ Excellent
internal/sys 1 System utilities ✅ Good
internal/testutil 4 Testing utilities ✅ Excellent
internal/timeutil 1 Time formatting ✅ Excellent
internal/tty 2 TTY detection ✅ Excellent
internal/version 1 Version management ✅ Excellent

Total Functions Cataloged: ~350 functions and methods
Semantic Clusters Identified: 9 major patterns
High-Priority Issues Found: 2
Duplicates Detected: 0 (all similar functions serve different domains)


Identified Issues

1. ⚠️ Misplaced Function: TruncateSessionID

Priority: 🔴 HIGH
Impact: Medium - Code organization and discoverability

Problem

TruncateSessionID() function is defined in internal/auth/header.go but is exclusively used by the logger package (internal/server/sdk_logging.go, 7 occurrences). This violates the principle of placing functions close to their usage.

Current Location

// File: internal/auth/header.go:172
func TruncateSessionID(sessionID string) string {
    if len(sessionID) <= 8 {
        return sessionID
    }
    return sessionID[:8] + "..."
}

Usage Analysis

All 7 calls are in internal/server/sdk_logging.go:

  • Line 54: Logging session comparison
  • Line 70: JSON-RPC request logging
  • Line 111: Error response logging
  • Lines 119-120: Session ID diagnostics
  • Lines 125-126: Session mismatch logging

Evidence

$ grep -r "TruncateSessionID" internal/ --include="*.go" | grep -v "_test.go"
internal/server/sdk_logging.go:54:     mode, auth.TruncateSessionID(sessionID), ...
internal/server/sdk_logging.go:70:     mode, jsonrpcReq.Method, auth.TruncateSessionID(sessionID))
internal/server/sdk_logging.go:111:    mode, jsonrpcReq.Method, auth.TruncateSessionID(sessionID), ...
internal/server/sdk_logging.go:119:    logSDK.Printf("    Session ID: %s", auth.TruncateSessionID(sessionID))
internal/server/sdk_logging.go:120:    logSDK.Printf("    MCP-Session-Id header: %s", auth.TruncateSessionID(mcpSessionID))
internal/server/sdk_logging.go:125:    mode, jsonrpcReq.Method, auth.TruncateSessionID(sessionID), ...
internal/server/sdk_logging.go:126:    auth.TruncateSessionID(mcpSessionID), errorMsg)
internal/auth/header.go:172:func TruncateSessionID(sessionID string) string {

Recommendation

Move TruncateSessionID to internal/logger/sanitize/sanitize.go

Benefits:

  1. ✅ Collocates with similar function TruncateSecret() (same purpose, different length)
  2. ✅ Places function near its only usage (logger package)
  3. ✅ Improves discoverability - all truncation helpers in one place
  4. ✅ Maintains consistency with existing sanitize package purpose

Implementation Steps:

  1. Move function from internal/auth/header.go to internal/logger/sanitize/sanitize.go
  2. Update imports in internal/server/sdk_logging.go: auth.TruncateSessionIDsanitize.TruncateSessionID
  3. Optionally: Parameterize truncation length for reusability
    func TruncateSt...

Custom agent used: agentic-workflows
GitHub Agentic Workflows (gh-aw) - Create, debug, and upgrade AI-powered workflows with intelligent prompt routing


💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.

…DockerInspect

- Moved TruncateSessionID() from internal/auth/header.go to internal/logger/sanitize/sanitize.go
- Added comprehensive tests for TruncateSessionID in sanitize_test.go (8 test cases)
- Updated all imports in internal/server/sdk_logging.go (7 occurrences)
- Added documentation to runDockerInspect explaining its intentional placement
- Updated package documentation in sanitize.go to mention TruncateSessionID
- All tests passing, build successful

Co-authored-by: lpcox <15877973+lpcox@users.noreply.github.com>
Copilot AI changed the title [WIP] Refactor semantic function clustering analysis Refactor: Move TruncateSessionID to sanitize package Feb 7, 2026
Copilot AI requested a review from lpcox February 7, 2026 01:31
@lpcox
Copy link
Collaborator

lpcox commented Feb 7, 2026

@copilot fix these linting errors

make lint
shell: /usr/bin/bash -e {0}
env:
GOTOOLCHAIN: local
Running linters...

github.com/github/gh-aw-mcpg/internal/auth

[github.com/github/gh-aw-mcpg/internal/auth]

Error: vet: internal/auth/header_test.go:441:11: undefined: TruncateSessionID
make: *** [Makefile:24: lint] Error 1
Error: Process completed with exit code 2.

…uncateSessionID

The test was added in main branch but was calling the function directly from the
auth package. Since the function was moved to sanitize package, the test now
correctly calls sanitize.TruncateSessionID.

Co-authored-by: lpcox <15877973+lpcox@users.noreply.github.com>
Copy link
Contributor Author

Copilot AI commented Feb 7, 2026

@copilot fix these linting errors

make lint
...

Fixed in e574f74. The main branch had a TestTruncateSessionID test that called TruncateSessionID directly from the auth package. Updated the test to call sanitize.TruncateSessionID since the function was moved to the sanitize package.

@lpcox
Copy link
Collaborator

lpcox commented Feb 7, 2026

@copilot please fix these linting errors

make lint
shell: /usr/bin/bash -e {0}
env:
GOTOOLCHAIN: local
Running linters...

github.com/github/gh-aw-mcpg/internal/auth

[github.com/github/gh-aw-mcpg/internal/auth]

Error: vet: internal/auth/header_test.go:447:6: TestTruncateSessionID redeclared in this block
make: *** [Makefile:24: lint] Error 1
Error: Process completed with exit code 2.

…teSessionID

The main branch was updated with TestTruncateSessionID that calls
TruncateSessionID directly. This caused a redeclaration error when
CI merged main into this PR branch. Updated the file to match main
branch's format and style while using sanitize.TruncateSessionID.

Co-authored-by: lpcox <15877973+lpcox@users.noreply.github.com>
Copy link
Contributor Author

Copilot AI commented Feb 7, 2026

@copilot please fix these linting errors

make lint
...

Fixed in f61fea8. The main branch was updated with a TestTruncateSessionID test that conflicts with this PR. Synced header_test.go to match main branch's format while using sanitize.TruncateSessionID instead of the direct call.

@lpcox lpcox closed this Feb 7, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[refactor] Semantic Function Clustering - High-Priority Refactoring Opportunities

2 participants