Skip to content

refactor: promote MarshalAndSanitize to sanitize package#4742

Merged
lpcox merged 3 commits intomainfrom
copilot/refactor-semantic-function-clustering
Apr 28, 2026
Merged

refactor: promote MarshalAndSanitize to sanitize package#4742
lpcox merged 3 commits intomainfrom
copilot/refactor-semantic-function-clustering

Conversation

Copy link
Copy Markdown
Contributor

Copilot AI commented Apr 28, 2026

The marshalAndSanitizeForLog helper in server/tool_registry.go duplicated a marshal+sanitize pattern that belongs in internal/logger/sanitize. Two other items from the same analysis (strutil.RandomHexWithFallback and its use in middleware.generateRandomID) were already implemented.

Changes

  • internal/logger/sanitize/sanitize.go — adds MarshalAndSanitize(value any) string, a public best-effort helper that marshals to JSON and redacts secrets via SanitizeString
  • internal/server/tool_registry.go — removes the local marshalAndSanitizeForLog wrapper; both call sites now use sanitize.MarshalAndSanitize
  • internal/server/tool_registry_test.go — updates the coverage test to call the promoted public function
// Before: unexported, duplicated in tool_registry.go
func marshalAndSanitizeForLog(value interface{}) string {
    resultJSON, _ := json.Marshal(value)
    return sanitize.SanitizeString(string(resultJSON))
}

// After: public, lives in the right package
func MarshalAndSanitize(value any) string {
    resultJSON, _ := json.Marshal(value)
    return SanitizeString(string(resultJSON))
}

Warning

Firewall rules blocked me from connecting to one or more addresses (expand for details)

I tried to connect to the following addresses, but was blocked by firewall rules:

  • example.com
    • Triggering command: /tmp/go-build2454462384/b513/launcher.test /tmp/go-build2454462384/b513/launcher.test -test.testlogfile=/tmp/go-build2454462384/b513/testlog.txt -test.paniconexit0 -test.timeout=10m0s /tmp/go-build2454462384/b492/_pkg_.a g_.a -I x_amd64/vet --gdwarf-5 serviceconfig lcache/go/1.25.9-bool x_amd64/vet -W .cfg 509129435/b314//-ifaceassert x_amd64/vet . --gdwarf2 9129435/b314/ x_amd64/vet (dns block)
  • invalid-host-that-does-not-exist-12345.com
    • Triggering command: /tmp/go-build2454462384/b495/config.test /tmp/go-build2454462384/b495/config.test -test.testlogfile=/tmp/go-build2454462384/b495/testlog.txt -test.paniconexit0 -test.timeout=10m0s /tmp/go-build2454462384/b396/vet.cfg o/otlp@v1.10.0/trace/v1/trace.pb.go om/tetratelabs/wazero@v1.11.0/api/wasm.go x_amd64/vet --gdwarf-5 --64 -o x_amd64/vet -I g_.a -I x_amd64/vet --gdwarf-5 /grpc_binarylog_-atomic -o x_amd64/vet (dns block)
  • nonexistent.local
    • Triggering command: /tmp/go-build2454462384/b513/launcher.test /tmp/go-build2454462384/b513/launcher.test -test.testlogfile=/tmp/go-build2454462384/b513/testlog.txt -test.paniconexit0 -test.timeout=10m0s /tmp/go-build2454462384/b492/_pkg_.a g_.a -I x_amd64/vet --gdwarf-5 serviceconfig lcache/go/1.25.9-bool x_amd64/vet -W .cfg 509129435/b314//-ifaceassert x_amd64/vet . --gdwarf2 9129435/b314/ x_amd64/vet (dns block)
  • slow.example.com
    • Triggering command: /tmp/go-build2454462384/b513/launcher.test /tmp/go-build2454462384/b513/launcher.test -test.testlogfile=/tmp/go-build2454462384/b513/testlog.txt -test.paniconexit0 -test.timeout=10m0s /tmp/go-build2454462384/b492/_pkg_.a g_.a -I x_amd64/vet --gdwarf-5 serviceconfig lcache/go/1.25.9-bool x_amd64/vet -W .cfg 509129435/b314//-ifaceassert x_amd64/vet . --gdwarf2 9129435/b314/ x_amd64/vet (dns block)
  • this-host-does-not-exist-12345.com
    • Triggering command: /tmp/go-build2454462384/b522/mcp.test /tmp/go-build2454462384/b522/mcp.test -test.testlogfile=/tmp/go-build2454462384/b522/testlog.txt -test.paniconexit0 -test.timeout=10m0s .cfg�� olang.org/grpc@v-errorsas ZnYX/DVOjGTTnGVe-ifaceassert x_amd64/vet . telabs/wazero --64 x_amd64/vet .cfg�� 9129435/b392/_pkg_.a ache/go/1.25.9/x64/src/net/http/-c=4 x_amd64/vet --gdwarf-5 g/protobuf/interrun -o x_amd64/vet (dns block)

If you need me to access, download, or install something from one of these locations, you can either:

…exWithFallback in middleware

Agent-Logs-Url: https://github.com/github/gh-aw-mcpg/sessions/750f30d1-5ed7-4441-80e3-c41c31f716a9

Co-authored-by: lpcox <15877973+lpcox@users.noreply.github.com>
Copilot AI changed the title [WIP] Refactor semantic function clustering analysis refactor: promote MarshalAndSanitize to sanitize package Apr 28, 2026
Copilot AI requested a review from lpcox April 28, 2026 15:39
@lpcox lpcox marked this pull request as ready for review April 28, 2026 15:40
Copilot AI review requested due to automatic review settings April 28, 2026 15:40
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Refactors best-effort “marshal to JSON then redact secrets” logging logic by promoting the helper from internal/server into the shared internal/logger/sanitize package, and updates call sites/tests to use the shared implementation.

Changes:

  • Add sanitize.MarshalAndSanitize(value any) string as a public helper for best-effort JSON marshal + secret redaction.
  • Replace marshalAndSanitizeForLog usage in tool_registry.go with the promoted helper and remove the local wrapper.
  • Update the existing redaction test to call the promoted helper.
Show a summary per file
File Description
internal/logger/sanitize/sanitize.go Adds shared MarshalAndSanitize helper for safe-to-log JSON serialization.
internal/server/tool_registry.go Switches response logging to use sanitize.MarshalAndSanitize and removes the duplicated local helper.
internal/server/tool_registry_test.go Updates redaction coverage test to use the promoted helper.

Copilot's findings

Tip

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

  • Files reviewed: 3/3 changed files
  • Comments generated: 1

Comment thread internal/server/tool_registry_test.go Outdated
Comment on lines +500 to +504
@@ -500,7 +501,7 @@ func TestMarshalAndSanitizeForLog_RedactsSecrets(t *testing.T) {
assert := assert.New(t)

const secret = "ghp_1234567890123456789012345678901234567890"
sanitized := marshalAndSanitizeForLog(map[string]interface{}{
sanitized := sanitize.MarshalAndSanitize(map[string]interface{}{
@lpcox
Copy link
Copy Markdown
Collaborator

lpcox commented Apr 28, 2026

@copilot addresss the review feedback

Copy link
Copy Markdown
Contributor Author

Copilot AI commented Apr 28, 2026

@copilot addresss the review feedback

Done in eee888b — renamed TestMarshalAndSanitizeForLog_RedactsSecrets to TestSanitizeMarshalAndSanitize_RedactsSecrets to match the promoted sanitize.MarshalAndSanitize function.

@lpcox lpcox merged commit 580b0ec into main Apr 28, 2026
16 checks passed
@lpcox lpcox deleted the copilot/refactor-semantic-function-clustering branch April 28, 2026 16:53
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 Analysis: Refactoring Opportunities

3 participants