diff --git a/internal/config/config_core_agent_id_coverage_test.go b/internal/config/config_core_agent_id_coverage_test.go new file mode 100644 index 000000000..cce9d344f --- /dev/null +++ b/internal/config/config_core_agent_id_coverage_test.go @@ -0,0 +1,292 @@ +package config + +import ( + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// TestEffectiveAgentID tests all branches of GatewayConfig.effectiveAgentID. +// +// effectiveAgentID is an unexported helper that returns the resolved agent ID: +// - nil receiver → "" +// - agentIDExplicit set → return AgentID (even if empty) +// - AgentID non-empty (and not explicit) → return AgentID +// - fallback → return APIKey +func TestEffectiveAgentID(t *testing.T) { + tests := []struct { + name string + gateway *GatewayConfig + want string + }{ + { + name: "nil receiver returns empty string", + gateway: nil, + want: "", + }, + { + name: "explicit agent_id returns AgentID", + gateway: &GatewayConfig{ + AgentID: "explicit-agent", + agentIDExplicit: true, + }, + want: "explicit-agent", + }, + { + name: "explicit agent_id even when empty returns empty AgentID", + gateway: &GatewayConfig{ + AgentID: "", + APIKey: "legacy-key", + agentIDExplicit: true, + }, + want: "", + }, + { + name: "non-explicit non-empty AgentID returns AgentID", + gateway: &GatewayConfig{ + AgentID: "derived-agent", + APIKey: "legacy-key", + agentIDExplicit: false, + }, + want: "derived-agent", + }, + { + name: "non-explicit empty AgentID falls back to APIKey", + gateway: &GatewayConfig{ + AgentID: "", + APIKey: "fallback-key", + agentIDExplicit: false, + }, + want: "fallback-key", + }, + { + name: "zero-value GatewayConfig returns empty string", + gateway: &GatewayConfig{}, + want: "", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got := tt.gateway.effectiveAgentID() + assert.Equal(t, tt.want, got) + }) + } +} + +// TestNormalizeAgentID tests all branches of GatewayConfig.normalizeAgentID. +// +// normalizeAgentID resolves the precedence between the preferred agent_id field +// and the deprecated api_key alias. It is called after TOML/JSON parsing with +// boolean flags indicating which fields were explicitly set. +func TestNormalizeAgentID(t *testing.T) { + tests := []struct { + name string + gateway *GatewayConfig + agentIDDefined bool + legacyAPIKeyDefined bool + source string + wantAgentID string + wantAPIKey string + wantExplicit bool + }{ + { + name: "nil receiver is a no-op", + gateway: nil, + agentIDDefined: false, + legacyAPIKeyDefined: false, + source: "test", + // All zero values — just verifying no panic. + }, + { + name: "neither field defined leaves zero values", + gateway: &GatewayConfig{}, + agentIDDefined: false, + legacyAPIKeyDefined: false, + source: "test", + wantAgentID: "", + wantAPIKey: "", + wantExplicit: false, + }, + { + name: "only agent_id defined marks explicit and sets APIKey alias", + gateway: &GatewayConfig{ + AgentID: "my-agent", + }, + agentIDDefined: true, + legacyAPIKeyDefined: false, + source: "TOML", + wantAgentID: "my-agent", + wantAPIKey: "my-agent", + wantExplicit: true, + }, + { + name: "only api_key defined copies api_key to AgentID", + gateway: &GatewayConfig{ + APIKey: "legacy-key", + }, + agentIDDefined: false, + legacyAPIKeyDefined: true, + source: "TOML", + wantAgentID: "legacy-key", + wantAPIKey: "legacy-key", + wantExplicit: false, + }, + { + name: "both defined with same value uses agent_id, marks explicit", + gateway: &GatewayConfig{ + AgentID: "shared-id", + APIKey: "shared-id", + }, + agentIDDefined: true, + legacyAPIKeyDefined: true, + source: "stdin JSON", + wantAgentID: "shared-id", + wantAPIKey: "shared-id", + wantExplicit: true, + }, + { + name: "both defined with different values: agent_id wins, conflict logged", + gateway: &GatewayConfig{ + AgentID: "new-agent-id", + APIKey: "old-api-key", + }, + agentIDDefined: true, + legacyAPIKeyDefined: true, + source: "TOML", + wantAgentID: "new-agent-id", + wantAPIKey: "new-agent-id", + wantExplicit: true, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + // nil receiver case: just confirm no panic. + if tt.gateway == nil { + assert.NotPanics(t, func() { + tt.gateway.normalizeAgentID(tt.agentIDDefined, tt.legacyAPIKeyDefined, tt.source) + }) + return + } + + tt.gateway.normalizeAgentID(tt.agentIDDefined, tt.legacyAPIKeyDefined, tt.source) + + assert.Equal(t, tt.wantAgentID, tt.gateway.AgentID, "AgentID mismatch") + assert.Equal(t, tt.wantAPIKey, tt.gateway.APIKey, "APIKey mismatch") + assert.Equal(t, tt.wantExplicit, tt.gateway.agentIDExplicit, "agentIDExplicit mismatch") + }) + } +} + +// TestLoadFromFile_BothAgentIDAndAPIKeySet_DifferentValues verifies that when +// a TOML config defines both [gateway].agent_id and [gateway].api_key with +// different values, LoadFromFile loads successfully, uses agent_id as the +// authoritative identifier, and marks it explicit. +// +// This exercises the conflict-detection branch in normalizeAgentID (the +// "using gateway.agent_id" deprecation warning) that is not reached by any +// other test, since all other tests set at most one of the two fields. +func TestLoadFromFile_BothAgentIDAndAPIKeySet_DifferentValues(t *testing.T) { + path := writeTempTOML(t, ` +[gateway] +agent_id = "preferred-agent-id" +api_key = "old-legacy-key" + +[servers.github] +command = "docker" +args = ["run", "--rm", "-i", "ghcr.io/github/github-mcp-server:latest"] +`) + + cfg, err := LoadFromFile(path) + require.NoError(t, err) + require.NotNil(t, cfg) + require.NotNil(t, cfg.Gateway) + + // agent_id always wins over api_key when both are present. + assert.Equal(t, "preferred-agent-id", cfg.Gateway.AgentID, "agent_id should be used as AgentID") + assert.Equal(t, "preferred-agent-id", cfg.Gateway.APIKey, "APIKey alias should mirror AgentID") + assert.True(t, cfg.Gateway.agentIDExplicit, "agentIDExplicit should be true when agent_id is present") +} + +// TestLoadFromFile_BothAgentIDAndAPIKeySet_SameValue verifies that when a TOML +// config sets agent_id and api_key to the same value, LoadFromFile succeeds and +// the gateway uses that value as the authoritative identifier. +func TestLoadFromFile_BothAgentIDAndAPIKeySet_SameValue(t *testing.T) { + path := writeTempTOML(t, ` +[gateway] +agent_id = "shared-key" +api_key = "shared-key" + +[servers.github] +command = "docker" +args = ["run", "--rm", "-i", "ghcr.io/github/github-mcp-server:latest"] +`) + + cfg, err := LoadFromFile(path) + require.NoError(t, err) + require.NotNil(t, cfg) + require.NotNil(t, cfg.Gateway) + + assert.Equal(t, "shared-key", cfg.Gateway.AgentID) + assert.Equal(t, "shared-key", cfg.Gateway.APIKey) + assert.True(t, cfg.Gateway.agentIDExplicit) +} + +// TestLoadFromFile_OnlyAgentIDSet verifies that when only [gateway].agent_id +// is set, the gateway uses it as the identifier and marks it explicit. +func TestLoadFromFile_OnlyAgentIDSet(t *testing.T) { + path := writeTempTOML(t, ` +[gateway] +agent_id = "only-agent-id" + +[servers.github] +command = "docker" +args = ["run", "--rm", "-i", "ghcr.io/github/github-mcp-server:latest"] +`) + + cfg, err := LoadFromFile(path) + require.NoError(t, err) + require.NotNil(t, cfg) + require.NotNil(t, cfg.Gateway) + + assert.Equal(t, "only-agent-id", cfg.Gateway.AgentID) + assert.Equal(t, "only-agent-id", cfg.Gateway.APIKey, "APIKey alias should mirror AgentID") + assert.True(t, cfg.Gateway.agentIDExplicit) +} + +// TestLoadFromFile_OnlyAPIKeySet verifies that when only the deprecated +// [gateway].api_key is set, its value is promoted to AgentID. +func TestLoadFromFile_OnlyAPIKeySet(t *testing.T) { + path := writeTempTOML(t, ` +[gateway] +api_key = "only-api-key" + +[servers.github] +command = "docker" +args = ["run", "--rm", "-i", "ghcr.io/github/github-mcp-server:latest"] +`) + + cfg, err := LoadFromFile(path) + require.NoError(t, err) + require.NotNil(t, cfg) + require.NotNil(t, cfg.Gateway) + + assert.Equal(t, "only-api-key", cfg.Gateway.AgentID, "api_key should be promoted to AgentID") + assert.Equal(t, "only-api-key", cfg.Gateway.APIKey) + assert.False(t, cfg.Gateway.agentIDExplicit, "agentIDExplicit should be false when only api_key is set") +} + +// TestGetAgentID_ExplicitAgentID verifies that GetAgentID returns the +// explicitly configured agent_id even when APIKey is also set. +func TestGetAgentID_ExplicitAgentID(t *testing.T) { + cfg := &Config{ + Gateway: &GatewayConfig{ + AgentID: "my-explicit-agent", + APIKey: "my-legacy-key", + agentIDExplicit: true, + }, + } + assert.Equal(t, "my-explicit-agent", cfg.GetAgentID()) +} diff --git a/internal/config/validate_helpers_coverage_test.go b/internal/config/validate_helpers_coverage_test.go index a09cb24ec..f8f20334e 100644 --- a/internal/config/validate_helpers_coverage_test.go +++ b/internal/config/validate_helpers_coverage_test.go @@ -102,6 +102,29 @@ func TestValidateToolResponseFilters_DirectCall(t *testing.T) { wantErr: true, errSubstr: "custom.path contains an empty tool name", }, + { + // $ENV.KEY is valid jq syntax and compiles successfully, but it triggers the + // gojq environ-loader option during compilation. This ensures the + // validateToolResponseFilters gojq.WithEnvironLoader closure is executed. + name: "filter using $ENV calls the env loader during compilation", + filters: map[string]string{ + "my_tool": "$ENV.SOME_VARIABLE", + }, + jsonPath: "mcpServers.myserver.tool_response_filters", + wantErr: false, + }, + { + // $undefinedVar | .field parses successfully but fails at gojq.Compile because + // the variable is never bound. This ensures the compile-error path in + // validateToolResponseFilters is exercised (distinct from parse errors). + name: "filter with undefined variable fails at compile step not parse step", + filters: map[string]string{ + "my_tool": "$undefinedVar | .field", + }, + jsonPath: "mcpServers.myserver.tool_response_filters", + wantErr: true, + errSubstr: "my_tool contains an invalid jq expression", + }, } for _, tt := range tests {