Skip to content

fix: expose safe-outputs.actions custom action tools to agent MCP toolset #26291

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
pelikhan merged 4 commits into from
Apr 14, 2026
Merged

fix: expose safe-outputs.actions custom action tools to agent MCP toolset #26291

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
9f6f3cd
Initial plan
Copilot Apr 14, 2026
11d5730
fix: expose safe-outputs.actions custom action tools to agent MCP too…
Copilot Apr 14, 2026
15d5f92
fix: guard against config key collision when adding safe-actions to c…
Copilot Apr 14, 2026
9b4709e
fix: return error on safe-action name collision instead of silently s…
Copilot Apr 14, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion .github/workflows/smoke-codex.lock.yml
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

4 changes: 3 additions & 1 deletion pkg/workflow/compile_outputs_allowed_labels_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down Expand Up @@ -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"`) {
Expand Down
5 changes: 3 additions & 2 deletions pkg/workflow/dispatch_repository_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
3 changes: 2 additions & 1 deletion pkg/workflow/dispatch_workflow_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
6 changes: 5 additions & 1 deletion pkg/workflow/mcp_setup_generator.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
5 changes: 4 additions & 1 deletion pkg/workflow/safe_jobs_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,8 @@ package workflow
import (
"strings"
"testing"

"github.com/stretchr/testify/require"
)

func TestParseSafeJobsConfig(t *testing.T) {
Expand Down Expand Up @@ -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")
Expand Down
28 changes: 24 additions & 4 deletions pkg/workflow/safe_outputs_config_generation.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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")

Expand Down Expand Up @@ -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)
Copy link

Copilot AI Apr 14, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Adding safeOutputsConfig[normalizedName] = true can clobber an existing config entry when a custom action name (after normalization) matches a built-in handler key (e.g. add_labels) or a custom script key. That would silently discard the original object config and can weaken enforcement (e.g. allowed labels/allowed repos/max), since downstream JS treats a truthy non-object config as “no restrictions”. Prefer to detect collisions and avoid overwriting existing entries (and ideally fail validation earlier with a clear error when a normalized custom action name conflicts with any existing safe-output type/tool name).

Suggested change
normalizedName := stringutil.NormalizeSafeOutputIdentifier(actionName)
normalizedName := stringutil.NormalizeSafeOutputIdentifier(actionName)
if _, exists := safeOutputsConfig[normalizedName]; exists {
safeOutputsConfigLog.Printf(
"Skipping safe action %q because normalized name %q conflicts with an existing safe outputs config entry",
actionName,
normalizedName,
)
continue
}

Copilot uses AI. Check for mistakes.
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 {
Expand Down Expand Up @@ -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.
Expand Down
112 changes: 96 additions & 16 deletions pkg/workflow/safe_outputs_config_generation_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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) {
Expand All @@ -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
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand All @@ -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
Expand All @@ -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
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand All @@ -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
Expand All @@ -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
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down
7 changes: 5 additions & 2 deletions pkg/workflow/safe_outputs_mentions_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,8 @@ package workflow
import (
"encoding/json"
"testing"

"github.com/stretchr/testify/require"
)

func TestParseMentionsConfig_Boolean(t *testing.T) {
Expand Down Expand Up @@ -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)
}
Expand Down
Loading
Loading