Skip to content

Add channel membership as a provisionable resource type#81

Open
c1-dev-bot[bot] wants to merge 3 commits into
mainfrom
feature/channel-membership-provisioning
Open

Add channel membership as a provisionable resource type#81
c1-dev-bot[bot] wants to merge 3 commits into
mainfrom
feature/channel-membership-provisioning

Conversation

@c1-dev-bot
Copy link
Copy Markdown

@c1-dev-bot c1-dev-bot Bot commented Mar 23, 2026

Summary

  • Adds a new channel resource type that syncs Slack channels (public and private, excluding archived) as resources with membership entitlements
  • Implements Grant (via conversations.invite) and Revoke (via conversations.kick) for channel membership provisioning
  • Channels are synced as child resources of workspaces, with cursor-based pagination
  • Idempotent provisioning: already_in_channel on grant returns success; not_in_channel on revoke returns GrantAlreadyRevoked

Required bot token scopes: channels:read, channels:join, groups:read, channels:manage

Test plan

  • Verify channel sync lists all public and private channels for a workspace
  • Verify channel membership grants are correctly listed for each channel
  • Test granting channel membership invites the user to the channel
  • Test revoking channel membership removes the user from the channel
  • Test idempotent grant (user already in channel) succeeds without error
  • Test idempotent revoke (user not in channel) returns GrantAlreadyRevoked
  • Verify pagination works correctly for channels and channel members
  • Verify the connector builds and existing tests pass

Fixes: CXH-1229


Automated PR Notice

This PR was automatically created by c1-dev-bot as a potential implementation.

This code requires:

  • Human review of the implementation approach
  • Manual testing to verify correctness
  • Approval from the appropriate team before merging

Add a new "channel" resource type to the Slack connector that syncs
Slack channels (public and private) and supports provisioning channel
membership via Grant and Revoke operations.

Changes:
- Add channel resource type definition with TRAIT_GROUP
- Add channelResourceType syncer with List, Entitlements, Grants,
  Grant (conversations.invite), and Revoke (conversations.kick)
- Register channel syncer in connector's ResourceSyncers
- Add channel as a child resource of workspace
- Update baton_capabilities.json with channel resource type

Required bot token scopes: channels:read, channels:join, groups:read,
channels:manage

Fixes: CXH-1229
@c1-dev-bot c1-dev-bot Bot requested a review from a team March 23, 2026 16:06
@linear
Copy link
Copy Markdown

linear Bot commented Mar 23, 2026

@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Mar 23, 2026

Claude finished @c1-dev-bot[bot]'s task in 1m 36s —— View job


PR Review: Add channel membership as a provisionable resource type

  • Gather context and read changed files
  • Review implementation against CLAUDE.md patterns
  • Post detailed review feedback

Overall Assessment

This is a solid implementation that follows the existing patterns in the codebase well. The channel builder correctly mirrors the structure of workspace.go and group.go. Pagination, resource creation, and the sync phases (List, Entitlements, Grants) are all well-implemented. However, there are a few issues that should be addressed before merging.


Issues

1. Bug: Missing GrantAlreadyExists annotation on idempotent Grant (Medium)

pkg/connector/channel.go:197-201 — When already_in_channel is detected, the Grant returns nil, nil instead of returning a GrantAlreadyExists annotation. Per CLAUDE.md and the SDK pattern, idempotent "already exists" should return the annotation:

// Current (line 199):
return nil, nil

// Should be:
outputAnnotations := annotations.New()
outputAnnotations.Append(&v2.GrantAlreadyExists{})
return outputAnnotations, nil

This is inconsistent with how the Revoke correctly returns GrantAlreadyRevoked (line 234-236). The Grant should symmetrically return GrantAlreadyExists.

Fix this →

2. Bug: Missing baton-slack: error prefix (Low)

Several error messages in channel.go are missing the baton-slack: prefix that the CLAUDE.md requires:

  • Line 77: "parsing page token: %w""baton-slack: parsing page token: %w"
  • Line 97: "creating channel resource: %w""baton-slack: creating channel resource: %w"
  • Line 104: "creating next page token: %w""baton-slack: creating next page token: %w"
  • Line 161: "creating user resource ID: %w"
  • Line 169: "creating next page token: %w"
  • Line 188: "only users can be granted channel membership"
  • Line 202: "inviting user to channel: %w"
  • Line 223: "only users can have channel membership revoked"
  • Line 239: "removing user from channel: %w"

Note: The existing group.go and workspace.go are also inconsistent about this prefix, so this is a minor style nit. But the CLAUDE.md is explicit about it.

3. Consideration: Missing WithExternalID on channel resources (Info)

pkg/connector/channel.go:55-63 — The channelResource function doesn't call WithExternalID() when creating channel resources. While this isn't needed right now (channels are the entitlement resource, not the principal), it's worth noting that if channel-to-channel relationships are ever needed, this would need to be added.

More importantly, user resources (user.go:80-86) also don't set WithExternalID(). Currently this works because principal.Id.Resource happens to contain the Slack user ID (set during NewUserResource). But per CLAUDE.md, WithExternalID() is the canonical way to store the native system ID for provisioning. This is a pre-existing issue, not introduced by this PR.

4. Style: Slack error handling could be more robust (Low)

pkg/connector/channel.go:197,232 — The Slack error type assertion uses slack.SlackErrorResponse. Consider also handling the cant_invite_self and channel_not_found errors for Grant, and cant_kick_self and cant_kick_from_general for Revoke, to provide clearer error messages. These are common Slack API error codes that would benefit from explicit handling.


What looks good

  • Pagination — Correctly uses pkg.ParsePageToken / bag.NextToken with cursor-based pagination, consistent with other builders.
  • Resource hierarchy — Channels are correctly registered as child resources of workspaces (workspace.go:63).
  • Idempotent Revoke — Correctly returns GrantAlreadyRevoked annotation for not_in_channel.
  • Channel types — Including both public_channel and private_channel while excluding archived is the right default.
  • baton_capabilities.json — Correctly declares CAPABILITY_PROVISION for the channel resource type.
  • Entitlements — Clean single-entitlement pattern with WithGrantableTo(resourceTypeUser).
  • Consistent structure — The channelResourceType struct and builder follow the same pattern as other resource types.

Summary

The implementation is well-structured and follows codebase conventions. The main actionable item is Issue #1 (missing GrantAlreadyExists annotation), which is a functional bug that should be fixed before merge. The error prefix issue (#2) is a minor consistency concern. The rest is solid work.

Regenerate baton_capabilities.json from binary output to match
expected CI format. Update docs/connector.mdx capability tables
to include the new Channels resource type with sync and provision
support, and add channels:manage to the documented bot token scopes.
@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Mar 23, 2026

Claude finished @c1-dev-bot[bot]'s task in 1m 33s —— View job


PR Review: Add channel membership as a provisionable resource type

  • Gather context and read changed files
  • Review implementation against CLAUDE.md patterns
  • Post detailed review feedback

Overall Assessment

This is a well-structured implementation that follows existing codebase patterns closely. The channel builder correctly mirrors workspace.go and group.go in structure. Pagination, resource creation, entitlements, and grants are all implemented consistently. A few issues should be addressed before merging.


Issues

1. Missing GrantAlreadyExists annotation on idempotent Grant (Medium)

pkg/connector/channel.go:197-199 — When already_in_channel is detected, the Grant returns nil, nil instead of returning a GrantAlreadyExists annotation. Per CLAUDE.md's Grant Idempotency section:

// Grant "already exists" = success, not error
if isAlreadyExistsError(err) {
    return nil, annotations.New(&v2.GrantAlreadyExists{}), nil
}

Current code:

if slackErr.Err == "already_in_channel" {
    return nil, nil  // Missing annotation
}

Should be:

if slackErr.Err == "already_in_channel" {
    outputAnnotations := annotations.New()
    outputAnnotations.Append(&v2.GrantAlreadyExists{})
    return outputAnnotations, nil
}

This is inconsistent with how Revoke correctly returns GrantAlreadyRevoked at line 234-236. Note: the existing group.go Grant also doesn't use GrantAlreadyExists, but that's because the SCIM API layer handles idempotency differently. Here, the Slack API explicitly returns already_in_channel which should be annotated.

Fix this →

2. Missing baton-slack: error prefix on several error messages (Low)

CLAUDE.md specifies that errors should include the connector prefix. Several error messages in channel.go are missing it:

  • Line 77: "parsing page token: %w"
  • Line 97: "creating channel resource: %w"
  • Line 104: "creating next page token: %w"
  • Line 161: "creating user resource ID: %w"
  • Line 169: "creating next page token: %w"
  • Line 188: "only users can be granted channel membership"
  • Line 202: "inviting user to channel: %w"
  • Line 223: "only users can have channel membership revoked"
  • Line 239: "removing user from channel: %w"

Note: The existing builders (workspace.go, group.go) are also inconsistent about this prefix, so this is a codebase-wide style issue, not unique to this PR. However, if fixing, the Grant/Revoke error messages at lines 188, 202, 223, 239 are the most important ones since they surface to users during provisioning.

3. Slack error type assertion style (Low)

pkg/connector/channel.go:197,232 — The error type assertion uses err.(slack.SlackErrorResponse). The slack-go library may return a pointer type (*slack.SlackErrorResponse) in some cases. Consider using errors.As for safer unwrapping:

var slackErr slack.SlackErrorResponse
if errors.As(err, &slackErr) {
    if slackErr.Err == "already_in_channel" {
        // ...
    }
}

This is more robust against future library changes and handles wrapped errors correctly.

4. Consider handling additional Slack error codes (Info)

For Grant, the Slack API can also return cant_invite_self, channel_not_found, is_archived, and not_authed. For Revoke, it can return cant_kick_self, cant_kick_from_general, and restricted_action. Handling a few of these explicitly (especially cant_kick_from_general which is a permanent constraint, not a transient error) would give operators clearer diagnostics. Not blocking, but worth considering.


What looks good

  • Pagination — Correctly uses pkg.ParsePageToken / bag.NextToken with cursor-based pagination, consistent with other builders
  • Resource hierarchy — Channels correctly registered as child resources of workspaces (workspace.go:63)
  • Idempotent Revoke — Correctly returns GrantAlreadyRevoked annotation for not_in_channel
  • Channel filtering — Including both public_channel and private_channel while excluding archived is the right default
  • baton_capabilities.json — Correctly declares CAPABILITY_PROVISION for the channel resource type
  • Entitlements — Clean single-entitlement pattern with WithGrantableTo(resourceTypeUser)
  • Documentationdocs/connector.mdx properly updated with Channels row and channels:manage scope
  • Connector metadataconnector.go:33 description updated to include channels

Summary

The implementation is well-structured and follows codebase conventions. Issue #1 (missing GrantAlreadyExists annotation) is the main functional item to fix before merge. The other items are minor improvements.

Replace direct type assertions on error with errors.As to properly
handle wrapped errors, fixing errorlint violations.
@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Mar 23, 2026

Claude finished @c1-dev-bot[bot]'s task in 1m 40s —— View job


PR Review: Add channel membership as a provisionable resource type

  • Gather context and read changed files
  • Review implementation against CLAUDE.md patterns
  • Post detailed review feedback

Overall Assessment

This is a well-structured implementation that follows existing codebase patterns closely. The channel builder correctly mirrors the structure of other builders (workspace.go, group.go). Pagination, resource hierarchy, entitlements, and grants are all implemented consistently. The latest commit (ff1a188) correctly addressed the errors.As usage flagged in prior reviews. One functional issue remains.


Issues

1. Missing GrantAlreadyExists annotation on idempotent Grant (Medium)

pkg/connector/channel.go:199-200 — When already_in_channel is detected, the Grant returns nil, nil instead of returning a GrantAlreadyExists annotation. Per CLAUDE.md's Grant Idempotency section:

// Current:
if errors.As(err, &slackErr) && slackErr.Err == "already_in_channel" {
    return nil, nil  // Missing annotation
}

// Should be:
if errors.As(err, &slackErr) && slackErr.Err == "already_in_channel" {
    outputAnnotations := annotations.New()
    outputAnnotations.Append(&v2.GrantAlreadyExists{})
    return outputAnnotations, nil
}

This is inconsistent with how Revoke correctly returns GrantAlreadyRevoked at lines 234-236. The Grant path should symmetrically annotate the idempotent case.

Fix this →

2. Missing baton-slack: error prefix on error messages (Low)

CLAUDE.md specifies errors should include the connector prefix (baton-slack:). Several error messages in channel.go are missing it:

  • Line 78: "parsing page token: %w"
  • Line 98: "creating channel resource: %w"
  • Line 105: "creating next page token: %w"
  • Line 145: "parsing page token: %w"
  • Line 163: "creating user resource ID: %w"
  • Line 169: "creating next page token: %w"
  • Line 189: "only users can be granted channel membership"
  • Line 202: "inviting user to channel: %w"
  • Line 223: "only users can have channel membership revoked"
  • Line 239: "removing user from channel: %w"

Note: The existing builders (workspace.go, group.go) are also inconsistent about this prefix, so this is a codebase-wide style issue. However, the provisioning error messages (lines 189, 202, 223, 239) are the most important since they surface to operators.

3. channels:join scope may be unnecessary for the channel resource type (Info)

pkg/connector/resource_types.go:97 — The channels:join scope is listed in resourceTypeChannel's capability permissions, but the channel builder never calls any "join" API. conversations.invite (used in Grant) requires channels:manage, not channels:join. The channels:join scope is used by the workspace builder (to join channels for reading members), not the channel builder itself. This is cosmetic — having an extra scope declared doesn't break anything — but it's worth noting for accuracy.


What looks good

  • Pagination — Correctly uses pkg.ParsePageToken / bag.NextToken with cursor-based pagination, consistent with other builders
  • Resource hierarchy — Channels correctly registered as child resources of workspaces (workspace.go:63)
  • Idempotent Revoke — Correctly returns GrantAlreadyRevoked annotation for not_in_channel (lines 234-236)
  • Channel filtering — Including both public_channel and private_channel while excluding archived is the right default
  • errors.As usage — Latest commit (ff1a188) correctly uses errors.As instead of type assertion, which handles wrapped errors properly
  • baton_capabilities.json — Correctly declares CAPABILITY_PROVISION for the channel resource type
  • Entitlements — Clean single-entitlement pattern with WithGrantableTo(resourceTypeUser)
  • Documentationdocs/connector.mdx properly updated with Channels row in both Pro and Business+ tables, and channels:manage scope documented
  • Connector metadataconnector.go:33 description updated to include channels

Summary

The implementation is well-structured and follows codebase conventions. The errors.As fix from the latest commit addresses a prior review finding. Issue #1 (missing GrantAlreadyExists annotation) is the remaining functional item to fix before merge — it's a one-line change for correctness and consistency with the Revoke path. The other items are minor.

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.

0 participants