Refactor duplicate code patterns in logger and DIFC packages by Claude · Pull Request #1791 · github/gh-aw-mcpg

Claude · 2026-03-11T19:04:57Z

Addresses duplicate code analysis findings: ~60 lines of repeated mutex-locking and logging patterns across logger and DIFC packages.

Changes

DIFC Tag Mutation (#1767) - 21 lines reduced

Extracted modifyTag helper to consolidate mutex-lock + dual-logging pattern
Refactored AddSecrecyTag, AddIntegrityTag, DropIntegrityTag

// Before: duplicated 7-line pattern in each method
func (a *AgentLabels) AddSecrecyTag(tag Tag) {
    logAgent.Printf("Agent %s adding secrecy tag: %s", a.AgentID, tag)
    a.mu.Lock()
    defer a.mu.Unlock()
    a.Secrecy.Label.Add(tag)
    log.Printf("[DIFC] Agent %s gained secrecy tag: %s", a.AgentID, tag)
}

// After: unified helper
func (a *AgentLabels) AddSecrecyTag(tag Tag) {
    a.modifyTag("secrecy", "adding", "gained", tag, func() {
        a.Secrecy.Label.Add(tag)
    })
}

Global Logger RWMutex Access (#1768) - 40 lines reduced

Created generic withGlobalLogger helper with closableLogger constraint
Refactored 8+ call sites across file_logger, markdown_logger, jsonl_logger, server_file_logger, tools_logger, rpc_logger

// Before: duplicated RWMutex pattern
globalLoggerMu.RLock()
defer globalLoggerMu.RUnlock()
if globalLogger != nil {
    globalLogger.DoSomething(args...)
}

// After: type-safe generic helper
withGlobalLogger(&globalLoggerMu, &globalLogger, func(logger *Logger) {
    logger.DoSomething(args...)
})

Logger Initialization (#1766) - No changes required

Reviewed existing initLogger generic - already well-abstracted
Documented pattern rationale in common.go

Documentation

Added "Global Logger RWMutex Access Pattern" section to internal/logger/common.go explaining the refactoring approach and benefits.

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-build2039083650/b332/launcher.test /tmp/go-build2039083650/b332/launcher.test -test.testlogfile=/tmp/go-build2039083650/b332/testlog.txt -test.paniconexit0 -test.timeout=10m0s -test.v=true se stmain.go ache/go/1.25.7/x64/pkg/tool/linux_amd64/link . --gdwarf2 --64 ache/go/1.25.7/x64/pkg/tool/linuHEAD -I 7410264/b320/difc.test -I 86_64/git --gdwarf-5 --64 -o Rlvht1SmvPjlx/EgKV67uNxy1s5Sk4WJTJ/FFffmGA1P5hLrHEAD (dns block)
- Triggering command: /tmp/go-build139309952/b332/launcher.test /tmp/go-build139309952/b332/launcher.test -test.testlogfile=/tmp/go-build139309952/b332/testlog.txt -test.paniconexit0 -test.timeout=10m0s -test.v=true .cfg /opt/hostedtoolc-goversion 64/pkg/tool/linux_amd64/vet -bool -buildtags ache/go/1.25.7/x/tmp/go-build139309952/b319/_pkg_.a 64/pkg/tool/linu-trimpath rev-�� .cfg HEAD ache/go/1.25.7/x-lang=go1.25 --abbrev-ref HEAD tnet/tools/git tf "%s%s", sep, -goversion (dns block)
- Triggering command: /tmp/go-build311971124/b336/launcher.test /tmp/go-build311971124/b336/launcher.test -test.testlogfile=/tmp/go-build311971124/b336/testlog.txt -test.paniconexit0 -test.timeout=10m0s star�� (dns block)
invalid-host-that-does-not-exist-12345.com
- Triggering command: /tmp/go-build2039083650/b314/config.test /tmp/go-build2039083650/b314/config.test -test.testlogfile=/tmp/go-build2039083650/b314/testlog.txt -test.paniconexit0 -test.timeout=10m0s -test.v=true 7410264/b253/_pkg_.a /tmp/go-build1837410264/b068/vet.cfg ache/go/1.25.7/x64/pkg/tool/linux_amd64/vet -dumpbase ntio/asm/base64 -dumpbase-ext ache/go/1.25.7/xHEAD eU_a�� /opt/hostedtoolcache/go/1.25.7/x64/src/net -I rgo/bin/git 7410264/b253/symbase64 --64 -o /opt/hostedtoolcache/go/1.25.7/xHEAD (dns block)
- Triggering command: /tmp/go-build139309952/b314/config.test /tmp/go-build139309952/b314/config.test -test.testlogfile=/tmp/go-build139309952/b314/testlog.txt -test.paniconexit0 -test.timeout=10m0s -test.v=true ; s#^/home/REDACTED/.nvm/##; \#^[^v]# d; \#^versions$# d; tr /opt/hostedtoolcache/go/1.25.7/xHEAD 64/pkg/tool/linux_amd64/vet pkg/mod/github.cgit pkg/mod/github.crev-parse ache/Python/3.12--abbrev-ref 64/pkg/tool/linuHEAD /usr�� .cfg /opt/hostedtoolc-test.timeout=10m0s 64/pkg/tool/linux_amd64/vet github.com/segmebase64 -trimpath .12/x64/git 64/pkg/tool/linux_amd64/vet (dns block)
- Triggering command: /tmp/go-build311971124/b318/config.test /tmp/go-build311971124/b318/config.test -test.testlogfile=/tmp/go-build311971124/b318/testlog.txt -test.paniconexit0 -test.timeout=10m0s .pro�� /opt/hostedtoolc--abbrev-ref /opt/hostedtoolcHEAD d-dispatcher/off.d/chrony-onoffline /tmp/go-build186dirname git 64/pkg/tool/linux_amd64/compile d-dispatcher/off.d/chrony-onoffline -nam�� moby -id /usr/bin/runc.original -address /run/containerd/-d docker-buildx /usr/bin/runc.original (dns block)
nonexistent.local
- Triggering command: /tmp/go-build2039083650/b332/launcher.test /tmp/go-build2039083650/b332/launcher.test -test.testlogfile=/tmp/go-build2039083650/b332/testlog.txt -test.paniconexit0 -test.timeout=10m0s -test.v=true se stmain.go ache/go/1.25.7/x64/pkg/tool/linux_amd64/link . --gdwarf2 --64 ache/go/1.25.7/x64/pkg/tool/linuHEAD -I 7410264/b320/difc.test -I 86_64/git --gdwarf-5 --64 -o Rlvht1SmvPjlx/EgKV67uNxy1s5Sk4WJTJ/FFffmGA1P5hLrHEAD (dns block)
- Triggering command: /tmp/go-build139309952/b332/launcher.test /tmp/go-build139309952/b332/launcher.test -test.testlogfile=/tmp/go-build139309952/b332/testlog.txt -test.paniconexit0 -test.timeout=10m0s -test.v=true .cfg /opt/hostedtoolc-goversion 64/pkg/tool/linux_amd64/vet -bool -buildtags ache/go/1.25.7/x/tmp/go-build139309952/b319/_pkg_.a 64/pkg/tool/linu-trimpath rev-�� .cfg HEAD ache/go/1.25.7/x-lang=go1.25 --abbrev-ref HEAD tnet/tools/git tf "%s%s", sep, -goversion (dns block)
- Triggering command: /tmp/go-build311971124/b336/launcher.test /tmp/go-build311971124/b336/launcher.test -test.testlogfile=/tmp/go-build311971124/b336/testlog.txt -test.paniconexit0 -test.timeout=10m0s star�� (dns block)
slow.example.com
- Triggering command: /tmp/go-build2039083650/b332/launcher.test /tmp/go-build2039083650/b332/launcher.test -test.testlogfile=/tmp/go-build2039083650/b332/testlog.txt -test.paniconexit0 -test.timeout=10m0s -test.v=true se stmain.go ache/go/1.25.7/x64/pkg/tool/linux_amd64/link . --gdwarf2 --64 ache/go/1.25.7/x64/pkg/tool/linuHEAD -I 7410264/b320/difc.test -I 86_64/git --gdwarf-5 --64 -o Rlvht1SmvPjlx/EgKV67uNxy1s5Sk4WJTJ/FFffmGA1P5hLrHEAD (dns block)
- Triggering command: /tmp/go-build139309952/b332/launcher.test /tmp/go-build139309952/b332/launcher.test -test.testlogfile=/tmp/go-build139309952/b332/testlog.txt -test.paniconexit0 -test.timeout=10m0s -test.v=true .cfg /opt/hostedtoolc-goversion 64/pkg/tool/linux_amd64/vet -bool -buildtags ache/go/1.25.7/x/tmp/go-build139309952/b319/_pkg_.a 64/pkg/tool/linu-trimpath rev-�� .cfg HEAD ache/go/1.25.7/x-lang=go1.25 --abbrev-ref HEAD tnet/tools/git tf "%s%s", sep, -goversion (dns block)
- Triggering command: /tmp/go-build311971124/b336/launcher.test /tmp/go-build311971124/b336/launcher.test -test.testlogfile=/tmp/go-build311971124/b336/testlog.txt -test.paniconexit0 -test.timeout=10m0s star�� (dns block)
this-host-does-not-exist-12345.com
- Triggering command: /tmp/go-build2039083650/b341/mcp.test /tmp/go-build2039083650/b341/mcp.test -test.testlogfile=/tmp/go-build2039083650/b341/testlog.txt -test.paniconexit0 -test.timeout=10m0s -test.v=true /opt/hostedtoolcache/go/1.25.7/x64/src/net -I iptables --gdwarf-5 --64 -o as -I /opt/hostedtoolcache/go/1.25.7/x64/src/net -I 7410264/b248/vet.cfg --gdwarf-5 --64 -o */bin.*$") { next } } { printf "%s%s", sep, HEAD (dns block)
- Triggering command: /tmp/go-build139309952/b341/mcp.test /tmp/go-build139309952/b341/mcp.test -test.testlogfile=/tmp/go-build139309952/b341/testlog.txt -test.paniconexit0 -test.timeout=10m0s -test.v=true -importcfg 4E-sSYy/pwgIjRIG-w ache/go/1.25.7/x-buildmode=exe --abbrev-ref HEAD nfig/composer/ve--version ache/go/1.25.7/x-extld=gcc (dns block)
- Triggering command: /tmp/go-build311971124/b345/mcp.test /tmp/go-build311971124/b345/mcp.test -test.testlogfile=/tmp/go-build311971124/b345/testlog.txt -test.paniconexit0 -test.timeout=10m0s /usr�� 1dedc330cd146d1731f9c903fe295e422bb440808c4a20ae /var/run/docker/runtime-runc/moby 86_64/git /run/containerd//opt/hostedtoolcache/go/1.25.7/x64/pkg/tool/linux_amd64/vet --log-format json 1a2fc60e6817294b-buildtags /usr�� --root /var/run/docker/-ifaceassert by/7842e098fbefc-nilfunc /run/containerd/bash 1a2fc60e6817294b/usr/bin/runc json 1a2fc60e6817294b-tests (dns block)

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

Configure Actions setup steps to set up my environment, which run before the firewall is enabled
Add the appropriate URLs or hosts to the custom allowlist in this repository's Copilot coding agent settings (admins only)

This commit addresses three duplicate code patterns identified in issues #1766, #1767, and #1768: 1. Pattern #1767 (DIFC Tag Mutation): Extract modifyTag helper - Created modifyTag helper function in internal/difc/agent.go - Refactored AddSecrecyTag, AddIntegrityTag, and DropIntegrityTag - Reduces ~21 lines of duplicated mutex-lock + logging pattern - All DIFC tests pass 2. Pattern #1768 (Global Logger RWMutex): Extract withGlobalLogger helper - Created generic withGlobalLogger helper in internal/logger/global_helpers.go - Refactored 8+ instances of RWMutex nil-check pattern across logger files - Updated file_logger.go, markdown_logger.go, jsonl_logger.go, server_file_logger.go, tools_logger.go, and rpc_logger.go - Eliminates ~40 lines of duplicated mutex code - Type-safe through closableLogger constraint - All logger tests pass 3. Pattern #1766 (Logger Init Boilerplate): Documented as intentional - Reviewed existing initLogger generic helper implementation - Pattern is already well-abstracted with clear fallback strategies - Added documentation section in common.go explaining the Global Logger RWMutex Access Pattern refactoring All unit tests pass. Code formatting verified with gofmt. Co-authored-by: lpcox <15877973+lpcox@users.noreply.github.com>

Copilot

Pull request overview

Refactors duplicated mutex + nil-check access patterns around global loggers and consolidates DIFC tag-mutation lock/log logic into a shared helper, reducing repeated boilerplate while keeping existing behavior.

Changes:

Added withGlobalLogger generic helper and refactored multiple logger call sites to use it.
Added DIFC modifyTag helper and refactored tag mutation methods to use it.
Added documentation describing the new global-logger access pattern.

Reviewed changes

Copilot reviewed 9 out of 9 changed files in this pull request and generated 3 comments.

Show a summary per file

File	Description
internal/logger/global_helpers.go	Introduces `withGlobalLogger` helper to centralize RWMutex + nil-check access.
internal/logger/file_logger.go	Refactors global file logger access to use `withGlobalLogger`.
internal/logger/markdown_logger.go	Refactors markdown global access through `withGlobalLogger`.
internal/logger/jsonl_logger.go	Refactors JSONL global access through `withGlobalLogger`.
internal/logger/server_file_logger.go	Refactors server-file global access through `withGlobalLogger`.
internal/logger/tools_logger.go	Refactors tools logger global access through `withGlobalLogger`.
internal/logger/rpc_logger.go	Refactors markdown global access for RPC logging through `withGlobalLogger`.
internal/logger/common.go	Adds documentation describing the global logger access pattern.
internal/difc/agent.go	Adds `modifyTag` helper and refactors tag mutation methods to use it.

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

You can also share your feedback on Copilot code review. Take the survey.

Copilot · 2026-03-11T19:24:13Z

internal/logger/global_helpers.go

+//	globalLoggerMu.RLock()
+//	defer globalLoggerMu.RUnlock()
+//	if globalLogger != nil {
+//	    globalLogger.DoSomething(args...)


The doc example here uses globalLogger and *Logger, but the package globals are globalFileLogger, globalMarkdownLogger, etc., and *Logger (debug logger) does not satisfy the closableLogger constraint. Please update the example to use a real global (e.g., globalFileLogger) and a type that actually works with withGlobalLogger.

Suggested change

// globalLoggerMu.RLock()

// defer globalLoggerMu.RUnlock()

// if globalLogger != nil {

// globalLogger.DoSomething(args...)

// globalFileLoggerMu.RLock()

// defer globalFileLoggerMu.RUnlock()

// if globalFileLogger != nil {

// globalFileLogger.Log(level, category, format, args...)

Copilot · 2026-03-11T19:24:13Z

internal/logger/common.go

+//
+// After refactoring (unified pattern):
+//
+//	withGlobalLogger(&globalLoggerMu, &globalLogger, func(logger *Logger) {


This documentation claims withGlobalLogger works with &globalLogger / *Logger, but *Logger in this package is the debug logger and is not part of the closableLogger constraint. Consider changing the example to one of the actual closable globals (e.g., &globalFileLogger with *FileLogger) to avoid confusion.

Suggested change

// withGlobalLogger(&globalLoggerMu, &globalLogger, func(logger *Logger) {

// withGlobalLogger(&globalFileLoggerMu, &globalFileLogger, func(logger *FileLogger) {

Copilot · 2026-03-11T19:24:13Z

internal/logger/common.go

+//   - Eliminates ~40 lines of duplicated mutex code
+//   - Provides a single point of control for the locking pattern
+//   - Type-safe through generics (enforces closableLogger constraint)
+//   - Easier to modify the locking strategy globally (e.g., add timeouts)


“Easier to modify the locking strategy globally (e.g., add timeouts)” is misleading because sync.RWMutex doesn’t support timed locking. Suggest rewording to concrete options like switching to an atomic pointer, adding metrics/instrumentation around lock hold times, or changing lock granularity.

Suggested change

// - Easier to modify the locking strategy globally (e.g., add timeouts)

// - Easier to evolve the locking strategy globally (e.g., switch to an atomic pointer,

// add metrics/instrumentation around lock hold times, or adjust lock granularity)

Initial plan

617174e

Claude AI assigned Claude and lpcox Mar 11, 2026

Claude started work on behalf of lpcox March 11, 2026 19:05 View session

Claude AI linked an issue Mar 11, 2026 that may be closed by this pull request

[duplicate-code] Duplicate Code Analysis Report #1765

Closed

Claude AI changed the title ~~[WIP] Fix duplicate code patterns in logger and DIFC packages~~ Refactor duplicate code patterns in logger and DIFC packages Mar 11, 2026

Claude finished work on behalf of lpcox March 11, 2026 19:16

lpcox marked this pull request as ready for review March 11, 2026 19:20

Copilot AI review requested due to automatic review settings March 11, 2026 19:20

Copilot started reviewing on behalf of lpcox March 11, 2026 19:21 View session

lpcox merged commit 165a060 into main Mar 11, 2026
14 checks passed

lpcox deleted the claude/fix-duplicate-code-patterns branch March 11, 2026 19:22

Copilot AI reviewed Mar 11, 2026

View reviewed changes

This was referenced Mar 11, 2026

Smoke Test: Copilot - 22970457894 #1794

Closed

Smoke Test: Copilot - 22970369860 #1796

Closed

Smoke Test: Copilot - 22971161535 #1797

Closed

Smoke Test: Copilot - 22971240966 #1798

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor duplicate code patterns in logger and DIFC packages#1791

Refactor duplicate code patterns in logger and DIFC packages#1791
lpcox merged 2 commits intomainfrom
claude/fix-duplicate-code-patterns

Claude AI commented Mar 11, 2026 •

edited

Loading

Uh oh!

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Mar 11, 2026

Uh oh!

Copilot AI Mar 11, 2026

Uh oh!

Copilot AI Mar 11, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

	// withGlobalLogger(&globalLoggerMu, &globalLogger, func(logger *Logger) {
	// withGlobalLogger(&globalFileLoggerMu, &globalFileLogger, func(logger *FileLogger) {

	// - Easier to modify the locking strategy globally (e.g., add timeouts)
	// - Easier to evolve the locking strategy globally (e.g., switch to an atomic pointer,
	// add metrics/instrumentation around lock hold times, or adjust lock granularity)

Conversation

Claude AI commented Mar 11, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Changes

Documentation

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

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Mar 11, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 11, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 11, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Claude AI commented Mar 11, 2026 •

edited

Loading