fix(im): reject --user-id under bot identity for chat-messages-list#340
Open
haozhenghua-code wants to merge 1 commit intolarksuite:mainfrom
Open
fix(im): reject --user-id under bot identity for chat-messages-list#340haozhenghua-code wants to merge 1 commit intolarksuite:mainfrom
haozhenghua-code wants to merge 1 commit intolarksuite:mainfrom
Conversation
|
|
github-actions
bot
added
domain/im
📝 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 a guard in Confidence Score: 5/5Safe to merge; the core fix is correct and well-tested. The two remaining findings are minor style/consistency notes. Both findings are P2: the DryRun inconsistency only affects preview output (Validate correctly blocks execution), and the dead-code guard is harmless defensive boilerplate. No correctness, data-integrity, or security issues remain. shortcuts/im/im_chat_messages_list.go — DryRun should mirror the bot+user-id guard from Validate for consistent preview output. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A["lark-cli im +chat-messages-list"] --> B{Validate}
B -->|bot identity + --user-id| C["❌ FlagErrorf: requires user identity"]
B -->|bot identity, no --chat-id| D["❌ FlagErrorf: specify --chat-id"]
B -->|user identity, both flags| E["❌ ExactlyOne: mutually exclusive"]
B -->|user identity, neither flag| F["❌ FlagErrorf: specify at least one"]
B -->|passes| G{Execute}
G -->|--chat-id set| H["Use chat_id directly"]
G -->|--user-id set| I{resolveP2PChatID}
I -->|bot identity| J["❌ bot guard error"]
I -->|user identity| K["POST /im/v1/chat_p2p/batch_query"]
K -->|found| L["GET /im/v1/messages"]
K -->|not found| M["❌ P2P chat not found"]
H --> L
L --> N["Return formatted messages"]
Reviews (4): 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@ac55dbf8f0021b974346d9956265e1429bac79d7🧩 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
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
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
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
3 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