feat(im): add recovery hint for cross-identity message resources#652
Open
chenxingtong-bytedance wants to merge 1 commit intolarksuite:mainfrom
Open
feat(im): add recovery hint for cross-identity message resources#652chenxingtong-bytedance wants to merge 1 commit intolarksuite:mainfrom
chenxingtong-bytedance wants to merge 1 commit intolarksuite:mainfrom
Conversation
📝 WalkthroughWalkthroughDetects Lark error code 231205 (ownership mismatch), classifies it as Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested labels
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 unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
github-actions
Bot
added
the
size/L
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
internal/output/ownership_recovery.go (1)
18-22: Optional: promote the hint to a package-level const.
buildOwnershipRecoveryHint()takes no inputs and always returns the same literal. Aconst(orvar) avoids the per-call string concatenation and makes the content easier to diff/locate. Purely stylistic — no behavior change.♻️ Proposed refactor
-func buildOwnershipRecoveryHint() string { - return "Step 1: ..." + - "Step 2: ..." + - "Step 3: ..." -} +const ownershipRecoveryHint = "Step 1: ...\n" + + "Step 2: ...\n" + + "Step 3: ..." + +func buildOwnershipRecoveryHint() string { return ownershipRecoveryHint }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/output/ownership_recovery.go` around lines 18 - 22, buildOwnershipRecoveryHint always returns the same long literal so replace it with a package-level constant (e.g., const OwnershipRecoveryHint = "...") containing the full multi-line hint and remove or update the buildOwnershipRecoveryHint() function to return that const (or delete the function and update all callers to use OwnershipRecoveryHint). Ensure the constant preserves the exact string content/escaping and update any imports/uses referencing buildOwnershipRecoveryHint to reference OwnershipRecoveryHint to avoid per-call concatenation overhead and make the text easier to locate.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/core/types.go`:
- Around line 43-45: The default Feishu endpoints were accidentally changed to
the staging host (*.feishu-pre.cn); restore the public defaults by changing the
Open, Accounts, and MCP values back to "https://open.feishu.cn",
"https://accounts.feishu.cn", and "https://mcp.feishu.cn" in the BrandFeishu
defaults (internal/core/types.go) and mirror the same correction for the
permission-error console_url usage in cmd/root.go so unrecognized brands/users
use the production Feishu hosts.
In `@internal/output/ownership_recovery.go`:
- Line 8: The prefix ownershipRecoveryMessagePrefix is worded "message send
failed…" but LarkErrOwnershipMismatch can occur for both im+messages-send and
im+messages-reply; update the string to a neutral phrasing (e.g., "message
operation failed because one or more referenced resources belong to another
user") so it accurately applies to send and reply paths; change the constant
value in ownership_recovery.go (ownershipRecoveryMessagePrefix) and keep
references to LarkErrOwnershipMismatch intact.
---
Nitpick comments:
In `@internal/output/ownership_recovery.go`:
- Around line 18-22: buildOwnershipRecoveryHint always returns the same long
literal so replace it with a package-level constant (e.g., const
OwnershipRecoveryHint = "...") containing the full multi-line hint and remove or
update the buildOwnershipRecoveryHint() function to return that const (or delete
the function and update all callers to use OwnershipRecoveryHint). Ensure the
constant preserves the exact string content/escaping and update any imports/uses
referencing buildOwnershipRecoveryHint to reference OwnershipRecoveryHint to
avoid per-call concatenation overhead and make the text easier to locate.
🪄 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: 229a22d3-9859-44a4-bf4e-64bf21a3fadf
📒 Files selected for processing (7)
cmd/root.gointernal/core/types.gointernal/output/errors.gointernal/output/lark_errors.gointernal/output/lark_errors_test.gointernal/output/ownership_recovery.gointernal/output/ownership_recovery_test.go
chenxingtong-bytedance
force-pushed
the
fix/recognize_send_err
branch
from
df0cf20 to
02b293e
Compare
github-actions
Bot
added
size/M
There was a problem hiding this comment.
🧹 Nitpick comments (1)
internal/output/errors.go (1)
97-102: Optional: key the rewrite offerrTypefor consistency.The preceding permission branch uses
errType == "permission", while the new branch checkslarkCode == LarkErrOwnershipMismatch. Both work today, but keying both onerrTypekeeps the classification authoritative and avoids drift if another code is ever mapped to theownership_mismatchtype inClassifyLarkError.♻️ Proposed refactor
- if larkCode == LarkErrOwnershipMismatch { + if errType == "ownership_mismatch" { msg = buildOwnershipRecoveryMessage(msg) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/output/errors.go` around lines 97 - 102, The ownership-recovery branch currently checks larkCode == LarkErrOwnershipMismatch which diverges from the earlier errType-based check; change that conditional to check the classified errType (e.g., errType == "ownership_mismatch") so both branches use the authoritative classification from ClassifyLarkError, leaving the call to buildOwnershipRecoveryMessage(msg) and the existing msg assignment 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 `@internal/output/errors.go`:
- Around line 97-102: The ownership-recovery branch currently checks larkCode ==
LarkErrOwnershipMismatch which diverges from the earlier errType-based check;
change that conditional to check the classified errType (e.g., errType ==
"ownership_mismatch") so both branches use the authoritative classification from
ClassifyLarkError, leaving the call to buildOwnershipRecoveryMessage(msg) and
the existing msg assignment unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 40b3a91b-81a7-40c7-aefc-2658d09baf3a
📒 Files selected for processing (5)
internal/output/errors.gointernal/output/lark_errors.gointernal/output/lark_errors_test.gointernal/output/ownership_recovery.gointernal/output/ownership_recovery_test.go
✅ Files skipped from review due to trivial changes (1)
- internal/output/lark_errors_test.go
🚧 Files skipped from review as they are similar to previous changes (2)
- internal/output/ownership_recovery.go
- internal/output/ownership_recovery_test.go
chenxingtong-bytedance
force-pushed
the
fix/recognize_send_err
branch
from
02b293e to
dd0c2c3
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
internal/output/errors.go (1)
97-102: Optional: dispatch ownership-mismatch offerrTypefor consistency.The permission branch above keys off
errType == "permission", while the new branch keys off the rawlarkCode. Either style works, but switching toerrType == "ownership_mismatch"would keep the message-rewrite dispatch uniform and makeErrAPIindependent of which specific Lark codes fall under that classification. Non-blocking.♻️ Suggested tweak
if errType == "permission" { msg = fmt.Sprintf("Permission denied [%d]", larkCode) } - if larkCode == LarkErrOwnershipMismatch { + if errType == "ownership_mismatch" { msg = buildOwnershipRecoveryMessage(msg) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/output/errors.go` around lines 97 - 102, Change the ownership-mismatch check to use the error classification string instead of the numeric Lark code so message dispatch is consistent: replace the larkCode == LarkErrOwnershipMismatch branch with a check for errType == "ownership_mismatch" and then call buildOwnershipRecoveryMessage(msg) there (keep the existing permission check on errType == "permission"); this makes ErrAPI rely on errType rather than specific Lark codes while preserving behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@internal/output/errors.go`:
- Around line 97-102: Change the ownership-mismatch check to use the error
classification string instead of the numeric Lark code so message dispatch is
consistent: replace the larkCode == LarkErrOwnershipMismatch branch with a check
for errType == "ownership_mismatch" and then call
buildOwnershipRecoveryMessage(msg) there (keep the existing permission check on
errType == "permission"); this makes ErrAPI rely on errType rather than specific
Lark codes while preserving behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 6653c6df-fd40-485e-9987-88ceb6b99718
📒 Files selected for processing (5)
internal/output/errors.gointernal/output/lark_errors.gointernal/output/lark_errors_test.gointernal/output/ownership_recovery.gointernal/output/ownership_recovery_test.go
✅ Files skipped from review due to trivial changes (2)
- internal/output/lark_errors_test.go
- internal/output/ownership_recovery.go
🚧 Files skipped from review as they are similar to previous changes (1)
- internal/output/ownership_recovery_test.go
Change-Id: I8a43486333638271f0fbbcffca81a60c9f9d2060
chenxingtong-bytedance
force-pushed
the
fix/recognize_send_err
branch
from
dd0c2c3 to
564dd05
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
internal/output/ownership_recovery.go (1)
18-22: Optional: extract the hint steps into named constants or a slice joined with\nfor readability.The function body is a single concatenated string literal where each "Step N" line is many sentences long. A
[]string{...}joined with"\n"(or oneconst stepN = ...per step) would make future edits and diffs much easier to read without changing behavior.♻️ Example refactor
func buildOwnershipRecoveryHint() string { - return "Step 1: download each original resource only with ..." + - "Step 2: if the original command used direct media flags ..." + - "Step 3: if the original command used structured JSON ..." + steps := []string{ + "Step 1: download each original resource only with `lark-cli im +messages-resources-download --message-id <message_id> --file-key <resource_key> --type <image|file> --output <local_path>`; use `--type image` for images and `--type file` for file/audio/video resources. `message_id` is mandatory for this download. Do not guess it, do not switch to any other download command or raw API, and do not keep retrying alternative download methods. If `message_id` is unavailable, stop and ask the user for it, or recover it from the original command context before continuing.", + "Step 2: if the original command used direct media flags (`--image`, `--file`, `--audio`, `--video`, `--video-cover`), retry with the downloaded local path instead of the old resource key; `im +messages-send` and `im +messages-reply` will upload the local file automatically under the current identity.", + "Step 3: if the original command used structured JSON (`--msg-type post`, `--msg-type interactive`, or `--content`), the downloaded output path cannot be embedded directly in JSON. Upload each resource first under the current identity (image -> `POST /open-apis/im/v1/images`, file/audio/video -> `POST /open-apis/im/v1/files`), replace the old `image_key` / `file_key` in JSON with the new keys, then retry the original send/reply. Repeat this for every referenced resource.", + } + return strings.Join(steps, "\n") }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/output/ownership_recovery.go` around lines 18 - 22, The buildOwnershipRecoveryHint function currently returns one long concatenated string; extract each "Step N" into either separate constants (e.g., step1, step2, step3) or into a []string{...} named steps and then return strings.Join(steps, "\n") so the content is easier to read and diff; ensure the final returned string remains identical and update buildOwnershipRecoveryHint to reference the new constants/slice.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@internal/output/ownership_recovery.go`:
- Around line 18-22: The buildOwnershipRecoveryHint function currently returns
one long concatenated string; extract each "Step N" into either separate
constants (e.g., step1, step2, step3) or into a []string{...} named steps and
then return strings.Join(steps, "\n") so the content is easier to read and diff;
ensure the final returned string remains identical and update
buildOwnershipRecoveryHint to reference the new constants/slice.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 314bdee4-d3cf-4d01-9a40-4990847c53fa
📒 Files selected for processing (5)
internal/output/errors.gointernal/output/lark_errors.gointernal/output/lark_errors_test.gointernal/output/ownership_recovery.gointernal/output/ownership_recovery_test.go
✅ Files skipped from review due to trivial changes (1)
- internal/output/lark_errors_test.go
🚧 Files skipped from review as they are similar to previous changes (3)
- internal/output/lark_errors.go
- internal/output/errors.go
- internal/output/ownership_recovery_test.go
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Change-Id: I8a43486333638271f0fbbcffca81a60c9f9d2060
Summary
This change improves recovery guidance for IM send/reply failures when referenced message resources belong to a different identity. It classifies the
ownership-mismatch API error explicitly and returns structured, actionable hints so users and AI agents can recover by downloading the original resource and
re-uploading it under the current identity.
Changes
231205), including a dedicated error type and clearer user-facing message formatting.Test Plan
go test ./internal/output/...lark imcommand works as expectedRelated Issues
Summary by CodeRabbit
New Features
Tests