Skip to content

Extract shared SafeOutputAllowBlockConfig across safe-output handlers #42322

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 5 commits into from
Jun 29, 2026
Merged

Extract shared SafeOutputAllowBlockConfig across safe-output handlers #42322

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
6ce08fa
Initial plan
Copilot Jun 29, 2026
715c51d
Extract shared safe-output allow/block config
Copilot Jun 29, 2026
0d7b207
Merge branch 'main' into copilot/deep-report-quick-win-extract-safeou…
github-actions[bot] Jun 29, 2026
4077c35
Fix silent blocked field exposure and integration test compile error
Copilot Jun 29, 2026
efe8074
Merge branch 'main' into copilot/deep-report-quick-win-extract-safeou…
pelikhan Jun 29, 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
9 changes: 1 addition & 8 deletions .github/extensions/agentic-workflows-dashboard/src/dashboard-cli.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -157,14 +157,7 @@ async function findDevBinary(cwd: string, accessFn: AccessLike = access, platfor
}
}

export function createGhAwRunner({
getWorkspacePath,
accessFn = access,
execFileFn = spawnExecFile,
platform = process.platform,
env = process.env,
resolveBin,
}: RunnerOptions): (args: string[]) => Promise<string> {
export function createGhAwRunner({ getWorkspacePath, accessFn = access, execFileFn = spawnExecFile, platform = process.platform, env = process.env, resolveBin }: RunnerOptions): (args: string[]) => Promise<string> {
// Memoize per cwd so findDevBinary is called at most once per workspace path.
const binCache = new Map<string, Promise<string | null>>();
const _resolveBin =
Expand Down
4 changes: 3 additions & 1 deletion pkg/cli/compile_workflow_processor_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,9 @@ func TestExtractSafeOutputLabels_IncludesLabelCommand(t *testing.T) {
AllowedLabels: []string{"triage"},
},
AddLabels: &workflow.AddLabelsConfig{
Allowed: []string{"automation"},
SafeOutputAllowBlockConfig: workflow.SafeOutputAllowBlockConfig{
Allowed: []string{"automation"},
},
},
},
LabelCommand: []string{"deploy"},
Expand Down
9 changes: 4 additions & 5 deletions pkg/workflow/add_labels.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,11 +8,10 @@ var addLabelsLog = logger.New("workflow:add_labels")

// AddLabelsConfig holds configuration for adding labels to issues/PRs from agent output
type AddLabelsConfig struct {
BaseSafeOutputConfig `yaml:",inline"`
SafeOutputTargetConfig `yaml:",inline"`
SafeOutputFilterConfig `yaml:",inline"`
Allowed []string `yaml:"allowed,omitempty"` // Optional list of allowed label patterns (supports glob patterns like "team-*", "area/*"). Labels will be created if they don't already exist in the repository. If omitted, any labels are allowed (including creating new ones).
Blocked []string `yaml:"blocked,omitempty"` // Optional list of blocked label patterns (supports glob patterns like "~*", "*[bot]"). Labels matching these patterns will be rejected.
BaseSafeOutputConfig `yaml:",inline"`
SafeOutputTargetConfig `yaml:",inline"`
SafeOutputFilterConfig `yaml:",inline"`
SafeOutputAllowBlockConfig `yaml:",inline"`

@github-actions github-actions Bot Jun 29, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Operator-critical side-effect stripped from documentation: the original Allowed comment noted that "Labels will be created if they don't already exist in the repository." — this is a write side-effect, not just a filter. The generic replacement says only "Optional list of allowed values" and conceals this completely.

💡 Why this matters

For add_labels, Allowed serves a dual role:

  1. Filter: only labels on this list may be applied.
  2. Creation whitelist: the handler will create labels in the repository if they don't already exist, trusting that any entry in Allowed was pre-approved.

A user adding a label pattern like team-backend to Allowed is implicitly authorizing the agent to create that label in the target repository. Without documentation at the struct definition site, this side-effect is invisible to anyone reading just the config struct. At minimum, the embedding struct or a comment at the field declaration site should note this behavior.

}

// parseAddLabelsConfig handles add-labels configuration
Expand Down
11 changes: 5 additions & 6 deletions pkg/workflow/assign_to_user.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,12 +8,11 @@ var assignToUserLog = logger.New("workflow:assign_to_user")

// AssignToUserConfig holds configuration for assigning users to issues from agent output
type AssignToUserConfig struct {
BaseSafeOutputConfig `yaml:",inline"`
SafeOutputTargetConfig `yaml:",inline"`
SafeOutputFilterConfig `yaml:",inline"`
Allowed []string `yaml:"allowed,omitempty"` // Optional list of allowed usernames. If omitted, any users are allowed.
Blocked []string `yaml:"blocked,omitempty"` // Optional list of blocked usernames or patterns (e.g., "copilot", "*[bot]")
UnassignFirst *string `yaml:"unassign-first,omitempty"` // If true, unassign all current assignees before assigning new ones
BaseSafeOutputConfig `yaml:",inline"`
SafeOutputTargetConfig `yaml:",inline"`
SafeOutputFilterConfig `yaml:",inline"`
SafeOutputAllowBlockConfig `yaml:",inline"`
UnassignFirst *string `yaml:"unassign-first,omitempty"` // If true, unassign all current assignees before assigning new ones
}
Comment on lines 10 to 16

Copilot AI Jun 29, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Fixed in the latest commit — safe_outputs_integration_test.go now initializes the embedded struct explicitly: SafeOutputAllowBlockConfig: SafeOutputAllowBlockConfig{Allowed: []string{"user1"}}. go vet -tags=integration passes cleanly.


// parseAssignToUserConfig handles assign-to-user configuration
Expand Down
32 changes: 24 additions & 8 deletions pkg/workflow/compiler_safe_outputs_config_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -95,7 +95,9 @@ func TestAddHandlerManagerConfigEnvVar(t *testing.T) {
name: "add labels config",
safeOutputs: &SafeOutputsConfig{
AddLabels: &AddLabelsConfig{
Allowed: []string{"bug", "enhancement", "documentation"},
SafeOutputAllowBlockConfig: SafeOutputAllowBlockConfig{
Allowed: []string{"bug", "enhancement", "documentation"},
},
},
},
checkContains: []string{
Expand Down Expand Up @@ -228,7 +230,9 @@ func TestAddHandlerManagerConfigEnvVar(t *testing.T) {
},
},
AddLabels: &AddLabelsConfig{
Allowed: []string{"bug"},
SafeOutputAllowBlockConfig: SafeOutputAllowBlockConfig{
Allowed: []string{"bug"},
},
},
},
checkContains: []string{
Expand Down Expand Up @@ -346,7 +350,9 @@ func TestAddHandlerManagerConfigEnvVar(t *testing.T) {
name: "remove_labels config",
safeOutputs: &SafeOutputsConfig{
RemoveLabels: &RemoveLabelsConfig{
Allowed: []string{"bug", "wontfix"},
SafeOutputAllowBlockConfig: SafeOutputAllowBlockConfig{
Allowed: []string{"bug", "wontfix"},
},
},
},
checkContains: []string{
Expand Down Expand Up @@ -725,7 +731,9 @@ func TestAddHandlerManagerConfigEnvVar(t *testing.T) {
BaseSafeOutputConfig: BaseSafeOutputConfig{
Max: strPtr("5"),
},
Allowed: []string{"user1", "user2"},
SafeOutputAllowBlockConfig: SafeOutputAllowBlockConfig{
Allowed: []string{"user1", "user2"},
},
},
},
checkContains: []string{
Expand Down Expand Up @@ -2088,7 +2096,9 @@ func TestHandlerConfigAssignToUser(t *testing.T) {
TargetRepoSlug: "org/target-repo",
AllowedRepos: []string{"org/repo1", "org/repo2"},
},
Allowed: []string{"user1", "user2", "copilot"},
SafeOutputAllowBlockConfig: SafeOutputAllowBlockConfig{
Allowed: []string{"user1", "user2", "copilot"},
},
},
},
}
Expand Down Expand Up @@ -2217,7 +2227,9 @@ func TestHandlerConfigUnassignFromUser(t *testing.T) {
TargetRepoSlug: "org/target-repo",
AllowedRepos: []string{"org/repo1"},
},
Allowed: []string{"githubactionagent", "bot-user"},
SafeOutputAllowBlockConfig: SafeOutputAllowBlockConfig{
Allowed: []string{"githubactionagent", "bot-user"},
},
},
},
}
Expand Down Expand Up @@ -2291,7 +2303,9 @@ func TestHandlerConfigAssignToUserWithBlocked(t *testing.T) {
Target: "*",
TargetRepoSlug: "microsoft/vscode",
},
Blocked: []string{"copilot", "*[bot]"},
SafeOutputAllowBlockConfig: SafeOutputAllowBlockConfig{
Blocked: []string{"copilot", "*[bot]"},
},
},
},
}
Expand Down Expand Up @@ -2341,7 +2355,9 @@ func TestHandlerConfigUnassignFromUserWithBlocked(t *testing.T) {
Target: "*",
TargetRepoSlug: "microsoft/vscode",
},
Blocked: []string{"copilot", "*[bot]"},
SafeOutputAllowBlockConfig: SafeOutputAllowBlockConfig{
Blocked: []string{"copilot", "*[bot]"},
},
},
},
}
Expand Down
16 changes: 12 additions & 4 deletions pkg/workflow/compiler_safe_outputs_env_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -75,7 +75,9 @@ func TestAddAllSafeOutputConfigEnvVars(t *testing.T) {
safeOutputs: &SafeOutputsConfig{
Staged: templatableBoolPtr("true"),
AddLabels: &AddLabelsConfig{
Allowed: []string{"bug"},
SafeOutputAllowBlockConfig: SafeOutputAllowBlockConfig{
Allowed: []string{"bug"},
},
},
},
checkContains: []string{
Expand Down Expand Up @@ -205,7 +207,9 @@ func TestStagedFlagOnlyAddedOnce(t *testing.T) {
},
},
AddLabels: &AddLabelsConfig{
Allowed: []string{"bug"},
SafeOutputAllowBlockConfig: SafeOutputAllowBlockConfig{
Allowed: []string{"bug"},
},
},
},
}
Expand Down Expand Up @@ -326,7 +330,9 @@ func TestEnvVarsWithMultipleSafeOutputTypes(t *testing.T) {
},
},
AddLabels: &AddLabelsConfig{
Allowed: []string{"bug", "enhancement"},
SafeOutputAllowBlockConfig: SafeOutputAllowBlockConfig{
Allowed: []string{"bug", "enhancement"},
},
},
UpdateIssues: &UpdateIssuesConfig{},
UpdateDiscussions: &UpdateDiscussionsConfig{},
Expand Down Expand Up @@ -516,7 +522,9 @@ func TestAddLabelsTargetRepoStagedBehavior(t *testing.T) {
SafeOutputs: &SafeOutputsConfig{
Staged: templatableBoolPtr("true"),
AddLabels: &AddLabelsConfig{
Allowed: []string{"bug"},
SafeOutputAllowBlockConfig: SafeOutputAllowBlockConfig{
Allowed: []string{"bug"},
},
SafeOutputTargetConfig: SafeOutputTargetConfig{
TargetRepoSlug: "org/target",
},
Expand Down
16 changes: 12 additions & 4 deletions pkg/workflow/compiler_safe_outputs_job_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -92,7 +92,9 @@ func TestBuildConsolidatedSafeOutputsJob(t *testing.T) {
},
},
AddLabels: &AddLabelsConfig{
Allowed: []string{"bug", "enhancement"},
SafeOutputAllowBlockConfig: SafeOutputAllowBlockConfig{
Allowed: []string{"bug", "enhancement"},
},
},
},
expectedJobName: "safe_outputs",
Expand Down Expand Up @@ -1012,7 +1014,9 @@ func TestGitHubAppTokenStepWithOTLPHeaders(t *testing.T) {
},
AddComments: &AddCommentsConfig{},
AddLabels: &AddLabelsConfig{
Allowed: []string{"bug"},
SafeOutputAllowBlockConfig: SafeOutputAllowBlockConfig{
Allowed: []string{"bug"},
},
},
},
}
Expand Down Expand Up @@ -1057,7 +1061,9 @@ func TestGitHubAppTokenStepWithOTLPAttributes(t *testing.T) {
},
AddComments: &AddCommentsConfig{},
AddLabels: &AddLabelsConfig{
Allowed: []string{"bug"},
SafeOutputAllowBlockConfig: SafeOutputAllowBlockConfig{
Allowed: []string{"bug"},
},
},
},
}
Expand Down Expand Up @@ -1102,7 +1108,9 @@ func TestGitHubAppTokenStepWithOTLPHeadersAndAttributes(t *testing.T) {
},
AddComments: &AddCommentsConfig{},
AddLabels: &AddLabelsConfig{
Allowed: []string{"bug"},
SafeOutputAllowBlockConfig: SafeOutputAllowBlockConfig{
Allowed: []string{"bug"},
},
},
},
}
Expand Down
9 changes: 4 additions & 5 deletions pkg/workflow/remove_labels.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,11 +8,10 @@ var removeLabelsLog = logger.New("workflow:remove_labels")

// RemoveLabelsConfig holds configuration for removing labels from issues/PRs from agent output
type RemoveLabelsConfig struct {
BaseSafeOutputConfig `yaml:",inline"`
SafeOutputTargetConfig `yaml:",inline"`
SafeOutputFilterConfig `yaml:",inline"`
Allowed []string `yaml:"allowed,omitempty"` // Optional list of allowed label patterns to remove (supports glob patterns like "team-*", "area/*"). If omitted, any labels can be removed.
Blocked []string `yaml:"blocked,omitempty"` // Optional list of blocked label patterns (supports glob patterns like "~*", "*[bot]"). Labels matching these patterns will be rejected.
BaseSafeOutputConfig `yaml:",inline"`
SafeOutputTargetConfig `yaml:",inline"`
SafeOutputFilterConfig `yaml:",inline"`
SafeOutputAllowBlockConfig `yaml:",inline"`
}

// parseRemoveLabelsConfig handles remove-labels configuration
Expand Down
10 changes: 7 additions & 3 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 @@ -104,7 +104,9 @@ func TestGenerateSafeOutputsConfigActionsCollisionReturnsError(t *testing.T) {
SafeOutputs: &SafeOutputsConfig{
// add_labels is a built-in handler that produces a real config object.
AddLabels: &AddLabelsConfig{
Allowed: []string{"bug"},
SafeOutputAllowBlockConfig: SafeOutputAllowBlockConfig{
Allowed: []string{"bug"},
},
},
// A custom action whose normalized name matches the built-in "add_labels" key.
Actions: map[string]*SafeOutputActionConfig{
Expand Down Expand Up @@ -434,8 +436,10 @@ func TestGenerateSafeOutputsConfigAddLabelsBlocked(t *testing.T) {
Target: "*",
TargetRepoSlug: "microsoft/vscode",
},
Allowed: []string{"bug", "enhancement"},
Blocked: []string{"[*]*", "~spam", "stale", "triage-needed"},
SafeOutputAllowBlockConfig: SafeOutputAllowBlockConfig{
Allowed: []string{"bug", "enhancement"},
Blocked: []string{"[*]*", "~spam", "stale", "triage-needed"},
},
},
},
}
Expand Down
4 changes: 3 additions & 1 deletion pkg/workflow/safe_outputs_integration_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -185,7 +185,9 @@ func TestConsolidatedSafeOutputsJobIntegration(t *testing.T) {
configBuilder: func() *SafeOutputsConfig {
return &SafeOutputsConfig{
AssignToUser: &AssignToUserConfig{
Allowed: []string{"user1"},
SafeOutputAllowBlockConfig: SafeOutputAllowBlockConfig{
Allowed: []string{"user1"},
},
},
}
},
Expand Down
12 changes: 9 additions & 3 deletions pkg/workflow/safe_outputs_parser.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,13 @@ type SafeOutputDiscussionFilterConfig struct {
RequiredCategory string `yaml:"required-category,omitempty"` // Required category for discussion operations
}

// SafeOutputAllowBlockConfig contains common allow/block lists for safe output configurations.
// Embed this in safe output config structs that support optional allowed/blocked value filters.
type SafeOutputAllowBlockConfig struct {
Allowed []string `yaml:"allowed,omitempty"` // Optional list of allowed values

@github-actions github-actions Bot Jun 29, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Semantic docstrings lost in generalization: the shared Allowed comment "Optional list of allowed values" replaces precise per-handler documentation that carried important semantic distinctions.

💡 What was lost
Handler Original comment
AddLabelsConfig "Labels will be created if they don't already exist in the repository. If omitted, any labels are allowed (including creating new ones)."
RemoveLabelsConfig "Optional list of allowed label patterns to remove ... If omitted, any labels can be removed."
SetIssueTypeConfig "Optional list of allowed issue type names. If omitted, any type is allowed (including clearing with "")."
AssignToUserConfig "If omitted, any users are allowed."

The generic comment doesn't convey: when the list is omitted vs. present, what the wildcard behavior is, whether labels are auto-created, or that the empty-string sentinel for clearing a type is allowed. This context should be preserved either by adding handler-specific doc on the embedding site or by enriching the shared struct comments.

Blocked []string `yaml:"blocked,omitempty"` // Optional list of blocked patterns (supports glob patterns)
}

// CloseJobConfig represents common configuration for close operations (close-issue, close-discussion, close-pull-request)
type CloseJobConfig struct {
SafeOutputTargetConfig `yaml:",inline"`
Expand All @@ -34,9 +41,8 @@ type CloseJobConfig struct {

// ListJobConfig represents common configuration for list-based operations (add-labels, add-reviewer, assign-milestone)
type ListJobConfig struct {
SafeOutputTargetConfig `yaml:",inline"`
Allowed []string `yaml:"allowed,omitempty"` // Optional list of allowed values
Blocked []string `yaml:"blocked,omitempty"` // Optional list of blocked patterns (supports glob patterns)
SafeOutputTargetConfig `yaml:",inline"`
SafeOutputAllowBlockConfig `yaml:",inline"`
}

// ParseTargetConfig parses target and target-repo fields from a config map.
Expand Down
8 changes: 6 additions & 2 deletions pkg/workflow/safe_outputs_tools_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -91,7 +91,9 @@ func TestEnhanceToolDescription(t *testing.T) {
safeOutputs: &SafeOutputsConfig{
AddLabels: &AddLabelsConfig{
BaseSafeOutputConfig: BaseSafeOutputConfig{Max: strPtr("5")},
Allowed: []string{"bug", "enhancement", "question"},
SafeOutputAllowBlockConfig: SafeOutputAllowBlockConfig{
Allowed: []string{"bug", "enhancement", "question"},
},
},
},
wantContains: []string{
Expand All @@ -107,7 +109,9 @@ func TestEnhanceToolDescription(t *testing.T) {
safeOutputs: &SafeOutputsConfig{
AddLabels: &AddLabelsConfig{
BaseSafeOutputConfig: BaseSafeOutputConfig{Max: strPtr("3")},
Allowed: []string{"bug", "feature request", "good first issue", "help wanted"},
SafeOutputAllowBlockConfig: SafeOutputAllowBlockConfig{
Allowed: []string{"bug", "feature request", "good first issue", "help wanted"},
},
},
},
wantContains: []string{
Expand Down
9 changes: 4 additions & 5 deletions pkg/workflow/unassign_from_user.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,11 +8,10 @@ var unassignFromUserLog = logger.New("workflow:unassign_from_user")

// UnassignFromUserConfig holds configuration for removing assignees from issues
type UnassignFromUserConfig struct {
BaseSafeOutputConfig `yaml:",inline"`
SafeOutputTargetConfig `yaml:",inline"`
SafeOutputFilterConfig `yaml:",inline"`
Allowed []string `yaml:"allowed,omitempty"` // Optional list of allowed usernames. If omitted, any users can be unassigned.
Blocked []string `yaml:"blocked,omitempty"` // Optional list of blocked usernames or patterns (e.g., "copilot", "*[bot]")
BaseSafeOutputConfig `yaml:",inline"`
SafeOutputTargetConfig `yaml:",inline"`
SafeOutputFilterConfig `yaml:",inline"`
SafeOutputAllowBlockConfig `yaml:",inline"`
}

// parseUnassignFromUserConfig handles unassign-from-user configuration
Expand Down
Loading