Rewrite connector on baton-sdk v0.24.6 and add provisioning - #10
Conversation
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| const ( | ||
| // guildAccessEntitlement is membership of the server itself. | ||
| guildAccessEntitlement = "access" | ||
| // roleMemberEntitlement is assignment of a role. | ||
| roleMemberEntitlement = "member" | ||
| ) |
There was a problem hiding this comment.
🟠 Bug: Breaking change ships ungated. Slug changes for guild/role/channel change every entitlement ID (<type>:<id>:<slug>) and every grant ID derived from them, and the channel grant semantics change at the same time (explicit overwrite Allow bits only, plus text entitlements removed from voice channels), so on first sync after deploy C1 sees mass entitlement/grant churn rather than a rename. The PR description documents this, but repo review criteria require breaking behavior to be opt-in behind a config flag or to carry a migration path — dual-emitting the old slugs alongside the new ones for one release would keep existing grants resolvable while C1 re-maps. If maintainers accept the re-map as intentional for this rewrite, that sign-off should be recorded on the PR.
There was a problem hiding this comment.
Not changing this — recording maintainer sign-off instead, which is the option your comment offers for an intentional re-map.
The slug change was raised with the repo owner before implementation and chosen explicitly, with the alternative (reproducing the display-derived slugs byte-for-byte to preserve existing IDs) presented alongside it. The decision was to take the break.
The reasoning: the previous slugs embedded display names, so entitlement and grant IDs already changed silently whenever a server, role, or channel was renamed. That is the instability this rewrite exists to remove, and dual-emitting both slug sets for a release would double every entitlement and grant in C1 while leaving the unstable identifiers live — trading a one-time, understood re-map for ongoing ambiguity.
The break is documented in the PR body and the commit message, and TestRoleEntitlementIDIsStable now pins the new behavior so IDs cannot drift on rename again.
@loganSaso — flagging explicitly so the sign-off is on the record and you can overrule if you would rather gate it.
— Written by Claude (Opus 5), acting on Logan's request to triage this review.
| } | ||
|
|
||
| return members, nextCursor(len(members), MemberPageSize, func() string { | ||
| return members[len(members)-1].User.ID |
There was a problem hiding this comment.
🟡 Suggestion: members[len(members)-1].User.ID dereferences Member.User without a nil check, while newMemberResource in pkg/connector/users.go:53 treats a nil Member.User as a real condition worth an explicit error ("Discord omits the user object when the bot application lacks the Server Members Intent"). If that premise holds, a full page whose last member has no user object panics here before the guarded path ever runs. Either drop the nil guard in newMemberResource or add one here and skip/error instead of dereferencing.
There was a problem hiding this comment.
Fixed in ea040ca.
Confirmed real: MembersPage dereferenced members[len(members)-1].User.ID for the cursor, while newMemberResource treats a nil Member.User as a genuine condition. A full page ending in such a member panicked before the guarded path could ever run — the exact inconsistency you identified.
The cursor now returns "" when the last member has no user object, ending pagination and letting newMemberResource report the missing-intent cause. Kept the guard rather than dropping it, since a member without a user really is a diagnosable misconfiguration rather than noise.
Regression test: TestMembersPageSurvivesMemberWithoutUser builds a full page whose final entry has no user, asserts the client does not panic and returns an empty cursor, and asserts the syncer surfaces an error naming the intent.
— Written by Claude (Opus 5), acting on Logan's request to triage this review.
| return nil, nil, err | ||
| } | ||
|
|
||
| permission, err := channelPermissionForEntitlement(ent) |
There was a problem hiding this comment.
🟡 Suggestion: channelPermissionForEntitlement resolves against the global channelPermissionsBySlug index, which is not scoped to the channel type. Entitlements only advertises permissionsForChannel(channel.Type), but a stale or hand-constructed grant task carrying e.g. send_messages against a voice channel resolves fine here and writes a text-only bit into the voice channel's overwrite. Consider checking permission.Scope against the fetched channel.Type after the channel read and rejecting the mismatch.
There was a problem hiding this comment.
Fixed in ea040ca, for Grant only.
Grant now checks permissionAppliesTo(permission, channel.Type) after the channel read and refuses a mismatch, so a stale task naming send_messages against a voice channel no longer writes a text bit into the overwrite.
Deliberately not applied to Revoke: if a voice channel somehow carries a text bit, removing it is the desired outcome, and refusing there would leave the connector unable to clean up exactly the state this check exists to prevent.
Regression test: TestChannelGrantRejectsMismatchedPermission asserts the grant is refused and that no PUT reaches the overwrite endpoint.
— Written by Claude (Opus 5), acting on Logan's request to triage this review.
| channel, err := c.client.Channel(ctx, channelID) | ||
| if err != nil { | ||
| return nil, nil, err | ||
| } | ||
|
|
||
| existing := findOverwrite(channel, principal.Id.Resource) | ||
|
|
||
| allow := permission.Value | ||
| var deny int64 | ||
| if existing != nil { | ||
| allow = existing.Allow | permission.Value | ||
| deny = existing.Deny & ^permission.Value | ||
| targetType = existing.Type | ||
| } | ||
|
|
||
| err = c.client.SetChannelOverwrite(ctx, channelID, principal.Id.Resource, targetType, allow, deny, | ||
| "Channel permission granted by ConductorOne") | ||
| if err != nil { | ||
| return nil, nil, err | ||
| } |
There was a problem hiding this comment.
🟡 Suggestion: The read-modify-write on the overwrite has no concurrency guard. Two grants of different permissions to the same principal on the same channel that interleave between the Channel read and the SetChannelOverwrite write will silently drop one of the bits, because each writes the full mask it computed from its own stale read. Discord offers no If-Match on this endpoint, so the practical mitigations are a re-read-and-verify after the write (retry on divergence) or an explicit note that concurrent grants on one channel are unsafe.
There was a problem hiding this comment.
Acknowledged as a known limitation rather than fixed — recorded in the code comment on Grant and in the README's new Known limitations section.
The race is real and your description of it is accurate. I did not add re-read-and-verify because it narrows the window without closing it — the verify is itself a second TOCTOU — while adding a request per grant and a retry path that can thrash. Discord offers no compare-and-set on this endpoint, so there is no correct fix available at this layer.
What makes it tolerable: provisioning tasks for one resource are not run concurrently in practice, and the next sync reports the true state. Documenting that honestly seemed better than machinery that looks like a fix but isn't.
— Written by Claude (Opus 5), acting on Logan's request to triage this review.
| if _, _, err := d.client.MembersPage(ctx, guilds[0].ID, ""); err != nil { | ||
| return nil, err | ||
| } |
There was a problem hiding this comment.
🟡 Suggestion: The member-listing probe only covers guilds[0]. Member listing is gated by both the application-wide Server Members Intent and per-guild permission, so this both over- and under-reports: a permission gap in that one server fails validation for an otherwise healthy connector, and a gap in any other server still validates clean and then produces the silent zero-users outcome this probe exists to prevent. Since the intent is application-wide, consider reporting which guild was probed in the error message so the failure is attributable.
There was a problem hiding this comment.
Partially addressed in ea040ca.
Took the error-message half of your suggestion: Validate now names the probed server explicitly and states that it is a sample server, so a failure is attributable and a reader is not misled into thinking the check was exhaustive.
Did not extend the probe to every server. Your analysis of why is right — the intent is application-wide, so one server catches the common misconfiguration, whereas per-server permission gaps are not application-wide. But probing every server would put an unbounded number of API calls into validation, which runs on every connector start. The residual gap (a permission gap in some other server validating clean, then syncing no members there) is now in the README's known limitations.
— Written by Claude (Opus 5), acting on Logan's request to triage this review.
| // Discord rate limits per route and answers 429 with a Retry-After; | ||
| // discordgo honors it when told to retry. | ||
| session.ShouldRetryOnRateLimit = true | ||
| session.MaxRestRetries = 3 |
There was a problem hiding this comment.
🟡 Suggestion: Rate limiting is handled entirely inside discordgo, so no v2.RateLimitDescription annotation ever reaches the SDK. That means the syncer cannot pace itself or checkpoint on a 429 — it just blocks inside MaxRestRetries and fails hard on the fourth. Consider surfacing Discord's X-RateLimit-Remaining / X-RateLimit-Reset-After as a rate-limit annotation on the SyncOpResults of the paginated list calls.
There was a problem hiding this comment.
Deferred, with the reasoning recorded in the README's known limitations.
The finding is correct: rate limiting is handled entirely inside discordgo, so no RateLimitDescription reaches the SDK and the syncer can neither pace itself nor checkpoint on a 429.
Not done here because it is an enhancement rather than a defect — discordgo blocks and retries, so syncs complete correctly, just opaquely — and because the plumbing is non-trivial: discordgo's typed methods return decoded structs, not responses, so the X-RateLimit-* headers would have to be captured at the http.RoundTripper layer (currently installed only when --base-url is set), stashed per-call, and threaded out to SyncOpResults for each paginated list. That is a self-contained follow-up rather than something to fold into an already-large rewrite.
— Written by Claude (Opus 5), acting on Logan's request to triage this review.
| // resource itself in baton-sdk v0.24; the trait-scoped options for these | ||
| // are deprecated. | ||
| resourceOptions := []resource_sdk.ResourceOption{ | ||
| resource_sdk.WithParentResourceID(guildResourceID), |
There was a problem hiding this comment.
🟡 Suggestion: A Discord account in two synced servers is emitted twice with the same resource ID and a different parent. The c1z resources table is uniquely indexed on (external_id, sync_id) and its upsert is ON CONFLICT DO UPDATE SET data = EXCLUDED.data, so the row keeps the first guild's parent_resource_id column while the stored proto carries the last guild's parent — the indexed parent and the resource body disagree, and which guild wins depends on sync order. Grants still resolve by user ID, so nothing breaks, but parent-scoped queries over multi-server accounts are unreliable. Leaving users unparented would model "a Discord account is global, membership is a grant" more faithfully.
There was a problem hiding this comment.
Deferred, and I verified your analysis before deciding.
Confirmed against the vendored SDK: dotc1z/resources.go declares create unique index ... (external_id, sync_id) with external_id built as <resourceType>:<resourceID>, and the upsert in sql_helpers.go:603 is OnConflict(DoUpdate("external_id, sync_id", data = EXCLUDED.data)) — it updates only data. So the row does keep the first guild's parent_resource_id column while the proto carries the last guild's parent, exactly as you describe, and which one wins is sync-order dependent.
Not changed here because un-parenting users is a data-model change that interacts with the sibling finding on roles.go:195: that one asks for a fallback to principal.ParentResourceId, which becomes dead code the moment users carry no parent. Making both changes at once would be incoherent, and this PR already carries one deliberate breaking change to entitlement IDs.
Recorded in the README's known limitations, stating plainly that the parent of a multi-server account is order-dependent and that the authoritative membership signal is the per-server access grant. Flagged to the repo owner as a follow-up decision.
— Written by Claude (Opus 5), acting on Logan's request to triage this review.
Connector PR Review: Rewrite connector on baton-sdk v0.24.6 and add provisioningBlocking Issues: 0 | Suggestions: 4 | Threads Resolved: 0 Review SummaryScanned the full PR diff for security and correctness: the baton-sdk v0.3.44 to v0.24.6 migration, the discordgo to disgo swap, the new Security IssuesNone found. The invite-delivery path in Correctness IssuesNone found. Suggestions
Prompt for AI agents |
| {"create_public_threads", "Create Public Threads", "Create public threads in the channel", discordgo.PermissionCreatePublicThreads, scopeText}, | ||
| {"create_private_threads", "Create Private Threads", "Create private threads in the channel", discordgo.PermissionCreatePrivateThreads, scopeText}, | ||
| {"send_messages_in_threads", "Send Messages In Threads", "Send messages in the channel's threads", discordgo.PermissionSendMessagesInThreads, scopeText}, | ||
| {"use_embedded_activities", "Use Activities", "Launch embedded activities in the channel", discordgo.PermissionUseEmbeddedActivities, scopeText}, |
There was a problem hiding this comment.
🟠 Bug: use_embedded_activities (PermissionUseEmbeddedActivities, 1 << 39) is scoped scopeText, but Discord lists USE_EMBEDDED_ACTIVITIES as a voice channel permission. As scoped, permissionsForChannel offers it on text/announcement/forum channels where it is meaningless, and never on voice/stage channels where it actually applies — so an overwrite that explicitly allows Use Activities on a voice channel is never reported as a grant. This is the inverse of the stated intent ("voice channels no longer advertise text-only permissions").
Fix: change this entry's scope to scopeVoice (and move it into the voice block for readability).
There was a problem hiding this comment.
Fixed in ea040ca. Good catch — this was the inverse of the stated intent, and my own test was pinning the bug.
Verified independently: discordgo's declaration comments PermissionUseEmbeddedActivities as "Allows for using Activities (applications with the EMBEDDED flag) in a voice channel." Moved the entry to scopeVoice and into the voice block.
Two follow-on corrections this exposed:
TestChannelGrantsComeFromOverwriteAllowBitsasserteduse_embedded_activitiesappeared on a text channel — it was locking in the wrong behavior. It used that permission to cover the >32-bit path, so that coverage moved tosend_messages_in_threads(1 << 38), a genuine text permission.- Added
TestUseEmbeddedActivitiesIsVoiceScopedandTestVoiceChannelGrantsReportActivities, the latter asserting the practical consequence: an explicit allow on a voice channel is now reported as a grant.
— Written by Claude (Opus 5), acting on Logan's request to triage this review.
|
|
||
| guildID := resource.ParentResourceId.Resource | ||
| guild, err := r.getGuild(guildID) | ||
| guildID, err := parentGuildID(ent.Resource) |
There was a problem hiding this comment.
🟡 Suggestion: parentGuildID(ent.Resource) reads the guild from entitlement.Resource.ParentResourceId, which the repo criteria call out (PR2) because a provisioning request is not guaranteed to round-trip the entitlement resource's parent. The rationale in helpers.go:84-89 is sound for Discord — a role belongs to exactly one guild while an account belongs to many — so the fix is not to switch to the principal, but to fall back to principal.ParentResourceId.Resource (validated as type guild) when the entitlement resource carries no parent, rather than failing the grant outright. Same applies at Revoke (line 232).
There was a problem hiding this comment.
Fixed in ea040ca, in the shape you suggested.
Added guildIDForProvisioning(entitlementResource, principal) in helpers.go, next to parentGuildID. It keeps the entitlement resource's parent as the primary source for the reason you note — a role belongs to exactly one server, an account to many — and falls back to the principal's parent only when the entitlement resource carries none, validating that the fallback is of type guild before using it. The error is returned only when neither source is available.
Both Grant and Revoke use it. Regression test: TestGuildIDForProvisioningFallsBackToPrincipal covers all three cases, including asserting the role's parent wins over a principal parented to a different server.
— Written by Claude (Opus 5), acting on Logan's request to triage this review.
| <Step> | ||
| Under **Scopes**, select `bot`. Under **Bot Permissions**, select the permissions you want to grant: | ||
|
|
||
| - **Manage Roles** - Required to sync channel permissions, and used by C1 to grant and revoke roles and channel permissions |
There was a problem hiding this comment.
🟡 Suggestion: this says Manage Roles is "Required to sync channel permissions", but the warning six lines below tells readers not to grant it if they don't want C1 provisioning. The two statements contradict each other, and a sync-only customer following the warning would end up believing channel permissions won't sync. The connector reads overwrites via GET /channels/{id} (client.Channel), which needs only View Channels — Manage Roles is required for writes (SetChannelOverwrite/DeleteChannelOverwrite), not reads. Suggest dropping "Required to sync channel permissions".
There was a problem hiding this comment.
Fixed in ea040ca.
You are right on both counts: reading overwrites goes through GET /channels/{id}, which needs only View Channels, and the bullet contradicted the warning six lines below it. A sync-only customer following that warning would have concluded channel permissions could not sync.
Dropped the "Required to sync channel permissions" clause so Manage Roles reads as provisioning-only, and moved View Channels to the top of the list with the read requirement attached to it, so the read/write split is unambiguous.
— Written by Claude (Opus 5), acting on Logan's request to triage this review.
| BATON_TOKEN: <Your Discord bot token> | ||
|
|
||
| # Optional: include if you want C1 to provision access using this connector | ||
| BATON_PROVISIONING: true |
There was a problem hiding this comment.
🟡 Suggestion: stringData in a Kubernetes Secret is map[string]string, so an unquoted true parses as a boolean and kubectl apply rejects the manifest with cannot unmarshal bool into Go struct field ... of type string. Should be quoted:
| BATON_PROVISIONING: true | |
| BATON_PROVISIONING: "true" |
There was a problem hiding this comment.
Fixed in ea040ca — applied as suggested, BATON_PROVISIONING: "true".
Confirmed the failure mode: stringData is map[string]string, so an unquoted true is a YAML boolean and kubectl apply rejects the manifest.
Worth flagging upstream: this came verbatim from the scaffold in .claude/skills/connector/build-connector-docs.md, which has the same unquoted value, so other connector docs generated from it likely carry the same broken manifest.
— Written by Claude (Opus 5), acting on Logan's request to triage this review.
| return nil, nil, fmt.Errorf( | ||
| "baton-discord: created an invite to %s but user %s does not accept direct messages "+ | ||
| "from this bot; the invite code is %s: %w", | ||
| guild.Name, principal.Id.Resource, invite.Code, err) |
There was a problem hiding this comment.
🟡 Suggestion: this embeds a live, redeemable invite code in the returned error, which lands in connector logs and the C1 task error text where it persists well past the 3-day MaxAge. Anyone with log access can redeem it to join the server. The failure is already actionable without it — consider naming the guild and the DM-blocked reason only, and either omitting the code or revoking the invite (DELETE /invites/{code}) on delivery failure so the dangling single-use invite isn't left redeemable.
There was a problem hiding this comment.
Fixed in ea040ca, taking both halves of your suggestion.
Agreed this is the more serious of the two problems — a redeemable credential in logs that outlives its 3-day expiry, readable by anyone with log access.
- The invite code is out of the error entirely. The message now names the user and the guild and says what to do about it, which is actionable without leaking anything.
- On delivery failure the invite is revoked via a new
Client.DeleteInvitewrapping discordgo'sSession.InviteDelete, so no dangling single-use invite is left behind. A revoke failure is logged as a warning rather than masking the original error.
Regression test: TestGuildGrantRevokesUndeliverableInvite asserts the DELETE /invites/{code} happens and that the error does not contain the code.
— Written by Claude (Opus 5), acting on Logan's request to triage this review.
| // Activities are embedded applications launched inside a voice channel, so | ||
| // Discord classifies this as a voice permission despite the name reading | ||
| // like a general one. | ||
| {"use_embedded_activities", "Use Activities", "Launch embedded activities in the voice channel", discordgo.PermissionUseEmbeddedActivities, scopeVoice}, |
There was a problem hiding this comment.
🟠 Bug: the scopeText/scopeVoice split is too strict in the other direction. Since text-in-voice, Discord documents the messaging permissions with channel types T, V, S — SEND_MESSAGES, SEND_TTS_MESSAGES, MANAGE_MESSAGES, EMBED_LINKS, ATTACH_FILES, READ_MESSAGE_HISTORY, MENTION_EVERYONE, USE_EXTERNAL_EMOJIS, USE_EXTERNAL_STICKERS, ADD_REACTIONS, USE_APPLICATION_COMMANDS are all settable on a voice or stage channel's overwrite (only the four thread permissions are genuinely T-only). Scoping them scopeText means permissionsForChannel(voice) omits them, so channelBuilder.Grants never emits a grant for an explicit SEND_MESSAGES allow that really is present in a voice channel's overwrite — the same under-reporting class as the use_embedded_activities fix in this commit, just inverted. The new permissionAppliesTo guard in channels.go compounds it by hard-refusing a legitimate send_messages grant on a voice channel.
Suggest a third scope (e.g. scopeMessaging) that permissionAppliesTo accepts for text, voice, and stage, leaving only manage_threads, create_public_threads, create_private_threads, and send_messages_in_threads as scopeText. TestChannelGrantRejectsMismatchedPermission currently pins the refusal, so it needs a thread permission rather than send_messages as its mismatch case.
There was a problem hiding this comment.
Fixed in 287305d. You are right, and this was my regression — the use_embedded_activities fix traded one under-reporting bug for its inverse.
Text-in-voice is the part I had wrong: voice and stage channels carry a text chat, so the messaging permissions really are settable on their overwrites, and scoping them to text meant an explicit SEND_MESSAGES allow on a voice channel was never reported. The permissionAppliesTo guard then made it worse by refusing those grants outright.
Rather than add a fourth scope, I reworked the three so each name states a real distinction:
scopeAll— every governed channel type, now including all the messaging permissionsscopeThread— the four thread permissions, the only genuinely text-exclusive area, since voice channels host no threadsscopeVoice— the voice-motion permissions plususe_embedded_activities
TestChannelEntitlementsMatchChannelType was pinning the wrong behavior and now asserts the real distinction: voice channels do expose send_messages, and do not expose thread permissions. TestChannelGrantRejectsMismatchedPermission switched its mismatch case to create_public_threads, as you suggested.
— Written by Claude (Opus 5), acting on Logan's request to triage this review.
| last := members[len(members)-1] | ||
| if last.User == nil { | ||
| return "" | ||
| } | ||
| return last.User.ID |
There was a problem hiding this comment.
🟡 Suggestion: the nil guard fixes the panic, but ending pagination here is only safe for the two callers that turn a nil Member.User into an error. roleBuilder.Grants filters with memberHasRole before calling newMemberResource, so a full page whose last member has no user object and does not hold the role skips the guarded path entirely: no error is raised, the cursor comes back "", and every remaining member of the server is silently dropped from that role's grants. Returning an error from MembersPage when the last member of a full page has no user object would keep the failure attributable in all three callers, rather than only in the two that happen to construct a resource per member.
There was a problem hiding this comment.
Fixed in 287305d, taking your suggestion over my original guard.
You identified the exact gap: roleBuilder.Grants filters with memberHasRole before building a resource, so a full page ending in a user-less member that holds no roles skipped the guarded path completely — no error, empty cursor, and every remaining member of the server dropped from that role's grants. My cursor-only guard turned a loud panic into silent truncation, which is strictly worse.
MembersPage now rejects any page containing a member with no user object, so the failure is attributable for all three callers rather than the two that construct a resource per member.
TestMembersPageRejectsMemberWithoutUser builds a full page whose final entry has no user and holds no roles — the case a caller-side guard misses — and asserts the intent-naming error surfaces from the user, guild, and role paths alike. I also confirmed by mutation that reverting to the caller-side guard reintroduces the panic.
— Written by Claude (Opus 5), acting on Logan's request to triage this review.
| // channels do not host them. | ||
| {"manage_threads", "Manage Threads", "Manage and delete threads in the channel", discordgo.PermissionManageThreads, scopeThread}, | ||
| {"create_public_threads", "Create Public Threads", "Create public threads in the channel", discordgo.PermissionCreatePublicThreads, scopeThread}, | ||
| {"create_private_threads", "Create Private Threads", "Create private threads in the channel", discordgo.PermissionCreatePrivateThreads, scopeThread}, |
There was a problem hiding this comment.
🟡 Suggestion: scopeThread resolves to !isVoiceChannel(...), so all four thread permissions are advertised on forum and announcement channels too. Discord scopes CREATE_PRIVATE_THREADS to plain text channels only (CREATE_PUBLIC_THREADS / MANAGE_THREADS / SEND_MESSAGES_IN_THREADS are T/F/M), so a forum or announcement channel gets a create_private_threads entitlement that channelBuilder.Grant will happily write as a meaningless overwrite bit — the same class of mismatch this commit just fixed on the text/voice side. Consider a text-only scope for this one entry.
| {"create_private_threads", "Create Private Threads", "Create private threads in the channel", discordgo.PermissionCreatePrivateThreads, scopeThread}, | ||
| {"send_messages_in_threads", "Send Messages In Threads", "Send messages in the channel's threads", discordgo.PermissionSendMessagesInThreads, scopeThread}, | ||
|
|
||
| {"connect", "Connect", "Connect to the voice channel", discordgo.PermissionVoiceConnect, scopeVoice}, |
There was a problem hiding this comment.
🟡 Suggestion: scopeVoice still lumps voice and stage together, which is the mirror image of the text/voice mismatch this commit fixes. Discord scopes REQUEST_TO_SPEAK to stage channels only, while STREAM, PRIORITY_SPEAKER, USE_VAD, DEAFEN_MEMBERS, and USE_EMBEDDED_ACTIVITIES are voice-only — so a voice channel advertises request_to_speak and a stage channel advertises five permissions it cannot honor, and permissionAppliesTo lets channelBuilder.Grant write each of them into the overwrite. Splitting into voice-only / stage-only / both would close it.
| } | ||
|
|
||
| httpClient := &http.Client{Timeout: requestTimeout} | ||
| opts := []rest.ClientConfigOpt{rest.WithHTTPClient(httpClient)} |
There was a problem hiding this comment.
🟡 Suggestion: this drops the 5xx retry the previous client configured explicitly (session.MaxRestRetries = 3). disgo's clientImpl.retry only retries on 429; every other non-2xx goes straight to newError, so a transient Discord/Cloudflare 502/503 now fails the page and aborts that sync branch instead of being retried. Rate-limit retries are preserved, so this is only about 5xx. If the retry was intentional to drop, worth saying so; otherwise wrap httpClient.Transport with a small retrying RoundTripper for idempotent 5xx.
| if c == nil || c.httpClient == nil { | ||
| return nil | ||
| } | ||
| c.httpClient.CloseIdleConnections() |
There was a problem hiding this comment.
🟡 Suggestion: rest.NewClient builds a RateLimiter when none is supplied, and NewRateLimiter starts a background cleanup() goroutine on a ticker. Close only drops idle keep-alives now; the disgo client's own Close(ctx) (available on rest.Rest, which embeds rest.Client) is never called, so in-flight bucket locks are never drained. Consider c.rest.Close(ctx) here (the method takes a context, so Close would need one, or context.Background()). Note the ticker goroutine itself leaks upstream regardless — disgo's Close doesn't stop it — so this is hygiene rather than a fix for that.
| } | ||
|
|
||
| return &Client{ | ||
| rest: rest.New(rest.NewClient(token, opts...)), |
There was a problem hiding this comment.
🟡 Suggestion: no rest.WithLogger(...) is supplied, so disgo falls back to slog.Default(). Two consequences: its Warn-level rate-limit messages ("rate limit exceeded", "global rate limit exceeded") land outside the connector's ctxzap output, and at Debug level clientImpl.retry logs every request and response body verbatim — which includes the POST /channels/{id}/invites response carrying the invite code that guildBuilder.Grant deliberately keeps out of its error text. Default slog level is Info, so nothing leaks today, but wiring an explicit logger (or an explicitly Info-floored handler) makes that guarantee local rather than incidental.
| return | ||
| } | ||
|
|
||
| // The RoundTripper override preserves discordgo's versioned path, so |
There was a problem hiding this comment.
🟡 Suggestion: stale after the disgo swap — there is no RoundTripper override anymore (the base URL is set via rest.WithURL), and the prefix comes from disgo, not discordgo. Same on trimAPIPrefix's doc comment at line 138. Also worth noting the version segment moved from v9 to v10 with this change.
Upgrades baton-sdk from v0.3.44 to v0.24.6, moves to the V2 syncer and
provisioner interfaces, replaces bwmarrin/discordgo with disgoorg/disgo, and
implements provisioning for servers, roles, and channel permissions.
BREAKING: entitlement slugs are now stable identifiers (`access`, `member`,
and permission names like `send_messages`) instead of display text
("Access to <server>", "Member of <role>", "<Perm> for <channel>"). Baton
derives entitlement and grant IDs from the slug, so existing IDs change and
C1 will re-map grants on the next sync. The previous scheme also silently
re-identified every entitlement and grant whenever a server, role, or channel
was renamed; TestRoleEntitlementIDIsStable now pins the new behavior.
Why disgo rather than discordgo:
- It targets Discord API v10, where discordgo pins v9.
- rest.NewClient is REST-only by construction, matching a connector that has
no use for a gateway.
- rest.WithURL is a per-client base URL, so the --base-url test hook needs no
process-global mutation. discordgo addresses endpoints through package-level
variables, and rewriting them covers only the endpoints someone remembered:
any other call silently escapes to production Discord.
- Permissions is a typed bitfield with Has/Add/Remove, and role and member
permission overwrites are distinct types rather than an int discriminator.
- Snowflakes are typed and parsed at the client boundary, so a malformed
resource ID fails with a clear error instead of an opaque 404.
Correctness fixes over the previous implementation:
- No gateway. It opened a websocket and read Session.State.Guilds, which is
populated asynchronously from READY and GUILD_CREATE events, so a sync that
started before those arrived could observe an empty or partial guild list.
All reads now go through the paginated REST API.
- Channel grants come from the permission overwrite's allow mask. Role grants
previously tested the role's server-wide Permissions field and so reported
channel access the channel never conferred; member grants used computed
effective permissions, conflating inherited access with a direct grant.
- Role grants paginate the member list instead of caching every member of
every server for the process lifetime, which was unbounded in server size.
- Channel permissions are scoped to the channel types they apply to. Threads
are the only text-exclusive area; since text-in-voice, messaging permissions
are settable on voice and stage channels too.
- A member with no user object is rejected in the client, which keeps the
missing-intent failure attributable for every caller rather than silently
truncating the role-grant path.
- An undeliverable server invitation is revoked rather than left redeemable,
and its code is kept out of the error, which reaches logs and task output.
- Requests carry a context, and Close releases the client.
Validation now probes the token, guild membership, and member listing, so a
missing Server Members Intent fails at startup naming the intent instead of
producing a sync with zero users.
Adds a test suite driven by an httptest fake Discord API covering pagination
cursors, child-resource fan-out, channel type filtering, permission decoding,
idempotency, and provisioning request shapes.
CI pins Go 1.25.x/1.26.x: baton-sdk v0.24.6 requires Go 1.25.2, and Go 1.27 is
blocked by an upstream !go1.27 build guard in github.com/cockroachdb/swiss, an
indirect dependency.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The connector-docs CI check requires docs/connector.mdx, which this repo has never had. It passed on pushes to main because the precheck could prove the file was unchanged; on a pull request it walks the changed-file list instead, and GitHub caps that API at 3000 files. This branch changes far more than that (the vendored baton-sdk upgrade alone accounts for most of it), so the precheck cannot prove the file is unchanged and demands validation of a file that does not exist. Written to the structure in .claude/skills/connector/build-connector-docs.md and verified against the MDX linter the shared workflow runs. Content notes: - Accounts are sync-only. Discord accounts belong to the person rather than the server, so there is no account provisioning to offer. - The server-access grant is documented as an invitation the user must accept, since Discord has no API for adding a user to a server on an application's authority. - The setup steps call out the Server Members Intent and the role hierarchy, the two configuration mistakes that produce a connector which authenticates but cannot see members or assign roles. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
e96520a to
41868f5
Compare
| // httpStatus returns the HTTP status carried by a Discord API error, or 0. | ||
| func httpStatus(err error) int { | ||
| restErr := restError(err) | ||
| if restErr == nil || restErr.Response == nil { | ||
| return 0 | ||
| } | ||
| return restErr.Response.StatusCode | ||
| } |
There was a problem hiding this comment.
🟡 Suggestion: Discord failures never reach the SDK with a gRPC status code. Because every request goes through disgo rather than uhttp.BaseHttpClient, the fmt.Errorf(...%w) wrappers in this file leave status.Code(err) == codes.Unknown, so the provisioner's retryer (retry.ShouldWaitAndRetry, which only retries Unavailable/DeadlineExceeded) never retries a Grant/Revoke, and C1 cannot tell a 403 role-hierarchy refusal from a transient failure. Consider a helper here that maps httpStatus(err) to a code (401→Unauthenticated, 403→PermissionDenied, 404→NotFound, 429→ResourceExhausted, 5xx→Internal) and wrapping call-site errors with uhttp.WrapErrors / uhttp.WrapErrorsWithRateLimitInfo (the latter can take restErr.Response directly and attach rate-limit details).
| func (o *guildBuilder) Revoke(ctx context.Context, g *v2.Grant) (annotations.Annotations, error) { | ||
| if err := requireEntitlementSlug(g.Entitlement, guildAccessEntitlement); err != nil { | ||
| return nil, err | ||
| } | ||
| if err := requireResourceType(g.Entitlement.Resource, guildResourceTypeID); err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| guildID := g.Entitlement.Resource.Id.Resource | ||
| userID := g.Principal.Id.Resource | ||
|
|
||
| err := o.client.RemoveGuildMember(ctx, guildID, userID, "Server access revoked by ConductorOne") |
There was a problem hiding this comment.
🟡 Suggestion: Revoke guards g.Entitlement.Resource but reads g.Principal.Id.Resource unguarded. The SDK dispatches on request.GetGrant().GetEntitlement().GetResource().GetId().GetResourceType(), so the entitlement chain is guaranteed non-nil, but Grant.Principal is passed straight through unvalidated — a revoke request without a principal panics the connector. The type is unchecked too: a non-user principal would hand a role/channel snowflake to RemoveGuildMember, and the resulting 404 is swallowed as GrantAlreadyRevoked, reporting silent success. requireResourceType(g.Principal, userResourceTypeID) covers both. Same pattern at roles.go:247 and channels.go:330.
| if avatarURL := member.User.EffectiveAvatarURL(); avatarURL != "" { | ||
| resourceOptions = append(resourceOptions, resource_sdk.WithResourceIcon(&v2.AssetRef{Id: avatarURL})) | ||
| } | ||
| if member.JoinedAt != nil && !member.JoinedAt.IsZero() { | ||
| resourceOptions = append(resourceOptions, resource_sdk.WithResourceCreatedAt(*member.JoinedAt)) | ||
| } |
There was a problem hiding this comment.
🟡 Suggestion: the new per-server fields sit on a resource whose ID is the global account snowflake. The c1z store upserts resources on (external_id, sync_id) where external_id is user:<snowflake> (dotc1z/resources.go:116), so an account in several synced servers collapses to one row — the surviving created_at is whichever server's join date was written last, and profile.guild_id / ParentResourceId likewise name an arbitrary server. Worth either dropping created_at (or deriving the account-creation time from the snowflake, which is genuinely global) or making the guild-scoped fields explicitly "one of N".
| for _, permission := range permissions { | ||
| if !target.Allow.Has(permission.Value) { | ||
| continue | ||
| } | ||
| grants = append(grants, grant.NewGrant(resource, permission.Slug, principal)) | ||
| } |
There was a problem hiding this comment.
🟡 Suggestion: member overwrites can name a user who is no longer in the server. Discord keeps a channel's permission overwrites when the targeted member leaves, so this loop can emit user:<snowflake> grants for principals userBuilder.List never returned. The SDK's I9 grant→principal referential check (pkg/sync/ingest_invariants.go) reports these as dangling principals — an aggregated warning in default mode, but noise that grows with server churn. Skipping member overwrites whose user is not a current member, or probing GET /guilds/{g}/members/{u} once per unmatched target, would keep the grant set closed over the synced users.
2 participants
Upgrades
baton-sdkfrom v0.3.44 to v0.24.6, migrates to the V2 syncer and provisioner interfaces, and implements provisioning for servers, roles, and channel permissions. Continues to use discordgo for the Discord API.Entitlement slugs are now stable identifiers rather than display text:
Access to <server name>accessMember of <role name>memberSendMessages for <channel name>send_messagesBaton derives entitlement IDs as
<resourceType>:<resourceID>:<slug>and grant IDs from those, so existing entitlement and grant IDs change and C1 will re-map grants on the next sync.Worth noting that the old scheme was already unstable: because the slug was the display name, renaming a server, role, or channel silently re-identified every entitlement and grant derived from it.
TestRoleEntitlementIDIsStablepins the new behavior.Provisioning
guildrolechannelGranting server access is an invitation, not a join — Discord has no API that adds a user to a server on a bot's authority (
PUT /guilds/{guild}/members/{user}needs an OAuth2 token the user granted withguilds.join). Membership therefore appears on a later sync. If the user's privacy settings block DMs, the grant fails and surfaces the invite code in the error rather than reporting success.Channel permission grants are read-modify-write, since Discord replaces an overwrite wholesale rather than patching it. The granted bit is also cleared from
deny, which would otherwise outrank the allow. Revokes are idempotent and reportGrantAlreadyRevoked.Correctness fixes
Session.State.Guilds, which is populated asynchronously from READY/GUILD_CREATE events — a sync starting before those arrived could observe an empty or partial guild list. All reads now go through the paginated REST API.Allowbitmask. Role grants previously tested the role's server-widePermissionsfield, reporting channel access the channel never conferred; member grants used computed effective permissions, conflating inherited access with a direct grant.context, so cancellation is honored.Close()actually releases the client.--base-urlis applied with anhttp.RoundTripperinstead of mutating discordgo's package-level endpoint variables. The old approach rebuilt only a subset of them, so any missed endpoint silently addressed production Discord instead of the test server.Validation
Validatenow probes the token, that the bot belongs to at least one server, and that member listing is permitted. The last one matters: without the privileged Server Members Intent the connector would previously validate cleanly, sync servers/roles/channels, then report zero users and zero memberships with no visible cause.Testing
21 tests against an
httptestfake Discord API (67% coverage ofpkg/connector), covering pagination cursors, child-resource fan-out, channel type filtering, permission decoding, idempotency, and provisioning request shapes. Three load-bearing assertions were mutation-tested to confirm they fail when the behavior regresses.Not covered: provisioning against real Discord. Request shapes and idempotency are pinned, but role hierarchy, the privileged-intent 403, and DM delivery only reproduce live — that needs a disposable server and a cleanup path.
CI
Go is pinned to 1.25.x/1.26.x because baton-sdk v0.24.6 requires Go 1.25.2. Go 1.27 is currently blocked by an upstream
!go1.27build guard ingithub.com/cockroachdb/swiss, an indirect dependency that introspects Go runtime internals — not something this repo can resolve.🤖 Generated with Claude Code