Skip to content

ci: don't flag private org members as external contributors #2546

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
teeohhem merged 1 commit into from
Jun 29, 2026
Merged

ci: don't flag private org members as external contributors #2546

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
28 changes: 22 additions & 6 deletions .github/workflows/external-contributor-alerts.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,18 +46,34 @@ jobs:
if (item.draft) { core.info(`Skipping draft PR #${item.number}`); return; }
// Skip bots (Dependabot, etc.).
if (item.user?.type === 'Bot') { core.info(`Skipping bot ${item.user?.login}`); return; }
// External == author has no write association with the repo.
const INTERNAL = ['OWNER', 'MEMBER', 'COLLABORATOR'];
if (INTERNAL.includes(item.author_association)) {
core.info(`#${item.number} by ${item.user?.login} is internal (${item.author_association}); skipping.`);
// External == author has no write access to the repo.
const { owner, repo } = context.repo;
// Fast path (free): public members & direct collaborators.
let internal = ['OWNER', 'MEMBER', 'COLLABORATOR'].includes(item.author_association);
// Slow path: catches PRIVATE org members via their granted repo access.
// author_association only reports MEMBER for *public* members, so a
// member with concealed membership would otherwise be misflagged as
// external. Public repos grant everyone implicit 'read', so require
// triage+ (not permission != 'none').
if (!internal) {
try {
const { data } = await github.rest.repos.getCollaboratorPermissionLevel({
owner, repo, username: item.user.login,
});
Comment on lines +60 to +62

@greptile-apps greptile-apps 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.

P2 Every other reference to item.user in this file uses optional chaining (item.user?.login, item.user?.type), but this call site accesses item.user.login without it. If item.user is somehow null (e.g. a deleted account triggering an event), this throws a TypeError inside the try/catch; since a TypeError has no .status, e.status !== 404 is true and the error is re-thrown, failing the workflow run rather than quietly passing. Using item.user?.login is consistent with the rest of the file and avoids the hard throw.

Suggested change
const { data } = await github.rest.repos.getCollaboratorPermissionLevel({
owner, repo, username: item.user.login,
});
const { data } = await github.rest.repos.getCollaboratorPermissionLevel({
owner, repo, username: item.user?.login,
});

Fix in Claude Code Fix in Conductor Fix in Cursor Fix in Codex

const p = data.user?.permissions ?? {};
internal = !!(p.admin || p.maintain || p.push || p.triage);
Comment on lines +63 to +64

@greptile-apps greptile-apps 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.

P1 The permission check relies on data.user?.permissions, but the GitHub API documents data.user as nullable and user.permissions.triage / user.permissions.maintain as optional fields. If data.user is null, or if those individual fields are absent, p falls back to {} and all boolean tests return false — meaning a private org member with a triage or maintain role would still be classified as external, silently defeating the whole fix. The top-level data.permission string (e.g. "triage", "write", "maintain", "admin") is marked required by the GitHub schema and is far more reliable for this check. Note: the API returns "write" (not "push") for the top-level string, so the allowed-set should use "write" instead of "push".

Suggested change
const p = data.user?.permissions ?? {};
internal = !!(p.admin || p.maintain || p.push || p.triage);
const TRIAGE_PLUS = ['triage', 'write', 'maintain', 'admin'];
internal = TRIAGE_PLUS.includes(data.permission);

Fix in Claude Code Fix in Conductor Fix in Cursor Fix in Codex

} catch (e) {
if (e.status !== 404) throw e; // 404 = not a collaborator => external
}
}
if (internal) {
core.info(`#${item.number} by ${item.user?.login} is internal; skipping.`);
return;
}
// The `external` label doubles as our "already alerted" marker, so a
// reopen (label persists) won't post to Slack twice.
if (item.labels?.some(l => l.name === LABEL)) { core.info(`#${item.number} already handled.`); return; }

const { owner, repo } = context.repo;

// Deliver the alert FIRST. We only mark the item handled (label it)
// after a confirmed send, so an unset/failing webhook leaves it
// unlabeled and a later event (reopen / ready_for_review) retries,
Expand Down
Loading