Refactor duplicated DIFC pipeline decisions and logger level wrappers#4301
Merged
Refactor duplicated DIFC pipeline decisions and logger level wrappers#4301
Conversation
Agent-Logs-Url: https://github.com/github/gh-aw-mcpg/sessions/97151d73-796a-42a0-9fb4-c0de9f160c32 Co-authored-by: lpcox <15877973+lpcox@users.noreply.github.com>
Copilot
AI
changed the title
[WIP] Fix duplicate code issues in Go source files
Refactor duplicated DIFC pipeline decisions and logger level wrappers
Apr 21, 2026
Contributor
There was a problem hiding this comment.
Pull request overview
This PR reduces duplication in two areas: DIFC enforcement decision predicates shared by the unified MCP tool-call path and the proxy REST handler, and the repeated per-level logger wrapper functions.
Changes:
- Centralized DIFC “pipeline decision” predicates into
internal/difc/pipeline_decisions.goand updated unified/proxy callers to use them. - Consolidated repeated logger level wrapper patterns via helper constructors in
internal/logger/common.goand refactored file/markdown/server loggers to use them. - Added targeted DIFC helper tests in
internal/difc/pipeline_decisions_test.go.
Show a summary per file
| File | Description |
|---|---|
| internal/server/unified.go | Swaps inline DIFC decision conditionals for shared difc helper predicates. |
| internal/proxy/handler.go | Uses shared difc helper predicates for coarse-deny bypass, strict blocking, and propagate accumulation. |
| internal/difc/pipeline_decisions.go | Introduces shared DIFC decision helpers to avoid drift across enforcement call sites. |
| internal/difc/pipeline_decisions_test.go | Adds unit tests locking helper behavior across operation/mode combinations. |
| internal/logger/common.go | Adds helper constructors for per-level wrapper creation (and retains logFuncs dispatch map). |
| internal/logger/file_logger.go | Refactors LogInfo/LogWarn/LogError/LogDebug wrappers to use helper constructor. |
| internal/logger/markdown_logger.go | Refactors markdown wrapper set (Log*Md) to use helper constructor. |
| internal/logger/server_file_logger.go | Refactors per-server wrapper set (Log*WithServer) to use helper constructor. |
Copilot's findings
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comments suppressed due to low confidence (1)
internal/proxy/handler.go:186
- The comment in this branch says “Read in filter mode” / “Write blocked”, but the predicate is now
ShouldBypassCoarseDeny(operation)(i.e., read operations regardless of enforcement mode), and the else branch covers both write and read-write. Please update the comments to match the actual condition/behavior so future diffs don’t reintroduce drift.
if difc.ShouldBypassCoarseDeny(operation) {
// Read in filter mode: skip coarse block, proceed to fine-grained filtering
logHandler.Printf("[DIFC] Phase 2: coarse check failed for read, proceeding to Phase 3")
} else {
// Write blocked
logHandler.Printf("[DIFC] Phase 2: BLOCKED %s %s — %s", r.Method, path, evalResult.Reason)
- Files reviewed: 8/8 changed files
- Comments generated: 4
Comment on lines
117
to
126
| // LogInfo logs an informational message. | ||
| LogInfo = makeLevelLogger(logWithLevel, LogLevelInfo) | ||
| // LogWarn logs a warning message. | ||
| LogWarn = makeLevelLogger(logWithLevel, LogLevelWarn) | ||
| // LogError logs an error message. | ||
| LogError = makeLevelLogger(logWithLevel, LogLevelError) | ||
| // LogDebug logs a debug message. | ||
| LogDebug = makeLevelLogger(logWithLevel, LogLevelDebug) | ||
| ) | ||
|
|
There was a problem hiding this comment.
These exported logging entry points are now variables holding function values. That makes them reassignable by any code in the module (package is internal, but still widely reachable), which can lead to accidental override, data races, or test pollution. Prefer keeping exported wrappers as func declarations and use an unexported closure/dispatcher helper internally (e.g., assign logInfo := makeLevelLogger(...) and have func LogInfo(...) { logInfo(...) }) so callers cannot mutate the API surface.
Suggested change
| // LogInfo logs an informational message. | |
| LogInfo = makeLevelLogger(logWithLevel, LogLevelInfo) | |
| // LogWarn logs a warning message. | |
| LogWarn = makeLevelLogger(logWithLevel, LogLevelWarn) | |
| // LogError logs an error message. | |
| LogError = makeLevelLogger(logWithLevel, LogLevelError) | |
| // LogDebug logs a debug message. | |
| LogDebug = makeLevelLogger(logWithLevel, LogLevelDebug) | |
| ) | |
| logInfo = makeLevelLogger(logWithLevel, LogLevelInfo) | |
| logWarn = makeLevelLogger(logWithLevel, LogLevelWarn) | |
| logError = makeLevelLogger(logWithLevel, LogLevelError) | |
| logDebug = makeLevelLogger(logWithLevel, LogLevelDebug) | |
| ) | |
| // LogInfo logs an informational message. | |
| func LogInfo(category, format string, args ...interface{}) { | |
| logInfo(category, format, args...) | |
| } | |
| // LogWarn logs a warning message. | |
| func LogWarn(category, format string, args ...interface{}) { | |
| logWarn(category, format, args...) | |
| } | |
| // LogError logs an error message. | |
| func LogError(category, format string, args ...interface{}) { | |
| logError(category, format, args...) | |
| } | |
| // LogDebug logs a debug message. | |
| func LogDebug(category, format string, args ...interface{}) { | |
| logDebug(category, format, args...) | |
| } |
Comment on lines
184
to
193
| // LogInfoMd logs to both regular and markdown loggers. | ||
| LogInfoMd = makeLevelLogger(logWithMarkdown, LogLevelInfo) | ||
| // LogWarnMd logs to both regular and markdown loggers. | ||
| LogWarnMd = makeLevelLogger(logWithMarkdown, LogLevelWarn) | ||
| // LogErrorMd logs to both regular and markdown loggers. | ||
| LogErrorMd = makeLevelLogger(logWithMarkdown, LogLevelError) | ||
| // LogDebugMd logs to both regular and markdown loggers. | ||
| LogDebugMd = makeLevelLogger(logWithMarkdown, LogLevelDebug) | ||
| ) | ||
|
|
There was a problem hiding this comment.
These exported logging entry points are now variables holding function values, which makes them reassignable from other packages within the module. That’s a behavioral/API change compared to exported funcs and can introduce hard-to-debug overrides or data races. Consider keeping LogInfoMd/LogWarnMd/... as func wrappers and using an unexported closure from makeLevelLogger behind them.
Suggested change
| // LogInfoMd logs to both regular and markdown loggers. | |
| LogInfoMd = makeLevelLogger(logWithMarkdown, LogLevelInfo) | |
| // LogWarnMd logs to both regular and markdown loggers. | |
| LogWarnMd = makeLevelLogger(logWithMarkdown, LogLevelWarn) | |
| // LogErrorMd logs to both regular and markdown loggers. | |
| LogErrorMd = makeLevelLogger(logWithMarkdown, LogLevelError) | |
| // LogDebugMd logs to both regular and markdown loggers. | |
| LogDebugMd = makeLevelLogger(logWithMarkdown, LogLevelDebug) | |
| ) | |
| logInfoMd = makeLevelLogger(logWithMarkdown, LogLevelInfo) | |
| logWarnMd = makeLevelLogger(logWithMarkdown, LogLevelWarn) | |
| logErrorMd = makeLevelLogger(logWithMarkdown, LogLevelError) | |
| logDebugMd = makeLevelLogger(logWithMarkdown, LogLevelDebug) | |
| ) | |
| // LogInfoMd logs to both regular and markdown loggers. | |
| func LogInfoMd(category, format string, args ...interface{}) { | |
| logInfoMd(category, format, args...) | |
| } | |
| // LogWarnMd logs to both regular and markdown loggers. | |
| func LogWarnMd(category, format string, args ...interface{}) { | |
| logWarnMd(category, format, args...) | |
| } | |
| // LogErrorMd logs to both regular and markdown loggers. | |
| func LogErrorMd(category, format string, args ...interface{}) { | |
| logErrorMd(category, format, args...) | |
| } | |
| // LogDebugMd logs to both regular and markdown loggers. | |
| func LogDebugMd(category, format string, args ...interface{}) { | |
| logDebugMd(category, format, args...) | |
| } |
Comment on lines
159
to
168
| // LogInfoWithServer logs an informational message to the server-specific log file. | ||
| LogInfoWithServer = makeServerLevelLogger(logWithLevelAndServer, LogLevelInfo) | ||
| // LogWarnWithServer logs a warning message to the server-specific log file. | ||
| LogWarnWithServer = makeServerLevelLogger(logWithLevelAndServer, LogLevelWarn) | ||
| // LogErrorWithServer logs an error message to the server-specific log file. | ||
| LogErrorWithServer = makeServerLevelLogger(logWithLevelAndServer, LogLevelError) | ||
| // LogDebugWithServer logs a debug message to the server-specific log file. | ||
| LogDebugWithServer = makeServerLevelLogger(logWithLevelAndServer, LogLevelDebug) | ||
| ) | ||
|
|
There was a problem hiding this comment.
Switching Log*WithServer from exported funcs to exported variables makes them reassignable by any internal package, which can cause accidental overrides and races. To preserve a stable, non-mutable API surface, keep these as func wrappers and have them delegate to an unexported closure created by makeServerLevelLogger.
Suggested change
| // LogInfoWithServer logs an informational message to the server-specific log file. | |
| LogInfoWithServer = makeServerLevelLogger(logWithLevelAndServer, LogLevelInfo) | |
| // LogWarnWithServer logs a warning message to the server-specific log file. | |
| LogWarnWithServer = makeServerLevelLogger(logWithLevelAndServer, LogLevelWarn) | |
| // LogErrorWithServer logs an error message to the server-specific log file. | |
| LogErrorWithServer = makeServerLevelLogger(logWithLevelAndServer, LogLevelError) | |
| // LogDebugWithServer logs a debug message to the server-specific log file. | |
| LogDebugWithServer = makeServerLevelLogger(logWithLevelAndServer, LogLevelDebug) | |
| ) | |
| logInfoWithServer = makeServerLevelLogger(logWithLevelAndServer, LogLevelInfo) | |
| logWarnWithServer = makeServerLevelLogger(logWithLevelAndServer, LogLevelWarn) | |
| logErrorWithServer = makeServerLevelLogger(logWithLevelAndServer, LogLevelError) | |
| logDebugWithServer = makeServerLevelLogger(logWithLevelAndServer, LogLevelDebug) | |
| ) | |
| // LogInfoWithServer logs an informational message to the server-specific log file. | |
| func LogInfoWithServer(serverID, category, format string, args ...interface{}) { | |
| logInfoWithServer(serverID, category, format, args...) | |
| } | |
| // LogWarnWithServer logs a warning message to the server-specific log file. | |
| func LogWarnWithServer(serverID, category, format string, args ...interface{}) { | |
| logWarnWithServer(serverID, category, format, args...) | |
| } | |
| // LogErrorWithServer logs an error message to the server-specific log file. | |
| func LogErrorWithServer(serverID, category, format string, args ...interface{}) { | |
| logErrorWithServer(serverID, category, format, args...) | |
| } | |
| // LogDebugWithServer logs a debug message to the server-specific log file. | |
| func LogDebugWithServer(serverID, category, format string, args ...interface{}) { | |
| logDebugWithServer(serverID, category, format, args...) | |
| } |
Comment on lines
+218
to
+225
| func makeLevelLogger( | ||
| dispatch func(level LogLevel, category, format string, args ...interface{}), | ||
| level LogLevel, | ||
| ) func(category, format string, args ...interface{}) { | ||
| return func(category, format string, args ...interface{}) { | ||
| dispatch(level, category, format, args...) | ||
| } | ||
| } |
There was a problem hiding this comment.
The large block comment immediately above this helper says the per-level one-liner wrappers are “intentionally kept” and instructs adding new wrappers in each file when adding a LogLevel. With this refactor, that guidance is now outdated/misleading. Please update or remove that section to reflect the new pattern (and, if you keep exported wrappers as funcs, document that makeLevelLogger is meant for internal delegation rather than replacing exported functions with mutable vars).
Collaborator
|
@copilot address this review feedback #4301 (review) |
Agent-Logs-Url: https://github.com/github/gh-aw-mcpg/sessions/85f29738-c288-4e1b-a0a2-7e46c41d094f Co-authored-by: lpcox <15877973+lpcox@users.noreply.github.com>
Contributor
Author
Addressed in 805548c. I updated the logger refactor to keep exported APIs as 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:
If you need me to access, download, or install something from one of these locations, you can either:
|
This was referenced Apr 22, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR addresses the duplicate-code report by extracting repeated DIFC enforcement decisions shared between unified MCP tool calls and proxy REST handling, and by collapsing repeated logger wrapper patterns. The goal is to reduce drift risk in policy behavior and simplify maintenance of level-based logging APIs.
DIFC pipeline decision logic centralized
internal/difc/pipeline_decisions.gowith shared predicates for:LabelResponseeligibility by operation/modeinternal/server/unified.gointernal/proxy/handler.goLogger 4-wrapper pattern consolidated
internal/logger/common.go:makeLevelLogger(...)makeServerLevelLogger(...)internal/logger/file_logger.gointernal/logger/markdown_logger.gointernal/logger/server_file_logger.goFocused DIFC helper coverage
internal/difc/pipeline_decisions_test.goto lock behavior of shared predicates across operation/mode combinations.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/tmp/go-build3351015982/b509/launcher.test /tmp/go-build3351015982/b509/launcher.test -test.testlogfile=/tmp/go-build3351015982/b509/testlog.txt -test.paniconexit0 -test.timeout=10m0s -test.v=true g_.a -I x_amd64/vet --gdwarf-5 pickfirst/intern-atomic -o x_amd64/vet -E 3Uglm7fcz 9224174/b287/ x_amd64/vet -I . -imultiarch x_amd64/vet(dns block)/tmp/go-build1206797649/b513/launcher.test /tmp/go-build1206797649/b513/launcher.test -test.testlogfile=/tmp/go-build1206797649/b513/testlog.txt -test.paniconexit0 -test.timeout=10m0s(dns block)invalid-host-that-does-not-exist-12345.com/tmp/go-build3351015982/b491/config.test /tmp/go-build3351015982/b491/config.test -test.testlogfile=/tmp/go-build3351015982/b491/testlog.txt -test.paniconexit0 -test.timeout=10m0s -test.v=true g_.a -I x_amd64/vet --gdwarf-5 ternal/engine/wa-atomic -o x_amd64/vet -I piOAqIlI1 -I x_amd64/vet --gdwarf-5 re/xxhash/v2 -o x_amd64/vet(dns block)/tmp/go-build1206797649/b495/config.test /tmp/go-build1206797649/b495/config.test -test.testlogfile=/tmp/go-build1206797649/b495/testlog.txt -test.paniconexit0 -test.timeout=10m0s go1.25.9 -c=4 -nolocalimports -importcfg /tmp/go-build2876961135/b477/importcfg -pack /home/REDACTED/work/gh-aw-mcpg/gh-aw-mcpg/internal/auth/header.go /home/REDACTED/work/gh-aw-mcpg/gh-aw-mcpg/internal/auth/header_test.go(dns block)nonexistent.local/tmp/go-build3351015982/b509/launcher.test /tmp/go-build3351015982/b509/launcher.test -test.testlogfile=/tmp/go-build3351015982/b509/testlog.txt -test.paniconexit0 -test.timeout=10m0s -test.v=true g_.a -I x_amd64/vet --gdwarf-5 pickfirst/intern-atomic -o x_amd64/vet -E 3Uglm7fcz 9224174/b287/ x_amd64/vet -I . -imultiarch x_amd64/vet(dns block)/tmp/go-build1206797649/b513/launcher.test /tmp/go-build1206797649/b513/launcher.test -test.testlogfile=/tmp/go-build1206797649/b513/testlog.txt -test.paniconexit0 -test.timeout=10m0s(dns block)slow.example.com/tmp/go-build3351015982/b509/launcher.test /tmp/go-build3351015982/b509/launcher.test -test.testlogfile=/tmp/go-build3351015982/b509/testlog.txt -test.paniconexit0 -test.timeout=10m0s -test.v=true g_.a -I x_amd64/vet --gdwarf-5 pickfirst/intern-atomic -o x_amd64/vet -E 3Uglm7fcz 9224174/b287/ x_amd64/vet -I . -imultiarch x_amd64/vet(dns block)/tmp/go-build1206797649/b513/launcher.test /tmp/go-build1206797649/b513/launcher.test -test.testlogfile=/tmp/go-build1206797649/b513/testlog.txt -test.paniconexit0 -test.timeout=10m0s(dns block)this-host-does-not-exist-12345.com/tmp/go-build3351015982/b518/mcp.test /tmp/go-build3351015982/b518/mcp.test -test.testlogfile=/tmp/go-build3351015982/b518/testlog.txt -test.paniconexit0 -test.timeout=10m0s -test.v=true .cfg elemetry.io/otel-ifaceassert x_amd64/vet . --gdwarf2 --64 x_amd64/vet -qui�� .cfg om/grpc-ecosystem/grpc-gateway/v2@v2.28.0/runtime/convert.go x_amd64/vet /tmp/go-build370bash g/grpc/internal//usr/bin/runc x86_64-linux-gnu--version x_amd64/vet(dns block)/tmp/go-build1206797649/b522/mcp.test /tmp/go-build1206797649/b522/mcp.test -test.testlogfile=/tmp/go-build1206797649/b522/testlog.txt -test.paniconexit0 -test.timeout=10m0s -uns�� tion_pool.go _monitor.go x_amd64/compile(dns block)If you need me to access, download, or install something from one of these locations, you can either: