Some changes to the client - #11
Conversation
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| return nil, false, nil | ||
| } | ||
| return nil, false, fmt.Errorf( | ||
| "baton-discord: failed to get member %s of guild %s: %w", userID, guildID, err) |
There was a problem hiding this comment.
🟡 Suggestion: Member is the one method this PR adds and the one that doesn't go through wrapErr, so its failures still reach the SDK as codes.Unknown. Since it's called per-overwrite inside channelBuilder.Grants, a 403 or a 5xx here is exactly the case the new classification was meant to distinguish. Use wrapErr(err, "baton-discord: failed to get member %s of guild %s", userID, guildID).
| if statusCode >= 500 && statusCode < 600 { | ||
| return codes.Unavailable | ||
| } | ||
| return codes.Unknown |
There was a problem hiding this comment.
🟡 Suggestion: httpStatus returns 0 whenever the error carries no rest.Error response, so every transport-level failure — the 60s http.Client{Timeout: requestTimeout} firing, connection resets, DNS/TLS errors, context.DeadlineExceeded, context.Canceled — falls through to codes.Unknown. By this function's own reasoning that leaves the most transient class of failure outside the retryer's Unavailable/DeadlineExceeded set, and turns a cancelled sync into an unknown failure. Consider checking errors.Is(err, context.DeadlineExceeded) → DeadlineExceeded, context.Canceled → Canceled, and mapping a remaining statusCode == 0 to Unavailable.
| "Server Members Intent on the bot application: %w", guildID, err) | ||
| } | ||
| return nil, "", fmt.Errorf("baton-discord: failed to list members of guild %s: %w", guildID, err) | ||
| return nil, "", wrapErr(err, "baton-discord: failed to list members of guild %s", guildID) |
There was a problem hiding this comment.
🟡 Suggestion: the fallthrough on the line below moved to wrapErr, but the IsForbidden branch just above it still uses plain fmt.Errorf. A missing Server Members Intent is the most likely real-world 403 in this connector, and it now reaches the SDK as codes.Unknown rather than PermissionDenied. Wrapping it while keeping the guidance text preserves both: uhttp.WrapErrors(codes.PermissionDenied, msg, fmt.Errorf(msg+": %w", err)).
| return nil, nil, err | ||
| } | ||
| if !stillAMember { | ||
| continue |
There was a problem hiding this comment.
🟡 Suggestion: this drops a permission overwrite from the sync with no record that it happened. client.IsNotFound matches more than a departed member — it also matches the UnknownGuild/UnknownUser/UnknownChannel JSON codes — so a probe that 404s for a reason other than "left the server" silently removes a real grant, which C1 reads as access revoked. A ctxzap.Extract(ctx).Debug(...) (or Warn) naming the channel and target here would keep the omission attributable.
| return nil | ||
| } | ||
| msg := fmt.Sprintf(format, args...) | ||
| return uhttp.WrapErrors(statusCodeFor(err), msg, fmt.Errorf("%s: %w", msg, err)) |
There was a problem hiding this comment.
🟡 Suggestion: msg is passed twice — once as the WrapErrors message and once more inside the wrapped error — so the rendered error most likely reads "...failed to list roles of guild X: ...failed to list roles of guild X: <cause>". Passing the bare cause (uhttp.WrapErrors(statusCodeFor(err), msg, err)) keeps both the status code and the unwrappability without the duplicated prefix. Worth confirming against the actual output before changing.
| "is_bot": member.User.Bot, | ||
| profileKeyUserID: member.User.ID.String(), | ||
| "username": member.User.Username, | ||
| "is_bot": member.User.Bot, |
There was a problem hiding this comment.
🟡 Suggestion: dropping guild_id from the profile and re-basing created_at on the account snowflake are both sound (nothing in this repo reads either), but they change data C1 already ingested for every user — an attribute mapping or report built on guild_id, or anything reading created_at as a join date, changes meaning on the next sync. The reasoning is in the README, but the PR body is empty and the title is "Some changes to the client"; please state the user-visible change in the PR description so it is visible at deploy time.
Connector PR Review: Some changes to the clientBlocking Issues: 0 | Suggestions: 6 | Threads Resolved: 0 Review SummaryThe only new commit since the last review is Security IssuesNone found. Correctness IssuesNone found. SuggestionsAll six carried over from the previous review — still present at the current head.
Prompt for AI agents |
swiss uses go:linkname into runtime internals, so it gates that file behind a manually bumped build-tag range. The vendored version was tagged (go1.20 && !go1.27), which excluded the file under go1.27 and left hashFn/getRuntimeHasher/fastrand64 undefined at build time. Bump to the pseudo-version tagged (go1.20 && !go1.28) and re-vendor. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
1 participant
No description provided.