fix(im): reject --user-id under bot identity for chat-messages-list#340
Merged
YangJunzhou-01 merged 1 commit intolarksuite:mainfrom Apr 13, 2026
Merged
Conversation
|
|
github-actions
Bot
added
domain/im
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
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 (6)
🚧 Files skipped from review as they are similar to previous changes (5)
📝 WalkthroughWalkthroughValidation now enforces that Changes
Sequence Diagram(s)sequenceDiagram
participant User as "CLI User"
participant CLI as "Command (Validate)"
participant Runtime as "Runtime (resolvedAs)"
participant P2P as "P2P Chat API"
rect rgba(200,230,201,0.5)
User->>CLI: run command with --user-id
CLI->>Runtime: check resolvedAs / IsBot()
end
alt runtime is user
Runtime-->>CLI: IsBot() == false
CLI->>P2P: call resolveP2PChatID(--user-id)
P2P-->>CLI: returns chat_id or not_found
CLI->>User: proceed with chat_id or report not_found
else runtime is bot
Runtime-->>CLI: IsBot() == true
CLI-->>User: validation error ("requires user identity" or "use --chat-id")
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Greptile SummaryThis PR adds input validation to reject Confidence Score: 5/5Safe to merge — the primary fix is correct and well-tested; remaining findings are minor test-quality suggestions. No P0/P1 issues found. The validation logic is correct and covers all bot/user combinations. Both P2 findings (missing auth-header assertion in the migrated test, and the DryRun gap for bot+user-id) are non-blocking style/coverage suggestions that do not affect production behaviour. shortcuts/im/helpers_network_test.go — the removed Authorization header assertion could be reinstated for user-identity coverage. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[lark-cli im +chat-messages-list] --> B{Validate}
B --> C{IsBot?}
C -- Yes --> D{--user-id set?}
D -- Yes --> E[❌ Error: requires user identity]
D -- No --> F{--chat-id set?}
F -- No --> G[❌ Error: specify --chat-id]
F -- Yes --> H[✅ Pass validation]
C -- No --> I{ExactlyOne chat-id / user-id}
I -- Neither --> J[❌ Error: specify at least one]
I -- Both --> K[❌ Error: mutually exclusive]
I -- Exactly one --> L[✅ Pass validation]
H --> M[Execute]
L --> M
M --> N{user-id provided?}
N -- Yes --> O[resolveP2PChatID]
O --> P{IsBot? defensive guard}
P -- Yes --> Q[❌ Error: requires user identity]
P -- No --> R[POST /im/v1/chat_p2p/batch_query]
R --> S[GET /im/v1/messages]
N -- No --> S
Reviews (5): Last reviewed commit: "fix(im): reject --user-id under bot iden..." | Re-trigger Greptile |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
shortcuts/im/helpers.go (1)
378-410:⚠️ Potential issue | 🟠 MajorLGTM on the bot-identity guard for
resolveP2PChatID.The defensive check correctly rejects bot identity before making the P2P API call, with a clear error message guiding users to use
--as useror--chat-id.However,
im_messages_send.goalso uses--user-idto resolve a target user but does not have this bot-identity check. In the Validate function, there is no check equivalent to the one inim_chat_messages_list.go(lines 77-79). This creates an inconsistency where:
im +chat-messages-list --user-id ou_xxxrejects bot identity ✓im +messages-send --user-id ou_xxxaccepts bot identity (and may silently fail or produce unexpected results)Consider adding the same bot-identity validation to
ImMessagesSend.Validatefor consistency.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@shortcuts/im/helpers.go` around lines 378 - 410, Add the same bot-identity guard to ImMessagesSend.Validate that resolveP2PChatID and im_chat_messages_list use: in the ImMessagesSend.Validate method check runtime.IsBot() and if true return output.Errorf(output.ExitValidation, "validation", "--user-id requires user identity (--as user); use --chat-id when calling with bot identity"); this mirrors the existing behavior in resolveP2PChatID and ensures --user-id is rejected when running as a bot.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@shortcuts/im/helpers.go`:
- Around line 378-410: Add the same bot-identity guard to
ImMessagesSend.Validate that resolveP2PChatID and im_chat_messages_list use: in
the ImMessagesSend.Validate method check runtime.IsBot() and if true return
output.Errorf(output.ExitValidation, "validation", "--user-id requires user
identity (--as user); use --chat-id when calling with bot identity"); this
mirrors the existing behavior in resolveP2PChatID and ensures --user-id is
rejected when running as a bot.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 69d09f44-d211-4344-bd21-4d4516ed51c2
📒 Files selected for processing (6)
shortcuts/im/builders_test.goshortcuts/im/coverage_additional_test.goshortcuts/im/helpers.goshortcuts/im/helpers_network_test.goshortcuts/im/im_chat_messages_list.goskills/lark-im/references/lark-im-chat-messages-list.md
🚀 PR Preview Install Guide🧰 CLI updatenpm i -g https://pkg.pr.new/larksuite/cli/@larksuite/cli@03f60f7e78af43863201064f90f456565db7db85🧩 Skill updatenpx skills add haozhenghua-code/larksuite-cli#fix/bot_identity -y -g |
sammi-bytedance
approved these changes
Apr 10, 2026
sammi-bytedance
approved these changes
Apr 10, 2026
haozhenghua-code
force-pushed
the
fix/bot_identity
branch
from
cb313d3 to
e01a48d
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
shortcuts/im/helpers_network_test.go (1)
164-173: Strengthen bot-rejection test by asserting no network dispatch.The error assertion is good; add an explicit request counter so this test also enforces early-return behavior before any HTTP call.
🔍 Suggested tightening for the test
func TestResolveP2PChatIDRejectsBot(t *testing.T) { + calls := 0 runtime := newBotShortcutRuntime(t, shortcutRoundTripFunc(func(req *http.Request) (*http.Response, error) { + calls++ return nil, fmt.Errorf("unexpected request: %s", req.URL.String()) })) _, err := resolveP2PChatID(runtime, "ou_123") if err == nil || !strings.Contains(err.Error(), "requires user identity") { t.Fatalf("resolveP2PChatID() error = %v, want requires user identity", err) } + if calls != 0 { + t.Fatalf("resolveP2PChatID() made %d HTTP call(s), want 0", calls) + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@shortcuts/im/helpers_network_test.go` around lines 164 - 173, The test TestResolveP2PChatIDRejectsBot should assert no HTTP requests are made by adding a request counter in the fake round-trip handler: declare a counter (e.g., reqCount := 0) and increment it inside the shortcutRoundTripFunc handler used by newBotShortcutRuntime, then call resolveP2PChatID(runtime, "ou_123") and after verifying the error contains "requires user identity" also assert reqCount == 0 to ensure resolveP2PChatID returns early and does not dispatch any network request.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@shortcuts/im/helpers_network_test.go`:
- Around line 164-173: The test TestResolveP2PChatIDRejectsBot should assert no
HTTP requests are made by adding a request counter in the fake round-trip
handler: declare a counter (e.g., reqCount := 0) and increment it inside the
shortcutRoundTripFunc handler used by newBotShortcutRuntime, then call
resolveP2PChatID(runtime, "ou_123") and after verifying the error contains
"requires user identity" also assert reqCount == 0 to ensure resolveP2PChatID
returns early and does not dispatch any network request.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 62fd76e4-1bee-43e5-b3db-bb9a7cb5a6fd
📒 Files selected for processing (6)
shortcuts/im/builders_test.goshortcuts/im/coverage_additional_test.goshortcuts/im/helpers.goshortcuts/im/helpers_network_test.goshortcuts/im/im_chat_messages_list.goskills/lark-im/references/lark-im-chat-messages-list.md
✅ Files skipped from review due to trivial changes (1)
- skills/lark-im/references/lark-im-chat-messages-list.md
🚧 Files skipped from review as they are similar to previous changes (4)
- shortcuts/im/builders_test.go
- shortcuts/im/coverage_additional_test.go
- shortcuts/im/helpers.go
- shortcuts/im/im_chat_messages_list.go
YangJunzhou-01
approved these changes
Apr 13, 2026
The chat_p2p/batch_query endpoint that resolves a user's p2p chat_id requires user identity. Calling +chat-messages-list with --user-id under bot identity previously failed silently or returned wrong results. - Validate: reject --user-id when runtime.IsBot(), with a hint to pass --as user or use --chat-id instead - resolveP2PChatID: add defensive guard for the same condition in case the helper is reached via another path - Update --user-id flag description and the lark-im skill reference to note the user-identity requirement - Tests: add bot-rejection cases for Validate and resolveP2PChatID, switch p2p happy-path tests to a user-identity runtime helper
haozhenghua-code
force-pushed
the
fix/bot_identity
branch
from
e01a48d to
03f60f7
Compare
2 tasks
yxzhaao
pushed a commit
to yxzhaao/cli
that referenced
this pull request
Apr 14, 2026
…arksuite#340) The chat_p2p/batch_query endpoint that resolves a user's p2p chat_id requires user identity. Calling +chat-messages-list with --user-id under bot identity previously failed silently or returned wrong results. - Validate: reject --user-id when runtime.IsBot(), with a hint to pass --as user or use --chat-id instead - resolveP2PChatID: add defensive guard for the same condition in case the helper is reached via another path - Update --user-id flag description and the lark-im skill reference to note the user-identity requirement - Tests: add bot-rejection cases for Validate and resolveP2PChatID, switch p2p happy-path tests to a user-identity runtime helper
3 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
lark-cli im +chat-messages-list --user-id ou_xxxresolves the target user's p2pchat_idthroughPOST /open-apis/im/v1/chat_p2p/batch_query, which requires user identity. Invoking it under bot identity previously failed silently or returned wrong results. This PR rejects the combination at the entry point with a clear error.Changes
shortcuts/im/im_chat_messages_list.go:Validatenow rejects--user-idwhenruntime.IsBot(), pointing users to--as useror--chat-id. Flag description updated to note "user identity only".shortcuts/im/helpers.go:resolveP2PChatIDgains a defensive guard returning the same validation error, in case the helper is reached via another path.skills/lark-im/references/lark-im-chat-messages-list.md: document the user-identity requirement on--user-id, add the new error row to the troubleshooting table, and update the AI usage guidance.builders_test.go: add subtests for mutual-exclusion and bot-identity rejection of--user-id.helpers_network_test.go: introducenewUserShortcutRuntimehelper; keep the p2p happy-path / not-found tests on user identity; addTestResolveP2PChatIDRejectsBot.coverage_additional_test.go:TestResolveChatIDForMessagesListkeeps the user-identity happy path and adds a bot-rejection subtest.Test Plan
go test ./shortcuts/im/...go build ./...cleanlark-cli im +chat-messages-list --as bot --user-id ou_xxx→ rejected at Validate withrequires user identitylark-cli im +chat-messages-list --as bot --chat-id oc_xxx→ returns messages normallylark-cli im +chat-messages-list --as user --user-id ou_xxx→ resolves p2p chat and returns DM messageslark-cli im +chat-messages-list --chat-id oc_xxx --user-id ou_xxx→ mutual-exclusion errorRelated Issues
Summary by CodeRabbit