fix: resolve duplicate test functions, nil panic, and missing Makefile target

[lpcox]

Problem

Main is broken — make test fails due to 3 issues introduced by recent PRs:

1. Duplicate test functions in internal/server/ — PR [test] Add tests for server.resolveGuardPolicy and normalizeScopeKind #2092 and [test-improver] Improve tests for server requireGuardPolicyIfGuardEnabled #2096 both added validAllowOnlyPolicy() and overlapping TestNormalizeScopeKind_* tests
2. Nil pointer dereference in ParseToolArguments — PR [log] Add debug logging to mcp/tool_result.go #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 feat: proxy mode TLS support and container-native entrypoint #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)

[Copilot]

Pull request overview

Fixes multiple regressions that were breaking make test by deduplicating overlapping server tests, preventing a nil dereference in MCP tool argument parsing, and restoring the missing container-proxy integration test target (plus the corresponding container-based proxy integration tests).

Changes:

• Remove duplicate/overlapping tests in internal/server and align expectations with shared helpers and guard-policy parsing shapes.
• Prevent nil dereference in internal/mcp.ParseToolArguments by moving logging behind the nil checks.
• Restore test-container-proxy Makefile target and add container-based proxy integration tests.

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 7 comments.

Show a summary per file

File	Description
test/integration/proxy_container_test.go	Adds container-tagged integration tests for proxy mode (TLS, auth forwarding, DIFC behavior, logs, gh CLI).
internal/server/resolve_guard_policy_test.go	Removes duplicate helper and fixes guard-policy test inputs/expectations.
internal/server/ensure_guard_initialized_test.go	Removes duplicate normalizeScopeKind tests and points to the canonical test location.
internal/mcp/tool_result.go	Moves a debug log behind nil checks and avoids dereferencing req.Params when absent.
Makefile	Restores test-container-proxy target and adds it to .PHONY.

Comments suppressed due to low confidence (4)

test/integration/proxy_container_test.go:319

• json.Unmarshal error is ignored; if the response isn't valid JSON the test can give misleading results. Capture the error and assert it before checking the decoded slice.

		var issues []interface{}
		json.Unmarshal(body, &issues)
		assert.NotEmpty(t, issues, "Scoped repo should return issues")

test/integration/proxy_container_test.go:339

• json.Unmarshal error is ignored; if the response isn't JSON (or is an API error), this will silently log len(branches)=0. Consider asserting unmarshal success (or at least logging the body on failure) so failures are actionable.

		var branches []interface{}
		json.Unmarshal(body, &branches)
		// Branches may be filtered by the guard if it doesn't label them individually.
		// Just verify we get a valid 200 response — don't assert non-empty.
		t.Logf("Scoped: %d branches", len(branches))

test/integration/proxy_container_test.go:328

• json.Unmarshal error is ignored; assert the unmarshal succeeds so the test fails clearly if the proxy returns an error body or non-JSON.

		var commits []interface{}
		json.Unmarshal(body, &commits)
		assert.NotEmpty(t, commits, "Scoped repo should return commits")

test/integration/proxy_container_test.go:357

• json.Unmarshal error is ignored in the out-of-scope case. Assert it succeeds (or fail with the raw body) so the test doesn't silently accept non-JSON error responses.

		if status == 200 {
			var commits []interface{}
			json.Unmarshal(body, &commits)
			assert.Empty(t, commits, "Out-of-scope should return empty")

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

The tests hardcode host ports (19001–19008) for docker run -p. This can fail on developer machines/CI where those ports are already in use or when tests are run concurrently. Prefer allocating a free port dynamically (or using Docker's ephemeral host port mapping) and discovering the mapped port before making requests.

[Copilot]

json.Unmarshal errors are ignored here, which can allow the test to pass even if the proxy returned non-JSON (e.g., an HTML error page) or a different shape than expected. Capture and assert the unmarshal error (at least require.NoError) before using/logging the decoded slice.

This issue also appears in the following locations of the same file:

• line 317
• line 335
• line 326

Suggested change

	json.Unmarshal(body, &issues)
	err := json.Unmarshal(body, &issues)
	require.NoError(t, err)

[Copilot]

This test skips on any gh failure. On Linux (where it’s expected to work), this would silently mask real regressions (e.g., proxy breaks gh api). Use runtime.GOOS (or a more specific error check) to only skip on macOS/system-trust-store limitations; otherwise fail the test when gh exits non-zero.

[Copilot]

buildContainerImage(t) is invoked in every TestContainerProxy* test, which can make make test-container-proxy very slow and also risks hitting the package -timeout 10m (especially on cold Docker caches/CI). Consider building the image once per package (e.g., TestMain or a sync.Once) and reusing it across tests.

[Copilot]

startContainerProxy allocates a temp log dir and starts a detached container, but cleanup is only done by callers via defer env.stop(t) after a successful return. If any require.* fails inside startContainerProxy (e.g., TLS certs/health never become ready), the container and temp directory will leak. Use t.Cleanup inside startContainerProxy right after creating logDir / starting the container so resources are always reclaimed on failure.

[Copilot]

containerID[:12] assumes docker run -d output is at least 12 chars. If Docker returns an unexpected short output (or includes warnings), this will panic and obscure the real failure. Guard the slice (check length) or log the full trimmed output safely.

Suggested change

	t.Logf("✓ Container started: %s (id=%s)", name, containerID[:12])
	shortID := containerID
	if len(shortID) > 12 {
	shortID = shortID[:12]
	}
	t.Logf("✓ Container started: %s (id=%s)", name, shortID)

[Copilot]

json.Unmarshal error is ignored in the out-of-scope case. If GitHub returns an error payload (rate limit, auth, etc.), the test could pass without actually validating the behavior. Assert the unmarshal succeeds before asserting emptiness.

This issue also appears on line 354 of the same file.