Skip to content

Commit 484c4d8

Browse files
authored
fix: resolve duplicate test functions, nil panic, and missing Makefile target (#2267)
## Problem Main is broken — `make test` fails due to 3 issues introduced by recent PRs: 1. **Duplicate test functions** in `internal/server/` — PR #2092 and #2096 both added `validAllowOnlyPolicy()` and overlapping `TestNormalizeScopeKind_*` tests 2. **Nil pointer dereference** in `ParseToolArguments` — PR #2061 added a log line that accesses `req.Params.Name` before the nil check 3. **Missing Makefile target** — `test-container-proxy` target and container test file were lost in the squash merge of PR #2231 ## Fixes - Remove duplicate `validAllowOnlyPolicy()` from `resolve_guard_policy_test.go` (keep `ensure_guard_initialized_test.go` version) - Remove duplicate `TestNormalizeScopeKind_*` tests from `ensure_guard_initialized_test.go` (keep the more comprehensive set in `resolve_guard_policy_test.go`) - Fix test using string repos pattern (`"github/gh-aw*"`) — must be `[]interface{}` array - Fix assertion expecting `"approved"` — now correctly matches the shared helper's `IntegrityNone` - Move `ParseToolArguments` log line after the nil check - Restore `test-container-proxy` Makefile target and `proxy_container_test.go` (8 container integration tests) ## Verification - `make agent-finished` ✅ all checks pass - `make test-container-proxy` ✅ 7 pass, 1 skip (macOS gh CLI TLS — expected)
2 parents 1c94045 + 262dac1 commit 484c4d8

5 files changed

Lines changed: 474 additions & 86 deletions

File tree

Makefile

Lines changed: 9 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
.PHONY: build lint test test-unit test-integration test-all test-serena test-serena-gateway coverage test-ci format clean install release help agent-finished
1+
.PHONY: build lint test test-unit test-integration test-container-proxy test-all test-serena test-serena-gateway coverage test-ci format clean install release help agent-finished
22

33
# Default target
44
.DEFAULT_GOAL := help
@@ -60,6 +60,14 @@ test-integration:
6060
fi
6161
@go test -v ./test/integration/...
6262

63+
# Run container proxy integration tests (requires Docker and gh CLI)
64+
test-container-proxy:
65+
@echo "Running container proxy integration tests..."
66+
@echo "This will build a Docker image and test proxy mode with TLS."
67+
@echo "Requires: Docker daemon, GitHub token (GITHUB_TOKEN, GH_TOKEN, or gh auth login)"
68+
@echo ""
69+
@go test -v -tags=container -run TestContainerProxy -timeout 10m ./test/integration/...
70+
6371
# Run format, build, lint, and all tests (for agents before completion)
6472
# Optimized: single go mod tidy, no redundant clean/vet/gofmt-check
6573
agent-finished:

internal/mcp/tool_result.go

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -108,16 +108,16 @@ func ConvertToCallToolResult(data interface{}) (*sdk.CallToolResult, error) {
108108
// ParseToolArguments extracts and unmarshals tool arguments from a CallToolRequest.
109109
// Returns the parsed arguments as a map, or an error if parsing fails.
110110
func ParseToolArguments(req *sdk.CallToolRequest) (map[string]interface{}, error) {
111-
logToolResult.Printf("Parsing arguments for tool: %s", req.Params.Name)
112111
var toolArgs map[string]interface{}
113112
if req.Params != nil && req.Params.Arguments != nil {
113+
logToolResult.Printf("Parsing arguments for tool: %s", req.Params.Name)
114114
if err := json.Unmarshal(req.Params.Arguments, &toolArgs); err != nil {
115115
return nil, fmt.Errorf("failed to parse arguments: %w", err)
116116
}
117117
} else {
118118
// No arguments provided, use empty map
119119
toolArgs = make(map[string]interface{})
120120
}
121-
logToolResult.Printf("Parsed %d arguments for tool: %s", len(toolArgs), req.Params.Name)
121+
logToolResult.Printf("Parsed %d arguments", len(toolArgs))
122122
return toolArgs, nil
123123
}

internal/server/ensure_guard_initialized_test.go

Lines changed: 1 addition & 70 deletions
Original file line numberDiff line numberDiff line change
@@ -93,77 +93,8 @@ func validAllowOnlyPolicy() *config.GuardPolicy {
9393
}
9494
}
9595

96-
// ─── normalizeScopeKind ───────────────────────────────────────────────────────
97-
98-
func TestNormalizeScopeKind_Nil(t *testing.T) {
99-
result := normalizeScopeKind(nil)
100-
assert.Nil(t, result)
101-
}
102-
103-
func TestNormalizeScopeKind_EmptyMap(t *testing.T) {
104-
result := normalizeScopeKind(map[string]interface{}{})
105-
require.NotNil(t, result)
106-
assert.Empty(t, result)
107-
}
108-
109-
func TestNormalizeScopeKind_NoScopeKindField(t *testing.T) {
110-
input := map[string]interface{}{
111-
"min-integrity": "none",
112-
"repos": "public",
113-
}
114-
result := normalizeScopeKind(input)
115-
require.NotNil(t, result)
116-
assert.Equal(t, "none", result["min-integrity"])
117-
assert.Equal(t, "public", result["repos"])
118-
assert.NotContains(t, result, "scope_kind")
119-
}
120-
121-
func TestNormalizeScopeKind_ScopeKindAlreadyLowercase(t *testing.T) {
122-
input := map[string]interface{}{
123-
"scope_kind": "composite",
124-
}
125-
result := normalizeScopeKind(input)
126-
assert.Equal(t, "composite", result["scope_kind"])
127-
}
128-
129-
func TestNormalizeScopeKind_ScopeKindUppercase(t *testing.T) {
130-
input := map[string]interface{}{
131-
"scope_kind": "COMPOSITE",
132-
}
133-
result := normalizeScopeKind(input)
134-
assert.Equal(t, "composite", result["scope_kind"])
135-
}
136-
137-
func TestNormalizeScopeKind_ScopeKindMixedCaseWithWhitespace(t *testing.T) {
138-
input := map[string]interface{}{
139-
"scope_kind": " Scoped ",
140-
}
141-
result := normalizeScopeKind(input)
142-
assert.Equal(t, "scoped", result["scope_kind"])
143-
}
144-
145-
func TestNormalizeScopeKind_NonStringScopeKind(t *testing.T) {
146-
// Non-string values must be preserved as-is (the type assertion fails silently).
147-
input := map[string]interface{}{
148-
"scope_kind": 42,
149-
"other": "value",
150-
}
151-
result := normalizeScopeKind(input)
152-
assert.Equal(t, 42, result["scope_kind"])
153-
assert.Equal(t, "value", result["other"])
154-
}
155-
156-
func TestNormalizeScopeKind_DoesNotMutateInput(t *testing.T) {
157-
input := map[string]interface{}{
158-
"scope_kind": "Composite",
159-
}
160-
original := input["scope_kind"]
161-
normalizeScopeKind(input)
162-
// The original map must not be modified — normalizeScopeKind returns a copy.
163-
assert.Equal(t, original, input["scope_kind"])
164-
}
165-
16696
// ─── ensureGuardInitialized ───────────────────────────────────────────────────
97+
// normalizeScopeKind tests live in resolve_guard_policy_test.go
16798

16899
// TestEnsureGuardInitialized_PolicyNil checks that when resolveGuardPolicy returns nil
169100
// (no guard policy configured for the server) the evaluator default mode is returned.

internal/server/resolve_guard_policy_test.go

Lines changed: 2 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -8,17 +8,6 @@ import (
88
"github.com/stretchr/testify/require"
99
)
1010

11-
// validAllowOnlyPolicy returns a valid AllowOnly guard policy for use in tests.
12-
// Uses repos="all" which is one of the two accepted string values ("all" or "public").
13-
func validAllowOnlyPolicy() *config.GuardPolicy {
14-
return &config.GuardPolicy{
15-
AllowOnly: &config.AllowOnlyPolicy{
16-
Repos: "all",
17-
MinIntegrity: "approved",
18-
},
19-
}
20-
}
21-
2211
// validWriteSinkPolicy returns a valid WriteSink guard policy for use in tests.
2312
func validWriteSinkPolicy() *config.GuardPolicy {
2413
return &config.GuardPolicy{
@@ -234,7 +223,7 @@ func TestResolveGuardPolicy_ServerWithValidGuardPolicies(t *testing.T) {
234223
GuardPolicies: map[string]interface{}{
235224
"allow-only": map[string]interface{}{
236225
"min-integrity": "approved",
237-
"repos": "github/gh-aw*",
226+
"repos": []interface{}{"github/gh-aw*"},
238227
},
239228
},
240229
},
@@ -387,7 +376,7 @@ func TestResolveGuardPolicy_GuardFieldSet_ValidGuardPolicy(t *testing.T) {
387376
require.NotNil(t, policy)
388377
assert.Equal(t, "config", source)
389378
require.NotNil(t, policy.AllowOnly)
390-
assert.Equal(t, "approved", policy.AllowOnly.MinIntegrity)
379+
assert.Equal(t, config.IntegrityNone, policy.AllowOnly.MinIntegrity)
391380
}
392381

393382
func TestResolveGuardPolicy_GuardFieldSet_WriteSinkGuardPolicy(t *testing.T) {

0 commit comments

Comments
 (0)