fix: restore team_id and pagination for workspace grants#78
Conversation
Commit 63d8340 removed the business+ client path for workspace grants, switching to slack-go's GetUsers() without team_id filtering or SDK pagination. For Enterprise Grid orgs, users.list without team_id can return empty results, causing all workspace member and role grants to be deleted on re-sync. Restore the paginated business+ client path when available, and fix the fallback to pass GetUsersOptionTeamID and the caller's context. Fixes: CXH-1162 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
Claude finished @btipling's task in 1m 51s —— View job PR Review: fix: restore team_id and pagination for workspace grantsBlocking Issues: 1 | Suggestions: 0 Security IssuesNone found. Correctness Issues
SuggestionsNone. OverallThe core fix is sound — restoring |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| outputAnnotations := annotations.New() | ||
| bpUsers, nextCursor, ratelimitData, err := o.businessPlusClient.GetUsers( | ||
| ctx, | ||
| resource.Id.Resource, | ||
| bag.PageToken(), | ||
| ) | ||
| outputAnnotations.WithRateLimiting(ratelimitData) | ||
| if err != nil { | ||
| return nil, nil, fmt.Errorf("fetching users for workspace: %w", err) |
There was a problem hiding this comment.
🟠 Bug: outputAnnotations is populated with rate limit data but never included in the return value. The annotations are silently discarded, so the SDK won't see rate limit information from the business+ client path.
Compare with group.go:117 and group.go:132 which correctly pass Annotations: outputAnnotations in the SyncOpResults.
| outputAnnotations := annotations.New() | |
| bpUsers, nextCursor, ratelimitData, err := o.businessPlusClient.GetUsers( | |
| ctx, | |
| resource.Id.Resource, | |
| bag.PageToken(), | |
| ) | |
| outputAnnotations.WithRateLimiting(ratelimitData) | |
| if err != nil { | |
| return nil, nil, fmt.Errorf("fetching users for workspace: %w", err) | |
| outputAnnotations := annotations.New() | |
| bpUsers, nextCursor, ratelimitData, err := o.businessPlusClient.GetUsers( | |
| ctx, | |
| resource.Id.Resource, | |
| bag.PageToken(), | |
| ) | |
| outputAnnotations.WithRateLimiting(ratelimitData) | |
| if err != nil { | |
| return nil, &resources.SyncOpResults{Annotations: outputAnnotations}, fmt.Errorf("fetching users for workspace: %w", err) |
| return rv, &resources.SyncOpResults{ | ||
| NextPageToken: pageToken, | ||
| }, nil |
There was a problem hiding this comment.
🟠 Bug: The outputAnnotations from the business+ path (containing rate limit data) should be included here. Currently they are lost.
You'll need to hoist outputAnnotations to the outer scope (alongside users and pageToken) and pass it through:
| return rv, &resources.SyncOpResults{ | |
| NextPageToken: pageToken, | |
| }, nil | |
| return rv, &resources.SyncOpResults{ | |
| NextPageToken: pageToken, | |
| Annotations: outputAnnotations, | |
| }, nil |
This requires declaring outputAnnotations in the var block at line 142 and initializing it in both branches.
Hoist outputAnnotations to outer scope and pass through in the return value so the SDK receives rate limit data from the business+ client path. Also return annotations on error paths, consistent with group.go. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
Claude finished @btipling's task in 1m 21s —— View job PR Review: fix: restore team_id and pagination for workspace grants
Blocking Issues: 0 | Suggestions: 1 Security IssuesNone found. CorrectnessThe fix is well-structured and addresses the root cause correctly. The second commit ( Both code paths are sound:
Suggestions
OverallThis is a clean, well-scoped fix. The root cause — |
Switch business+ error paths from raw fmt.Errorf to client.WrapError for consistent baton-slack: error prefixing across both code paths. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
Claude finished @btipling's task in 1m 50s —— View job PR Review: fix: restore team_id and pagination for workspace grants
Blocking Issues: 0 | Suggestions: 1 Security IssuesNone found. CorrectnessThe fix correctly addresses the root cause: Business+ path ( Fallback path ( Return values ( Error wrapping: All error paths consistently use Suggestions
OverallThis is a clean, well-scoped fix across three focused commits. The root cause is correctly addressed in both the business+ and fallback paths, rate limit annotations are properly propagated, and error wrapping is consistent. The dual-path approach follows the same patterns used in |
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
Claude finished @btipling's task in 1m 20s —— View job PR Review: fix: restore team_id and pagination for workspace grants
Blocking Issues: 0 | Suggestions: 0 Security IssuesNone found. CorrectnessThe fix is sound and all previously flagged issues have been addressed across the four commits:
Both code paths are correct:
Field mapping in fallback path ( OverallClean, well-scoped fix across four focused commits. The root cause — |
2 participants
Summary
Grants()when available (with team_id filtering and rate limit tracking)GetUsersOptionTeamIDand caller context when business+ client is unavailableNextPageTokenin sync results for SDK pagination supportRoot cause:
users.listwithoutteam_idon Enterprise Grid orgs returns users from the wrong workspace or empty results, causing all workspace/role grants to disappear on re-sync.Fixes CXH-1162
Test plan
🤖 Generated with Claude Code