Skip to content

feat: handle GitHub invitation provisioning lifecycle (IGA-1212)#139

Open
highb wants to merge 4 commits intomainfrom
highb/iga-1212-github-invite-action-required
Open

feat: handle GitHub invitation provisioning lifecycle (IGA-1212)#139
highb wants to merge 4 commits intomainfrom
highb/iga-1212-github-invite-action-required

Conversation

@highb
Copy link
Copy Markdown

@highb highb commented Apr 7, 2026

Summary

Fixes the GitHub org invitation provisioning flow to properly handle the async invitation lifecycle across all GitHub product tiers (Free, Team, Enterprise Cloud with personal accounts, EMU).

The core problem (IGA-1212): CreateAccount returned SuccessResult after creating an org invitation, causing C1 to immediately attempt team provisioning. But GitHub invitations are async — the user must accept before they're an org member, so team membership operations fail with "only users can be granted to team membership."

Changes

  • Remove TRAIT_USER from invitation resource type — Invitations are transient pending actions, not identities. TRAIT_USER caused the C1 uplift pipeline to create fake AppUsers with invitation-typed SourceConnectorIds that broke downstream grant operations.

  • Return ActionRequiredResult instead of SuccessResult for new invitations, signaling to C1 that the user must accept before further provisioning can proceed.

  • Handle "already a member" (422) — Return AlreadyExistsResult with the user resource when possible (looked up by GitHub username or email), or with nil resource to let C1 resolve from its DB where sync already created the AppUser.

  • Handle "already invited" (422) — Return ActionRequiredResult with the pending invitation resource, matching by login or email.

  • Add github_username schema field — Optional field for reliable user lookup when email is private. GitHub usernames are the only reliable key since many users don't have public emails. C1 can map this from subject.github_username via CEL.

  • Detect EMU orgs — Enterprise Managed Users orgs don't support the invitation API (accounts are provisioned by the IdP via SCIM). Returns a clear error instead of a confusing 422. Checked after already-a-member/already-invited since those are success cases regardless of org type.

  • Detect expired/failed invitations — Checks ListFailedOrgInvitations before creating a new invite. If a previous invite expired (7-day TTL), logs a warning with the failure reason and timestamp, then proceeds to send a new invitation.

  • Match GitHub's actual error messageisAlreadyOrgMemberError now matches "already a part of" in addition to "already a member" (GitHub's actual 422 message is "A user with this email address is already a part of this organization").

Lookup strategy

lookupUser(login, email) resolves a GitHub user to a resourceTypeUser resource:

  1. If login (GitHub username) is provided, calls GET /users/{username} — works regardless of email privacy
  2. Falls back to GET /search/users?q={email}+in:email — only finds public emails
  3. If both fail, returns nil resource and lets C1 resolve from its DB

lookupPendingInvitation and lookupFailedInvitation both use invitationMatches which checks login first, then email.

GitHub product tier coverage

Product Invites? Handled?
Free / Team Yes (7-day TTL) ✅ ActionRequiredResult
Enterprise Cloud (personal) Yes (7-day TTL) ✅ ActionRequiredResult
Enterprise Cloud (EMU) No (SCIM direct) ✅ Clear error
Enterprise Server No (JIT/SCIM) N/A (different code path)

Test plan

  • Create invitation for new user → returns ActionRequiredResult with invitation resource
  • Create invitation for existing org member → returns AlreadyExistsResult
  • Create invitation for user with pending invite → returns ActionRequiredResult with pending invite
  • Create invitation after previous invite expired → logs warning, sends new invite
  • Create invitation on EMU org → returns clear error
  • Verify invitation resources don't create AppUsers (no TRAIT_USER)
  • Verify github_username field enables lookup when email is private

🤖 Generated with Claude Code

@highb highb requested a review from a team April 7, 2026 21:20
@linear
Copy link
Copy Markdown

linear bot commented Apr 7, 2026

@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Apr 7, 2026

Claude finished @highb's task in 1m 4s —— View job


PR Review: Return ActionRequiredResult for org invites

  • Read changed files and gather context
  • Analyze the diff against origin/main
  • Post review feedback

Overall: This is a clean, well-scoped fix. The change correctly addresses the semantic mismatch where SuccessResult was being returned for an operation (org invite) that isn't actually complete until the user accepts.

What changed

  1. Success path (invitation.go:169-172): SuccessResultActionRequiredResult with a clear message and the invitation resource attached.
  2. Already-a-member error (invitation.go:146-148): Returns AlreadyExistsResult on 422 "already a member".
  3. Already-invited error (invitation.go:149-153): Returns ActionRequiredResult with a pending message on 422 "already invited".
  4. Two new helper functions (isAlreadyOrgMemberError, isAlreadyInvitedError): Parse GitHub ErrorResponse for specific 422 messages.

Feedback

Looks good — no blocking issues. A few observations:

  1. Error matching is reasonably defensive. Checking both ghErr.Message and ghErr.Errors[].Message with case-insensitive matching is good practice for GitHub API error parsing. The string patterns ("already a member", "already invited", "already been invited") cover the known error messages.

  2. AlreadyExistsResult without a Resource (invitation.go:147): The AlreadyExistsResult struct has a Resource field, but it's left nil here. This is fine since we don't have the existing member's resource from the error response — just noting it for awareness in case the C1 engine ever requires a resource on AlreadyExistsResult.

  3. ActionRequiredResult for already-invited (invitation.go:150-152): No Resource is set here either. This differs from the success path which attaches the invitation resource. This makes sense since we don't get the existing invitation back from the 422 error, but worth confirming the C1 engine handles a nil Resource on ActionRequiredResult gracefully.

  4. Idempotency is solid. Both 422 cases return nil error per the CLAUDE.md pattern for grant idempotency, which is correct — these are expected states, not failures.

  5. Messages are clear and actionable. Both "User must accept the existing invitation" and "User must accept the invitation before team membership can be granted" give good context for operators.

LGTM — this is a well-targeted fix for the root cause described in the PR summary. The C1 engine will no longer treat an invitation as a completed account creation, which should resolve the downstream team membership grant failures.

})
if err != nil {
if isAlreadyOrgMemberError(err, resp) {
return &v2.CreateAccountResponse_AlreadyExistsResult{}, nil, nil, nil
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you need to return the resource as part of the response so c1 caches this value?

@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Apr 9, 2026

Claude finished @highb's task in 2m 1s —— View job


PR Review: Return ActionRequiredResult for org invites

  • Read changed files and gather full context
  • Analyze the diff against origin/main
  • Address reviewer feedback (resource in AlreadyExistsResult/ActionRequiredResult)
  • Post review feedback

Overall: Well-structured PR that correctly fixes a semantic mismatch. The core change — returning ActionRequiredResult instead of SuccessResult for org invites — is correct and prevents the C1 engine from treating an invite as completed account creation. The additional handling for already-member, already-invited, EMU org, and expired/failed invitations is thorough.

What looks good

  1. Reviewer feedback addressed. @luisina-santos' comment about returning the resource in responses has been addressed — lookupUser, lookupPendingInvitation, and lookupFailedInvitation now populate the Resource field in all result types.

  2. Error classification is solid. The isAlreadyOrgMemberError, isAlreadyInvitedError, and isEMUOrgError helpers correctly check both ghErr.Message and ghErr.Errors[].Message with case-insensitive matching, covering known GitHub API error variations.

  3. Idempotency is correct. Both 422 cases return nil error, consistent with the CLAUDE.md pattern for grant idempotency.

  4. Rate limit wait plumbing (waitForRateLimitwithRateLimitWait) is clean, using go-github's built-in SleepUntilPrimaryRateLimitResetWhenRateLimited context key.

  5. Login field in account creation schema — good addition for reliable user lookup when email is private.

Issues to consider

1. Failed invitation check blocks re-invitation (potential blocker)

invitation.go:149-164lookupFailedInvitation runs before CreateOrgInvitation. If a previous invitation expired, CreateAccount returns ActionRequiredResult with the message "A new invitation must be sent" — but it never actually sends one. This creates a dead end: the message says a new invite is needed, but the code path returns early before creating one.

If the intent is to surface the expired invite so an operator can take manual action, this makes sense — but the message should be clearer (e.g., "Manual intervention required: previous invitation expired"). If the intent is that C1 should retry, this blocks retries permanently since the failed invitation stays in the list.

Question: Is the expected C1 engine behavior to surface this to an operator, or should this fall through to actually create a new invitation?

2. lookupFailedInvitation iterates all pages without bounds

invitation.go:326-344 — Both lookupPendingInvitation and lookupFailedInvitation paginate through all results with PerPage: 100. For orgs with a long history of failed invitations, this could be slow and consume rate limit. The failed invitations endpoint in particular can accumulate over time. Consider adding a page limit or only checking the first few pages. Not a blocker, but worth considering for large orgs.

3. Error from invitationToUserResource silently dropped

invitation.go:156failedResource, _ := invitationToUserResource(failedInv) discards the error. If the conversion fails, failedResource is nil and the ActionRequiredResult is returned with a nil Resource. This is probably fine in practice (the invitation has all needed fields), but logging the error at Debug level would be more defensive:

failedResource, convErr := invitationToUserResource(failedInv)
if convErr != nil {
    l.Debug("failed to convert failed invitation to resource", zap.Error(convErr))
}

4. lookupUser search query isn't quoted

invitation.go:294email+" in:email" passes the email directly into the GitHub search query. If the email contains special characters (e.g., + tags like user+tag@example.com), GitHub's search parser may misinterpret it. Consider quoting: fmt.Sprintf("%q in:email", email). Minor edge case.

5. Info-level log in CreateAccount

invitation.go:142l.Info("CreateAccount called", ...) logs the full account profile at Info level on every invocation. This is fine for debugging but could be noisy in production. Consider Debug level.

Summary

The core fix is correct and well-motivated. The reviewer feedback about including resources has been addressed across all result paths. The main question is about the failed invitation pre-check (issue #1) — whether it intentionally blocks re-invitation or should fall through. Everything else is minor polish.

LGTM with the caveat on the failed invitation flow.

@highb highb force-pushed the highb/iga-1212-github-invite-action-required branch from add921d to b1a6608 Compare April 9, 2026 23:16
@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Apr 9, 2026

Claude finished @highb's task in 2m 9s —— View job


PR Review: Return ActionRequiredResult for org invites

  • Read changed files and gather full context
  • Analyze the diff against origin/main
  • Post review feedback

Overall: The core fix is correct and well-targeted. Returning ActionRequiredResult instead of SuccessResult for org invites properly communicates the invite's async nature to the C1 engine. The reviewer feedback about including resources in results has been addressed. Error classification helpers are defensively written. Good work.

What looks good

  1. Core semantic fix (invitation.go:204-207): SuccessResultActionRequiredResult with a clear message and the invitation resource — this directly fixes the root cause where C1 treated an invite as completed account creation.

  2. Idempotency is correct (invitation.go:171-188): Both the "already a member" and "already invited" 422 cases return nil error per the CLAUDE.md pattern. AlreadyExistsResult for existing members and ActionRequiredResult for pending invites are the right semantic choices.

  3. Error classification helpers (invitation.go:355-419): Checking both ghErr.Message and ghErr.Errors[].Message with case-insensitive matching is good practice. The string patterns cover known GitHub API error variations well.

  4. lookupUser fallback chain (invitation.go:280-299): Trying login via Users.Get first (bypasses email privacy), then falling back to email search — practical and well-documented.

  5. github_username schema field (connector.go:168-177): Good addition — GitHub email search is unreliable when users have private emails. Making it optional with clear docs is the right call.

Issues

1. Failed invitation pre-check creates a dead end (blocking concern)

invitation.go:147-162 — If lookupFailedInvitation finds an expired invite, CreateAccount returns ActionRequiredResult with "A new invitation must be sent" — but it never sends one. The code returns early before calling CreateOrgInvitation.

This creates a permanent blocker: C1 sees ActionRequiredResult, the message says a new invite is needed, but no subsequent call will get past this check since the failed invitation remains in the list forever.

Question: Is this intentional (surface to operator for manual action)? If so, the message should say something like "Manual intervention required" instead of "A new invitation must be sent" — currently it reads like the system should retry. If the intent is to allow re-invitation, this check should be moved after the CreateOrgInvitation call or removed entirely (GitHub will create a new invite regardless of past failures).

Discuss this →

2. lookupFailedInvitation error handling has a logic gap

invitation.go:149-153:

failedInv, failedErr := i.lookupFailedInvitation(...)
if failedErr != nil {
    l.Debug("failed to check for expired invitations", zap.Error(failedErr))
}
if failedErr == nil && failedInv != nil {

When no failed invitation is found, lookupFailedInvitation returns (nil, error) with the "no failed invitation found" message. This means failedErr is always non-nil when there's no match — the failedErr == nil && failedInv != nil condition only triggers on a successful match. This works correctly, but the Debug log at line 151 fires on every normal invocation (no failed invite = "error"). Consider using a sentinel error or returning (nil, nil) when no match is found to avoid noisy debug logs.

3. Info-level log on every CreateAccount call

invitation.go:140:

l.Info("CreateAccount called", zap.Any("account_info_profile", accountInfo.GetProfile().AsMap()))

This logs the full account profile (including email) at Info level on every invocation. Should be Debug for production. The profile may contain PII.

Fix this →

4. invitationToUserResource error silently dropped

invitation.go:154:

failedResource, _ := invitationToUserResource(failedInv)

If the conversion fails, failedResource is nil and the ActionRequiredResult goes out with a nil Resource. Log the error at Debug level for observability:

failedResource, convErr := invitationToUserResource(failedInv)
if convErr != nil {
    l.Debug("failed to convert expired invitation to resource", zap.Error(convErr))
}

5. Email search query isn't quoted

invitation.go:292:

result, _, err := i.client.Search.Users(ctx, email+" in:email", nil)

Emails with special characters (user+tag@example.com, dots, etc.) could be misinterpreted by GitHub's search parser. Consider quoting: fmt.Sprintf("%q in:email", email). Minor edge case but worth hardening.

6. Unbounded pagination in lookup helpers

invitation.go:303-321 and invitation.go:324-342 — Both lookupPendingInvitation and lookupFailedInvitation paginate through all results. For orgs with long invitation histories (especially failed invitations which accumulate over time), this could be slow and consume rate limits. Consider adding a page limit (e.g., 5 pages) — if the invitation isn't in the first 500 results, it's unlikely to be found.

Minor notes

  • connector.go changes — The enterprise licenses guard (enterpriseLicensesAvailable) and improved debug messages are clean, sensible improvements that are orthogonal to the main PR but reasonable to include.

  • Error prefix inconsistency — The PR uses both github-connector: and github-connectorv2: prefixes. The existing codebase uses both, so this isn't new debt, but worth noting.

Summary

The core fix (ActionRequiredResult for invites) is correct and important. The main concern is the failed invitation pre-check (issue #1) which may create a permanent blocker for re-invitation flows. Issues #3-#6 are minor improvements. LGTM once the failed invitation flow intent is clarified.


// Before attempting to create an invitation, check if a previous invite
// expired. If so, return a distinct error so C1 can surface it in the UI.
failedInv, failedErr := i.lookupFailedInvitation(ctx, params.org, params.login, *params.email)
Copy link
Copy Markdown
Contributor

@Bencheng21 Bencheng21 Apr 9, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hm, github allows us to override the previous invitation. I don't think we should return an error if the invitation exists.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, just caught that. I'll warn if it detects a failed invite and then resend.

highb and others added 4 commits April 10, 2026 12:26
Invitations are transient pending actions, not identities. Having
TRAIT_USER caused the C1 uplift pipeline to create fake AppUsers with
invitation-typed SourceConnectorIds that broke downstream grant
operations. Invitations should only exist as pending state until the
user accepts and appears as a real user resource via sync.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Rewrite CreateAccount to properly handle the async invitation
lifecycle across GitHub product tiers (Free, Team, Enterprise Cloud
with personal accounts):

- Return ActionRequiredResult (not SuccessResult) for new invites,
  since the user must accept before team membership can be granted
- Detect "already a member" (422) and return AlreadyExistsResult with
  the user resource when possible, or nil resource to let C1 resolve
  from its DB
- Detect "already invited" (422) and return ActionRequiredResult with
  the pending invitation resource
- Add github_username as an optional schema field for reliable user
  lookup when email is private
- Add lookupUser (tries Users.Get by username, falls back to email
  search) and lookupPendingInvitation (matches by login or email)
- Match GitHub's actual error message "already a part of" in addition
  to "already a member"

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Enterprise Managed Users (EMU) orgs don't support the invitation API
— accounts are provisioned directly by the IdP via SCIM. Detect the
EMU-specific 422 error and return a clear message instead of a
confusing generic failure.

The EMU check runs after already-a-member and already-invited checks
since those are success cases regardless of org type.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Check ListFailedOrgInvitations before calling CreateOrgInvitation.
If a matching expired/failed invite is found, log a warning with the
failure reason and timestamp, then proceed to send a new invitation.
This provides an audit trail of expired invites without blocking
re-invitations.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@highb highb force-pushed the highb/iga-1212-github-invite-action-required branch from b1a6608 to 20d23fc Compare April 10, 2026 19:32
@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Apr 10, 2026

Claude finished @highb's task in 2m 45s —— View job


PR Review: Return ActionRequiredResult for org invites

  • Read changed files and gather full context
  • Analyze the diff against origin/main
  • Address reviewer feedback from Bencheng21 and luisina-santos
  • Post review feedback

Overall: The core fix is correct and important. Returning ActionRequiredResult instead of SuccessResult for org invites properly communicates the async nature of invitations to the C1 engine, directly fixing the root cause where downstream team membership grants fail because C1 treats the invite as a completed account creation.

What changed (4 commits)

  1. c90711a — Remove TRAIT_USER from invitation resource type (invitations aren't users)
  2. 0e0af0d — Core fix: SuccessResultActionRequiredResult, plus AlreadyExistsResult / ActionRequiredResult for 422 cases, EMU org detection, github_username schema field, and user/invitation lookup helpers
  3. 6f7443f — EMU org error detection
  4. 20d23fc — Failed invitation pre-check: now logs and proceeds to create new invite (addresses the "dead end" concern from previous reviews)

What looks good

  1. Core semantic fix (invitation.go:200-203): ActionRequiredResult with a clear message and the invitation resource is the right choice — invites are async.

  2. Idempotency is correct (invitation.go:164-185): Both 422 cases return nil error per CLAUDE.md patterns. AlreadyExistsResult for existing members and ActionRequiredResult for pending invites are semantically correct.

  3. Failed invitation handling fixed (invitation.go:147-158): The latest commit addresses the "dead end" concern raised in previous reviews — the failed invitation is now logged as a warning and the code proceeds to create a new invite. Good fix.

  4. Error classification helpers (invitation.go:351-415): Defensively check both ghErr.Message and ghErr.Errors[].Message with case-insensitive matching, covering known GitHub API error variations.

  5. lookupUser fallback chain (invitation.go:276-296): Trying login via Users.Get first (bypasses email privacy), then falling back to email search — practical approach with good documentation.

  6. github_username schema field (connector.go:166-176): Good addition since GitHub email search is unreliable with private emails. Optional with clear docs.

  7. Resource included in all result paths: Addresses @luisina-santos' feedback — lookupUser and lookupPendingInvitation populate the Resource field so C1 can cache it.

Issues

1. @Bencheng21's concern about already-invited handling

invitation.go:172-181@Bencheng21 noted that GitHub allows overriding previous invitations, questioning whether we should return an error (well, ActionRequiredResult) when one already exists.

The current behavior returns ActionRequiredResult with nil error, which is not a failure — it tells C1 the user must accept the pending invite. This seems correct if the goal is to surface that the invitation is pending. But if GitHub's API supports overriding invitations (e.g., via InviteeID instead of Email in CreateOrgInvitationOptions), it might be worth trying that approach first so the invite gets refreshed rather than just returning "already pending."

Worth clarifying: Can the invite be re-sent via the API to reset the expiry timer? If so, that might be more useful than just reporting the existing pending state.

2. lookupFailedInvitation returns error for normal "not found" case

invitation.go:319-338 — When no failed invitation exists (the common case), lookupFailedInvitation returns (nil, error) with "no failed invitation found." This means the Debug log at invitation.go:151 fires on every normal CreateAccount call:

if failedErr != nil {
    l.Debug("failed to check for expired invitations", zap.Error(failedErr))
}

The message "failed to check for expired invitations" is misleading — it's not a failure, just no match. Consider returning (nil, nil) when no match is found, and reserving errors for actual API failures. Same applies to lookupPendingInvitation at invitation.go:298-317.

Fix this →

3. Info-level log with PII on every invocation

invitation.go:140:

l.Info("CreateAccount called", zap.Any("account_info_profile", accountInfo.GetProfile().AsMap()))

This logs the full account profile (including email) at Info level on every CreateAccount call. Should be Debug for production — the profile may contain PII.

Fix this →

4. Unbounded pagination in lookup helpers

invitation.go:300-316 and invitation.go:321-337 — Both lookupPendingInvitation and lookupFailedInvitation paginate through all results with PerPage: 100. For orgs with long invitation histories (especially failed invitations which accumulate), this could be slow and consume rate limit budget. Consider adding a page limit (e.g., 5 pages = 500 invitations) or returning early after a reasonable threshold.

5. Email search query isn't quoted

invitation.go:288:

result, _, err := i.client.Search.Users(ctx, email+" in:email", nil)

Emails with special characters (e.g., user+tag@example.com) could be misinterpreted by GitHub's search parser. Consider quoting: fmt.Sprintf("%q in:email", email). Minor edge case but worth hardening since this is a provisioning path.

Minor observations

  • Error prefix inconsistency: The PR uses both github-connector: and github-connectorv2: prefixes. Existing codebase uses both, so not new debt, but worth noting.
  • lookupUser silently drops Users.Get error (invitation.go:278-285): When the login lookup fails, the error is discarded before falling through to email search. A Debug log would help with troubleshooting.

Summary

The core fix (ActionRequiredResult for invites) is correct, well-targeted, and important. The failed invitation handling has been improved to no longer block re-invitations. Issues #2-#5 are minor improvements. Issue #1 (Bencheng21's concern) warrants discussion about whether re-sending the invite is preferable to returning "already pending."

LGTM with minor suggestions.

@highb highb changed the title fix: return ActionRequiredResult for org invites instead of SuccessResult feat: handle GitHub invitation provisioning lifecycle (IGA-1212) Apr 10, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants