Skip to content

Commit 291b130

Browse files
authored
Deduplicate guard policy JSON roundtrip and enforce logger wrapper level parity (#4155)
Duplicate-code analysis flagged two remaining patterns: repeated policy `Marshal`→`Unmarshal` map conversion in guard/proxy paths, and repeated log-level wrapper sets across logger variants. This PR centralizes the policy conversion path and adds a guardrail test for logger wrapper completeness. - **Policy conversion deduplication (guard + proxy)** - Added `PolicyToMap(policy interface{}) (map[string]interface{}, error)` in `internal/guard/policy_helpers.go`. - Replaced duplicated roundtrip blocks in: - `internal/guard/wasm.go` (`buildStrictLabelAgentPayload`, `BuildLabelAgentPayload`) - `internal/proxy/proxy.go` (`initGuardPolicy`, including `allow-only` extraction path) - Standardized error surface for invalid/non-object policies while preserving caller-specific context. - **Logger wrapper drift prevention** - Added `internal/logger/log_level_wrappers_test.go`. - Test asserts file/markdown/server wrapper sets all cover the same levels as `logFuncs` to prevent partial additions when introducing new log levels. - **Focused helper coverage** - Added `internal/guard/policy_helpers_test.go` with coverage for: - nil policy - non-object policy - unmarshalable policy - deep-copy behavior for nested map mutation safety ```go // shared helper now used by both guard and proxy policy paths payload, err := guard.PolicyToMap(policy) if err != nil { return fmt.Errorf("policy must be a JSON object: %w", err) } ``` > [!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-build73390749/b509/launcher.test /tmp/go-build73390749/b509/launcher.test -test.testlogfile=/tmp/go-build73390749/b509/testlog.txt -test.paniconexit0 -test.timeout=10m0s -test.v=true 1.80.0/internal/resolver/delegatingresolver/delegatingresolver.go xrGu/Ij3Uman6G7fpe4BrxrGu x_amd64/vet --gdwarf-5 envconfig -o x_amd64/vet 7745�� .a ache/go/1.25.8/x-ifaceassert x_amd64/vet --gdwarf-5 --64 -o x_amd64/vet` (dns block) > - `nonexistent.local` > - Triggering command: `/tmp/go-build73390749/b509/launcher.test /tmp/go-build73390749/b509/launcher.test -test.testlogfile=/tmp/go-build73390749/b509/testlog.txt -test.paniconexit0 -test.timeout=10m0s -test.v=true 1.80.0/internal/resolver/delegatingresolver/delegatingresolver.go xrGu/Ij3Uman6G7fpe4BrxrGu x_amd64/vet --gdwarf-5 envconfig -o x_amd64/vet 7745�� .a ache/go/1.25.8/x-ifaceassert x_amd64/vet --gdwarf-5 --64 -o x_amd64/vet` (dns block) > - `slow.example.com` > - Triggering command: `/tmp/go-build73390749/b509/launcher.test /tmp/go-build73390749/b509/launcher.test -test.testlogfile=/tmp/go-build73390749/b509/testlog.txt -test.paniconexit0 -test.timeout=10m0s -test.v=true 1.80.0/internal/resolver/delegatingresolver/delegatingresolver.go xrGu/Ij3Uman6G7fpe4BrxrGu x_amd64/vet --gdwarf-5 envconfig -o x_amd64/vet 7745�� .a ache/go/1.25.8/x-ifaceassert x_amd64/vet --gdwarf-5 --64 -o x_amd64/vet` (dns block) > - `this-host-does-not-exist-12345.com` > - Triggering command: `/tmp/go-build73390749/b518/mcp.test /tmp/go-build73390749/b518/mcp.test -test.testlogfile=/tmp/go-build73390749/b518/testlog.txt -test.paniconexit0 -test.timeout=10m0s -test.v=true fflib@v1.0.0/difflib/difflib.go otection x_amd64/vet --gdwarf-5 --64 lcache/go/1.25.8-bool x_amd64/vet -E 60I2XHAs9 8177450/b287//_c-ifaceassert x_amd64/vet -I . 77450/b287/ x_amd64/vet` (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 cf4fde7 + 1ab7530 commit 291b130

File tree

5 files changed

+147
-19
lines changed

5 files changed

+147
-19
lines changed

internal/guard/policy_helpers.go

Lines changed: 27 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,27 @@
1+
package guard
2+
3+
import (
4+
"encoding/json"
5+
"fmt"
6+
)
7+
8+
// PolicyToMap converts a policy value to a generic map through a JSON roundtrip.
9+
// It returns an error if the value cannot be serialized or does not decode to a
10+
// JSON object.
11+
func PolicyToMap(policy interface{}) (map[string]interface{}, error) {
12+
if policy == nil {
13+
return nil, fmt.Errorf("policy is required")
14+
}
15+
16+
policyJSON, err := json.Marshal(policy)
17+
if err != nil {
18+
return nil, fmt.Errorf("failed to serialize policy: %w", err)
19+
}
20+
21+
var payload map[string]interface{}
22+
if err := json.Unmarshal(policyJSON, &payload); err != nil {
23+
return nil, fmt.Errorf("policy must decode to a JSON object: %w", err)
24+
}
25+
26+
return payload, nil
27+
}
Lines changed: 50 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,50 @@
1+
package guard
2+
3+
import (
4+
"math"
5+
"testing"
6+
7+
"github.com/stretchr/testify/assert"
8+
"github.com/stretchr/testify/require"
9+
)
10+
11+
func TestPolicyToMap(t *testing.T) {
12+
t.Run("returns deep copy for map input", func(t *testing.T) {
13+
policy := map[string]interface{}{
14+
"allow-only": map[string]interface{}{
15+
"repos": "public",
16+
"min-integrity": "none",
17+
},
18+
}
19+
20+
payload, err := PolicyToMap(policy)
21+
require.NoError(t, err)
22+
require.NotNil(t, payload)
23+
24+
allowOnly, ok := payload["allow-only"].(map[string]interface{})
25+
require.True(t, ok)
26+
allowOnly["repos"] = "all"
27+
28+
original, ok := policy["allow-only"].(map[string]interface{})
29+
require.True(t, ok)
30+
assert.Equal(t, "public", original["repos"])
31+
})
32+
33+
t.Run("nil policy returns error", func(t *testing.T) {
34+
_, err := PolicyToMap(nil)
35+
require.Error(t, err)
36+
assert.Contains(t, err.Error(), "policy is required")
37+
})
38+
39+
t.Run("non-object policy returns error", func(t *testing.T) {
40+
_, err := PolicyToMap([]string{"not-an-object"})
41+
require.Error(t, err)
42+
assert.Contains(t, err.Error(), "policy must decode to a JSON object")
43+
})
44+
45+
t.Run("unmarshalable policy returns error", func(t *testing.T) {
46+
_, err := PolicyToMap(math.NaN())
47+
require.Error(t, err)
48+
assert.Contains(t, err.Error(), "failed to serialize policy")
49+
})
50+
}

internal/guard/wasm.go

Lines changed: 6 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -364,13 +364,8 @@ func buildStrictLabelAgentPayload(policy interface{}) (map[string]interface{}, e
364364
}
365365
}
366366

367-
policyJSON, err := json.Marshal(policy)
367+
payload, err := PolicyToMap(policy)
368368
if err != nil {
369-
return nil, fmt.Errorf("failed to serialize label_agent policy: %w", err)
370-
}
371-
372-
var payload map[string]interface{}
373-
if err := json.Unmarshal(policyJSON, &payload); err != nil {
374369
return nil, fmt.Errorf("failed to decode label_agent policy payload: %w", err)
375370
}
376371

@@ -549,20 +544,16 @@ func BuildLabelAgentPayload(policy interface{}, trustedBots []string, trustedUse
549544
return policy
550545
}
551546

552-
// Marshal the policy to a generic map so we can inject the trusted-bots and trusted-users
553-
// keys alongside the allow-only policy without altering the policy itself.
554-
policyJSON, err := json.Marshal(policy)
547+
// Convert the policy to a generic map so we can inject the trusted-bots and
548+
// trusted-users keys alongside the allow-only policy without altering the
549+
// policy itself.
550+
payload, err := PolicyToMap(policy)
555551
if err != nil {
556-
// If we can't marshal the policy, return it as-is; buildStrictLabelAgentPayload
552+
// If we can't convert the policy, return it as-is; buildStrictLabelAgentPayload
557553
// will surface the error later.
558554
return policy
559555
}
560556

561-
var payload map[string]interface{}
562-
if err := json.Unmarshal(policyJSON, &payload); err != nil {
563-
return policy
564-
}
565-
566557
if len(trustedBots) > 0 {
567558
// trusted-bots is a top-level key in the label_agent payload.
568559
// Convert []string to []interface{} for JSON compatibility.
Lines changed: 53 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,53 @@
1+
package logger
2+
3+
import (
4+
"testing"
5+
6+
"github.com/stretchr/testify/assert"
7+
)
8+
9+
func TestLogLevelWrappers_CoverAllRegisteredLevels(t *testing.T) {
10+
fileWrappers := map[LogLevel]func(string, string, ...interface{}){
11+
LogLevelInfo: LogInfo,
12+
LogLevelWarn: LogWarn,
13+
LogLevelError: LogError,
14+
LogLevelDebug: LogDebug,
15+
}
16+
17+
markdownWrappers := map[LogLevel]func(string, string, ...interface{}){
18+
LogLevelInfo: LogInfoMd,
19+
LogLevelWarn: LogWarnMd,
20+
LogLevelError: LogErrorMd,
21+
LogLevelDebug: LogDebugMd,
22+
}
23+
24+
serverWrappers := map[LogLevel]func(string, string, string, ...interface{}){
25+
LogLevelInfo: LogInfoWithServer,
26+
LogLevelWarn: LogWarnWithServer,
27+
LogLevelError: LogErrorWithServer,
28+
LogLevelDebug: LogDebugWithServer,
29+
}
30+
31+
expectedLevels := make([]LogLevel, 0, len(logFuncs))
32+
for level := range logFuncs {
33+
expectedLevels = append(expectedLevels, level)
34+
}
35+
36+
fileLevels := make([]LogLevel, 0, len(fileWrappers))
37+
for level := range fileWrappers {
38+
fileLevels = append(fileLevels, level)
39+
}
40+
assert.ElementsMatch(t, expectedLevels, fileLevels)
41+
42+
markdownLevels := make([]LogLevel, 0, len(markdownWrappers))
43+
for level := range markdownWrappers {
44+
markdownLevels = append(markdownLevels, level)
45+
}
46+
assert.ElementsMatch(t, expectedLevels, markdownLevels)
47+
48+
serverLevels := make([]LogLevel, 0, len(serverWrappers))
49+
for level := range serverWrappers {
50+
serverLevels = append(serverLevels, level)
51+
}
52+
assert.ElementsMatch(t, expectedLevels, serverLevels)
53+
}

internal/proxy/proxy.go

Lines changed: 11 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -156,14 +156,21 @@ func (s *Server) initGuardPolicy(ctx context.Context, policyJSON string, trusted
156156
}
157157

158158
// Validate the policy structure
159-
policyMap, ok := policy.(map[string]interface{})
160-
if !ok {
161-
return fmt.Errorf("policy must be a JSON object")
159+
policyMap, err := guard.PolicyToMap(policy)
160+
if err != nil {
161+
return fmt.Errorf("policy must be a JSON object: %w", err)
162162
}
163163
guardPolicy := &config.GuardPolicy{}
164164
if ao, hasAO := policyMap["allow-only"]; hasAO {
165165
logProxy.Printf("Parsing allow-only policy from guard configuration")
166-
aoBytes, _ := json.Marshal(ao)
166+
aoMap, err := guard.PolicyToMap(ao)
167+
if err != nil {
168+
return fmt.Errorf("invalid allow-only policy: %w", err)
169+
}
170+
aoBytes, err := json.Marshal(aoMap)
171+
if err != nil {
172+
return fmt.Errorf("invalid allow-only policy: %w", err)
173+
}
167174
var allowOnly config.AllowOnlyPolicy
168175
if err := json.Unmarshal(aoBytes, &allowOnly); err != nil {
169176
return fmt.Errorf("invalid allow-only policy: %w", err)

0 commit comments

Comments
 (0)