feat(safe-outputs): add allowed-teams to mentions configuration
#40368
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -101,6 +101,70 @@ function extractKnownAuthorsFromPayload(context) { | |
| return users; | ||
| } | ||
|
|
||
| /** | ||
| * Fetch members of a GitHub team and return their logins. | ||
| * Accepts "team-slug" (resolved against the current org) or "org/team-slug" format. | ||
| * Failures are non-fatal: a warning is logged and an empty array is returned. | ||
| * @param {string} teamEntry - Team identifier, e.g. "my-team" or "myorg/my-team" | ||
| * @param {string} defaultOrg - The org to use when no org is specified in teamEntry | ||
| * @param {any} github - GitHub API client | ||
| * @param {any} core - GitHub Actions core | ||
| * @returns {Promise<string[]>} Array of member logins (non-bot), empty on any failure | ||
| */ | ||
| 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); | ||
| } | ||
|
Comment on lines
+114
to
+123
|
||
|
|
||
| if (!org || !teamSlug) { | ||
|
Contributor
There was a problem hiding this comment.
💡 Suggested fixAdd 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 |
||
| core.warning(`[MENTIONS] Skipping invalid team entry: "${teamEntry}"`); | ||
| return []; | ||
| } | ||
|
|
||
| try { | ||
| 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++; | ||
| } | ||
|
Comment on lines
+131
to
+148
Contributor
There was a problem hiding this comment. Silent truncation at 1000 members emits no warning: when a team has >1000 members the 💡 Suggested fixAdd 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 |
||
|
|
||
| core.info(`[MENTIONS] Fetched ${logins.length} member(s) from team ${org}/${teamSlug}`); | ||
|
Contributor
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 — 💡 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 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);
}); |
||
| return logins; | ||
| } catch (error) { | ||
| const status = /** @type {any} */ error?.status; | ||
| const isRateLimit = status === 429 || (status === 403 && /rate.?limit/i.test(getErrorMessage(error))); | ||
| const isPermission = !isRateLimit && (status === 403 || status === 404); | ||
|
|
||
| if (isRateLimit) { | ||
| core.warning(`[MENTIONS] Rate limit reached while fetching team ${org}/${teamSlug} members - skipping team (retry later or reduce team count)`); | ||
| } else if (isPermission) { | ||
| core.warning(`[MENTIONS] Cannot access team ${org}/${teamSlug} (HTTP ${status}) - ensure the token has 'read:org' scope and the team exists`); | ||
| } else { | ||
| core.warning(`[MENTIONS] Failed to fetch members for team ${org}/${teamSlug}: ${getErrorMessage(error)}`); | ||
| } | ||
| return []; | ||
| } | ||
| } | ||
|
|
||
| /** | ||
| * Resolve allowed mentions from the current GitHub event context | ||
| * @param {any} context - GitHub Actions context | ||
|
|
@@ -126,6 +190,7 @@ async function resolveAllowedMentionsFromPayload(context, github, core, mentions | |
| const allowTeamMembers = mentionsConfig?.allowTeamMembers !== false; // default: true | ||
| const allowContext = mentionsConfig?.allowContext !== false; // default: true | ||
| const allowedList = mentionsConfig?.allowed || []; | ||
| const allowedTeams = mentionsConfig?.allowedTeams || []; | ||
| const maxMentions = mentionsConfig?.max || 50; | ||
|
|
||
| try { | ||
|
|
@@ -137,6 +202,17 @@ 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) | ||
| if (Array.isArray(allowedTeams) && allowedTeams.length > 0) { | ||
| core.info(`[MENTIONS] Fetching members for ${allowedTeams.length} configured team(s)`); | ||
| for (const teamEntry of allowedTeams) { | ||
|
Contributor
There was a problem hiding this comment. Sequential team fetches block workflow execution for O(N × pages) API calls: all 💡 Suggested fixParallelize at the team level with 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.
Contributor
There was a problem hiding this comment. [/zoom-out] Team members are fetched sequentially inside a 💡 Suggested refactorif (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. |
||
| if (typeof teamEntry === "string" && teamEntry.length > 0) { | ||
| const teamMembers = await fetchTeamMembers(teamEntry, owner, github, core); | ||
| knownAuthors.push(...teamMembers); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| // Add extra known authors (e.g. pre-fetched target issue authors for explicit item_number) | ||
| if (extraKnownAuthors && extraKnownAuthors.length > 0) { | ||
| core.info(`[MENTIONS] Adding ${extraKnownAuthors.length} extra known author(s): ${extraKnownAuthors.join(", ")}`); | ||
|
|
@@ -192,6 +268,7 @@ async function resolveAllowedMentionsFromPayload(context, github, core, mentions | |
| module.exports = { | ||
| resolveAllowedMentionsFromPayload, | ||
| extractKnownAuthorsFromPayload, | ||
| fetchTeamMembers, | ||
| pushNonBotUser, | ||
| pushNonBotAssignees, | ||
| }; | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[/grill-with-docs] The Go parser strips leading
@fromallowed-teamsentries before emitting them into the JS config, so in practice this function never receives"@org/team". However,fetchTeamMembersis 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)
This makes the JS function self-consistent regardless of call site, mirrors what
parseMentionsConfigalready does on the Go side, and prevents a subtle silent failure in future tests or integrations that callfetchTeamMembersdirectly.