feat: validate IM idempotency key length#1797
Conversation
github-actions
Bot
added
domain/im
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
🚧 Files skipped from review as they are similar to previous changes (5)
📝 WalkthroughWalkthroughAdds 50-character validation for ChangesIdempotency Key Length Validation
Estimated code review effort: 2 (Simple) | ~10 minutes Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
d3c14e7 to
69ec416
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@shortcuts/im/im_messages_send.go`:
- Around line 218-226: Add tests to cover the idempotency-key validation logic
in validateIdempotencyKey and the Validate methods for ImMessagesSend and
ImMessagesReply. Include boundary cases at exactly 50 characters and 51
characters, verify multi-byte runes are counted by characters rather than bytes,
and assert that Validate() rejects invalid idempotency keys in both message
builders.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 2ba27a9a-93e0-4b2e-b64c-1604bbb16bdd
📒 Files selected for processing (4)
shortcuts/im/im_messages_reply.goshortcuts/im/im_messages_send.goskills/lark-im/references/lark-im-messages-reply.mdskills/lark-im/references/lark-im-messages-send.md
🚀 PR Preview Install Guide🧰 CLI updatenpm i -g https://pkg.pr.new/larksuite/cli/@larksuite/cli@030b3be4a40486ec4314e6b45bf9fe523eeb327d🧩 Skill updatenpx skills add 91-enjoy/cli#feat/add_message_send_param_validate -y -g |
69ec416 to
9edad3e
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
shortcuts/im/builders_test.go (1)
519-524: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert typed error metadata in error-path tests, not just message substrings.
The new error-path test cases (
validateIdempotencyKey 51 chars fails,validateIdempotencyKey 51 Chinese chars fails,ImMessagesSend idempotency key too long,ImMessagesReply idempotency key too long) only checkstrings.Contains(err.Error(), ...). Per coding guidelines, error-path tests must assert typed metadata viaerrs.ProblemOf(category/subtype) andParam, not message substrings alone.Since
errs.ProblemOfdoes not exposeParam, useerrors.As(err, &ve)with a*errs.ValidationErrorto assertve.Param == "--idempotency-key". Based on learnings,errs.ProblemOf(err)returns(*errs.Problem, bool)containing only problem-level fields (Code,Hint,Subtype,Category,Retryable) — it does not haveParam.♻️ Proposed refactor for typed metadata assertions
t.Run("validateIdempotencyKey 51 chars fails", func(t *testing.T) { err := validateIdempotencyKey(strings.Repeat("a", 51)) if err == nil || !strings.Contains(err.Error(), "--idempotency-key exceeds the maximum of 50 characters") { t.Fatalf("validateIdempotencyKey() error = %v", err) } + problem, ok := errs.ProblemOf(err) + if !ok || problem.Category != errs.CategoryValidation || problem.Subtype != errs.SubtypeInvalidArgument { + t.Fatalf("validateIdempotencyKey() expected validation/invalid_argument, got %+v", problem) + } + var ve *errs.ValidationError + if !errors.As(err, &ve) || ve.Param != "--idempotency-key" { + t.Fatalf("validateIdempotencyKey() expected param --idempotency-key, got %v", ve) + } })As per coding guidelines: "Error-path tests must assert typed metadata via
errs.ProblemOf(category/subtype/param) and cause preservation, not message substrings alone."Also applies to: 532-537, 539-549, 562-572
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@shortcuts/im/builders_test.go` around lines 519 - 524, The idempotency-key error-path tests only assert message substrings, so update the affected tests around validateIdempotencyKey, ImMessagesSend, and ImMessagesReply to assert typed metadata instead. Use errs.ProblemOf to check the problem category/subtype, and use errors.As with errs.ValidationError to verify the Param is --idempotency-key; keep the existing failure cases but replace substring-only checks with typed assertions and cause preservation.Sources: Coding guidelines, Learnings
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@shortcuts/im/builders_test.go`:
- Around line 519-524: The idempotency-key error-path tests only assert message
substrings, so update the affected tests around validateIdempotencyKey,
ImMessagesSend, and ImMessagesReply to assert typed metadata instead. Use
errs.ProblemOf to check the problem category/subtype, and use errors.As with
errs.ValidationError to verify the Param is --idempotency-key; keep the existing
failure cases but replace substring-only checks with typed assertions and cause
preservation.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 9d7126cc-5d61-4be3-8be3-941eb57697d5
📒 Files selected for processing (5)
shortcuts/im/builders_test.goshortcuts/im/im_messages_reply.goshortcuts/im/im_messages_send.goskills/lark-im/references/lark-im-messages-reply.mdskills/lark-im/references/lark-im-messages-send.md
✅ Files skipped from review due to trivial changes (2)
- skills/lark-im/references/lark-im-messages-send.md
- skills/lark-im/references/lark-im-messages-reply.md
🚧 Files skipped from review as they are similar to previous changes (2)
- shortcuts/im/im_messages_reply.go
- shortcuts/im/im_messages_send.go
9edad3e to
030b3be
Compare
2 participants
Summary
Fixes #1774 by adding client-side validation for IM
--idempotency-key.Previously, keys longer than the OpenAPI
uuidlimit were sent to the server and returned a genericfield validation failederror. This change rejects overlong keys locally with a typed validation error that identifies--idempotency-keyand the 50-character limit.Changes
--idempotency-keylength for:im +messages-sendim +messages-replyvalidation/invalid_argumentwithparam: "--idempotency-key"when the key exceeds 50 characters.Test Plan
go test ./shortcuts/imim +messages-send --dry-runrejects a 51-character--idempotency-key.im +messages-reply --dry-runrejects a 51-character--idempotency-key.--helpshowsidempotency key, max 50 characters.Related Issues
close #1774
Summary by CodeRabbit