Refactor duplicated DIFC pipeline decisions and logger level wrappers (#4301)

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**
- Added `internal/difc/pipeline_decisions.go` with shared predicates
for:
    - coarse-deny bypass on reads
    - `LabelResponse` eligibility by operation/mode
    - strict-mode blocking on filtered collections
    - propagate-mode label accumulation rules
  - Replaced inline condition blocks in:
    - `internal/server/unified.go`
    - `internal/proxy/handler.go`

- **Logger 4-wrapper pattern consolidated**
  - Added generic wrapper constructors in `internal/logger/common.go`:
    - `makeLevelLogger(...)`
    - `makeServerLevelLogger(...)`
- Replaced repetitive per-level one-liner wrappers with generated
wrappers while preserving public API names/signatures in:
    - `internal/logger/file_logger.go`
    - `internal/logger/markdown_logger.go`
    - `internal/logger/server_file_logger.go`

- **Focused DIFC helper coverage**
- Added `internal/difc/pipeline_decisions_test.go` to lock behavior of
shared predicates across operation/mode combinations.

```go
// internal/difc/pipeline_decisions.go
func ShouldCallLabelResponse(operation OperationType, enforcementMode EnforcementMode) bool {
	isPureWrite := operation == OperationWrite
	return !isPureWrite && (operation != OperationReadWrite || enforcementMode != EnforcementStrict)
}
```

> [!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-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)
> - Triggering command: `/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`
> - Triggering command: `/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)
> - Triggering command: `/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`
> - Triggering command: `/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)
> - Triggering command: `/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`
> - Triggering command: `/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)
> - Triggering command: `/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`
> - Triggering command: `/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)
> - Triggering command: `/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:
>
> - 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>

Author: lpcox

internal/difc/pipeline_decisions.go

@@ -0,0 +1,26 @@
+package difc
+
+// ShouldBypassCoarseDeny returns true when a coarse-grained deny should still
+// proceed to backend execution so Phase 5 can enforce per-item policy.
+func ShouldBypassCoarseDeny(operation OperationType) bool {
+	return operation == OperationRead
+}
+
+// ShouldCallLabelResponse returns true when guards should label response data
+// for possible fine-grained filtering.
+func ShouldCallLabelResponse(operation OperationType, enforcementMode EnforcementMode) bool {
+	isPureWrite := operation == OperationWrite
+	return !isPureWrite && (operation != OperationReadWrite || enforcementMode != EnforcementStrict)
+}
+
+// ShouldBlockFilteredResponse returns true when filtered items should block the
+// whole response instead of returning a partially filtered result.
+func ShouldBlockFilteredResponse(enforcementMode EnforcementMode, filteredCount int) bool {
+	return enforcementMode == EnforcementStrict && filteredCount > 0
+}
+
+// ShouldAccumulateReadLabels returns true when read labels should be
+// accumulated back into the agent label set.
+func ShouldAccumulateReadLabels(operation OperationType, enforcementMode EnforcementMode) bool {
+	return operation != OperationWrite && enforcementMode == EnforcementPropagate
+}

internal/difc/pipeline_decisions_test.go

@@ -0,0 +1,36 @@
+package difc
+
+import (
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+)
+
+func TestShouldBypassCoarseDeny(t *testing.T) {
+	assert.True(t, ShouldBypassCoarseDeny(OperationRead))
+	assert.False(t, ShouldBypassCoarseDeny(OperationWrite))
+	assert.False(t, ShouldBypassCoarseDeny(OperationReadWrite))
+}
+
+func TestShouldCallLabelResponse(t *testing.T) {
+	assert.False(t, ShouldCallLabelResponse(OperationWrite, EnforcementStrict))
+	assert.False(t, ShouldCallLabelResponse(OperationReadWrite, EnforcementStrict))
+	assert.True(t, ShouldCallLabelResponse(OperationRead, EnforcementStrict))
+	assert.True(t, ShouldCallLabelResponse(OperationReadWrite, EnforcementFilter))
+	assert.True(t, ShouldCallLabelResponse(OperationReadWrite, EnforcementPropagate))
+}
+
+func TestShouldBlockFilteredResponse(t *testing.T) {
+	assert.True(t, ShouldBlockFilteredResponse(EnforcementStrict, 1))
+	assert.False(t, ShouldBlockFilteredResponse(EnforcementStrict, 0))
+	assert.False(t, ShouldBlockFilteredResponse(EnforcementFilter, 3))
+	assert.False(t, ShouldBlockFilteredResponse(EnforcementPropagate, 2))
+}
+
+func TestShouldAccumulateReadLabels(t *testing.T) {
+	assert.True(t, ShouldAccumulateReadLabels(OperationRead, EnforcementPropagate))
+	assert.True(t, ShouldAccumulateReadLabels(OperationReadWrite, EnforcementPropagate))
+	assert.False(t, ShouldAccumulateReadLabels(OperationWrite, EnforcementPropagate))
+	assert.False(t, ShouldAccumulateReadLabels(OperationRead, EnforcementStrict))
+	assert.False(t, ShouldAccumulateReadLabels(OperationRead, EnforcementFilter))
+}

internal/logger/common.go

@@ -183,38 +183,64 @@ import (
 
 // Log-Level Quad-Function Pattern
 //
-// Three sets of four public functions — one set per logger variant — share an identical
-// structural pattern where each function is a one-liner that delegates to an internal
-// helper with the appropriate LogLevel constant:
+// Three sets of four public functions — one set per logger variant — share an
+// identical structure where each exported one-liner delegates to an unexported
+// per-level closure created by helper constructors in this file:
 //
 //	func Log<Level>(category, format string, args ...interface{}) {
-//	    <internalHelper>(LogLevel<Level>, category, format, args...)
+//	    log<level>(category, format, args...)
 //	}
 //
-// The three sets and their internal helpers are:
+// The three sets and their internal dispatch helpers are:
 //
 //	file_logger.go       LogInfo / LogWarn / LogError / LogDebug          → logWithLevel
 //	markdown_logger.go   LogInfoMd / LogWarnMd / LogErrorMd / LogDebugMd  → logWithMarkdown
 //	server_file_logger.go LogInfoWithServer / ... / LogDebugWithServer    → logWithLevelAndServer
 //
-// This pattern is intentionally kept across the three files because:
+// This pattern keeps exported APIs immutable (`func` declarations) while still
+// eliminating repeated inline level wiring.
+//
+// The makeLevelLogger and makeServerLevelLogger helpers are for internal
+// delegation only and should not replace exported functions with reassignable
+// function variables.
+//
+// This remains intentionally consistent across the three files because:
 //   - Each set is a distinct public API with a different signature and set of callers.
-//   - The one-liner wrappers are trivial and unlikely to diverge.
-//   - Go lacks the metaprogramming to eliminate them without sacrificing readability.
+//   - The exported wrappers preserve a stable, non-mutable API surface.
+//   - Internal closure generation removes repetitive level-binding boilerplate.
 //
 // The shared logFuncs map below centralises the LogLevel → log-function
 // mapping so that the internal helpers (logWithMarkdown, logWithLevelAndServer)
 // do not need their own switch-on-level blocks.
 //
 // When adding a new LogLevel constant (e.g., LogLevelTrace):
 //  1. Add a new entry to the logFuncs map below.
-//  2. Add a new LogTrace wrapper to each of the three files above.
+//  2. Add a new internal per-level closure and exported wrapper in each of the
+//     three files above.
 //
 // logFuncs maps each LogLevel to its corresponding global log function.
 // This eliminates repeated switch-on-level blocks in logWithMarkdown
 // (markdown_logger.go) and logWithLevelAndServer (server_file_logger.go).
 // When adding a new LogLevel constant, add a corresponding entry here so
 // that all dispatch sites automatically support the new level.
+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...)
+	}
+}
+
+func makeServerLevelLogger(
+	dispatch func(serverID string, level LogLevel, category, format string, args ...interface{}),
+	level LogLevel,
+) func(serverID, category, format string, args ...interface{}) {
+	return func(serverID, category, format string, args ...interface{}) {
+		dispatch(serverID, level, category, format, args...)
+	}
+}
+
 var logFuncs = map[LogLevel]func(string, string, ...interface{}){
 	LogLevelInfo:  LogInfo,
 	LogLevelWarn:  LogWarn,

internal/logger/file_logger.go

@@ -113,24 +113,31 @@ func logWithLevel(level LogLevel, category, format string, args ...interface{})
 	})
 }
 
-// LogInfo logs an informational message
+var (
+	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{}) {
-	logWithLevel(LogLevelInfo, category, format, args...)
+	logInfo(category, format, args...)
 }
 
-// LogWarn logs a warning message
+// LogWarn logs a warning message.
 func LogWarn(category, format string, args ...interface{}) {
-	logWithLevel(LogLevelWarn, category, format, args...)
+	logWarn(category, format, args...)
 }
 
-// LogError logs an error message
+// LogError logs an error message.
 func LogError(category, format string, args ...interface{}) {
-	logWithLevel(LogLevelError, category, format, args...)
+	logError(category, format, args...)
 }
 
-// LogDebug logs a debug message
+// LogDebug logs a debug message.
 func LogDebug(category, format string, args ...interface{}) {
-	logWithLevel(LogLevelDebug, category, format, args...)
+	logDebug(category, format, args...)
 }
 
 // CloseGlobalLogger closes the global file logger

internal/logger/markdown_logger.go

@@ -180,24 +180,31 @@ func logWithMarkdown(level LogLevel, category, format string, args ...interface{
 	})
 }
 
-// LogInfoMd logs to both regular and markdown loggers
+var (
+	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{}) {
-	logWithMarkdown(LogLevelInfo, category, format, args...)
+	logInfoMd(category, format, args...)
 }
 
-// LogWarnMd logs to both regular and markdown loggers
+// LogWarnMd logs to both regular and markdown loggers.
 func LogWarnMd(category, format string, args ...interface{}) {
-	logWithMarkdown(LogLevelWarn, category, format, args...)
+	logWarnMd(category, format, args...)
 }
 
-// LogErrorMd logs to both regular and markdown loggers
+// LogErrorMd logs to both regular and markdown loggers.
 func LogErrorMd(category, format string, args ...interface{}) {
-	logWithMarkdown(LogLevelError, category, format, args...)
+	logErrorMd(category, format, args...)
 }
 
-// LogDebugMd logs to both regular and markdown loggers
+// LogDebugMd logs to both regular and markdown loggers.
 func LogDebugMd(category, format string, args ...interface{}) {
-	logWithMarkdown(LogLevelDebug, category, format, args...)
+	logDebugMd(category, format, args...)
 }
 
 // CloseMarkdownLogger closes the global markdown logger

internal/logger/server_file_logger.go

@@ -155,24 +155,31 @@ func logWithLevelAndServer(serverID string, level LogLevel, category, format str
 	}
 }
 
-// LogInfoWithServer logs an informational message to the server-specific log file
+var (
+	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{}) {
-	logWithLevelAndServer(serverID, LogLevelInfo, category, format, args...)
+	logInfoWithServer(serverID, category, format, args...)
 }
 
-// LogWarnWithServer logs a warning message to the server-specific log file
+// LogWarnWithServer logs a warning message to the server-specific log file.
 func LogWarnWithServer(serverID, category, format string, args ...interface{}) {
-	logWithLevelAndServer(serverID, LogLevelWarn, category, format, args...)
+	logWarnWithServer(serverID, category, format, args...)
 }
 
-// LogErrorWithServer logs an error message to the server-specific log file
+// LogErrorWithServer logs an error message to the server-specific log file.
 func LogErrorWithServer(serverID, category, format string, args ...interface{}) {
-	logWithLevelAndServer(serverID, LogLevelError, category, format, args...)
+	logErrorWithServer(serverID, category, format, args...)
 }
 
-// LogDebugWithServer logs a debug message to the server-specific log file
+// LogDebugWithServer logs a debug message to the server-specific log file.
 func LogDebugWithServer(serverID, category, format string, args ...interface{}) {
-	logWithLevelAndServer(serverID, LogLevelDebug, category, format, args...)
+	logDebugWithServer(serverID, category, format, args...)
 }
 
 // CloseServerFileLogger closes the global server file logger

internal/proxy/handler.go

@@ -178,7 +178,7 @@ func (h *proxyHandler) handleWithDIFC(w http.ResponseWriter, r *http.Request, pa
 	evalResult := s.evaluator.Evaluate(agentLabels.Secrecy, agentLabels.Integrity, resource, operation)
 
 	if !evalResult.IsAllowed() {
-		if operation == difc.OperationRead {
+		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 {
@@ -266,7 +266,7 @@ func (h *proxyHandler) handleWithDIFC(w http.ResponseWriter, r *http.Request, pa
 			}
 
 			// Strict mode: block entire response if any item filtered
-			if s.enforcementMode == difc.EnforcementStrict && filtered.GetFilteredCount() > 0 {
+			if difc.ShouldBlockFilteredResponse(s.enforcementMode, filtered.GetFilteredCount()) {
 				logHandler.Printf("[DIFC] STRICT: blocking response — %d filtered items", filtered.GetFilteredCount())
 				writeDIFCForbidden(w, fmt.Sprintf("DIFC policy violation: %d of %d items not accessible",
 					filtered.GetFilteredCount(), filtered.TotalCount))
@@ -318,7 +318,7 @@ func (h *proxyHandler) handleWithDIFC(w http.ResponseWriter, r *http.Request, pa
 	}
 
 	// **Phase 6: Label accumulation (propagate mode)**
-	if s.enforcementMode == difc.EnforcementPropagate && labeledData != nil {
+	if labeledData != nil && difc.ShouldAccumulateReadLabels(operation, s.enforcementMode) {
 		overall := labeledData.Overall()
 		agentLabels.AccumulateFromRead(overall)
 		logHandler.Printf("[DIFC] Phase 6: accumulated labels")

internal/server/unified.go

@@ -531,7 +531,7 @@ func (us *UnifiedServer) callBackendTool(ctx context.Context, serverID, toolName
 	// For read operations in any mode, we skip the coarse-grained block
 	// and let the request proceed. Fine-grained filtering at Phase 5 will filter
 	// individual items from the response based on their actual labels from LabelResponse().
-	isReadOperation := (operation == difc.OperationRead)
+	isReadOperation := difc.ShouldBypassCoarseDeny(operation)
 	result := requestEvaluator.Evaluate(agentLabels.Secrecy, agentLabels.Integrity, resource, operation)
 
 	if !result.IsAllowed() {
@@ -603,8 +603,7 @@ func (us *UnifiedServer) callBackendTool(ctx context.Context, serverID, toolName
 	// Per spec: LabelResponse() is only called for read operations in all modes,
 	// and for read-write operations in filter/propagate modes.
 	// For write operations and read-write in strict mode, skip LabelResponse().
-	isPureWrite := (operation == difc.OperationWrite)
-	shouldCallLabelResponse := !isPureWrite && (operation != difc.OperationReadWrite || enforcementMode != difc.EnforcementStrict)
+	shouldCallLabelResponse := difc.ShouldCallLabelResponse(operation, enforcementMode)
 
 	var labeledData difc.LabeledData
 	if shouldCallLabelResponse {
@@ -631,7 +630,7 @@ func (us *UnifiedServer) callBackendTool(ctx context.Context, serverID, toolName
 				filtered.GetAccessibleCount(), filtered.TotalCount)
 
 			// **Strict mode: block entire response if ANY item is filtered**
-			if enforcementMode == difc.EnforcementStrict && filtered.GetFilteredCount() > 0 {
+			if difc.ShouldBlockFilteredResponse(enforcementMode, filtered.GetFilteredCount()) {
 				logger.LogWarn("difc", "STRICT MODE: Blocking entire response - %d/%d items violate DIFC policy",
 					filtered.GetFilteredCount(), filtered.TotalCount)
 				blockErr := fmt.Errorf("DIFC policy violation: %d of %d items in response are not accessible to agent %s",
@@ -664,7 +663,7 @@ func (us *UnifiedServer) callBackendTool(ctx context.Context, serverID, toolName
 		// **Phase 6: Accumulate labels from this operation (for reads in PROPAGATE mode only)**
 		// Label accumulation should only happen when mode is EnforcementPropagate
 		// Filter mode does NOT accumulate - it just filters what the agent can see
-		if !isPureWrite && enforcementMode == difc.EnforcementPropagate {
+		if difc.ShouldAccumulateReadLabels(operation, enforcementMode) {
 			overall := labeledData.Overall()
 			agentLabels.AccumulateFromRead(overall)
 			logUnified.Printf("[DIFC] Agent %s accumulated labels (propagate mode) | Secrecy: %v | Integrity: %v",
@@ -675,7 +674,7 @@ func (us *UnifiedServer) callBackendTool(ctx context.Context, serverID, toolName
 		finalResult = backendResult
 
 		// **Phase 6: Accumulate labels from resource (for reads in PROPAGATE mode only)**
-		if !isPureWrite && enforcementMode == difc.EnforcementPropagate {
+		if difc.ShouldAccumulateReadLabels(operation, enforcementMode) {
 			agentLabels.AccumulateFromRead(resource)
 			logUnified.Printf("[DIFC] Agent %s accumulated labels (propagate mode) | Secrecy: %v | Integrity: %v",
 				agentID, agentLabels.GetSecrecyTags(), agentLabels.GetIntegrityTags())