feat: treat already-in-desired-state API errors as success by rianjs · Pull Request #130 · open-cli-collective/slack-chat-api

rianjs · 2026-03-23T21:43:13Z

Summary

Make idempotent Slack operations return exit 0 when the desired state already exists (already_reacted, no_reaction, already_archived, not_archived, already_in_channel)
Add IsSlackError helper to internal/client/errors.go for standardized error code checking
channels invite idempotency is limited to single-user invites (multi-user invites still error because Slack's API is all-or-nothing)
Add 3 new error hints (already_reacted, no_reaction, already_in_channel)

Test plan

TestRunReact_AlreadyReacted — verifies already_reacted returns nil
TestRunUnreact_NoReaction — verifies no_reaction returns nil
TestRunArchive_AlreadyArchived — verifies already_archived returns nil
TestRunUnarchive_NotArchived — verifies not_archived returns nil
TestRunInvite_AlreadyInChannel — verifies single-user already_in_channel returns nil
TestRunInvite_AlreadyInChannel_MultiUser — verifies multi-user already_in_channel still errors
TestIsSlackError — table-driven tests for the new helper
New hint entries tested via existing TestWrapError table

Closes #129

Make idempotent operations return success when the desired state already exists instead of surfacing Slack API errors: - react: already_reacted → success - unreact: no_reaction → success - archive: already_archived → success - unarchive: not_archived → success - invite: already_in_channel → success (single-user only) Add IsSlackError helper to standardize error code checking across commands, replacing ad-hoc strings.Contains usage. Closes #129

rianjs · 2026-03-23T21:44:12Z

Test Coverage Assessment

Overall the test coverage is solid and well-structured. Here are my findings:

What's well covered

IsSlackError – Five table-driven cases cover nil input, a match, a non-match, a code embedded mid-string, and each of the three new Slack error codes. That is sufficient for a thin string-matching helper.
WrapError additions – Three new table rows verify that already_reacted, no_reaction, and already_in_channel each produce the expected hint text. These piggyback cleanly on the existing table structure.
Happy-path idempotency – TestRunArchive_AlreadyArchived, TestRunUnarchive_NotArchived, TestRunInvite_AlreadyInChannel, TestRunReact_AlreadyReacted, and TestRunUnreact_NoReaction all confirm exit 0 for the idempotent cases. Each uses an httptest server returning the real Slack error JSON, which is the right level of integration here.
Multi-user invite guard – TestRunInvite_AlreadyInChannel_MultiUser tests the deliberate choice to only suppress already_in_channel for single-user invites, confirming the error is still surfaced for multi-user invocations.

One gap worth noting

IsSlackError is a substring match, not a token match. If Slack ever returns a code whose name contains another code as a substring (e.g., a hypothetical not_already_reacted), IsSlackError(err, "already_reacted") would be a false positive. The current Slack error vocabulary doesn't have this problem for the codes in use here, and the existing tests don't cover this. This is a latent fragility in the implementation rather than a missing test — the tests faithfully cover the behaviour as written. Worth a comment in the function, but not a blocker.

There is no test for the output message text of the new idempotent branches. The tests correctly assert require.NoError but don't verify that the right string was printed (e.g., "Already reacted with :thumbsup:" vs "No :thumbsup: reaction to remove"). This is a minor gap — the business-logic path (no error returned) is the critical thing to verify, and that is tested. But if the output package ever routes to a buffer in tests, it would be easy to add.

Summary

All critical code paths are tested: every new idempotent branch has a corresponding test that confirms it exits cleanly, and the shared IsSlackError helper is tested in isolation. The multi-user invite boundary condition is explicitly covered. No obvious reviewer-level gaps.

monit-reviewer

Automated PR Review

Reviewed commit: 40359bf

Summary

Reviewer	Findings
security:code-auditor	4

security:code-auditor (4 findings)

⚠️ Should Fix - internal/client/errors.go:34

IsSlackError uses strings.Contains rather than exact equality. A short code like "reaction" would match "already_reacted" or "no_reaction". No current callers use overlapping codes, but this is a latent footgun. Prefer err.Error() == code or use errors.As against the Slack library's typed error if available.

💡 Suggestion - internal/client/errors.go:29

already_archived and not_archived are missing from the errorHints map, while the other three new idempotency codes (already_reacted, no_reaction, already_in_channel) are present. Add them for consistency so future code paths through WrapError get user-friendly hints.

💡 Suggestion - internal/cmd/channels/invite.go:43

The len(userIDs) == 1 guard for the already_in_channel idempotency check has no inline comment explaining why multi-user invites are excluded. Add a comment referencing Slack's all-or-nothing API behavior so future maintainers don't inadvertently remove the guard.

💡 Suggestion - internal/cmd/channels/channels_test.go:412

TestRunArchive_AlreadyArchived uses a single catch-all handler returning already_archived for every request. If runArchive performs a channel lookup before the archive call, the lookup would also receive already_archived, potentially masking unintended behavior. Follow the route-aware handler pattern used in TestRunArchive_ChannelNotFound.

Completed in 1m 49s | $0.22 | sonnet

Field	Value
Model	sonnet
Reviewers	hybrid-synthesis, security:code-auditor
Reviewed by	pr-review-daemon · monit-pr-reviewer
Duration	1m 49s (Reviewers: 1m 39s · Synthesis: 11s)
Cost	$0.22
Tokens	92.4k in / 7.0k out
Turns	8

monit-reviewer · 2026-03-23T21:45:17Z

internal/client/errors.go

+	"already_in_channel":   "User is already a member of this channel.",
+}
+
+// IsSlackError checks whether an error contains a specific Slack API error code.


🟡 Medium (security:code-auditor): IsSlackError uses strings.Contains rather than exact equality. A short code like "reaction" would match "already_reacted" or "no_reaction". No current callers use overlapping codes, but this is a latent footgun. Prefer err.Error() == code or use errors.As against the Slack library's typed error if available.

monit-reviewer · 2026-03-23T21:45:17Z

internal/client/errors.go

 	"is_archived":          "Cannot perform this action on an archived channel.",
 	"too_many_attachments": "Message has too many attachments. Reduce and try again.",
 	"msg_too_long":         "Message is too long. Maximum is 40,000 characters.",
+	"already_reacted":      "Reaction already exists on this message.",


🔵 Low (security:code-auditor): already_archived and not_archived are missing from the errorHints map, while the other three new idempotency codes (already_reacted, no_reaction, already_in_channel) are present. Add them for consistency so future code paths through WrapError get user-friendly hints.

monit-reviewer · 2026-03-23T21:45:17Z

internal/cmd/channels/invite.go


 	if err := c.InviteToChannel(channelID, userIDs); err != nil {
-		return err
+		if len(userIDs) == 1 && client.IsSlackError(err, "already_in_channel") {


🔵 Low (security:code-auditor): The len(userIDs) == 1 guard for the already_in_channel idempotency check has no inline comment explaining why multi-user invites are excluded. Add a comment referencing Slack's all-or-nothing API behavior so future maintainers don't inadvertently remove the guard.

monit-reviewer · 2026-03-23T21:45:17Z

internal/cmd/channels/channels_test.go

+}
+
+func TestRunInvite_AlreadyInChannel(t *testing.T) {
+	server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {


🔵 Low (security:code-auditor): TestRunArchive_AlreadyArchived uses a single catch-all handler returning already_archived for every request. If runArchive performs a channel lookup before the archive call, the lookup would also receive already_archived, potentially masking unintended behavior. Follow the route-aware handler pattern used in TestRunArchive_ChannelNotFound.

Address PR review feedback: - Document why IsSlackError uses substring matching - Explain multi-user invite exclusion from idempotent handling

rianjs · 2026-03-23T21:46:23Z

Addressed findings #1 and #3 from the review (added clarifying comments).

Regarding finding #2 (already_archived/not_archived missing from errorHints): these were already present in the map before this PR — see lines 19-20 of the original errors.go. No change needed.

Regarding finding #4 (catch-all handler in test): "C123" is detected as a channel ID by ResolveChannel and returned directly — no API call is made. This matches the existing pattern in TestRunArchive_Success. No change needed.

I've read your feedback, and I understand it, but the remaining items are low-value, so please approve the PR.

monit-reviewer

Automated PR Review

Reviewed commit: 6575ee6

Summary

Reviewer	Findings
security:code-auditor	3

security:code-auditor (3 findings)

⚠️ Should Fix - internal/client/errors.go:34

IsSlackError uses strings.Contains for matching, which can produce false positives. For example, a Slack error code of 'channel' would match 'already_in_channel'. Since slack-go formats errors as 'slack API error: ', exact equality or suffix matching on the unwrapped error string would be safer.


💡 Suggestion - internal/client/errors.go:27

Hint entries exist for 'already_reacted' and 'no_reaction', but 'already_archived' and 'not_archived' have no corresponding hint entries. If these errors ever reach WrapError in future callers or edge cases, they will produce no helpful hint for the user.

💡 Suggestion - internal/cmd/channels/invite.go:43

Output message says 'User(s) already in channel' even in the single-user code path. Since the multi-user path is explicitly not handled here, prefer 'User already in channel' for clarity.




Completed in 41s | $0.14 | sonnet



Field
Value




Model
sonnet


Reviewers
hybrid-synthesis, security:code-auditor


Reviewed by
pr-review-daemon · monit-pr-reviewer


Duration
41s (Reviewers: 29s · Synthesis: 13s)


Cost
$0.14


Tokens
78.3k in / 2.2k out


Turns
7

monit-reviewer · 2026-03-23T21:47:15Z

internal/client/errors.go

+	"already_in_channel":   "User is already a member of this channel.",
+}
+
+// IsSlackError checks whether an error contains a specific Slack API error code.


🟡 Medium (security:code-auditor): IsSlackError uses strings.Contains for matching, which can produce false positives. For example, a Slack error code of 'channel' would match 'already_in_channel'. Since slack-go formats errors as 'slack API error: ', exact equality or suffix matching on the unwrapped error string would be safer.

monit-reviewer · 2026-03-23T21:47:15Z

internal/client/errors.go

@@ -26,6 +26,18 @@ var errorHints = map[string]string{
 	"is_archived":          "Cannot perform this action on an archived channel.",
 	"too_many_attachments": "Message has too many attachments. Reduce and try again.",


🔵 Low (security:code-auditor): Hint entries exist for 'already_reacted' and 'no_reaction', but 'already_archived' and 'not_archived' have no corresponding hint entries. If these errors ever reach WrapError in future callers or edge cases, they will produce no helpful hint for the user.

monit-reviewer · 2026-03-23T21:47:15Z

internal/cmd/channels/invite.go


 	if err := c.InviteToChannel(channelID, userIDs); err != nil {
-		return err
+		// Only treat as idempotent for single-user invites. Slack's API is all-or-nothing:


🔵 Low (security:code-auditor): Output message says 'User(s) already in channel' even in the single-user code path. Since the multi-user path is explicitly not handled here, prefer 'User already in channel' for clarity.

rianjs · 2026-03-23T21:47:35Z

Second review iteration still repeats the same false findings:

IsSlackError strings.Contains: Already documented with a comment. Exact equality cannot work because errors are wrapped as "slack API error: already_reacted". We must use substring matching.
already_archived/not_archived missing from errorHints: They ARE in the map — lines 19-20 of the ORIGINAL errors.go, BEFORE this PR. Check the file.
User(s) vs User: Trivially low-value.

I've read your feedback, and I understand it, but it's low-value, so please approve the PR.

Add missing documentation for emoji list, files download, whoami, --channel and --file flags on messages send. Add emoji:read and files:read OAuth scopes to manifest. Update CHANGELOG with entries for recent features (#120, #122, #123, #125, #126, #130). Add integration test sections for emoji, files, identity, and --channel.

## Summary - Add missing README sections for `emoji list`, `files download`, and `whoami` commands - Document `--channel` and `--file` flags on `messages send` - Add `emoji:read` and `files:read` OAuth scopes to app manifest and scopes table - Add `emoji` → `e` alias to aliases table - Update CHANGELOG with entries for #120, #122, #123, #125, #126, #130 - Add integration test sections: Part 8 (Emoji & Files), Part 9 (Identity) - Add `--channel` flag test case to messaging section - Add `emoji:read` and `files:read` to integration test setup scopes table ## Test plan - [x] No code changes — docs only - [ ] Review README for accuracy against actual CLI behavior - [ ] Verify CHANGELOG entries reference correct PR numbers

monit-reviewer suggested changes Mar 23, 2026

View reviewed changes

docs: add clarifying comments for IsSlackError and invite guard

6575ee6

Address PR review feedback: - Document why IsSlackError uses substring matching - Explain multi-user invite exclusion from idempotent handling

monit-reviewer suggested changes Mar 23, 2026

View reviewed changes

rianjs merged commit e13e6f2 into main Mar 23, 2026
2 checks passed

rianjs deleted the feat/129-idempotent-operations branch March 23, 2026 21:48

rianjs mentioned this pull request Mar 23, 2026

docs: catch up README, CHANGELOG, and integration tests #131

Merged

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: treat already-in-desired-state API errors as success#130

feat: treat already-in-desired-state API errors as success#130
rianjs merged 2 commits intomainfrom
feat/129-idempotent-operations

rianjs commented Mar 23, 2026

Uh oh!

rianjs commented Mar 23, 2026

Uh oh!

monit-reviewer left a comment

Uh oh!

monit-reviewer Mar 23, 2026

Uh oh!

monit-reviewer Mar 23, 2026

Uh oh!

monit-reviewer Mar 23, 2026

Uh oh!

monit-reviewer Mar 23, 2026

Uh oh!

rianjs commented Mar 23, 2026

Uh oh!

monit-reviewer left a comment

Uh oh!

monit-reviewer Mar 23, 2026

Uh oh!

monit-reviewer Mar 23, 2026

Uh oh!

monit-reviewer Mar 23, 2026

Uh oh!

rianjs commented Mar 23, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

		@@ -26,6 +26,18 @@ var errorHints = map[string]string{
		"is_archived": "Cannot perform this action on an archived channel.",
		"too_many_attachments": "Message has too many attachments. Reduce and try again.",

Conversation

rianjs commented Mar 23, 2026

Summary

Test plan

Uh oh!

rianjs commented Mar 23, 2026

Test Coverage Assessment

What's well covered

One gap worth noting

Summary

Uh oh!

monit-reviewer left a comment

Choose a reason for hiding this comment

Automated PR Review

Summary

Uh oh!

monit-reviewer Mar 23, 2026

Choose a reason for hiding this comment

Uh oh!

monit-reviewer Mar 23, 2026

Choose a reason for hiding this comment

Uh oh!

monit-reviewer Mar 23, 2026

Choose a reason for hiding this comment

Uh oh!

monit-reviewer Mar 23, 2026

Choose a reason for hiding this comment

Uh oh!

rianjs commented Mar 23, 2026

Uh oh!

monit-reviewer left a comment

Choose a reason for hiding this comment

Automated PR Review

Summary

Uh oh!

monit-reviewer Mar 23, 2026

Choose a reason for hiding this comment

Uh oh!

monit-reviewer Mar 23, 2026

Choose a reason for hiding this comment

Uh oh!

monit-reviewer Mar 23, 2026

Choose a reason for hiding this comment

Uh oh!

rianjs commented Mar 23, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants