Skip to content

feat(contact +search-user): add search filters and richer profile fields#648

Merged
liangshuo-1 merged 3 commits intomainfrom
feat/search-user-upgrade
Apr 28, 2026
Merged

feat(contact +search-user): add search filters and richer profile fields#648
liangshuo-1 merged 3 commits intomainfrom
feat/search-user-upgrade

Conversation

@evandance
Copy link
Copy Markdown
Collaborator

@evandance evandance commented Apr 23, 2026

Summary

Upgrade lark-cli contact +search-user to the new POST /contact/v3/users/search API. 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 xxx invocations continue to work unchanged.

# keyword + filter
lark-cli contact +search-user --query 张 --has-contact --exclude-outer-contact --as user

# enrich a known open_id list
lark-cli contact +search-user --user-ids me,ou_xxx --as user

# pull just the fields you care about
lark-cli contact +search-user --query 张 --jq '.data.users[] | {name, has_chatted, chat_id}' --as user

Key capabilities

  • New search filters: --user-ids (CSV with me expansion, max 100), --is-resigned, --has-contact, --exclude-outer-contact, --has-enterprise-email, plus --lang for the locale used in the picked name.
  • --query is 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.
  • Richer result fields: multilingual names (i18n_names map + locale-picked name), real contact info (email, enterprise_email), relationship signals (chat_id + derived has_chatted), tenant context (is_cross_tenant, tenant_id), user signature (description), and the backend's display_info hit-highlight line (matched segment + department path + recent-contact hint).
  • Always-empty legacy columns dropped: mobile, department, avatar, user_id no longer surfaced (the new API does not return them).
  • Drive-by skill-doc fix: skills/lark-contact/references/lark-contact-get-user.md previously instructed callers to pass --table (a flag that never existed and would error out under cobra). Now correctly documents --format table and the full --format enum (json / pretty / table / ndjson / csv).

Test Plan

  • Unit tests in shortcuts/contact/contact_search_user_test.go: pickName fallback chain (8 cases), response mapping (3), validation (8), body/params builders (7), integration via mountAndRun + httpmock (6)
  • go test ./... green; no regressions in shortcuts/minutes (the shared resolveUserIDs helper moved to shortcuts/common.ResolveOpenIDs)
  • golangci-lint run --new-from-rev=origin/main clean (no net/http import in shortcuts; uses string method literal with runtime.CallAPI per repo convention)
  • Manual E2E against a real Lark tenant: all 9 input flags, all 12 output fields, all 5 output formats (pretty / json / table / ndjson / csv), validation paths, pagination round-trip, empty result, locale override, multi-filter composition, special-char inputs, API-limit boundaries (--page-size 30, --user-ids 100, --query 64 rune)
  • Verified the skill-doc fix: +get-user --format table works against a real tenant; the previously-documented --table correctly errors out

Related Issues

  • None

Summary by CodeRabbit

  • New Features

    • Redesigned contact user search: POST-based API, richer opt-in filters, --user-ids (supports "me"), --lang, tightened page-size (1–30), and streamlined table columns.
  • Behavior Changes / Bug Fixes

    • Centralized "me" → logged-in OpenID expansion with case-insensitive dedupe (preserves first-seen casing); error when not logged in. Refine hints go to stderr; removed auto-pagination flags.
  • Tests

    • Large suite of unit/integration tests for search, validation, name selection, ID resolution, parsing, and outputs.
  • Documentation

    • Updated skill and command docs, examples, and usage guidance.
  • Chores

    • Linting rules updated to forbid raw HTTP construction in shortcuts.

@github-actions github-actions Bot added domain/contact PR touches the contact domain size/L Large or sensitive change across domains or core paths labels Apr 23, 2026
@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented Apr 23, 2026

Note

Reviews paused

It 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 reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review
📝 Walkthrough

Walkthrough

Centralizes "me" expansion into common.ResolveOpenIDs; replaces contact search GET with typed POST /open-apis/contact/v3/users/search and tightens validation/output shaping; minutes search now uses the shared resolver; adds legacy contact helpers; updates tests, docs, and linter rules.

Changes

Cohort / File(s) Summary
User ID resolution
shortcuts/common/userids.go, shortcuts/common/userids_test.go
Add ResolveOpenIDs(flagName string, ids []string, runtime *RuntimeContext) to expand case-insensitive me to runtime.UserOpenId(), error if me requested without a resolvable open_id, and case-insensitive dedupe preserving first-seen spelling. Tests cover nil input, expansion, dedupe, case variants, and missing open_id error.
Contact search (behavior + tests)
shortcuts/contact/contact_search_user.go, shortcuts/contact/contact_search_user_test.go
Replace legacy GET with typed POST /open-apis/contact/v3/users/search; move query/filter into JSON body; add --user-ids, opt-in boolean filters, --lang, stricter validations (query rune limit, user-ids count/prefix, page-size 1–30, reject explicit =false booleans), deterministic i18n name selection, parse display_info into highlights/department/recency hint, reshape output schema, change pagination hint routing, and add broad unit/integration tests.
Contact legacy helpers & tests
shortcuts/contact/helpers_legacy.go, shortcuts/contact/helpers_legacy_test.go
Introduce pickUserName and firstNonEmpty to select legacy name keys by priority; tests verify precedence, skipping non-strings, empty-map behavior, and key-order semantics.
Minutes search
shortcuts/minutes/minutes_search.go, shortcuts/minutes/minutes_search_test.go
Refactor minutes search to call common.ResolveOpenIDs for --owner-ids and --participant-ids; remove local resolveUserIDs test.
Docs / Skill content
skills/lark-contact/references/lark-contact-get-user.md, skills/lark-contact/references/lark-contact-search-user.md, skills/lark-contact/SKILL.md
Rewrite docs to reflect new flag contract and behaviors: `--as user
Linter config
.golangci.yml
Replace depguard raw-HTTP deny with forbidigo call-pattern enforcement and scope depguard exclusions to avoid false positives in shortcuts/; update messaging to reference RuntimeContext API helpers.

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
Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Suggested reviewers

  • chanthuang
  • liangshuo-1
  • kongenpei

Poem

🐰
I hopped from "me" to open_id bright,
resolved and deduped each tiny bite,
sent queries by POST and picked names with care,
parsed highlights and flattened rows to share,
a rabbit's little fix — neat CLI delight.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 22.22% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly and concisely summarizes the main changes: adding search filters and richer profile fields to the contact +search-user command.
Description check ✅ Passed The description comprehensively covers summary, key capabilities, changes, test plan with checkboxes, and related issues as required by the template.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch feat/search-user-upgrade

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@codecov
Copy link
Copy Markdown

codecov Bot commented Apr 23, 2026

Codecov Report

❌ Patch coverage is 92.34450% with 16 lines in your changes missing coverage. Please review.
✅ Project coverage is 63.16%. Comparing base (d92f0a2) to head (826a506).
⚠️ Report is 26 commits behind head on main.

Files with missing lines Patch % Lines
shortcuts/contact/contact_search_user.go 91.01% 8 Missing and 8 partials ⚠️
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.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

📥 Commits

Reviewing files that changed from the base of the PR and between d3340f5 and 9a46ade.

📒 Files selected for processing (7)
  • shortcuts/common/userids.go
  • shortcuts/common/userids_test.go
  • shortcuts/contact/contact_search_user.go
  • shortcuts/contact/contact_search_user_test.go
  • shortcuts/contact/helpers_legacy.go
  • shortcuts/minutes/minutes_search.go
  • shortcuts/minutes/minutes_search_test.go
💤 Files with no reviewable changes (1)
  • shortcuts/minutes/minutes_search_test.go

Comment thread shortcuts/contact/contact_search_user.go Outdated
@github-actions
Copy link
Copy Markdown

github-actions Bot commented Apr 23, 2026

🚀 PR Preview Install Guide

🧰 CLI update

npm i -g https://pkg.pr.new/larksuite/cli/@larksuite/cli@826a5069fb4490b67cfb04c000a55b79887994d5

🧩 Skill update

npx skills add larksuite/cli#feat/search-user-upgrade -y -g

@evandance evandance force-pushed the feat/search-user-upgrade branch 3 times, most recently from 214e424 to e5152e6 Compare April 23, 2026 14:21
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

📥 Commits

Reviewing files that changed from the base of the PR and between 9a46ade and 214e424.

📒 Files selected for processing (8)
  • shortcuts/common/userids.go
  • shortcuts/common/userids_test.go
  • shortcuts/contact/contact_search_user.go
  • shortcuts/contact/contact_search_user_test.go
  • shortcuts/contact/helpers_legacy.go
  • shortcuts/minutes/minutes_search.go
  • shortcuts/minutes/minutes_search_test.go
  • skills/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

Comment thread shortcuts/common/userids_test.go
@evandance evandance force-pushed the feat/search-user-upgrade branch from e5152e6 to 2225c3d Compare April 23, 2026 14:27
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

♻️ Duplicate comments (1)
shortcuts/contact/contact_search_user.go (1)

104-106: ⚠️ Potential issue | 🟡 Minor

Send the pagination hint to stderr.

Line 105 still writes the non-result hint to stdout; route it to runtime.IO().ErrOut instead.
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

📥 Commits

Reviewing files that changed from the base of the PR and between 214e424 and e5152e6.

📒 Files selected for processing (8)
  • shortcuts/common/userids.go
  • shortcuts/common/userids_test.go
  • shortcuts/contact/contact_search_user.go
  • shortcuts/contact/contact_search_user_test.go
  • shortcuts/contact/helpers_legacy.go
  • shortcuts/minutes/minutes_search.go
  • shortcuts/minutes/minutes_search_test.go
  • skills/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

Comment thread shortcuts/contact/helpers_legacy.go
@evandance evandance force-pushed the feat/search-user-upgrade branch 2 times, most recently from 1b69afe to 083ed14 Compare April 23, 2026 14:30
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

📥 Commits

Reviewing files that changed from the base of the PR and between e5152e6 and 083ed14.

📒 Files selected for processing (9)
  • shortcuts/common/userids.go
  • shortcuts/common/userids_test.go
  • shortcuts/contact/contact_search_user.go
  • shortcuts/contact/contact_search_user_test.go
  • shortcuts/contact/helpers_legacy.go
  • shortcuts/contact/helpers_legacy_test.go
  • shortcuts/minutes/minutes_search.go
  • shortcuts/minutes/minutes_search_test.go
  • skills/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

Comment thread shortcuts/common/userids.go Outdated
@evandance evandance force-pushed the feat/search-user-upgrade branch from 083ed14 to e807c6d Compare April 23, 2026 15:06
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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: true and supports pretty/table/ndjson/csv as 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 resolved user-ids to avoid repeating SplitCSV + ResolveOpenIDs.

The same CSV parse and ResolveOpenIDs pipeline runs again in buildSearchUserBody (lines 290–298). Since Validate runs before Execute, you could compute the resolved list once (e.g., store on a small request struct, or a helper resolvedUserIDs(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

📥 Commits

Reviewing files that changed from the base of the PR and between 083ed14 and e807c6d.

📒 Files selected for processing (11)
  • shortcuts/common/userids.go
  • shortcuts/common/userids_test.go
  • shortcuts/contact/contact_search_user.go
  • shortcuts/contact/contact_search_user_test.go
  • shortcuts/contact/helpers_legacy.go
  • shortcuts/contact/helpers_legacy_test.go
  • shortcuts/minutes/minutes_search.go
  • shortcuts/minutes/minutes_search_test.go
  • skills/lark-contact/SKILL.md
  • skills/lark-contact/references/lark-contact-get-user.md
  • skills/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

Comment thread skills/lark-contact/references/lark-contact-search-user.md Outdated
@evandance evandance force-pushed the feat/search-user-upgrade branch 4 times, most recently from 19a67c0 to 4adc9bc Compare April 24, 2026 08:10
@evandance
Copy link
Copy Markdown
Collaborator Author

@coderabbitai resume

@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented Apr 24, 2026

✅ Actions performed

Reviews resumed.

Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

🧹 Nitpick comments (3)
shortcuts/contact/contact_search_user.go (3)

229-252: primary may contain a duplicate locale.

When --lang happens to equal one of the brand-preferred locales (e.g. --lang en_us on a Lark brand, or --lang zh_cn on Feishu), the resulting primary slice 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 ordering switch so 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: guard runtime.Cmd before .Flags().Changed(...).

hasAnySearchInput (line 347) and buildSearchUserBody (line 377) dereference runtime.Cmd.Flags() unconditionally, whereas searchUserPaginationConfig (lines 177, 181) and validateSearchUser (line 327) in the same file defensively check runtime.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.

validateSearchUser rejects --page-limit outside [1, 40] at lines 328–330, so by the time searchUserPaginationConfig runs, runtime.Int("page-limit") is guaranteed to be ≤ maxSearchUserPageLimit; the if pageLimit > maxSearchUserPageLimit branch 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

📥 Commits

Reviewing files that changed from the base of the PR and between 083ed14 and 4adc9bc.

📒 Files selected for processing (11)
  • shortcuts/common/userids.go
  • shortcuts/common/userids_test.go
  • shortcuts/contact/contact_search_user.go
  • shortcuts/contact/contact_search_user_test.go
  • shortcuts/contact/helpers_legacy.go
  • shortcuts/contact/helpers_legacy_test.go
  • shortcuts/minutes/minutes_search.go
  • shortcuts/minutes/minutes_search_test.go
  • skills/lark-contact/SKILL.md
  • skills/lark-contact/references/lark-contact-get-user.md
  • skills/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

Comment thread shortcuts/contact/contact_search_user.go Outdated
Comment thread skills/lark-contact/references/lark-contact-search-user.md Outdated
- 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 evandance force-pushed the feat/search-user-upgrade branch from 4adc9bc to 1667856 Compare April 24, 2026 08:32
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick comments (1)
shortcuts/contact/contact_search_user.go (1)

233-242: Optional: dedup primary to avoid redundant lookups when --lang matches a brand default.

If a user passes --lang zh-CN under BrandFeishu (or --lang en-US under BrandLark), primary becomes ["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

📥 Commits

Reviewing files that changed from the base of the PR and between 4adc9bc and 1667856.

📒 Files selected for processing (11)
  • shortcuts/common/userids.go
  • shortcuts/common/userids_test.go
  • shortcuts/contact/contact_search_user.go
  • shortcuts/contact/contact_search_user_test.go
  • shortcuts/contact/helpers_legacy.go
  • shortcuts/contact/helpers_legacy_test.go
  • shortcuts/minutes/minutes_search.go
  • shortcuts/minutes/minutes_search_test.go
  • skills/lark-contact/SKILL.md
  • skills/lark-contact/references/lark-contact-get-user.md
  • skills/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 liangshuo-1 force-pushed the feat/search-user-upgrade branch from 8873644 to 9a65cc8 Compare April 27, 2026 17:33
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

📥 Commits

Reviewing files that changed from the base of the PR and between 1667856 and 8873644.

📒 Files selected for processing (5)
  • shortcuts/contact/contact_search_user.go
  • shortcuts/contact/contact_search_user_test.go
  • skills/lark-contact/SKILL.md
  • skills/lark-contact/references/lark-contact-get-user.md
  • skills/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 thread shortcuts/contact/contact_search_user.go
Comment thread shortcuts/contact/contact_search_user.go
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

♻️ Duplicate comments (1)
shortcuts/contact/contact_search_user.go (1)

197-199: ⚠️ Potential issue | 🟡 Minor

Misleading 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

📥 Commits

Reviewing files that changed from the base of the PR and between 8873644 and 9a65cc8.

📒 Files selected for processing (5)
  • shortcuts/contact/contact_search_user.go
  • shortcuts/contact/contact_search_user_test.go
  • skills/lark-contact/SKILL.md
  • skills/lark-contact/references/lark-contact-get-user.md
  • skills/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

Comment thread shortcuts/contact/contact_search_user.go
- 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 liangshuo-1 force-pushed the feat/search-user-upgrade branch from 9a65cc8 to 60ba249 Compare April 27, 2026 17:44
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

📥 Commits

Reviewing files that changed from the base of the PR and between 9a65cc8 and 60ba249.

📒 Files selected for processing (6)
  • .golangci.yml
  • shortcuts/contact/contact_search_user.go
  • shortcuts/contact/contact_search_user_test.go
  • skills/lark-contact/SKILL.md
  • skills/lark-contact/references/lark-contact-get-user.md
  • skills/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 thread .golangci.yml
Comment thread shortcuts/contact/contact_search_user_test.go
Comment thread shortcuts/contact/contact_search_user_test.go
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
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

♻️ Duplicate comments (5)
shortcuts/contact/contact_search_user_test.go (2)

258-260: ⚠️ Potential issue | 🟡 Minor

Keep 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 | 🟠 Major

This 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/http is still a CI blocker here.

This file still imports net/http and uses http.MethodPost / http.StatusOK / http.StatusText inside shortcuts/, 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_info still misclassifies recency-only payloads as department.

If the API returns "<h>...</h>\n[Contacted 2 days ago]", Line 248 copies the recency line into department. Scan for the first non-empty non-recency line instead of blindly taking lines[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 | 🟡 Minor

Use an internal/API error kind for decode failures.

A json.Unmarshal failure on the upstream response is not a user validation error. Returning kind "validation" with ExitInternal misclassifies 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

📥 Commits

Reviewing files that changed from the base of the PR and between 60ba249 and 826a506.

📒 Files selected for processing (2)
  • shortcuts/contact/contact_search_user.go
  • shortcuts/contact/contact_search_user_test.go

Comment thread shortcuts/contact/contact_search_user.go
@liangshuo-1 liangshuo-1 merged commit fc9f9c1 into main Apr 28, 2026
22 checks passed
@liangshuo-1 liangshuo-1 deleted the feat/search-user-upgrade branch April 28, 2026 12:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

domain/contact PR touches the contact domain size/L Large or sensitive change across domains or core paths

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants