feat: support user access token for im file and image upload#474
Conversation
📝 WalkthroughWalkthroughThe pull request updates Feishu IM media upload requests to use runtime identity instead of bot identity, with corresponding documentation updates clarifying that upload and send operations now use consistent identity throughout the shortcut execution. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 |
14155c0 to
30ea80e
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
shortcuts/im/helpers.go (1)
1134-1138: Please add explicit--as userupload-path tests.Given this behavior change, it would be good to add coverage that asserts image/file upload requests use user identity in user mode (not bot-forced), alongside existing bot-path tests.
Also applies to: 1175-1179, 1203-1207, 1235-1239
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@shortcuts/im/helpers.go` around lines 1134 - 1138, Add explicit tests asserting that upload requests use the user identity when running in "user" mode: for each runtime.DoAPI call that uploads files (the ApiReq with ApiPath "/open-apis/im/v1/images" and other similar calls at the other locations using larkcore.WithFileUpload()), create a test that invokes the command with "--as user" and asserts the outgoing request path/headers reflect user identity (not bot-forced). Locate the calls to runtime.DoAPI/ApiReq and WithFileUpload() in shortcuts/im/helpers.go (lines around the shown diffs and the other groups at ~1175-1179, ~1203-1207, ~1235-1239), add corresponding upload-path tests (naming them e.g. TestUpload_AsUser_UsesUserPath) that mirror existing bot-path tests but pass "--as user" and verify the request target/identity accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@shortcuts/im/helpers.go`:
- Around line 1134-1138: Add explicit tests asserting that upload requests use
the user identity when running in "user" mode: for each runtime.DoAPI call that
uploads files (the ApiReq with ApiPath "/open-apis/im/v1/images" and other
similar calls at the other locations using larkcore.WithFileUpload()), create a
test that invokes the command with "--as user" and asserts the outgoing request
path/headers reflect user identity (not bot-forced). Locate the calls to
runtime.DoAPI/ApiReq and WithFileUpload() in shortcuts/im/helpers.go (lines
around the shown diffs and the other groups at ~1175-1179, ~1203-1207,
~1235-1239), add corresponding upload-path tests (naming them e.g.
TestUpload_AsUser_UsesUserPath) that mirror existing bot-path tests but pass
"--as user" and verify the request target/identity accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 9d375c91-efe6-43f4-a8fa-b8ba0a80cd39
📒 Files selected for processing (4)
shortcuts/common/runner.goshortcuts/im/helpers.goskills/lark-im/references/lark-im-messages-reply.mdskills/lark-im/references/lark-im-messages-send.md
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
cmd/root.go (1)
328-331: Use centralized endpoint resolution here to prevent host drift.This local brand/host branch duplicates endpoint selection logic. Consider deriving the host from the shared endpoint resolver so future domain transitions only need one change point.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/root.go` around lines 328 - 331, The host selection currently duplicates logic via the local branch using host and cfg.Brand; replace this with the centralized endpoint resolver: remove the host := ... / if cfg.Brand == "lark" ... block and obtain the host by calling the shared resolver function (e.g., endpointResolver.Resolve(cfg.Brand) or GetHostForBrand(cfg.Brand)) so host is derived from the single source of truth; ensure any error or fallback handling used by the resolver is preserved when assigning host.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cmd/root.go`:
- Line 328: Update the test expectations that still assert the old domain
"open.feishu.cn" to match the new default host "open.feishu-pre.cn" introduced
by changing host in cmd/root.go; specifically update assertions in the tests
referenced (e.g., those in root_integration_test.go and types_test.go) to expect
"open.feishu-pre.cn" wherever they compare against the console host or default
endpoint string (search for "open.feishu.cn" in tests and replace with
"open.feishu-pre.cn").
---
Nitpick comments:
In `@cmd/root.go`:
- Around line 328-331: The host selection currently duplicates logic via the
local branch using host and cfg.Brand; replace this with the centralized
endpoint resolver: remove the host := ... / if cfg.Brand == "lark" ... block and
obtain the host by calling the shared resolver function (e.g.,
endpointResolver.Resolve(cfg.Brand) or GetHostForBrand(cfg.Brand)) so host is
derived from the single source of truth; ensure any error or fallback handling
used by the resolver is preserved when assigning host.
🪄 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: 9d9bd82f-1388-4a7b-a9f8-ed4b14ba2498
📒 Files selected for processing (6)
cmd/root.gointernal/core/types.goshortcuts/common/runner.goshortcuts/im/helpers.goskills/lark-im/references/lark-im-messages-reply.mdskills/lark-im/references/lark-im-messages-send.md
✅ Files skipped from review due to trivial changes (3)
- skills/lark-im/references/lark-im-messages-send.md
- internal/core/types.go
- shortcuts/common/runner.go
🚧 Files skipped from review as they are similar to previous changes (2)
- skills/lark-im/references/lark-im-messages-reply.md
- shortcuts/im/helpers.go
30ea80e to
7a62872
Compare
… send the resource message Change-Id: I3d7fd528dd30fef9aea2d88100ceb03db4c7c3ac
7a62872 to
e8c3637
Compare
🚀 PR Preview Install Guide🧰 CLI updatenpm i -g https://pkg.pr.new/larksuite/cli/@larksuite/cli@e8c36373d51fc57d1c6081a6ed443f702cce31eb🧩 Skill updatenpx skills add chenxingtong-bytedance/cli#feat/support_uat_upload_file -y -g |
Summary
The
/open-apis/im/v1/imagesand/open-apis/im/v1/filesAPIs now support User Access Token (UAT) in addition to Tenant Access Token (TAT). Previously the upload helpers forced bot identity unconditionally; this PR aligns them with the surrounding shortcut's--asflag so uploads and sends share the same identity.Changes
shortcuts/im/helpers.go: replaceDoAPIAsBotwithDoAPIin all four upload helpers (uploadImageToIM,uploadFileToIM,uploadImageFromReader,uploadFileFromReader) so they respect the runtime identity instead of always using TATshortcuts/common/runner.go: remove the stale "e.g. image/file upload" example from theDoAPIAsBotdoc commentskills/lark-im/references/lark-im-messages-send.md: update note to reflect that upload and send now use the same identityskills/lark-im/references/lark-im-messages-reply.md: same doc fix as aboveTest Plan
make unit-testpassed--as user:lark-cli im +messages-send --user-id ou_xxx --image ./photo.png --as userlark-cli im +messages-send --user-id ou_xxx --file ./doc.pdf --as user--as botbehavior unchanged (still uses TAT)Related Issues
Closes #
Summary by CodeRabbit
Bug Fixes
Documentation