From 69a6bd7daba300aa3aabc923417d937856b543e2 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Sat, 11 Apr 2026 11:54:08 +0000 Subject: [PATCH 1/2] Initial plan From cd875c5dfa3014f89556f0e446751f736261590b Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Sat, 11 Apr 2026 12:17:20 +0000 Subject: [PATCH 2/2] refactor: extract shared buildInputSchema helper to deduplicate input-schema generation Extract the repeated input-schema construction logic from three generator functions into a shared buildInputSchema helper in build_input_schema.go. The helper consolidates the JSON Schema property/required generation from workflow/tool input definitions, including type mapping (string, number, boolean, choice, environment), choice enum/default handling, and required field tracking. Replaces duplicated loops in: - generateCallWorkflowTool (safe_outputs_call_workflow.go) - generateDispatchWorkflowTool (safe_outputs_dispatch.go) - generateDispatchRepositoryTool (dispatch_repository.go) Adds comprehensive tests for the shared helper covering all type mappings, default propagation, required tracking, invalid input skipping, and edge cases. Fixes #685 Agent-Logs-Url: https://github.com/github/gh-aw/sessions/59ee6590-52dd-4f72-9a51-b2e0ef119be2 Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com> --- pkg/workflow/build_input_schema.go | 79 +++++++ pkg/workflow/build_input_schema_test.go | 263 +++++++++++++++++++++ pkg/workflow/dispatch_repository.go | 65 +---- pkg/workflow/safe_outputs_call_workflow.go | 68 +----- pkg/workflow/safe_outputs_dispatch.go | 69 +----- 5 files changed, 353 insertions(+), 191 deletions(-) create mode 100644 pkg/workflow/build_input_schema.go create mode 100644 pkg/workflow/build_input_schema_test.go diff --git a/pkg/workflow/build_input_schema.go b/pkg/workflow/build_input_schema.go new file mode 100644 index 00000000000..5b816dff463 --- /dev/null +++ b/pkg/workflow/build_input_schema.go @@ -0,0 +1,79 @@ +package workflow + +// buildInputSchema converts GitHub Actions input definitions (workflow_dispatch, +// workflow_call, or dispatch_repository inputs) into JSON Schema properties and +// a required field list suitable for MCP tool inputSchema. +// +// descriptionFn is called to produce the fallback description when an input +// definition does not include its own "description" field. +// +// Supported input types: string (default), number, boolean, choice, environment. +// Choice inputs with options are mapped to a string enum. Unknown types default +// to string. +func buildInputSchema(inputs map[string]any, descriptionFn func(inputName string) string) (properties map[string]any, required []string) { + properties = make(map[string]any) + required = []string{} + + for inputName, inputDef := range inputs { + inputDefMap, ok := inputDef.(map[string]any) + if !ok { + continue + } + + inputType := "string" + inputDescription := descriptionFn(inputName) + inputRequired := false + + if desc, ok := inputDefMap["description"].(string); ok && desc != "" { + inputDescription = desc + } + + if req, ok := inputDefMap["required"].(bool); ok { + inputRequired = req + } + + // Map GitHub Actions input types to JSON Schema types. + if typeStr, ok := inputDefMap["type"].(string); ok { + switch typeStr { + case "number": + inputType = "number" + case "boolean": + inputType = "boolean" + case "choice": + inputType = "string" + if options, ok := inputDefMap["options"].([]any); ok && len(options) > 0 { + prop := map[string]any{ + "type": inputType, + "description": inputDescription, + "enum": options, + } + if defaultVal, ok := inputDefMap["default"]; ok { + prop["default"] = defaultVal + } + properties[inputName] = prop + if inputRequired { + required = append(required, inputName) + } + continue + } + case "environment": + inputType = "string" + } + } + + prop := map[string]any{ + "type": inputType, + "description": inputDescription, + } + if defaultVal, ok := inputDefMap["default"]; ok { + prop["default"] = defaultVal + } + properties[inputName] = prop + + if inputRequired { + required = append(required, inputName) + } + } + + return properties, required +} diff --git a/pkg/workflow/build_input_schema_test.go b/pkg/workflow/build_input_schema_test.go new file mode 100644 index 00000000000..d45cc38c11d --- /dev/null +++ b/pkg/workflow/build_input_schema_test.go @@ -0,0 +1,263 @@ +//go:build !integration + +package workflow + +import ( + "fmt" + "sort" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// defaultDescFn is a simple description function used in tests. +func defaultDescFn(inputName string) string { + return fmt.Sprintf("Input parameter '%s'", inputName) +} + +// TestBuildInputSchemaStringType tests that the default type is string. +func TestBuildInputSchemaStringType(t *testing.T) { + inputs := map[string]any{ + "message": map[string]any{ + "type": "string", + "description": "A message", + }, + } + + properties, required := buildInputSchema(inputs, defaultDescFn) + + prop, ok := properties["message"].(map[string]any) + require.True(t, ok, "message property should exist") + assert.Equal(t, "string", prop["type"], "type should be string") + assert.Equal(t, "A message", prop["description"], "description should match") + assert.Empty(t, required, "required should be empty") +} + +// TestBuildInputSchemaNumberType tests number type mapping. +func TestBuildInputSchemaNumberType(t *testing.T) { + inputs := map[string]any{ + "count": map[string]any{ + "type": "number", + "description": "A count", + }, + } + + properties, _ := buildInputSchema(inputs, defaultDescFn) + + prop, ok := properties["count"].(map[string]any) + require.True(t, ok, "count property should exist") + assert.Equal(t, "number", prop["type"], "type should be number") +} + +// TestBuildInputSchemaBooleanType tests boolean type mapping. +func TestBuildInputSchemaBooleanType(t *testing.T) { + inputs := map[string]any{ + "dry_run": map[string]any{ + "type": "boolean", + }, + } + + properties, _ := buildInputSchema(inputs, defaultDescFn) + + prop, ok := properties["dry_run"].(map[string]any) + require.True(t, ok, "dry_run property should exist") + assert.Equal(t, "boolean", prop["type"], "type should be boolean") +} + +// TestBuildInputSchemaChoiceType tests choice type with enum options. +func TestBuildInputSchemaChoiceType(t *testing.T) { + inputs := map[string]any{ + "environment": map[string]any{ + "type": "choice", + "description": "Target environment", + "options": []any{"staging", "production"}, + }, + } + + properties, _ := buildInputSchema(inputs, defaultDescFn) + + prop, ok := properties["environment"].(map[string]any) + require.True(t, ok, "environment property should exist") + assert.Equal(t, "string", prop["type"], "choice maps to string") + assert.Equal(t, []any{"staging", "production"}, prop["enum"], "enum values should match") +} + +// TestBuildInputSchemaChoiceWithDefault tests that choice type preserves default. +func TestBuildInputSchemaChoiceWithDefault(t *testing.T) { + inputs := map[string]any{ + "environment": map[string]any{ + "type": "choice", + "description": "Target environment", + "options": []any{"staging", "production"}, + "default": "staging", + }, + } + + properties, _ := buildInputSchema(inputs, defaultDescFn) + + prop, ok := properties["environment"].(map[string]any) + require.True(t, ok, "environment property should exist") + assert.Equal(t, "string", prop["type"], "choice maps to string") + assert.Equal(t, []any{"staging", "production"}, prop["enum"], "enum values should match") + assert.Equal(t, "staging", prop["default"], "default should be preserved for choice") +} + +// TestBuildInputSchemaEnvironmentType tests environment type mapping. +func TestBuildInputSchemaEnvironmentType(t *testing.T) { + inputs := map[string]any{ + "deploy_env": map[string]any{ + "type": "environment", + "description": "Deployment environment", + }, + } + + properties, _ := buildInputSchema(inputs, defaultDescFn) + + prop, ok := properties["deploy_env"].(map[string]any) + require.True(t, ok, "deploy_env property should exist") + assert.Equal(t, "string", prop["type"], "environment maps to string") +} + +// TestBuildInputSchemaDefaultPropagation tests that default values are propagated. +func TestBuildInputSchemaDefaultPropagation(t *testing.T) { + inputs := map[string]any{ + "version": map[string]any{ + "type": "string", + "description": "Version", + "default": "latest", + }, + } + + properties, _ := buildInputSchema(inputs, defaultDescFn) + + prop, ok := properties["version"].(map[string]any) + require.True(t, ok, "version property should exist") + assert.Equal(t, "latest", prop["default"], "default value should be set") +} + +// TestBuildInputSchemaRequiredInputs tests required field tracking. +func TestBuildInputSchemaRequiredInputs(t *testing.T) { + inputs := map[string]any{ + "name": map[string]any{ + "type": "string", + "required": true, + }, + "optional": map[string]any{ + "type": "string", + "required": false, + }, + } + + _, required := buildInputSchema(inputs, defaultDescFn) + + assert.Contains(t, required, "name", "name should be required") + assert.NotContains(t, required, "optional", "optional should not be required") +} + +// TestBuildInputSchemaRequiredSorting tests that required list order is deterministic +// across multiple runs (callers typically sort the result). +func TestBuildInputSchemaRequiredSorting(t *testing.T) { + inputs := map[string]any{ + "z_param": map[string]any{"type": "string", "required": true}, + "a_param": map[string]any{"type": "string", "required": true}, + "m_param": map[string]any{"type": "string", "required": true}, + } + + for i := range 10 { + _, required := buildInputSchema(inputs, defaultDescFn) + sort.Strings(required) + assert.Equal(t, []string{"a_param", "m_param", "z_param"}, required, + "required should be sortable to deterministic order (iteration %d)", i) + } +} + +// TestBuildInputSchemaSkipsInvalidDefs tests that non-map input definitions are skipped. +func TestBuildInputSchemaSkipsInvalidDefs(t *testing.T) { + inputs := map[string]any{ + "valid": map[string]any{ + "type": "string", + "description": "Valid input", + }, + "invalid_string": "not a map", + "invalid_number": 42, + "invalid_nil": nil, + } + + properties, required := buildInputSchema(inputs, defaultDescFn) + + assert.Len(t, properties, 1, "Only valid input should produce a property") + assert.Contains(t, properties, "valid", "valid input should be present") + assert.Empty(t, required, "no required inputs") +} + +// TestBuildInputSchemaEmptyInputs tests that empty inputs produce empty results. +func TestBuildInputSchemaEmptyInputs(t *testing.T) { + properties, required := buildInputSchema(make(map[string]any), defaultDescFn) + + assert.Empty(t, properties, "properties should be empty") + assert.Empty(t, required, "required should be empty") +} + +// TestBuildInputSchemaNilInputs tests that nil inputs produce empty results. +func TestBuildInputSchemaNilInputs(t *testing.T) { + properties, required := buildInputSchema(nil, defaultDescFn) + + assert.Empty(t, properties, "properties should be empty") + assert.Empty(t, required, "required should be empty") +} + +// TestBuildInputSchemaFallbackDescription tests the descriptionFn callback. +func TestBuildInputSchemaFallbackDescription(t *testing.T) { + inputs := map[string]any{ + "param": map[string]any{ + "type": "string", + // No description provided - should use fallback + }, + } + + descFn := func(inputName string) string { + return fmt.Sprintf("Custom fallback for '%s'", inputName) + } + + properties, _ := buildInputSchema(inputs, descFn) + + prop, ok := properties["param"].(map[string]any) + require.True(t, ok, "param property should exist") + assert.Equal(t, "Custom fallback for 'param'", prop["description"], "should use fallback description") +} + +// TestBuildInputSchemaChoiceWithoutOptions tests choice type without options falls back to string. +func TestBuildInputSchemaChoiceWithoutOptions(t *testing.T) { + inputs := map[string]any{ + "env": map[string]any{ + "type": "choice", + "description": "Environment", + // No options - should fall through to regular string property + }, + } + + properties, _ := buildInputSchema(inputs, defaultDescFn) + + prop, ok := properties["env"].(map[string]any) + require.True(t, ok, "env property should exist") + assert.Equal(t, "string", prop["type"], "choice without options maps to string") + _, hasEnum := prop["enum"] + assert.False(t, hasEnum, "should not have enum when no options") +} + +// TestBuildInputSchemaUnknownType tests that unknown type defaults to string. +func TestBuildInputSchemaUnknownType(t *testing.T) { + inputs := map[string]any{ + "param": map[string]any{ + "type": "unknown_type", + "description": "Some param", + }, + } + + properties, _ := buildInputSchema(inputs, defaultDescFn) + + prop, ok := properties["param"].(map[string]any) + require.True(t, ok, "param property should exist") + assert.Equal(t, "string", prop["type"], "unknown type should default to string") +} diff --git a/pkg/workflow/dispatch_repository.go b/pkg/workflow/dispatch_repository.go index 3663fd10582..648e48bf2f8 100644 --- a/pkg/workflow/dispatch_repository.go +++ b/pkg/workflow/dispatch_repository.go @@ -145,68 +145,9 @@ func generateDispatchRepositoryTool(toolKey string, toolConfig *DispatchReposito } // Build input schema from the tool's inputs definition - properties := make(map[string]any) - required := []string{} - - for inputName, inputDef := range toolConfig.Inputs { - inputDefMap, ok := inputDef.(map[string]any) - if !ok { - continue - } - - inputType := "string" - inputDescription := "Input parameter '" + inputName + "'" - inputRequired := false - - if desc, ok := inputDefMap["description"].(string); ok && desc != "" { - inputDescription = desc - } - if req, ok := inputDefMap["required"].(bool); ok { - inputRequired = req - } - - // Map input types to JSON Schema types - if typeStr, ok := inputDefMap["type"].(string); ok { - switch typeStr { - case "number": - inputType = "number" - case "boolean": - inputType = "boolean" - case "choice": - inputType = "string" - if options, ok := inputDefMap["options"].([]any); ok && len(options) > 0 { - prop := map[string]any{ - "type": inputType, - "description": inputDescription, - "enum": options, - } - if defaultVal, ok := inputDefMap["default"]; ok { - prop["default"] = defaultVal - } - properties[inputName] = prop - if inputRequired { - required = append(required, inputName) - } - continue - } - case "environment": - inputType = "string" - } - } - - prop := map[string]any{ - "type": inputType, - "description": inputDescription, - } - if defaultVal, ok := inputDefMap["default"]; ok { - prop["default"] = defaultVal - } - properties[inputName] = prop - - if inputRequired { - required = append(required, inputName) - } - } + properties, required := buildInputSchema(toolConfig.Inputs, func(inputName string) string { + return "Input parameter '" + inputName + "'" + }) tool := map[string]any{ "name": toolName, diff --git a/pkg/workflow/safe_outputs_call_workflow.go b/pkg/workflow/safe_outputs_call_workflow.go index 96a0968b565..c7ae4cdc68d 100644 --- a/pkg/workflow/safe_outputs_call_workflow.go +++ b/pkg/workflow/safe_outputs_call_workflow.go @@ -81,70 +81,10 @@ func generateCallWorkflowTool(workflowName string, workflowInputs map[string]any // Build the description description := fmt.Sprintf("Call the '%s' reusable workflow via workflow_call. This workflow must support workflow_call and be in .github/workflows/ directory in the same repository.", workflowName) - // Build input schema properties - properties := make(map[string]any) - required := []string{} - - // Convert GitHub Actions workflow_call inputs to MCP tool schema - for inputName, inputDef := range workflowInputs { - inputDefMap, ok := inputDef.(map[string]any) - if !ok { - continue - } - - // Extract input properties - inputType := "string" // Default type - inputDescription := fmt.Sprintf("Input parameter '%s' for workflow %s", inputName, workflowName) - inputRequired := false - - if desc, ok := inputDefMap["description"].(string); ok && desc != "" { - inputDescription = desc - } - - if req, ok := inputDefMap["required"].(bool); ok { - inputRequired = req - } - - // GitHub Actions workflow_call supports: string, number, boolean, choice, environment - if typeStr, ok := inputDefMap["type"].(string); ok { - switch typeStr { - case "number": - inputType = "number" - case "boolean": - inputType = "boolean" - case "choice": - inputType = "string" - if options, ok := inputDefMap["options"].([]any); ok && len(options) > 0 { - properties[inputName] = map[string]any{ - "type": inputType, - "description": inputDescription, - "enum": options, - } - if inputRequired { - required = append(required, inputName) - } - continue - } - case "environment": - inputType = "string" - } - } - - prop := map[string]any{ - "type": inputType, - "description": inputDescription, - } - - if defaultVal, ok := inputDefMap["default"]; ok { - prop["default"] = defaultVal - } - - properties[inputName] = prop - - if inputRequired { - required = append(required, inputName) - } - } + // Build input schema properties from workflow_call inputs + properties, required := buildInputSchema(workflowInputs, func(inputName string) string { + return fmt.Sprintf("Input parameter '%s' for workflow %s", inputName, workflowName) + }) // Build the complete tool definition tool := map[string]any{ diff --git a/pkg/workflow/safe_outputs_dispatch.go b/pkg/workflow/safe_outputs_dispatch.go index 1d19cee1522..c6cf007873a 100644 --- a/pkg/workflow/safe_outputs_dispatch.go +++ b/pkg/workflow/safe_outputs_dispatch.go @@ -114,71 +114,10 @@ func generateDispatchWorkflowTool(workflowName string, workflowInputs map[string // Build the description description := fmt.Sprintf("Dispatch the '%s' workflow with workflow_dispatch trigger. This workflow must support workflow_dispatch and be in .github/workflows/ directory in the same repository.", workflowName) - // Build input schema properties - properties := make(map[string]any) - required := []string{} // No required fields by default - - // Convert GitHub Actions workflow_dispatch inputs to MCP tool schema - for inputName, inputDef := range workflowInputs { - inputDefMap, ok := inputDef.(map[string]any) - if !ok { - continue - } - - // Extract input properties - inputType := "string" // Default type - inputDescription := fmt.Sprintf("Input parameter '%s' for workflow %s", inputName, workflowName) - inputRequired := false - - if desc, ok := inputDefMap["description"].(string); ok && desc != "" { - inputDescription = desc - } - - if req, ok := inputDefMap["required"].(bool); ok { - inputRequired = req - } - - // GitHub Actions workflow_dispatch supports: string, number, boolean, choice, environment - // Map these to JSON schema types - if typeStr, ok := inputDefMap["type"].(string); ok { - switch typeStr { - case "number": - inputType = "number" - case "boolean": - inputType = "boolean" - case "choice": - inputType = "string" - // Add enum if options are provided - if options, ok := inputDefMap["options"].([]any); ok && len(options) > 0 { - properties[inputName] = map[string]any{ - "type": inputType, - "description": inputDescription, - "enum": options, - } - if inputRequired { - required = append(required, inputName) - } - continue - } - case "environment": - inputType = "string" - } - } - - properties[inputName] = map[string]any{ - "type": inputType, - "description": inputDescription, - } - - // Add default value if provided - if defaultVal, ok := inputDefMap["default"]; ok { - properties[inputName].(map[string]any)["default"] = defaultVal - } - - if inputRequired { - required = append(required, inputName) - } - } + // Build input schema properties from workflow_dispatch inputs + properties, required := buildInputSchema(workflowInputs, func(inputName string) string { + return fmt.Sprintf("Input parameter '%s' for workflow %s", inputName, workflowName) + }) // Add internal workflow_name parameter (hidden from description but used internally) // This will be injected by the safe output handler