Repository navigation
fix: make account creation idempotent and surface Linear API errors - #62
russellhaering wants to merge 3 commits into
Conversation
CreateAccount always sent a workspace invite, which Linear rejects when the user is already a member or already has a pending invite. Retries of a create that had already succeeded therefore failed with an opaque "success=false" error. - Look up the user by email first and return AlreadyExists for active users; fail with a clear message for suspended users. - Return ActionRequired when an unexpired invite is already pending. - Include Linear's GraphQL error message when an invite fails. - Only treat a 400 as rate limiting when Linear says so (RATELIMITED error code or an exhausted rate-limit header), so invalid requests are no longer retried as if they were rate limited.
Superseded — see the current review report for commit
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| return nil, err | ||
| } | ||
|
|
||
| if len(res.Data.Users.Nodes) == 0 { |
There was a problem hiding this comment.
🟡 Suggestion (medium confidence): GraphQLUsersResponse has no errors field. If Linear answers this lookup with HTTP 200 plus GraphQL errors (for example a resolver or permission error), nodes comes back empty. The user is then reported as not found, and CreateAccount sends a duplicate invite, which skips the idempotency check this PR adds. GetPendingOrganizationInvite has the same gap. Consider embedding GraphQLError in both lookup responses and returning an error when HasErrors() is true.
There was a problem hiding this comment.
Fixed in 675b981: both lookups now embed GraphQLError and return an error when HasErrors() is true, so a failed lookup no longer looks like "not found". Added tests for each.
| if !res.Data.OrganizationInvites.PageInfo.HasNextPage { | ||
| return nil, nil | ||
| } | ||
| after = res.Data.OrganizationInvites.PageInfo.EndCursor |
There was a problem hiding this comment.
🟡 Suggestion (low confidence): if Linear ever returns hasNextPage: true with an empty endCursor, after resets to "" and the loop refetches page 1 until the context is cancelled. Guard with if after == "" { return nil, error }. Also note that every CreateAccount now pages through the workspace's entire invite history, accepted invites included. That cost grows over time, so a server-side filter would help if the API supports one.
There was a problem hiding this comment.
Fixed in 675b981: pagination now returns an error if endCursor is empty or unchanged. On cost: Linear's organizationInvites query takes no filter argument, so a client-side scan is the only option. It runs only on account creation, not during sync.
| return resp, rlData, uhttp.WrapErrorsWithRateLimitInfo(codes.Unavailable, resp, err) | ||
| } | ||
|
|
||
| if gqlErr.HasErrors() { |
There was a problem hiding this comment.
🟡 Suggestion (medium confidence): uhttp.WithErrorResponse (vendor pkg/uhttp/wrapper.go:531-563) already returns "Request failed with status %d: " + resource.Message(), and Do carries that into err. Appending gqlErr.Message() here prints the message twice, e.g. ...status 400: Argument Validation Error: Argument Validation Error. Consider dropping this append; non-rate-limit HTTP errors already included Linear's message before this PR.
There was a problem hiding this comment.
Fixed in 675b981: removed the extra wrap, since WithErrorResponse already includes the message. The existing TestDoRequestBadRequest still checks that Linear's message appears in the error.
Superseded — see the current review report for commit
|
There was a problem hiding this comment.
No blocking issues found — see the full review report
…ation Return an error when the user or invite lookup carries GraphQL errors instead of treating it as not found, stop invite pagination if the cursor does not advance, and drop the duplicated error message in doRequest.
Superseded — see the current review report for commit
|
| } | ||
| if existing != nil { | ||
| if !existing.Active { | ||
| return nil, nil, nil, fmt.Errorf("baton-linear: %s already has a suspended Linear account, which must be reactivated rather than invited", email) |
There was a problem hiding this comment.
🟡 Suggestion (medium confidence): this provisioning error has no gRPC status code, so it comes through as Unknown (criteria P4/E3). Retrying won't fix it until someone reactivates the account, so status.Errorf(codes.FailedPrecondition, ...) would show the failure correctly in C1.
There was a problem hiding this comment.
Fixed in 1fe4c2a: now returns codes.FailedPrecondition, and TestUserCreateAccount_SuspendedUser asserts the code.
Superseded — see the current review report for commit
|
There was a problem hiding this comment.
No blocking issues found — see the full review report
Superseded — see the current review report for commit
|
Connector PR Review: fix: make account creation idempotent and surface Linear API errorsBlocking Issues: 0 | Suggestions: 0 | Threads Resolved: 0 Review SummaryBefore inviting, The new commit ( I rescanned the whole PR diff for security and correctness. In I applied the repo criteria:
The incremental diff was complete ( Security IssuesNone found. Correctness IssuesNone found. SuggestionsNone. Resolved prior findings
Reviewed commit: |
There was a problem hiding this comment.
No blocking issues found — see the full review report
Summary
Account provisioning always sent a workspace invite. Linear rejects an invite when the user is already a member or already has a pending invite, so a retried or repeated create failed with
organizationInviteCreate returned success=falseand no detail.Changes
CreateAccountlooks the user up by email (case-insensitive, including suspended users) before inviting. An active user is returned asAlreadyExistsResult. A suspended user returns an error explaining that the account must be reactivated.ActionRequiredResultinstead of sending a duplicate.RATELIMITEDerror code, or an exhaustedX-RateLimit-*-Remainingheader). Previously every 400 was converted into a retryable rate-limit error, so invalid requests were retried until timeout.Testing
go test ./pkg/...passes. New tests cover the existing-user, suspended-user, pending-invite and invite-error cases, plus rate-limit vs. invalid-request 400s and the email lookup.usersfiltered by email,organizationInvites) against the live Linear GraphQL schema with a read-only query.golangci-lintlocally; my installed binary is older than this module's Go version, so CI covers lint.