feat(messages): validate message length before sending by rianjs · Pull Request #140 · open-cli-collective/slack-chat-api

rianjs · 2026-04-02T20:55:34Z

Summary

Adds pre-send validation for message text length against Slack's 40,000 character hard limit
Returns an actionable error with alternatives (--file, canvas create, manual chunking) instead of letting Slack silently truncate
Applies to both messages send and messages update; skipped for file uploads
Trues up CHANGELOG.md: removes [Unreleased], creates [3.2.0] and [3.1.35] sections
Adds Message Length Limits entry to README Known Limitations

Closes #139

Test plan

TestValidateMessageLength — under/at/over limit boundary tests
TestRunSend_MessageTooLong — send rejects oversized text
TestRunUpdate_MessageTooLong — update rejects oversized text
TestRunSend_FileUpload_LongTextAllowed — file uploads bypass length check
Manual: python3 -c "print('x' * 50000)" | ./bin/slck messages send C123 - → error with helpful message
make build && make test && make lint all pass

…ent Slack truncation (#139) Closes #139

rianjs · 2026-04-02T20:56:19Z

Test Coverage Review

All four new tests pass. Coverage is solid for a focused validation feature.

What's covered well:

Unit test for validateMessageLength hits all meaningful boundary cases: empty, under limit, at exact limit, over by one, well over. Boundary-at-limit (40000) returning nil is the most important edge case and it's there.
Error message content is asserted (--file, canvas create hints), not just the presence of an error.
runSend integration: long text rejected correctly.
runUpdate integration: long text rejected correctly.
File-upload bypass: long text with --file is explicitly allowed — this matches the skip-validation branch in send.go and is the most important behavioral distinction in the PR.

Gaps worth noting:

--blocks + long text combination in runSend: validation fires for non-file paths even when blocks are provided. Not tested, but the behavior is correct by inspection since validateMessageLength is called before the block path diverges. Low risk, minor coverage gap.
runSend with long text piped via stdin (text == "-"): stdin is read before validation runs, so an oversized stdin message will also be rejected. No test for this path, but it's the same code path as direct text — still low risk.
runUpdate has no file-upload path, so no bypass case to test. The asymmetry is correct.
Error message includes actual character count (%d characters) but TestValidateMessageLength only checks for the static limit string, not the dynamic count. Fine for this feature, but a small assertion gap if the format changes.

Verdict: Coverage is adequate for the change. The critical happy/sad paths and the file-bypass carve-out are all tested. The gaps are minor and consistent with the rest of the test suite's style.

monit-reviewer

Automated PR Review

Reviewed commit: 8997bc0

Summary

Reviewer	Findings
database:sql-reviewer	1
security:code-auditor	2

database:sql-reviewer (1 findings)

💡 Suggestion - internal/cmd/messages/messages_test.go:1618

All test strings use ASCII-only strings.Repeat("x", n), so byte count equals rune count and the len() vs rune-count bug is not exercised. Add a test case with multi-byte characters (e.g., strings.Repeat("😀", maxMessageTextLen-1)) to catch the discrepancy.

security:code-auditor (2 findings)

⚠️ Should Fix - internal/cmd/messages/messages.go:83

len(text) counts UTF-8 bytes, not Unicode characters. Slack's 40,000 limit is on characters. A message with multi-byte characters (emoji, CJK, accented text) will be incorrectly rejected — e.g., 20,000 emoji (4 bytes each) = 80,000 bytes but only 20,000 characters, well under the limit. Use utf8.RuneCountInString(text) instead. Also update line 93 (len(text) in the error message) to report rune count so the number matches the limit.

💡 Suggestion - internal/cmd/messages/update.go:46

The update command calls validateMessageLength which suggests --file <path> as an alternative, but update doesn't support --file. Consider using a variant error message for update, or at minimum note that --file applies to send only.

1 PR discussion thread considered.

Completed in 5m 07s | $0.68

Field	Value
Reviewers	hybrid-synthesis, database:sql-reviewer, security:code-auditor, harness-engineering:knowledge-reviewer, harness-engineering:enforcement-reviewer, harness-engineering:architecture-reviewer, harness-engineering:legibility-reviewer
Reviewed by	pr-review-daemon · monit-pr-reviewer
Duration	5m 07s (Reviewers: 1m 13s · Synthesis: 52s)
Cost	$0.68
Tokens	198.9k in / 19.1k out
Turns	17

monit-reviewer · 2026-04-02T20:58:50Z

internal/cmd/messages/messages.go

+
+// validateMessageLength checks whether text exceeds Slack's message length limit.
+func validateMessageLength(text string) error {
+	if len(text) <= maxMessageTextLen {


🟡 Medium (security:code-auditor): len(text) counts UTF-8 bytes, not Unicode characters. Slack's 40,000 limit is on characters. A message with multi-byte characters (emoji, CJK, accented text) will be incorrectly rejected — e.g., 20,000 emoji (4 bytes each) = 80,000 bytes but only 20,000 characters, well under the limit. Use utf8.RuneCountInString(text) instead. Also update line 93 (len(text) in the error message) to report rune count so the number matches the limit.

monit-reviewer · 2026-04-02T20:58:51Z

internal/cmd/messages/messages_test.go

+			err := validateMessageLength(text)
+			if tt.wantErr {
+				require.Error(t, err)
+				assert.Contains(t, err.Error(), "exceeds Slack's 40000 character limit")


🔵 Low (database:sql-reviewer): All test strings use ASCII-only strings.Repeat("x", n), so byte count equals rune count and the len() vs rune-count bug is not exercised. Add a test case with multi-byte characters (e.g., strings.Repeat("😀", maxMessageTextLen-1)) to catch the discrepancy.

monit-reviewer · 2026-04-02T20:58:51Z

internal/cmd/messages/update.go

 	// Unescape shell-escaped characters (e.g., \! from zsh)
 	text = unescapeShellChars(text)

+	if err := validateMessageLength(text); err != nil {


🔵 Low (security:code-auditor): The update command calls validateMessageLength which suggests --file <path> as an alternative, but update doesn't support --file. Consider using a variant error message for update, or at minimum note that --file applies to send only.

…entiate update error message

rianjs · 2026-04-02T21:00:18Z

All three review findings addressed in cfe1285:

UTF-8 bytes vs runes — switched to utf8.RuneCountInString(text) for both validation and error message reporting
Multi-byte test cases — added emoji test cases (strings.Repeat("😀", n)) for under/at/over limit boundaries
Update error message — validateMessageLength now takes a forUpdate bool param; update path omits the --file suggestion since update doesn't support it. Added TestValidateMessageLength_UpdateOmitsFileHint to verify.

monit-reviewer

Automated PR Review

Reviewed commit: cfe1285 | Previous: 8997bc0 (incremental)

Summary

No issues found.

5 PR discussion threads considered.

Completed in 5m 18s | $0.73

Field	Value
Mode	Re-review · Cycle 2 · Session resumed
Reviewers	hybrid-synthesis, database:sql-reviewer, security:code-auditor, harness-engineering:knowledge-reviewer, harness-engineering:enforcement-reviewer, harness-engineering:architecture-reviewer, harness-engineering:legibility-reviewer
Reviewed by	pr-review-daemon · monit-pr-reviewer
Duration	5m 18s (Reviewers: 1m 11s · Synthesis: 35s)
Cost	$0.73
Tokens	239.1k in / 19.9k out
Turns	15

feat(messages): validate message length before sending to prevent sil…

8997bc0

…ent Slack truncation (#139) Closes #139

monit-reviewer suggested changes Apr 2, 2026

View reviewed changes

fix: address PR review — use rune count, add multi-byte tests, differ…

cfe1285

…entiate update error message

monit-reviewer approved these changes Apr 2, 2026

View reviewed changes

rianjs merged commit 4b899ce into main Apr 2, 2026
2 checks passed

rianjs deleted the feat/139-message-length-validation branch April 2, 2026 21:02

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(messages): validate message length before sending#140

feat(messages): validate message length before sending#140
rianjs merged 2 commits intomainfrom
feat/139-message-length-validation

rianjs commented Apr 2, 2026

Uh oh!

rianjs commented Apr 2, 2026

Uh oh!

monit-reviewer left a comment

Uh oh!

monit-reviewer Apr 2, 2026

Uh oh!

monit-reviewer Apr 2, 2026

Uh oh!

monit-reviewer Apr 2, 2026

Uh oh!

rianjs commented Apr 2, 2026

Uh oh!

monit-reviewer left a comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

rianjs commented Apr 2, 2026

Summary

Test plan

Uh oh!

rianjs commented Apr 2, 2026

Test Coverage Review

Uh oh!

monit-reviewer left a comment

Choose a reason for hiding this comment

Automated PR Review

Summary

Uh oh!

monit-reviewer Apr 2, 2026

Choose a reason for hiding this comment

Uh oh!

monit-reviewer Apr 2, 2026

Choose a reason for hiding this comment

Uh oh!

monit-reviewer Apr 2, 2026

Choose a reason for hiding this comment

Uh oh!

rianjs commented Apr 2, 2026

Uh oh!

monit-reviewer left a comment

Choose a reason for hiding this comment

Automated PR Review

Summary

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants