feat: add application domain with slash command management shortcuts#1806
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
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 (19)
🚧 Files skipped from review as they are similar to previous changes (17)
📝 WalkthroughWalkthroughAdds an ChangesApplication slash-command feature
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Suggested labels: Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant CLI
participant Shortcut
participant ApplicationAPI
participant Resolver
CLI->>Shortcut: Run slash-command operation
Shortcut->>ApplicationAPI: GET, POST, PATCH, or DELETE
ApplicationAPI-->>Shortcut: Response or command collision
Shortcut->>Resolver: Resolve command name when needed
Resolver->>ApplicationAPI: GET slash-command list
ApplicationAPI-->>Resolver: Command items
Resolver-->>Shortcut: command_id
Shortcut-->>CLI: Formatted result or typed error
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #1806 +/- ##
========================================
Coverage 74.58% 74.59%
========================================
Files 863 870 +7
Lines 90123 90340 +217
========================================
+ Hits 67220 67386 +166
- Misses 17696 17730 +34
- Partials 5207 5224 +17 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
🚀 PR Preview Install Guide🧰 CLI updatenpm i -g https://pkg.pr.new/larksuite/cli/@larksuite/cli@cbf71112915f583b71633591225e72b2e3c222c3🧩 Skill updatenpx skills add larksuite/cli#feat/application-slash-command -y -g |
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (4)
shortcuts/application/slash_command_common.go (1)
92-98: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse
p.Code == 40000000here.CallAPITypedpreserves the upstream code onerrs.Problem, so code-based matching is more stable thanstrings.Contains(p.Message, "command already exists").🤖 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/application/slash_command_common.go` around lines 92 - 98, Update isCommandExists in slash_command_common.go to use the upstream problem code instead of matching the message text. Keep the errs.ProblemOf(err) check, but replace the strings.Contains(p.Message, "command already exists") logic with a direct comparison against p.Code == 40000000 so CallAPITyped-based errors are identified more reliably.shortcuts/application/slash_command_create_test.go (1)
146-158: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert typed metadata in the non-conflict error test.
TestSlashCommandCreate_ForceDoesNotConvertNonConflictonly checkserr.Error()for the "icon_key" substring. Per coding guidelines, error-path tests must assert typed metadata viaerrs.ProblemOf(category/subtype/param), not message substrings alone. Consider also verifying the error category/subtype to confirm the original API error surfaces unchanged.💡 Suggested addition
if !strings.Contains(err.Error(), "icon_key") { t.Fatalf("expected original icon_key failure to surface unchanged, got %v", err) } + p, ok := errs.ProblemOf(err) + if !ok || p.Code != 40000031 { + t.Fatalf("expected original API error (code 40000031) to surface unchanged, got %v (problem=%v, ok=%v)", err, p, ok) + }🤖 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/application/slash_command_create_test.go` around lines 146 - 158, `TestSlashCommandCreate_ForceDoesNotConvertNonConflict` only asserts on the error message substring, so update it to verify typed metadata instead of relying on `err.Error()` alone. In the test, keep the original failure expectation but also use `errs.ProblemOf` against the returned error from `mountAndRun` to assert the expected category/subtype/param from the original icon_key API error, ensuring `SlashCommandCreate` still surfaces the non-conflict error unchanged.Source: Coding guidelines
shortcuts/application/slash_command_update_test.go (1)
46-55: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert typed metadata in the not-found error test.
TestSlashCommandUpdate_ByNameNotFoundonly checkserr.Error()for the "not found" substring. Per coding guidelines, error-path tests must assert typed metadata viaerrs.ProblemOf(category/subtype/param), not message substrings alone. Consider also verifying the error category/subtype to confirm it's a typed not-found error fromresolveCommandID.💡 Suggested addition
if err == nil || !strings.Contains(err.Error(), "not found") { t.Fatalf("expected not found, got %v", err) } + p, ok := errs.ProblemOf(err) + if !ok || p.Category != errs.CategoryNotFound { + t.Fatalf("expected typed not-found error, got %v (problem=%v, ok=%v)", err, p, ok) + }🤖 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/application/slash_command_update_test.go` around lines 46 - 55, TestSlashCommandUpdate_ByNameNotFound currently only checks the error string, so update it to assert the typed problem metadata as well. Use errs.ProblemOf on the returned error from mountAndRun to verify the not-found category/subtype and the relevant param for the lookup failure, and keep the existing substring check only as a secondary guard if needed. This should reference the resolveCommandID path and ensure the test validates the typed not-found error rather than just matching "not found" in the message.Source: Coding guidelines
shortcuts/application/slash_command_update.go (1)
82-89: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAdd a Desc for the direct
--command-idPATCH path in DryRun.When
--command-idis provided, the DryRunAPI has no description set before the PATCH call. The by-name path sets a Desc for the GET step, but the direct-id path produces a descriptionless dry-run entry. Add a brief description for consistency.💡 Suggested fix
d := common.NewDryRunAPI() target := runtime.Str("command-id") if target == "" { d.Desc(fmt.Sprintf("resolve command_id by name %q via GET list first", runtime.Str("command"))). GET(slashCommandBasePath) target = "<resolved_command_id>" + } else { + d.Desc("Update a slash command by command_id") } return d.PATCH(slashCommandBasePath + "/" + target).Body(body)🤖 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/application/slash_command_update.go` around lines 82 - 89, The DryRun flow in slash command update is missing a description when `--command-id` is used directly, so the PATCH entry is anonymous compared with the by-name branch. Update the logic around `common.NewDryRunAPI()` and the `target := runtime.Str("command-id")` branch to set a brief `Desc` on `d` before calling `PATCH`, keeping the direct-id path consistent with the existing description set for the GET resolution path.
🤖 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/application/slash_command_common_test.go`:
- Around line 49-54: In TestParseDescriptionI18n_DuplicateLang, stop validating
the error only by substring and assert its typed metadata instead. Use
errs.ProblemOf on the error returned from parseDescriptionI18n to verify the
expected category, subtype, and param values for the duplicate-language case,
and keep any cause-preservation check if available. This test should
specifically target the parseDescriptionI18n error contract rather than the
error message text.
In `@shortcuts/application/slash_command_create.go`:
- Around line 84-92: The rewrapped API error in the slash command creation flow
is dropping the original cause, so update the error construction in the
CallAPITyped handling path to preserve the chain. In the branch that builds the
new API error for “slash command already exists”, add the original err via
WithCause(err) on the errs.NewAPIError result before returning it, while keeping
the existing Subtype, Code, LogID, and hint behavior intact.
In `@shortcuts/application/slash_command_delete_test.go`:
- Around line 93-109: The validation error-path test in
TestSlashCommandDelete_Validate only checks errs.CategoryValidation and should
also assert the typed subtype from errs.ProblemOf. Update the test to verify
p.Subtype is errs.SubtypeInvalidArgument for the validation failures triggered
by SlashCommandDelete, alongside the existing category check.
- Line 67: The `TestSlashCommandDelete_ByNameWithYes` test is ignoring the
`json.Unmarshal` error, unlike `TestSlashCommandDelete_ByIDWithYes`, which can
hide invalid output and lead to a later panic. Update the unmarshalling in
`TestSlashCommandDelete_ByNameWithYes` to check and fail on the error the same
way as the by-ID test, using the `stdout.Bytes()` to `got` decode step so the
test stops with a clear failure if the output is not valid JSON.
In `@shortcuts/application/slash_command_resolve_test.go`:
- Around line 31-39: `TestResolveNotFoundErrorShape` is only validating the
error string from `commandNotFoundError`, but it should assert the typed problem
metadata instead. Update the test to inspect the returned error with
`errs.ProblemOf` and verify the expected category/subtype (and param if
applicable) for `commandNotFoundError`, while also keeping any
cause-preservation check needed for this error path. Avoid relying on
`strings.Contains` message substrings as the primary assertion.
In `@skills/lark-application/references/lark-application-slash-command-update.md`:
- Line 17: Update the `--description` option description in the
`lark-application` slash-command update reference to use clearer Chinese
wording. The current phrasing `新的默认描述` is awkward for user-facing docs; change
it to something like `默认描述的新值`, keeping the meaning aligned with
`description.default_value` and preserving the rest of the table entry
structure.
In `@skills/lark-application/SKILL.md`:
- Around line 13-17: The scope summary in SKILL.md is incomplete because it
implies application:app_slash_command:read is only needed for listing, but the
by-name update/delete flows and create --force also require it. Update the
permissions note near the top-level guidance and reference the relevant command
behavior from the app slash command docs so it explicitly calls out that read
scope is needed for list plus name-based update/delete and force-create paths,
while write scope covers create/update/delete actions.
---
Nitpick comments:
In `@shortcuts/application/slash_command_common.go`:
- Around line 92-98: Update isCommandExists in slash_command_common.go to use
the upstream problem code instead of matching the message text. Keep the
errs.ProblemOf(err) check, but replace the strings.Contains(p.Message, "command
already exists") logic with a direct comparison against p.Code == 40000000 so
CallAPITyped-based errors are identified more reliably.
In `@shortcuts/application/slash_command_create_test.go`:
- Around line 146-158: `TestSlashCommandCreate_ForceDoesNotConvertNonConflict`
only asserts on the error message substring, so update it to verify typed
metadata instead of relying on `err.Error()` alone. In the test, keep the
original failure expectation but also use `errs.ProblemOf` against the returned
error from `mountAndRun` to assert the expected category/subtype/param from the
original icon_key API error, ensuring `SlashCommandCreate` still surfaces the
non-conflict error unchanged.
In `@shortcuts/application/slash_command_update_test.go`:
- Around line 46-55: TestSlashCommandUpdate_ByNameNotFound currently only checks
the error string, so update it to assert the typed problem metadata as well. Use
errs.ProblemOf on the returned error from mountAndRun to verify the not-found
category/subtype and the relevant param for the lookup failure, and keep the
existing substring check only as a secondary guard if needed. This should
reference the resolveCommandID path and ensure the test validates the typed
not-found error rather than just matching "not found" in the message.
In `@shortcuts/application/slash_command_update.go`:
- Around line 82-89: The DryRun flow in slash command update is missing a
description when `--command-id` is used directly, so the PATCH entry is
anonymous compared with the by-name branch. Update the logic around
`common.NewDryRunAPI()` and the `target := runtime.Str("command-id")` branch to
set a brief `Desc` on `d` before calling `PATCH`, keeping the direct-id path
consistent with the existing description set for the GET resolution path.
🪄 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: 2a75613c-4e75-4220-b34c-adbb874a2764
📒 Files selected for processing (25)
cmd/auth/login_messages.gointernal/registry/registry_test.gointernal/registry/scope_overrides.jsoninternal/registry/service_descriptions.jsonshortcuts/application/shortcuts.goshortcuts/application/slash_command_common.goshortcuts/application/slash_command_common_test.goshortcuts/application/slash_command_create.goshortcuts/application/slash_command_create_test.goshortcuts/application/slash_command_delete.goshortcuts/application/slash_command_delete_test.goshortcuts/application/slash_command_list.goshortcuts/application/slash_command_list_test.goshortcuts/application/slash_command_resolve.goshortcuts/application/slash_command_resolve_test.goshortcuts/application/slash_command_update.goshortcuts/application/slash_command_update_test.goshortcuts/register.goskill-template/domains/application.mdskills/lark-application/SKILL.mdskills/lark-application/references/lark-application-slash-command-create.mdskills/lark-application/references/lark-application-slash-command-delete.mdskills/lark-application/references/lark-application-slash-command-list.mdskills/lark-application/references/lark-application-slash-command-update.mdtests/cli_e2e/application/slash_command_dryrun_test.go
| |---|---|---|---| | ||
| | `--command-id` | string | 与 `--command` 二选一 | 目标指令的 `command_id`(来自 `+slash-command-list` 或 create 的输出) | | ||
| | `--command` | string | 与 `--command-id` 二选一 | 按名字定位(不带前导 `/`),CLI 会先调用 list 解析出 `command_id`,需要 read scope | | ||
| | `--description` | string | 否 | 新的默认描述(`description.default_value`) | |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Polish the --description label.
新的默认描述 reads a bit awkwardly in Chinese user-facing docs; 默认描述的新值 (or similar) is clearer. Based on the static analysis hint on this line.
Suggested wording
-| `--description` | string | 否 | 新的默认描述(`description.default_value`) |
+| `--description` | string | 否 | 默认描述的新值(`description.default_value`) |📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| | `--description` | string | 否 | 新的默认描述(`description.default_value`) | | |
| | `--description` | string | 否 | 默认描述的新值(`description.default_value`) | |
🧰 Tools
🪛 LanguageTool
[uncategorized] ~17-~17: 动词的修饰一般为‘形容词(副词)+地+动词’。您的意思是否是:新"地"默认
Context: ...ope | | --description | string | 否 | 新的默认描述(description.default_value) | | `-...
(wb4)
🤖 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 `@skills/lark-application/references/lark-application-slash-command-update.md`
at line 17, Update the `--description` option description in the
`lark-application` slash-command update reference to use clearer Chinese
wording. The current phrasing `新的默认描述` is awkward for user-facing docs; change
it to something like `默认描述的新值`, keeping the meaning aligned with
`description.default_value` and preserving the rest of the table entry
structure.
Source: Linters/SAST tools
| ## 身份权限和风险处理 | ||
|
|
||
| 开始前先读 [`../lark-shared/SKILL.md`](../lark-shared/SKILL.md)(认证、权限处理、`--dry-run`、exit 10 高风险确认协议)。 | ||
| `--as bot` 或 `--as user`均支持。 | ||
| 两个权限:`application:app_slash_command:read`(列出)/ `application:app_slash_command:write`(创建/更新/删除) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Clarify the conditional read-scope requirement.
The scope summary reads as if application:app_slash_command:read is only for listing, but the by-name update/delete paths and create --force also need it. Please call that out here so the top-level guidance matches the per-command docs and avoids preventable auth failures. Based on the command docs in this PR.
Suggested wording
-两个权限:`application:app_slash_command:read`(列出)/ `application:app_slash_command:write`(创建/更新/删除)
+两个主要权限:`application:app_slash_command:read`(列出;以及按名字定位 / `--force` 撞名重跑时需要)/ `application:app_slash_command:write`(创建/更新/删除)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| ## 身份权限和风险处理 | |
| 开始前先读 [`../lark-shared/SKILL.md`](../lark-shared/SKILL.md)(认证、权限处理、`--dry-run`、exit 10 高风险确认协议)。 | |
| `--as bot` 或 `--as user`均支持。 | |
| 两个权限:`application:app_slash_command:read`(列出)/ `application:app_slash_command:write`(创建/更新/删除) | |
| ## 身份权限和风险处理 | |
| 开始前先读 [`../lark-shared/SKILL.md`](../lark-shared/SKILL.md)(认证、权限处理、`--dry-run`、exit 10 高风险确认协议)。 | |
| `--as bot` 或 `--as user`均支持。 | |
| 两个主要权限:`application:app_slash_command:read`(列出;以及按名字定位 / `--force` 撞名重跑时需要)/ `application:app_slash_command:write`(创建/更新/删除) |
🤖 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 `@skills/lark-application/SKILL.md` around lines 13 - 17, The scope summary in
SKILL.md is incomplete because it implies application:app_slash_command:read is
only needed for listing, but the by-name update/delete flows and create --force
also require it. Update the permissions note near the top-level guidance and
reference the relevant command behavior from the app slash command docs so it
explicitly calls out that read scope is needed for list plus name-based
update/delete and force-create paths, while write scope covers
create/update/delete actions.
|
Fixed in 78acceb — added |
…ict code - commandNotFoundError now returns errs.NewAPIError(SubtypeNotFound, ...) instead of a validation error: a resolution miss against the live list means the resource doesn't exist, not that the caller's argument shape was invalid. - The no-force name-collision rewrap in +slash-command-create now carries over the original Code/LogID from the upstream conflict response instead of dropping them. - +slash-command-delete now prints an extra stderr note that recreating the same command name yields a NEW command_id. - Add a test guarding that --force only converts a name-collision response into an update; any other POST failure (e.g. invalid icon_key) must surface unchanged with no PATCH attempted.
Pin the dry-run request shapes for +slash-command-list/create/update/delete (GET/POST/PATCH/DELETE paths, top-level icon key sibling to description, description.i18n) and confirm the high-risk-write delete path exits 10 with a confirmation_required envelope on stderr when --yes is omitted.
41ea383 to
cbf7111
Compare
Summary
Feishu Open Platform now exposes slash command management APIs (application v7, not yet in the meta catalog). This PR adds a new
applicationdomain with four handwritten shortcuts so agents and developers can manage the bound app's slash commands directly from the CLI instead of hand-crafting raw v7 requests. The read-only list command is admitted despite being a thin wrapper because it is the by-name resolution base for update/delete andcreate --force, and it anchors the new domain's output contract.Changes
shortcuts/application/with+slash-command-list,+slash-command-create,+slash-command-update,+slash-command-delete(raw v7 paths viaruntime.CallAPIRawTyped-style call, path segments encoded)+slash-command-create:--forceturns a name collision into a resolve-by-name PATCH (likegh label create --force); repeatable--description-i18n <lang>=<text>; icon key passed top-level per live-tested API shape+slash-command-update: field-level PATCH semantics; target by--command-idor--command(resolved via live list); i18n map replace semantics documented with a CAUTION+slash-command-delete: high-risk-write behind the framework confirmation gate (exit 10 without--yes); dry-run states the two-step by-name pathScopesplusConditionalScopes(read scope for by-name/--forcepaths) declared for both bot and user identitiesshortcuts/register.go; bilingual domain descriptions ininternal/registry/service_descriptions.jsonauth login: listapplicationamong shortcut-only domains incmd/auth/login_messages.go; admitapplication:app_slash_command:readto the recommended tier viainternal/registry/scope_overrides.json(v7 scopes are absent from the platform priorities catalog, so the common tier previously resolved to zero scopes; write scope intentionally stays out of the recommended tier)--help— the usage gotchas live in flag descriptions and command tipstests/cli_e2e/application/slash_command_dryrun_test.goTest Plan
make unit-testpassedlark-cli application +slash-command-list --json,+slash-command-create --command test-cli-demo --description ... [--force],+slash-command-update --command test-cli-demo --description ...,+slash-command-delete --command test-cli-demo --yes, plus validation-error, name-collision, missing-scope and confirmation-gate (exit 10) pathsRelated Issues
N/A
Summary by CodeRabbit
New Features
Improvements
Tests