feat(codemods): rename allow-team-members → allowed-collaborators in safe-outputs.mentions#40394
Conversation
… safe-outputs.mentions Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
allow-team-members → allowed-collaborators in safe-outputs.mentions
There was a problem hiding this comment.
Pull request overview
This PR migrates safe-outputs.mentions from the legacy allow-team-members key to the new canonical allowed-collaborators, adding an automatic gh aw fix codemod and updating the Go config types, parsing, and schema accordingly.
Changes:
- Renames the mentions field in Go (
MentionsConfig) and updates parsing/generation to preferallowed-collaboratorswith fallback toallow-team-members. - Updates the workflow schema to make
allowed-collaboratorscanonical and retainallow-team-membersas a deprecated alias with a deprecation message. - Adds a new
gh aw fixcodemod to rewrite frontmatter keys undersafe-outputs.mentions.
Show a summary per file
| File | Description |
|---|---|
| pkg/workflow/safe_outputs_messages_config.go | Prefer allowed-collaborators when parsing mentions, with fallback to deprecated key. |
| pkg/workflow/safe_outputs_mentions_test.go | Updates mentions parsing/generation tests for the new key (but currently contains issues noted in review comments). |
| pkg/workflow/safe_outputs_config.go | Emits mentions config JSON using allowedCollaborators. |
| pkg/workflow/safe_outputs_config_generation_test.go | Updates generation test expectations to the new JSON key. |
| pkg/workflow/safe_output_validation_config_test.go | Updates validation-config JSON tests to the new mentions JSON key. |
| pkg/workflow/compiler_types.go | Renames the MentionsConfig field/tag to AllowedCollaborators. |
| pkg/parser/schemas/main_workflow_schema.json | Makes allowed-collaborators canonical and keeps allow-team-members as a deprecated alias. |
| pkg/cli/fix_codemods.go | Registers the new mentions migration codemod. |
| pkg/cli/fix_codemods_test.go | Updates codemod registry tests to include the new codemod ID and order. |
| pkg/cli/codemod_mentions_allow_team_members.go | Implements frontmatter rewrite from allow-team-members → allowed-collaborators under safe-outputs.mentions. |
| pkg/cli/codemod_mentions_allow_team_members_test.go | Adds unit tests for the new codemod behavior. |
Copilot's findings
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 11/11 changed files
- Comments generated: 11
| if m.AllowedCollaborators != nil { | ||
| cfg["allowedCollaborators"] = *m.AllowedCollaborators | ||
| } |
| AllowContext: boolPtr(false), | ||
| Allowed: []string{"bot1"}, | ||
| AllowedTeams: []string{"myorg/eng"}, | ||
| Max: new(10), |
| AllowedCollaborators: boolPtr(false), | ||
| AllowContext: boolPtr(true), | ||
| Allowed: []string{"bot1", "bot2"}, | ||
| Max: new(20), |
| AllowedCollaborators: boolPtr(false), | ||
| AllowedTeams: []string{"myorg/eng"}, | ||
| Allowed: []string{"bot1"}, | ||
| Max: new(30), |
| AllowedCollaborators: boolPtr(false), | ||
| AllowContext: boolPtr(true), | ||
| Allowed: []string{"bot1"}, | ||
| Max: new(15), |
| AllowedCollaborators: &allowTeamMembers, | ||
| Max: &max, |
| // AllowedCollaborators determines if repository collaborators (team members) can be mentioned (default: true) | ||
| AllowedCollaborators *bool `yaml:"allowed-collaborators,omitempty" json:"allowedCollaborators,omitempty"` |
| "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 (team members with any permission level, excluding bots). Default: true", |
| if inSafeOutputs && isDescendant(indent, safeOutputsIndent) && strings.HasSuffix(trimmed, ":") && !strings.HasPrefix(trimmed, "#") { | ||
| if safeOutputsChildIndent == "" { | ||
| safeOutputsChildIndent = indent | ||
| } | ||
| if indent == safeOutputsChildIndent { | ||
| key := strings.TrimSuffix(trimmed, ":") | ||
| if key == "mentions" { | ||
| inMentions = true | ||
| mentionsIndent = indent | ||
| mentionsChildIndent = "" | ||
| } else { | ||
| inMentions = false | ||
| mentionsIndent = "" | ||
| mentionsChildIndent = "" | ||
| } | ||
| } | ||
| result = append(result, line) | ||
| continue | ||
| } |
| require.NoError(t, err) | ||
| assert.False(t, applied) | ||
| assert.Equal(t, content, result) | ||
| } |
|
✅ Test Quality Sentinel completed test quality analysis. |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. |
|
|
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
🏗️ Design Decision Gate — ADR RequiredThis PR makes significant changes to core business logic (>100 new lines in 📄 Draft ADR committed:
📋 What to do next
Once an ADR is linked in the PR body, this gate will re-run and verify the implementation matches the decision. ❓ Why ADRs MatterADRs create a searchable, permanent record of why the codebase looks the way it does — including how a public configuration field was evolved without a breaking change. Future contributors (and your future self) will thank you. 📋 Michael Nygard ADR Format ReferenceAn ADR must contain these four sections to be considered complete:
All ADRs are stored in
|
🧪 Test Quality Sentinel Report
📊 Metrics & Test Classification (11 tests analyzed)
Go: 11 (
|
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /tdd — requesting changes on two test coverage gaps in the backward-compatibility path.
📋 Key Themes & Highlights
Key Themes
- Untested backward-compat fallback:
parseMentionsConfignow prefersallowed-collaboratorswith a fallback toallow-team-members, but the test cases insafe_outputs_mentions_test.gowere updated to use only the new key as input. The fallback branch (else if allowTeamMembers) is never exercised by a test. - Undocumented no-op edge case: When both keys exist simultaneously, the codemod skips without a test asserting that expectation. This should be documented as intentional behavior.
Positive Highlights
- ✅ Excellent codemod test coverage: happy path, four distinct no-op conditions, idempotency, and preservation of indentation / inline comments
- ✅ Clean two-key fallback structure in
parseMentionsConfig— reads intuitively - ✅ Schema correctly retains
allow-team-membersas a deprecated alias withx-deprecation-messagepointing users togh aw fix - ✅
hasExitedBlockcorrectly returnsfalsefor blank lines, so YAML with internal blank lines is handled safely
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · 102.1 AIC · ⌖ 7.43 AIC · ⊞ 6.9K
| if val, ok := allowedCollaborators.(bool); ok { | ||
| config.AllowedCollaborators = &val | ||
| } | ||
| } else if allowTeamMembers, exists := mentionsMap["allow-team-members"]; exists { |
There was a problem hiding this comment.
[/tdd] The else if allowTeamMembers backward-compat fallback path is untested — this is the most critical part of the migration story.
All TestParseMentionsConfig_Object test cases were updated to use allowed-collaborators as the input key, so no test now exercises the scenario where a user's un-migrated workflow still uses allow-team-members. If this fallback is silently broken, existing workflows would lose their collaborator-mention filtering without any visible error.
💡 Suggested test case to add to `safe_outputs_mentions_test.go`
{
name: "allow-team-members key (backward-compat fallback)",
input: map[string]any{
"allow-team-members": false,
"allow-context": true,
},
expected: &MentionsConfig{
AllowedCollaborators: boolPtr(false),
AllowContext: boolPtr(true),
},
},This directly exercises the else if allowTeamMembers branch added in parseMentionsConfig.
|
|
||
| _, hasOld := mentionsMap["allow-team-members"] | ||
| _, hasNew := mentionsMap["allowed-collaborators"] | ||
| return hasOld && !hasNew |
There was a problem hiding this comment.
[/tdd] When both allow-team-members and allowed-collaborators are present simultaneously, mentionsAllowTeamMembersNeedsMigration returns false and the codemod silently skips. The old deprecated key stays in the file even after the user runs gh aw fix.
The TestMentionsAllowTeamMembersCodemod_NoOp_AlreadyMigrated test only covers the case where allow-team-members is absent — not the "both keys present" case. A user who hand-edited their YAML to add allowed-collaborators but forgot to remove the old key would be surprised that gh aw fix does nothing.
💡 Suggested additional test
func TestMentionsAllowTeamMembersCodemod_NoOp_BothKeysPresent(t *testing.T) {
codemod := getMentionsAllowTeamMembersCodemod()
content := `---
safe-outputs:
mentions:
allow-team-members: false
allowed-collaborators: false
---`
frontmatter := map[string]any{
"safe-outputs": map[string]any{
"mentions": map[string]any{
"allow-team-members": false,
"allowed-collaborators": false,
},
},
}
result, applied, err := codemod.Apply(content, frontmatter)
require.NoError(t, err)
assert.False(t, applied)
assert.Equal(t, content, result)
}This documents the intended no-op and prevents a future refactor from accidentally "fixing" both-key documents in unexpected ways.
|
@copilot run pr-reviewer skill |
There was a problem hiding this comment.
REQUEST_CHANGES — one critical blocker (flagged by prior review) plus one new medium finding.
Critical blocker (pre-existing comment, still unresolved)
The change in buildMentionsHandlerConfig (line 1005) now emits allowedCollaborators as the JSON key, but the safe-outputs runtime handler still reads allowTeamMembers. This means any workflow that has already migrated to allowed-collaborators: false will have that setting silently ignored at runtime — collaborators remain mentionable by default. This is a correctness regression that must be resolved before merging (see existing inline comment at L1005).
Additional finding (new)
Backward-compat fallback in parseMentionsConfig is untested
The else if allow-team-members branch (lines 71-74 of safe_outputs_messages_config.go) has no test coverage. All test cases that previously exercised allow-team-members as input were migrated to allowed-collaborators, leaving the fallback as dead code from a testing perspective. A regression here silently breaks filtering for workflows still on the old key — see inline comment for a suggested fix.
🔎 Code quality review by PR Code Quality Reviewer · 202.5 AIC · ⌖ 9.78 AIC · ⊞ 5.1K
| if val, ok := allowedCollaborators.(bool); ok { | ||
| config.AllowedCollaborators = &val | ||
| } | ||
| } else if allowTeamMembers, exists := mentionsMap["allow-team-members"]; exists { |
There was a problem hiding this comment.
Backward-compat fallback untested: the allow-team-members fallback path has zero test coverage — every prior test case that used allow-team-members input was migrated to allowed-collaborators, leaving lines 71-74 as a dead branch from a test perspective.
💡 Impact and suggested fix
During the migration window, workflows that have not yet run gh aw fix still carry allow-team-members in their YAML. If this fallback regresses, AllowedCollaborators silently stays nil. Because the runtime defaults the field to true when nil, a workflow that explicitly set allow-team-members: false would silently stop filtering collaborators — with no failing test to catch it.
Add a test case to TestParseMentionsConfig_Object that exercises the deprecated key:
{
name: "backward compat — allow-team-members maps to AllowedCollaborators",
input: map[string]any{
"allow-team-members": false,
},
expected: &MentionsConfig{
AllowedCollaborators: boolPtr(false),
},
},Same gap exists in TestExtractSafeOutputsConfig_WithMentions.
|
@copilot run pr-finisher skill |
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Done. I ran the PR review workflow and addressed the actionable feedback in commits up to |
allow-team-membersis being replaced by the more consistentallowed-collaboratorsname insafe-outputs.mentions. This adds agh aw fixcodemod for automatic migration and renames the field throughout the codebase.Codemod (
mentions-allow-team-members-to-allowed-collaborators)safe-outputs.mentions.allow-team-membersand rewrites it toallowed-collaboratorsmentionsis a bare boolean, or field is already migratedField rename
MentionsConfig.AllowTeamMembers→AllowedCollaboratorswith YAML tagallowed-collaboratorsallowTeamMembers→allowedCollaboratorsparseMentionsConfigfalls back toallow-team-membersfor backward compatibility during migrationSchema
allow-team-membersreplaced byallowed-collaboratorsas the canonical entryx-deprecation-messagepointing users togh aw fixBefore / after: