feat(contact +search-user): add search filters and richer profile fields#648
Open
feat(contact +search-user): add search filters and richer profile fields#648
Conversation
github-actions
Bot
added
domain/contact
|
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:
📝 WalkthroughWalkthroughCentralizes "me" expansion into Changes
Sequence Diagram(s)sequenceDiagram
participant CLI as CLI
participant Runtime as RuntimeContext
participant API as Contact API
participant Formatter as Output Formatter
CLI->>Runtime: ResolveOpenIDs(--user-ids)
alt resolved
Runtime-->>CLI: resolved user_ids
CLI->>API: POST /open-apis/contact/v3/users/search (query: page_size, body: { query?, filter{ user_ids?, ... } })
API-->>CLI: response (items, has_more)
CLI->>Formatter: project items (pick i18n name, parse display_info, flatten fields)
Formatter-->>CLI: render output (pretty/table/json/ndjson) + stderr refine hint if has_more and format is human
else resolution failed
Runtime-->>CLI: validation error (flagName message)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #648 +/- ##
==========================================
+ Coverage 62.67% 63.16% +0.48%
==========================================
Files 434 486 +52
Lines 38072 41641 +3569
==========================================
+ Hits 23862 26302 +2440
- Misses 12076 13053 +977
- Partials 2134 2286 +152 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![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 the current code and only fix it if needed.
Inline comments:
In `@shortcuts/contact/contact_search_user.go`:
- Around line 105-107: The pagination hint is being written to stdout via
fmt.Fprintf(runtime.IO().Out, ...); change this to write to stderr so program
output stays on stdout: in the block guarded by hasMore &&
isHumanReadableFormat(runtime.Format) use fmt.Fprintf(runtime.IO().Err, "\n(more
available, page_token: %s)\n", pageToken) (retain the same condition, message
and pageToken variable, only replace runtime.IO().Out with runtime.IO().Err).
🪄 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: be69dc45-3529-47a2-b676-427f9e62531d
📒 Files selected for processing (7)
shortcuts/common/userids.goshortcuts/common/userids_test.goshortcuts/contact/contact_search_user.goshortcuts/contact/contact_search_user_test.goshortcuts/contact/helpers_legacy.goshortcuts/minutes/minutes_search.goshortcuts/minutes/minutes_search_test.go
💤 Files with no reviewable changes (1)
- shortcuts/minutes/minutes_search_test.go
🚀 PR Preview Install Guide🧰 CLI updatenpm i -g https://pkg.pr.new/larksuite/cli/@larksuite/cli@826a5069fb4490b67cfb04c000a55b79887994d5🧩 Skill updatenpx skills add larksuite/cli#feat/search-user-upgrade -y -g |
evandance
force-pushed
the
feat/search-user-upgrade
branch
3 times, most recently
from
214e424 to
e5152e6
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@shortcuts/common/userids_test.go`:
- Around line 6-10: The test currently asserts only that an error occurred when
invoking the command with the --user-ids flag; tighten it to assert the error
message specifically mentions the flag name so the validation is flag-scoped.
Update the assertions in userids_test.go (the failing cases around lines that
pass "--user-ids") to check error.Error() or
require.ErrorContains/strings.Contains for the substring "--user-ids" (or the
exact flag name used in the command implementation), and apply the same check to
the related cases noted (around 54-60) so regressions in flag-scoped validation
are caught.
🪄 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: 9711d85d-0f6c-4350-b0a5-26818e325020
📒 Files selected for processing (8)
shortcuts/common/userids.goshortcuts/common/userids_test.goshortcuts/contact/contact_search_user.goshortcuts/contact/contact_search_user_test.goshortcuts/contact/helpers_legacy.goshortcuts/minutes/minutes_search.goshortcuts/minutes/minutes_search_test.goskills/lark-contact/references/lark-contact-get-user.md
💤 Files with no reviewable changes (1)
- shortcuts/minutes/minutes_search_test.go
🚧 Files skipped from review as they are similar to previous changes (3)
- shortcuts/contact/helpers_legacy.go
- shortcuts/common/userids.go
- shortcuts/contact/contact_search_user_test.go
evandance
force-pushed
the
feat/search-user-upgrade
branch
from
e5152e6 to
2225c3d
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
shortcuts/contact/contact_search_user.go (1)
104-106:⚠️ Potential issue | 🟡 MinorSend the pagination hint to stderr.
Line 105 still writes the non-result hint to stdout; route it to
runtime.IO().ErrOutinstead.
As per coding guidelines, Program output (JSON envelopes) must go to stdout; progress, warnings, and hints must go to stderr.Proposed fix
if hasMore && isHumanReadableFormat(runtime.Format) { - fmt.Fprintf(runtime.IO().Out, "\n(more available, page_token: %s)\n", pageToken) + fmt.Fprintf(runtime.IO().ErrOut, "\n(more available, page_token: %s)\n", pageToken) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@shortcuts/contact/contact_search_user.go` around lines 104 - 106, The pagination hint is incorrectly written to stdout using fmt.Fprintf(runtime.IO().Out...) in the contact search output; change that write to stderr by using runtime.IO().ErrOut instead so progress/hints (when hasMore && isHumanReadableFormat(runtime.Format)) go to stderr while JSON/program output stays on stdout—locate the fmt.Fprintf call in the contact_search_user.go code block that checks hasMore and isHumanReadableFormat(runtime.Format) and switch the output writer to runtime.IO().ErrOut.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@shortcuts/contact/helpers_legacy.go`:
- Around line 10-27: Add table-driven unit tests for pickUserName and
firstNonEmpty that assert the legacy name fallback order and edge cases: create
cases where multiple keys exist to verify priority ("name", "user_name",
"display_name", "employee_name", "cn_name"), cases where the higher-priority key
is an empty string so the helpers skip it and pick the next non-empty, and cases
with no matching keys returning empty string; implement assertions by calling
pickUserName and firstNonEmpty with maps using those keys and expected results
to cover priority, empty-string skipping, and no-match fallback.
---
Duplicate comments:
In `@shortcuts/contact/contact_search_user.go`:
- Around line 104-106: The pagination hint is incorrectly written to stdout
using fmt.Fprintf(runtime.IO().Out...) in the contact search output; change that
write to stderr by using runtime.IO().ErrOut instead so progress/hints (when
hasMore && isHumanReadableFormat(runtime.Format)) go to stderr while
JSON/program output stays on stdout—locate the fmt.Fprintf call in the
contact_search_user.go code block that checks hasMore and
isHumanReadableFormat(runtime.Format) and switch the output writer to
runtime.IO().ErrOut.
🪄 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: ce8ec9fa-378e-449c-92d0-0c40ef823c92
📒 Files selected for processing (8)
shortcuts/common/userids.goshortcuts/common/userids_test.goshortcuts/contact/contact_search_user.goshortcuts/contact/contact_search_user_test.goshortcuts/contact/helpers_legacy.goshortcuts/minutes/minutes_search.goshortcuts/minutes/minutes_search_test.goskills/lark-contact/references/lark-contact-get-user.md
💤 Files with no reviewable changes (1)
- shortcuts/minutes/minutes_search_test.go
🚧 Files skipped from review as they are similar to previous changes (4)
- skills/lark-contact/references/lark-contact-get-user.md
- shortcuts/common/userids_test.go
- shortcuts/minutes/minutes_search.go
- shortcuts/contact/contact_search_user_test.go
evandance
force-pushed
the
feat/search-user-upgrade
branch
2 times, most recently
from
1b69afe to
083ed14
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@shortcuts/common/userids.go`:
- Around line 21-33: The deduplication uses seen[id] which is case-sensitive so
IDs like "ou_a" and "OU_A" both pass; change the dedupe key to a case-normalized
form (e.g., key := strings.ToLower(id)) after resolving "me" (use currentUserID
and flagName as already done), then check and set seen[key] rather than seen[id]
so duplicates are removed case-insensitively while still appending the original
resolved id to out.
🪄 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: 7c8159db-2c17-4a22-8e6e-c1ccd3a3ed37
📒 Files selected for processing (9)
shortcuts/common/userids.goshortcuts/common/userids_test.goshortcuts/contact/contact_search_user.goshortcuts/contact/contact_search_user_test.goshortcuts/contact/helpers_legacy.goshortcuts/contact/helpers_legacy_test.goshortcuts/minutes/minutes_search.goshortcuts/minutes/minutes_search_test.goskills/lark-contact/references/lark-contact-get-user.md
💤 Files with no reviewable changes (1)
- shortcuts/minutes/minutes_search_test.go
✅ Files skipped from review due to trivial changes (1)
- shortcuts/contact/helpers_legacy.go
🚧 Files skipped from review as they are similar to previous changes (2)
- skills/lark-contact/references/lark-contact-get-user.md
- shortcuts/common/userids_test.go
evandance
force-pushed
the
feat/search-user-upgrade
branch
from
083ed14 to
e807c6d
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
skills/lark-contact/references/lark-contact-search-user.md (1)
160-160: Minor: "必须使用--format json" is overly strong.The shortcut advertises
HasFormat: trueand supportspretty/table/ndjson/csvas well; only the jq-chaining example on line 135 actually requires JSON. Consider softening to "推荐使用--format json（或ndjson）便于 jq 解析" to avoid implying other formats are unsupported.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@skills/lark-contact/references/lark-contact-search-user.md` at line 160, The phrase "必须使用 `--format json` 输出，便于 jq / 解析。" is too strong; update the sentence that currently forces `--format json` (the line that reads "必须使用 `--format json` 输出，便于 jq / 解析。") to a softer recommendation such as "推荐使用 `--format json`（或 `ndjson`）便于 jq 解析"，so it acknowledges that `pretty`/`table`/`csv` are supported and only the jq-chaining example requires JSON/NDJSON; keep the rest of the paragraph and examples unchanged.shortcuts/contact/contact_search_user.go (1)
238-251: Optional: hoist resolveduser-idsto avoid repeatingSplitCSV + ResolveOpenIDs.The same CSV parse and
ResolveOpenIDspipeline runs again inbuildSearchUserBody(lines 290–298). SinceValidateruns beforeExecute, you could compute the resolved list once (e.g., store on a small request struct, or a helperresolvedUserIDs(runtime)) and reuse it, making it harder for the two code paths to drift (e.g., if cap/validation is tightened in one place but not the other).Non-blocking — current behavior is correct.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@shortcuts/contact/contact_search_user.go` around lines 238 - 251, Hoist the CSV parse + ResolveOpenIDs pipeline so it's computed once during validation and reused in body construction: in the Validate method (where the snippet lives), parse and resolve runtime.Str("user-ids") using common.SplitCSV and common.ResolveOpenIDs, enforce the maxSearchUserUserIDs cap and run common.ValidateUserID, then store the resulting []string on the request struct (or return it from a small helper like resolvedUserIDs(runtime)). Update buildSearchUserBody to read that stored resolved list instead of re-running SplitCSV+ResolveOpenIDs, keeping references to common.ResolveOpenIDs, common.SplitCSV, common.ValidateUserID, maxSearchUserUserIDs and buildSearchUserBody.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@skills/lark-contact/references/lark-contact-search-user.md`:
- Around line 77-78: Doc and code disagree about bool-filter false: the CLI
currently sends filter.has_contact: false whenever the flag was explicitly
provided (Detected via Changed()), but the doc claims `=false` is equivalent to
omitting the flag. Fix by either (A) changing the shortcut builder so it only
sets filter.has_contact when Changed() && value == true (preserving doc
behavior) — locate the contact search shortcut builder where Changed() is used
and remove/guard the assignment when value is false — or (B) update the
lark-contact-search-user.md wording to state that an explicit
`--has-contact=false` is preserved and sent on the wire (mention tri-state
explicit-false vs unset); pick one approach and make the corresponding single
change (code: modify the Changed() handling around filter.has_contact; docs:
update the footnote and troubleshooting row to reflect explicit false is
transmitted).
---
Nitpick comments:
In `@shortcuts/contact/contact_search_user.go`:
- Around line 238-251: Hoist the CSV parse + ResolveOpenIDs pipeline so it's
computed once during validation and reused in body construction: in the Validate
method (where the snippet lives), parse and resolve runtime.Str("user-ids")
using common.SplitCSV and common.ResolveOpenIDs, enforce the
maxSearchUserUserIDs cap and run common.ValidateUserID, then store the resulting
[]string on the request struct (or return it from a small helper like
resolvedUserIDs(runtime)). Update buildSearchUserBody to read that stored
resolved list instead of re-running SplitCSV+ResolveOpenIDs, keeping references
to common.ResolveOpenIDs, common.SplitCSV, common.ValidateUserID,
maxSearchUserUserIDs and buildSearchUserBody.
In `@skills/lark-contact/references/lark-contact-search-user.md`:
- Line 160: The phrase "必须使用 `--format json` 输出，便于 jq / 解析。" is too strong;
update the sentence that currently forces `--format json` (the line that reads
"必须使用 `--format json` 输出，便于 jq / 解析。") to a softer recommendation such as "推荐使用
`--format json`（或 `ndjson`）便于 jq 解析"，so it acknowledges that
`pretty`/`table`/`csv` are supported and only the jq-chaining example requires
JSON/NDJSON; keep the rest of the paragraph and examples unchanged.
🪄 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: 7df917f6-3d7a-436d-9e2a-215503e085ab
📒 Files selected for processing (11)
shortcuts/common/userids.goshortcuts/common/userids_test.goshortcuts/contact/contact_search_user.goshortcuts/contact/contact_search_user_test.goshortcuts/contact/helpers_legacy.goshortcuts/contact/helpers_legacy_test.goshortcuts/minutes/minutes_search.goshortcuts/minutes/minutes_search_test.goskills/lark-contact/SKILL.mdskills/lark-contact/references/lark-contact-get-user.mdskills/lark-contact/references/lark-contact-search-user.md
💤 Files with no reviewable changes (1)
- shortcuts/minutes/minutes_search_test.go
✅ Files skipped from review due to trivial changes (5)
- shortcuts/contact/helpers_legacy.go
- shortcuts/common/userids_test.go
- shortcuts/contact/helpers_legacy_test.go
- skills/lark-contact/SKILL.md
- shortcuts/contact/contact_search_user_test.go
🚧 Files skipped from review as they are similar to previous changes (3)
- skills/lark-contact/references/lark-contact-get-user.md
- shortcuts/common/userids.go
- shortcuts/minutes/minutes_search.go
evandance
force-pushed
the
feat/search-user-upgrade
branch
4 times, most recently
from
19a67c0 to
4adc9bc
Compare
Collaborator
Author
|
@coderabbitai resume |
✅ Actions performedReviews resumed. |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
shortcuts/contact/contact_search_user.go (3)
229-252:primarymay contain a duplicate locale.When
--langhappens to equal one of the brand-preferred locales (e.g.--lang en_uson a Lark brand, or--lang zh_cnon Feishu), the resultingprimaryslice is["en_us", "en_us", "zh_cn"]/["zh_cn", "zh_cn", "en_us"]and the loop visits the same key twice. Harmless, but a tiny dedup (or orderingswitchso the explicit lang short-circuits via the prepend only) would read cleaner.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@shortcuts/contact/contact_search_user.go` around lines 229 - 252, pickName builds a primary locale list that can contain duplicates when lang equals a brand-preferred locale; update pickName so before appending each brand locale (the "en_us"/"zh_cn" entries) you check if that locale is already present in primary (or use a small set to dedupe) to avoid duplicates; reference variables/functions: pickName, primary, lang, brand, i18n, fixedLocaleFallback — ensure the loop behavior and fallback order remain unchanged while preventing duplicate entries in primary.
339-352: Consistency: guardruntime.Cmdbefore.Flags().Changed(...).
hasAnySearchInput(line 347) andbuildSearchUserBody(line 377) dereferenceruntime.Cmd.Flags()unconditionally, whereassearchUserPaginationConfig(lines 177, 181) andvalidateSearchUser(line 327) in the same file defensively checkruntime.Cmd != nil &&first. Either the guards elsewhere are unnecessary, or these two sites are a latent nil-deref risk. Worth aligning one way or the other so the invariant is unambiguous.Also applies to: 376-380
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@shortcuts/contact/contact_search_user.go` around lines 339 - 352, The functions hasAnySearchInput and buildSearchUserBody currently call runtime.Cmd.Flags().Changed(...) without ensuring runtime.Cmd is non-nil; align them with other functions (searchUserPaginationConfig, validateSearchUser) by guarding runtime.Cmd first. Add a nil check (e.g., if runtime.Cmd == nil { return false } in hasAnySearchInput, and similarly short-circuit or skip flag checks in buildSearchUserBody) before iterating searchUserBoolFilters or calling Flags().Changed to avoid a nil-deref on runtime.Cmd while preserving existing behavior.
183-188: Cap is unreachable given the upstream validation.
validateSearchUserrejects--page-limitoutside[1, 40]at lines 328–330, so by the timesearchUserPaginationConfigruns,runtime.Int("page-limit")is guaranteed to be ≤maxSearchUserPageLimit; theif pageLimit > maxSearchUserPageLimitbranch is dead. Keeping it is fine as defensive code, but you can drop it (or replace with a comment) to avoid the false impression that the config layer silently caps invalid values.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@shortcuts/contact/contact_search_user.go` around lines 183 - 188, The branch in searchUserPaginationConfig that checks "if pageLimit > maxSearchUserPageLimit" is unreachable because validateSearchUser already enforces runtime.Int("page-limit") ∈ [1, maxSearchUserPageLimit]; remove that dead check (or replace it with a short comment) to avoid implying the config layer silently caps values; keep references to validateSearchUser, searchUserPaginationConfig, maxSearchUserPageLimit, runtime.Int("page-limit") and runtime.Bool("page-all") when locating and updating the code.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@shortcuts/contact/contact_search_user.go`:
- Around line 95-101: The nil-slice declaration of allUsers causes JSON to
render "users": null for empty results; change the declaration of allUsers to
initialize it as a non-nil empty slice (e.g., an empty
[]map[string]interface{}), and apply the same initialization for the other
user-collection variable referenced around lines 132-143 so structured output
always emits [] not null; also add/modify TestSearchUser_Integration_EmptyResult
to assert the JSON-format response `data.users` is an empty array (not null) to
prevent regressions.
In `@skills/lark-contact/references/lark-contact-search-user.md`:
- Around line 141-143: The fenced code block containing the sample terminal
warning ("warning: stopped after fetching N page(s); refine the query with more
filters, or raise --page-limit (max 40)") is missing a language tag; update that
fenced block in lark-contact-search-user.md to use a language identifier such as
text or console (e.g., change ``` to ```text) so markdownlint MD040 is satisfied
and the sample is correctly recognized as plain terminal output.
---
Nitpick comments:
In `@shortcuts/contact/contact_search_user.go`:
- Around line 229-252: pickName builds a primary locale list that can contain
duplicates when lang equals a brand-preferred locale; update pickName so before
appending each brand locale (the "en_us"/"zh_cn" entries) you check if that
locale is already present in primary (or use a small set to dedupe) to avoid
duplicates; reference variables/functions: pickName, primary, lang, brand, i18n,
fixedLocaleFallback — ensure the loop behavior and fallback order remain
unchanged while preventing duplicate entries in primary.
- Around line 339-352: The functions hasAnySearchInput and buildSearchUserBody
currently call runtime.Cmd.Flags().Changed(...) without ensuring runtime.Cmd is
non-nil; align them with other functions (searchUserPaginationConfig,
validateSearchUser) by guarding runtime.Cmd first. Add a nil check (e.g., if
runtime.Cmd == nil { return false } in hasAnySearchInput, and similarly
short-circuit or skip flag checks in buildSearchUserBody) before iterating
searchUserBoolFilters or calling Flags().Changed to avoid a nil-deref on
runtime.Cmd while preserving existing behavior.
- Around line 183-188: The branch in searchUserPaginationConfig that checks "if
pageLimit > maxSearchUserPageLimit" is unreachable because validateSearchUser
already enforces runtime.Int("page-limit") ∈ [1, maxSearchUserPageLimit]; remove
that dead check (or replace it with a short comment) to avoid implying the
config layer silently caps values; keep references to validateSearchUser,
searchUserPaginationConfig, maxSearchUserPageLimit, runtime.Int("page-limit")
and runtime.Bool("page-all") when locating and updating the code.
🪄 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: fb310216-fdef-4b1a-b56c-24b631819af9
📒 Files selected for processing (11)
shortcuts/common/userids.goshortcuts/common/userids_test.goshortcuts/contact/contact_search_user.goshortcuts/contact/contact_search_user_test.goshortcuts/contact/helpers_legacy.goshortcuts/contact/helpers_legacy_test.goshortcuts/minutes/minutes_search.goshortcuts/minutes/minutes_search_test.goskills/lark-contact/SKILL.mdskills/lark-contact/references/lark-contact-get-user.mdskills/lark-contact/references/lark-contact-search-user.md
💤 Files with no reviewable changes (1)
- shortcuts/minutes/minutes_search_test.go
✅ Files skipped from review due to trivial changes (5)
- shortcuts/contact/helpers_legacy_test.go
- shortcuts/contact/helpers_legacy.go
- shortcuts/common/userids_test.go
- shortcuts/minutes/minutes_search.go
- skills/lark-contact/SKILL.md
🚧 Files skipped from review as they are similar to previous changes (2)
- skills/lark-contact/references/lark-contact-get-user.md
- shortcuts/common/userids.go
- Filter results by chat history, employment status, tenant boundary,
or enterprise email presence; keyword is now optional so filter-only
queries ("list all my external contacts") work end-to-end.
- Each result now carries multilingual names, contact email, activation
state, whether you've chatted with them, tenant context, user
signature, and a hit-highlight line that surfaces the matched segment
and the user's department path.
- Always-empty legacy columns and fields the new backend no longer
returns are dropped.
- Also fixes the contact +get-user skill doc, which previously
instructed callers to pass --table (a flag that never existed); now
correctly documents --format table and the full --format enum.
evandance
force-pushed
the
feat/search-user-upgrade
branch
from
4adc9bc to
1667856
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
shortcuts/contact/contact_search_user.go (1)
233-242: Optional: dedupprimaryto avoid redundant lookups when--langmatches a brand default.If a user passes
--lang zh-CNunderBrandFeishu(or--lang en-USunderBrandLark),primarybecomes["zh_cn", "zh_cn", "en_us"]. Correctness is preserved because the first hit wins, but empty-string values trigger an extra (harmless) lookup. Minor readability/efficiency nit.♻️ Proposed tweak
primary := make([]string, 0, 3) + add := func(loc string) { + for _, existing := range primary { + if existing == loc { + return + } + } + primary = append(primary, loc) + } if lang != "" { - primary = append(primary, strings.ReplaceAll(strings.ToLower(lang), "-", "_")) + add(strings.ReplaceAll(strings.ToLower(lang), "-", "_")) } switch brand { case core.BrandLark: - primary = append(primary, "en_us", "zh_cn") + add("en_us") + add("zh_cn") default: // feishu or unknown brand: Chinese first - primary = append(primary, "zh_cn", "en_us") + add("zh_cn") + add("en_us") }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@shortcuts/contact/contact_search_user.go` around lines 233 - 242, The primary slice construction can produce duplicate entries when lang normalizes to a brand default (e.g., "zh_cn" twice); update the logic around the primary variable (the code that appends the normalized lang and the switch on brand / core.BrandLark) to deduplicate while preserving order — either by checking for existence before appending or by rebuilding primary through a simple seen map to filter duplicates — so that redundant lookups are avoided but the first-match order remains unchanged.
🤖 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/contact/contact_search_user.go`:
- Around line 233-242: The primary slice construction can produce duplicate
entries when lang normalizes to a brand default (e.g., "zh_cn" twice); update
the logic around the primary variable (the code that appends the normalized lang
and the switch on brand / core.BrandLark) to deduplicate while preserving order
— either by checking for existence before appending or by rebuilding primary
through a simple seen map to filter duplicates — so that redundant lookups are
avoided but the first-match order remains unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: aca7bcac-3bd4-4ca9-95a5-74c8969380fb
📒 Files selected for processing (11)
shortcuts/common/userids.goshortcuts/common/userids_test.goshortcuts/contact/contact_search_user.goshortcuts/contact/contact_search_user_test.goshortcuts/contact/helpers_legacy.goshortcuts/contact/helpers_legacy_test.goshortcuts/minutes/minutes_search.goshortcuts/minutes/minutes_search_test.goskills/lark-contact/SKILL.mdskills/lark-contact/references/lark-contact-get-user.mdskills/lark-contact/references/lark-contact-search-user.md
💤 Files with no reviewable changes (1)
- shortcuts/minutes/minutes_search_test.go
✅ Files skipped from review due to trivial changes (5)
- skills/lark-contact/references/lark-contact-get-user.md
- shortcuts/contact/helpers_legacy.go
- shortcuts/contact/helpers_legacy_test.go
- skills/lark-contact/SKILL.md
- skills/lark-contact/references/lark-contact-search-user.md
🚧 Files skipped from review as they are similar to previous changes (1)
- shortcuts/minutes/minutes_search.go
liangshuo-1
force-pushed
the
feat/search-user-upgrade
branch
from
8873644 to
9a65cc8
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@shortcuts/contact/contact_search_user.go`:
- Around line 197-199: The json.Unmarshal error path currently returns
output.ErrWithHint(output.ExitInternal, "validation", ...) which misclassifies a
server-side JSON decode failure; update the error kind to something like
"api_error" or "decode" (e.g., replace the "validation" string in the
json.Unmarshal block that checks apiResp.RawBody and populates resp) so it
matches ExitInternal semantics and aligns with internal/client/api_errors.go
conventions; apply the same change for the identical patterns found in
calendar_suggestion.go and calendar_room_find.go where json.Unmarshal uses
output.ErrWithHint.
- Around line 238-261: parseDisplayInfo currently assigns department =
strings.TrimSpace(lines[1]) which can pick up a recency hint; change the logic
in parseDisplayInfo to find the department by scanning the lines (e.g., from top
after highlights) and only assign a line as department if it's non-empty and
does not match displayInfoRecencyRE; ensure you still extract recencyHint by
testing lines (e.g., from bottom) with displayInfoRecencyRE as before and keep
displayInfoHighlightRE usage unchanged so highlights, department, and recency
are set according to the documented "extract by role" contract.
🪄 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: c26d5b1f-0410-4fb7-9c60-8690484ce775
📒 Files selected for processing (5)
shortcuts/contact/contact_search_user.goshortcuts/contact/contact_search_user_test.goskills/lark-contact/SKILL.mdskills/lark-contact/references/lark-contact-get-user.mdskills/lark-contact/references/lark-contact-search-user.md
✅ Files skipped from review due to trivial changes (1)
- skills/lark-contact/references/lark-contact-search-user.md
Comment on lines
+197
to
+199
| if err := json.Unmarshal(apiResp.RawBody, &resp); err != nil { | ||
| return output.ErrWithHint(output.ExitInternal, "validation", "unmarshal response failed", err.Error()) | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Confirm the ErrWithHint signature and how 'kind' is exposed in structured output,
# and survey other call sites to see what kinds are conventionally used.
ast-grep --pattern $'func ErrWithHint($$$) $_ {
$$$
}'
rg -nP '\boutput\.ErrWithHint\s*\(' -C1 --type=goRepository: larksuite/cli
Length of output: 17165
Misleading error kind for JSON response parsing failure.
The error uses "validation" as the kind with ExitInternal, but "validation" conventionally signals user-input problems elsewhere in the codebase (config validation, parameter validation, etc.). For a server-side JSON parsing failure, this creates a semantic mismatch that can confuse structured error consumers.
This pattern appears in multiple shortcuts files (calendar_suggestion.go, calendar_room_find.go). Consider using "api_error" or "decode" to match the ExitInternal semantics, consistent with how internal/client/api_errors.go handles similar JSON decode failures.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@shortcuts/contact/contact_search_user.go` around lines 197 - 199, The
json.Unmarshal error path currently returns
output.ErrWithHint(output.ExitInternal, "validation", ...) which misclassifies a
server-side JSON decode failure; update the error kind to something like
"api_error" or "decode" (e.g., replace the "validation" string in the
json.Unmarshal block that checks apiResp.RawBody and populates resp) so it
matches ExitInternal semantics and aligns with internal/client/api_errors.go
conventions; apply the same change for the identical patterns found in
calendar_suggestion.go and calendar_room_find.go where json.Unmarshal uses
output.ErrWithHint.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
shortcuts/contact/contact_search_user.go (1)
197-199:⚠️ Potential issue | 🟡 MinorMisleading error kind for response unmarshal failure.
output.ErrWithHint(output.ExitInternal, "validation", ...)uses the"validation"kind, which the rest of the codebase reserves for user-input/config problems. A server-side JSON decode failure is better classified as"api_error"or"decode"so structured-error consumers don't treat it as a flag/validation issue.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@shortcuts/contact/contact_search_user.go` around lines 197 - 199, The JSON unmarshal error is being reported with the "validation" kind which is reserved for user input issues; update the error kind in the json.Unmarshal failure handling (the block using apiResp.RawBody, resp and calling output.ErrWithHint with output.ExitInternal) to a server-side kind such as "api_error" or "decode" so consumers won't treat it as a user validation problem—replace the `"validation"` argument passed to output.ErrWithHint(...) with `"api_error"` (or `"decode"`) while keeping the same exit code and message.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@shortcuts/contact/contact_search_user.go`:
- Line 11: Remove the net/http import and replace all usages with literals and
DoAPI: change http.MethodPost to "POST", change http.StatusOK to the literal
200, and remove the http.StatusText(...) call; perform the HTTP call using
RuntimeContext.DoAPI(...) (which handles auth and normalized errors) and only
inspect the raw response.StatusCode for the specific error branch. Ensure you
drop the net/http import from contact_search_user.go and update any
variable/conditional that referenced http.StatusText to use the numeric status
check and custom handling instead.
---
Duplicate comments:
In `@shortcuts/contact/contact_search_user.go`:
- Around line 197-199: The JSON unmarshal error is being reported with the
"validation" kind which is reserved for user input issues; update the error kind
in the json.Unmarshal failure handling (the block using apiResp.RawBody, resp
and calling output.ErrWithHint with output.ExitInternal) to a server-side kind
such as "api_error" or "decode" so consumers won't treat it as a user validation
problem—replace the `"validation"` argument passed to output.ErrWithHint(...)
with `"api_error"` (or `"decode"`) while keeping the same exit code and message.
🪄 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: 7b0fa872-8034-445d-8bd1-9cadf85c001b
📒 Files selected for processing (5)
shortcuts/contact/contact_search_user.goshortcuts/contact/contact_search_user_test.goskills/lark-contact/SKILL.mdskills/lark-contact/references/lark-contact-get-user.mdskills/lark-contact/references/lark-contact-search-user.md
🚧 Files skipped from review as they are similar to previous changes (2)
- skills/lark-contact/SKILL.md
- shortcuts/contact/contact_search_user_test.go
| "fmt" | ||
| "io" | ||
| "math" | ||
| "net/http" |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# How do other shortcuts express HTTP method / status without importing net/http?
rg -nP --type=go -C2 'HttpMethod\s*:\s*' shortcuts/ | head -50
rg -nP --type=go -C2 'StatusCode\s*!=' shortcuts/ | head -40
rg -nP --type=go '"net/http"' shortcuts/ | headRepository: larksuite/cli
Length of output: 6524
🏁 Script executed:
# Check for depguard config
find . -name ".depguard*" -o -name "depguard*" -o -name ".golangci*" | grep -v node_modules | head -20Repository: larksuite/cli
Length of output: 73
🏁 Script executed:
# Look for Go CI config files
fd -e yaml -e yml | grep -E "(golangci|lint|ci)" | head -20Repository: larksuite/cli
Length of output: 39
🏁 Script executed:
# Check the actual contact_search_user.go to see current state
cat -n shortcuts/contact/contact_search_user.go | head -30Repository: larksuite/cli
Length of output: 829
🏁 Script executed:
# Read the golangci config to check depguard rules
cat -n .golangci.ymlRepository: larksuite/cli
Length of output: 7290
🏁 Script executed:
# Find shortcuts files that DON'T import net/http
cd shortcuts && for f in $(fd -e go -type f); do if ! grep -q '"net/http"' "$f"; then echo "$f"; fi; done | head -20Repository: larksuite/cli
Length of output: 228
🏁 Script executed:
# Check task_get_my_tasks.go which uses string "GET" instead of http.MethodGet
cat -n shortcuts/task/task_get_my_tasks.go | sed -n '110,130p'Repository: larksuite/cli
Length of output: 772
🏁 Script executed:
# Check how status codes are checked in files without net/http
rg -l "StatusCode" shortcuts/ --type=go | while read f; do if ! grep -q '"net/http"' "$f"; then echo "=== $f ==="; grep -n "StatusCode" "$f"; fi; doneRepository: larksuite/cli
Length of output: 1678
🏁 Script executed:
# Check git history of contact_search_user.go
git log --oneline shortcuts/contact/contact_search_user.go | head -5Repository: larksuite/cli
Length of output: 138
🏁 Script executed:
# Check what whiteboard_update.go looks like - does it import net/http?
head -20 shortcuts/whiteboard/whiteboard_update.go | grep -A2 "import"Repository: larksuite/cli
Length of output: 94
🏁 Script executed:
# Verify: do files with net/http import currently exist and pass linting?
# Check one of the files that imports net/http from our results
rg '"net/http"' shortcuts/whiteboard/whiteboard_update.go
rg 'HttpMethod.*:' shortcuts/whiteboard/whiteboard_update.go | head -3Repository: larksuite/cli
Length of output: 133
🏁 Script executed:
# Check when the depguard rule was added - look at git history of .golangci.yml
git log --oneline --all -S 'shortcuts-no-raw-http' -- .golangci.yml | head -5Repository: larksuite/cli
Length of output: 221
🏁 Script executed:
# Check if there's a CI config that would show failing checks
ls -la | grep -E "(\.github|\.gitlab|\.circleci|Makefile)" | head -10Repository: larksuite/cli
Length of output: 178
🏁 Script executed:
# Check if this is a recently added rule by looking at recent commits
git log --oneline -20 .golangci.ymlRepository: larksuite/cli
Length of output: 138
🏁 Script executed:
# Check GitHub workflows for linting
find .github -name "*.yml" -o -name "*.yaml" | xargs cat | grep -A5 -B5 "golangci\|depguard" | head -40Repository: larksuite/cli
Length of output: 474
🏁 Script executed:
# Check Makefile for lint targets
cat Makefile | grep -A10 "lint"Repository: larksuite/cli
Length of output: 39
CI blocker: net/http import violates depguard rule shortcuts-no-raw-http.
The CI pipeline enforces a depguard rule that forbids net/http imports in shortcuts/. Since this file was recently modified, the import at line 11 will fail linting. Replace the three HTTP uses with literals: http.MethodPost → "POST" (line 184), http.StatusOK → 200 (line 192), and remove http.StatusText() (line 193). Then drop the net/http import.
The pattern is consistent with other shortcuts (e.g., task_get_my_tasks.go uses "GET" string literal). Use RuntimeContext.DoAPI() for the request itself, which handles auth and error normalization; you only need to check the raw status code for this specific error path.
Proposed fix
import (
"context"
"encoding/json"
"fmt"
"io"
- "net/http"
"regexp"
"sort"
"strconv"
"strings"
"unicode/utf8"
@@
apiResp, err := runtime.DoAPI(&larkcore.ApiReq{
- HttpMethod: http.MethodPost,
+ HttpMethod: "POST",
ApiPath: searchUserURL,
Body: body,
QueryParams: larkcore.QueryParams{"page_size": []string{strconv.Itoa(runtime.Int("page-size"))}},
})
if err != nil {
return err
}
- if apiResp.StatusCode != http.StatusOK {
- return output.ErrAPI(apiResp.StatusCode, http.StatusText(apiResp.StatusCode), string(apiResp.RawBody))
+ if apiResp.StatusCode != 200 {
+ return output.ErrAPI(apiResp.StatusCode, "", string(apiResp.RawBody))
}🧰 Tools
🪛 GitHub Check: lint
[failure] 11-11:
import 'net/http' is not allowed from list 'shortcuts-no-raw-http': use RuntimeContext.DoAPI/CallAPI/DoAPIJSON instead of raw net/http. The client layer handles auth, headers, and error normalization. (depguard)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@shortcuts/contact/contact_search_user.go` at line 11, Remove the net/http
import and replace all usages with literals and DoAPI: change http.MethodPost to
"POST", change http.StatusOK to the literal 200, and remove the
http.StatusText(...) call; perform the HTTP call using RuntimeContext.DoAPI(...)
(which handles auth and normalized errors) and only inspect the raw
response.StatusCode for the specific error branch. Ensure you drop the net/http
import from contact_search_user.go and update any variable/conditional that
referenced http.StatusText to use the numeric status check and custom handling
instead.
- contact_search_user.go / _test.go: simplify and clarify - SKILL.md: focus description on user-facing trigger scenarios; rework decision table; trim notes to load-bearing constraints - references/lark-contact-search-user.md: add flag table covering all four bool filters; add multi-filter examples; clean up output field contract (drop server <h> tag implementation detail) - references/lark-contact-get-user.md: trim to two real use cases (self via user identity; full profile of others via bot identity); point user-mode-by-id users to +search-user instead - .golangci.yml: replace package-level deny on net/http with a symbol-level forbidigo rule. Constants (http.MethodPost, http.StatusOK) and helpers (http.StatusText) were never the intent; only Client / NewRequest / Get / Post / Do etc. are now blocked in shortcuts/, matching the rule's actual purpose Change-Id: Ic42043d3f4c1b675800e48229c7ba2e970da26fe
liangshuo-1
force-pushed
the
feat/search-user-upgrade
branch
from
9a65cc8 to
60ba249
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.golangci.yml:
- Around line 85-90: The forbid-pattern includes a non-existent package-level
symbol `http.Do` which never matches real usage (Do is a method on
*http.Client); update the regex in the rule that currently contains `http\.Do`
to remove that token (i.e., delete `http\.Do` from the alternation) so the
pattern only lists actual package-level functions like
`http\.Client|http\.NewRequest|...` or, if you intended to catch method calls,
replace `http\.Do` with a pattern that matches method calls (e.g., `\.Do\b`
scoped appropriately), but the simplest fix is to remove `http\.Do` from the
alternation.
In `@shortcuts/contact/contact_search_user_test.go`:
- Around line 259-260: The test's hint-flags assertion loop is missing the
"--has-enterprise-email" entry, so update the slice of expected flags used in
the for loop (the []string{"--query", "--user-ids", "--has-chatted",
"--exclude-external-users", "--left-organization"}) to include
"--has-enterprise-email" so the test checks that err.Error() contains that flag
in the strings.Contains(err.Error(), want) assertions.
- Around line 799-811: The test TestSearchUser_Integration_NoAutoPaginationFlags
currently treats any non-nil error as a pass which can hide runtime failures;
update the test to ensure the failure is specifically due to an unknown/invalid
flag from parsing. After calling mountAndRun with ContactSearchUser, assert that
err is non-nil and that err.Error() contains a flag-parsing indicator like
"unknown flag" or "flag provided but not defined"; alternatively invoke the
command's flag parsing step directly (e.g., parse args on the command) before
execution so you only assert parse-time errors from the flag parsing phase.
🪄 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: 334095bc-f652-4c11-89a6-91f780b81814
📒 Files selected for processing (6)
.golangci.ymlshortcuts/contact/contact_search_user.goshortcuts/contact/contact_search_user_test.goskills/lark-contact/SKILL.mdskills/lark-contact/references/lark-contact-get-user.mdskills/lark-contact/references/lark-contact-search-user.md
✅ Files skipped from review due to trivial changes (2)
- skills/lark-contact/references/lark-contact-search-user.md
- shortcuts/contact/contact_search_user.go
Comment on lines
+85
to
+90
| - pattern: http\.(Client|NewRequest|NewRequestWithContext|Get|Post|PostForm|Head|DefaultClient|DefaultTransport|RoundTripper|Do|Serve|ListenAndServe)\b | ||
| msg: >- | ||
| [shortcuts-no-raw-http] use RuntimeContext.DoAPI/CallAPI/DoAPIJSON | ||
| instead of constructing raw HTTP. The runtime handles auth, headers, | ||
| and error normalization. (Constants and helpers like http.MethodPost, | ||
| http.StatusOK, http.StatusText remain allowed.) |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Go net/http package API: does a package-level function http.Doexist, or isDoonly a method on*http.Client?
💡 Result:
No, there is no package-level function http.Do in the Go net/http package. Do is only a method on *http.Client.
Citations:
- 1: https://pkg.go.dev/net/http
- 2: https://pkg.go.dev/net/http@go1.25.1
- 3: https://pkg.go.dev/http
- 4: https://go.dev/src/net/http/doc.go
Remove http.Do from the forbid pattern—it does not exist as a package-level function in net/http.
Do is only a method on *http.Client, so the regex http\.Do\b will not match actual usage of (*http.Client).Do(...). The pattern includes non-existent functions that create misleading coverage.
Suggested fix
- - pattern: http\.(Client|NewRequest|NewRequestWithContext|Get|Post|PostForm|Head|DefaultClient|DefaultTransport|RoundTripper|Do|Serve|ListenAndServe)\b
+ - pattern: http\.(Client|NewRequest|NewRequestWithContext|Get|Post|PostForm|Head|DefaultClient|DefaultTransport|RoundTripper|Serve|ListenAndServe)\b📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| - pattern: http\.(Client|NewRequest|NewRequestWithContext|Get|Post|PostForm|Head|DefaultClient|DefaultTransport|RoundTripper|Do|Serve|ListenAndServe)\b | |
| msg: >- | |
| [shortcuts-no-raw-http] use RuntimeContext.DoAPI/CallAPI/DoAPIJSON | |
| instead of constructing raw HTTP. The runtime handles auth, headers, | |
| and error normalization. (Constants and helpers like http.MethodPost, | |
| http.StatusOK, http.StatusText remain allowed.) | |
| - pattern: http\.(Client|NewRequest|NewRequestWithContext|Get|Post|PostForm|Head|DefaultClient|DefaultTransport|RoundTripper|Serve|ListenAndServe)\b | |
| msg: >- | |
| [shortcuts-no-raw-http] use RuntimeContext.DoAPI/CallAPI/DoAPIJSON | |
| instead of constructing raw HTTP. The runtime handles auth, headers, | |
| and error normalization. (Constants and helpers like http.MethodPost, | |
| http.StatusOK, http.StatusText remain allowed.) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.golangci.yml around lines 85 - 90, The forbid-pattern includes a
non-existent package-level symbol `http.Do` which never matches real usage (Do
is a method on *http.Client); update the regex in the rule that currently
contains `http\.Do` to remove that token (i.e., delete `http\.Do` from the
alternation) so the pattern only lists actual package-level functions like
`http\.Client|http\.NewRequest|...` or, if you intended to catch method calls,
replace `http\.Do` with a pattern that matches method calls (e.g., `\.Do\b`
scoped appropriately), but the simplest fix is to remove `http\.Do` from the
alternation.
Comment on lines
+259
to
+260
| for _, want := range []string{"--query", "--user-ids", "--has-chatted", "--exclude-external-users", "--left-organization"} { | ||
| if !strings.Contains(err.Error(), want) { |
There was a problem hiding this comment.
Add --has-enterprise-email to the “hint flags” assertion set.
The validation error string includes this flag, but this test doesn’t assert it, so regressions in that hint can slip through.
Proposed fix
- for _, want := range []string{"--query", "--user-ids", "--has-chatted", "--exclude-external-users", "--left-organization"} {
+ for _, want := range []string{"--query", "--user-ids", "--has-chatted", "--has-enterprise-email", "--exclude-external-users", "--left-organization"} {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| for _, want := range []string{"--query", "--user-ids", "--has-chatted", "--exclude-external-users", "--left-organization"} { | |
| if !strings.Contains(err.Error(), want) { | |
| for _, want := range []string{"--query", "--user-ids", "--has-chatted", "--has-enterprise-email", "--exclude-external-users", "--left-organization"} { | |
| if !strings.Contains(err.Error(), want) { |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@shortcuts/contact/contact_search_user_test.go` around lines 259 - 260, The
test's hint-flags assertion loop is missing the "--has-enterprise-email" entry,
so update the slice of expected flags used in the for loop (the
[]string{"--query", "--user-ids", "--has-chatted", "--exclude-external-users",
"--left-organization"}) to include "--has-enterprise-email" so the test checks
that err.Error() contains that flag in the strings.Contains(err.Error(), want)
assertions.
Comment on lines
+799
to
+811
| func TestSearchUser_Integration_NoAutoPaginationFlags(t *testing.T) { | ||
| for _, removed := range []string{"--page-all", "--page-limit"} { | ||
| t.Run(removed, func(t *testing.T) { | ||
| f, stdout, _, _ := cmdutil.TestFactory(t, searchUserDefaultConfig()) | ||
| args := []string{"+search-user", "--query", "x", removed} | ||
| if removed == "--page-limit" { | ||
| args = append(args, "5") | ||
| } | ||
| args = append(args, "--as", "user") | ||
| err := mountAndRun(t, ContactSearchUser, args, f, stdout) | ||
| if err == nil { | ||
| t.Errorf("%s should be rejected (unknown flag), but command succeeded", removed) | ||
| } |
There was a problem hiding this comment.
Unknown-flag regression test can pass for the wrong reason.
Line 809 currently treats any non-nil error as success. If parsing unexpectedly succeeds and execution later fails (e.g., unstubbed API), this test still passes.
Proposed fix
err := mountAndRun(t, ContactSearchUser, args, f, stdout)
- if err == nil {
- t.Errorf("%s should be rejected (unknown flag), but command succeeded", removed)
- }
+ if err == nil {
+ t.Fatalf("%s should be rejected (unknown flag), but command succeeded", removed)
+ }
+ if !strings.Contains(err.Error(), "unknown flag") || !strings.Contains(err.Error(), removed) {
+ t.Fatalf("expected unknown flag error for %s, got: %v", removed, err)
+ }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@shortcuts/contact/contact_search_user_test.go` around lines 799 - 811, The
test TestSearchUser_Integration_NoAutoPaginationFlags currently treats any
non-nil error as a pass which can hide runtime failures; update the test to
ensure the failure is specifically due to an unknown/invalid flag from parsing.
After calling mountAndRun with ContactSearchUser, assert that err is non-nil and
that err.Error() contains a flag-parsing indicator like "unknown flag" or "flag
provided but not defined"; alternatively invoke the command's flag parsing step
directly (e.g., parse args on the command) before execution so you only assert
parse-time errors from the flag parsing phase.
API rejects queries longer than 50 characters; local cap was 64 runes,
producing confusing "passed local validation but server-rejected"
behaviour. Lower the cap to 50 and rename the constant accordingly.
Also reject --user-ids inputs that parse to zero entries (",,,",
" , , ", ","): SplitCSV silently dropped empty segments, so the
shortcut sent an empty body to the API and returned indeterminate
results.
Change-Id: Ib34fe897023e175bf4c657273bdb49a33d2f083b
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (5)
shortcuts/contact/contact_search_user_test.go (2)
258-260:⚠️ Potential issue | 🟡 MinorKeep the hint assertion aligned with the new flag set.
The production error message includes
--has-enterprise-email, but this loop does not assert it, so that agent-facing hint can regress unnoticed.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@shortcuts/contact/contact_search_user_test.go` around lines 258 - 260, The test's assertion loop over expected flag hints is missing the new flag name "--has-enterprise-email", so add it to the slice in the loop (the []string literal used in the test at contact_search_user_test.go where the loop iterates over want and checks strings.Contains(err.Error(), want)) so the error message assertion includes "--has-enterprise-email" along with the existing flags.
823-826:⚠️ Potential issue | 🟠 MajorThis removed-flag regression test still passes on the wrong error.
Any later execution failure satisfies
err != nil, so the test does not prove Cobra rejected the removed flag at parse time. Assert the error contains both the removed flag and an unknown-flag message.Suggested tightening
err := mountAndRun(t, ContactSearchUser, args, f, stdout) - if err == nil { - t.Errorf("%s should be rejected (unknown flag), but command succeeded", removed) - } + if err == nil { + t.Fatalf("%s should be rejected (unknown flag), but command succeeded", removed) + } + if !strings.Contains(err.Error(), removed) || + (!strings.Contains(err.Error(), "unknown flag") && + !strings.Contains(err.Error(), "flag provided but not defined")) { + t.Fatalf("expected parse-time unknown-flag error for %s, got: %v", removed, err) + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@shortcuts/contact/contact_search_user_test.go` around lines 823 - 826, The test using mountAndRun with ContactSearchUser currently only checks err != nil, which can be satisfied by any later runtime error; change the assertion to verify that the returned error message indicates a parse-time unknown-flag and mentions the specific removed flag (e.g., assert err.Error() contains the removed flag string and an "unknown flag" substring). Update the test around mountAndRun/ContactSearchUser to fail unless both the removed flag name and an unknown-flag parse error are present in the error message.shortcuts/contact/contact_search_user.go (3)
11-11:⚠️ Potential issue | 🔴 Critical
net/httpis still a CI blocker here.This file still imports
net/httpand useshttp.MethodPost/http.StatusOK/http.StatusTextinsideshortcuts/, so the depguard failure from the earlier review is still present. Replace those with literals and drop the import before merge.Also applies to: 183-193
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@shortcuts/contact/contact_search_user.go` at line 11, The file still imports net/http and uses http.MethodPost, http.StatusOK and http.StatusText; remove the net/http import and replace those uses with string/number literals (use "POST" for http.MethodPost, 200 for http.StatusOK and "OK" for http.StatusText(http.StatusOK)) in the functions/methods in contact_search_user.go (look for occurrences of http.MethodPost, http.StatusOK, http.StatusText) so the depguard CI failure is resolved and the import can be dropped.
238-260:⚠️ Potential issue | 🟡 Minor
display_infostill misclassifies recency-only payloads as department.If the API returns
"<h>...</h>\n[Contacted 2 days ago]", Line 248 copies the recency line intodepartment. Scan for the first non-empty non-recency line instead of blindly takinglines[1].🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@shortcuts/contact/contact_search_user.go` around lines 238 - 260, The department assignment in parseDisplayInfo incorrectly uses lines[1] which can be a recency-only line; update parseDisplayInfo to scan lines[1:] and set department to the first non-empty line that does NOT match displayInfoRecencyRE (use strings.TrimSpace) so recency-only payloads like "[Contacted 2 days ago]" are skipped; keep the existing recency extraction loop intact (use displayInfoRecencyRE when checking/skipping and when later extracting recencyHint).
197-199:⚠️ Potential issue | 🟡 MinorUse an internal/API error kind for decode failures.
A
json.Unmarshalfailure on the upstream response is not a user validation error. Returning kind"validation"withExitInternalmisclassifies the failure for structured error consumers.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@shortcuts/contact/contact_search_user.go` around lines 197 - 199, The json.Unmarshal error is being misclassified as a "validation" error; update the ErrWithHint call for the unmarshal failure to use an internal/API error kind (e.g., "api") while keeping ExitInternal, so structured consumers see this as an upstream/decoding problem; change the call that currently uses output.ErrWithHint(output.ExitInternal, "validation", "unmarshal response failed", err.Error()) (the one handling json.Unmarshal(apiResp.RawBody, &resp)) to use a more appropriate kind such as "api" (e.g., output.ErrWithHint(output.ExitInternal, "api", "unmarshal response failed", err.Error())).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@shortcuts/contact/contact_search_user.go`:
- Around line 101-112: searchUserAPIMeta and the searchUser projection drop
tenant_id and i18n_names from the public response; restore them by adding
TenantID string `tenant_id` and keeping I18nNames map[string]string `i18n_names`
on the searchUserAPIMeta struct and ensure the projection logic in the
searchUser conversion (the function/method that maps API meta -> searchUser DTO
around the blocks referenced at 116-129 and 323-344) copies meta.TenantID and
meta.I18nNames into the resulting output fields so downstream code can perform
locale selection and cross-tenant disambiguation.
---
Duplicate comments:
In `@shortcuts/contact/contact_search_user_test.go`:
- Around line 258-260: The test's assertion loop over expected flag hints is
missing the new flag name "--has-enterprise-email", so add it to the slice in
the loop (the []string literal used in the test at contact_search_user_test.go
where the loop iterates over want and checks strings.Contains(err.Error(),
want)) so the error message assertion includes "--has-enterprise-email" along
with the existing flags.
- Around line 823-826: The test using mountAndRun with ContactSearchUser
currently only checks err != nil, which can be satisfied by any later runtime
error; change the assertion to verify that the returned error message indicates
a parse-time unknown-flag and mentions the specific removed flag (e.g., assert
err.Error() contains the removed flag string and an "unknown flag" substring).
Update the test around mountAndRun/ContactSearchUser to fail unless both the
removed flag name and an unknown-flag parse error are present in the error
message.
In `@shortcuts/contact/contact_search_user.go`:
- Line 11: The file still imports net/http and uses http.MethodPost,
http.StatusOK and http.StatusText; remove the net/http import and replace those
uses with string/number literals (use "POST" for http.MethodPost, 200 for
http.StatusOK and "OK" for http.StatusText(http.StatusOK)) in the
functions/methods in contact_search_user.go (look for occurrences of
http.MethodPost, http.StatusOK, http.StatusText) so the depguard CI failure is
resolved and the import can be dropped.
- Around line 238-260: The department assignment in parseDisplayInfo incorrectly
uses lines[1] which can be a recency-only line; update parseDisplayInfo to scan
lines[1:] and set department to the first non-empty line that does NOT match
displayInfoRecencyRE (use strings.TrimSpace) so recency-only payloads like
"[Contacted 2 days ago]" are skipped; keep the existing recency extraction loop
intact (use displayInfoRecencyRE when checking/skipping and when later
extracting recencyHint).
- Around line 197-199: The json.Unmarshal error is being misclassified as a
"validation" error; update the ErrWithHint call for the unmarshal failure to use
an internal/API error kind (e.g., "api") while keeping ExitInternal, so
structured consumers see this as an upstream/decoding problem; change the call
that currently uses output.ErrWithHint(output.ExitInternal, "validation",
"unmarshal response failed", err.Error()) (the one handling
json.Unmarshal(apiResp.RawBody, &resp)) to use a more appropriate kind such as
"api" (e.g., output.ErrWithHint(output.ExitInternal, "api", "unmarshal response
failed", err.Error())).
🪄 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: 6d324e72-97c5-46b9-af32-07d6042459db
📒 Files selected for processing (2)
shortcuts/contact/contact_search_user.goshortcuts/contact/contact_search_user_test.go
Comment on lines
+101
to
+112
| type searchUserAPIMeta struct { | ||
| I18nNames map[string]string `json:"i18n_names"` | ||
| MailAddress string `json:"mail_address"` | ||
| EnterpriseMailAddress string `json:"enterprise_mail_address"` | ||
| IsRegistered bool `json:"is_registered"` | ||
| ChatID string `json:"chat_id"` | ||
| IsCrossTenant bool `json:"is_cross_tenant"` | ||
| // API ships the user's profile signature in `description`; the field name | ||
| // is misleading because it carries the personal signature ("个性签名"), | ||
| // not a generic description. We surface it as `signature` downstream. | ||
| Description string `json:"description"` | ||
| } |
There was a problem hiding this comment.
The new public response shape drops tenant_id and i18n_names.
The PR contract calls out both fields as part of the richer profile payload, but the typed API/meta model and searchUser projection discard them entirely. That prevents downstream agents from doing their own locale selection and from disambiguating cross-tenant users by tenant.
Possible minimal fix
type searchUserAPIMeta struct {
I18nNames map[string]string `json:"i18n_names"`
MailAddress string `json:"mail_address"`
EnterpriseMailAddress string `json:"enterprise_mail_address"`
IsRegistered bool `json:"is_registered"`
ChatID string `json:"chat_id"`
IsCrossTenant bool `json:"is_cross_tenant"`
+ TenantID string `json:"tenant_id"`
// API ships the user's profile signature in `description`; the field name
// is misleading because it carries the personal signature ("个性签名"),
// not a generic description. We surface it as `signature` downstream.
Description string `json:"description"`
}
type searchUser struct {
OpenID string `json:"open_id"`
LocalizedName string `json:"localized_name"`
+ I18nNames map[string]string `json:"i18n_names"`
Email string `json:"email"`
EnterpriseEmail string `json:"enterprise_email"`
IsActivated bool `json:"is_activated"`
IsCrossTenant bool `json:"is_cross_tenant"`
+ TenantID string `json:"tenant_id"`
P2PChatID string `json:"p2p_chat_id"`
HasChatted bool `json:"has_chatted"`
Department string `json:"department"`
Signature string `json:"signature"`
ChatRecencyHint string `json:"chat_recency_hint"`
MatchSegments []string `json:"match_segments"`
}
return searchUser{
OpenID: item.ID,
LocalizedName: pickName(i18n, lang, brand, item.ID),
+ I18nNames: i18n,
Email: meta.MailAddress,
EnterpriseEmail: meta.EnterpriseMailAddress,
IsActivated: meta.IsRegistered,
IsCrossTenant: meta.IsCrossTenant,
+ TenantID: meta.TenantID,
P2PChatID: meta.ChatID,
HasChatted: meta.ChatID != "",
Department: department,
Signature: meta.Description,
ChatRecencyHint: recencyHint,Also applies to: 116-129, 323-344
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@shortcuts/contact/contact_search_user.go` around lines 101 - 112,
searchUserAPIMeta and the searchUser projection drop tenant_id and i18n_names
from the public response; restore them by adding TenantID string `tenant_id` and
keeping I18nNames map[string]string `i18n_names` on the searchUserAPIMeta struct
and ensure the projection logic in the searchUser conversion (the
function/method that maps API meta -> searchUser DTO around the blocks
referenced at 116-129 and 323-344) copies meta.TenantID and meta.I18nNames into
the resulting output fields so downstream code can perform locale selection and
cross-tenant disambiguation.
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
2 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
Upgrade
lark-cli contact +search-userto the newPOST /contact/v3/users/searchAPI. Adds search filters and richer result fields so callers (primarily AI agents) can find people more precisely and tell similar candidates apart at a glance. Existing+search-user --query xxxinvocations continue to work unchanged.Key capabilities
--user-ids(CSV withmeexpansion, max 100),--is-resigned,--has-contact,--exclude-outer-contact,--has-enterprise-email, plus--langfor the locale used in the pickedname.--queryis now optional: pair with at least one filter for "list all my external contacts" style queries; bounded to 64 runes to match the client search box.i18n_namesmap + locale-pickedname), real contact info (email,enterprise_email), relationship signals (chat_id+ derivedhas_chatted), tenant context (is_cross_tenant,tenant_id), user signature (description), and the backend'sdisplay_infohit-highlight line (matched segment + department path + recent-contact hint).mobile,department,avatar,user_idno longer surfaced (the new API does not return them).skills/lark-contact/references/lark-contact-get-user.mdpreviously instructed callers to pass--table(a flag that never existed and would error out under cobra). Now correctly documents--format tableand the full--formatenum (json/pretty/table/ndjson/csv).Test Plan
shortcuts/contact/contact_search_user_test.go:pickNamefallback chain (8 cases), response mapping (3), validation (8), body/params builders (7), integration viamountAndRun+httpmock(6)go test ./...green; no regressions inshortcuts/minutes(the sharedresolveUserIDshelper moved toshortcuts/common.ResolveOpenIDs)golangci-lint run --new-from-rev=origin/mainclean (nonet/httpimport in shortcuts; uses string method literal withruntime.CallAPIper repo convention)pretty/json/table/ndjson/csv), validation paths, pagination round-trip, empty result, locale override, multi-filter composition, special-char inputs, API-limit boundaries (--page-size30,--user-ids100,--query64 rune)+get-user --format tableworks against a real tenant; the previously-documented--tablecorrectly errors outRelated Issues
Summary by CodeRabbit
New Features
Behavior Changes / Bug Fixes
Tests
Documentation
Chores