feat(safe-outputs): add allowed-teams to mentions configuration#40368
Conversation
Allows organizations to specify team slugs in safe-outputs.mentions.allowed-teams so that all members of those teams are automatically allowed to be mentioned without listing individual usernames. - Add AllowedTeams []string field to MentionsConfig struct - Parse allowed-teams in parseMentionsConfig (normalizes @ prefix) - Include allowedTeams in buildMentionsHandlerConfig - Add fetchTeamMembers() helper in resolve_mentions_from_payload.cjs that fetches team members via GitHub API, supporting both "team-slug" and "org/team-slug" formats, excluding bots - Wire allowedTeams handling into resolveAllowedMentionsFromPayload - Add Go and JS tests for all new functionality - Update frontmatter-full.md and safe-outputs.md documentation Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
add_comment mentions sanitizationallowed-teams to mentions configuration
|
@copilot Because team membership lookup uses the API, is rate-limited, expensive, and may fail with the default token, ensure you have proper fallback and error handling so failures are non-fatal. |
…tion Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Done in the latest commit.
All failure paths are non-fatal and covered by new tests. |
|
@copilot clarify docs, JSON schema description that this feature requires a special permission from a PAT or a GitHub app. |
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Done in the latest commit. Added
|
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. |
|
✅ PR Code Quality Reviewer completed the code quality review. |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
|
✅ Test Quality Sentinel completed test quality analysis. |
There was a problem hiding this comment.
Pull request overview
Adds mentions.allowed-teams to the safe-outputs mention filtering pipeline, enabling allowlisting GitHub team members at runtime (instead of enumerating individual users), and documents the required org-level permissions.
Changes:
- Extend compiler/frontmatter parsing and handler config emission to support
allowed-teams→allowedTeams. - Add JS runtime support to resolve team members via
github.rest.teams.listMembersInOrg(paged, bot-filtered, non-fatal on errors). - Update schema + docs to document the new configuration and required token permissions.
Show a summary per file
| File | Description |
|---|---|
| pkg/workflow/compiler_types.go | Adds AllowedTeams field and documents required permissions. |
| pkg/workflow/safe_outputs_messages_config.go | Parses allowed-teams from frontmatter and normalizes @ prefix. |
| pkg/workflow/safe_outputs_config.go | Emits allowedTeams into the JS handler config. |
| pkg/workflow/safe_outputs_mentions_test.go | Adds tests for parsing/extraction/propagation of allowed-teams. |
| actions/setup/js/resolve_mentions_from_payload.cjs | Implements team member expansion and integrates it into mention resolution. |
| actions/setup/js/resolve_mentions_from_payload.test.cjs | Adds unit tests covering allowed-teams behavior and the helper. |
| pkg/parser/schemas/main_workflow_schema.json | Adds allowed-teams to the workflow schema for editor/autocomplete support. |
| docs/src/content/docs/reference/safe-outputs.md | Documents allowed-teams usage and token requirements. |
| docs/src/content/docs/reference/frontmatter-full.md | Updates frontmatter reference to include allowed-teams and permission notes. |
Copilot's findings
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 9/9 changed files
- Comments generated: 6
| async function fetchTeamMembers(teamEntry, defaultOrg, github, core) { | ||
| let org = defaultOrg; | ||
| let teamSlug = teamEntry; | ||
|
|
||
| // Support "org/team-slug" format | ||
| const slashIdx = teamEntry.indexOf("/"); | ||
| if (slashIdx !== -1) { | ||
| org = teamEntry.slice(0, slashIdx); | ||
| teamSlug = teamEntry.slice(slashIdx + 1); | ||
| } |
| const logins = /** @type {string[]} */ []; | ||
| let page = 1; | ||
| const maxPages = 10; // cap at 1000 members to avoid excessive API calls | ||
|
|
||
| while (page <= maxPages) { | ||
| const response = await github.rest.teams.listMembersInOrg({ | ||
| org, | ||
| team_slug: teamSlug, | ||
| per_page: 100, | ||
| page, | ||
| }); | ||
| const pageLogins = response.data.filter(member => member.type !== "Bot" && typeof member.login === "string").map(member => member.login); | ||
| logins.push(...pageLogins); | ||
| if (response.data.length < 100) { | ||
| break; // no more pages | ||
| } | ||
| page++; | ||
| } |
| // Check AllowedTeams | ||
| if len(tt.expected.AllowedTeams) > 0 { | ||
| if len(result.AllowedTeams) != len(tt.expected.AllowedTeams) { | ||
| t.Errorf("Expected AllowedTeams length %d, got %d", len(tt.expected.AllowedTeams), len(result.AllowedTeams)) | ||
| } else { | ||
| for i, expected := range tt.expected.AllowedTeams { | ||
| if result.AllowedTeams[i] != expected { | ||
| t.Errorf("Expected AllowedTeams[%d] to be %q, got %q", i, expected, result.AllowedTeams[i]) | ||
| } | ||
| } | ||
| } | ||
| } |
| // Check AllowedTeams | ||
| if len(tt.expected.AllowedTeams) > 0 { | ||
| if len(config.Mentions.AllowedTeams) != len(tt.expected.AllowedTeams) { | ||
| t.Errorf("Expected AllowedTeams length %d, got %d", len(tt.expected.AllowedTeams), len(config.Mentions.AllowedTeams)) | ||
| } else { | ||
| for i, expected := range tt.expected.AllowedTeams { | ||
| if config.Mentions.AllowedTeams[i] != expected { | ||
| t.Errorf("Expected AllowedTeams[%d] to be %q, got %q", i, expected, config.Mentions.AllowedTeams[i]) | ||
| } | ||
| } | ||
| } | ||
| } |
| # IMPORTANT: Requires read:org scope — not available with the default GITHUB_TOKEN. | ||
| # Use a classic PAT with read:org, a fine-grained PAT with Members:Read, or a | ||
| # GitHub App with the Members:Read permission. Without the required scope, team | ||
| # lookups fail silently (warning logged) and those team members are skipped. |
| > [!IMPORTANT] | ||
| > `allowed-teams` requires the workflow token to have `read:org` scope. The default `GITHUB_TOKEN` does **not** include this scope. Use one of the following: | ||
| > - A **classic PAT** with the `read:org` scope stored as a repository secret | ||
| > - A **fine-grained PAT** with the "Members" repository permission (read) | ||
| > - A **GitHub App** installation token with the "Members" permission (read) | ||
| > | ||
| > If the token lacks `read:org`, team membership lookup will fail with HTTP 403/404 and a warning will be logged. The workflow continues without those team members in the allowlist. |
🏗️ Design Decision Gate — ADR RequiredThis PR makes significant changes to core business logic (159 new lines, >100 threshold) but does not have a linked Architecture Decision Record (ADR). 📄 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 Matter
ADRs create a searchable, permanent record of why the codebase looks the way it does. 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
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
REQUEST_CHANGES — one correctness bug and two medium issues need fixing before merge.
Blocking themes
Silent truncation at 1000 members (high)
fetchTeamMembers hard-caps at 10 pages but emits no warning when the cap is hit. Teams with >1000 members will have their allowlist silently truncated; the success log actively misleads by printing the capped count as if it were the full membership. Members 1001+ will have their @mentions unexpectedly escaped with no operator-visible signal. See inline comment.
Sequential team fetches (medium)
All allowedTeams are fetched with await inside a for...of, serializing up to N × 10 API calls. For orgs with several teams this is a meaningful workflow latency hit and increases secondary rate-limit risk. Promise.all would eliminate the serialization. See inline comment.
Malformed entries reach the JS runtime (medium × 2)
Two gaps let bad config escape compile-time validation: (1) Go doesn't reject empty strings produced by @-only normalization; (2) the exported JS fetchTeamMembers doesn't strip @ itself, so direct callers get confusing errors rather than a clean validation warning. Both are inline.
Warning
Firewall blocked 1 domain
The following domain was blocked by the firewall during workflow execution:
proxy.golang.org
To allow these domains, add them to the
network.allowedlist in your workflow frontmatter:
network:
allowed:
- defaults
- "proxy.golang.org"See Network Configuration for more information.
🔎 Code quality review by PR Code Quality Reviewer · 105.8 AIC · ⌖ 10.4 AIC · ⊞ 5.1K
| break; // no more pages | ||
| } | ||
| page++; | ||
| } |
There was a problem hiding this comment.
Silent truncation at 1000 members emits no warning: when a team has >1000 members the while loop exits without logging anything, and the success message "Fetched 1000 member(s)" actively misleads operators into thinking the list is complete.
💡 Suggested fix
Add a warning after the loop closes so operators know the cap was hit:
while (page <= maxPages) {
// ... existing pagination logic ...
page++;
}
// Add this:
if (page > maxPages) {
core.warning(
`[MENTIONS] Team ${org}/${teamSlug} has more than ${maxPages * 100} members; ` +
`only the first ${logins.length} were loaded. Consider splitting the team or raising the cap.`
);
}This matters for allowlist correctness: team members 1001+ are silently excluded and their @mentions will be unexpectedly escaped in AI-generated content, with no indication of why.
| // Add members from allowed-teams (always included regardless of allow-team-members setting) | ||
| if (Array.isArray(allowedTeams) && allowedTeams.length > 0) { | ||
| core.info(`[MENTIONS] Fetching members for ${allowedTeams.length} configured team(s)`); | ||
| for (const teamEntry of allowedTeams) { |
There was a problem hiding this comment.
Sequential team fetches block workflow execution for O(N × pages) API calls: all allowedTeams lookups are awaited one-by-one; with N teams each potentially making up to 10 paginated requests, large configs serialize tens of round-trips before mention resolution can proceed.
💡 Suggested fix
Parallelize at the team level with Promise.all:
if (Array.isArray(allowedTeams) && allowedTeams.length > 0) {
core.info(`[MENTIONS] Fetching members for ${allowedTeams.length} configured team(s)`);
const teamResults = await Promise.all(
allowedTeams
.filter(teamEntry => typeof teamEntry === "string" && teamEntry.length > 0)
.map(teamEntry => fetchTeamMembers(teamEntry, owner, github, core))
);
for (const members of teamResults) {
knownAuthors.push(...members);
}
}Each team is still capped at 10 pages internally; the change just runs teams concurrently rather than in series. If secondary rate limits are a concern, a small concurrency limiter (e.g. p-limit or a simple chunk-based approach) is preferable to full serialization.
| normalized = str[1:] | ||
| safeOutputMessagesLog.Printf("Normalized team mention '%s' to '%s'", str, normalized) | ||
| } | ||
| allowedTeamsStrings = append(allowedTeamsStrings, normalized) |
There was a problem hiding this comment.
@-only or @/team entries normalize to empty strings and slip through to runtime: after stripping the @ prefix, the Go parser never checks that the result is non-empty; inputs like "@" become "" and "@/team" becomes "/team", both of which are appended to AllowedTeams and emitted to the JS runtime.
💡 Suggested fix
Add a post-normalization length check before appending:
for _, item := range allowedTeamsArray {
if str, ok := item.(string); ok {
normalized := str
if len(str) > 0 && str[0] == '@' {
normalized = str[1:]
safeOutputMessagesLog.Printf(...)
}
if normalized == "" {
safeOutputMessagesLog.Printf("[MENTIONS] Skipping empty allowed-teams entry (was: %q)", str)
continue
}
allowedTeamsStrings = append(allowedTeamsStrings, normalized)
}
}Catching this at compile time produces a clear config-validation signal rather than a generic runtime warning deep inside the JS fetch path.
| teamSlug = teamEntry.slice(slashIdx + 1); | ||
| } | ||
|
|
||
| if (!org || !teamSlug) { |
There was a problem hiding this comment.
fetchTeamMembers doesn't normalize @-prefixed entries, unlike the Go parser: the Go layer strips @ before emitting config, but since fetchTeamMembers is now exported and independently tested, a caller passing "@myorg/eng" directly would send "@myorg" as the org name, get a 404, and see a confusing "Failed to fetch members for team @myorg/eng" message instead of an "invalid team entry" validation warning.
💡 Suggested fix
Add defensive normalization at the top of the function, mirroring what Go already does:
async function fetchTeamMembers(teamEntry, defaultOrg, github, core) {
// Normalize: strip leading '@' if present (Go parser does this too, but be defensive)
const entry = teamEntry.startsWith("@") ? teamEntry.slice(1) : teamEntry;
let org = defaultOrg;
let teamSlug = entry;
const slashIdx = entry.indexOf("/");
// ... rest unchangedThis keeps the exported JS function self-contained and correctly validates @-prefixed inputs regardless of how the function is invoked.
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /zoom-out, /tdd, and /grill-with-docs — approving with three suggestions, none blocking.
📋 Key Themes & Highlights
Suggestions
- Sequential team fetching (
/zoom-out): thefor...ofloopawaits each team in series;Promise.allwould parallelize them with no semantic change (see inline comment) - Silent 1000-member truncation (
/tdd): when a team exceeds themaxPages × 100cap the loop exits without any warning; users debugging missing mentions have no signal (see inline comment + suggested test) fetchTeamMembers@-prefix gap (/grill-with-docs): Go normalizes@org/team→org/teambefore emitting, but the exported JS function doesn't, creating a subtle contract inconsistency for direct callers (see inline comment)
Positive Highlights
- ✅ Comprehensive tests: 8 unit tests for
fetchTeamMembers+ 4 integration tests cover pagination, bot exclusion, org-slug inference, and all three error paths (429, 403, 404) - ✅ Non-fatal by design: all three error branches (rate-limit, permission, generic) return
[]with a warning — the workflow is never blocked by a missingread:orgscope - ✅ End-to-end pipeline correctness:
AllowedTeamsis properly threaded Go struct tag → YAML parser → JSON emitter → JSmentionsConfigwith matchingyaml:"allowed-teams"/json:"allowedTeams"tags - ✅ Documentation:
[!IMPORTANT]callout forread:orgis well-placed and the token-type table (classic PAT / fine-grained PAT / GitHub App) is exactly what users need to diagnose failures - ✅ Schema updated:
main_workflow_schema.jsonincludes the newallowed-teamsproperty with the permission note, keeping editor autocomplete accurate
Warning
Firewall blocked 1 domain
The following domain was blocked by the firewall during workflow execution:
proxy.golang.org
To allow these domains, add them to the
network.allowedlist in your workflow frontmatter:
network:
allowed:
- defaults
- "proxy.golang.org"See Network Configuration for more information.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · 118.7 AIC · ⌖ 10.1 AIC · ⊞ 6.9K
| // Add members from allowed-teams (always included regardless of allow-team-members setting) | ||
| if (Array.isArray(allowedTeams) && allowedTeams.length > 0) { | ||
| core.info(`[MENTIONS] Fetching members for ${allowedTeams.length} configured team(s)`); | ||
| for (const teamEntry of allowedTeams) { |
There was a problem hiding this comment.
[/zoom-out] Team members are fetched sequentially inside a for...of loop with await. For an org with several allowed-teams entries, latency grows linearly (one full paginated round-trip per team). Promise.all would fetch all teams in parallel.
💡 Suggested refactor
if (Array.isArray(allowedTeams) && allowedTeams.length > 0) {
core.info(`[MENTIONS] Fetching members for ${allowedTeams.length} configured team(s)`);
const teamMemberArrays = await Promise.all(
allowedTeams
.filter(e => typeof e === "string" && e.length > 0)
.map(e => fetchTeamMembers(e, owner, github, core))
);
knownAuthors.push(...teamMemberArrays.flat());
}Reduces latency from O(n teams) serial round-trips to a single parallel fan-out.
| page++; | ||
| } | ||
|
|
||
| core.info(`[MENTIONS] Fetched ${logins.length} member(s) from team ${org}/${teamSlug}`); |
There was a problem hiding this comment.
[/tdd] When a team has more than 1,000 members the while-loop exits silently after page 10 — core.info reports the (truncated) count with no indication that results are incomplete. Users with large teams will see some members unexpectedly absent from the allowlist with no warning to diagnose why.
💡 Log a warning on truncation + add a test
}
if (page > maxPages) {
core.warning(
`[MENTIONS] Team ${org}/${teamSlug} has more than 1,000 members; only the first 1,000 were fetched. Increase maxPages or split into sub-teams.`
);
}
core.info(`[MENTIONS] Fetched ${logins.length} member(s) from team ${org}/${teamSlug}`);
return logins;Suggested test (in fetchTeamMembers describe):
it("warns when team exceeds 1000-member cap", async () => {
const fullPage = Array.from({ length: 100 }, (_, i) => ({ login: `user${i}`, type: "User" }));
const mockGithub = { rest: { teams: { listMembersInOrg: vi.fn(async () => ({ data: fullPage })) } } };
await fetchTeamMembers("myorg/huge-team", "defaultorg", mockGithub, mockCore);
expect(mockCore.warning).toHaveBeenCalledWith(expect.stringContaining("1,000"));
expect(mockGithub.rest.teams.listMembersInOrg).toHaveBeenCalledTimes(10);
});| let teamSlug = teamEntry; | ||
|
|
||
| // Support "org/team-slug" format | ||
| const slashIdx = teamEntry.indexOf("/"); |
There was a problem hiding this comment.
[/grill-with-docs] The Go parser strips leading @ from allowed-teams entries before emitting them into the JS config, so in practice this function never receives "@org/team". However, fetchTeamMembers is exported and its contract as a standalone function is unclear — a caller passing "@myorg/eng" directly would silently query for org "@myorg", which would 404.
💡 Add defensive normalization (1 line)
async function fetchTeamMembers(teamEntry, defaultOrg, github, core) {
// Normalize: strip leading '@' to match the Go-layer convention
teamEntry = teamEntry.startsWith("@") ? teamEntry.slice(1) : teamEntry;
let org = defaultOrg;
// ...This makes the JS function self-consistent regardless of call site, mirrors what parseMentionsConfig already does on the Go side, and prevents a subtle silent failure in future tests or integrations that call fetchTeamMembers directly.
🧪 Test Quality Sentinel Report
📊 Metrics & Test Classification (21 tests analyzed)
Go: 8 new table rows ( 📝 Inflation Note — Justified by Feature ScopeBoth test files exceed the 2:1 line ratio threshold:
The inflation is structural rather than copy-paste bloat. Verdict
Score breakdown: behavioral coverage 40/40 · error/edge case 16/30 · no duplication 20/20 · inflation penalty −10/10 = 76/100. References: §27848743380
|
The
mentions.allowedlist requires enumerating every individual user, making it impractical for organizations that want to freely mention their engineers. This addsallowed-teamssupport so team membership can be declared once and resolved dynamically at runtime.Changes
Go (compiler)
compiler_types.go: AddedAllowedTeams []stringtoMentionsConfig(yaml:"allowed-teams",json:"allowedTeams") with documentedread:orgpermission requirementsafe_outputs_messages_config.go: Parseallowed-teamsinparseMentionsConfig; normalizes@prefix likealloweddoessafe_outputs_config.go: EmitallowedTeamsfrombuildMentionsHandlerConfigso it flows through to the JS runtimeJS (runtime)
resolve_mentions_from_payload.cjs: AddedfetchTeamMembers(teamEntry, defaultOrg, github, core)— fetches team members viagithub.rest.teams.listMembersInOrgwith full pagination (up to 1,000 members), excludes bots, and handles API errors gracefully. Wired intoresolveAllowedMentionsFromPayloadbefore the collaborator lookup step.[]read:orgscope hint, returns[]Documentation & Schema
safe-outputs.md: Added[!IMPORTANT]callout explaining thatallowed-teamsrequiresread:orgscope and documenting supported token types (classic PAT, fine-grained PAT, GitHub App)frontmatter-full.md: Updated inline YAML comment with permission requirementmain_workflow_schema.json: Added missingallowed-teamsschema property with description noting theread:orgrequirement (used by editor autocomplete)Usage
Team members are fetched at runtime (
GET /orgs/{org}/teams/{team_slug}/members, up to 1,000 members across paginated requests). Bot members are excluded.allowed-teamsis applied independently ofallow-team-members.