feat(contact): add bot search shortcut - #2083
Open
sang-neo03 wants to merge 17 commits into
Open
Conversation
|
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:
📝 WalkthroughWalkthroughAdds the ChangesContact bot search
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant CLI
participant ContactSearchBot
participant BotSearchAPI
User->>CLI: run contact +search-bot
CLI->>ContactSearchBot: validate flags and build request
ContactSearchBot->>BotSearchAPI: POST search with pagination
BotSearchAPI-->>ContactSearchBot: return bot data and page metadata
ContactSearchBot-->>CLI: render JSON or human-readable results
CLI-->>User: display bots and pagination hint
Suggested labels: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
github-actions
Bot
added
domain/contact
🚀 PR Preview Install Guide🧰 CLI updatenpm i -g https://pkg.pr.new/larksuite/cli/@larksuite/cli@fcb661bebe99fc9fed3c392741f04c7d6cbb9b91🧩 Skill updatenpx skills add larksuite/cli#feat/search-bot -y -g |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
shortcuts/contact/contact_search_bot.go (1)
160-184: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winChat-ids parsing/validation is duplicated between
buildBotSearchBodyandvalidateBotSearch.Lines 165-174 re-implement the same CSV parse + "no valid chat_id parsed" check already performed in
validateBotSearch(lines 134-140). SincebuildBotSearchBodyrelies onValidatehaving already run (both inExecuteand inDryRun), this duplicate check is currently dead weight, but if one copy is updated (e.g., a new chat-id constraint) and the other isn't, the two functions can silently diverge.♻️ Suggested extraction
+func parseAndValidateChatIDs(raw string) ([]string, error) { + chatIDs := common.SplitCSV(raw) + if len(chatIDs) == 0 { + return nil, common.ValidationErrorf("--chat-ids: no valid chat_id parsed from %q (separate entries with ',')", raw). + WithParam("--chat-ids") + } + return chatIDs, nil +}Then call this helper from both
validateBotSearch(adding the> maxBotSearchChatIDscheck after) andbuildBotSearchBody.🤖 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/contact/contact_search_bot.go` around lines 160 - 184, Extract the shared chat-IDs CSV parsing and empty-result validation from validateBotSearch and buildBotSearchBody into a helper, then call it from both functions. Keep the maxBotSearchChatIDs limit check in validateBotSearch after the helper returns, and use the helper’s parsed IDs when populating filter.ChatIDs in buildBotSearchBody.
🤖 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/contact/contact_search_bot.go`:
- Around line 160-184: Extract the shared chat-IDs CSV parsing and empty-result
validation from validateBotSearch and buildBotSearchBody into a helper, then
call it from both functions. Keep the maxBotSearchChatIDs limit check in
validateBotSearch after the helper returns, and use the helper’s parsed IDs when
populating filter.ChatIDs in buildBotSearchBody.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 6e5770c0-de40-4525-90e9-a5be1964f151
📒 Files selected for processing (6)
shortcuts/contact/contact_search_bot.goshortcuts/contact/contact_search_bot_test.goshortcuts/contact/shortcuts.goskills/lark-contact/SKILL.mdtests/cli_e2e/contact/contact_search_bot_workflow_test.gotests/cli_e2e/dryrun/contact_search_bot_dryrun_test.go
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #2083 +/- ##
==========================================
+ Coverage 75.16% 75.22% +0.05%
==========================================
Files 912 914 +2
Lines 96475 96816 +341
==========================================
+ Hits 72517 72826 +309
- Misses 18381 18399 +18
- Partials 5577 5591 +14 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
parseBotDisplayInfo read the name from the first non-empty line but always took the description from line 1. When line 0 is blank the two collapse: the name is echoed back as its own description and the real description on the next line is dropped. Track which line the name came from and read the description from the line after it. Also document the enable_join_group trap. The flag says a bot may be added to chats, but doing so needs the app's cli_ app_id, which this command does not return -- the ou_ open_id it returns is rejected by the chat-member APIs and there is no open_id to app_id lookup. An agent reading the bare flag will claim a bot was added when it cannot be. Called out in Tips and in the skill doc, and the "open_id is usable downstream" wording is softened to say what the id identifies rather than implying every downstream API accepts it. The command Description listed keyword, chat and chat history as alternatives, which reads as if a chat filter alone is a valid search; --query is required, so the filters are now described as narrowing it.
Both contact/v3/users/search and bot/v4/bot/search accept user_id_type, and union_id does come back as on_... on either. Neither +search-user nor +search-bot exposes it, because both project the response into a fixed struct whose field is named open_id -- switching the type would leave that field holding a union_id or an employee id, so the name would lie. +get-user can offer --user-id-type precisely because it passes the raw response through. Record the rule and the escape hatch so the omission reads as a decision rather than an oversight, and so the next reader does not add the flag without also renaming the field.
Two spots diverged from the sibling command for no reason.
parseBotSearchChatIDs gated on Cmd.Flags().Changed("chat-ids"). +search-user
never does that for a string flag -- it tests emptiness
(strings.TrimSpace(runtime.Str(...)) != "") and reserves Changed() for bool
filters, where emptiness cannot distinguish unset from false. The practical
difference is that --chat-ids "" used to be an error here and a no-op there;
now a blank value reads as "no filter" in both, and only a non-blank value
that parses to zero entries is rejected. Tests pin both halves.
botSearchPagination existed only to return two runtime lookups to two callers.
+search-user builds its query params inline in DryRun and Execute, so do the
same and keep the opaque-page-token note where the token is actually read.
+search-user decodes page_token off the wire and deliberately never surfaces
it: no --page-token flag, no page_token in searchUserResponse, and a has_more
hint that tells the caller to refine instead ("add filters or tighten
--query"). Bot search had grown the opposite shape -- a --page-token flag, the
token in the envelope, and a hint pointing at it -- so the two sibling search
commands disagreed on whether pagination exists.
Align on the sibling: keep decoding page_token so the field is not silently
lost from the API type, but stop exposing it, and reword both the tip and the
stderr hint to name the narrowing options this command actually has
(--chat-ids, --has-chatted, a more specific --query).
TestBotSearchIntegrationEmptyPageTokenOmitted becomes
TestBotSearchIntegrationNeverSurfacesPageToken and now has the stub return a
token, so the test fails if the field is ever re-added to the envelope rather
than only covering the empty case.
+search-user has --queries: comma-separated keywords searched in parallel, a flat users[] where every row carries matched_query, and a queries[] sidecar holding each keyword's has_more and notice. Bot search had no equivalent, which hurts more here than there because --query is mandatory, so resolving three bot names meant three sequential invocations. Mirror the sibling rather than invent a second mechanism: contact_search_bot_ fanout.go reuses parseAndDedupQueries, querySummary, fanoutConcurrency, isFanoutSummaryFormat and the contactFanout* error helpers, and repeats its structure -- one worker per keyword behind the same concurrency cap, panic recovery per worker, results reindexed into query order, and failure only when every keyword fails so a single bad keyword cannot sink the batch. --chat-ids and --has-chatted narrow every keyword in the batch, matching how the bool filters apply across the user fanout. --query and --queries are mutually exclusive, and the "--query is required" rule is scoped to single-search mode so it does not leak into fanout.
A consistency pass against +search-user turned up four real divergences. The keyword error named only --query. Adding --queries made that a lie: an agent reading param="--query" concludes --queries is not a way out. Switch to WithParams naming both flags with a reason each, and fix the flag help, the tip and the skill line that still said --query was unconditionally required. p2p_chat_id carried omitempty while searchUser.P2PChatID does not, so the two sibling commands disagreed on whether the key exists when there is no p2p chat. Callers should not need a bot-specific presence check; drop omitempty and flip the test to require the empty key. affordance/contact.md had no +search-bot section, so its --help was missing the When to use / Avoid when / Examples / Related skills blocks the other three contact commands all have. Added, without a ### Tips block: affordance tips replace the shortcut's own Tips rather than adding to them, which would have hidden the fanout example and the enable_join_group warning. --chat-ids validated the raw entry count against the 100 cap and then sent duplicates through, while --user-ids is normalized and deduped by common.resolveOpenIDs before its cap is checked. Duplicates spent the server's array budget, and 101 copies of one chat were rejected here but accepted there. Normalize, dedupe on the normalized value, then check the cap -- so a chat URL and the bare id it contains collapse into one entry too. Also reuse displayInfoHighlightRE instead of a second identical regex, and add the fanout tests the sibling had and this one lacked: concurrency cap, panic contained to one query with no stack trace leaking to stderr, and all-queries- failed surfacing a typed error that keeps the upstream status.
Three leftovers from the review, all in guidance rather than request shaping. --chat-ids and --has-chatted described themselves as narrowing "--query", written before --queries existed. Both filters apply to every keyword in a fanout, so the text told agents a combination they can use is unavailable, and the skill doc contradicted itself: one paragraph demanded --query on every call, another explained the filters applying across --queries. Both now say a keyword search, and the skill line names either flag. Kept "narrow" over the sibling's "restrict": these two filters genuinely cannot enumerate on their own. The affordance section repeated the domain-level skill, so Related skills listed lark-contact and lark-contact/SKILL.md -- the same file twice, since the merge dedupes on the raw string. +search-user's second entry is a distinct per-command reference; there is no such file here, so drop the block and let the domain entry stand alone. Validation reported the missing keyword before rejecting an explicit --has-chatted=false, so a caller passing only that flag had to fix two errors in sequence, the first of which was not what was actually wrong. +search-user lands on the =false error first because it counts a Changed bool as search input; move the check ahead of the keyword requirement to reach the same place, and pin the order with a test. Also add the fanout tests the sibling still had and this one did not: partial failure keeping the surviving query's rows plus the stderr counters, ndjson staying parseable with no summary line mixed in, and a cancelled context short-circuiting every query before it reaches the transport.
Last round hoisted the explicit =false rejection to the top of validation to fix which error a bare --has-chatted=false reports. That overshot: sitting ahead of every keyword check, it also masked them. Measured against the sibling, --query x --queries y --has-chatted=false returned --has-chatted here and the mutual-exclusion error there, and a 51-character --query alongside =false returned --has-chatted here and the length error there. Fixing one combination had broken two others. +search-user only reaches the =false error first when nothing else was supplied: a Changed bool satisfies its "at least one search input" gate, then the keyword checks no-op, then =false fires. Reproduce that shape instead of reordering wholesale -- the =false error now returns from the no-keyword branch, and otherwise stays after the keyword and chat-id checks. All four combinations now report the same param, params and message as +search-user. The partial-failure fanout test claimed to prove the surviving query's notice reaches the caller but only checked that CSV stdout contained a keyword, which comes from the row rather than the notice; deleting either notice assignment left it green. Rewritten against JSON with explicit assertions on the top-level notice, the surviving query's sidecar notice, the failed query's upstream status and the single contributed row — verified by deleting the assignment and watching it fail. The CSV summary it used to cover moved into its own test that also pins the matched_query column. Also add the fanout dry-run test: one previewed request per deduped keyword, each carrying the filter, and no page_token in the preview.
…eference Two things were wrong with how this branch edited skills/lark-contact/SKILL.md. It changed content that has nothing to do with bot search. Commit 2830d5e rewrote the user_profiles batch_query row in the command table, rewrote the paragraph introducing it, and deleted its whole example block, redirecting readers to lark-openapi-explorer. That was done to silence a local quality-gate rejection -- "example references unknown command contact user_profiles batch_query" -- which only fires because a stale internal/registry/meta_data.json carries 13 services and no contact service. Refreshing the metadata makes the gate pass with the documentation intact, so the removal deleted correct content to work around a local artifact. Restored verbatim. It also grew the bot search section inside the main skill file while the sibling keeps that depth in references/lark-contact-search-user.md, so the section had swollen past the surrounding material and still lacked the input and output contract details the sibling documents. Added references/lark-contact-search-bot.md covering the normalization rules (--queries dedupe, --chat-ids URL normalization, both caps counted after dedupe), the field contract, the fanout shape including the absent top-level has_more and the per-query error, and which --format values put the fanout counters on stderr. The main file keeps a short section that links to it, the way the +search-user row already does. Everything else in the file is now purely additive: the new table row, the new section and two new caveat bullets, with the pre-existing 41050 and ID-type bullets left untouched. The description front matter is the one edit that cannot be an addition -- a skill has a single description and the capability has to appear there for routing -- so it only inserts clauses and leaves every original word in place. Also corrects the pretty column count: the fanout table adds matched_query, so it is seven columns, not the six the old line claimed unconditionally.
Eight places -- the command Description, the --chat-ids flag help, two tips, the keyword-required error, the stderr hint, the affordance blurb and the reference doc -- called --chat-ids a narrowing filter. Live probing shows it is not: a bot inside a chat can be absent from the tenant-wide result set entirely, so naming the chat widens what is reachable rather than filtering it down. query '红黑榜' no filter -> 0 rows with 3 eng chats -> 红黑榜小助手 query '助' no filter -> 2 rows with 3 eng chats -> 红黑榜小助手 The second row is the point: the filtered result is not a subset, it is a different set. --has-chatted by contrast is a real narrowing filter -- the same '助' query drops from two rows to one, keeping the same open_id -- so wording about narrowing now refers only to it. This mattered beyond accuracy. An agent told --chat-ids "only narrows" would never reach for it to *find* a bot a plain --query cannot see, which is the only way to answer "what bot is in this chat". A tip now states that outright. Also documents that keyword matching is not substring matching: '会议助手' finds 会议小助手 across an inserted character, yet '小助手' does not find 会议小助手 and '红黑榜' does not find 红黑榜小助手, so a shorter fragment is not a safe fallback. The reference doc gains what the +search-user reference has and it lacked: the field contract as a table with types and per-field empty-value behaviour (three distinct semantics -- omitted key, empty string, empty array), a disambiguation section ranking description > has_chatted > is_agent since a single keyword returns eight near-identically named bots, a dedicated fanout section, and a table of rejected flag combinations with the param each one names. Every row of that table and every remaining example was executed before being written down.
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
1 participant
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
Add
contact +search-botfor searching bots visible to the calling user with a user token. The shortcut validates keyword and filter inputs, preserves server notices, and returns stable agent-facing fields.Changes
contact +search-botread shortcut withsearch:botscope and user authentication.--chat-idsvalue with the shared typed Chat ID validator, including URL normalization.--page-sizein the API-supported range 1-30 and preserve opaque page tokens verbatim across real and dry-run requests.+search-useroutput behavior:prettyuses the command summary whiletable,csv, andndjsonuse the generic full-field formatter.display_infowhile preserving match segments and bot metadata.lark-contactskill, remove a stale command example rejected by the manifest gate, add unit, dry-run E2E, and guarded live E2E coverage, and update the contact E2E coverage inventory.Test Plan
make unit-testgo vet ./...gofmt -l .go mod tidyleavesgo.modandgo.sumunchangedgo run github.com/golangci/golangci-lint/v2/cmd/golangci-lint@v2.1.6 run --new-from-rev=origin/mainmake build && make quality-gatenode scripts/skill-format-check/index.jsgo test -count=1 ./tests/cli_e2e/dryrun -run 'ContactSearchBot'lark-cli contact +search-botis registered and absent from theapplicationdomain.Related Issues
Summary by CodeRabbit
contact +search-botto search the current user’s visible bots by keyword.--chat-idsand--has-chatted, plus manual pagination via--page-sizeand--page-token.+search-botusage, returned fields, filters, and pagination flow.