diff --git a/.github/workflows/smoke-codex.lock.yml b/.github/workflows/smoke-codex.lock.yml index 574f3270e04..219a9a4326f 100644 --- a/.github/workflows/smoke-codex.lock.yml +++ b/.github/workflows/smoke-codex.lock.yml @@ -537,7 +537,7 @@ jobs: mkdir -p /tmp/gh-aw/safeoutputs mkdir -p /tmp/gh-aw/mcp-logs/safeoutputs cat > "${RUNNER_TEMP}/gh-aw/safeoutputs/config.json" << 'GH_AW_SAFE_OUTPUTS_CONFIG_7fd0a6e0f1cb4a80_EOF' - {"add_comment":{"hide_older_comments":true,"max":2},"add_labels":{"allowed":["smoke-codex"]},"create_issue":{"close_older_issues":true,"close_older_key":"smoke-codex","expires":2,"labels":["automation","testing"],"max":1},"create_report_incomplete_issue":{},"hide_comment":{"max":5},"missing_data":{},"missing_tool":{},"noop":{"max":1,"report-as-issue":"true"},"remove_labels":{"allowed":["smoke"]},"report_incomplete":{},"unassign_from_user":{"allowed":["githubactionagent"],"max":1}} + {"add_comment":{"hide_older_comments":true,"max":2},"add_labels":{"allowed":["smoke-codex"]},"add_smoked_label":true,"create_issue":{"close_older_issues":true,"close_older_key":"smoke-codex","expires":2,"labels":["automation","testing"],"max":1},"create_report_incomplete_issue":{},"hide_comment":{"max":5},"missing_data":{},"missing_tool":{},"noop":{"max":1,"report-as-issue":"true"},"remove_labels":{"allowed":["smoke"]},"report_incomplete":{},"unassign_from_user":{"allowed":["githubactionagent"],"max":1}} GH_AW_SAFE_OUTPUTS_CONFIG_7fd0a6e0f1cb4a80_EOF - name: Write Safe Outputs Tools env: diff --git a/pkg/workflow/compile_outputs_allowed_labels_test.go b/pkg/workflow/compile_outputs_allowed_labels_test.go index 9771f29e3d7..fbdd3da8abe 100644 --- a/pkg/workflow/compile_outputs_allowed_labels_test.go +++ b/pkg/workflow/compile_outputs_allowed_labels_test.go @@ -11,6 +11,7 @@ import ( "github.com/github/gh-aw/pkg/stringutil" "github.com/github/gh-aw/pkg/testutil" + "github.com/stretchr/testify/require" ) func TestAllowedLabelsConfigParsing(t *testing.T) { @@ -302,7 +303,8 @@ This workflow tests that allowed-labels are included in safe outputs config JSON } // Generate safe outputs config - configJSON := generateSafeOutputsConfig(workflowData) + configJSON, err := generateSafeOutputsConfig(workflowData) + require.NoError(t, err, "generateSafeOutputsConfig should not return an error") // Verify that allowed_labels is in the config if !strings.Contains(configJSON, `"allowed_labels"`) { diff --git a/pkg/workflow/dispatch_repository_test.go b/pkg/workflow/dispatch_repository_test.go index 6ac742b44ef..1b77838511c 100644 --- a/pkg/workflow/dispatch_repository_test.go +++ b/pkg/workflow/dispatch_repository_test.go @@ -478,11 +478,12 @@ func TestDispatchRepositoryConfigSerialization(t *testing.T) { }, } - configJSON := generateSafeOutputsConfig(workflowData) + configJSON, err := generateSafeOutputsConfig(workflowData) + require.NoError(t, err, "generateSafeOutputsConfig should not return an error") require.NotEmpty(t, configJSON, "Config JSON should not be empty") var config map[string]any - err := json.Unmarshal([]byte(configJSON), &config) + err = json.Unmarshal([]byte(configJSON), &config) require.NoError(t, err, "Config JSON should be valid") dispatchRepo, ok := config["dispatch_repository"].(map[string]any) diff --git a/pkg/workflow/dispatch_workflow_test.go b/pkg/workflow/dispatch_workflow_test.go index c7498485a71..063d43cb223 100644 --- a/pkg/workflow/dispatch_workflow_test.go +++ b/pkg/workflow/dispatch_workflow_test.go @@ -381,7 +381,8 @@ This workflow dispatches to different workflow types. "yml-test should use .yml extension") // Generate safe outputs config to verify workflow_files is included - configJSON := generateSafeOutputsConfig(workflowData) + configJSON, err := generateSafeOutputsConfig(workflowData) + require.NoError(t, err, "generateSafeOutputsConfig should not return an error") require.NotEmpty(t, configJSON, "Config JSON should not be empty") // Parse config to verify workflow_files is present diff --git a/pkg/workflow/mcp_setup_generator.go b/pkg/workflow/mcp_setup_generator.go index 575086354e8..a069176d781 100644 --- a/pkg/workflow/mcp_setup_generator.go +++ b/pkg/workflow/mcp_setup_generator.go @@ -129,7 +129,11 @@ func (c *Compiler) generateMCPSetup(yaml *strings.Builder, tools map[string]any, // Generate safe-outputs configuration once to avoid duplicate computation var safeOutputConfig string if HasSafeOutputsEnabled(workflowData.SafeOutputs) { - safeOutputConfig = generateSafeOutputsConfig(workflowData) + var err error + safeOutputConfig, err = generateSafeOutputsConfig(workflowData) + if err != nil { + return fmt.Errorf("failed to generate safe outputs config: %w", err) + } } // Sort tools to ensure stable code generation diff --git a/pkg/workflow/safe_jobs_test.go b/pkg/workflow/safe_jobs_test.go index e3114aabba8..0b5c2f526bc 100644 --- a/pkg/workflow/safe_jobs_test.go +++ b/pkg/workflow/safe_jobs_test.go @@ -5,6 +5,8 @@ package workflow import ( "strings" "testing" + + "github.com/stretchr/testify/require" ) func TestParseSafeJobsConfig(t *testing.T) { @@ -392,7 +394,8 @@ func TestSafeJobsInSafeOutputsConfig(t *testing.T) { }, } - configJSON := generateSafeOutputsConfig(workflowData) + configJSON, err := generateSafeOutputsConfig(workflowData) + require.NoError(t, err, "generateSafeOutputsConfig should not return an error") if configJSON == "" { t.Fatal("Expected safe-outputs config JSON to be generated") diff --git a/pkg/workflow/safe_outputs_config_generation.go b/pkg/workflow/safe_outputs_config_generation.go index fde86bf8019..50695b278a5 100644 --- a/pkg/workflow/safe_outputs_config_generation.go +++ b/pkg/workflow/safe_outputs_config_generation.go @@ -25,10 +25,10 @@ import ( // generateSafeOutputsConfig generates the JSON configuration for the safe-outputs // MCP server. Standard handler configs are sourced from handlerRegistry to ensure // they stay in sync with GH_AW_SAFE_OUTPUTS_HANDLER_CONFIG. -func generateSafeOutputsConfig(data *WorkflowData) string { +func generateSafeOutputsConfig(data *WorkflowData) (string, error) { if data.SafeOutputs == nil { safeOutputsConfigLog.Print("No safe outputs configuration found, returning empty config") - return "" + return "", nil } safeOutputsConfigLog.Print("Generating safe outputs configuration for workflow") @@ -109,6 +109,26 @@ func generateSafeOutputsConfig(data *WorkflowData) string { } } + // Safe-actions configuration: custom GitHub Actions exposed as safe output tools. + // The normalized action names are added as config keys so both MCP server implementations + // recognise them as enabled tools (the tool schema is already in tools.json via + // tools_meta.json; the MCP server just needs to see the name in config.json). + if len(data.SafeOutputs.Actions) > 0 { + safeOutputsConfigLog.Printf("Processing %d safe action configurations", len(data.SafeOutputs.Actions)) + for actionName := range data.SafeOutputs.Actions { + normalizedName := stringutil.NormalizeSafeOutputIdentifier(actionName) + if _, exists := safeOutputsConfig[normalizedName]; exists { + return "", fmt.Errorf( + "safe-outputs action %q has a normalized name %q that conflicts with an existing safe outputs config entry; rename the action to avoid the conflict", + actionName, + normalizedName, + ) + } + safeOutputsConfigLog.Printf("Adding safe action to config: %s (normalized: %s)", actionName, normalizedName) + safeOutputsConfig[normalizedName] = true + } + } + // Mentions configuration: controls which @mentions are allowed in AI output. // This is consumed by the ingestion step, not by standard handlers. if data.SafeOutputs.Mentions != nil { @@ -165,11 +185,11 @@ func generateSafeOutputsConfig(data *WorkflowData) string { } if len(safeOutputsConfig) == 0 { - return "" + return "", nil } configJSON, _ := json.Marshal(safeOutputsConfig) safeOutputsConfigLog.Printf("Safe outputs config generation complete: %d tool types configured", len(safeOutputsConfig)) - return string(configJSON) + return string(configJSON), nil } // generateCustomJobToolDefinition creates an MCP tool definition for a custom safe-output job. diff --git a/pkg/workflow/safe_outputs_config_generation_test.go b/pkg/workflow/safe_outputs_config_generation_test.go index d02e9f38007..61e4aa36a66 100644 --- a/pkg/workflow/safe_outputs_config_generation_test.go +++ b/pkg/workflow/safe_outputs_config_generation_test.go @@ -43,7 +43,8 @@ jobs: }, } - result := generateSafeOutputsConfig(data) + result, err := generateSafeOutputsConfig(data) + require.NoError(t, err, "generateSafeOutputsConfig should not return an error") require.NotEmpty(t, result, "Expected non-empty config") var parsed map[string]any @@ -59,6 +60,70 @@ jobs: assert.Equal(t, ".lock.yml", workflowFiles["ci"], "ci should map to .lock.yml") } +// TestGenerateSafeOutputsConfigActions tests that generateSafeOutputsConfig includes custom +// action tool names as enabled keys so both MCP server implementations register them. +func TestGenerateSafeOutputsConfigActions(t *testing.T) { + data := &WorkflowData{ + SafeOutputs: &SafeOutputsConfig{ + Actions: map[string]*SafeOutputActionConfig{ + "upload_report": { + Uses: "actions/upload-artifact@v4", + Description: "Upload the report", + }, + "publish-results": { + Uses: "owner/action@v1", + Description: "Publish results", + }, + }, + }, + } + + result, err := generateSafeOutputsConfig(data) + require.NoError(t, err, "generateSafeOutputsConfig should not return an error") + require.NotEmpty(t, result, "Expected non-empty config") + + var parsed map[string]any + require.NoError(t, json.Unmarshal([]byte(result), &parsed), "Result must be valid JSON") + + // Each action tool should appear as a truthy key in config.json so the MCP server + // registers it. Names are normalized (hyphens converted to underscores). + uploadVal, hasUploadReport := parsed["upload_report"] + assert.True(t, hasUploadReport, "Expected upload_report key in config") + assert.Equal(t, true, uploadVal, "upload_report value should be true") + + publishVal, hasPublishResults := parsed["publish_results"] + assert.True(t, hasPublishResults, "Expected publish_results key in config (hyphen normalized to underscore)") + assert.Equal(t, true, publishVal, "publish_results value should be true") +} + +// TestGenerateSafeOutputsConfigActionsCollisionReturnsError tests that a custom action +// whose normalized name collides with an existing built-in handler key returns an error. +func TestGenerateSafeOutputsConfigActionsCollisionReturnsError(t *testing.T) { + trueVal := "true" + data := &WorkflowData{ + SafeOutputs: &SafeOutputsConfig{ + // add_labels is a built-in handler that produces a real config object. + AddLabels: &AddLabelsConfig{ + Allowed: []string{"bug"}, + }, + // A custom action whose normalized name matches the built-in "add_labels" key. + Actions: map[string]*SafeOutputActionConfig{ + "add-labels": { + Uses: "owner/some-action@v1", + Description: "Should trigger a collision error", + }, + }, + // Ensure at least one handler is set to make config non-empty. + NoOp: &NoOpConfig{BaseSafeOutputConfig: BaseSafeOutputConfig{Max: &trueVal}}, + }, + } + + _, err := generateSafeOutputsConfig(data) + require.Error(t, err, "Expected an error when a custom action name collides with a built-in handler key") + assert.Contains(t, err.Error(), "add-labels", "Error should mention the conflicting action name") + assert.Contains(t, err.Error(), "add_labels", "Error should mention the conflicting normalized name") +} + // TestGenerateSafeOutputsConfigMissingToolWithIssue tests the missing_tool config. // The legacy create_missing_tool_issue sub-key is no longer generated; only missing_tool is present. func TestGenerateSafeOutputsConfigMissingToolWithIssue(t *testing.T) { @@ -73,7 +138,8 @@ func TestGenerateSafeOutputsConfigMissingToolWithIssue(t *testing.T) { }, } - result := generateSafeOutputsConfig(data) + result, err := generateSafeOutputsConfig(data) + require.NoError(t, err, "generateSafeOutputsConfig should not return an error") require.NotEmpty(t, result, "Expected non-empty config") var parsed map[string]any @@ -105,7 +171,8 @@ func TestGenerateSafeOutputsConfigMentions(t *testing.T) { }, } - result := generateSafeOutputsConfig(data) + result, err := generateSafeOutputsConfig(data) + require.NoError(t, err, "generateSafeOutputsConfig should not return an error") require.NotEmpty(t, result, "Expected non-empty config") var parsed map[string]any @@ -347,7 +414,8 @@ func TestGenerateSafeOutputsConfigAddLabelsBlocked(t *testing.T) { }, } - result := generateSafeOutputsConfig(data) + result, err := generateSafeOutputsConfig(data) + require.NoError(t, err, "generateSafeOutputsConfig should not return an error") require.NotEmpty(t, result, "Expected non-empty config") var parsed map[string]any @@ -386,7 +454,8 @@ func TestGenerateSafeOutputsConfigCreatePullRequestTargetRepo(t *testing.T) { }, } - result := generateSafeOutputsConfig(data) + result, err := generateSafeOutputsConfig(data) + require.NoError(t, err, "generateSafeOutputsConfig should not return an error") require.NotEmpty(t, result, "Expected non-empty config") var parsed map[string]any @@ -429,7 +498,8 @@ func TestGenerateSafeOutputsConfigCreatePullRequestBackwardCompat(t *testing.T) }, } - result := generateSafeOutputsConfig(data) + result, err := generateSafeOutputsConfig(data) + require.NoError(t, err, "generateSafeOutputsConfig should not return an error") require.NotEmpty(t, result, "Expected non-empty config") var parsed map[string]any @@ -462,7 +532,8 @@ func TestGenerateSafeOutputsConfigCreatePullRequestAutoCloseIssue(t *testing.T) }, } - result := generateSafeOutputsConfig(data) + result, err := generateSafeOutputsConfig(data) + require.NoError(t, err, "generateSafeOutputsConfig should not return an error") require.NotEmpty(t, result, "Expected non-empty config") var parsed map[string]any @@ -487,7 +558,8 @@ func TestGenerateSafeOutputsConfigCreatePullRequestAutoCloseIssueExpression(t *t }, } - result := generateSafeOutputsConfig(data) + result, err := generateSafeOutputsConfig(data) + require.NoError(t, err, "generateSafeOutputsConfig should not return an error") require.NotEmpty(t, result, "Expected non-empty config") var parsed map[string]any @@ -510,7 +582,8 @@ func TestGenerateSafeOutputsConfigCreatePullRequestAutoCloseIssueOmittedByDefaul }, } - result := generateSafeOutputsConfig(data) + result, err := generateSafeOutputsConfig(data) + require.NoError(t, err, "generateSafeOutputsConfig should not return an error") require.NotEmpty(t, result, "Expected non-empty config") var parsed map[string]any @@ -546,7 +619,8 @@ func TestGenerateSafeOutputsConfigRepoMemory(t *testing.T) { }, } - result := generateSafeOutputsConfig(data) + result, err := generateSafeOutputsConfig(data) + require.NoError(t, err, "generateSafeOutputsConfig should not return an error") require.NotEmpty(t, result, "Expected non-empty config") var parsed map[string]any @@ -586,7 +660,8 @@ func TestGenerateSafeOutputsConfigNoRepoMemory(t *testing.T) { RepoMemoryConfig: nil, } - result := generateSafeOutputsConfig(data) + result, err := generateSafeOutputsConfig(data) + require.NoError(t, err, "generateSafeOutputsConfig should not return an error") require.NotEmpty(t, result, "Expected non-empty config") var parsed map[string]any @@ -609,7 +684,8 @@ func TestGenerateSafeOutputsConfigEmptyRepoMemory(t *testing.T) { }, } - result := generateSafeOutputsConfig(data) + result, err := generateSafeOutputsConfig(data) + require.NoError(t, err, "generateSafeOutputsConfig should not return an error") require.NotEmpty(t, result, "Expected non-empty config") var parsed map[string]any @@ -632,7 +708,8 @@ func TestGenerateSafeOutputsConfigReplyToPullRequestReviewComment(t *testing.T) }, } - result := generateSafeOutputsConfig(data) + result, err := generateSafeOutputsConfig(data) + require.NoError(t, err, "generateSafeOutputsConfig should not return an error") require.NotEmpty(t, result, "Expected non-empty config") var parsed map[string]any @@ -661,7 +738,8 @@ func TestGenerateSafeOutputsConfigReplyToPullRequestReviewCommentWithTarget(t *t }, } - result := generateSafeOutputsConfig(data) + result, err := generateSafeOutputsConfig(data) + require.NoError(t, err, "generateSafeOutputsConfig should not return an error") require.NotEmpty(t, result, "Expected non-empty config") var parsed map[string]any @@ -705,7 +783,8 @@ func TestGenerateSafeOutputsConfigClosePullRequest(t *testing.T) { }, } - result := generateSafeOutputsConfig(data) + result, err := generateSafeOutputsConfig(data) + require.NoError(t, err, "generateSafeOutputsConfig should not return an error") require.NotEmpty(t, result, "Expected non-empty config") var parsed map[string]any @@ -743,7 +822,8 @@ func TestGenerateSafeOutputsConfigClosePullRequestStaged(t *testing.T) { }, } - result := generateSafeOutputsConfig(data) + result, err := generateSafeOutputsConfig(data) + require.NoError(t, err, "generateSafeOutputsConfig should not return an error") require.NotEmpty(t, result, "Expected non-empty config") var parsed map[string]any diff --git a/pkg/workflow/safe_outputs_mentions_test.go b/pkg/workflow/safe_outputs_mentions_test.go index ead41ea63ba..5d414c0762e 100644 --- a/pkg/workflow/safe_outputs_mentions_test.go +++ b/pkg/workflow/safe_outputs_mentions_test.go @@ -5,6 +5,8 @@ package workflow import ( "encoding/json" "testing" + + "github.com/stretchr/testify/require" ) func TestParseMentionsConfig_Boolean(t *testing.T) { @@ -216,9 +218,10 @@ func TestGenerateSafeOutputsConfig_WithMentions(t *testing.T) { }, } - configJSON := generateSafeOutputsConfig(data) + configJSON, err := generateSafeOutputsConfig(data) + require.NoError(t, err, "generateSafeOutputsConfig should not return an error") var parsed map[string]any - err := json.Unmarshal([]byte(configJSON), &parsed) + err = json.Unmarshal([]byte(configJSON), &parsed) if err != nil { t.Fatalf("Failed to parse config JSON: %v", err) } diff --git a/pkg/workflow/safe_outputs_test.go b/pkg/workflow/safe_outputs_test.go index 456a5224fc5..6646c3fe248 100644 --- a/pkg/workflow/safe_outputs_test.go +++ b/pkg/workflow/safe_outputs_test.go @@ -8,6 +8,7 @@ import ( "testing" "github.com/github/gh-aw/pkg/stringutil" + "github.com/stretchr/testify/require" ) // ======================================== @@ -661,7 +662,8 @@ func TestGenerateSafeOutputsConfig(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - result := generateSafeOutputsConfig(tt.workflowData) + result, err := generateSafeOutputsConfig(tt.workflowData) + require.NoError(t, err, "generateSafeOutputsConfig should not return an error") if tt.expectEmpty { if result != "" {