Skip to content

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

Merged
teeohhem merged 1 commit into
mainfrom
fix/external-contrib-private-members
Jun 29, 2026
Merged

ci: don't flag private org members as external contributors#2546
teeohhem merged 1 commit into
mainfrom
fix/external-contrib-private-members

Conversation

@teeohhem

Copy link
Copy Markdown
Contributor

Teammates with private (concealed) organization membership are no longer misflagged as external contributors. They previously got a Slack alert and an external label on every issue/PR they opened, because author_association only reports MEMBER for publicly visible members — a private member shows up as CONTRIBUTOR/NONE, which the workflow treated as external. This is what happened on #2545 (author knudtty is a private org member).

The internal check now keeps author_association as a free fast path, then falls back to resolving the author's actual repository permission via repos.getCollaboratorPermissionLevel, which sees access granted through private teams.

Two design points worth noting:

  • Threshold on granted access, not permission !== 'none'. This is a public repo, so GitHub grants every user implicit read. A naive != 'none' check would mark every genuine external contributor as internal. The check requires triage/push/maintain/admin instead.
  • No new secret or permission. The built-in GITHUB_TOKEN can call the endpoint under the existing issues:write / pull-requests:write scope.

Both behaviors were verified in a throwaway Actions run (in a sibling repo with the identical workflow) before writing this: a private member with write access resolved to INTERNAL with no 403, and a non-collaborator (implicit read) correctly resolved to EXTERNAL.


Compound Engineering
Claude Code

author_association only reports MEMBER for *public* org members, so a
teammate with private (concealed) membership was classified as external,
alerted to Slack, and labeled. Resolve actual repo access via the
collaborator-permission endpoint as a fallback. Threshold on granted
access (triage and up) rather than permission != 'none', since public
repos grant everyone implicit read. Uses the built-in GITHUB_TOKEN; no
new secret or permission required.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@changeset-bot

changeset-bot Bot commented Jun 29, 2026

Copy link
Copy Markdown

⚠️ No Changeset found

Latest commit: e4000af

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@vercel

vercel Bot commented Jun 29, 2026

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for GitHub.

2 Skipped Deployments
Project Deployment Actions Updated (UTC)
hyperdx-oss Ignored Ignored Jun 29, 2026 8:55pm
hyperdx-storybook Ignored Ignored Jun 29, 2026 8:55pm

Request Review

@github-actions github-actions Bot added external review/tier-1 Trivial — auto-merge candidate once CI passes labels Jun 29, 2026
@github-actions

Copy link
Copy Markdown
Contributor

🟢 Tier 1 — Trivial

Docs, images, lock files, or a dependency bump. No functional code changes detected.

Why this tier:

  • All files are docs / images / lock files

Review process: Auto-merge once CI passes. No human review required.
SLA: Resolves automatically.

Stats
  • Production files changed: 0
  • Production lines changed: 0
  • Branch: fix/external-contrib-private-members
  • Author: teeohhem

To override this classification, remove the review/tier-1 label and apply a different review/tier-* label. Manual overrides are preserved on subsequent pushes.

@teeohhem teeohhem changed the title fix: don't flag private org members as external contributors ci: don't flag private org members as external contributors Jun 29, 2026
@teeohhem teeohhem removed the external label Jun 29, 2026
@teeohhem teeohhem merged commit dae3da5 into main Jun 29, 2026
19 checks passed
@greptile-apps

greptile-apps Bot commented Jun 29, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

This PR fixes false-positive "external contributor" alerts for org members who have concealed (private) membership — their author_association shows as CONTRIBUTOR/NONE rather than MEMBER, so the old check would Slack-alert and label them as external. The fix adds a slow-path call to repos.getCollaboratorPermissionLevel that sees access granted through private teams.

  • The fast path (free, no API call) is preserved for public members, owners, and direct collaborators via author_association.
  • The slow path calls getCollaboratorPermissionLevel only when the fast path misses; a 404 response correctly classifies the user as external, while triage+ access marks them internal.

Confidence Score: 3/5

The slow-path permission check reads from a nested field GitHub documents as nullable/optional rather than the guaranteed top-level field, leaving a silent gap for private members with triage or maintain roles.

The core logic is sound but the permission extraction from data.user?.permissions can silently return false for private org members with triage or maintain roles, defeating the fix.

.github/workflows/external-contributor-alerts.yml — specifically the permission extraction on lines 63-64.

Important Files Changed

Filename Overview
.github/workflows/external-contributor-alerts.yml Adds a slow-path API call to detect private org members; the permission check reads from the optional data.user?.permissions object instead of the always-present top-level data.permission string, which can silently misidentify private members with triage/maintain roles as external contributors.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[Issue / PR event fires] --> B{Draft?}
    B -- Yes --> Z[Skip]
    B -- No --> C{Bot user?}
    C -- Yes --> Z
    C -- No --> D{author_association OWNER/MEMBER/COLLABORATOR?}
    D -- Yes --> E[internal = true]
    D -- No --> F[Call getCollaboratorPermissionLevel]
    F --> G{HTTP 404?}
    G -- Yes --> H[internal = false]
    G -- No --> I{data.user?.permissions triage/push/maintain/admin?}
    I -- Any true --> E
    I -- All false/absent --> H
    F --> J{Other error?}
    J -- Yes --> K[Rethrow - workflow fails]
    E --> L{Already labeled external?}
    L -- Yes --> Z
    L -- No --> M[Post Slack alert]
    H --> L
    M --> N[Add external label]
Loading
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
flowchart TD
    A[Issue / PR event fires] --> B{Draft?}
    B -- Yes --> Z[Skip]
    B -- No --> C{Bot user?}
    C -- Yes --> Z
    C -- No --> D{author_association OWNER/MEMBER/COLLABORATOR?}
    D -- Yes --> E[internal = true]
    D -- No --> F[Call getCollaboratorPermissionLevel]
    F --> G{HTTP 404?}
    G -- Yes --> H[internal = false]
    G -- No --> I{data.user?.permissions triage/push/maintain/admin?}
    I -- Any true --> E
    I -- All false/absent --> H
    F --> J{Other error?}
    J -- Yes --> K[Rethrow - workflow fails]
    E --> L{Already labeled external?}
    L -- Yes --> Z
    L -- No --> M[Post Slack alert]
    H --> L
    M --> N[Add external label]
Loading

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

Reviews (1): Last reviewed commit: "fix: don't flag private org members as e..." | Re-trigger Greptile

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

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

Comment on lines +60 to +62
const { data } = await github.rest.repos.getCollaboratorPermissionLevel({
owner, repo, username: item.user.login,
});

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

@github-actions

Copy link
Copy Markdown
Contributor

E2E Test Results

All tests passed • 222 passed • 3 skipped • 1502s

Status Count
✅ Passed 222
❌ Failed 0
⚠️ Flaky 4
⏭️ Skipped 3

Tests ran across 4 shards in parallel.

View full report →

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

review/tier-1 Trivial — auto-merge candidate once CI passes

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant