diff --git a/docs/adr/28897-extract-parsemcpscripttoolconfig-helper.md b/docs/adr/28897-extract-parsemcpscripttoolconfig-helper.md new file mode 100644 index 0000000000..92ecd1dbcd --- /dev/null +++ b/docs/adr/28897-extract-parsemcpscripttoolconfig-helper.md @@ -0,0 +1,68 @@ +# ADR-28897: Extract `parseMCPScriptToolConfig` Helper to Eliminate Parsing Duplication + +**Date**: 2026-04-28 +**Status**: Draft +**Deciders**: pelikhan, Copilot + +--- + +## Part 1 — Narrative (Human-Friendly) + +### Context + +The MCP scripts parser (`pkg/workflow/mcp_scripts_parser.go`) contained two separate ~100-line blocks that each parsed a single `MCPScriptToolConfig` from a `map[string]any`: one inside `parseMCPScriptsMap` and one inside `mergeMCPScripts`. The blocks were structurally identical, covering description, inputs/params, script/run/py/go, env, and timeout parsing. When a `uint64` integer overflow bug was discovered in the timeout handling, it had to be patched at both sites independently (alerts #413 and #414), demonstrating that the duplication was an active maintenance hazard. Any future addition of an `MCPScriptToolConfig` field would require two separate edits with no compiler-enforced guarantee of consistency. + +### Decision + +We will extract a single private helper function `parseMCPScriptToolConfig(toolName string, toolMap map[string]any) *MCPScriptToolConfig` that contains the entire parsing logic for one MCP script tool config entry. Both `parseMCPScriptsMap` and `mergeMCPScripts` will call this helper instead of duplicating the logic inline. This makes `MCPScriptToolConfig` parsing a single, auditable code path that is modified in one place when the struct evolves. + +### Alternatives Considered + +#### Alternative 1: Accept the Duplication + +Keep both inline parsing blocks and rely on code-review discipline to keep them in sync. This was the status quo prior to this PR. It was rejected because the `uint64` overflow fix showed that even careful reviewers missed the need to update both sites — the duplication caused a real defect that required two separate security-alert fixes. + +#### Alternative 2: Parsing via a Configuration-Driven Strategy or Interface + +Introduce an interface (e.g., `MCPToolParser`) or a table-driven approach where parsing rules are declared as data rather than code. This would be more extensible if the tool config format were highly variable or plugin-driven. It was not chosen because the config format is well-defined and stable; the added complexity of an interface brings no benefit over a simple concrete helper function for this use case. + +### Consequences + +#### Positive +- A single canonical code path for parsing `MCPScriptToolConfig` eliminates the risk of the two sites drifting apart again. +- Future additions to `MCPScriptToolConfig` (new fields, validation logic) require only one edit, and the compiler enforces that both callers receive the updated behaviour automatically. + +#### Negative +- `parseMCPScriptToolConfig` is a package-private function; it is not exported and therefore cannot be easily tested in isolation by external test packages. Tests must exercise it through the higher-level callers. +- The extraction adds one level of indirection when reading either call site — a reader must follow the function call to understand full parsing behaviour. + +#### Neutral +- Net code change is −97 lines (205 deletions, 108 additions), reducing the size of the parser file significantly. +- The timeout `uint64` overflow safe-conversion comment is now consolidated into a single location, referencing both original alerts (#413 and #414). + +--- + +## Part 2 — Normative Specification (RFC 2119) + +> The key words **MUST**, **MUST NOT**, **REQUIRED**, **SHALL**, **SHALL NOT**, **SHOULD**, **SHOULD NOT**, **RECOMMENDED**, **MAY**, and **OPTIONAL** in this section are to be interpreted as described in [RFC 2119](https://www.rfc-editor.org/rfc/rfc2119). + +### MCP Script Tool Config Parsing + +1. All parsing of a single `MCPScriptToolConfig` from a raw `map[string]any` **MUST** be performed by the `parseMCPScriptToolConfig` helper function. +2. Implementations **MUST NOT** duplicate the per-tool parsing logic inline within `parseMCPScriptsMap`, `mergeMCPScripts`, or any future caller. +3. The `parseMCPScriptToolConfig` function **MUST** apply all field parsing (description, inputs, script, run, py, go, env, timeout) and **MUST** return a fully initialised `*MCPScriptToolConfig` with all map fields initialised to non-nil defaults. +4. Integer overflow-safe conversion of `uint64` timeout values **MUST** use `typeutil.SafeUint64ToInt` within `parseMCPScriptToolConfig` as the single authoritative conversion site. + +### Callers + +1. `parseMCPScriptsMap` **MUST** delegate per-tool config construction to `parseMCPScriptToolConfig` rather than constructing `MCPScriptToolConfig` values inline. +2. `mergeMCPScripts` **MUST** delegate per-tool config construction to `parseMCPScriptToolConfig` rather than constructing `MCPScriptToolConfig` values inline. +3. Any new function that needs to produce an `MCPScriptToolConfig` from a raw map **SHOULD** call `parseMCPScriptToolConfig` rather than re-implementing the parsing logic. + +### Conformance + +An implementation is considered conformant with this ADR if it satisfies all **MUST** and **MUST NOT** requirements above. Failure to meet any **MUST** or **MUST NOT** requirement constitutes non-conformance. + +--- + +*This is a DRAFT ADR generated by the [Design Decision Gate](https://github.com/github/gh-aw/actions/runs/25047928528) workflow. The PR author must review, complete, and finalize this document before the PR can merge.* diff --git a/pkg/workflow/mcp_scripts_parser.go b/pkg/workflow/mcp_scripts_parser.go index 8df2e8af13..4da5ec11a6 100644 --- a/pkg/workflow/mcp_scripts_parser.go +++ b/pkg/workflow/mcp_scripts_parser.go @@ -78,138 +78,149 @@ func IsMCPScriptsEnabled(mcpScripts *MCPScriptsConfig) bool { return HasMCPScripts(mcpScripts) } -// parseMCPScriptsMap parses mcp-scripts configuration from a map. -// This is the shared implementation used by both ParseMCPScripts and extractMCPScriptsConfig. -// Returns the config and a boolean indicating whether any tools were found. -func parseMCPScriptsMap(mcpScriptsMap map[string]any) (*MCPScriptsConfig, bool) { - config := &MCPScriptsConfig{ - Mode: "http", // Only HTTP mode is supported - Tools: make(map[string]*MCPScriptToolConfig), +// parseMCPScriptToolConfig parses a single MCP script tool configuration from a map. +// It initialises all fields to their defaults and populates them from toolMap. +func parseMCPScriptToolConfig(toolName string, toolMap map[string]any) *MCPScriptToolConfig { + toolConfig := &MCPScriptToolConfig{ + Name: toolName, + Inputs: make(map[string]*MCPScriptParam), + Env: make(map[string]string), + Timeout: 60, // Default timeout: 60 seconds } - // Mode field is ignored - only HTTP mode is supported - // All mcp-scripts configurations use HTTP transport - - for toolName, toolValue := range mcpScriptsMap { - // Skip the "mode" field as it's not a tool definition - if toolName == "mode" { - continue - } - - toolMap, ok := toolValue.(map[string]any) - if !ok { - continue - } - - toolConfig := &MCPScriptToolConfig{ - Name: toolName, - Inputs: make(map[string]*MCPScriptParam), - Env: make(map[string]string), - Timeout: 60, // Default timeout: 60 seconds + // Parse description (required) + if desc, exists := toolMap["description"]; exists { + if descStr, ok := desc.(string); ok { + toolConfig.Description = descStr } + } - // Parse description (required) - if desc, exists := toolMap["description"]; exists { - if descStr, ok := desc.(string); ok { - toolConfig.Description = descStr - } - } - - // Parse inputs (optional) - if inputs, exists := toolMap["inputs"]; exists { - if inputsMap, ok := inputs.(map[string]any); ok { - for paramName, paramValue := range inputsMap { - if paramMap, ok := paramValue.(map[string]any); ok { - param := &MCPScriptParam{ - Type: "string", // default type - } - - if t, exists := paramMap["type"]; exists { - if tStr, ok := t.(string); ok { - param.Type = tStr - } - } + // Parse inputs (optional) + if inputs, exists := toolMap["inputs"]; exists { + if inputsMap, ok := inputs.(map[string]any); ok { + for paramName, paramValue := range inputsMap { + if paramMap, ok := paramValue.(map[string]any); ok { + param := &MCPScriptParam{ + Type: "string", // default type + } - if desc, exists := paramMap["description"]; exists { - if descStr, ok := desc.(string); ok { - param.Description = descStr - } + if t, exists := paramMap["type"]; exists { + if tStr, ok := t.(string); ok { + param.Type = tStr } + } - if req, exists := paramMap["required"]; exists { - if reqBool, ok := req.(bool); ok { - param.Required = reqBool - } + if desc, exists := paramMap["description"]; exists { + if descStr, ok := desc.(string); ok { + param.Description = descStr } + } - if def, exists := paramMap["default"]; exists { - param.Default = def + if req, exists := paramMap["required"]; exists { + if reqBool, ok := req.(bool); ok { + param.Required = reqBool } + } - toolConfig.Inputs[paramName] = param + if def, exists := paramMap["default"]; exists { + param.Default = def } + + toolConfig.Inputs[paramName] = param } } } + } - // Parse script (JavaScript implementation) - if script, exists := toolMap["script"]; exists { - if scriptStr, ok := script.(string); ok { - toolConfig.Script = scriptStr - } + // Parse script (JavaScript implementation) + if script, exists := toolMap["script"]; exists { + if scriptStr, ok := script.(string); ok { + toolConfig.Script = scriptStr } + } - // Parse run (shell script implementation) - if run, exists := toolMap["run"]; exists { - if runStr, ok := run.(string); ok { - toolConfig.Run = runStr - } + // Parse run (shell script implementation) + if run, exists := toolMap["run"]; exists { + if runStr, ok := run.(string); ok { + toolConfig.Run = runStr } + } - // Parse py (Python script implementation) - if py, exists := toolMap["py"]; exists { - if pyStr, ok := py.(string); ok { - toolConfig.Py = pyStr - } + // Parse py (Python script implementation) + if py, exists := toolMap["py"]; exists { + if pyStr, ok := py.(string); ok { + toolConfig.Py = pyStr } + } - // Parse go (Go script implementation) - if goScript, exists := toolMap["go"]; exists { - if goStr, ok := goScript.(string); ok { - toolConfig.Go = goStr - } + // Parse go (Go script implementation) + if goScript, exists := toolMap["go"]; exists { + if goStr, ok := goScript.(string); ok { + toolConfig.Go = goStr } + } - // Parse env (environment variables) - if env, exists := toolMap["env"]; exists { - if envMap, ok := env.(map[string]any); ok { - for envName, envValue := range envMap { - if envStr, ok := envValue.(string); ok { - toolConfig.Env[envName] = envStr - } + // Parse env (environment variables) + if env, exists := toolMap["env"]; exists { + if envMap, ok := env.(map[string]any); ok { + for envName, envValue := range envMap { + if envStr, ok := envValue.(string); ok { + toolConfig.Env[envName] = envStr } } } + } - // Parse timeout (optional, default is 60 seconds) - if timeout, exists := toolMap["timeout"]; exists { - switch t := timeout.(type) { - case int: - toolConfig.Timeout = t - case uint64: - toolConfig.Timeout = typeutil.SafeUint64ToInt(t) // Safe conversion to prevent overflow (alert #414) - case float64: + // Parse timeout (optional, default is 60 seconds) + if timeout, exists := toolMap["timeout"]; exists { + switch t := timeout.(type) { + case int: + toolConfig.Timeout = t + case uint64: + toolConfig.Timeout = typeutil.SafeUint64ToInt(t) // Safe conversion to prevent overflow (alert #413, #414) + case float64: + maxInt := int(^uint(0) >> 1) + if t != t || t < 0 || t > float64(maxInt) { + mcpScriptsLog.Printf("Warning: invalid timeout value %v for tool %q, keeping default timeout (60s)", t, toolName) + } else { toolConfig.Timeout = int(t) - case string: - if n, ok := parseTimeoutString(t); ok { - toolConfig.Timeout = n - } else { - mcpScriptsLog.Printf("Warning: invalid timeout value %q for tool %q, keeping default timeout (60s)", t, toolName) - } + } + case string: + if n, ok := parseTimeoutString(t); ok { + toolConfig.Timeout = n + } else { + mcpScriptsLog.Printf("Warning: invalid timeout value %q for tool %q, keeping default timeout (60s)", t, toolName) } } + } + + return toolConfig +} - config.Tools[toolName] = toolConfig +// parseMCPScriptsMap parses mcp-scripts configuration from a map. +// It is used by extractMCPScriptsConfig to convert frontmatter into an MCPScriptsConfig. +// Returns the config and a boolean indicating whether any tools were found. +func parseMCPScriptsMap(mcpScriptsMap map[string]any) (*MCPScriptsConfig, bool) { + config := &MCPScriptsConfig{ + Mode: "http", // Only HTTP mode is supported + Tools: make(map[string]*MCPScriptToolConfig), + } + + // Mode field is ignored - only HTTP mode is supported + // All mcp-scripts configurations use HTTP transport + + for toolName, toolValue := range mcpScriptsMap { + // Skip the "mode" field as it's not a tool definition + if toolName == "mode" { + continue + } + + toolMap, ok := toolValue.(map[string]any) + if !ok { + continue + } + + config.Tools[toolName] = parseMCPScriptToolConfig(toolName, toolMap) } return config, len(config.Tools) > 0 @@ -280,110 +291,7 @@ func (c *Compiler) mergeMCPScripts(main *MCPScriptsConfig, importedConfigs []str continue } - toolConfig := &MCPScriptToolConfig{ - Name: toolName, - Inputs: make(map[string]*MCPScriptParam), - Env: make(map[string]string), - Timeout: 60, // Default timeout: 60 seconds - } - - // Parse description - if desc, exists := toolMap["description"]; exists { - if descStr, ok := desc.(string); ok { - toolConfig.Description = descStr - } - } - - // Parse inputs - if inputs, exists := toolMap["inputs"]; exists { - if inputsMap, ok := inputs.(map[string]any); ok { - for paramName, paramValue := range inputsMap { - if paramMap, ok := paramValue.(map[string]any); ok { - param := &MCPScriptParam{ - Type: "string", - } - if t, exists := paramMap["type"]; exists { - if tStr, ok := t.(string); ok { - param.Type = tStr - } - } - if desc, exists := paramMap["description"]; exists { - if descStr, ok := desc.(string); ok { - param.Description = descStr - } - } - if req, exists := paramMap["required"]; exists { - if reqBool, ok := req.(bool); ok { - param.Required = reqBool - } - } - if def, exists := paramMap["default"]; exists { - param.Default = def - } - toolConfig.Inputs[paramName] = param - } - } - } - } - - // Parse script - if script, exists := toolMap["script"]; exists { - if scriptStr, ok := script.(string); ok { - toolConfig.Script = scriptStr - } - } - - // Parse run - if run, exists := toolMap["run"]; exists { - if runStr, ok := run.(string); ok { - toolConfig.Run = runStr - } - } - - // Parse py - if py, exists := toolMap["py"]; exists { - if pyStr, ok := py.(string); ok { - toolConfig.Py = pyStr - } - } - - // Parse go - if goScript, exists := toolMap["go"]; exists { - if goStr, ok := goScript.(string); ok { - toolConfig.Go = goStr - } - } - - // Parse env - if env, exists := toolMap["env"]; exists { - if envMap, ok := env.(map[string]any); ok { - for envName, envValue := range envMap { - if envStr, ok := envValue.(string); ok { - toolConfig.Env[envName] = envStr - } - } - } - } - - // Parse timeout (optional, default is 60 seconds) - if timeout, exists := toolMap["timeout"]; exists { - switch t := timeout.(type) { - case int: - toolConfig.Timeout = t - case uint64: - toolConfig.Timeout = typeutil.SafeUint64ToInt(t) // Safe conversion to prevent overflow (alert #413) - case float64: - toolConfig.Timeout = int(t) - case string: - if n, ok := parseTimeoutString(t); ok { - toolConfig.Timeout = n - } else { - mcpScriptsLog.Printf("Warning: invalid timeout value %q for tool %q, keeping default timeout (60s)", t, toolName) - } - } - } - - main.Tools[toolName] = toolConfig + main.Tools[toolName] = parseMCPScriptToolConfig(toolName, toolMap) mcpScriptsLog.Printf("Merged imported mcp-script tool: %s", toolName) } }