fix(mcp): add JSON struct tags and call SetDefaults before Validate by sks · Pull Request #8 · stackgenhq/genie

sks · 2026-03-01T23:52:55Z

Problem

MCPServerConfig, MCPConfig, and RetryConfig only had yaml/toml struct tags. When MCP configs are stored as JSONB in PostgreSQL and deserialized via json.Unmarshal, fields like server_url didn't map to ServerURL, causing "server_url is required for sse transport" errors.

Additionally, NewClient called Validate() before SetDefaults(), so empty sub-configs like {"retry": {}} failed validation on zero-valued fields.

Changes

config.go: Added json struct tags to all fields in MCPConfig, MCPServerConfig, and RetryConfig
client.go: Call SetDefaults() on each server config before Validate() in NewClient()

Testing

Verified end-to-end with Guild: Terraform-deployed github-scm agent with MCP SSE transport successfully called github_list_prs and returned real PR data from GitHub.

- justification.go: handle sjson.DeleteBytes error, return found bool so callers can distinguish missing vs empty _justification - mw_hitl.go: use found bool to always strip _justification when present - configbased_provider.go: gate auditSecretLookup on non-empty resolved values in all three env-fallback paths - websearch.go: populate GetSecretRequest.Reason for credential lookup - config.go: update comment to mention Disabled flag alongside Enabled - mw_contextmode.go: implement word-boundary split fallback in chunkText, precompute lowercase chunks in scoreChunks for perf, fix QF1012 lints

MCPServerConfig, MCPConfig, and RetryConfig only had yaml/toml struct tags, causing json.Unmarshal to use Go default field names (e.g. ServerURL instead of server_url). This broke deserialization when MCP configs are stored as JSONB in a database and loaded via json.Unmarshal. Additionally, NewClient now calls SetDefaults() on each server config before Validate(). This prevents empty sub-configs (e.g. {"retry": {}}) from failing validation on zero-valued fields like backoff_factor.

Copilot

Pull request overview

This PR fixes MCP configuration handling when configs are stored as JSON (e.g., JSONB in Postgres) by ensuring snake_case JSON keys map correctly to Go structs, and by applying defaults before validation so empty sub-configs don’t fail validation.

Changes:

Add json struct tags to MCPConfig, MCPServerConfig, and RetryConfig fields.
In NewClient, call SetDefaults() on each server config before calling Validate().

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.

File	Description
pkg/mcp/config.go	Adds JSON tags to MCP config structs to support JSON unmarshalling alongside YAML/TOML.
pkg/mcp/client.go	Applies server defaults before validation during client creation.

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

pkg/mcp/client.go

pkg/mcp/config.go

codecov · 2026-03-02T00:11:02Z

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

- Remove duplicate SetDefaults() call in NewClient server loop - Fix Timeout comment: 10s -> 60s to match actual default - Fix InitialBackoff comment: document that 0 is allowed (unset/default) - Add JSON deserialization tests for snake_case JSONB/PostgreSQL use case - Add SetDefaults-before-Validate integration tests - Add comprehensive tests for shouldIncludeTool, buildStdioEnv, expandEnvValue, convertAndFilterTools, NewClient, Close, GetTools - Test coverage: 32.5% -> 79.5%

… methods - Introduce MCPCaller interface with counterfeiter fake for testability - Add 8 test scenarios for ClientTool.Call: valid args, empty args, invalid JSON, MCP server errors, multi-text, image, mixed content, empty response - Add tests for Client.shouldIncludeTool (include/exclude/both/none) - Add tests for Client.buildStdioEnv (with/without secret provider) - Add tests for Client.expandEnvValue (single/multiple vars, failures) - Add tests for GetTools and NewClient validation - Add Arrange-Act-Assert (AAA) rule to Agents.md testing requirements - All tests follow AAA pattern per new coding standard

… fake

… to README - CHANGELOG: add entries for justification error handling, audit gating, chunking word-boundary fix, scoreChunks optimization, MCPCaller interface, MCP unit tests, and AAA testing rule - README: add early-stage API stability notice — library signatures may evolve as Genie matures toward v1.0

Copilot

Pull request overview

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

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

pkg/mcp/client_internal_test.go

pkg/mcp/client.go

pkg/mcp/config.go

pkg/mcp/config_test.go

- Update README timeout from 10s to 60s across all examples and config table - Remove omitempty from MaxRetries JSON/YAML/TOML tags to preserve explicit zero - Remove duplicate JSON round-trip tests (consolidated into JSON Deserialization section)

Copilot

Pull request overview

Copilot reviewed 12 out of 12 changed files in this pull request and generated 4 comments.

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

pkg/mcp/config.go

pkg/mcp/client_internal_test.go

pkg/mcp/tool_adapter_test.go

pkg/mcp/client_internal_test.go

- Change MaxRetries from int to *int so explicit 0 (disable retries) is distinguishable from unset (nil, defaults to 2). Restore omitempty. - Strip client_internal_test.go to only convertAndFilterTools tests; duplicate shouldIncludeTool/buildStdioEnv/expandEnvValue/GetTools/Close tests removed (already covered in tool_adapter_test.go via test helpers). - Remove unreachable $NAME expansion test (buildStdioEnv only triggers expandEnvValue for ${} patterns). - Remove flaky NewClient subprocess test (rewrites to direct SetDefaults+Validate are already in tool_adapter_test.go).

Copilot

Pull request overview

Copilot reviewed 12 out of 12 changed files in this pull request and generated 3 comments.

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

pkg/mcp/tool_adapter.go

pkg/mcp/tool_adapter_test.go

CHANGELOG.md

Copilot

Pull request overview

Copilot reviewed 13 out of 13 changed files in this pull request and generated 4 comments.

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

Copilot · 2026-03-02T04:11:21Z

pkg/mcp/config.go

 	// SessionReconnect enables automatic session reconnection with max retry attempts
 	// Set to 0 to disable session reconnection
-	SessionReconnect int `yaml:"session_reconnect,omitempty" toml:"session_reconnect,omitempty,omitzero"`
-
-	// Retry configuration for MCP operations
-	Retry *RetryConfig `yaml:"retry,omitempty" toml:"retry,omitempty"`
-}
-
-// RetryConfig represents retry configuration for MCP operations.
-// This matches the retry configuration pattern from trpc-agent-go.
-type RetryConfig struct {
-	// MaxRetries is the maximum number of retry attempts (range: 0-10, default: 2)
-	MaxRetries int `yaml:"max_retries,omitempty" toml:"max_retries,omitempty,omitzero"`
-
-	// InitialBackoff is the initial delay before first retry (range: 1ms-30s, default: 500ms)
-	InitialBackoff time.Duration `yaml:"initial_backoff,omitempty" toml:"initial_backoff,omitempty"`
-
-	// BackoffFactor is the exponential backoff multiplier (range: 1.0-10.0, default: 2.0)
-	BackoffFactor float64 `yaml:"backoff_factor,omitempty" toml:"backoff_factor,omitempty,omitzero"`
-
-	// MaxBackoff is the maximum delay cap (range: up to 5 minutes, default: 8s)
-	MaxBackoff time.Duration `yaml:"max_backoff,omitempty" toml:"max_backoff,omitempty"`
+	SessionReconnect int `json:"session_reconnect,omitempty" yaml:"session_reconnect,omitempty" toml:"session_reconnect,omitempty,omitzero"`
 }


MCPServerConfig no longer includes a Retry field / RetryConfig type, but this PR (title/description) and tests (config_test.go) still reference JSON like { "retry": {} }, and NewClient’s comment mentions it. If retry is still intended to be configurable, please reintroduce the Retry field/type with json tags and keep SetDefaults()/Validate() aligned; otherwise, remove/update the remaining retry references and adjust the PR description to reflect the schema change (this would be a breaking config change).

Copilot · 2026-03-02T04:11:21Z

pkg/mcp/client.go

+	// Apply defaults before validation so that empty sub-configs
+	// (e.g. {"retry": {}}) receive sensible values.


The comment here says defaults are applied so empty {"retry": {}} sub-configs validate, but MCPServerConfig.SetDefaults() no longer deals with retry (and Retry isn’t part of the struct). Please update the comment to reflect the actual defaults being applied (e.g., timeout), or reintroduce retry defaults if retry is meant to remain part of MCP config.

Suggested change

// Apply defaults before validation so that empty sub-configs

// (e.g. {"retry": {}}) receive sensible values.

// Apply server defaults (such as timeouts and other optional fields)

// before validation so that configs with omitted optional sub-fields

// still receive sensible values.

Copilot · 2026-03-02T04:11:21Z

pkg/mcp/config_test.go

+		It("should correctly map all fields from JSONB/PostgreSQL format", func() {
+			jsonData := `{
+				"servers": [
+					{
+						"name": "github-mcp",
+						"transport": "sse",
+						"server_url": "http://localhost:8080/sse",
+						"timeout": 30000000000,
+						"include_tools": ["github_list_prs", "github_create_issue"],
+						"exclude_tools": ["github_delete_repo"],
+						"session_reconnect": 3,
+						"retry": {
+							"max_retries": 5,
+							"initial_backoff": 1000000000,
+							"backoff_factor": 3.0,
+							"max_backoff": 16000000000
+						}


This JSON deserialization test includes a retry object (and the test name says it maps “all fields”), but MCPServerConfig in this PR no longer has a Retry field, so json.Unmarshal will silently discard retry and the test won’t catch it. Either (a) assert that retry config is actually parsed (if it’s meant to be supported), or (b) remove retry from the fixture and rename the test to avoid implying coverage it doesn’t provide.

Copilot · 2026-03-02T04:11:21Z

docs/data/docs.yml

@@ -124,14 +124,6 @@ config_sections:
          - ["include_tools", "string[]", "Allowlist — only use these tools"]
          - ["exclude_tools", "string[]", "Blocklist — block these tools"]
          - ["session_reconnect", "int", "Auto-reconnect retries (`0` = off)"]


The MCP “Server Configuration” docs no longer mention the timeout field or its default (and this PR changes the default to 60s in code/README). Please add a timeout row (and, if retry config is still intended to exist, document retry.* here as well) so the docs stay in sync with the actual config schema.

Suggested change

- ["session_reconnect", "int", "Auto-reconnect retries (`0` = off)"]

- ["session_reconnect", "int", "Auto-reconnect retries (`0` = off)"]

- ["timeout", "int", "Request timeout in seconds (default: `60`)"]

- ["retry.*", "map", "Advanced retry configuration fields under `retry` (e.g. limits, backoff). See config schema for supported keys."]

sks added 7 commits February 28, 2026 23:56

get justification for secret lookup

a3ebda6

update changelog

2bc28e4

config builder

20849c9

Docs

b6c3f31

self review

c89d2c6

Copilot AI review requested due to automatic review settings March 1, 2026 23:52

Copilot started reviewing on behalf of sks March 1, 2026 23:53 View session

Copilot AI reviewed Mar 1, 2026

View reviewed changes

pkg/mcp/client.go Show resolved Hide resolved

pkg/mcp/config.go Show resolved Hide resolved

pkg/mcp/config.go Outdated Show resolved Hide resolved

pkg/mcp/config.go Outdated Show resolved Hide resolved

sks added 3 commits March 1, 2026 16:35

refactor(mcp): use counterfeiter FakeSecretProvider instead of custom…

255e8eb

… fake

Copilot AI review requested due to automatic review settings March 2, 2026 00:47

Copilot started reviewing on behalf of sks March 2, 2026 00:47 View session

sks added 2 commits March 1, 2026 16:47

merge: bring PR #4 review fixes and MCP unit tests from feature/reasons

abd600c

sks force-pushed the feature/mcp branch from d3b46ad to 3aa53bb Compare March 2, 2026 00:51

Copilot AI reviewed Mar 2, 2026

View reviewed changes

pkg/mcp/client_internal_test.go Show resolved Hide resolved

pkg/mcp/client.go Show resolved Hide resolved

pkg/mcp/config.go Outdated Show resolved Hide resolved

pkg/mcp/config.go Show resolved Hide resolved

pkg/mcp/config_test.go Outdated Show resolved Hide resolved

sks added 2 commits March 1, 2026 16:53

fix codecov badge

6917d2b

Copilot AI review requested due to automatic review settings March 2, 2026 01:04

Copilot started reviewing on behalf of sks March 2, 2026 01:04 View session

Copilot AI reviewed Mar 2, 2026

View reviewed changes

pkg/mcp/config.go Outdated Show resolved Hide resolved

pkg/mcp/client_internal_test.go Outdated Show resolved Hide resolved

pkg/mcp/tool_adapter_test.go Show resolved Hide resolved

pkg/mcp/client_internal_test.go Outdated Show resolved Hide resolved

sks requested a review from Copilot March 2, 2026 01:31

Copilot started reviewing on behalf of sks March 2, 2026 01:31 View session

Copilot AI reviewed Mar 2, 2026

View reviewed changes

pkg/mcp/tool_adapter.go Outdated Show resolved Hide resolved

pkg/mcp/tool_adapter_test.go Show resolved Hide resolved

CHANGELOG.md Show resolved Hide resolved

remove unwnated retry

3d6c079

sks requested a review from a team as a code owner March 2, 2026 04:02

Revert the instruction

6079c19

Copilot AI review requested due to automatic review settings March 2, 2026 04:06

sks merged commit 89f86e7 into main Mar 2, 2026
6 checks passed

sks deleted the feature/mcp branch March 2, 2026 04:07

Copilot started reviewing on behalf of sks March 2, 2026 04:07 View session

Copilot AI reviewed Mar 2, 2026

View reviewed changes

		// Apply defaults before validation so that empty sub-configs
		// (e.g. {"retry": {}}) receive sensible values.

-	// Apply defaults before validation so that empty sub-configs
-	// (e.g. {"retry": {}}) receive sensible values.
+	// Apply server defaults (such as timeouts and other optional fields)
+	// before validation so that configs with omitted optional sub-fields
+	// still receive sensible values.

Conversation

sks commented Mar 1, 2026

Problem

Changes

Testing

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

codecov bot commented Mar 2, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI Mar 2, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 2, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 2, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 2, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

codecov bot commented Mar 2, 2026 •

edited

Loading