Skip to content

Refactor duplicated marshal/log paths and unify sanitized sys tool response logging#4106

Merged
lpcox merged 3 commits intomainfrom
copilot/duplicate-code-analysis-report
Apr 18, 2026
Merged

Refactor duplicated marshal/log paths and unify sanitized sys tool response logging#4106
lpcox merged 3 commits intomainfrom
copilot/duplicate-code-analysis-report

Conversation

Copy link
Copy Markdown
Contributor

Copilot AI commented Apr 18, 2026

Recent SendRequestWithServerID cleanup left three duplication/safety gaps: repeated marshal-for-debug blocks, duplicated sys tool handler skeletons, and unsanitized sys_list_servers result logging. This PR consolidates those paths and closes the sanitization gap.

  • Consolidate marshal-for-debug logic

    • Added logger.LogMarshaledForDebug(...) in internal/logger/marshal_debug.go.
    • Replaced repeated marshal+error/success logging blocks in:
      • internal/guard/wasm.go
      • internal/server/guard_init.go
    • Result: one shared marshaling pattern for debug logging across guard/server paths.
  • Unify sys tool call/error/log skeleton

    • Added callAndLogSysTool(...) in internal/server/tool_registry.go.
    • sysInitHandler and sysListHandler now share one call path (call sys server → handle error → marshal/log result → return).
  • Fix sanitization gap in sys_list_servers logging

    • Added shared marshalAndSanitizeForLog(...) in tool_registry.go.
    • sys_list_servers response logs now use sanitize.SanitizeString(...), matching sys_init and backend tool logging behavior.
  • Focused test coverage

    • Added helper tests for marshal debug utility:
      • internal/logger/marshal_debug_test.go
    • Added server tests for log sanitization/error behavior:
      • internal/server/tool_registry_test.go
func (us *UnifiedServer) callAndLogSysTool(sessionID, operationName, sysToolName string) (*sdk.CallToolResult, interface{}, error) {
	result, err := us.callSysServer(sysToolName)
	if err != nil {
		logger.LogError("client", "MCP %s call failed, session=%s, error=%v", operationName, sessionID, err)
		return mcp.NewErrorCallToolResult(err)
	}
	logger.LogInfo("client", "MCP %s response, session=%s, result=%s", operationName, sessionID, marshalAndSanitizeForLog(result))
	return nil, result, nil
}

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-build1793350213/b510/launcher.test /tmp/go-build1793350213/b510/launcher.test -test.testlogfile=/tmp/go-build1793350213/b510/testlog.txt -test.paniconexit0 -test.timeout=10m0s -test.v=true 3350213/b184/_pk-errorsas 64/src/log/slog/-ifaceassert bin/as -p zevo/backend/isa-atomic -lang=go1.25 /opt/hostedtoolc-buildtags -o s/attributes.go idRS/k9xnSFeesAm-ifaceassert x_amd64/compile -p crypto/internal/info -lang=go1.25 x_amd64/compile (dns block)
    • Triggering command: /tmp/go-build3990767025/b514/launcher.test /tmp/go-build3990767025/b514/launcher.test -test.testlogfile=/tmp/go-build3990767025/b514/testlog.txt -test.paniconexit0 -test.timeout=10m0s /tmp/go-build3990767025/b495/vet.cfg ry=1 -trimpath x_amd64/vet -p github.com/githupush -lang=go1.25 x_amd64/vet -ato�� .cfg -buildtags 64/pkg/tool/linux_amd64/vet -errorsas -ifaceassert -nilfunc 64/pkg/tool/linu-trimpath (dns block)
    • Triggering command: /tmp/go-build848674421/b514/launcher.test /tmp/go-build848674421/b514/launcher.test -test.testlogfile=/tmp/go-build848674421/b514/testlog.txt -test.paniconexit0 -test.timeout=10m0s know�� l/server/tool_registry_test.go known-linux-gnu/lib/rustlib/x86_--format=format:%H %ct %D stable-x86_64-REDACTED-linux-gnu/--end-of-options 6e1dc71b.rlib ccc.rlib --systemd-cgroup-bool (dns block)
  • invalid-host-that-does-not-exist-12345.com
    • Triggering command: /tmp/go-build1793350213/b492/config.test /tmp/go-build1793350213/b492/config.test -test.testlogfile=/tmp/go-build1793350213/b492/testlog.txt -test.paniconexit0 -test.timeout=10m0s -test.v=true 3350213/b151/_pk-p o x_amd64/compile (dns block)
    • Triggering command: /tmp/go-build3990767025/b496/config.test /tmp/go-build3990767025/b496/config.test -test.testlogfile=/tmp/go-build3990767025/b496/testlog.txt -test.paniconexit0 -test.timeout=10m0s n-me�� g_.a pkg/mod/go.opentelemetry.io/otel/exporters/otlp/otlptrace/otlptracehttp@v1.43.0/doc.go x_amd64/vet -p github.com/segme--norc -lang=go1.17 x_amd64/vet -ato�� MQJZCSQ0B -buildtags x_amd64/vet -errorsas -ifaceassert -nilfunc ks..."; \ elif c--revs (dns block)
    • Triggering command: /tmp/go-build848674421/b496/config.test /tmp/go-build848674421/b496/config.test -test.testlogfile=/tmp/go-build848674421/b496/testlog.txt -test.paniconexit0 -test.timeout=10m0s 05ed�� 05ed-cgu.10.rcgu-errorsas 05ed-cgu.11.rcgu-ifaceassert 05ed-cgu.12.rcgu-nilfunc 05ed-cgu.13.rcgugit 05ed-cgu.14.rcguls-files pt_build-842ae9a--exclude-standard pt_build-842ae9a--others /usr�� --root .rlib 6b2b26a06.rlib 327.rlib 653.rlib 86b29d.rlib ive.12123747d8da05ed-cgu.00.rcgu.o (dns block)
  • nonexistent.local
    • Triggering command: /tmp/go-build1793350213/b510/launcher.test /tmp/go-build1793350213/b510/launcher.test -test.testlogfile=/tmp/go-build1793350213/b510/testlog.txt -test.paniconexit0 -test.timeout=10m0s -test.v=true 3350213/b184/_pk-errorsas 64/src/log/slog/-ifaceassert bin/as -p zevo/backend/isa-atomic -lang=go1.25 /opt/hostedtoolc-buildtags -o s/attributes.go idRS/k9xnSFeesAm-ifaceassert x_amd64/compile -p crypto/internal/info -lang=go1.25 x_amd64/compile (dns block)
    • Triggering command: /tmp/go-build3990767025/b514/launcher.test /tmp/go-build3990767025/b514/launcher.test -test.testlogfile=/tmp/go-build3990767025/b514/testlog.txt -test.paniconexit0 -test.timeout=10m0s /tmp/go-build3990767025/b495/vet.cfg ry=1 -trimpath x_amd64/vet -p github.com/githupush -lang=go1.25 x_amd64/vet -ato�� .cfg -buildtags 64/pkg/tool/linux_amd64/vet -errorsas -ifaceassert -nilfunc 64/pkg/tool/linu-trimpath (dns block)
    • Triggering command: /tmp/go-build848674421/b514/launcher.test /tmp/go-build848674421/b514/launcher.test -test.testlogfile=/tmp/go-build848674421/b514/testlog.txt -test.paniconexit0 -test.timeout=10m0s know�� l/server/tool_registry_test.go known-linux-gnu/lib/rustlib/x86_--format=format:%H %ct %D stable-x86_64-REDACTED-linux-gnu/--end-of-options 6e1dc71b.rlib ccc.rlib --systemd-cgroup-bool (dns block)
  • slow.example.com
    • Triggering command: /tmp/go-build1793350213/b510/launcher.test /tmp/go-build1793350213/b510/launcher.test -test.testlogfile=/tmp/go-build1793350213/b510/testlog.txt -test.paniconexit0 -test.timeout=10m0s -test.v=true 3350213/b184/_pk-errorsas 64/src/log/slog/-ifaceassert bin/as -p zevo/backend/isa-atomic -lang=go1.25 /opt/hostedtoolc-buildtags -o s/attributes.go idRS/k9xnSFeesAm-ifaceassert x_amd64/compile -p crypto/internal/info -lang=go1.25 x_amd64/compile (dns block)
    • Triggering command: /tmp/go-build3990767025/b514/launcher.test /tmp/go-build3990767025/b514/launcher.test -test.testlogfile=/tmp/go-build3990767025/b514/testlog.txt -test.paniconexit0 -test.timeout=10m0s /tmp/go-build3990767025/b495/vet.cfg ry=1 -trimpath x_amd64/vet -p github.com/githupush -lang=go1.25 x_amd64/vet -ato�� .cfg -buildtags 64/pkg/tool/linux_amd64/vet -errorsas -ifaceassert -nilfunc 64/pkg/tool/linu-trimpath (dns block)
    • Triggering command: /tmp/go-build848674421/b514/launcher.test /tmp/go-build848674421/b514/launcher.test -test.testlogfile=/tmp/go-build848674421/b514/testlog.txt -test.paniconexit0 -test.timeout=10m0s know�� l/server/tool_registry_test.go known-linux-gnu/lib/rustlib/x86_--format=format:%H %ct %D stable-x86_64-REDACTED-linux-gnu/--end-of-options 6e1dc71b.rlib ccc.rlib --systemd-cgroup-bool (dns block)
  • this-host-does-not-exist-12345.com
    • Triggering command: /tmp/go-build1793350213/b519/mcp.test /tmp/go-build1793350213/b519/mcp.test -test.testlogfile=/tmp/go-build1793350213/b519/testlog.txt -test.paniconexit0 -test.timeout=10m0s -test.v=true transport/bdp_estimator.go transport/client_stream.go x_amd64/compile -pthread e/otlptracehttp/--version -fmessage-length=0 x_amd64/compile 3350�� g_.a 3350213/b165/ x_amd64/vet --gdwarf-5 mcpgodebug -o x_amd64/vet (dns block)
    • Triggering command: /tmp/go-build3990767025/b523/mcp.test /tmp/go-build3990767025/b523/mcp.test -test.testlogfile=/tmp/go-build3990767025/b523/testlog.txt -test.paniconexit0 -test.timeout=10m0s -ato�� .cfg -buildtags 0278e5566a20dc05-d -errorsas -ifaceassert -nilfunc 64/pkg/tool/linu-buildtags --ve�� .cfg -tests 64/pkg/tool/linu-nilfunc 1024825-9d38bb40/usr/bin/runc.original toml@v1.6.0/depr--version x_amd64/compile 64/pkg/tool/linu-tests (dns block)
    • Triggering command: /tmp/go-build848674421/b523/mcp.test /tmp/go-build848674421/b523/mcp.test -test.testlogfile=/tmp/go-build848674421/b523/testlog.txt -test.paniconexit0 -test.timeout=10m0s ogpt�� q7jvxzdnkys32nfz-errorsas 0ok7l9sshaaz07e8-ifaceassert hw1mmy6a1aekmmsh-nilfunc 4p2kq4dabhq5b2kedocker wmuqycv01jbjtq95inspect ntained/cc stable-x86_64-un{{.Config.OpenStdin}} stab�� stable-x86_64-REDACTED-linux-gnu/lib/rustlib/x86_64-REDACTED-linux-gnu/lib/libobject-926daa94a00ee/usr/libexec/docker/docker-init stable-x86_64-REDACTED-linux-gnu/lib/rustlib/x86_64-REDACTED-linux-gnu/lib/libmemchr-48d5b0db80402--version .1/x64/codeql/tools/linux64/java/lib/jspawnhelper stable-x86_64-un/usr/libexec/docker/cli-plugins/docker-compose 7a5585.0r6f2y9pmdocker-cli-plugin-metadata 7a5585.0y8i0suihruczucboywd9kbz6/tmp/go-build985941549/b001/_pkg_.a .1/x64/codeql/tools/linux64/java-trimpath (dns block)

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

Copilot AI linked an issue Apr 18, 2026 that may be closed by this pull request
Copilot AI and others added 2 commits April 18, 2026 16:23
Copilot AI changed the title [WIP] Fix duplicate code patterns in codebase Refactor duplicated marshal/log paths and unify sanitized sys tool response logging Apr 18, 2026
Copilot AI requested a review from lpcox April 18, 2026 16:28
@lpcox lpcox marked this pull request as ready for review April 18, 2026 17:01
Copilot AI review requested due to automatic review settings April 18, 2026 17:01
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

This PR refactors duplicated JSON-marshal-for-logging code paths and centralizes sys-tool call/error/log handling, while ensuring sys tool responses (notably sys_list_servers) are sanitized before being written to logs.

Changes:

  • Added logger.LogMarshaledForDebug(...) and replaced repeated marshal+log blocks in guard/server DIFC paths.
  • Introduced callAndLogSysTool(...) and marshalAndSanitizeForLog(...) to unify sys tool call flow and sanitize sys tool response logging.
  • Added focused unit tests covering marshal-debug behavior and sanitization/error behavior in the sys-tool helper path.
Show a summary per file
File Description
internal/server/tool_registry.go Consolidates response marshaling+sanitization for logging and unifies sys tool call/log flow.
internal/server/tool_registry_test.go Adds coverage for sanitization redaction and sys-tool error result behavior.
internal/server/guard_init.go Replaces duplicated marshal-for-debug logging with shared helper.
internal/guard/wasm.go Replaces duplicated marshal-for-debug logging with shared helper in WASM guard label flow.
internal/logger/marshal_debug.go Adds shared helper to marshal values for debug logging with success/failure callbacks.
internal/logger/marshal_debug_test.go Tests success and marshal-failure paths for the new marshal-debug helper.

Copilot's findings

Tip

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

  • Files reviewed: 6/6 changed files
  • Comments generated: 0

@lpcox lpcox merged commit a4700bd into main Apr 18, 2026
30 checks passed
@lpcox lpcox deleted the copilot/duplicate-code-analysis-report branch April 18, 2026 17:10
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.

[duplicate-code] Duplicate Code Analysis Report

3 participants