diff --git a/pkg/agentdrain/miner.go b/pkg/agentdrain/miner.go index 2059322a9d..3ae787456d 100644 --- a/pkg/agentdrain/miner.go +++ b/pkg/agentdrain/miner.go @@ -73,26 +73,6 @@ func (m *Miner) Train(line string) (*MatchResult, error) { }, nil } -// Match performs inference only: it finds the best matching cluster but does -// not mutate any state. Returns (result, true) when a match is found. -// It is safe to call from multiple goroutines. -func (m *Miner) Match(line string) (*MatchResult, bool, error) { - masked := m.masker.Mask(line) - tokens := Tokenize(masked) - if len(tokens) == 0 { - return nil, false, errors.New("agentdrain: Match: empty line after masking") - } - - m.mu.RLock() - defer m.mu.RUnlock() - - result, _ := m.match(tokens) - if result == nil { - return nil, false, nil - } - return result, true, nil -} - // match is the internal (non-locking) lookup. Must be called with mu held. func (m *Miner) match(tokens []string) (*MatchResult, bool) { candidates := m.tree.search(tokens, m.cfg.Depth, m.cfg.ParamToken) diff --git a/pkg/agentdrain/miner_test.go b/pkg/agentdrain/miner_test.go index 35c48addb8..1bd454f604 100644 --- a/pkg/agentdrain/miner_test.go +++ b/pkg/agentdrain/miner_test.go @@ -67,28 +67,6 @@ func TestTrain_ClusterMerge(t *testing.T) { } } -func TestMatch_InferenceOnly(t *testing.T) { - m, err := NewMiner(DefaultConfig()) - if err != nil { - t.Fatalf("NewMiner: %v", err) - } - // Train once. - _, err = m.Train("stage=plan action=start") - if err != nil { - t.Fatalf("Train: %v", err) - } - before := m.ClusterCount() - - // Match should not create new clusters. - _, _, err = m.Match("stage=plan action=unknown_value") - if err != nil { - t.Fatalf("Match: unexpected error: %v", err) - } - if m.ClusterCount() != before { - t.Errorf("Match: cluster count changed from %d to %d", before, m.ClusterCount()) - } -} - func TestMasking(t *testing.T) { masker, err := NewMasker(DefaultConfig().MaskRules) if err != nil { @@ -154,38 +132,6 @@ func TestFlattenEvent(t *testing.T) { } } -func TestSaveLoadJSON(t *testing.T) { - cfg := DefaultConfig() - m, err := NewMiner(cfg) - if err != nil { - t.Fatalf("NewMiner: %v", err) - } - lines := []string{ - "stage=plan action=start", - "stage=plan action=start", - "stage=tool_call tool=search", - } - for _, l := range lines { - if _, err := m.Train(l); err != nil { - t.Fatalf("Train(%q): %v", l, err) - } - } - originalCount := m.ClusterCount() - - data, err := m.SaveJSON() - if err != nil { - t.Fatalf("SaveJSON: %v", err) - } - - m2, err := LoadMinerJSON(data) - if err != nil { - t.Fatalf("LoadMinerJSON: %v", err) - } - if m2.ClusterCount() != originalCount { - t.Errorf("round-trip: expected %d clusters, got %d", originalCount, m2.ClusterCount()) - } -} - func TestConcurrency(t *testing.T) { m, err := NewMiner(DefaultConfig()) if err != nil { diff --git a/pkg/agentdrain/persist.go b/pkg/agentdrain/persist.go index 71fe16202b..6422def078 100644 --- a/pkg/agentdrain/persist.go +++ b/pkg/agentdrain/persist.go @@ -85,19 +85,3 @@ func (m *Miner) LoadJSON(data []byte) error { persistLog.Printf("Loaded miner state: clusters=%d", len(snap.Clusters)) return nil } - -// LoadMinerJSON creates a new Miner by restoring state from JSON bytes. -func LoadMinerJSON(data []byte) (*Miner, error) { - var snap Snapshot - if err := json.Unmarshal(data, &snap); err != nil { - return nil, fmt.Errorf("agentdrain: LoadMinerJSON: %w", err) - } - m, err := NewMiner(snap.Config) - if err != nil { - return nil, err - } - if err := m.LoadJSON(data); err != nil { - return nil, err - } - return m, nil -} diff --git a/pkg/workflow/compiler_draft_test.go b/pkg/workflow/compiler_draft_test.go index 435a4334b4..d93463809e 100644 --- a/pkg/workflow/compiler_draft_test.go +++ b/pkg/workflow/compiler_draft_test.go @@ -184,8 +184,8 @@ This is a test workflow for draft filtering. if tt.shouldHaveIf { // Check that the expected if condition is present (normalize for multiline comparison) - normalizedLockContent := NormalizeExpressionForComparison(lockContent) - normalizedExpectedIf := NormalizeExpressionForComparison(tt.expectedIf) + normalizedLockContent := strings.Join(strings.Fields(lockContent), " ") + normalizedExpectedIf := strings.Join(strings.Fields(tt.expectedIf), " ") if !strings.Contains(normalizedLockContent, normalizedExpectedIf) { t.Errorf("Expected lock file to contain '%s' but it didn't.\nExpected (normalized): %s\nActual (normalized): %s\nOriginal Content:\n%s", tt.expectedIf, normalizedExpectedIf, normalizedLockContent, lockContent) diff --git a/pkg/workflow/compiler_events_test.go b/pkg/workflow/compiler_events_test.go index ba95ca79bf..e34f71b99f 100644 --- a/pkg/workflow/compiler_events_test.go +++ b/pkg/workflow/compiler_events_test.go @@ -236,8 +236,8 @@ This is a test workflow for command triggering. } // Check that the expected if condition is present (normalize for multiline comparison) - normalizedLockContent := NormalizeExpressionForComparison(lockContent) - normalizedExpectedIf := NormalizeExpressionForComparison(tt.expectedIf) + normalizedLockContent := strings.Join(strings.Fields(lockContent), " ") + normalizedExpectedIf := strings.Join(strings.Fields(tt.expectedIf), " ") if !strings.Contains(normalizedLockContent, normalizedExpectedIf) { t.Errorf("Expected lock file to contain '%s' but it didn't.\nExpected (normalized): %s\nActual (normalized): %s\nOriginal Content:\n%s", tt.expectedIf, normalizedExpectedIf, normalizedLockContent, lockContent) @@ -464,8 +464,8 @@ This is a test workflow for command merging with other events. } // Check that the expected if condition is present (normalize for multiline comparison) - normalizedLockContent := NormalizeExpressionForComparison(lockContent) - normalizedExpectedIf := NormalizeExpressionForComparison(tt.expectedIf) + normalizedLockContent := strings.Join(strings.Fields(lockContent), " ") + normalizedExpectedIf := strings.Join(strings.Fields(tt.expectedIf), " ") if !strings.Contains(normalizedLockContent, normalizedExpectedIf) { t.Errorf("Expected lock file to contain '%s' but it didn't.\nExpected (normalized): %s\nActual (normalized): %s\nOriginal Content:\n%s", tt.expectedIf, normalizedExpectedIf, normalizedLockContent, lockContent) diff --git a/pkg/workflow/expression_parser.go b/pkg/workflow/expression_parser.go index d6fed9ca02..eeebc88e47 100644 --- a/pkg/workflow/expression_parser.go +++ b/pkg/workflow/expression_parser.go @@ -508,19 +508,3 @@ func escapeForYAMLDoubleQuoted(s string) string { } return b.String() } - -// NormalizeExpressionForComparison normalizes an expression by removing extra spaces and newlines -// This is used for comparing multiline expressions with their single-line equivalents -func NormalizeExpressionForComparison(expression string) string { - // Replace newlines and tabs with spaces - normalized := strings.ReplaceAll(expression, "\n", " ") - normalized = strings.ReplaceAll(normalized, "\t", " ") - - // Replace multiple spaces with single spaces - for strings.Contains(normalized, " ") { - normalized = strings.ReplaceAll(normalized, " ", " ") - } - - // Trim leading and trailing spaces - return strings.TrimSpace(normalized) -} diff --git a/pkg/workflow/expressions_test.go b/pkg/workflow/expressions_test.go index 1ccf7f314c..7f20fa216e 100644 --- a/pkg/workflow/expressions_test.go +++ b/pkg/workflow/expressions_test.go @@ -946,8 +946,8 @@ func TestBreakLongExpression(t *testing.T) { // Verify that joined lines equal normalized original (whitespace differences allowed) joined := strings.Join(lines, " ") - originalNorm := NormalizeExpressionForComparison(tt.expression) - joinedNorm := NormalizeExpressionForComparison(joined) + originalNorm := strings.Join(strings.Fields(tt.expression), " ") + joinedNorm := strings.Join(strings.Fields(joined), " ") if joinedNorm != originalNorm { t.Errorf("Joined lines don't match original expression\nOriginal: %s\nJoined: %s", originalNorm, joinedNorm) @@ -985,8 +985,8 @@ func TestBreakAtParentheses(t *testing.T) { // Verify that joined lines equal normalized original joined := strings.Join(lines, " ") - originalNorm := NormalizeExpressionForComparison(tt.expression) - joinedNorm := NormalizeExpressionForComparison(joined) + originalNorm := strings.Join(strings.Fields(tt.expression), " ") + joinedNorm := strings.Join(strings.Fields(joined), " ") if joinedNorm != originalNorm { t.Errorf("Joined lines don't match original expression\nOriginal: %s\nJoined: %s", originalNorm, joinedNorm) @@ -995,48 +995,6 @@ func TestBreakAtParentheses(t *testing.T) { } } -// TestNormalizeExpressionForComparison tests the expression normalization function -func TestNormalizeExpressionForComparison(t *testing.T) { - tests := []struct { - name string - input string - expected string - }{ - { - name: "single line with extra spaces", - input: "github.event_name == 'issues' || github.event.action == 'opened'", - expected: "github.event_name == 'issues' || github.event.action == 'opened'", - }, - { - name: "multiline expression", - input: `github.event_name == 'issues' || -github.event_name == 'pull_request' || -github.event.action == 'opened'`, - expected: "github.event_name == 'issues' || github.event_name == 'pull_request' || github.event.action == 'opened'", - }, - { - name: "expression with mixed whitespace", - input: `github.event_name == 'issues' || - github.event_name == 'pull_request'`, - expected: "github.event_name == 'issues' || github.event_name == 'pull_request'", - }, - { - name: "expression with leading/trailing whitespace", - input: " github.event_name == 'issues' ", - expected: "github.event_name == 'issues'", - }, - } - - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - result := NormalizeExpressionForComparison(tt.input) - if result != tt.expected { - t.Errorf("NormalizeExpressionForComparison() = '%s', expected '%s'", result, tt.expected) - } - }) - } -} - // TestMultilineExpressionEquivalence tests that multiline expressions are equivalent to single-line expressions func TestMultilineExpressionEquivalence(t *testing.T) { tests := []struct { @@ -1079,8 +1037,8 @@ github.event_name == 'issue_comment' || for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - singleNorm := NormalizeExpressionForComparison(tt.singleLine) - multiNorm := NormalizeExpressionForComparison(tt.multiLine) + singleNorm := strings.Join(strings.Fields(tt.singleLine), " ") + multiNorm := strings.Join(strings.Fields(tt.multiLine), " ") isEqual := singleNorm == multiNorm if isEqual != tt.shouldBeEqual { @@ -1134,8 +1092,8 @@ func TestLongExpressionBreakingDetailed(t *testing.T) { // Most importantly: verify equivalence joined := strings.Join(lines, " ") - originalNorm := NormalizeExpressionForComparison(tt.expression) - joinedNorm := NormalizeExpressionForComparison(joined) + originalNorm := strings.Join(strings.Fields(tt.expression), " ") + joinedNorm := strings.Join(strings.Fields(joined), " ") if joinedNorm != originalNorm { t.Errorf("Broken expression is not equivalent to original\nOriginal: %s\nBroken: %s\nJoined: %s\nOriginal normalized: %s\nJoined normalized: %s", @@ -1175,8 +1133,8 @@ func TestExpressionBreakingWithQuotes(t *testing.T) { // Verify that quotes are preserved and no breaking happens inside quoted strings joined := strings.Join(lines, " ") - originalNorm := NormalizeExpressionForComparison(tt.expression) - joinedNorm := NormalizeExpressionForComparison(joined) + originalNorm := strings.Join(strings.Fields(tt.expression), " ") + joinedNorm := strings.Join(strings.Fields(joined), " ") if joinedNorm != originalNorm { t.Errorf("Expression with quotes not preserved correctly\nOriginal: %s\nJoined: %s", originalNorm, joinedNorm) diff --git a/pkg/workflow/safe_outputs_custom_job_tools_test.go b/pkg/workflow/safe_outputs_custom_job_tools_test.go deleted file mode 100644 index bb969bc0ee..0000000000 --- a/pkg/workflow/safe_outputs_custom_job_tools_test.go +++ /dev/null @@ -1,284 +0,0 @@ -//go:build !integration - -package workflow - -import ( - "encoding/json" - "testing" - - "github.com/stretchr/testify/assert" - "github.com/stretchr/testify/require" -) - -// TestCustomJobToolsInToolsJSON verifies that custom safe-output jobs -// are properly included in the tools.json file with correct MCP tool schema -func TestCustomJobToolsInToolsJSON(t *testing.T) { - workflowData := &WorkflowData{ - SafeOutputs: &SafeOutputsConfig{ - Jobs: map[string]*SafeJobConfig{ - "test_environment": { - Description: "A test job with choice input", - Inputs: map[string]*InputDefinition{ - "environment": { - Description: "Target environment", - Required: true, - Type: "choice", - Options: []string{"staging", "production"}, - }, - "test_type": { - Description: "Type of test to run", - Required: true, - Type: "choice", - Options: []string{"smoke", "integration", "e2e"}, - }, - }, - Output: "Environment test completed successfully", - }, - }, - }, - } - - // Generate the tools JSON - toolsJSON, err := generateFilteredToolsJSON(workflowData, ".github/workflows/test-workflow.md") - require.NoError(t, err, "Should generate tools JSON") - - // Parse the JSON - var tools []map[string]any - err = json.Unmarshal([]byte(toolsJSON), &tools) - require.NoError(t, err, "Should parse tools JSON: %s", toolsJSON) - - // Find the test_environment tool - var testEnvTool map[string]any - for _, tool := range tools { - if name, ok := tool["name"].(string); ok && name == "test_environment" { - testEnvTool = tool - break - } - } - - require.NotNil(t, testEnvTool, "Should find test_environment tool in tools.json") - - // Verify the tool structure - assert.Equal(t, "test_environment", testEnvTool["name"], "Tool name should be test_environment") - assert.Equal(t, "A test job with choice input", testEnvTool["description"], "Description should match") - - // Verify the input schema - inputSchema, ok := testEnvTool["inputSchema"].(map[string]any) - require.True(t, ok, "Should have inputSchema") - - assert.Equal(t, "object", inputSchema["type"], "Schema type should be object") - assert.Equal(t, false, inputSchema["additionalProperties"], "Should not allow additional properties") - - // Verify properties - properties, ok := inputSchema["properties"].(map[string]any) - require.True(t, ok, "Should have properties") - - // Check environment property - envProp, ok := properties["environment"].(map[string]any) - require.True(t, ok, "Should have environment property") - assert.Equal(t, "string", envProp["type"], "Environment type should be string (choice)") - assert.Equal(t, "Target environment", envProp["description"], "Environment description should match") - - envEnum, ok := envProp["enum"].([]any) - require.True(t, ok, "Should have enum for environment") - assert.Len(t, envEnum, 2, "Should have 2 options") - assert.Contains(t, envEnum, "staging", "Should contain staging option") - assert.Contains(t, envEnum, "production", "Should contain production option") - - // Check test_type property - testTypeProp, ok := properties["test_type"].(map[string]any) - require.True(t, ok, "Should have test_type property") - assert.Equal(t, "string", testTypeProp["type"], "Test type should be string (choice)") - assert.Equal(t, "Type of test to run", testTypeProp["description"], "Test type description should match") - - testTypeEnum, ok := testTypeProp["enum"].([]any) - require.True(t, ok, "Should have enum for test_type") - assert.Len(t, testTypeEnum, 3, "Should have 3 options") - assert.Contains(t, testTypeEnum, "smoke", "Should contain smoke option") - assert.Contains(t, testTypeEnum, "integration", "Should contain integration option") - assert.Contains(t, testTypeEnum, "e2e", "Should contain e2e option") - - // Verify required fields - required, ok := inputSchema["required"].([]any) - require.True(t, ok, "Should have required array") - assert.Len(t, required, 2, "Should have 2 required fields") - assert.Contains(t, required, "environment", "Environment should be required") - assert.Contains(t, required, "test_type", "Test type should be required") -} - -// TestCustomJobToolsWithDifferentInputTypes verifies that custom jobs -// with different input types are correctly converted to JSON Schema -func TestCustomJobToolsWithDifferentInputTypes(t *testing.T) { - workflowData := &WorkflowData{ - SafeOutputs: &SafeOutputsConfig{ - Jobs: map[string]*SafeJobConfig{ - "multi_input_job": { - Description: "Job with multiple input types", - Inputs: map[string]*InputDefinition{ - "name": { - Description: "User name", - Required: true, - Type: "string", - Default: "Alice", - }, - "count": { - Description: "Number of items", - Required: false, - Type: "number", - Default: 10, - }, - "enabled": { - Description: "Enable feature", - Required: true, - Type: "boolean", - }, - "mode": { - Description: "Operation mode", - Required: false, - Type: "choice", - Options: []string{"fast", "slow", "medium"}, - Default: "medium", - }, - }, - }, - }, - }, - } - - // Generate the tools JSON - toolsJSON, err := generateFilteredToolsJSON(workflowData, ".github/workflows/test-workflow.md") - require.NoError(t, err, "Should generate tools JSON") - - // Parse the JSON - var tools []map[string]any - err = json.Unmarshal([]byte(toolsJSON), &tools) - require.NoError(t, err, "Should parse tools JSON") - - // Find the multi_input_job tool - var jobTool map[string]any - for _, tool := range tools { - if name, ok := tool["name"].(string); ok && name == "multi_input_job" { - jobTool = tool - break - } - } - - require.NotNil(t, jobTool, "Should find multi_input_job tool in tools.json") - - // Verify the input schema - inputSchema, ok := jobTool["inputSchema"].(map[string]any) - require.True(t, ok, "Should have inputSchema") - - properties, ok := inputSchema["properties"].(map[string]any) - require.True(t, ok, "Should have properties") - - // Check string type - nameProp, ok := properties["name"].(map[string]any) - require.True(t, ok, "Should have name property") - assert.Equal(t, "string", nameProp["type"], "Name should be string type") - assert.Equal(t, "Alice", nameProp["default"], "Name should have default value") - - // Check number type - countProp, ok := properties["count"].(map[string]any) - require.True(t, ok, "Should have count property") - assert.Equal(t, "number", countProp["type"], "Count should be number type") - // Note: JSON numbers are float64 after unmarshal - assert.InDelta(t, float64(10), countProp["default"], 0.01, "Count should have default value") - - // Check boolean type - enabledProp, ok := properties["enabled"].(map[string]any) - require.True(t, ok, "Should have enabled property") - assert.Equal(t, "boolean", enabledProp["type"], "Enabled should be boolean type") - - // Check choice type - modeProp, ok := properties["mode"].(map[string]any) - require.True(t, ok, "Should have mode property") - assert.Equal(t, "string", modeProp["type"], "Mode should be string type (choice)") - assert.Equal(t, "medium", modeProp["default"], "Mode should have default value") - - modeEnum, ok := modeProp["enum"].([]any) - require.True(t, ok, "Should have enum for mode") - assert.Len(t, modeEnum, 3, "Should have 3 options") - - // Verify required fields - required, ok := inputSchema["required"].([]any) - require.True(t, ok, "Should have required array") - assert.Len(t, required, 2, "Should have 2 required fields") - assert.Contains(t, required, "name", "Name should be required") - assert.Contains(t, required, "enabled", "Enabled should be required") - assert.NotContains(t, required, "count", "Count should not be required") - assert.NotContains(t, required, "mode", "Mode should not be required") -} - -// TestCustomJobToolsRequiredFieldsSorted verifies that the required array -// is sorted alphabetically for stable output -func TestCustomJobToolsRequiredFieldsSorted(t *testing.T) { - workflowData := &WorkflowData{ - SafeOutputs: &SafeOutputsConfig{ - Jobs: map[string]*SafeJobConfig{ - "sorted_test": { - Description: "Job to test sorted required fields", - Inputs: map[string]*InputDefinition{ - "zebra": { - Description: "Last alphabetically", - Required: true, - Type: "string", - }, - "apple": { - Description: "First alphabetically", - Required: true, - Type: "string", - }, - "middle": { - Description: "Middle alphabetically", - Required: true, - Type: "string", - }, - "banana": { - Description: "Second alphabetically", - Required: true, - Type: "string", - }, - }, - }, - }, - }, - } - - // Generate the tools JSON - toolsJSON, err := generateFilteredToolsJSON(workflowData, ".github/workflows/test-workflow.md") - require.NoError(t, err, "Should generate tools JSON") - - // Parse the JSON - var tools []map[string]any - err = json.Unmarshal([]byte(toolsJSON), &tools) - require.NoError(t, err, "Should parse tools JSON") - - // Find the sorted_test tool - var sortedTestTool map[string]any - for _, tool := range tools { - if name, ok := tool["name"].(string); ok && name == "sorted_test" { - sortedTestTool = tool - break - } - } - - require.NotNil(t, sortedTestTool, "Should find sorted_test tool in tools.json") - - // Verify the input schema - inputSchema, ok := sortedTestTool["inputSchema"].(map[string]any) - require.True(t, ok, "Should have inputSchema") - - // Verify required fields are sorted - required, ok := inputSchema["required"].([]any) - require.True(t, ok, "Should have required array") - assert.Len(t, required, 4, "Should have 4 required fields") - - // Check that the required array is sorted alphabetically - expectedOrder := []string{"apple", "banana", "middle", "zebra"} - for i, expectedField := range expectedOrder { - actualField, ok := required[i].(string) - require.True(t, ok, "Required field should be a string") - assert.Equal(t, expectedField, actualField, "Required field at index %d should be %s", i, expectedField) - } -} diff --git a/pkg/workflow/safe_outputs_state.go b/pkg/workflow/safe_outputs_state.go index 8b371a081a..bc727c5e6d 100644 --- a/pkg/workflow/safe_outputs_state.go +++ b/pkg/workflow/safe_outputs_state.go @@ -3,7 +3,6 @@ package workflow import ( "fmt" "reflect" - "sort" "github.com/github/gh-aw/pkg/logger" ) @@ -171,36 +170,6 @@ func HasSafeOutputsEnabled(safeOutputs *SafeOutputsConfig) bool { return enabled } -// checkAllEnabledToolsPresent verifies that every tool in enabledTools has a matching entry -// in filteredTools. This is a compiler error check: if a safe-output type is registered in -// Go code but its definition is missing from safe-output-tools.json, it will not appear in -// filteredTools and this function returns an error. -// -// Dispatch-workflow and custom-job tools are intentionally excluded from this check because -// they are generated dynamically and are never part of the static tools JSON. -func checkAllEnabledToolsPresent(enabledTools map[string]bool, filteredTools []map[string]any) error { - presentTools := make(map[string]bool, len(filteredTools)) - for _, tool := range filteredTools { - if name, ok := tool["name"].(string); ok { - presentTools[name] = true - } - } - - var missingTools []string - for toolName := range enabledTools { - if !presentTools[toolName] { - missingTools = append(missingTools, toolName) - } - } - - if len(missingTools) == 0 { - return nil - } - - sort.Strings(missingTools) - return fmt.Errorf("compiler error: safe-output tool(s) %v are registered but missing from safe-output-tools.json; please report this issue to the developer", missingTools) -} - // applyDefaultCreateIssue injects a default create-issues safe output when safe-outputs is configured // but has no non-builtin output types. The injected config uses the workflow ID as the label // and [workflowID] as the title prefix. The AutoInjectedCreateIssue flag is set so the prompt diff --git a/pkg/workflow/safe_outputs_tools.go b/pkg/workflow/safe_outputs_tools.go index 4076a63779..df6264cc3b 100644 --- a/pkg/workflow/safe_outputs_tools.go +++ b/pkg/workflow/safe_outputs_tools.go @@ -57,8 +57,3 @@ func GetSafeOutputToolOptions() []SafeOutputToolOption { } return options } - -// GetSafeOutputsToolsJSON returns the raw JSON content of safe_outputs_tools.json. -func GetSafeOutputsToolsJSON() string { - return safeOutputsToolsJSONContent -} diff --git a/pkg/workflow/safe_outputs_tools_generation.go b/pkg/workflow/safe_outputs_tools_generation.go index cdeacbcf5c..0d19b41076 100644 --- a/pkg/workflow/safe_outputs_tools_generation.go +++ b/pkg/workflow/safe_outputs_tools_generation.go @@ -3,7 +3,6 @@ package workflow import ( "encoding/json" "fmt" - "maps" "sort" "github.com/github/gh-aw/pkg/stringutil" @@ -19,468 +18,11 @@ import ( // SafeOutputsConfig. Dynamic tools (dispatch-workflow, custom jobs) are also // generated here. // -// There are two generation strategies: -// -// 1. generateFilteredToolsJSON: Legacy approach that loads the embedded -// safe_outputs_tools.json, filters and enhances it in Go, then returns the -// full JSON to be inlined directly into the compiled workflow YAML as a -// heredoc. Used by tests and kept for backward compatibility. -// -// 2. generateToolsMetaJSON: New approach that generates a small "meta" JSON -// (description suffixes, repo params, dynamic tools) to be written at -// compile time. At runtime, generate_safe_outputs_tools.cjs reads the -// source safe_outputs_tools.json from the actions folder, applies the meta -// overrides, and writes the final tools.json—avoiding inlining the entire -// file into the compiled workflow YAML. - -// generateFilteredToolsJSON filters the ALL_TOOLS array based on enabled safe outputs. -// Returns a JSON string containing only the tools that are enabled in the workflow. -func generateFilteredToolsJSON(data *WorkflowData, markdownPath string) (string, error) { - if data.SafeOutputs == nil { - return "[]", nil - } - - safeOutputsConfigLog.Print("Generating filtered tools JSON for workflow") - - // Load the full tools JSON - allToolsJSON := GetSafeOutputsToolsJSON() - - // Parse the JSON to get all tools - var allTools []map[string]any - if err := json.Unmarshal([]byte(allToolsJSON), &allTools); err != nil { - safeOutputsConfigLog.Printf("Failed to parse safe outputs tools JSON: %v", err) - return "", fmt.Errorf("failed to parse safe outputs tools JSON: %w", err) - } - - // Create a set of enabled tool names - enabledTools := make(map[string]bool) - - // Check which safe outputs are enabled and add their corresponding tool names - if data.SafeOutputs.CreateIssues != nil { - enabledTools["create_issue"] = true - } - if data.SafeOutputs.CreateAgentSessions != nil { - enabledTools["create_agent_session"] = true - } - if data.SafeOutputs.CreateDiscussions != nil { - enabledTools["create_discussion"] = true - } - if data.SafeOutputs.UpdateDiscussions != nil { - enabledTools["update_discussion"] = true - } - if data.SafeOutputs.CloseDiscussions != nil { - enabledTools["close_discussion"] = true - } - if data.SafeOutputs.CloseIssues != nil { - enabledTools["close_issue"] = true - } - if data.SafeOutputs.ClosePullRequests != nil { - enabledTools["close_pull_request"] = true - } - if data.SafeOutputs.MarkPullRequestAsReadyForReview != nil { - enabledTools["mark_pull_request_as_ready_for_review"] = true - } - if data.SafeOutputs.AddComments != nil { - enabledTools["add_comment"] = true - } - if data.SafeOutputs.CreatePullRequests != nil { - enabledTools["create_pull_request"] = true - } - if data.SafeOutputs.CreatePullRequestReviewComments != nil { - enabledTools["create_pull_request_review_comment"] = true - } - if data.SafeOutputs.SubmitPullRequestReview != nil { - enabledTools["submit_pull_request_review"] = true - } - if data.SafeOutputs.ReplyToPullRequestReviewComment != nil { - enabledTools["reply_to_pull_request_review_comment"] = true - } - if data.SafeOutputs.ResolvePullRequestReviewThread != nil { - enabledTools["resolve_pull_request_review_thread"] = true - } - if data.SafeOutputs.CreateCodeScanningAlerts != nil { - enabledTools["create_code_scanning_alert"] = true - } - if data.SafeOutputs.AutofixCodeScanningAlert != nil { - enabledTools["autofix_code_scanning_alert"] = true - } - if data.SafeOutputs.AddLabels != nil { - enabledTools["add_labels"] = true - } - if data.SafeOutputs.RemoveLabels != nil { - enabledTools["remove_labels"] = true - } - if data.SafeOutputs.AddReviewer != nil { - enabledTools["add_reviewer"] = true - } - if data.SafeOutputs.AssignMilestone != nil { - enabledTools["assign_milestone"] = true - } - if data.SafeOutputs.AssignToAgent != nil { - enabledTools["assign_to_agent"] = true - } - if data.SafeOutputs.AssignToUser != nil { - enabledTools["assign_to_user"] = true - } - if data.SafeOutputs.UnassignFromUser != nil { - enabledTools["unassign_from_user"] = true - } - if data.SafeOutputs.UpdateIssues != nil { - enabledTools["update_issue"] = true - } - if data.SafeOutputs.UpdatePullRequests != nil { - enabledTools["update_pull_request"] = true - } - if data.SafeOutputs.PushToPullRequestBranch != nil { - enabledTools["push_to_pull_request_branch"] = true - } - if data.SafeOutputs.UploadAssets != nil { - enabledTools["upload_asset"] = true - } - if data.SafeOutputs.MissingTool != nil { - enabledTools["missing_tool"] = true - } - if data.SafeOutputs.MissingData != nil { - enabledTools["missing_data"] = true - } - if data.SafeOutputs.UpdateRelease != nil { - enabledTools["update_release"] = true - } - if data.SafeOutputs.NoOp != nil { - enabledTools["noop"] = true - } - if data.SafeOutputs.LinkSubIssue != nil { - enabledTools["link_sub_issue"] = true - } - if data.SafeOutputs.HideComment != nil { - enabledTools["hide_comment"] = true - } - if data.SafeOutputs.SetIssueType != nil { - enabledTools["set_issue_type"] = true - } - if data.SafeOutputs.UpdateProjects != nil { - enabledTools["update_project"] = true - } - if data.SafeOutputs.CreateProjectStatusUpdates != nil { - enabledTools["create_project_status_update"] = true - } - if data.SafeOutputs.CreateProjects != nil { - enabledTools["create_project"] = true - } - // Note: dispatch_workflow tools are generated dynamically below, not from the static tools list - - // Add push_repo_memory tool if repo-memory is configured - // This tool enables early size validation during the agent session - if data.RepoMemoryConfig != nil && len(data.RepoMemoryConfig.Memories) > 0 { - enabledTools["push_repo_memory"] = true - } - - // Filter tools to only include enabled ones and enhance descriptions - var filteredTools []map[string]any - for _, tool := range allTools { - toolName, ok := tool["name"].(string) - if !ok { - continue - } - if enabledTools[toolName] { - // Create a copy of the tool to avoid modifying the original - enhancedTool := make(map[string]any) - maps.Copy(enhancedTool, tool) - - // Enhance the description with configuration details - if description, ok := enhancedTool["description"].(string); ok { - enhancedDescription := enhanceToolDescription(toolName, description, data.SafeOutputs) - enhancedTool["description"] = enhancedDescription - } - - // Filter schema fields based on what is configured as allowed - filterToolSchemaFields(enhancedTool, toolName, data.SafeOutputs) - - // Add repo parameter to inputSchema if allowed-repos has entries - addRepoParameterIfNeeded(enhancedTool, toolName, data.SafeOutputs) - - filteredTools = append(filteredTools, enhancedTool) - } - } - - // Verify all registered safe-outputs are present in the static tools JSON. - // Dispatch-workflow and custom-job tools are excluded because they are generated dynamically. - if err := checkAllEnabledToolsPresent(enabledTools, filteredTools); err != nil { - return "", err - } - - // Add custom job tools from SafeOutputs.Jobs - if len(data.SafeOutputs.Jobs) > 0 { - safeOutputsConfigLog.Printf("Adding %d custom job tools", len(data.SafeOutputs.Jobs)) - - // Sort job names for deterministic output - // This ensures compiled workflows have consistent tool ordering - jobNames := make([]string, 0, len(data.SafeOutputs.Jobs)) - for jobName := range data.SafeOutputs.Jobs { - jobNames = append(jobNames, jobName) - } - sort.Strings(jobNames) - - // Iterate over jobs in sorted order - for _, jobName := range jobNames { - jobConfig := data.SafeOutputs.Jobs[jobName] - // Normalize job name to use underscores for consistency - normalizedJobName := stringutil.NormalizeSafeOutputIdentifier(jobName) - - // Create the tool definition for this custom job - customTool := generateCustomJobToolDefinition(normalizedJobName, jobConfig) - filteredTools = append(filteredTools, customTool) - } - } - - // Add custom script tools from SafeOutputs.Scripts - if len(data.SafeOutputs.Scripts) > 0 { - safeOutputsConfigLog.Printf("Adding %d custom script tools", len(data.SafeOutputs.Scripts)) - - // Sort script names for deterministic output - scriptNames := make([]string, 0, len(data.SafeOutputs.Scripts)) - for scriptName := range data.SafeOutputs.Scripts { - scriptNames = append(scriptNames, scriptName) - } - sort.Strings(scriptNames) - - for _, scriptName := range scriptNames { - scriptConfig := data.SafeOutputs.Scripts[scriptName] - normalizedScriptName := stringutil.NormalizeSafeOutputIdentifier(scriptName) - customTool := generateCustomScriptToolDefinition(normalizedScriptName, scriptConfig) - filteredTools = append(filteredTools, customTool) - } - } - - // Add custom action tools from SafeOutputs.Actions. - // Action resolution (action.yml fetch + pinning) is done once in buildJobs before - // generateToolsMetaJSON/generateFilteredToolsJSON are called, so Inputs and - // ActionDescription are already populated here. - if len(data.SafeOutputs.Actions) > 0 { - safeOutputsConfigLog.Printf("Adding %d custom action tools", len(data.SafeOutputs.Actions)) - - actionNames := make([]string, 0, len(data.SafeOutputs.Actions)) - for actionName := range data.SafeOutputs.Actions { - actionNames = append(actionNames, actionName) - } - sort.Strings(actionNames) - - for _, actionName := range actionNames { - actionConfig := data.SafeOutputs.Actions[actionName] - customTool := generateActionToolDefinition(actionName, actionConfig) - filteredTools = append(filteredTools, customTool) - } - } - - if safeOutputsConfigLog.Enabled() { - safeOutputsConfigLog.Printf("Filtered %d tools from %d total tools (including %d custom jobs, %d custom scripts, %d custom actions)", len(filteredTools), len(allTools), len(data.SafeOutputs.Jobs), len(data.SafeOutputs.Scripts), len(data.SafeOutputs.Actions)) - } - - // Add dynamic dispatch_workflow tools - if data.SafeOutputs.DispatchWorkflow != nil && len(data.SafeOutputs.DispatchWorkflow.Workflows) > 0 { - safeOutputsConfigLog.Printf("Adding %d dispatch_workflow tools", len(data.SafeOutputs.DispatchWorkflow.Workflows)) - - // Initialize WorkflowFiles map if not already initialized - if data.SafeOutputs.DispatchWorkflow.WorkflowFiles == nil { - data.SafeOutputs.DispatchWorkflow.WorkflowFiles = make(map[string]string) - } - - for _, workflowName := range data.SafeOutputs.DispatchWorkflow.Workflows { - // Find the workflow file in multiple locations - fileResult, err := findWorkflowFile(workflowName, markdownPath) - if err != nil { - safeOutputsConfigLog.Printf("Warning: error finding workflow %s: %v", workflowName, err) - // Continue with empty inputs - tool := generateDispatchWorkflowTool(workflowName, make(map[string]any)) - filteredTools = append(filteredTools, tool) - continue - } - - // Determine which file to use - priority: .lock.yml > .yml > .md (batch target) - var workflowPath string - var extension string - var useMD bool - if fileResult.lockExists { - workflowPath = fileResult.lockPath - extension = ".lock.yml" - } else if fileResult.ymlExists { - workflowPath = fileResult.ymlPath - extension = ".yml" - } else if fileResult.mdExists { - // .md-only: the workflow is a same-batch compilation target that will produce a .lock.yml - workflowPath = fileResult.mdPath - extension = ".lock.yml" - useMD = true - } else { - safeOutputsConfigLog.Printf("Warning: no workflow file found for %s (checked .lock.yml, .yml, .md)", workflowName) - // Continue with empty inputs - tool := generateDispatchWorkflowTool(workflowName, make(map[string]any)) - filteredTools = append(filteredTools, tool) - continue - } - - // Store the file extension for runtime use - data.SafeOutputs.DispatchWorkflow.WorkflowFiles[workflowName] = extension - - // Extract workflow_dispatch inputs - var workflowInputs map[string]any - var inputsErr error - if useMD { - workflowInputs, inputsErr = extractMDWorkflowDispatchInputs(workflowPath) - } else { - workflowInputs, inputsErr = extractWorkflowDispatchInputs(workflowPath) - } - if inputsErr != nil { - safeOutputsConfigLog.Printf("Warning: failed to extract inputs for workflow %s from %s: %v", workflowName, workflowPath, inputsErr) - // Continue with empty inputs - workflowInputs = make(map[string]any) - } - - // Generate tool schema - tool := generateDispatchWorkflowTool(workflowName, workflowInputs) - filteredTools = append(filteredTools, tool) - } - } - - // Add dynamic dispatch_repository tools - if data.SafeOutputs.DispatchRepository != nil && len(data.SafeOutputs.DispatchRepository.Tools) > 0 { - safeOutputsConfigLog.Printf("Adding %d dispatch_repository tools", len(data.SafeOutputs.DispatchRepository.Tools)) - - // Sort tool keys for deterministic output - toolKeys := make([]string, 0, len(data.SafeOutputs.DispatchRepository.Tools)) - for toolKey := range data.SafeOutputs.DispatchRepository.Tools { - toolKeys = append(toolKeys, toolKey) - } - sort.Strings(toolKeys) - - for _, toolKey := range toolKeys { - toolConfig := data.SafeOutputs.DispatchRepository.Tools[toolKey] - tool := generateDispatchRepositoryTool(toolKey, toolConfig) - filteredTools = append(filteredTools, tool) - } - } - - // Add dynamic call_workflow tools - if data.SafeOutputs.CallWorkflow != nil && len(data.SafeOutputs.CallWorkflow.Workflows) > 0 { - safeOutputsConfigLog.Printf("Adding %d call_workflow tools", len(data.SafeOutputs.CallWorkflow.Workflows)) - - // Initialize WorkflowFiles map if not already initialized - if data.SafeOutputs.CallWorkflow.WorkflowFiles == nil { - data.SafeOutputs.CallWorkflow.WorkflowFiles = make(map[string]string) - } - - for _, workflowName := range data.SafeOutputs.CallWorkflow.Workflows { - // Find the workflow file in multiple locations - fileResult, err := findWorkflowFile(workflowName, markdownPath) - if err != nil { - safeOutputsConfigLog.Printf("Warning: error finding workflow %s: %v", workflowName, err) - tool := generateCallWorkflowTool(workflowName, make(map[string]any)) - filteredTools = append(filteredTools, tool) - continue - } - - // Determine which file to use - priority: .lock.yml > .yml > .md (batch target) - var workflowPath string - var extension string - var useMD bool - if fileResult.lockExists { - workflowPath = fileResult.lockPath - extension = ".lock.yml" - } else if fileResult.ymlExists { - workflowPath = fileResult.ymlPath - extension = ".yml" - } else if fileResult.mdExists { - workflowPath = fileResult.mdPath - extension = ".lock.yml" - useMD = true - } else { - safeOutputsConfigLog.Printf("Warning: no workflow file found for %s (checked .lock.yml, .yml, .md)", workflowName) - tool := generateCallWorkflowTool(workflowName, make(map[string]any)) - filteredTools = append(filteredTools, tool) - continue - } - - // Store the relative path for compile-time use - relativePath := fmt.Sprintf("./.github/workflows/%s%s", workflowName, extension) - data.SafeOutputs.CallWorkflow.WorkflowFiles[workflowName] = relativePath - - // Extract workflow_call inputs - var workflowInputs map[string]any - var inputsErr error - if useMD { - workflowInputs, inputsErr = extractMDWorkflowCallInputs(workflowPath) - } else { - workflowInputs, inputsErr = extractWorkflowCallInputs(workflowPath) - } - if inputsErr != nil { - safeOutputsConfigLog.Printf("Warning: failed to extract inputs for workflow %s from %s: %v", workflowName, workflowPath, inputsErr) - workflowInputs = make(map[string]any) - } - - // Generate tool schema - tool := generateCallWorkflowTool(workflowName, workflowInputs) - filteredTools = append(filteredTools, tool) - } - } - - // Marshal the filtered tools back to JSON with indentation for better readability - // and to reduce merge conflicts in generated lockfiles - filteredJSON, err := json.MarshalIndent(filteredTools, "", " ") - if err != nil { - safeOutputsConfigLog.Printf("Failed to marshal filtered tools: %v", err) - return "", fmt.Errorf("failed to marshal filtered tools: %w", err) - } - - safeOutputsConfigLog.Printf("Successfully generated filtered tools JSON with %d tools", len(filteredTools)) - return string(filteredJSON), nil -} - -// filterToolSchemaFields removes fields from a tool's inputSchema properties -// that are not enabled by the safe output configuration. This prevents the AI -// from attempting to use fields that would be rejected at runtime. -func filterToolSchemaFields(tool map[string]any, toolName string, safeOutputs *SafeOutputsConfig) { - if safeOutputs == nil { - return - } - - switch toolName { - case "update_discussion": - config := safeOutputs.UpdateDiscussions - if config == nil { - return - } - // Deep-copy the inputSchema so we don't modify the shared original - inputSchema, ok := tool["inputSchema"].(map[string]any) - if !ok { - return - } - properties, ok := inputSchema["properties"].(map[string]any) - if !ok { - return - } - // Clone properties and remove fields that are not configured as allowed (nil or false). - // This aligns with the runtime check in update_discussion.cjs which requires - // allow_title/allow_body/allow_labels to be explicitly true. - newProps := make(map[string]any, len(properties)) - maps.Copy(newProps, properties) - if config.Title == nil || !*config.Title { - delete(newProps, "title") - } - if config.Body == nil || !*config.Body { - delete(newProps, "body") - } - if config.Labels == nil || !*config.Labels { - delete(newProps, "labels") - } - // Clone inputSchema with new properties - newSchema := make(map[string]any, len(inputSchema)) - maps.Copy(newSchema, inputSchema) - newSchema["properties"] = newProps - tool["inputSchema"] = newSchema - safeOutputsConfigLog.Printf("Filtered update_discussion schema fields: title=%v, body=%v, labels=%v", - config.Title != nil && *config.Title, config.Body != nil && *config.Body, config.Labels != nil && *config.Labels) - } -} +// generateToolsMetaJSON generates a small "meta" JSON (description suffixes, +// repo params, dynamic tools) to be written at compile time. At runtime, +// generate_safe_outputs_tools.cjs reads the source safe_outputs_tools.json +// from the actions folder, applies the meta overrides, and writes the final +// tools.json—avoiding inlining the entire file into the compiled workflow YAML. // generateDynamicTools generates MCP tool definitions for dynamic tools: // custom safe-jobs, dispatch_workflow targets, and call_workflow targets. diff --git a/pkg/workflow/safe_outputs_tools_generation_test.go b/pkg/workflow/safe_outputs_tools_generation_test.go index b6defc598f..22c1cffe78 100644 --- a/pkg/workflow/safe_outputs_tools_generation_test.go +++ b/pkg/workflow/safe_outputs_tools_generation_test.go @@ -3,7 +3,6 @@ package workflow import ( - "encoding/json" "testing" "github.com/stretchr/testify/assert" @@ -202,46 +201,6 @@ func TestAddRepoParameterIfNeededSpecificTargetRepoNoAllowedRepos(t *testing.T) assert.False(t, hasRepo, "repo parameter should NOT be added when target-repo is specific and no allowed-repos") } -func TestGenerateFilteredToolsJSONUpdateIssueWithWildcardTargetRepo(t *testing.T) { - data := &WorkflowData{ - SafeOutputs: &SafeOutputsConfig{ - UpdateIssues: &UpdateIssuesConfig{ - UpdateEntityConfig: UpdateEntityConfig{ - SafeOutputTargetConfig: SafeOutputTargetConfig{ - TargetRepoSlug: "*", - }, - }, - }, - }, - } - - result, err := generateFilteredToolsJSON(data, ".github/workflows/test.md") - require.NoError(t, err, "generateFilteredToolsJSON should not error") - - var tools []map[string]any - require.NoError(t, json.Unmarshal([]byte(result), &tools), "Result should be valid JSON") - - // Find update_issue tool - var updateIssueTool map[string]any - for _, tool := range tools { - if tool["name"] == "update_issue" { - updateIssueTool = tool - break - } - } - require.NotNil(t, updateIssueTool, "update_issue tool should be present when target-repo is wildcard") - - // Verify repo parameter is in the schema - inputSchema, ok := updateIssueTool["inputSchema"].(map[string]any) - require.True(t, ok, "inputSchema should be present") - properties, ok := inputSchema["properties"].(map[string]any) - require.True(t, ok, "properties should be present") - _, hasRepo := properties["repo"] - assert.True(t, hasRepo, "repo parameter should be added to update_issue when target-repo is wildcard") -} - -// TestParseUpdateIssuesConfigWithWildcardTargetRepo tests that parseUpdateIssuesConfig -// returns a non-nil config when target-repo is "*". func TestParseUpdateIssuesConfigWithWildcardTargetRepo(t *testing.T) { compiler := &Compiler{} outputMap := map[string]any{ @@ -334,217 +293,5 @@ func TestGenerateDispatchWorkflowToolRequiredSorted(t *testing.T) { } } -// TestCheckAllEnabledToolsPresentAllMatch tests that no error is returned when all enabled tools are present. -func TestCheckAllEnabledToolsPresentAllMatch(t *testing.T) { - enabledTools := map[string]bool{ - "create_issue": true, - "add_comment": true, - } - filteredTools := []map[string]any{ - {"name": "create_issue"}, - {"name": "add_comment"}, - } - - err := checkAllEnabledToolsPresent(enabledTools, filteredTools) - assert.NoError(t, err, "should not error when all enabled tools are present") -} - -// TestCheckAllEnabledToolsPresentMissing tests that an error is returned when a tool is missing. -func TestCheckAllEnabledToolsPresentMissing(t *testing.T) { - enabledTools := map[string]bool{ - "create_issue": true, - "nonexistent_tool": true, - } - filteredTools := []map[string]any{ - {"name": "create_issue"}, - } - - err := checkAllEnabledToolsPresent(enabledTools, filteredTools) - require.Error(t, err, "should error when an enabled tool is missing from filtered tools") - assert.Contains(t, err.Error(), "nonexistent_tool", "error should mention the missing tool") - assert.Contains(t, err.Error(), "compiler error", "error should be labelled as a compiler error") - assert.Contains(t, err.Error(), "report this issue to the developer", "error should instruct user to report") -} - -// TestCheckAllEnabledToolsPresentEmpty tests that no error is returned when there are no enabled tools. -func TestCheckAllEnabledToolsPresentEmpty(t *testing.T) { - err := checkAllEnabledToolsPresent(map[string]bool{}, []map[string]any{}) - assert.NoError(t, err, "should not error with empty inputs") -} - -// TestCheckAllEnabledToolsPresentMultipleMissing tests that multiple missing tools are all reported. -func TestCheckAllEnabledToolsPresentMultipleMissing(t *testing.T) { - enabledTools := map[string]bool{ - "tool_a": true, - "tool_b": true, - "tool_c": true, - } - filteredTools := []map[string]any{ - {"name": "tool_a"}, - } - - err := checkAllEnabledToolsPresent(enabledTools, filteredTools) - require.Error(t, err, "should error when multiple enabled tools are missing") - assert.Contains(t, err.Error(), "tool_b", "error should mention tool_b") - assert.Contains(t, err.Error(), "tool_c", "error should mention tool_c") -} - // TestGenerateFilteredToolsJSONWithStandardOutputs tests that standard safe outputs produce // the expected tools in the filtered output (regression test for the completeness check). -func TestGenerateFilteredToolsJSONWithStandardOutputs(t *testing.T) { - data := &WorkflowData{ - SafeOutputs: &SafeOutputsConfig{ - CreateIssues: &CreateIssuesConfig{}, - MissingData: &MissingDataConfig{}, - // MissingTool is the built-in safe-output type that lets the AI report that a - // requested tool is unavailable; it is distinct from "a tool definition that is missing". - MissingTool: &MissingToolConfig{}, - // NoOp (null) is the built-in fallback safe-output type for transparency. - NoOp: &NoOpConfig{}, - }, - } - - result, err := generateFilteredToolsJSON(data, ".github/workflows/test.md") - require.NoError(t, err, "should not error when all standard tools are present in static JSON") - - var tools []map[string]any - require.NoError(t, json.Unmarshal([]byte(result), &tools), "result should be valid JSON") - - toolNames := make(map[string]bool) - for _, tool := range tools { - if name, ok := tool["name"].(string); ok { - toolNames[name] = true - } - } - assert.True(t, toolNames["create_issue"], "create_issue should be present") - assert.True(t, toolNames["missing_data"], "missing_data should be present") - assert.True(t, toolNames["missing_tool"], "missing_tool should be present") - assert.True(t, toolNames["noop"], "noop should be present") -} - -// TestGenerateFilteredToolsJSONCustomJobs tests that custom job tools are included in filtered output. -func TestGenerateFilteredToolsJSONCustomJobs(t *testing.T) { - data := &WorkflowData{ - SafeOutputs: &SafeOutputsConfig{ - Jobs: map[string]*SafeJobConfig{ - "my_job": { - Description: "My custom job", - Inputs: map[string]*InputDefinition{ - "input1": { - Type: "string", - Description: "First input", - Required: true, - }, - }, - }, - }, - }, - } - - result, err := generateFilteredToolsJSON(data, ".github/workflows/test.md") - require.NoError(t, err, "generateFilteredToolsJSON should not error") - - var tools []map[string]any - require.NoError(t, json.Unmarshal([]byte(result), &tools), "Result should be valid JSON") - - require.Len(t, tools, 1, "Should have exactly 1 tool") - assert.Equal(t, "my_job", tools[0]["name"], "Tool name should match job name") - assert.Equal(t, "My custom job", tools[0]["description"], "Description should match") -} - -// TestGenerateFilteredToolsJSONSortsCustomJobs tests that custom jobs are sorted deterministically. -func TestGenerateFilteredToolsJSONSortsCustomJobs(t *testing.T) { - data := &WorkflowData{ - SafeOutputs: &SafeOutputsConfig{ - Jobs: map[string]*SafeJobConfig{ - "z_job": {Description: "Z job"}, - "a_job": {Description: "A job"}, - "m_job": {Description: "M job"}, - }, - }, - } - - result, err := generateFilteredToolsJSON(data, ".github/workflows/test.md") - require.NoError(t, err, "generateFilteredToolsJSON should not error") - - var tools []map[string]any - require.NoError(t, json.Unmarshal([]byte(result), &tools), "Result should be valid JSON") - - require.Len(t, tools, 3, "Should have 3 tools") - assert.Equal(t, "a_job", tools[0]["name"], "First tool should be a_job (alphabetical)") - assert.Equal(t, "m_job", tools[1]["name"], "Second tool should be m_job") - assert.Equal(t, "z_job", tools[2]["name"], "Third tool should be z_job") -} - -// TestGenerateFilteredToolsJSONIncludesPushRepoMemoryWithRepoMemoryConfig tests that -// push_repo_memory is included in the filtered tools when RepoMemoryConfig is present. -func TestGenerateFilteredToolsJSONIncludesPushRepoMemoryWithRepoMemoryConfig(t *testing.T) { - data := &WorkflowData{ - SafeOutputs: &SafeOutputsConfig{}, - RepoMemoryConfig: &RepoMemoryConfig{ - Memories: []RepoMemoryEntry{ - {ID: "default", MaxFileSize: 10240, MaxPatchSize: 10240, MaxFileCount: 100}, - }, - }, - } - - result, err := generateFilteredToolsJSON(data, ".github/workflows/test.md") - require.NoError(t, err, "generateFilteredToolsJSON should not error") - - var tools []map[string]any - require.NoError(t, json.Unmarshal([]byte(result), &tools), "Result should be valid JSON") - - toolNames := make(map[string]bool) - for _, tool := range tools { - if name, ok := tool["name"].(string); ok { - toolNames[name] = true - } - } - assert.True(t, toolNames["push_repo_memory"], "push_repo_memory should be present when RepoMemoryConfig is set") -} - -// TestGenerateFilteredToolsJSONExcludesPushRepoMemoryWithoutRepoMemoryConfig tests that -// push_repo_memory is NOT included in the filtered tools when RepoMemoryConfig is absent. -func TestGenerateFilteredToolsJSONExcludesPushRepoMemoryWithoutRepoMemoryConfig(t *testing.T) { - data := &WorkflowData{ - SafeOutputs: &SafeOutputsConfig{ - CreateIssues: &CreateIssuesConfig{}, - }, - RepoMemoryConfig: nil, - } - - result, err := generateFilteredToolsJSON(data, ".github/workflows/test.md") - require.NoError(t, err, "generateFilteredToolsJSON should not error") - - var tools []map[string]any - require.NoError(t, json.Unmarshal([]byte(result), &tools), "Result should be valid JSON") - - for _, tool := range tools { - name, _ := tool["name"].(string) - assert.NotEqual(t, "push_repo_memory", name, "push_repo_memory should NOT be present when RepoMemoryConfig is nil") - } -} - -// TestGenerateFilteredToolsJSONExcludesPushRepoMemoryWithEmptyMemories tests that -// push_repo_memory is NOT included when RepoMemoryConfig has an empty Memories slice. -func TestGenerateFilteredToolsJSONExcludesPushRepoMemoryWithEmptyMemories(t *testing.T) { - data := &WorkflowData{ - SafeOutputs: &SafeOutputsConfig{ - MissingData: &MissingDataConfig{}, - }, - RepoMemoryConfig: &RepoMemoryConfig{ - Memories: []RepoMemoryEntry{}, - }, - } - - result, err := generateFilteredToolsJSON(data, ".github/workflows/test.md") - require.NoError(t, err, "generateFilteredToolsJSON should not error") - - var tools []map[string]any - require.NoError(t, json.Unmarshal([]byte(result), &tools), "Result should be valid JSON") - - for _, tool := range tools { - name, _ := tool["name"].(string) - assert.NotEqual(t, "push_repo_memory", name, "push_repo_memory should NOT be present when Memories is empty") - } -} diff --git a/pkg/workflow/safe_outputs_tools_meta_integration_test.go b/pkg/workflow/safe_outputs_tools_meta_integration_test.go index fef9237a55..765181ca73 100644 --- a/pkg/workflow/safe_outputs_tools_meta_integration_test.go +++ b/pkg/workflow/safe_outputs_tools_meta_integration_test.go @@ -18,321 +18,6 @@ import ( // TestToolsMetaJSONContainsDescriptionSuffixes verifies that the compiled workflow YAML // embeds a tools_meta.json with the correct description suffixes instead of the large // inlined tools.json used in the old approach. -func TestToolsMetaJSONContainsDescriptionSuffixes(t *testing.T) { - tmpDir := testutil.TempDir(t, "tools-meta-desc-test") - - testContent := `--- -on: push -name: Test Tools Meta Description -engine: copilot -safe-outputs: - create-issue: - max: 5 - title-prefix: "[bot] " - labels: - - automation - - testing - add-comment: - max: 10 ---- - -Test workflow for tools meta description suffixes. -` - - testFile := filepath.Join(tmpDir, "test-tools-meta-desc.md") - require.NoError(t, os.WriteFile(testFile, []byte(testContent), 0644), "should write test file") - - compiler := NewCompiler() - require.NoError(t, compiler.CompileWorkflow(testFile), "compilation should succeed") - - lockFile := filepath.Join(tmpDir, "test-tools-meta-desc.lock.yml") - yamlBytes, err := os.ReadFile(lockFile) - require.NoError(t, err, "should read lock file") - yamlStr := string(yamlBytes) - - // Must write tools_meta.json (new approach), NOT tools.json - assert.Contains(t, yamlStr, "cat > ${RUNNER_TEMP}/gh-aw/safeoutputs/tools_meta.json", "should write tools_meta.json") - assert.NotContains(t, yamlStr, "cat > ${RUNNER_TEMP}/gh-aw/safeoutputs/tools.json", "should NOT inline tools.json") - - // Must invoke the JavaScript generator at runtime - assert.Contains(t, yamlStr, "node ${RUNNER_TEMP}/gh-aw/actions/generate_safe_outputs_tools.cjs", "should invoke JS generator") - - // Extract the tools_meta.json content from the lock file - meta := extractToolsMetaFromLockFile(t, yamlStr) - - // Verify description suffixes are present and correct - createIssueSuffix, ok := meta.DescriptionSuffixes["create_issue"] - require.True(t, ok, "create_issue should have a description suffix") - assert.Contains(t, createIssueSuffix, "CONSTRAINTS:", "suffix should contain CONSTRAINTS") - assert.Contains(t, createIssueSuffix, "Maximum 5 issue(s)", "suffix should include max constraint") - assert.Contains(t, createIssueSuffix, `Title will be prefixed with "[bot] "`, "suffix should include title prefix") - assert.Contains(t, createIssueSuffix, `Labels ["automation" "testing"]`, "suffix should include labels") - - addCommentSuffix, ok := meta.DescriptionSuffixes["add_comment"] - require.True(t, ok, "add_comment should have a description suffix") - assert.Contains(t, addCommentSuffix, "Maximum 10 comment(s)", "suffix should include max constraint") -} - -// TestToolsMetaJSONDescriptionsMatchFilteredTools verifies that the description suffixes -// in tools_meta.json match what generateFilteredToolsJSON would have embedded directly. -// This is the primary regression test for the strategy change. -func TestToolsMetaJSONDescriptionsMatchFilteredTools(t *testing.T) { - tests := []struct { - name string - safeOutputs *SafeOutputsConfig - toolName string - }{ - { - name: "create_issue with max and labels", - safeOutputs: &SafeOutputsConfig{ - CreateIssues: &CreateIssuesConfig{ - BaseSafeOutputConfig: BaseSafeOutputConfig{Max: strPtr("3")}, - TitlePrefix: "[automated] ", - Labels: []string{"bot", "enhancement"}, - AllowedLabels: []string{"bug", "feature"}, - }, - }, - toolName: "create_issue", - }, - { - name: "add_comment with max and target", - safeOutputs: &SafeOutputsConfig{ - AddComments: &AddCommentsConfig{ - BaseSafeOutputConfig: BaseSafeOutputConfig{Max: strPtr("5")}, - Target: "trigger", - }, - }, - toolName: "add_comment", - }, - { - name: "create_pull_request with draft and reviewers", - safeOutputs: &SafeOutputsConfig{ - CreatePullRequests: &CreatePullRequestsConfig{ - BaseSafeOutputConfig: BaseSafeOutputConfig{Max: strPtr("2")}, - TitlePrefix: "[pr] ", - Labels: []string{"ci"}, - Draft: strPtr("true"), - Reviewers: []string{"reviewer1"}, - }, - }, - toolName: "create_pull_request", - }, - { - name: "upload_asset with max and size limit", - safeOutputs: &SafeOutputsConfig{ - UploadAssets: &UploadAssetsConfig{ - BaseSafeOutputConfig: BaseSafeOutputConfig{Max: strPtr("1")}, - MaxSizeKB: 1024, - AllowedExts: []string{".zip", ".tar.gz"}, - }, - }, - toolName: "upload_asset", - }, - } - - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - data := &WorkflowData{SafeOutputs: tt.safeOutputs} - - // Old approach: full tool JSON with description already embedded - filteredJSON, err := generateFilteredToolsJSON(data, ".github/workflows/test.md") - require.NoError(t, err, "generateFilteredToolsJSON should not error") - - var filteredTools []map[string]any - require.NoError(t, json.Unmarshal([]byte(filteredJSON), &filteredTools), "filtered JSON should parse") - - var oldTool map[string]any - for _, tool := range filteredTools { - if tool["name"] == tt.toolName { - oldTool = tool - break - } - } - require.NotNil(t, oldTool, "old approach should include tool %s", tt.toolName) - oldDescription, ok := oldTool["description"].(string) - require.True(t, ok, "old tool should have string description") - - // New approach: tools_meta.json with description suffix - metaJSON, err := generateToolsMetaJSON(data, ".github/workflows/test.md") - require.NoError(t, err, "generateToolsMetaJSON should not error") - - var meta ToolsMeta - require.NoError(t, json.Unmarshal([]byte(metaJSON), &meta), "meta JSON should parse") - - // The suffix should equal the constraint portion of the old description. - // Old description = baseDescription + suffix - // Example: "Create a new GitHub issue..." + " CONSTRAINTS: Maximum 3 issue(s) can be created." - suffix, hasSuffix := meta.DescriptionSuffixes[tt.toolName] - if strings.Contains(oldDescription, "CONSTRAINTS:") { - require.True(t, hasSuffix, "tools_meta should have suffix for %s when old description has constraints", tt.toolName) - assert.True(t, strings.HasSuffix(oldDescription, suffix), - "old description should end with the suffix from tools_meta\n old: %q\n suffix: %q", oldDescription, suffix) - } else { - assert.False(t, hasSuffix, "tools_meta should NOT have suffix for %s when there are no constraints", tt.toolName) - } - }) - } -} - -// TestToolsMetaJSONRepoParamsMatchFilteredTools verifies that repo parameter definitions -// in tools_meta.json match the repo parameters embedded in the old full tools.json. -func TestToolsMetaJSONRepoParamsMatchFilteredTools(t *testing.T) { - tests := []struct { - name string - safeOutputs *SafeOutputsConfig - toolName string - expectRepoParam bool - expectWildcard bool - }{ - { - name: "create_issue with allowed-repos", - safeOutputs: &SafeOutputsConfig{ - CreateIssues: &CreateIssuesConfig{ - BaseSafeOutputConfig: BaseSafeOutputConfig{Max: strPtr("1")}, - TargetRepoSlug: "org/default-repo", - AllowedRepos: []string{"org/other-repo"}, - }, - }, - toolName: "create_issue", - expectRepoParam: true, - }, - { - name: "update_issue with wildcard target-repo", - safeOutputs: &SafeOutputsConfig{ - UpdateIssues: &UpdateIssuesConfig{ - UpdateEntityConfig: UpdateEntityConfig{ - SafeOutputTargetConfig: SafeOutputTargetConfig{ - TargetRepoSlug: "*", - }, - }, - }, - }, - toolName: "update_issue", - expectRepoParam: true, - expectWildcard: true, - }, - { - name: "create_issue without allowed-repos", - safeOutputs: &SafeOutputsConfig{ - CreateIssues: &CreateIssuesConfig{ - BaseSafeOutputConfig: BaseSafeOutputConfig{Max: strPtr("1")}, - TargetRepoSlug: "org/target", - }, - }, - toolName: "create_issue", - expectRepoParam: false, - }, - } - - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - data := &WorkflowData{SafeOutputs: tt.safeOutputs} - - // Old approach: get repo param from embedded tools.json - filteredJSON, err := generateFilteredToolsJSON(data, ".github/workflows/test.md") - require.NoError(t, err, "generateFilteredToolsJSON should not error") - - var filteredTools []map[string]any - require.NoError(t, json.Unmarshal([]byte(filteredJSON), &filteredTools), "filtered JSON should parse") - - var oldTool map[string]any - for _, tool := range filteredTools { - if tool["name"] == tt.toolName { - oldTool = tool - break - } - } - require.NotNil(t, oldTool, "old approach should include tool %s", tt.toolName) - - oldInputSchema, _ := oldTool["inputSchema"].(map[string]any) - oldProperties, _ := oldInputSchema["properties"].(map[string]any) - oldRepoParam, oldHasRepo := oldProperties["repo"] - - // New approach - metaJSON, err := generateToolsMetaJSON(data, ".github/workflows/test.md") - require.NoError(t, err, "generateToolsMetaJSON should not error") - - var meta ToolsMeta - require.NoError(t, json.Unmarshal([]byte(metaJSON), &meta), "meta JSON should parse") - - newRepoParam, newHasRepo := meta.RepoParams[tt.toolName] - - // Both old and new approaches should agree on whether repo param is present - assert.Equal(t, oldHasRepo, newHasRepo, - "old and new approaches should agree on repo param presence for %s", tt.toolName) - - if tt.expectRepoParam { - require.True(t, newHasRepo, "tools_meta should include repo_param for %s", tt.toolName) - - // Description should be consistent - oldRepoMap, _ := oldRepoParam.(map[string]any) - oldRepoDesc, _ := oldRepoMap["description"].(string) - newRepoDesc, _ := newRepoParam["description"].(string) - assert.Equal(t, oldRepoDesc, newRepoDesc, "repo param description should match between old and new") - - if tt.expectWildcard { - assert.Contains(t, newRepoDesc, "Any repository can be targeted", - "wildcard repo param description should mention any repo") - } - } else { - assert.False(t, newHasRepo, "tools_meta should NOT include repo_param for %s", tt.toolName) - } - }) - } -} - -// TestToolsMetaJSONDynamicToolsFromCustomJobs verifies that custom safe-job tool -// definitions are placed in dynamic_tools in tools_meta.json. -func TestToolsMetaJSONDynamicToolsFromCustomJobs(t *testing.T) { - data := &WorkflowData{ - SafeOutputs: &SafeOutputsConfig{ - Jobs: map[string]*SafeJobConfig{ - "deploy_app": { - Description: "Deploy the application", - Inputs: map[string]*InputDefinition{ - "env": { - Type: "choice", - Options: []string{"staging", "production"}, - Description: "Target environment", - Required: true, - }, - }, - }, - }, - }, - } - - // Old approach produces a single custom-job tool in filtered JSON - filteredJSON, err := generateFilteredToolsJSON(data, ".github/workflows/test.md") - require.NoError(t, err, "generateFilteredToolsJSON should not error") - var filteredTools []map[string]any - require.NoError(t, json.Unmarshal([]byte(filteredJSON), &filteredTools), "filtered JSON should parse") - require.Len(t, filteredTools, 1, "old approach should have exactly 1 tool") - assert.Equal(t, "deploy_app", filteredTools[0]["name"], "tool should be named deploy_app") - - // New approach places it in dynamic_tools - metaJSON, err := generateToolsMetaJSON(data, ".github/workflows/test.md") - require.NoError(t, err, "generateToolsMetaJSON should not error") - var meta ToolsMeta - require.NoError(t, json.Unmarshal([]byte(metaJSON), &meta), "meta JSON should parse") - - require.Len(t, meta.DynamicTools, 1, "tools_meta should have exactly 1 dynamic tool") - dynTool := meta.DynamicTools[0] - assert.Equal(t, "deploy_app", dynTool["name"], "dynamic tool name should match") - assert.Equal(t, "Deploy the application", dynTool["description"], "dynamic tool description should match") - - inputSchema, ok := dynTool["inputSchema"].(map[string]any) - require.True(t, ok, "dynamic tool should have inputSchema") - properties, ok := inputSchema["properties"].(map[string]any) - require.True(t, ok, "inputSchema should have properties") - envProp, ok := properties["env"].(map[string]any) - require.True(t, ok, "env property should exist") - assert.Equal(t, "string", envProp["type"], "choice type should be mapped to string in JSON Schema") - assert.Equal(t, []any{"staging", "production"}, envProp["enum"], "enum values should match options") -} - -// TestToolsMetaJSONCompiledWorkflowEmbedsMeta verifies end-to-end that a compiled lock file -// contains tools_meta.json (not an inlined tools.json) with the right description constraints. func TestToolsMetaJSONCompiledWorkflowEmbedsMeta(t *testing.T) { tmpDir := testutil.TempDir(t, "tools-meta-compiled-test") @@ -393,38 +78,6 @@ Test workflow to verify tools_meta in compiled output. // TestToolsMetaJSONPushRepoMemory verifies that push_repo_memory appears in // description_suffixes when repo-memory is configured. -func TestToolsMetaJSONPushRepoMemory(t *testing.T) { - data := &WorkflowData{ - SafeOutputs: &SafeOutputsConfig{}, - RepoMemoryConfig: &RepoMemoryConfig{ - Memories: []RepoMemoryEntry{ - {ID: "default", MaxFileSize: 102400, MaxPatchSize: 10240, MaxFileCount: 100}, - }, - }, - } - - // Old approach includes push_repo_memory in filtered tools - filteredJSON, err := generateFilteredToolsJSON(data, ".github/workflows/test.md") - require.NoError(t, err, "generateFilteredToolsJSON should not error") - var filteredTools []map[string]any - require.NoError(t, json.Unmarshal([]byte(filteredJSON), &filteredTools), "filtered JSON should parse") - var foundOld bool - for _, tool := range filteredTools { - if tool["name"] == "push_repo_memory" { - foundOld = true - break - } - } - assert.True(t, foundOld, "old approach should include push_repo_memory when RepoMemoryConfig is set") - - // New approach: push_repo_memory is a predefined static tool, so it appears in - // computeEnabledToolNames and its description suffix (if any) goes in tools_meta. - enabledTools := computeEnabledToolNames(data) - assert.True(t, enabledTools["push_repo_memory"], "computeEnabledToolNames should include push_repo_memory") -} - -// TestToolsMetaJSONEmptyWhenNoSafeOutputs verifies that tools_meta.json is empty -// (no description_suffixes, no repo_params, no dynamic_tools) when safe-outputs is nil. func TestToolsMetaJSONEmptyWhenNoSafeOutputs(t *testing.T) { data := &WorkflowData{SafeOutputs: nil} @@ -439,117 +92,6 @@ func TestToolsMetaJSONEmptyWhenNoSafeOutputs(t *testing.T) { assert.Empty(t, meta.DynamicTools, "dynamic_tools should be empty when no safe-outputs") } -// TestToolsMetaJSONCustomDescriptionsAllToolTypes exercises all tool types that have -// constraint-bearing configurations to ensure no tool type regresses silently. -func TestToolsMetaJSONCustomDescriptionsAllToolTypes(t *testing.T) { - tests := []struct { - name string - safeOutputs *SafeOutputsConfig - toolName string - wantContains string - }{ - { - name: "create_discussion with max and category", - safeOutputs: &SafeOutputsConfig{ - CreateDiscussions: &CreateDiscussionsConfig{ - BaseSafeOutputConfig: BaseSafeOutputConfig{Max: strPtr("2")}, - Category: "announcements", - }, - }, - toolName: "create_discussion", - wantContains: `Discussions will be created in category "announcements"`, - }, - { - name: "close_issue with max and required prefix", - safeOutputs: &SafeOutputsConfig{ - CloseIssues: &CloseIssuesConfig{ - BaseSafeOutputConfig: BaseSafeOutputConfig{Max: strPtr("5")}, - SafeOutputFilterConfig: SafeOutputFilterConfig{ - RequiredTitlePrefix: "[bot] ", - }, - }, - }, - toolName: "close_issue", - wantContains: `Only issues with title prefix "[bot] "`, - }, - { - name: "add_labels with allowed labels", - safeOutputs: &SafeOutputsConfig{ - AddLabels: &AddLabelsConfig{ - BaseSafeOutputConfig: BaseSafeOutputConfig{Max: strPtr("3")}, - Allowed: []string{"bug", "enhancement"}, - }, - }, - toolName: "add_labels", - wantContains: `Only these labels are allowed: ["bug" "enhancement"]`, - }, - { - name: "update_project with project URL", - safeOutputs: &SafeOutputsConfig{ - UpdateProjects: &UpdateProjectConfig{ - BaseSafeOutputConfig: BaseSafeOutputConfig{Max: strPtr("1")}, - Project: "https://github.com/orgs/org/projects/1", - }, - }, - toolName: "update_project", - wantContains: "https://github.com/orgs/org/projects/1", - }, - { - name: "assign_to_agent with base branch", - safeOutputs: &SafeOutputsConfig{ - AssignToAgent: &AssignToAgentConfig{ - BaseSafeOutputConfig: BaseSafeOutputConfig{Max: strPtr("1")}, - BaseBranch: "main", - SafeOutputTargetConfig: SafeOutputTargetConfig{ - TargetRepoSlug: "org/target", - }, - }, - }, - toolName: "assign_to_agent", - wantContains: `Pull requests will target the "main" branch`, - }, - } - - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - data := &WorkflowData{SafeOutputs: tt.safeOutputs} - - metaJSON, err := generateToolsMetaJSON(data, ".github/workflows/test.md") - require.NoError(t, err, "generateToolsMetaJSON should not error") - - var meta ToolsMeta - require.NoError(t, json.Unmarshal([]byte(metaJSON), &meta), "meta JSON should parse") - - suffix, ok := meta.DescriptionSuffixes[tt.toolName] - require.True(t, ok, "tools_meta should have description_suffix for %s", tt.toolName) - assert.Contains(t, suffix, tt.wantContains, - "description suffix for %s should contain %q\n got: %q", tt.toolName, tt.wantContains, suffix) - - // Regression guard: verify generateFilteredToolsJSON (old approach) would produce - // the same constraint text in the tool's description. - filteredJSON, err := generateFilteredToolsJSON(data, ".github/workflows/test.md") - require.NoError(t, err, "generateFilteredToolsJSON should not error") - - // Parse the JSON to access the actual description string (avoids JSON escape issues) - var filteredTools []map[string]any - require.NoError(t, json.Unmarshal([]byte(filteredJSON), &filteredTools), "filtered JSON should parse") - var oldTool map[string]any - for _, tool := range filteredTools { - if tool["name"] == tt.toolName { - oldTool = tool - break - } - } - require.NotNil(t, oldTool, "old filtered tools should include %s", tt.toolName) - oldDescription, ok := oldTool["description"].(string) - require.True(t, ok, "old tool description should be a string") - assert.Contains(t, oldDescription, tt.wantContains, - "old filtered JSON should also contain the constraint text for %s (regression guard)", tt.toolName) - }) - } -} - -// extractToolsMetaFromLockFile parses the tools_meta.json heredoc embedded in a compiled lock file. func extractToolsMetaFromLockFile(t *testing.T, yamlStr string) ToolsMeta { t.Helper() @@ -584,3 +126,5 @@ func extractToolsMetaFromLockFile(t *testing.T, yamlStr string) ToolsMeta { "tools_meta.json from lock file should be valid JSON") return meta } + +// constraint-bearing configurations to ensure no tool type regresses silently. diff --git a/pkg/workflow/safe_outputs_tools_schema_test.go b/pkg/workflow/safe_outputs_tools_schema_test.go deleted file mode 100644 index 80cf6d05a0..0000000000 --- a/pkg/workflow/safe_outputs_tools_schema_test.go +++ /dev/null @@ -1,227 +0,0 @@ -//go:build !integration - -package workflow - -import ( - _ "embed" - "encoding/json" - "testing" - - "github.com/santhosh-tekuri/jsonschema/v6" - "github.com/stretchr/testify/assert" - "github.com/stretchr/testify/require" -) - -//go:embed schemas/mcp-tools.json -var mcpToolsSchema string - -func TestSafeOutputsToolsJSONCompliesWithMCPSchema(t *testing.T) { - // Get the safe outputs tools JSON - toolsJSON := GetSafeOutputsToolsJSON() - require.NotEmpty(t, toolsJSON, "Tools JSON should not be empty") - - // Compile the MCP tools schema - compiler := jsonschema.NewCompiler() - - // Parse the schema JSON - var schemaDoc any - if err := json.Unmarshal([]byte(mcpToolsSchema), &schemaDoc); err != nil { - t.Fatalf("Failed to parse MCP tools schema: %v", err) - } - - // Add the schema to the compiler - if err := compiler.AddResource("mcp-tools.json", schemaDoc); err != nil { - t.Fatalf("Failed to add MCP tools schema: %v", err) - } - - schema, err := compiler.Compile("mcp-tools.json") - require.NoError(t, err, "MCP tools schema should be valid") - - // Parse the tools JSON as a generic interface for validation - var toolsData any - err = json.Unmarshal([]byte(toolsJSON), &toolsData) - require.NoError(t, err, "Tools JSON should be valid JSON") - - // Validate the tools JSON against the schema - err = schema.Validate(toolsData) - if err != nil { - // Provide detailed error information - t.Errorf("Tools JSON does not comply with MCP schema: %v", err) - - // Parse as array for debugging - var tools []map[string]any - if err := json.Unmarshal([]byte(toolsJSON), &tools); err != nil { - t.Logf("Failed to parse tools for debugging: %v", err) - return - } - - // Print the problematic tools for debugging - t.Logf("Number of tools: %d", len(tools)) - for i, tool := range tools { - toolJSON, _ := json.MarshalIndent(tool, "", " ") - t.Logf("Tool %d:\n%s", i+1, string(toolJSON)) - } - } - - assert.NoError(t, err, "Tools JSON should comply with MCP tools schema") -} - -func TestEachToolHasRequiredMCPFields(t *testing.T) { - // Get the safe outputs tools JSON - toolsJSON := GetSafeOutputsToolsJSON() - require.NotEmpty(t, toolsJSON, "Tools JSON should not be empty") - - // Parse the tools JSON - var tools []map[string]any - err := json.Unmarshal([]byte(toolsJSON), &tools) - require.NoError(t, err, "Tools JSON should be valid JSON") - - // Check each tool has the required fields according to MCP spec - for i, tool := range tools { - t.Run(tool["name"].(string), func(t *testing.T) { - // Required: name - assert.Contains(t, tool, "name", "Tool %d should have 'name' field", i) - assert.IsType(t, "", tool["name"], "Tool %d 'name' should be a string", i) - assert.NotEmpty(t, tool["name"], "Tool %d 'name' should not be empty", i) - - // Optional but recommended: description - if desc, ok := tool["description"]; ok { - assert.IsType(t, "", desc, "Tool %d 'description' should be a string if present", i) - } - - // Required: inputSchema - assert.Contains(t, tool, "inputSchema", "Tool %d should have 'inputSchema' field", i) - - // Validate inputSchema structure - inputSchema, ok := tool["inputSchema"].(map[string]any) - require.True(t, ok, "Tool %d 'inputSchema' should be an object", i) - - // inputSchema must have type: "object" - assert.Contains(t, inputSchema, "type", "Tool %d inputSchema should have 'type' field", i) - assert.Equal(t, "object", inputSchema["type"], "Tool %d inputSchema type should be 'object'", i) - - // inputSchema should have properties - assert.Contains(t, inputSchema, "properties", "Tool %d inputSchema should have 'properties' field", i) - properties, ok := inputSchema["properties"].(map[string]any) - require.True(t, ok, "Tool %d inputSchema 'properties' should be an object", i) - assert.NotEmpty(t, properties, "Tool %d inputSchema 'properties' should not be empty", i) - - // If required field exists, it should be an array of strings - if required, ok := inputSchema["required"]; ok { - requiredArray, ok := required.([]any) - require.True(t, ok, "Tool %d inputSchema 'required' should be an array", i) - for _, req := range requiredArray { - assert.IsType(t, "", req, "Tool %d inputSchema 'required' items should be strings", i) - } - } - }) - } -} - -func TestToolsJSONStructureMatchesMCPSpecification(t *testing.T) { - // Get the safe outputs tools JSON - toolsJSON := GetSafeOutputsToolsJSON() - require.NotEmpty(t, toolsJSON, "Tools JSON should not be empty") - - // Parse the tools JSON - var tools []map[string]any - err := json.Unmarshal([]byte(toolsJSON), &tools) - require.NoError(t, err, "Tools JSON should be valid JSON") - - // Verify the structure matches MCP specification - for _, tool := range tools { - name := tool["name"].(string) - t.Run(name, func(t *testing.T) { - // Verify no unexpected top-level fields - allowedFields := map[string]bool{ - "name": true, - "title": true, - "description": true, - "inputSchema": true, - "outputSchema": true, - "annotations": true, - } - - for field := range tool { - assert.Contains(t, allowedFields, field, - "Tool '%s' has unexpected field '%s'. MCP tools should only have: name, title, description, inputSchema, outputSchema, annotations", - name, field) - } - - // If outputSchema exists, validate its structure - if outputSchema, ok := tool["outputSchema"]; ok { - outputSchemaObj, ok := outputSchema.(map[string]any) - require.True(t, ok, "Tool '%s' outputSchema should be an object", name) - - // outputSchema must have type: "object" - assert.Contains(t, outputSchemaObj, "type", "Tool '%s' outputSchema should have 'type' field", name) - assert.Equal(t, "object", outputSchemaObj["type"], "Tool '%s' outputSchema type should be 'object'", name) - } - - // If annotations exists, validate its structure - if annotations, ok := tool["annotations"]; ok { - annotationsObj, ok := annotations.(map[string]any) - require.True(t, ok, "Tool '%s' annotations should be an object", name) - - // Verify only allowed annotation fields - allowedAnnotations := map[string]bool{ - "title": true, - "readOnlyHint": true, - "destructiveHint": true, - "idempotentHint": true, - "openWorldHint": true, - } - - for field := range annotationsObj { - assert.Contains(t, allowedAnnotations, field, - "Tool '%s' annotations has unexpected field '%s'. Allowed fields: title, readOnlyHint, destructiveHint, idempotentHint, openWorldHint", - name, field) - } - } - }) - } -} - -// TestNoTopLevelOneOfAllOfAnyOf validates that no tools have oneOf/allOf/anyOf at the top level -// of their inputSchema, as these are not supported by Claude's API and other AI engines. -// This is a regression test for https://github.com/github/gh-aw/actions/runs/21142123455 -func TestNoTopLevelOneOfAllOfAnyOf(t *testing.T) { - // Get the safe outputs tools JSON - toolsJSON := GetSafeOutputsToolsJSON() - require.NotEmpty(t, toolsJSON, "Tools JSON should not be empty") - - // Parse the tools JSON - var tools []map[string]any - err := json.Unmarshal([]byte(toolsJSON), &tools) - require.NoError(t, err, "Tools JSON should be valid JSON") - - // Check each tool for forbidden top-level schema constraints - for _, tool := range tools { - name := tool["name"].(string) - t.Run(name, func(t *testing.T) { - // Get the inputSchema - inputSchema, ok := tool["inputSchema"].(map[string]any) - require.True(t, ok, "Tool '%s' should have inputSchema as an object", name) - - // Check for forbidden top-level constraints - // These constraints are not supported by Claude's API and should be avoided - assert.NotContains(t, inputSchema, "oneOf", - "Tool '%s' has 'oneOf' at top level of inputSchema. "+ - "Claude API does not support oneOf/allOf/anyOf at the top level. "+ - "Use optional fields with validation documented in descriptions instead. "+ - "See: https://github.com/github/gh-aw/actions/runs/21142123455", name) - - assert.NotContains(t, inputSchema, "allOf", - "Tool '%s' has 'allOf' at top level of inputSchema. "+ - "Claude API does not support oneOf/allOf/anyOf at the top level. "+ - "Use optional fields with validation documented in descriptions instead. "+ - "See: https://github.com/github/gh-aw/actions/runs/21142123455", name) - - assert.NotContains(t, inputSchema, "anyOf", - "Tool '%s' has 'anyOf' at top level of inputSchema. "+ - "Claude API does not support oneOf/allOf/anyOf at the top level. "+ - "Use optional fields with validation documented in descriptions instead. "+ - "See: https://github.com/github/gh-aw/actions/runs/21142123455", name) - }) - } -} diff --git a/pkg/workflow/safe_outputs_tools_test.go b/pkg/workflow/safe_outputs_tools_test.go index 62c0607525..98eea44514 100644 --- a/pkg/workflow/safe_outputs_tools_test.go +++ b/pkg/workflow/safe_outputs_tools_test.go @@ -3,374 +3,11 @@ package workflow import ( - "encoding/json" "testing" "github.com/stretchr/testify/assert" - "github.com/stretchr/testify/require" ) -func TestGenerateFilteredToolsJSON(t *testing.T) { - tests := []struct { - name string - safeOutputs *SafeOutputsConfig - expectedTools []string - }{ - { - name: "nil safe outputs returns empty array", - safeOutputs: nil, - expectedTools: []string{}, - }, - { - name: "empty safe outputs returns empty array", - safeOutputs: &SafeOutputsConfig{}, - expectedTools: []string{}, - }, - { - name: "create issues enabled", - safeOutputs: &SafeOutputsConfig{ - CreateIssues: &CreateIssuesConfig{ - BaseSafeOutputConfig: BaseSafeOutputConfig{Max: strPtr("5")}, - }, - }, - expectedTools: []string{"create_issue"}, - }, - { - name: "create agent sessions enabled", - safeOutputs: &SafeOutputsConfig{ - CreateAgentSessions: &CreateAgentSessionConfig{ - BaseSafeOutputConfig: BaseSafeOutputConfig{Max: strPtr("3")}, - }, - }, - expectedTools: []string{"create_agent_session"}, - }, - { - name: "create discussions enabled", - safeOutputs: &SafeOutputsConfig{ - CreateDiscussions: &CreateDiscussionsConfig{ - BaseSafeOutputConfig: BaseSafeOutputConfig{Max: strPtr("2")}, - }, - }, - expectedTools: []string{"create_discussion"}, - }, - { - name: "add comments enabled", - safeOutputs: &SafeOutputsConfig{ - AddComments: &AddCommentsConfig{ - BaseSafeOutputConfig: BaseSafeOutputConfig{Max: strPtr("10")}, - }, - }, - expectedTools: []string{"add_comment"}, - }, - { - name: "create pull requests enabled", - safeOutputs: &SafeOutputsConfig{ - CreatePullRequests: &CreatePullRequestsConfig{}, - }, - expectedTools: []string{"create_pull_request"}, - }, - { - name: "create pull request review comments enabled", - safeOutputs: &SafeOutputsConfig{ - CreatePullRequestReviewComments: &CreatePullRequestReviewCommentsConfig{ - BaseSafeOutputConfig: BaseSafeOutputConfig{Max: strPtr("5")}, - }, - }, - expectedTools: []string{"create_pull_request_review_comment"}, - }, - { - name: "submit pull request review enabled", - safeOutputs: &SafeOutputsConfig{ - SubmitPullRequestReview: &SubmitPullRequestReviewConfig{ - BaseSafeOutputConfig: BaseSafeOutputConfig{Max: strPtr("1")}, - }, - }, - expectedTools: []string{"submit_pull_request_review"}, - }, - { - name: "reply to pull request review comment enabled", - safeOutputs: &SafeOutputsConfig{ - ReplyToPullRequestReviewComment: &ReplyToPullRequestReviewCommentConfig{ - BaseSafeOutputConfig: BaseSafeOutputConfig{Max: strPtr("10")}, - }, - }, - expectedTools: []string{"reply_to_pull_request_review_comment"}, - }, - { - name: "resolve pull request review thread enabled", - safeOutputs: &SafeOutputsConfig{ - ResolvePullRequestReviewThread: &ResolvePullRequestReviewThreadConfig{ - BaseSafeOutputConfig: BaseSafeOutputConfig{Max: strPtr("10")}, - }, - }, - expectedTools: []string{"resolve_pull_request_review_thread"}, - }, - { - name: "create code scanning alerts enabled", - safeOutputs: &SafeOutputsConfig{ - CreateCodeScanningAlerts: &CreateCodeScanningAlertsConfig{ - BaseSafeOutputConfig: BaseSafeOutputConfig{Max: strPtr("100")}, - }, - }, - expectedTools: []string{"create_code_scanning_alert"}, - }, - { - name: "add labels enabled", - safeOutputs: &SafeOutputsConfig{ - AddLabels: &AddLabelsConfig{ - BaseSafeOutputConfig: BaseSafeOutputConfig{Max: strPtr("5")}, - }, - }, - expectedTools: []string{"add_labels"}, - }, - { - name: "update issues enabled", - safeOutputs: &SafeOutputsConfig{ - UpdateIssues: &UpdateIssuesConfig{ - UpdateEntityConfig: UpdateEntityConfig{ - BaseSafeOutputConfig: BaseSafeOutputConfig{Max: strPtr("3")}, - }, - }, - }, - expectedTools: []string{"update_issue"}, - }, - { - name: "push to pull request branch enabled", - safeOutputs: &SafeOutputsConfig{ - PushToPullRequestBranch: &PushToPullRequestBranchConfig{ - BaseSafeOutputConfig: BaseSafeOutputConfig{Max: strPtr("1")}, - }, - }, - expectedTools: []string{"push_to_pull_request_branch"}, - }, - { - name: "upload assets enabled", - safeOutputs: &SafeOutputsConfig{ - UploadAssets: &UploadAssetsConfig{ - BaseSafeOutputConfig: BaseSafeOutputConfig{Max: strPtr("10")}, - }, - }, - expectedTools: []string{"upload_asset"}, - }, - { - name: "missing tool enabled", - safeOutputs: &SafeOutputsConfig{ - MissingTool: &MissingToolConfig{ - BaseSafeOutputConfig: BaseSafeOutputConfig{Max: strPtr("5")}, - }, - }, - expectedTools: []string{"missing_tool"}, - }, - { - name: "multiple tools enabled", - safeOutputs: &SafeOutputsConfig{ - CreateIssues: &CreateIssuesConfig{BaseSafeOutputConfig: BaseSafeOutputConfig{Max: strPtr("5")}}, - AddComments: &AddCommentsConfig{BaseSafeOutputConfig: BaseSafeOutputConfig{Max: strPtr("10")}}, - AddLabels: &AddLabelsConfig{BaseSafeOutputConfig: BaseSafeOutputConfig{Max: strPtr("3")}}, - }, - expectedTools: []string{"create_issue", "add_comment", "add_labels"}, - }, - { - name: "all tools enabled", - safeOutputs: &SafeOutputsConfig{ - CreateIssues: &CreateIssuesConfig{BaseSafeOutputConfig: BaseSafeOutputConfig{Max: strPtr("5")}}, - CreateAgentSessions: &CreateAgentSessionConfig{BaseSafeOutputConfig: BaseSafeOutputConfig{Max: strPtr("3")}}, - CreateDiscussions: &CreateDiscussionsConfig{BaseSafeOutputConfig: BaseSafeOutputConfig{Max: strPtr("2")}}, - AddComments: &AddCommentsConfig{BaseSafeOutputConfig: BaseSafeOutputConfig{Max: strPtr("10")}}, - CreatePullRequests: &CreatePullRequestsConfig{}, - CreatePullRequestReviewComments: &CreatePullRequestReviewCommentsConfig{BaseSafeOutputConfig: BaseSafeOutputConfig{Max: strPtr("5")}}, - SubmitPullRequestReview: &SubmitPullRequestReviewConfig{BaseSafeOutputConfig: BaseSafeOutputConfig{Max: strPtr("1")}}, - ReplyToPullRequestReviewComment: &ReplyToPullRequestReviewCommentConfig{BaseSafeOutputConfig: BaseSafeOutputConfig{Max: strPtr("10")}}, - ResolvePullRequestReviewThread: &ResolvePullRequestReviewThreadConfig{BaseSafeOutputConfig: BaseSafeOutputConfig{Max: strPtr("10")}}, - CreateCodeScanningAlerts: &CreateCodeScanningAlertsConfig{BaseSafeOutputConfig: BaseSafeOutputConfig{Max: strPtr("100")}}, - AddLabels: &AddLabelsConfig{BaseSafeOutputConfig: BaseSafeOutputConfig{Max: strPtr("3")}}, - AddReviewer: &AddReviewerConfig{BaseSafeOutputConfig: BaseSafeOutputConfig{Max: strPtr("3")}}, - UpdateIssues: &UpdateIssuesConfig{UpdateEntityConfig: UpdateEntityConfig{BaseSafeOutputConfig: BaseSafeOutputConfig{Max: strPtr("3")}}}, - PushToPullRequestBranch: &PushToPullRequestBranchConfig{BaseSafeOutputConfig: BaseSafeOutputConfig{Max: strPtr("1")}}, - UploadAssets: &UploadAssetsConfig{BaseSafeOutputConfig: BaseSafeOutputConfig{Max: strPtr("10")}}, - MissingTool: &MissingToolConfig{BaseSafeOutputConfig: BaseSafeOutputConfig{Max: strPtr("5")}}, - }, - expectedTools: []string{ - "create_issue", - "create_agent_session", - "create_discussion", - "add_comment", - "create_pull_request", - "create_pull_request_review_comment", - "submit_pull_request_review", - "reply_to_pull_request_review_comment", - "resolve_pull_request_review_thread", - "create_code_scanning_alert", - "add_labels", - "add_reviewer", - "update_issue", - "push_to_pull_request_branch", - "upload_asset", - "missing_tool", - }, - }, - } - - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - workflowData := &WorkflowData{ - SafeOutputs: tt.safeOutputs, - } - - result, err := generateFilteredToolsJSON(workflowData, ".github/workflows/test-workflow.md") - require.NoError(t, err, "generateFilteredToolsJSON should not error") - - // Parse the JSON result - var tools []map[string]any - err = json.Unmarshal([]byte(result), &tools) - require.NoError(t, err, "Result should be valid JSON") - - // Extract tool names from the result - var actualTools []string - for _, tool := range tools { - if name, ok := tool["name"].(string); ok { - actualTools = append(actualTools, name) - } - } - - // Check that the expected tools are present - assert.ElementsMatch(t, tt.expectedTools, actualTools, "Tool names should match") - - // Verify each tool has required fields - for _, tool := range tools { - assert.Contains(t, tool, "name", "Tool should have name field") - assert.Contains(t, tool, "description", "Tool should have description field") - assert.Contains(t, tool, "inputSchema", "Tool should have inputSchema field") - } - }) - } -} - -func TestGenerateFilteredToolsJSONValidStructure(t *testing.T) { - workflowData := &WorkflowData{ - SafeOutputs: &SafeOutputsConfig{ - CreateIssues: &CreateIssuesConfig{BaseSafeOutputConfig: BaseSafeOutputConfig{Max: strPtr("5")}}, - AddComments: &AddCommentsConfig{BaseSafeOutputConfig: BaseSafeOutputConfig{Max: strPtr("10")}}, - }, - } - - result, err := generateFilteredToolsJSON(workflowData, ".github/workflows/test-workflow.md") - require.NoError(t, err) - - // Parse the JSON result - var tools []map[string]any - err = json.Unmarshal([]byte(result), &tools) - require.NoError(t, err) - - // Verify create_issue tool structure - var createIssueTool map[string]any - for _, tool := range tools { - if tool["name"] == "create_issue" { - createIssueTool = tool - break - } - } - require.NotNil(t, createIssueTool, "create_issue tool should be present") - - // Check inputSchema structure - inputSchema, ok := createIssueTool["inputSchema"].(map[string]any) - require.True(t, ok, "inputSchema should be a map") - - assert.Equal(t, "object", inputSchema["type"], "inputSchema type should be object") - - properties, ok := inputSchema["properties"].(map[string]any) - require.True(t, ok, "properties should be a map") - - // Verify required properties exist - assert.Contains(t, properties, "title", "Should have title property") - assert.Contains(t, properties, "body", "Should have body property") - - // Verify required field - required, ok := inputSchema["required"].([]any) - require.True(t, ok, "required should be an array") - assert.Contains(t, required, "title", "title should be required") - assert.Contains(t, required, "body", "body should be required") -} - -func TestGetSafeOutputsToolsJSON(t *testing.T) { - // Test that the embedded JSON can be retrieved and parsed - toolsJSON := GetSafeOutputsToolsJSON() - require.NotEmpty(t, toolsJSON, "Tools JSON should not be empty") - - // Parse the JSON to ensure it's valid - var tools []map[string]any - err := json.Unmarshal([]byte(toolsJSON), &tools) - require.NoError(t, err, "Tools JSON should be valid") - require.NotEmpty(t, tools, "Tools array should not be empty") - - // Verify all expected tools are present - expectedTools := []string{ - "create_issue", - "create_agent_session", - "create_discussion", - "update_discussion", - "close_discussion", - "close_issue", - "close_pull_request", - "mark_pull_request_as_ready_for_review", - "add_comment", - "create_pull_request", - "create_pull_request_review_comment", - "submit_pull_request_review", - "reply_to_pull_request_review_comment", - "resolve_pull_request_review_thread", - "create_code_scanning_alert", - "add_labels", - "remove_labels", - "add_reviewer", - "assign_milestone", - "assign_to_agent", - "assign_to_user", - "unassign_from_user", - "update_issue", - "update_pull_request", - "push_to_pull_request_branch", - "upload_asset", - "update_release", - "link_sub_issue", - "hide_comment", - "set_issue_type", - "update_project", - "create_project", - "create_project_status_update", - "autofix_code_scanning_alert", - "push_repo_memory", - "missing_tool", - "missing_data", - "noop", - } - - var actualTools []string - for _, tool := range tools { - if name, ok := tool["name"].(string); ok { - actualTools = append(actualTools, name) - } - } - - assert.ElementsMatch(t, expectedTools, actualTools, "All expected tools should be present") - - // Verify each tool has the required structure - for _, tool := range tools { - name := tool["name"].(string) - t.Run("tool_"+name, func(t *testing.T) { - assert.Contains(t, tool, "name", "Tool should have name") - assert.Contains(t, tool, "description", "Tool should have description") - assert.Contains(t, tool, "inputSchema", "Tool should have inputSchema") - - // Verify inputSchema structure - inputSchema, ok := tool["inputSchema"].(map[string]any) - require.True(t, ok, "inputSchema should be a map") - assert.Equal(t, "object", inputSchema["type"], "inputSchema type should be object") - assert.Contains(t, inputSchema, "properties", "inputSchema should have properties") - }) - } -} - func TestEnhanceToolDescription(t *testing.T) { tests := []struct { name string @@ -737,419 +374,3 @@ func TestEnhanceToolDescription(t *testing.T) { }) } } - -func TestGenerateFilteredToolsJSONWithEnhancedDescriptions(t *testing.T) { - workflowData := &WorkflowData{ - SafeOutputs: &SafeOutputsConfig{ - CreateIssues: &CreateIssuesConfig{ - BaseSafeOutputConfig: BaseSafeOutputConfig{Max: strPtr("5")}, - TitlePrefix: "[automated] ", - Labels: []string{"bot", "enhancement"}, - }, - AddLabels: &AddLabelsConfig{ - BaseSafeOutputConfig: BaseSafeOutputConfig{Max: strPtr("3")}, - Allowed: []string{"bug", "enhancement"}, - }, - }, - } - - result, err := generateFilteredToolsJSON(workflowData, ".github/workflows/test-workflow.md") - require.NoError(t, err) - - // Parse the JSON result - var tools []map[string]any - err = json.Unmarshal([]byte(result), &tools) - require.NoError(t, err) - - // Find and verify create_issue tool has enhanced description - var createIssueTool map[string]any - for _, tool := range tools { - if tool["name"] == "create_issue" { - createIssueTool = tool - break - } - } - require.NotNil(t, createIssueTool, "create_issue tool should be present") - - description, ok := createIssueTool["description"].(string) - require.True(t, ok, "description should be a string") - assert.Contains(t, description, "CONSTRAINTS:", "Description should contain constraints") - assert.Contains(t, description, "Maximum 5 issue(s)", "Description should include max constraint") - assert.Contains(t, description, `Title will be prefixed with "[automated] "`, "Description should include title prefix") - assert.Contains(t, description, `Labels ["bot" "enhancement"]`, "Description should include labels") - - // Find and verify add_labels tool has enhanced description - var addLabelsTool map[string]any - for _, tool := range tools { - if tool["name"] == "add_labels" { - addLabelsTool = tool - break - } - } - require.NotNil(t, addLabelsTool, "add_labels tool should be present") - - labelsDescription, ok := addLabelsTool["description"].(string) - require.True(t, ok, "description should be a string") - assert.Contains(t, labelsDescription, "CONSTRAINTS:", "Description should contain constraints") - assert.Contains(t, labelsDescription, `Only these labels are allowed: ["bug" "enhancement"]`, "Description should include allowed labels") -} - -func TestRepoParameterAddedOnlyWithAllowedRepos(t *testing.T) { - tests := []struct { - name string - safeOutputs *SafeOutputsConfig - toolName string - expectRepo bool - expectRepoDesc string - }{ - { - name: "create_issue without allowed-repos should not have repo parameter", - safeOutputs: &SafeOutputsConfig{ - CreateIssues: &CreateIssuesConfig{ - BaseSafeOutputConfig: BaseSafeOutputConfig{Max: strPtr("1")}, - TargetRepoSlug: "org/target-repo", - }, - }, - toolName: "create_issue", - expectRepo: false, - }, - { - name: "create_issue with allowed-repos should have repo parameter", - safeOutputs: &SafeOutputsConfig{ - CreateIssues: &CreateIssuesConfig{ - BaseSafeOutputConfig: BaseSafeOutputConfig{Max: strPtr("1")}, - TargetRepoSlug: "org/target-repo", - AllowedRepos: []string{"org/other-repo"}, - }, - }, - toolName: "create_issue", - expectRepo: true, - expectRepoDesc: "org/target-repo", - }, - { - name: "add_comment with allowed-repos should have repo parameter", - safeOutputs: &SafeOutputsConfig{ - AddComments: &AddCommentsConfig{ - BaseSafeOutputConfig: BaseSafeOutputConfig{Max: strPtr("5")}, - AllowedRepos: []string{"org/repo-a", "org/repo-b"}, - }, - }, - toolName: "add_comment", - expectRepo: true, - }, - { - name: "create_pull_request always has repo parameter (defined in base schema)", - safeOutputs: &SafeOutputsConfig{ - CreatePullRequests: &CreatePullRequestsConfig{ - BaseSafeOutputConfig: BaseSafeOutputConfig{Max: strPtr("1")}, - }, - }, - toolName: "create_pull_request", - expectRepo: true, - }, - { - name: "create_pull_request with allowed-repos has repo parameter with enhanced description", - safeOutputs: &SafeOutputsConfig{ - CreatePullRequests: &CreatePullRequestsConfig{ - BaseSafeOutputConfig: BaseSafeOutputConfig{Max: strPtr("1")}, - AllowedRepos: []string{"org/repo-c"}, - }, - }, - toolName: "create_pull_request", - expectRepo: true, - }, - } - - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - workflowData := &WorkflowData{ - SafeOutputs: tt.safeOutputs, - } - - result, err := generateFilteredToolsJSON(workflowData, ".github/workflows/test-workflow.md") - require.NoError(t, err) - - // Parse the JSON result - var tools []map[string]any - err = json.Unmarshal([]byte(result), &tools) - require.NoError(t, err) - - // Find the tool - var targetTool map[string]any - for _, tool := range tools { - if tool["name"] == tt.toolName { - targetTool = tool - break - } - } - require.NotNil(t, targetTool, "%s tool should be present", tt.toolName) - - // Check inputSchema - inputSchema, ok := targetTool["inputSchema"].(map[string]any) - require.True(t, ok, "inputSchema should exist") - - properties, ok := inputSchema["properties"].(map[string]any) - require.True(t, ok, "properties should exist") - - // Check if repo parameter exists - repoParam, hasRepo := properties["repo"] - if tt.expectRepo { - assert.True(t, hasRepo, "Tool %s should have repo parameter when allowed-repos is configured", tt.toolName) - if hasRepo { - repoMap, ok := repoParam.(map[string]any) - require.True(t, ok, "repo parameter should be a map") - assert.Equal(t, "string", repoMap["type"], "repo type should be string") - - description, ok := repoMap["description"].(string) - require.True(t, ok, "repo description should be a string") - assert.Contains(t, description, "Target repository", "repo description should mention target repository") - assert.Contains(t, description, "allowed-repos", "repo description should mention allowed-repos") - - if tt.expectRepoDesc != "" { - assert.Contains(t, description, tt.expectRepoDesc, "repo description should include target-repo value") - } - } - } else { - assert.False(t, hasRepo, "Tool %s should not have repo parameter when allowed-repos is not configured", tt.toolName) - } - }) - } -} - -// TestFilterUpdateDiscussionSchemaFields verifies that filterToolSchemaFields correctly -// removes fields from the update_discussion tool schema based on what is enabled in config. -func TestFilterUpdateDiscussionSchemaFields(t *testing.T) { - boolPtr := func(b bool) *bool { return &b } - - makeToolWithAllFields := func() map[string]any { - return map[string]any{ - "name": "update_discussion", - "inputSchema": map[string]any{ - "type": "object", - "properties": map[string]any{ - "title": map[string]any{"type": "string"}, - "body": map[string]any{"type": "string"}, - "labels": map[string]any{"type": "array"}, - "discussion_number": map[string]any{"type": "number"}, - "secrecy": map[string]any{"type": "string"}, - "integrity": map[string]any{"type": "string"}, - }, - }, - } - } - - tests := []struct { - name string - safeOutputs *SafeOutputsConfig - expectTitle bool - expectBody bool - expectLabels bool - expectDiscNum bool - expectSecrecy bool - expectIntegrity bool - }{ - { - name: "labels only - title and body removed", - safeOutputs: &SafeOutputsConfig{ - UpdateDiscussions: &UpdateDiscussionsConfig{ - Labels: boolPtr(true), - AllowedLabels: []string{"Label1", "Label2"}, - }, - }, - expectTitle: false, - expectBody: false, - expectLabels: true, - expectDiscNum: true, - expectSecrecy: true, - expectIntegrity: true, - }, - { - name: "title and body only - labels removed", - safeOutputs: &SafeOutputsConfig{ - UpdateDiscussions: &UpdateDiscussionsConfig{ - Title: boolPtr(true), - Body: boolPtr(true), - }, - }, - expectTitle: true, - expectBody: true, - expectLabels: false, - expectDiscNum: true, - expectSecrecy: true, - expectIntegrity: true, - }, - { - name: "all fields enabled", - safeOutputs: &SafeOutputsConfig{ - UpdateDiscussions: &UpdateDiscussionsConfig{ - Title: boolPtr(true), - Body: boolPtr(true), - Labels: boolPtr(true), - }, - }, - expectTitle: true, - expectBody: true, - expectLabels: true, - expectDiscNum: true, - expectSecrecy: true, - expectIntegrity: true, - }, - { - name: "no fields enabled - all content fields removed", - safeOutputs: &SafeOutputsConfig{ - UpdateDiscussions: &UpdateDiscussionsConfig{}, - }, - expectTitle: false, - expectBody: false, - expectLabels: false, - expectDiscNum: true, - expectSecrecy: true, - expectIntegrity: true, - }, - { - name: "nil safe outputs - no changes", - safeOutputs: nil, - expectTitle: true, - expectBody: true, - expectLabels: true, - expectDiscNum: true, - expectSecrecy: true, - expectIntegrity: true, - }, - { - name: "no update_discussion config - no changes", - safeOutputs: &SafeOutputsConfig{ - CreateIssues: &CreateIssuesConfig{}, - }, - expectTitle: true, - expectBody: true, - expectLabels: true, - expectDiscNum: true, - expectSecrecy: true, - expectIntegrity: true, - }, - } - - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - tool := makeToolWithAllFields() - filterToolSchemaFields(tool, "update_discussion", tt.safeOutputs) - - inputSchema, ok := tool["inputSchema"].(map[string]any) - require.True(t, ok, "inputSchema should be present") - - properties, ok := inputSchema["properties"].(map[string]any) - require.True(t, ok, "properties should be present") - - _, hasTitle := properties["title"] - _, hasBody := properties["body"] - _, hasLabels := properties["labels"] - _, hasDiscNum := properties["discussion_number"] - _, hasSecrecy := properties["secrecy"] - _, hasIntegrity := properties["integrity"] - - assert.Equal(t, tt.expectTitle, hasTitle, "title field presence mismatch") - assert.Equal(t, tt.expectBody, hasBody, "body field presence mismatch") - assert.Equal(t, tt.expectLabels, hasLabels, "labels field presence mismatch") - assert.Equal(t, tt.expectDiscNum, hasDiscNum, "discussion_number field should always be present") - assert.Equal(t, tt.expectSecrecy, hasSecrecy, "secrecy field should always be present") - assert.Equal(t, tt.expectIntegrity, hasIntegrity, "integrity field should always be present") - }) - } -} - -// TestFilterUpdateDiscussionSchemaFieldsViaGenerateFiltered verifies that the end-to-end -// generateFilteredToolsJSON correctly filters update_discussion fields based on config. -func TestFilterUpdateDiscussionSchemaFieldsViaGenerateFiltered(t *testing.T) { - boolPtr := func(b bool) *bool { return &b } - - tests := []struct { - name string - safeOutputs *SafeOutputsConfig - expectTitle bool - expectBody bool - expectLabels bool - }{ - { - name: "only allowed-labels configured removes title and body from schema", - safeOutputs: &SafeOutputsConfig{ - UpdateDiscussions: &UpdateDiscussionsConfig{ - UpdateEntityConfig: UpdateEntityConfig{ - SafeOutputTargetConfig: SafeOutputTargetConfig{Target: "*"}, - }, - Labels: boolPtr(true), - AllowedLabels: []string{"Label1", "Label2"}, - }, - }, - expectTitle: false, - expectBody: false, - expectLabels: true, - }, - { - name: "title and body configured removes labels from schema", - safeOutputs: &SafeOutputsConfig{ - UpdateDiscussions: &UpdateDiscussionsConfig{ - Title: boolPtr(true), - Body: boolPtr(true), - }, - }, - expectTitle: true, - expectBody: true, - expectLabels: false, - }, - { - name: "all fields configured", - safeOutputs: &SafeOutputsConfig{ - UpdateDiscussions: &UpdateDiscussionsConfig{ - Title: boolPtr(true), - Body: boolPtr(true), - Labels: boolPtr(true), - }, - }, - expectTitle: true, - expectBody: true, - expectLabels: true, - }, - } - - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - data := &WorkflowData{ - SafeOutputs: tt.safeOutputs, - } - - toolsJSON, err := generateFilteredToolsJSON(data, "") - require.NoError(t, err, "generateFilteredToolsJSON should not error") - - var tools []map[string]any - err = json.Unmarshal([]byte(toolsJSON), &tools) - require.NoError(t, err, "tools JSON should be valid") - - // Find update_discussion tool - var discTool map[string]any - for _, tool := range tools { - if tool["name"] == "update_discussion" { - discTool = tool - break - } - } - require.NotNil(t, discTool, "update_discussion tool should be present") - - inputSchema, ok := discTool["inputSchema"].(map[string]any) - require.True(t, ok, "inputSchema should be present") - - properties, ok := inputSchema["properties"].(map[string]any) - require.True(t, ok, "properties should be present") - - _, hasTitle := properties["title"] - _, hasBody := properties["body"] - _, hasLabels := properties["labels"] - _, hasDiscNum := properties["discussion_number"] - - assert.Equal(t, tt.expectTitle, hasTitle, "title field presence mismatch") - assert.Equal(t, tt.expectBody, hasBody, "body field presence mismatch") - assert.Equal(t, tt.expectLabels, hasLabels, "labels field presence mismatch") - assert.True(t, hasDiscNum, "discussion_number should always be present") - }) - } -} diff --git a/pkg/workflow/safe_outputs_tools_update_issue_test.go b/pkg/workflow/safe_outputs_tools_update_issue_test.go deleted file mode 100644 index 684cd0b92c..0000000000 --- a/pkg/workflow/safe_outputs_tools_update_issue_test.go +++ /dev/null @@ -1,49 +0,0 @@ -//go:build !integration - -package workflow - -import ( - "encoding/json" - "testing" - - "github.com/stretchr/testify/assert" - "github.com/stretchr/testify/require" -) - -func TestUpdateIssueToolSupportsBodyOperationsAndMetadata(t *testing.T) { - toolsJSON := GetSafeOutputsToolsJSON() - require.NotEmpty(t, toolsJSON, "Tools JSON should not be empty") - - var tools []map[string]any - err := json.Unmarshal([]byte(toolsJSON), &tools) - require.NoError(t, err, "Tools JSON should be valid") - - var updateIssueTool map[string]any - for _, tool := range tools { - if tool["name"] == "update_issue" { - updateIssueTool = tool - break - } - } - require.NotNil(t, updateIssueTool, "update_issue tool should be present") - - inputSchema, ok := updateIssueTool["inputSchema"].(map[string]any) - require.True(t, ok, "inputSchema should be an object") - - properties, ok := inputSchema["properties"].(map[string]any) - require.True(t, ok, "properties should be an object") - - // These are required for a great campaign-generator UX: append-by-default, plus metadata updates. - assert.Contains(t, properties, "operation", "update_issue should support body operations") - assert.Contains(t, properties, "labels", "update_issue should support labels") - assert.Contains(t, properties, "assignees", "update_issue should support assignees") - assert.Contains(t, properties, "milestone", "update_issue should support milestone") - - body, ok := properties["body"].(map[string]any) - require.True(t, ok, "body schema should be an object") - bodyDesc, ok := body["description"].(string) - require.True(t, ok, "body.description should be a string") - assert.Contains(t, bodyDesc, "append", "body description should document append") - assert.Contains(t, bodyDesc, "prepend", "body description should document prepend") - assert.Contains(t, bodyDesc, "replace-island", "body description should document replace-island") -} diff --git a/pkg/workflow/safe_scripts_test.go b/pkg/workflow/safe_scripts_test.go index 92d13a833f..e01ef51dfe 100644 --- a/pkg/workflow/safe_scripts_test.go +++ b/pkg/workflow/safe_scripts_test.go @@ -174,52 +174,6 @@ func TestGenerateCustomScriptToolDefinition(t *testing.T) { } // TestScriptToolsInFilteredJSON verifies scripts appear in the filtered tools JSON -func TestScriptToolsInFilteredJSON(t *testing.T) { - workflowData := &WorkflowData{ - SafeOutputs: &SafeOutputsConfig{ - Scripts: map[string]*SafeScriptConfig{ - "my-custom-handler": { - Description: "A custom script handler", - Inputs: map[string]*InputDefinition{ - "target": { - Description: "Target to process", - Required: true, - Type: "string", - }, - }, - Script: "return async (m) => ({ success: true });", - }, - }, - }, - } - - toolsJSON, err := generateFilteredToolsJSON(workflowData, ".github/workflows/test.md") - require.NoError(t, err, "Should generate tools JSON without error") - - var tools []map[string]any - err = json.Unmarshal([]byte(toolsJSON), &tools) - require.NoError(t, err, "Tools JSON should be parseable") - - var customTool map[string]any - for _, tool := range tools { - if name, ok := tool["name"].(string); ok && name == "my_custom_handler" { - customTool = tool - break - } - } - require.NotNil(t, customTool, "Should find my_custom_handler tool in tools JSON") - assert.Equal(t, "A custom script handler", customTool["description"], "Description should match") - - inputSchema, ok := customTool["inputSchema"].(map[string]any) - require.True(t, ok, "Should have inputSchema") - - properties, ok := inputSchema["properties"].(map[string]any) - require.True(t, ok, "Should have properties") - assert.Contains(t, properties, "target", "Should have target property") -} - -// TestGenerateSafeOutputScriptContent verifies that the handler body is wrapped with config -// destructuring and a handler function — users write only the handler body. func TestGenerateSafeOutputScriptContent(t *testing.T) { scriptConfig := &SafeScriptConfig{ Script: "core.info(`Channel: ${item.channel}`); return { success: true };",