Skip to content

Commit 165a060

Browse files
authored
Refactor duplicate code patterns in logger and DIFC packages (#1791)
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` ```go // 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 ```go // 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] > > <details> > <summary>Firewall rules blocked me from connecting to one or more addresses (expand for details)</summary> > > #### 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 &#34;%s%s&#34;, 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 &#34;%s%s&#34;, 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 &#34;%s%s&#34;, 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.*$&#34;) { next } } { printf &#34;%s%s&#34;, 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](https://gh.io/copilot/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](https://github.com/github/gh-aw-mcpg/settings/copilot/coding_agent) (admins only) > > </details>
2 parents da4f29f + f999d14 commit 165a060

9 files changed

Lines changed: 156 additions & 90 deletions

File tree

internal/difc/agent.go

Lines changed: 26 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -39,31 +39,42 @@ func NewAgentLabelsWithTags(agentID string, secrecyTags []Tag, integrityTags []T
3939
}
4040
}
4141

42-
// AddSecrecyTag adds a secrecy tag to the agent
43-
func (a *AgentLabels) AddSecrecyTag(tag Tag) {
44-
logAgent.Printf("Agent %s adding secrecy tag: %s", a.AgentID, tag)
42+
// modifyTag is a helper that encapsulates the common pattern for tag modification with locking and logging.
43+
// It handles the mutex lock, executes the modification action, and logs the operation.
44+
//
45+
// Parameters:
46+
// - labelType: The type of label being modified ("secrecy" or "integrity")
47+
// - action: The verb describing the action ("adding", "dropping", etc.)
48+
// - pastTense: The past tense verb for the operational log ("gained", "dropped", etc.)
49+
// - tag: The tag being modified
50+
// - fn: The function that performs the actual label modification
51+
func (a *AgentLabels) modifyTag(labelType, action, pastTense string, tag Tag, fn func()) {
52+
logAgent.Printf("Agent %s %s %s tag: %s", a.AgentID, action, labelType, tag)
4553
a.mu.Lock()
4654
defer a.mu.Unlock()
47-
a.Secrecy.Label.Add(tag)
48-
log.Printf("[DIFC] Agent %s gained secrecy tag: %s", a.AgentID, tag)
55+
fn()
56+
log.Printf("[DIFC] Agent %s %s %s tag: %s", a.AgentID, pastTense, labelType, tag)
57+
}
58+
59+
// AddSecrecyTag adds a secrecy tag to the agent
60+
func (a *AgentLabels) AddSecrecyTag(tag Tag) {
61+
a.modifyTag("secrecy", "adding", "gained", tag, func() {
62+
a.Secrecy.Label.Add(tag)
63+
})
4964
}
5065

5166
// AddIntegrityTag adds an integrity tag to the agent
5267
func (a *AgentLabels) AddIntegrityTag(tag Tag) {
53-
logAgent.Printf("Agent %s adding integrity tag: %s", a.AgentID, tag)
54-
a.mu.Lock()
55-
defer a.mu.Unlock()
56-
a.Integrity.Label.Add(tag)
57-
log.Printf("[DIFC] Agent %s gained integrity tag: %s", a.AgentID, tag)
68+
a.modifyTag("integrity", "adding", "gained", tag, func() {
69+
a.Integrity.Label.Add(tag)
70+
})
5871
}
5972

6073
// DropIntegrityTag removes an integrity tag from the agent
6174
func (a *AgentLabels) DropIntegrityTag(tag Tag) {
62-
logAgent.Printf("Agent %s dropping integrity tag: %s", a.AgentID, tag)
63-
a.mu.Lock()
64-
defer a.mu.Unlock()
65-
a.Integrity.Label.Remove(tag)
66-
log.Printf("[DIFC] Agent %s dropped integrity tag: %s", a.AgentID, tag)
75+
a.modifyTag("integrity", "dropping", "dropped", tag, func() {
76+
a.Integrity.Label.Remove(tag)
77+
})
6778
}
6879

6980
// DropIntegrityTags removes multiple integrity tags from the agent

internal/logger/common.go

Lines changed: 38 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -224,6 +224,44 @@ import (
224224
// 1. Add a new entry to the logFuncs map in file_logger.go.
225225
// 2. Add a new LogTrace wrapper to each of the three files above.
226226

227+
// Global Logger RWMutex Access Pattern
228+
//
229+
// All access to global logger instances uses the withGlobalLogger helper function
230+
// (defined in global_helpers.go) to eliminate duplicated RWMutex locking patterns.
231+
//
232+
// Before refactoring (duplicated pattern):
233+
//
234+
// globalLoggerMu.RLock()
235+
// defer globalLoggerMu.RUnlock()
236+
//
237+
// if globalLogger != nil {
238+
// globalLogger.DoSomething(args...)
239+
// }
240+
//
241+
// After refactoring (unified pattern):
242+
//
243+
// withGlobalLogger(&globalLoggerMu, &globalLogger, func(logger *Logger) {
244+
// logger.DoSomething(args...)
245+
// })
246+
//
247+
// The withGlobalLogger helper is used in:
248+
// - file_logger.go: logWithLevel (for FileLogger)
249+
// - markdown_logger.go: logWithMarkdown (for MarkdownLogger)
250+
// - jsonl_logger.go: LogRPCMessageJSONLWithTags (for JSONLLogger)
251+
// - server_file_logger.go: logWithLevelAndServer (for ServerFileLogger)
252+
// - tools_logger.go: LogToolsForServer (for ToolsLogger)
253+
// - rpc_logger.go: logRPCMessageToAll and LogRPCMessage (for MarkdownLogger)
254+
//
255+
// Benefits:
256+
// - Eliminates ~40 lines of duplicated mutex code
257+
// - Provides a single point of control for the locking pattern
258+
// - Type-safe through generics (enforces closableLogger constraint)
259+
// - Easier to modify the locking strategy globally (e.g., add timeouts)
260+
//
261+
// When adding a new logger access point:
262+
// 1. Use withGlobalLogger instead of manual RLock/RUnlock
263+
// 2. Pass the appropriate mutex, logger pointer, and callback function
264+
227265
// It syncs buffered data before closing and handles errors appropriately.
228266
// The mutex should already be held by the caller.
229267
//

internal/logger/file_logger.go

Lines changed: 6 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -111,15 +111,13 @@ func (fl *FileLogger) GetWriter() io.Writer {
111111
// Global logging functions that use the global file logger
112112

113113
// logWithLevel is a helper that reduces code duplication for logging at different levels.
114-
// It handles mutex locking and nil-checking in a single place, eliminating the need
115-
// for repeated mutex lock/unlock patterns across LogInfo, LogWarn, LogError, and LogDebug.
114+
// It uses the withGlobalLogger helper from global_helpers.go to handle mutex locking and
115+
// nil-checking, eliminating the need for repeated RWMutex lock/unlock patterns across
116+
// LogInfo, LogWarn, LogError, and LogDebug.
116117
func logWithLevel(level LogLevel, category, format string, args ...interface{}) {
117-
globalLoggerMu.RLock()
118-
defer globalLoggerMu.RUnlock()
119-
120-
if globalFileLogger != nil {
121-
globalFileLogger.Log(level, category, format, args...)
122-
}
118+
withGlobalLogger(&globalLoggerMu, &globalFileLogger, func(logger *FileLogger) {
119+
logger.Log(level, category, format, args...)
120+
})
123121
}
124122

125123
// LogInfo logs an informational message

internal/logger/global_helpers.go

Lines changed: 33 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -23,6 +23,39 @@ type closableLogger interface {
2323
Close() error
2424
}
2525

26+
// withGlobalLogger is a generic helper that encapsulates the common pattern for
27+
// accessing a global logger with proper RWMutex locking and nil-checking.
28+
//
29+
// This eliminates the repeated pattern of:
30+
//
31+
// globalLoggerMu.RLock()
32+
// defer globalLoggerMu.RUnlock()
33+
// if globalLogger != nil {
34+
// globalLogger.DoSomething(args...)
35+
// }
36+
//
37+
// Type parameters:
38+
// - T: Any pointer type that satisfies closableLogger constraint
39+
//
40+
// Parameters:
41+
// - mu: RWMutex to protect access to the global logger
42+
// - logger: Pointer to the global logger instance
43+
// - fn: Function to execute with the logger if it's not nil
44+
//
45+
// Example usage:
46+
//
47+
// withGlobalLogger(&globalLoggerMu, &globalFileLogger, func(l *FileLogger) {
48+
// l.Log(level, category, format, args...)
49+
// })
50+
func withGlobalLogger[T closableLogger](mu *sync.RWMutex, logger *T, fn func(T)) {
51+
mu.RLock()
52+
defer mu.RUnlock()
53+
54+
if *logger != nil {
55+
fn(*logger)
56+
}
57+
}
58+
2659
// initGlobalLogger is a generic helper that encapsulates the common pattern for
2760
// initializing a global logger with proper mutex handling.
2861
//

internal/logger/jsonl_logger.go

Lines changed: 25 additions & 29 deletions
Original file line numberDiff line numberDiff line change
@@ -108,34 +108,30 @@ func LogRPCMessageJSONL(direction RPCMessageDirection, messageType RPCMessageTyp
108108
}
109109

110110
// LogRPCMessageJSONLWithTags logs an RPC message to the global JSONL logger with optional agent tag snapshots.
111+
// It uses the withGlobalLogger helper from global_helpers.go to handle mutex locking and nil-checking.
111112
func LogRPCMessageJSONLWithTags(direction RPCMessageDirection, messageType RPCMessageType, serverID, method string, payloadBytes []byte, err error, agentSecrecy, agentIntegrity []string) {
112-
globalJSONLMu.RLock()
113-
defer globalJSONLMu.RUnlock()
114-
115-
if globalJSONLLogger == nil {
116-
return
117-
}
118-
119-
entry := &JSONLRPCMessage{
120-
Timestamp: time.Now().UTC().Format(time.RFC3339Nano),
121-
Direction: string(direction),
122-
Type: string(messageType),
123-
ServerID: serverID,
124-
Method: method,
125-
Payload: sanitize.SanitizeJSON(payloadBytes),
126-
}
127-
128-
if len(agentSecrecy) > 0 {
129-
entry.AgentSecrecy = append([]string(nil), agentSecrecy...)
130-
}
131-
if len(agentIntegrity) > 0 {
132-
entry.AgentIntegrity = append([]string(nil), agentIntegrity...)
133-
}
134-
135-
if err != nil {
136-
entry.Error = err.Error()
137-
}
138-
139-
// Best effort logging - don't fail if JSONL logging fails
140-
_ = globalJSONLLogger.LogMessage(entry)
113+
withGlobalLogger(&globalJSONLMu, &globalJSONLLogger, func(logger *JSONLLogger) {
114+
entry := &JSONLRPCMessage{
115+
Timestamp: time.Now().UTC().Format(time.RFC3339Nano),
116+
Direction: string(direction),
117+
Type: string(messageType),
118+
ServerID: serverID,
119+
Method: method,
120+
Payload: sanitize.SanitizeJSON(payloadBytes),
121+
}
122+
123+
if len(agentSecrecy) > 0 {
124+
entry.AgentSecrecy = append([]string(nil), agentSecrecy...)
125+
}
126+
if len(agentIntegrity) > 0 {
127+
entry.AgentIntegrity = append([]string(nil), agentIntegrity...)
128+
}
129+
130+
if err != nil {
131+
entry.Error = err.Error()
132+
}
133+
134+
// Best effort logging - don't fail if JSONL logging fails
135+
_ = logger.LogMessage(entry)
136+
})
141137
}

internal/logger/markdown_logger.go

Lines changed: 7 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -169,18 +169,17 @@ func (ml *MarkdownLogger) Log(level LogLevel, category, format string, args ...i
169169

170170
// Global logging functions that also write to markdown logger
171171

172-
// logWithMarkdown is a helper that logs to both regular and markdown loggers
172+
// logWithMarkdown is a helper that logs to both regular and markdown loggers.
173+
// It uses the withGlobalLogger helper from global_helpers.go to handle mutex locking
174+
// and nil-checking for the markdown logger access.
173175
func logWithMarkdown(level LogLevel, regularLogFunc func(string, string, ...interface{}), category, format string, args ...interface{}) {
174176
// Log to regular logger
175177
regularLogFunc(category, format, args...)
176178

177-
// Log to markdown logger
178-
globalMarkdownMu.RLock()
179-
defer globalMarkdownMu.RUnlock()
180-
181-
if globalMarkdownLogger != nil {
182-
globalMarkdownLogger.Log(level, category, format, args...)
183-
}
179+
// Log to markdown logger using withGlobalLogger helper
180+
withGlobalLogger(&globalMarkdownMu, &globalMarkdownLogger, func(logger *MarkdownLogger) {
181+
logger.Log(level, category, format, args...)
182+
})
184183
}
185184

186185
// logWithMarkdownLevel is a helper that reduces code duplication for markdown logging at different levels.

internal/logger/rpc_logger.go

Lines changed: 12 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -59,7 +59,8 @@ type RPCMessageInfo struct {
5959
Error string // Error message if any (for responses)
6060
}
6161

62-
// logRPCMessageToAll is a helper that logs RPC messages to text, markdown, and JSONL logs
62+
// logRPCMessageToAll is a helper that logs RPC messages to text, markdown, and JSONL logs.
63+
// It uses the withGlobalLogger helper from global_helpers.go to handle mutex locking and nil-checking.
6364
func logRPCMessageToAll(direction RPCMessageDirection, messageType RPCMessageType, serverID, method string, payload []byte, err error, agentSecrecy, agentIntegrity []string) {
6465
// Create info for text log (with larger payload preview)
6566
infoText := &RPCMessageInfo{
@@ -92,13 +93,10 @@ func logRPCMessageToAll(direction RPCMessageDirection, messageType RPCMessageTyp
9293
infoMarkdown.Error = err.Error()
9394
}
9495

95-
// Log to markdown file
96-
globalMarkdownMu.RLock()
97-
defer globalMarkdownMu.RUnlock()
98-
99-
if globalMarkdownLogger != nil {
100-
globalMarkdownLogger.Log(LogLevelDebug, "rpc", "%s", formatRPCMessageMarkdown(infoMarkdown))
101-
}
96+
// Log to markdown file using withGlobalLogger helper
97+
withGlobalLogger(&globalMarkdownMu, &globalMarkdownLogger, func(logger *MarkdownLogger) {
98+
logger.Log(LogLevelDebug, "rpc", "%s", formatRPCMessageMarkdown(infoMarkdown))
99+
})
102100

103101
// Log to JSONL file (full payload, sanitized)
104102
LogRPCMessageJSONLWithTags(direction, messageType, serverID, method, payload, err, agentSecrecy, agentIntegrity)
@@ -124,16 +122,14 @@ func LogRPCResponseWithAgentSnapshot(direction RPCMessageDirection, serverID str
124122
logRPCMessageToAll(direction, RPCMessageResponse, serverID, "", payload, err, agentSecrecy, agentIntegrity)
125123
}
126124

127-
// LogRPCMessage logs a generic RPC message with custom info
125+
// LogRPCMessage logs a generic RPC message with custom info.
126+
// It uses the withGlobalLogger helper from global_helpers.go to handle mutex locking and nil-checking.
128127
func LogRPCMessage(info *RPCMessageInfo) {
129128
// Log to text file
130129
LogDebug("rpc", "%s", formatRPCMessage(info))
131130

132-
// Log to markdown file
133-
globalMarkdownMu.RLock()
134-
defer globalMarkdownMu.RUnlock()
135-
136-
if globalMarkdownLogger != nil {
137-
globalMarkdownLogger.Log(LogLevelDebug, "rpc", "%s", formatRPCMessageMarkdown(info))
138-
}
131+
// Log to markdown file using withGlobalLogger helper
132+
withGlobalLogger(&globalMarkdownMu, &globalMarkdownLogger, func(logger *MarkdownLogger) {
133+
logger.Log(LogLevelDebug, "rpc", "%s", formatRPCMessageMarkdown(info))
134+
})
139135
}

internal/logger/server_file_logger.go

Lines changed: 4 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -159,16 +159,13 @@ func (sfl *ServerFileLogger) Close() error {
159159
// Global logging functions that use the global server file logger
160160

161161
// logWithLevelAndServer is a helper that reduces code duplication for per-server logging at different levels.
162-
// It handles mutex locking, nil-checking, and dual logging (server-specific + unified) in a single place,
162+
// It uses the withGlobalLogger helper from global_helpers.go to handle mutex locking and nil-checking,
163163
// eliminating repeated patterns across LogInfoWithServer, LogWarnWithServer, LogErrorWithServer, and LogDebugWithServer.
164164
// It uses the logFuncs map (file_logger.go) for the unified log dispatch, avoiding a repeated switch-on-level block.
165165
func logWithLevelAndServer(serverID string, level LogLevel, category, format string, args ...interface{}) {
166-
globalServerLoggerMu.RLock()
167-
defer globalServerLoggerMu.RUnlock()
168-
169-
if globalServerFileLogger != nil {
170-
globalServerFileLogger.Log(serverID, level, category, format, args...)
171-
}
166+
withGlobalLogger(&globalServerLoggerMu, &globalServerFileLogger, func(logger *ServerFileLogger) {
167+
logger.Log(serverID, level, category, format, args...)
168+
})
172169

173170
// Also log to the main log file for unified view
174171
// Use logFuncs to dispatch to the appropriate level function

internal/logger/tools_logger.go

Lines changed: 5 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -131,17 +131,15 @@ func (tl *ToolsLogger) Close() error {
131131

132132
// Global logging function that uses the global tools logger
133133

134-
// LogToolsForServer logs the tools for a specific server
134+
// LogToolsForServer logs the tools for a specific server.
135+
// It uses the withGlobalLogger helper from global_helpers.go to handle mutex locking and nil-checking.
135136
func LogToolsForServer(serverID string, tools []ToolInfo) {
136-
globalToolsMu.RLock()
137-
defer globalToolsMu.RUnlock()
138-
139-
if globalToolsLogger != nil {
140-
if err := globalToolsLogger.LogTools(serverID, tools); err != nil {
137+
withGlobalLogger(&globalToolsMu, &globalToolsLogger, func(logger *ToolsLogger) {
138+
if err := logger.LogTools(serverID, tools); err != nil {
141139
// Log errors using the standard logger to avoid recursion
142140
log.Printf("WARNING: Failed to log tools for server %s: %v", serverID, err)
143141
}
144-
}
142+
})
145143
}
146144

147145
// CloseToolsLogger closes the global tools logger

0 commit comments

Comments
 (0)