feat: add IM message update shortcut#1195
Open
David-Buxy wants to merge 1 commit into
Open
Conversation
|
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (12)
✅ Files skipped from review due to trivial changes (4)
🚧 Files skipped from review as they are similar to previous changes (8)
📝 WalkthroughWalkthroughAdds two IM shortcuts: ChangesIM Message Update Feature
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested labels
Possibly related PRs
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
domain/im
David-Buxy
force-pushed
the
codex/im-message-update-shortcut
branch
from
0151cdb to
975c535
Compare
github-actions
Bot
added
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
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@shortcuts/im/im_messages_update.go`:
- Around line 38-40: The current call to normalizeAtMentions runs after the
content has been JSON-serialized for post payloads and can corrupt the inner
JSON; change the flow so that for markdown/post paths you run
normalizeAtMentions on the raw markdown before calling
wrapMarkdownAsPostForDryRun/resolveMarkdownAsPost (and avoid running
normalizeAtMentions on already-serialized content strings), and for --content
either skip the post-serialization normalize or decode, normalize fields, and
re-marshal; update the logic in the code paths that call normalizeAtMentions
(refer to normalizeAtMentions, wrapMarkdownAsPostForDryRun,
resolveMarkdownAsPost, and marshalUpdateTextContent) and apply the same fix in
the analogous locations in im_messages_send.go and im_messages_reply.go.
In `@tests/cli_e2e/im/message_update_workflow_test.go`:
- Around line 27-42: The test registers an empty cleanup for IM chats
(createChat/createChatAs and sendMessage/sendMessageAs) so it violates the
create→use→cleanup convention; update the test to either call the real IM chat
delete flow in the registered cleanup (replace the no-op parentT.Cleanup with a
cleanup that runs the appropriate CLI command or helper to delete the chat) or
explicitly remove/adjust the cleanup registration and add a short comment
documenting that IM chat deletion is not supported yet; reference
createChat/createChatAs, sendMessage/sendMessageAs and the im +messages-update
usage in message_update_workflow_test.go when making the change.
🪄 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: 6f0ff0aa-a0e4-4679-b2ba-14d8abcaec66
📒 Files selected for processing (9)
shortcuts/im/builders_test.goshortcuts/im/helpers_test.goshortcuts/im/im_messages_update.goshortcuts/im/im_messages_update_test.goshortcuts/im/shortcuts.goskills/lark-im/SKILL.mdskills/lark-im/references/lark-im-messages-update.mdtests/cli_e2e/im/message_update_dryrun_test.gotests/cli_e2e/im/message_update_workflow_test.go
Comment on lines
+27
to
+42
| chatID := createChat(t, parentT, ctx, chatName) | ||
| messageID := sendMessage(t, ctx, chatID, originalText) | ||
|
|
||
| result, err := clie2e.RunCmd(ctx, clie2e.Request{ | ||
| Args: []string{"im", "+messages-update", | ||
| "--message-id", messageID, | ||
| "--text", updatedText, | ||
| }, | ||
| DefaultAs: "bot", | ||
| }) | ||
| require.NoError(t, err) | ||
| result.AssertExitCode(t, 0) | ||
| result.AssertStdoutStatus(t, true) | ||
| require.Equal(t, messageID, gjson.Get(result.Stdout, "data.message_id").String(), "stdout:\n%s", result.Stdout) | ||
| require.Equal(t, chatID, gjson.Get(result.Stdout, "data.chat_id").String(), "stdout:\n%s", result.Stdout) | ||
| require.True(t, gjson.Get(result.Stdout, "data.updated").Bool(), "stdout:\n%s", result.Stdout) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Inspect the shared helpers used by the workflow test.
ast-grep --pattern 'func createChat($$$) $_ {
$$$
}'
ast-grep --pattern 'func sendMessage($$$) $_ {
$$$
}'
# Look for any cleanup registration tied to chat creation.
rg -nP -C3 '\b(t\.Cleanup|DeleteChat|chat-disband|\+chat-)' --type=go -g 'tests/cli_e2e/im/**'Repository: larksuite/cli
Length of output: 7679
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Show helper implementations to confirm identity and teardown/cleanup.
ls -1 tests/cli_e2e/im
# Print createChatAs/sendMessageAs and any cleanup logic from helpers_test.go (and related files if present).
for f in tests/cli_e2e/im/helpers_test.go tests/cli_e2e/im/helpers.go; do
if [ -f "$f" ]; then
echo "==== $f ===="
wc -l "$f"
sed -n '1,220p' "$f" | cat -n
fi
done
# Locate createChatAs definition anywhere under tests/cli_e2e/im.
rg -n "func createChatAs\(" tests/cli_e2e/im --hidden
rg -n "func sendMessageAs\(" tests/cli_e2e/im --hidden
# Look for chat cleanup patterns more broadly in helpers/IM e2e code.
rg -n "Cleanup\\(|DeleteChat|chat[-_ ]disband|disband|leave|Delete\\w*Chat|+chat" tests/cli_e2e/im --hiddenRepository: larksuite/cli
Length of output: 3503
Update message-update E2E comment: bot identity is consistent, but chat cleanup is not
- The seed message is sent as the same bot identity:
createChatusescreateChatAs(..., "bot")andsendMessageusessendMessageAs(..., "bot"), matching the+messages-updateDefaultAs: "bot"behavior. createChatAsregistersparentT.Cleanup(...)but the cleanup function is intentionally empty (“No IM chat delete command is currently available”), so this live test doesn’t follow the create→use→cleanup convention for IM chats. Either implement real cleanup (when available) or adjust the convention/documentation for IM chat lifecycle.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/cli_e2e/im/message_update_workflow_test.go` around lines 27 - 42, The
test registers an empty cleanup for IM chats (createChat/createChatAs and
sendMessage/sendMessageAs) so it violates the create→use→cleanup convention;
update the test to either call the real IM chat delete flow in the registered
cleanup (replace the no-op parentT.Cleanup with a cleanup that runs the
appropriate CLI command or helper to delete the chat) or explicitly
remove/adjust the cleanup registration and add a short comment documenting that
IM chat deletion is not supported yet; reference createChat/createChatAs,
sendMessage/sendMessageAs and the im +messages-update usage in
message_update_workflow_test.go when making the change.
Author
There was a problem hiding this comment.
Thanks for checking this. The IM E2E helpers already document that chat deletion is not available through lark-cli, so the cleanup registration is intentionally a no-op for all IM chat workflow tests. I am leaving that behavior unchanged here because adding real cleanup would require a separate CLI capability, outside the message update shortcut scope.
There was a problem hiding this comment.
@David-Buxy, understood — that's a reasonable scoping decision. Since the no-op is already documented in the helpers and all IM chat workflow tests follow the same convention, no change is needed here.
Would you like me to open a follow-up GitHub issue to track implementing real IM chat cleanup in E2E tests once a delete/disband chat CLI capability is available? That way it won't get lost.
🧠 Learnings used
Learnt from: caojie0621
Repo: larksuite/cli PR: 343
File: shortcuts/sheets/sheet_write_image_test.go:261-263
Timestamp: 2026-04-08T13:54:11.437Z
Learning: In larksuite/cli Go tests (files matching *_test.go), when creating or populating test fixtures under t.TempDir(), use the standard library filesystem APIs: os.Mkdir, os.Create, os.CreateTemp, and os.WriteFile. Treat the vfs.* abstraction as production-only infrastructure: do not use vfs.* for test fixture setup inside *_test.go files, since it would make the tests circular (vfs is intended to be mockable/mocked by tests rather than used to create the fixtures themselves).
Learnt from: JayYoung2021
Repo: larksuite/cli PR: 410
File: cmd/auth/login_scope_cache.go:47-54
Timestamp: 2026-04-11T23:06:23.250Z
Learning: In larksuite/cli Go code, use `validate.SafeInputPath` only for user-supplied file path inputs (e.g., `--file` flags). Do NOT require `validate.SafeInputPath` for internally constructed, application-managed filesystem paths when: (1) their root directory is already validated via `validate.SafeEnvDirPath()` inside helpers like `appdir.CacheDir()`, `appdir.StateDir()`, or `appdir.LogDir()`, and (2) the filename/path components are sanitized by helpers such as `sanitizeLoginScopeCacheKey()`. In code reviews, missing `SafeInputPath` calls should not be flagged for these app-managed paths if the above safeguards are already in place.
Learnt from: JayYoung2021
Repo: larksuite/cli PR: 410
File: internal/update/update.go:147-147
Timestamp: 2026-04-11T23:06:28.717Z
Learning: In this repo’s Go code (larksuite/cli), treat `validate.SafeInputPath` as a guard for **user-supplied local file paths only**. When you build a path by joining a previously validated base directory (e.g., from `appdir.StateDir()`, `appdir.CacheDir()`, `appdir.ConfigDir()` which already validate env overrides via `validate.SafeEnvDirPath()`) with a compile-time constant filename (e.g., `"update-state.json"`, `"config.json"`), do **not** require an additional `validate.SafeInputPath` call before using `vfs.*` or `validate.AtomicWrite`. Reviewers should avoid flagging/adding redundant `validate.SafeInputPath` in these computed-path cases; only require it when the path ultimately comes from user input.
Learnt from: ViperCai
Repo: larksuite/cli PR: 450
File: shortcuts/slides/slides_create.go:50-60
Timestamp: 2026-04-14T03:13:09.459Z
Learning: When reviewing this Go codebase, do not require explicit `validate.SafeInputPath` / `validate.SafeOutputPath` calls for user-supplied paths if the code uses the `runtime.FileIO()` abstraction methods (`Stat`, `Open`, `Save`, `ResolvePath`). `runtime.FileIO()` is backed by `LocalFileIO` (in `localfileio.go`), which enforces `SafeInputPath`/`SafeOutputPath` before any filesystem access and reports violations as `fileio.PathValidationError`. Only require explicit `validate.SafeInputPath` when bypassing `runtime.FileIO()` and calling `vfs.*` or `os.*` directly with user-influenced paths.
Learnt from: fangshuyu-768
Repo: larksuite/cli PR: 696
File: tests/cli_e2e/drive/drive_pull_dryrun_test.go:73-100
Timestamp: 2026-04-28T13:14:18.257Z
Learning: In the `larksuite/cli` E2E dry-run test suite under `tests/cli_e2e/`, error handling has two intentional conventions—preserve them during reviews:
1) **Validate-stage rejects** should be treated as failures: the command should exit with a **non-zero exit code** and tests should assert `result.ExitCode != 0` and validate the error message from `result.Stdout + result.Stderr`.
2) **Dry-run callback/API errors** (e.g., errors set via `common.NewDryRunAPI().Set("error", ...)`) should be treated as successful execution at the process level: the command should exit with **code 0**, and the test should read the error message from the JSON output (e.g., `gjson.Get(result.Stdout, "error")`).
When `Validate` is the rejection mechanism, do **not** refactor tests to expect `exit=0` plus `stdout.error`; instead keep the `ExitCode != 0` assertions for those cases.
Learnt from: evandance
Repo: larksuite/cli PR: 984
File: internal/lintcheck/scan_test.go:0-0
Timestamp: 2026-05-20T08:44:00.596Z
Learning: In Go, the "declared and not used" compiler error is triggered only for variables that are never referenced after declaration. A statement like `x++` counts as a use, so a variable that is only incremented (e.g., `rejectCount++`) will still compile even if it is never read. When reviewing Go code, don’t assume `x++`-only variables would fail to compile—treat them as potentially semantically dead code instead (remove/avoid unless it’s required for later reads, logging, metrics, or other side effects).
Learnt from: kyalpha313
Repo: larksuite/cli PR: 1114
File: shortcuts/drive/drive_list_comments_test.go:86-132
Timestamp: 2026-05-26T14:20:06.037Z
Learning: In larksuite/cli Go test files, use the established test helper patterns based on what the test exercises:
- Use `common.TestNewRuntimeContext(t, config)` for validate/dry-run unit tests that only cover flag parsing and branch logic and do not perform HTTP requests (or HTTP mocking).
- Use `cmdutil.TestFactory(t, config)` for tests that need to make requests and/or mock HTTP interactions (e.g., HTTP-mocked E2E tests).
When reviewing, do not treat `common.TestNewRuntimeContext` usage as incorrect if the test is a validate/dry-run unit test that doesn’t require HTTP request mocking; only require `cmdutil.TestFactory` when HTTP calls/mocking are actually involved.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
shortcuts/im/im_messages_card_update.go (1)
59-75: 💤 Low valueOptional: surface the server response instead of hardcoding
updated: true.
Executediscards the PATCH response and always reportsupdated: true. The siblingImMessagesUpdatereadsmessage_id/chat_id/update_time/updatedfrom the response payload. Consider mirroring that for output consistency, if the card-update response carries those fields.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@shortcuts/im/im_messages_card_update.go` around lines 59 - 75, The Execute handler currently discards the PATCH response and returns a hardcoded updated: true; change it to capture and use the API response from runtime.DoAPIJSON (the call inside Execute) and mirror the same output fields used by ImMessagesUpdate (e.g., message_id, chat_id, update_time, updated) if present. After calling runtime.DoAPIJSON, inspect the returned JSON payload (or decoded map) and pass those values into runtime.Out instead of the hardcoded map so the card output reflects the server response.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@shortcuts/im/im_messages_card_update.go`:
- Around line 59-75: The Execute handler currently discards the PATCH response
and returns a hardcoded updated: true; change it to capture and use the API
response from runtime.DoAPIJSON (the call inside Execute) and mirror the same
output fields used by ImMessagesUpdate (e.g., message_id, chat_id, update_time,
updated) if present. After calling runtime.DoAPIJSON, inspect the returned JSON
payload (or decoded map) and pass those values into runtime.Out instead of the
hardcoded map so the card output reflects the server response.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 6661fe02-f175-4a02-9320-0116cebc2824
📒 Files selected for processing (12)
shortcuts/im/builders_test.goshortcuts/im/helpers_test.goshortcuts/im/im_messages_card_update.goshortcuts/im/im_messages_card_update_test.goshortcuts/im/im_messages_update.goshortcuts/im/im_messages_update_test.goshortcuts/im/shortcuts.goskills/lark-im/SKILL.mdskills/lark-im/references/lark-im-messages-card-update.mdskills/lark-im/references/lark-im-messages-update.mdtests/cli_e2e/im/message_update_dryrun_test.gotests/cli_e2e/im/message_update_workflow_test.go
✅ Files skipped from review due to trivial changes (3)
- skills/lark-im/references/lark-im-messages-card-update.md
- skills/lark-im/references/lark-im-messages-update.md
- skills/lark-im/SKILL.md
🚧 Files skipped from review as they are similar to previous changes (4)
- shortcuts/im/shortcuts.go
- shortcuts/im/helpers_test.go
- shortcuts/im/im_messages_update_test.go
- shortcuts/im/im_messages_update.go
David-Buxy
force-pushed
the
codex/im-message-update-shortcut
branch
from
975c535 to
7e5b06f
Compare
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
Add IM shortcuts for editing messages sent by the current app. This supports agent workflows that send a placeholder/progress message and later replace it with the final text, post, or shared interactive card result.
Changes
im +messages-updatebacked byPUT /open-apis/im/v1/messages/{message_id}fortextandpostmessages.im +messages-card-updatebacked byPATCH /open-apis/im/v1/messages/{message_id}for shared interactive card messages.--text,--markdown, and raw--contentJSON inputs for text/post updates, reusing existing IM markdown/post helpers and mention normalization.Test Plan
make unit-testgo vet ./...,make fmt-check,go mod tidyproduced nogo.mod/go.sumdiff, andgolangci-lint --new-from-rev=origin/mainreported 0 issuesgo test ./shortcuts/im -count=1go test ./tests/cli_e2e/im -run 'TestIM_Message(Update|CardUpdate)' -count=1lark-cli im +messages-updateworks against a Feishu test group: send placeholder, update it, then confirm viaim +messages-mgetthatcontentchanged andupdatedis truelark-cli im +messages-card-updateemits the expectedPATCH /open-apis/im/v1/messages/{message_id}card update request shapeRelated Issues
Summary by CodeRabbit
New Features
Documentation
Tests