diff --git a/actions/setup/js/resolve_mentions_from_payload.cjs b/actions/setup/js/resolve_mentions_from_payload.cjs index 80379bf19b1..a81a5e7fb52 100644 --- a/actions/setup/js/resolve_mentions_from_payload.cjs +++ b/actions/setup/js/resolve_mentions_from_payload.cjs @@ -187,7 +187,7 @@ async function resolveAllowedMentionsFromPayload(context, github, core, mentions } // Get configuration options (with defaults) - const allowTeamMembers = mentionsConfig?.allowTeamMembers !== false; // default: true + const allowCollaboratorMentions = (mentionsConfig?.allowedCollaborators ?? mentionsConfig?.allowTeamMembers) !== false; // default: true const allowContext = mentionsConfig?.allowContext !== false; // default: true const allowedList = mentionsConfig?.allowed || []; const allowedTeams = mentionsConfig?.allowedTeams || []; @@ -202,7 +202,7 @@ async function resolveAllowedMentionsFromPayload(context, github, core, mentions knownAuthors.push(...allowedList.filter(alias => typeof alias === "string" && alias.length > 0)); } - // Add members from allowed-teams (always included regardless of allow-team-members setting) + // Add members from allowed-teams (always included regardless of collaborator mention setting) if (Array.isArray(allowedTeams) && allowedTeams.length > 0) { core.info(`[MENTIONS] Fetching members for ${allowedTeams.length} configured team(s)`); for (const teamEntry of allowedTeams) { @@ -231,9 +231,9 @@ async function resolveAllowedMentionsFromPayload(context, github, core, mentions deduplicatedKnownAuthors.push(author); } - // If allow-team-members is disabled, only use known authors (context + allowed list) - if (!allowTeamMembers) { - core.info(`[MENTIONS] Team members disabled - only allowing context (${deduplicatedKnownAuthors.length} users)`); + // If collaborator mentions are disabled, only use known authors (context + allowed list) + if (!allowCollaboratorMentions) { + core.info(`[MENTIONS] Collaborator mentions disabled - only allowing context (${deduplicatedKnownAuthors.length} users)`); if (deduplicatedKnownAuthors.length > maxMentions) { core.warning(`[MENTIONS] Mention limit exceeded: ${deduplicatedKnownAuthors.length} mentions, limiting to ${maxMentions}`); } diff --git a/actions/setup/js/resolve_mentions_from_payload.test.cjs b/actions/setup/js/resolve_mentions_from_payload.test.cjs index 518475cde82..44ab2584e9f 100644 --- a/actions/setup/js/resolve_mentions_from_payload.test.cjs +++ b/actions/setup/js/resolve_mentions_from_payload.test.cjs @@ -285,6 +285,20 @@ describe("resolveAllowedMentionsFromPayload", () => { expect(result).not.toContain("alice"); }); + it("respects allowedCollaborators: false", async () => { + const context = { + eventName: "issues", + payload: { issue: { user: { login: "alice", type: "User" }, assignees: [] } }, + repo: { owner: "o", repo: "r" }, + }; + const result = await resolveAllowedMentionsFromPayload(context, mockGithub, mockCore, { + allowContext: false, + allowedCollaborators: false, + allowed: ["trusted-user"], + }); + expect(result).toEqual(["trusted-user"]); + }); + it("resolves mentions with team members enabled", async () => { const context = { eventName: "issues", diff --git a/docs/adr/40394-rename-allow-team-members-to-allowed-collaborators.md b/docs/adr/40394-rename-allow-team-members-to-allowed-collaborators.md new file mode 100644 index 00000000000..0f7d62c9e98 --- /dev/null +++ b/docs/adr/40394-rename-allow-team-members-to-allowed-collaborators.md @@ -0,0 +1,38 @@ +# ADR-40394: Rename `allow-team-members` to `allowed-collaborators` in `safe-outputs.mentions` + +**Date**: 2026-06-19 +**Status**: Draft + +## Context + +The `safe-outputs.mentions` configuration historically exposed a boolean field named `allow-team-members` to control whether repository collaborators could be @mentioned. The rest of the mentions configuration uses an `allowed-*` naming family (`allowed`, `allowed-teams`), so `allow-team-members` was inconsistent with the surrounding vocabulary and described its subject ("collaborators with any permission level") imprecisely. Because workflow frontmatter is authored by users in repositories outside this codebase, any rename must avoid silently breaking existing workflows that still set the old key. This decision concerns how to evolve a public configuration field name without a breaking change. + +## Decision + +We will rename the field to `allowed-collaborators` as the canonical name and retain `allow-team-members` as a backward-compatible deprecated alias. Concretely: `MentionsConfig.AllowTeamMembers` becomes `AllowedCollaborators` (YAML tag `allowed-collaborators`, runtime JSON key `allowedCollaborators`); `parseMentionsConfig` reads `allowed-collaborators` first and falls back to `allow-team-members`; the JSON schema marks the old key `deprecated` with an `x-deprecation-message`; and a `gh aw fix` codemod (`mentions-allow-team-members-to-allowed-collaborators`) rewrites the old key to the new one in-place while preserving indentation, inline comments, and the markdown body. The codemod is a no-op when the key is absent, when `mentions` is a bare boolean, or when the field is already migrated. + +## Alternatives Considered + +### Alternative 1: Hard rename with no alias +Remove `allow-team-members` entirely and accept only `allowed-collaborators`. Rejected because it would immediately break every existing workflow that still sets the old key, with no migration path, violating the non-negotiable backward-compatibility constraint. + +### Alternative 2: Leave the field named `allow-team-members` +Keep the existing name to avoid any migration work. Rejected because it perpetuates the naming inconsistency with the `allowed-*` family and the imprecise "team-members" terminology, and the cost of a backward-compatible rename plus an automated codemod is low. + +## Consequences + +### Positive +- The field name is consistent with the `allowed-*` configuration family and more accurately describes repository collaborators. +- Existing workflows continue to compile unchanged via the deprecated-alias fallback, and `gh aw fix` migrates them automatically. + +### Negative +- The parser and schema must carry two keys for the same setting until the deprecated alias is removed, increasing maintenance surface. +- The deprecated `allow-team-members` entry lingers in the schema and documentation, which can confuse new authors about which key to use. + +### Neutral +- The runtime JSON key emitted to the handler changes from `allowTeamMembers` to `allowedCollaborators`; downstream consumers read the new key. +- A future ADR will be required to schedule and execute removal of the deprecated alias once adoption is confirmed. + +--- + +*This is a DRAFT ADR generated by the [Design Decision Gate](https://github.com/github/gh-aw/actions/runs/27852098024) workflow. The PR author must review, complete, and finalize this document before the PR can merge.* diff --git a/pkg/cli/codemod_mentions_allow_team_members.go b/pkg/cli/codemod_mentions_allow_team_members.go new file mode 100644 index 00000000000..3de95a3ac0c --- /dev/null +++ b/pkg/cli/codemod_mentions_allow_team_members.go @@ -0,0 +1,151 @@ +package cli + +import ( + "strings" + + "github.com/github/gh-aw/pkg/logger" +) + +var mentionsAllowTeamMembersCodemodLog = logger.New("cli:codemod_mentions_allow_team_members") + +func getMentionsAllowTeamMembersCodemod() Codemod { + return Codemod{ + ID: "mentions-allow-team-members-to-allowed-collaborators", + Name: "Rename allow-team-members to allowed-collaborators in mentions", + Description: "Renames allow-team-members to allowed-collaborators in safe-outputs.mentions.", + IntroducedIn: "1.0.0", + Apply: func(content string, frontmatter map[string]any) (string, bool, error) { + if !mentionsAllowTeamMembersNeedsMigration(frontmatter) { + return content, false, nil + } + + newContent, applied, err := applyFrontmatterLineTransform(content, func(lines []string) ([]string, bool) { + return renameMentionsAllowTeamMembers(lines) + }) + if applied { + mentionsAllowTeamMembersCodemodLog.Print("Renamed allow-team-members to allowed-collaborators in safe-outputs.mentions") + } + return newContent, applied, err + }, + } +} + +func mentionsAllowTeamMembersNeedsMigration(frontmatter map[string]any) bool { + safeOutputsAny, ok := frontmatter["safe-outputs"] + if !ok { + return false + } + safeOutputsMap, ok := safeOutputsAny.(map[string]any) + if !ok { + return false + } + mentionsAny, ok := safeOutputsMap["mentions"] + if !ok { + return false + } + mentionsMap, ok := mentionsAny.(map[string]any) + if !ok { + return false + } + + _, hasOld := mentionsMap["allow-team-members"] + _, hasNew := mentionsMap["allowed-collaborators"] + return hasOld && !hasNew +} + +func renameMentionsAllowTeamMembers(lines []string) ([]string, bool) { + result := make([]string, 0, len(lines)) + modified := false + + inSafeOutputs := false + safeOutputsIndent := "" + safeOutputsChildIndent := "" + inMentions := false + mentionsIndent := "" + mentionsChildIndent := "" + + for i, line := range lines { + trimmed := strings.TrimSpace(line) + indent := getIndentation(line) + + if !strings.HasPrefix(trimmed, "#") { + if inSafeOutputs && hasExitedBlock(line, safeOutputsIndent) { + inSafeOutputs = false + safeOutputsChildIndent = "" + inMentions = false + mentionsIndent = "" + mentionsChildIndent = "" + } + if inMentions && hasExitedBlock(line, mentionsIndent) { + inMentions = false + mentionsIndent = "" + mentionsChildIndent = "" + } + } + + if strings.HasPrefix(trimmed, "safe-outputs:") { + inSafeOutputs = true + safeOutputsIndent = indent + safeOutputsChildIndent = "" + inMentions = false + mentionsIndent = "" + mentionsChildIndent = "" + result = append(result, line) + continue + } + + if inSafeOutputs && isDescendant(indent, safeOutputsIndent) && !strings.HasPrefix(trimmed, "#") { + if (safeOutputsChildIndent == "" || indent == safeOutputsChildIndent) && strings.HasPrefix(trimmed, "mentions:") { + if safeOutputsChildIndent == "" { + safeOutputsChildIndent = indent + } + if strings.HasSuffix(trimmed, ":") { + inMentions = true + mentionsIndent = indent + mentionsChildIndent = "" + } else { + inMentions = false + mentionsIndent = "" + mentionsChildIndent = "" + if strings.Contains(trimmed, "allow-team-members:") { + newLine := strings.Replace(line, "allow-team-members:", "allowed-collaborators:", 1) + result = append(result, newLine) + modified = true + mentionsAllowTeamMembersCodemodLog.Printf("Renamed allow-team-members to allowed-collaborators in safe-outputs.mentions on line %d", i+1) + continue + } + } + result = append(result, line) + continue + } + if strings.HasSuffix(trimmed, ":") && (safeOutputsChildIndent == "" || indent == safeOutputsChildIndent) { + if safeOutputsChildIndent == "" { + safeOutputsChildIndent = indent + } + inMentions = false + mentionsIndent = "" + mentionsChildIndent = "" + result = append(result, line) + continue + } + } + + if inMentions && mentionsChildIndent == "" && isDescendant(indent, mentionsIndent) && trimmed != "" && !strings.HasPrefix(trimmed, "#") { + mentionsChildIndent = indent + } + + if inMentions && indent == mentionsChildIndent && strings.HasPrefix(trimmed, "allow-team-members:") { + newLine, replaced := findAndReplaceInLine(line, "allow-team-members", "allowed-collaborators") + if replaced { + result = append(result, newLine) + modified = true + mentionsAllowTeamMembersCodemodLog.Printf("Renamed allow-team-members to allowed-collaborators in safe-outputs.mentions on line %d", i+1) + continue + } + } + + result = append(result, line) + } + + return result, modified +} diff --git a/pkg/cli/codemod_mentions_allow_team_members_test.go b/pkg/cli/codemod_mentions_allow_team_members_test.go new file mode 100644 index 00000000000..1e7168a4274 --- /dev/null +++ b/pkg/cli/codemod_mentions_allow_team_members_test.go @@ -0,0 +1,315 @@ +//go:build !integration + +package cli + +import ( + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestGetMentionsAllowTeamMembersCodemod(t *testing.T) { + codemod := getMentionsAllowTeamMembersCodemod() + + assert.Equal(t, "mentions-allow-team-members-to-allowed-collaborators", codemod.ID) + assert.Equal(t, "Rename allow-team-members to allowed-collaborators in mentions", codemod.Name) + assert.NotEmpty(t, codemod.Description) + assert.Equal(t, "1.0.0", codemod.IntroducedIn) + require.NotNil(t, codemod.Apply) +} + +func TestMentionsAllowTeamMembersCodemod_HappyPath(t *testing.T) { + codemod := getMentionsAllowTeamMembersCodemod() + + content := `--- +safe-outputs: + mentions: + allow-team-members: false +--- + +# Test workflow` + + frontmatter := map[string]any{ + "safe-outputs": map[string]any{ + "mentions": map[string]any{ + "allow-team-members": false, + }, + }, + } + + result, applied, err := codemod.Apply(content, frontmatter) + + require.NoError(t, err) + assert.True(t, applied) + assert.Contains(t, result, "allowed-collaborators: false") + assert.NotContains(t, result, "allow-team-members:") +} + +func TestMentionsAllowTeamMembersCodemod_NoOp_NoSafeOutputs(t *testing.T) { + codemod := getMentionsAllowTeamMembersCodemod() + + content := `--- +name: test +--- + +# Test workflow` + + frontmatter := map[string]any{ + "name": "test", + } + + result, applied, err := codemod.Apply(content, frontmatter) + + require.NoError(t, err) + assert.False(t, applied) + assert.Equal(t, content, result) +} + +func TestMentionsAllowTeamMembersCodemod_NoOp_NoMentions(t *testing.T) { + codemod := getMentionsAllowTeamMembersCodemod() + + content := `--- +safe-outputs: + add-comment: {} +--- + +# Test workflow` + + frontmatter := map[string]any{ + "safe-outputs": map[string]any{ + "add-comment": map[string]any{}, + }, + } + + result, applied, err := codemod.Apply(content, frontmatter) + + require.NoError(t, err) + assert.False(t, applied) + assert.Equal(t, content, result) +} + +func TestMentionsAllowTeamMembersCodemod_NoOp_MentionsBoolean(t *testing.T) { + codemod := getMentionsAllowTeamMembersCodemod() + + content := `--- +safe-outputs: + mentions: false +--- + +# Test workflow` + + frontmatter := map[string]any{ + "safe-outputs": map[string]any{ + "mentions": false, + }, + } + + result, applied, err := codemod.Apply(content, frontmatter) + + require.NoError(t, err) + assert.False(t, applied) + assert.Equal(t, content, result) +} + +func TestMentionsAllowTeamMembersCodemod_NoOp_AlreadyMigrated(t *testing.T) { + codemod := getMentionsAllowTeamMembersCodemod() + + content := `--- +safe-outputs: + mentions: + allowed-collaborators: false +--- + +# Test workflow` + + frontmatter := map[string]any{ + "safe-outputs": map[string]any{ + "mentions": map[string]any{ + "allowed-collaborators": false, + }, + }, + } + + result, applied, err := codemod.Apply(content, frontmatter) + + require.NoError(t, err) + assert.False(t, applied) + assert.Equal(t, content, result) +} + +func TestMentionsAllowTeamMembersCodemod_Idempotent(t *testing.T) { + codemod := getMentionsAllowTeamMembersCodemod() + + content := `--- +safe-outputs: + mentions: + allow-team-members: true +--- + +# Test workflow` + + frontmatter := map[string]any{ + "safe-outputs": map[string]any{ + "mentions": map[string]any{ + "allow-team-members": true, + }, + }, + } + + result1, applied1, err := codemod.Apply(content, frontmatter) + require.NoError(t, err) + assert.True(t, applied1) + + // Apply again with the updated frontmatter — should be a no-op + frontmatter2 := map[string]any{ + "safe-outputs": map[string]any{ + "mentions": map[string]any{ + "allowed-collaborators": true, + }, + }, + } + result2, applied2, err := codemod.Apply(result1, frontmatter2) + require.NoError(t, err) + assert.False(t, applied2) + assert.Equal(t, result1, result2) +} + +func TestMentionsAllowTeamMembersCodemod_PreservesInlineComment(t *testing.T) { + codemod := getMentionsAllowTeamMembersCodemod() + + content := `--- +safe-outputs: + mentions: + allow-team-members: false # disable team member mentions +--- + +# Test workflow` + + frontmatter := map[string]any{ + "safe-outputs": map[string]any{ + "mentions": map[string]any{ + "allow-team-members": false, + }, + }, + } + + result, applied, err := codemod.Apply(content, frontmatter) + + require.NoError(t, err) + assert.True(t, applied) + assert.Contains(t, result, "allowed-collaborators: false # disable team member mentions") +} + +func TestMentionsAllowTeamMembersCodemod_PreservesIndentation(t *testing.T) { + codemod := getMentionsAllowTeamMembersCodemod() + + content := `--- +safe-outputs: + mentions: + allow-team-members: false + allow-context: true +--- + +# Test workflow` + + frontmatter := map[string]any{ + "safe-outputs": map[string]any{ + "mentions": map[string]any{ + "allow-team-members": false, + "allow-context": true, + }, + }, + } + + result, applied, err := codemod.Apply(content, frontmatter) + + require.NoError(t, err) + assert.True(t, applied) + assert.Contains(t, result, " allowed-collaborators: false") + assert.Contains(t, result, " allow-context: true") + assert.NotContains(t, result, "allow-team-members:") +} + +func TestMentionsAllowTeamMembersCodemod_PreservesMarkdownBody(t *testing.T) { + codemod := getMentionsAllowTeamMembersCodemod() + + content := `--- +safe-outputs: + mentions: + allow-team-members: false +--- + +# Workflow title + +Some markdown content here. +` + + frontmatter := map[string]any{ + "safe-outputs": map[string]any{ + "mentions": map[string]any{ + "allow-team-members": false, + }, + }, + } + + result, applied, err := codemod.Apply(content, frontmatter) + + require.NoError(t, err) + assert.True(t, applied) + assert.Contains(t, result, "# Workflow title") + assert.Contains(t, result, "Some markdown content here.") +} + +func TestMentionsAllowTeamMembersCodemod_NoOp_OldKeyAbsent(t *testing.T) { + codemod := getMentionsAllowTeamMembersCodemod() + + content := `--- +safe-outputs: + mentions: + allow-context: false +--- + +# Test workflow` + + frontmatter := map[string]any{ + "safe-outputs": map[string]any{ + "mentions": map[string]any{ + "allow-context": false, + }, + }, + } + + result, applied, err := codemod.Apply(content, frontmatter) + + require.NoError(t, err) + assert.False(t, applied) + assert.Equal(t, content, result) +} + +func TestMentionsAllowTeamMembersCodemod_FlowStyleMentions(t *testing.T) { + codemod := getMentionsAllowTeamMembersCodemod() + + content := `--- +safe-outputs: + mentions: { allow-team-members: false, allow-context: true } +--- + +# Test workflow` + + frontmatter := map[string]any{ + "safe-outputs": map[string]any{ + "mentions": map[string]any{ + "allow-team-members": false, + "allow-context": true, + }, + }, + } + + result, applied, err := codemod.Apply(content, frontmatter) + + require.NoError(t, err) + assert.True(t, applied) + assert.Contains(t, result, "mentions: { allowed-collaborators: false, allow-context: true }") + assert.NotContains(t, result, "allow-team-members:") +} diff --git a/pkg/cli/fix_codemods.go b/pkg/cli/fix_codemods.go index c8237218da5..0a334236d58 100644 --- a/pkg/cli/fix_codemods.go +++ b/pkg/cli/fix_codemods.go @@ -87,6 +87,7 @@ func GetAllCodemods() []Codemod { getSandboxAgentFalseRemovalCodemod(), // Remove deprecated sandbox.agent: false (rejected in strict mode) getInferToDisableModelInvocationCodemod(), // Migrate deprecated 'infer' to 'disable-model-invocation' getRunInstallScriptsToRuntimesNodeCodemod(), // Move top-level run-install-scripts under runtimes.node + getMentionsAllowTeamMembersCodemod(), // Rename allow-team-members to allowed-collaborators in safe-outputs.mentions } fixCodemodsLog.Printf("Loaded codemod registry: %d codemods available", len(codemods)) return codemods diff --git a/pkg/cli/fix_codemods_test.go b/pkg/cli/fix_codemods_test.go index 6e7baa9c343..70e6fb3ab59 100644 --- a/pkg/cli/fix_codemods_test.go +++ b/pkg/cli/fix_codemods_test.go @@ -106,6 +106,7 @@ func TestGetAllCodemods_ContainsExpectedCodemods(t *testing.T) { "mount-as-clis-to-cli-proxy", "bash-single-quoted-args-rewrite", "infer-to-disable-model-invocation", + "mentions-allow-team-members-to-allowed-collaborators", } for _, expectedID := range expectedIDs { @@ -219,5 +220,6 @@ func expectedCodemodOrder() []string { "sandbox-agent-false-removal", "infer-to-disable-model-invocation", "run-install-scripts-to-runtimes-node", + "mentions-allow-team-members-to-allowed-collaborators", } } diff --git a/pkg/parser/schemas/main_workflow_schema.json b/pkg/parser/schemas/main_workflow_schema.json index 05e150a91c3..0e6256a5205 100644 --- a/pkg/parser/schemas/main_workflow_schema.json +++ b/pkg/parser/schemas/main_workflow_schema.json @@ -10203,11 +10203,17 @@ "type": "object", "description": "Advanced configuration for @mention filtering with fine-grained control", "properties": { - "allow-team-members": { + "allowed-collaborators": { "type": "boolean", - "description": "Allow mentions of repository team members (collaborators with any permission level, excluding bots). Default: true", + "description": "Allow mentions of repository collaborators (users with repository access, excluding bots). Default: true", "default": true }, + "allow-team-members": { + "type": "boolean", + "description": "Deprecated alias for allowed-collaborators", + "deprecated": true, + "x-deprecation-message": "'allow-team-members' is deprecated in safe-outputs.mentions. Use 'allowed-collaborators' instead. Run 'gh aw fix' to automatically migrate." + }, "allow-context": { "type": "boolean", "description": "Allow mentions inferred from event context (issue/PR authors, assignees, commenters). Default: true", diff --git a/pkg/workflow/compiler_types.go b/pkg/workflow/compiler_types.go index d8659f6809f..64e0208e0b1 100644 --- a/pkg/workflow/compiler_types.go +++ b/pkg/workflow/compiler_types.go @@ -777,8 +777,8 @@ type MentionsConfig struct { // nil: use default behavior with team members and context Enabled *bool `yaml:"enabled,omitempty" json:"enabled,omitempty"` - // AllowTeamMembers determines if team members can be mentioned (default: true) - AllowTeamMembers *bool `yaml:"allow-team-members,omitempty" json:"allowTeamMembers,omitempty"` + // AllowedCollaborators determines if repository collaborators can be mentioned (default: true) + AllowedCollaborators *bool `yaml:"allowed-collaborators,omitempty" json:"allowedCollaborators,omitempty"` // AllowContext determines if mentions from event context are allowed (default: true) AllowContext *bool `yaml:"allow-context,omitempty" json:"allowContext,omitempty"` diff --git a/pkg/workflow/safe_output_validation_config_test.go b/pkg/workflow/safe_output_validation_config_test.go index 6ceb4a2c90e..44f57586982 100644 --- a/pkg/workflow/safe_output_validation_config_test.go +++ b/pkg/workflow/safe_output_validation_config_test.go @@ -123,11 +123,11 @@ func TestGetValidationConfigJSONEmpty(t *testing.T) { func TestGetValidationConfigJSONWithMentions(t *testing.T) { mentions := map[string]any{ - "enabled": true, - "allowTeamMembers": false, - "allowContext": false, - "allowed": []string{"copilot", "github-actions"}, - "max": 5, + "enabled": true, + "allowedCollaborators": false, + "allowContext": false, + "allowed": []string{"copilot", "github-actions"}, + "max": 5, } jsonStr, err := GetValidationConfigJSON([]string{"add_comment"}, mentions) @@ -163,8 +163,8 @@ func TestGetValidationConfigJSONWithMentions(t *testing.T) { if enabled, _ := mentionsParsed["enabled"].(bool); !enabled { t.Errorf("Expected mentions.enabled to be true, got %v", mentionsParsed["enabled"]) } - if allowTeam, _ := mentionsParsed["allowTeamMembers"].(bool); allowTeam { - t.Errorf("Expected mentions.allowTeamMembers to be false, got %v", mentionsParsed["allowTeamMembers"]) + if allowTeam, _ := mentionsParsed["allowedCollaborators"].(bool); allowTeam { + t.Errorf("Expected mentions.allowedCollaborators to be false, got %v", mentionsParsed["allowedCollaborators"]) } // A second call without mentions must not include the key (cache safety). diff --git a/pkg/workflow/safe_outputs_config.go b/pkg/workflow/safe_outputs_config.go index bf2e9b2b37f..25e7e1c50d0 100644 --- a/pkg/workflow/safe_outputs_config.go +++ b/pkg/workflow/safe_outputs_config.go @@ -1000,8 +1000,8 @@ func buildMentionsHandlerConfig(m *MentionsConfig) map[string]any { if m.Enabled != nil { cfg["enabled"] = *m.Enabled } - if m.AllowTeamMembers != nil { - cfg["allowTeamMembers"] = *m.AllowTeamMembers + if m.AllowedCollaborators != nil { + cfg["allowedCollaborators"] = *m.AllowedCollaborators } if m.AllowContext != nil { cfg["allowContext"] = *m.AllowContext diff --git a/pkg/workflow/safe_outputs_config_generation_test.go b/pkg/workflow/safe_outputs_config_generation_test.go index 19d658c4401..c1555d48616 100644 --- a/pkg/workflow/safe_outputs_config_generation_test.go +++ b/pkg/workflow/safe_outputs_config_generation_test.go @@ -158,16 +158,16 @@ func TestGenerateSafeOutputsConfigMissingToolWithIssue(t *testing.T) { // TestGenerateSafeOutputsConfigMentions tests the mentions configuration generation. func TestGenerateSafeOutputsConfigMentions(t *testing.T) { enabled := true - allowTeamMembers := false + allowedCollaborators := false max := 5 data := &WorkflowData{ SafeOutputs: &SafeOutputsConfig{ Mentions: &MentionsConfig{ - Enabled: &enabled, - AllowTeamMembers: &allowTeamMembers, - Max: &max, - Allowed: []string{"user1", "user2"}, + Enabled: &enabled, + AllowedCollaborators: &allowedCollaborators, + Max: &max, + Allowed: []string{"user1", "user2"}, }, }, } @@ -182,7 +182,7 @@ func TestGenerateSafeOutputsConfigMentions(t *testing.T) { mentions, ok := parsed["mentions"].(map[string]any) require.True(t, ok, "Expected mentions key in config") assert.True(t, mentions["enabled"].(bool), "enabled should be true") - assert.False(t, mentions["allowTeamMembers"].(bool), "allowTeamMembers should be false") + assert.False(t, mentions["allowedCollaborators"].(bool), "allowedCollaborators should be false") assert.InDelta(t, float64(5), mentions["max"], 0.0001, "max should be 5") } diff --git a/pkg/workflow/safe_outputs_mentions_test.go b/pkg/workflow/safe_outputs_mentions_test.go index c290280fd65..c561f637e8b 100644 --- a/pkg/workflow/safe_outputs_mentions_test.go +++ b/pkg/workflow/safe_outputs_mentions_test.go @@ -59,16 +59,16 @@ func TestParseMentionsConfig_Object(t *testing.T) { { name: "full config", input: map[string]any{ - "allow-team-members": true, - "allow-context": false, - "allowed": []any{"bot1", "bot2"}, - "max": 10, + "allowed-collaborators": true, + "allow-context": false, + "allowed": []any{"bot1", "bot2"}, + "max": 10, }, expected: &MentionsConfig{ - AllowTeamMembers: boolPtr(true), - AllowContext: boolPtr(false), - Allowed: []string{"bot1", "bot2"}, - Max: new(10), + AllowedCollaborators: boolPtr(true), + AllowContext: boolPtr(false), + Allowed: []string{"bot1", "bot2"}, + Max: new(10), }, }, { @@ -82,6 +82,17 @@ func TestParseMentionsConfig_Object(t *testing.T) { Max: new(5), }, }, + { + name: "deprecated allow-team-members alias", + input: map[string]any{ + "allow-team-members": false, + "allow-context": false, + }, + expected: &MentionsConfig{ + AllowedCollaborators: boolPtr(false), + AllowContext: boolPtr(false), + }, + }, { name: "allowed list with @ prefix - should normalize", input: map[string]any{ @@ -130,18 +141,18 @@ func TestParseMentionsConfig_Object(t *testing.T) { { name: "full config with allowed-teams", input: map[string]any{ - "allow-team-members": true, - "allow-context": false, - "allowed": []any{"bot1"}, - "allowed-teams": []any{"myorg/eng"}, - "max": 10, + "allowed-collaborators": true, + "allow-context": false, + "allowed": []any{"bot1"}, + "allowed-teams": []any{"myorg/eng"}, + "max": 10, }, expected: &MentionsConfig{ - AllowTeamMembers: boolPtr(true), - AllowContext: boolPtr(false), - Allowed: []string{"bot1"}, - AllowedTeams: []string{"myorg/eng"}, - Max: new(10), + AllowedCollaborators: boolPtr(true), + AllowContext: boolPtr(false), + Allowed: []string{"bot1"}, + AllowedTeams: []string{"myorg/eng"}, + Max: new(10), }, }, { @@ -170,12 +181,12 @@ func TestParseMentionsConfig_Object(t *testing.T) { t.Fatal("Expected non-nil result") } - // Check AllowTeamMembers - if tt.expected.AllowTeamMembers != nil { - if result.AllowTeamMembers == nil { - t.Errorf("Expected AllowTeamMembers to be %v, got nil", *tt.expected.AllowTeamMembers) - } else if *result.AllowTeamMembers != *tt.expected.AllowTeamMembers { - t.Errorf("Expected AllowTeamMembers to be %v, got %v", *tt.expected.AllowTeamMembers, *result.AllowTeamMembers) + // Check AllowedCollaborators + if tt.expected.AllowedCollaborators != nil { + if result.AllowedCollaborators == nil { + t.Errorf("Expected AllowedCollaborators to be %v, got nil", *tt.expected.AllowedCollaborators) + } else if *result.AllowedCollaborators != *tt.expected.AllowedCollaborators { + t.Errorf("Expected AllowedCollaborators to be %v, got %v", *tt.expected.AllowedCollaborators, *result.AllowedCollaborators) } } @@ -253,16 +264,16 @@ func TestGenerateSafeOutputsConfig_WithMentions(t *testing.T) { { name: "full mentions config", config: &MentionsConfig{ - AllowTeamMembers: boolPtr(false), - AllowContext: boolPtr(true), - Allowed: []string{"bot1", "bot2"}, - Max: new(20), + AllowedCollaborators: boolPtr(false), + AllowContext: boolPtr(true), + Allowed: []string{"bot1", "bot2"}, + Max: new(20), }, expected: map[string]any{ - "allowTeamMembers": false, - "allowContext": true, - "allowed": []string{"bot1", "bot2"}, - "max": 20, + "allowedCollaborators": false, + "allowContext": true, + "allowed": []string{"bot1", "bot2"}, + "max": 20, }, }, { @@ -277,16 +288,16 @@ func TestGenerateSafeOutputsConfig_WithMentions(t *testing.T) { { name: "full config with allowed-teams", config: &MentionsConfig{ - AllowTeamMembers: boolPtr(false), - AllowedTeams: []string{"myorg/eng"}, - Allowed: []string{"bot1"}, - Max: new(30), + AllowedCollaborators: boolPtr(false), + AllowedTeams: []string{"myorg/eng"}, + Allowed: []string{"bot1"}, + Max: new(30), }, expected: map[string]any{ - "allowTeamMembers": false, - "allowedTeams": []string{"myorg/eng"}, - "allowed": []string{"bot1"}, - "max": 30, + "allowedCollaborators": false, + "allowedTeams": []string{"myorg/eng"}, + "allowed": []string{"bot1"}, + "max": 30, }, }, } @@ -384,18 +395,18 @@ func TestExtractSafeOutputsConfig_WithMentions(t *testing.T) { frontmatter: map[string]any{ "safe-outputs": map[string]any{ "mentions": map[string]any{ - "allow-team-members": false, - "allow-context": true, - "allowed": []any{"bot1"}, - "max": 15, + "allowed-collaborators": false, + "allow-context": true, + "allowed": []any{"bot1"}, + "max": 15, }, }, }, expected: &MentionsConfig{ - AllowTeamMembers: boolPtr(false), - AllowContext: boolPtr(true), - Allowed: []string{"bot1"}, - Max: new(15), + AllowedCollaborators: boolPtr(false), + AllowContext: boolPtr(true), + Allowed: []string{"bot1"}, + Max: new(15), }, }, { @@ -474,12 +485,12 @@ func TestExtractSafeOutputsConfig_WithMentions(t *testing.T) { } } - // Check AllowTeamMembers - if tt.expected.AllowTeamMembers != nil { - if config.Mentions.AllowTeamMembers == nil { - t.Errorf("Expected AllowTeamMembers to be %v, got nil", *tt.expected.AllowTeamMembers) - } else if *config.Mentions.AllowTeamMembers != *tt.expected.AllowTeamMembers { - t.Errorf("Expected AllowTeamMembers to be %v, got %v", *tt.expected.AllowTeamMembers, *config.Mentions.AllowTeamMembers) + // Check AllowedCollaborators + if tt.expected.AllowedCollaborators != nil { + if config.Mentions.AllowedCollaborators == nil { + t.Errorf("Expected AllowedCollaborators to be %v, got nil", *tt.expected.AllowedCollaborators) + } else if *config.Mentions.AllowedCollaborators != *tt.expected.AllowedCollaborators { + t.Errorf("Expected AllowedCollaborators to be %v, got %v", *tt.expected.AllowedCollaborators, *config.Mentions.AllowedCollaborators) } } diff --git a/pkg/workflow/safe_outputs_messages_config.go b/pkg/workflow/safe_outputs_messages_config.go index e4ba1a98cee..bf6c97fa853 100644 --- a/pkg/workflow/safe_outputs_messages_config.go +++ b/pkg/workflow/safe_outputs_messages_config.go @@ -49,7 +49,7 @@ func parseMessagesConfig(messagesMap map[string]any) *SafeOutputMessagesConfig { // Mentions can be: // - false: always escapes mentions // - true: always allows mentions (error in strict mode) -// - object: detailed configuration with allow-team-members, allow-context, allowed, max +// - object: detailed configuration with allowed-collaborators, allow-context, allowed, max func parseMentionsConfig(mentions any) *MentionsConfig { safeOutputMessagesLog.Printf("Parsing mentions configuration: type=%T", mentions) config := &MentionsConfig{} @@ -63,10 +63,14 @@ func parseMentionsConfig(mentions any) *MentionsConfig { // Handle object configuration if mentionsMap, ok := mentions.(map[string]any); ok { - // Parse allow-team-members - if allowTeamMembers, exists := mentionsMap["allow-team-members"]; exists { + // Parse allowed-collaborators (preferred) with fallback to deprecated allow-team-members + if allowedCollaborators, exists := mentionsMap["allowed-collaborators"]; exists { + if val, ok := allowedCollaborators.(bool); ok { + config.AllowedCollaborators = &val + } + } else if allowTeamMembers, exists := mentionsMap["allow-team-members"]; exists { if val, ok := allowTeamMembers.(bool); ok { - config.AllowTeamMembers = &val + config.AllowedCollaborators = &val } }