From 09feefe96b756590eeb5235efda230ea5c9b4fb1 Mon Sep 17 00:00:00 2001 From: evandance <120630830+evandance@users.noreply.github.com> Date: Wed, 5 Aug 2026 19:44:11 +0800 Subject: [PATCH] fix: make agent recovery and concealment reliable (#2189) --- cmd/auth/check.go | 3 +- cmd/auth/check_test.go | 7 +- cmd/auth/login.go | 16 +- cmd/auth/login_messages.go | 41 ++- cmd/auth/login_messages_test.go | 37 ++- cmd/auth/login_test.go | 127 +++++++- cmd/build.go | 10 +- cmd/error_auth_hint.go | 97 +----- cmd/error_presenter_test.go | 279 ++++++++++++++++-- cmd/presentation_test.go | 53 ++++ cmd/root.go | 2 +- cmd/root_test.go | 117 ++++++-- cmd/schema/schema.go | 173 ++++++++++- cmd/schema/schema_test.go | 161 +++++++++- cmd/skill_customization_test.go | 46 +++ errs/ERROR_CONTRACT.md | 42 ++- extension/platform/README.md | 4 +- extension/platform/skillsoverlay.go | 5 + internal/cmdutil/error_presenter.go | 126 ++++++++ internal/cmdutil/error_presenter_test.go | 105 +++++++ internal/cmdutil/factory.go | 27 +- internal/cmdutil/factory_test.go | 57 ++++ internal/errclass/classify.go | 95 ++++-- internal/errclass/classify_test.go | 129 ++++++++ internal/errclass/hint_gate_test.go | 37 ++- internal/recovery/context.go | 54 ++++ internal/recovery/hint.go | 77 +++-- internal/recovery/hint_test.go | 88 ++++++ internal/recovery/projector.go | 19 +- internal/recovery/render.go | 14 +- internal/skillpolicy/dependencies.go | 112 +++++++ internal/skillpolicy/overlay.go | 28 +- internal/skillpolicy/resolver.go | 22 +- internal/skillpolicy/resolver_test.go | 160 ++++++++++ shortcuts/common/mcp_client.go | 81 +++-- shortcuts/common/mcp_client_test.go | 279 +++++++++++++++++- shortcuts/common/runner.go | 53 +++- .../common/runner_error_presenter_test.go | 137 +++++++++ shortcuts/common/skill_references.go | 42 +++ shortcuts/doc/docs_create_test.go | 19 +- shortcuts/doc/docs_fetch_v2_test.go | 7 +- shortcuts/doc/docs_update_test.go | 17 +- shortcuts/doc/v2_only.go | 28 +- shortcuts/doc/v2_only_test.go | 129 +++++++- shortcuts/task/tasklist_add_task.go | 5 +- shortcuts/task/tasklist_add_task_test.go | 72 +++++ shortcuts/task/tasklist_create.go | 9 +- shortcuts/task/tasklist_create_test.go | 86 +++++- .../vc/vc_calendar_event_recovery_test.go | 156 ++++++++++ shortcuts/vc/vc_notes.go | 20 +- shortcuts/vc/vc_notes_test.go | 66 +++++ shortcuts/vc/vc_recording.go | 2 +- skills/lark-doc/SKILL.md | 1 + skills/lark-shared/SKILL.md | 6 +- tests/cli_e2e/docs/docs_update_dryrun_test.go | 34 +++ tests/plugin_e2e/harness.go | 13 +- tests/plugin_e2e/restrict_test.go | 171 +++++++++++ tests/plugin_e2e/skills_test.go | 131 +++++++- 58 files changed, 3606 insertions(+), 328 deletions(-) create mode 100644 internal/cmdutil/error_presenter.go create mode 100644 internal/cmdutil/error_presenter_test.go create mode 100644 internal/recovery/context.go create mode 100644 internal/skillpolicy/dependencies.go create mode 100644 shortcuts/common/runner_error_presenter_test.go create mode 100644 shortcuts/common/skill_references.go create mode 100644 shortcuts/vc/vc_calendar_event_recovery_test.go diff --git a/cmd/auth/check.go b/cmd/auth/check.go index fd650ecef..41f211174 100644 --- a/cmd/auth/check.go +++ b/cmd/auth/check.go @@ -4,7 +4,6 @@ package auth import ( - "fmt" "strings" "github.com/spf13/cobra" @@ -96,7 +95,7 @@ func authCheckRunWithRecovery(opts *CheckOptions, projector *recovery.Projector) ok := len(missing) == 0 result := map[string]interface{}{"ok": ok, "granted": granted, "missing": missing} if len(missing) > 0 && projector.CanReference(recovery.TargetAuthLogin) { - result["suggestion"] = fmt.Sprintf(`lark-cli auth login --scope "%s"`, strings.Join(missing, " ")) + result["suggestion"] = projector.RenderHint(recovery.UserAuthorization(missing...)) } output.PrintJson(f.IOStreams.Out, result) if !ok { diff --git a/cmd/auth/check_test.go b/cmd/auth/check_test.go index 2d5341b40..5863fd0e9 100644 --- a/cmd/auth/check_test.go +++ b/cmd/auth/check_test.go @@ -6,7 +6,6 @@ package auth import ( "encoding/json" "errors" - "strings" "testing" "time" @@ -170,6 +169,7 @@ func TestAuthCheckRun_ConcealedLoginOmitsSuggestion(t *testing.T) { keyring.MockInit() t.Setenv("HOME", t.TempDir()) t.Setenv("LARKSUITE_CLI_DATA_DIR", t.TempDir()) + t.Setenv("LARKSUITE_CLI_CONFIG_DIR", t.TempDir()) cfg := &core.CliConfig{ AppID: "test-app", @@ -203,8 +203,9 @@ func TestAuthCheckRun_ConcealedLoginOmitsSuggestion(t *testing.T) { if err := json.Unmarshal(visibleStdout.Bytes(), &visiblePayload); err != nil { t.Fatalf("default stdout must be valid JSON: %v", err) } - if suggestion, _ := visiblePayload["suggestion"].(string); !strings.Contains(suggestion, "auth login") { - t.Fatalf("default output lost established login suggestion: %#v", visiblePayload) + const wantSuggestion = "run `lark-cli auth login --scope \"calendar:calendar:read\" --no-wait --json` to get device_code and verification_url; present verification_url to the user exactly and end this turn; after the user confirms authorization, run `lark-cli auth login --device-code ` in a later turn to finish login" + if suggestion, _ := visiblePayload["suggestion"].(string); suggestion != wantSuggestion { + t.Fatalf("default suggestion = %q, want executable split-flow recovery %q", suggestion, wantSuggestion) } f, stdout, stderr, _ := cmdutil.TestFactory(t, cfg) diff --git a/cmd/auth/login.go b/cmd/auth/login.go index 3b240b176..6cbbce281 100644 --- a/cmd/auth/login.go +++ b/cmd/auth/login.go @@ -20,6 +20,7 @@ import ( "github.com/larksuite/cli/internal/core" "github.com/larksuite/cli/internal/i18n" "github.com/larksuite/cli/internal/output" + "github.com/larksuite/cli/internal/recovery" "github.com/larksuite/cli/internal/registry" "github.com/larksuite/cli/shortcuts" "github.com/larksuite/cli/shortcuts/common" @@ -131,6 +132,7 @@ func authLoginRun(opts *LoginOptions) error { } } msg := getLoginMsg(lang) + renderContext := recovery.RenderContext{Profile: f.Invocation.Profile} log := func(format string, a ...interface{}) { if !opts.JSON { @@ -279,13 +281,7 @@ func authLoginRun(opts *LoginOptions) error { "verification_url": authResp.VerificationUriComplete, "device_code": authResp.DeviceCode, "expires_in": authResp.ExpiresIn, - "hint": "**MUST generate QR code AND display it:** You MUST call lark-cli auth qrcode to convert verification_url into a QR code. This is a required step, do NOT skip it. Prefer PNG QR code (--output); use ASCII (--ascii) only when the user explicitly requests it." + - "**CRITICAL: You MUST include the QR image in your response.** Generating the file alone is NOT enough—use image tags, inline images, or file attachments to display it." + - "**Display order:** Output the URL first, then place the QR code image below the URL." + - "**URL Output Rules:** Treat verification_url as an opaque string that cannot be modified. Do NOT URL-encode/decode or add spaces/punctuation." + - "For agent harnesses that only deliver final turn messages, make the QR code image (or URL) the final message of the turn and return control to the user; do not block on --device-code in the same turn. **Before ending the turn, tell the user to come back and notify you after completing authorization.**" + - "**After the user confirms authorization:** YOU must execute `lark-cli auth login --device-code ` yourself." + - "**Do NOT cache verification_url or device_code for future use.** Always run `lark-cli auth login --no-wait --json` fresh when authorization is needed.", + "hint": noWaitAgentHint(renderContext), } encoder := json.NewEncoder(f.IOStreams.Out) encoder.SetEscapeHTML(false) @@ -308,7 +304,7 @@ func authLoginRun(opts *LoginOptions) error { "verification_uri_complete": authResp.VerificationUriComplete, "user_code": authResp.UserCode, "expires_in": authResp.ExpiresIn, - "agent_hint": msg.AgentTimeoutHint, + "agent_hint": msg.AgentTimeoutHint(renderContext), } encoder := json.NewEncoder(f.IOStreams.Out) encoder.SetEscapeHTML(false) @@ -319,7 +315,7 @@ func authLoginRun(opts *LoginOptions) error { fmt.Fprintf(f.IOStreams.ErrOut, msg.OpenURL) fmt.Fprintf(f.IOStreams.ErrOut, " %s\n\n", authResp.VerificationUriComplete) if f.IOStreams != nil && !f.IOStreams.IsTerminal { - fmt.Fprintln(f.IOStreams.ErrOut, msg.AgentTimeoutHint) + fmt.Fprintln(f.IOStreams.ErrOut, msg.AgentTimeoutHint(renderContext)) } } @@ -412,7 +408,7 @@ func authLoginPollDeviceCode(opts *LoginOptions, config *core.CliConfig, msg *lo // when running on an interactive terminal — the agent-oriented // instructions only matter for piped / harness environments. if !opts.JSON && f.IOStreams != nil && !f.IOStreams.IsTerminal { - fmt.Fprintln(f.IOStreams.ErrOut, msg.AgentTimeoutHint) + fmt.Fprintln(f.IOStreams.ErrOut, msg.AgentTimeoutHint(recovery.RenderContext{Profile: f.Invocation.Profile})) } log(msg.WaitingAuth) result := pollDeviceToken(opts.Ctx, httpClient, config.AppID, config.AppSecret, config.Brand, diff --git a/cmd/auth/login_messages.go b/cmd/auth/login_messages.go index 2dee8992f..46fbd3c41 100644 --- a/cmd/auth/login_messages.go +++ b/cmd/auth/login_messages.go @@ -3,7 +3,10 @@ package auth -import "github.com/larksuite/cli/internal/i18n" +import ( + "github.com/larksuite/cli/internal/i18n" + "github.com/larksuite/cli/internal/recovery" +) type loginMsg struct { // Interactive UI (login_interactive.go) @@ -24,7 +27,7 @@ type loginMsg struct { // Non-interactive prompts (login.go) OpenURL string WaitingAuth string - AgentTimeoutHint string + AgentTimeoutHint func(recovery.RenderContext) string AuthSuccess string LoginSuccess string AuthorizedUser string @@ -61,7 +64,7 @@ var loginMsgZh = &loginMsg{ OpenURL: "在浏览器中打开以下链接进行认证:\n\n", WaitingAuth: "等待用户授权...", - AgentTimeoutHint: "[AI agent] 此命令最长阻塞约 10 分钟，等待用户在浏览器内完成授权。请确保 runner 的 timeout >= 600s。若你的 harness 或 agent 工具只会把最终回复发给用户，请改用 \"lark-cli auth login --no-wait --json\" 拿到 device_code 和 verification_url，把 verification_url 作为本轮最终消息发给用户并结束本轮；等用户回复已完成授权后，再在后续步骤运行 \"lark-cli auth login --device-code \" 续上轮询。**不要在同一轮里展示 URL 后立刻阻塞执行 --device-code**，也不要短 timeout 反复重试；每次重启会作废上一轮的 device code，导致用户授权链接失效。**必须生成二维码并展示**: 你必须调用 lark-cli auth qrcode 将 verification_url 转为二维码，这是必须步骤，不要跳过。优先生成 PNG 二维码（--output）；仅当用户明确要求时才使用 ASCII（--ascii）。**关键：生成后必须在回复中展示图片**，仅生成文件不算完成，须用 image 标签或内联图片等方式将图片包含在回复中。**展示顺序**: 先输出 URL，再将二维码图片置于 URL 下方完整展示。**URL 输出规则**: 将 verification_url 视为不可修改的 opaque string，不要做任何修改（包括 URL 编码/解码、添加空格或标点）。", + AgentTimeoutHint: renderAgentTimeoutHintZh, AuthSuccess: "已收到授权确认，正在获取用户信息并校验授权结果...", LoginSuccess: "授权成功! 用户: %s (%s)", AuthorizedUser: "当前授权账号: %s (%s)", @@ -97,7 +100,7 @@ var loginMsgEn = &loginMsg{ OpenURL: "Open this URL in your browser to authenticate:\n\n", WaitingAuth: "Waiting for user authorization...", - AgentTimeoutHint: "[AI agent] This command blocks for up to ~10 minutes while waiting for the user to authorize in their browser. Make sure your runner's timeout is >= 600s. If your harness or agent tool only delivers final turn messages, use \"lark-cli auth login --no-wait --json\" to get device_code and verification_url, present verification_url to the user exactly as the final message of this turn, then end the turn; after the user replies that they authorized, run \"lark-cli auth login --device-code \" in a later step to resume polling. **Do NOT show the URL and then immediately block on --device-code in the same turn**, and do not retry with a short timeout; each restart invalidates the previous device code and makes the earlier authorization URL useless.**MUST generate QR code AND display it:** You MUST call lark-cli auth qrcode to convert verification_url into a QR code. This is a required step, do NOT skip it. Prefer PNG QR code (--output); use ASCII (--ascii) only when the user explicitly requests it.**CRITICAL: You MUST include the QR image in your response.** Generating the file alone is NOT enough—use image tags, inline images, or file attachments to display it.**Display order:** Output the URL first, then place the QR code image below the URL.**URL Output Rules:** Treat verification_url as an opaque string that cannot be modified. Do NOT URL-encode/decode or add spaces/punctuation.", + AgentTimeoutHint: renderAgentTimeoutHintEn, AuthSuccess: "Authorization confirmed, fetching user info and validating granted scopes...", LoginSuccess: "Authorization successful! User: %s (%s)", AuthorizedUser: "Authorized account: %s (%s)", @@ -116,6 +119,36 @@ var loginMsgEn = &loginMsg{ HintFooter: " lark-cli auth login --help", } +func renderAgentTimeoutHintZh(context recovery.RenderContext) string { + profileInstruction := "" + if context.Profile != "" { + profileInstruction = "，并使用 " + context.InlineAuthLoginCommand("") + " 保留显式 --profile" + } + return "[AI agent] 此命令最长阻塞约 10 分钟，等待用户在浏览器内完成授权。请确保 runner 的 timeout >= 600s。若你的 harness 或 agent 工具只会把最终回复发给用户，请沿用原请求的 --scope、--domain 或 --recommend 选择以及任何 --exclude 值" + + profileInstruction + + "，并附加 --no-wait --json，拿到 device_code 和 verification_url；把 verification_url 作为本轮最终消息发给用户并结束本轮；等用户回复已完成授权后，再在后续步骤运行 \"" + + context.AuthLoginCommand("--device-code ") + + "\" 续上轮询。**不要在同一轮里展示 URL 后立刻阻塞执行 --device-code**，也不要短 timeout 反复重试；每次重启会作废上一轮的 device code，导致用户授权链接失效。**必须生成二维码并展示**: 你必须调用 lark-cli auth qrcode 将 verification_url 转为二维码，这是必须步骤，不要跳过。优先生成 PNG 二维码（--output）；仅当用户明确要求时才使用 ASCII（--ascii）。**关键：生成后必须在回复中展示图片**，仅生成文件不算完成，须用 image 标签或内联图片等方式将图片包含在回复中。**展示顺序**: 先输出 URL，再将二维码图片置于 URL 下方完整展示。**URL 输出规则**: 将 verification_url 视为不可修改的 opaque string，不要做任何修改（包括 URL 编码/解码、添加空格或标点）。" +} + +func renderAgentTimeoutHintEn(context recovery.RenderContext) string { + return "[AI agent] This command blocks for up to ~10 minutes while waiting for the user to authorize in their browser. Make sure your runner's timeout is >= 600s. If your harness or agent tool only delivers final turn messages, rerun " + + context.InlineAuthLoginCommand("") + + " with the same --scope, --domain, or --recommend selection and any --exclude values, plus --no-wait --json, to get device_code and verification_url; present verification_url to the user exactly as the final message of this turn, then end the turn; after the user replies that they authorized, run \"" + + context.AuthLoginCommand("--device-code ") + + "\" in a later step to resume polling. **Do NOT show the URL and then immediately block on --device-code in the same turn**, and do not retry with a short timeout; each restart invalidates the previous device code and makes the earlier authorization URL useless.**MUST generate QR code AND display it:** You MUST call lark-cli auth qrcode to convert verification_url into a QR code. This is a required step, do NOT skip it. Prefer PNG QR code (--output); use ASCII (--ascii) only when the user explicitly requests it.**CRITICAL: You MUST include the QR image in your response.** Generating the file alone is NOT enough—use image tags, inline images, or file attachments to display it.**Display order:** Output the URL first, then place the QR code image below the URL.**URL Output Rules:** Treat verification_url as an opaque string that cannot be modified. Do NOT URL-encode/decode or add spaces/punctuation." +} + +func noWaitAgentHint(context recovery.RenderContext) string { + return "**MUST generate QR code AND display it:** You MUST call lark-cli auth qrcode to convert verification_url into a QR code. This is a required step, do NOT skip it. Prefer PNG QR code (--output); use ASCII (--ascii) only when the user explicitly requests it." + + "**CRITICAL: You MUST include the QR image in your response.** Generating the file alone is NOT enough—use image tags, inline images, or file attachments to display it." + + "**Display order:** Output the URL first, then place the QR code image below the URL." + + "**URL Output Rules:** Treat verification_url as an opaque string that cannot be modified. Do NOT URL-encode/decode or add spaces/punctuation." + + "For agent harnesses that only deliver final turn messages, make the QR code image (or URL) the final message of the turn and return control to the user; do not block on --device-code in the same turn. **Before ending the turn, tell the user to come back and notify you after completing authorization.**" + + "**After the user confirms authorization:** YOU must execute " + context.InlineAuthLoginCommand("--device-code ") + " yourself." + + "**Do NOT cache verification_url or device_code for future use.** When authorization is needed again, rerun " + context.InlineAuthLoginCommand("") + " with the same `--scope`, `--domain`, or `--recommend` selection and any `--exclude` values, plus `--no-wait --json` to get a fresh link." +} + // getLoginMsg returns the login message bundle for the given language. func getLoginMsg(lang i18n.Lang) *loginMsg { if lang.IsEnglish() { diff --git a/cmd/auth/login_messages_test.go b/cmd/auth/login_messages_test.go index a5c7f936c..21761e97c 100644 --- a/cmd/auth/login_messages_test.go +++ b/cmd/auth/login_messages_test.go @@ -4,12 +4,14 @@ package auth import ( + "crypto/sha256" "fmt" "reflect" "strings" "testing" "github.com/larksuite/cli/internal/i18n" + "github.com/larksuite/cli/internal/recovery" ) func TestGetLoginMsg_Zh(t *testing.T) { @@ -105,8 +107,8 @@ func TestLoginMsg_FormatStrings(t *testing.T) { // after presenting the URL instead of blocking in the same turn. func TestAgentTimeoutHint_CarriesKeyInfo(t *testing.T) { for _, lang := range []i18n.Lang{i18n.LangZhCN, i18n.LangEnUS} { - hint := getLoginMsg(lang).AgentTimeoutHint - for _, want := range []string{"--no-wait", "--device-code", "turn"} { + hint := getLoginMsg(lang).AgentTimeoutHint(recovery.RenderContext{}) + for _, want := range []string{"--scope", "--domain", "--recommend", "--exclude", "--no-wait", "--device-code", "turn"} { if lang == i18n.LangZhCN && want == "turn" { want = "本轮" } @@ -114,5 +116,36 @@ func TestAgentTimeoutHint_CarriesKeyInfo(t *testing.T) { t.Errorf("%s AgentTimeoutHint missing %q: %s", lang, want, hint) } } + if strings.Contains(hint, "lark-cli auth login --no-wait --json") { + t.Errorf("%s AgentTimeoutHint recommends an invalid optionless retry: %s", lang, hint) + } + } +} + +func TestAgentTimeoutHint_DefaultBytesStable(t *testing.T) { + wantSHA256 := map[i18n.Lang]string{ + i18n.LangZhCN: "9b9d23f6785d7a259de98620184fb05a4952464687a9f60982ce007aee39451e", + i18n.LangEnUS: "f39c9cd432668401040a4eda43b5ced0d4f20c0b8f55e06ef1773bc4048c6071", + } + for lang, want := range wantSHA256 { + hint := getLoginMsg(lang).AgentTimeoutHint(recovery.RenderContext{}) + if got := fmt.Sprintf("%x", sha256.Sum256([]byte(hint))); got != want { + t.Errorf("%s default AgentTimeoutHint digest = %s, want legacy %s", lang, got, want) + } + } +} + +func TestAgentTimeoutHint_ExplicitProfilePreservesStartAndResume(t *testing.T) { + context := recovery.RenderContext{Profile: "team-beta"} + for _, lang := range []i18n.Lang{i18n.LangZhCN, i18n.LangEnUS} { + hint := getLoginMsg(lang).AgentTimeoutHint(context) + for _, want := range []string{ + "`lark-cli auth login --profile='team-beta'`", + `"lark-cli auth login --profile='team-beta' --device-code "`, + } { + if !strings.Contains(hint, want) { + t.Errorf("%s profile-aware AgentTimeoutHint missing %q: %s", lang, want, hint) + } + } } } diff --git a/cmd/auth/login_test.go b/cmd/auth/login_test.go index f2aa3389a..f52d318a8 100644 --- a/cmd/auth/login_test.go +++ b/cmd/auth/login_test.go @@ -5,8 +5,10 @@ package auth import ( "context" + "crypto/sha256" "encoding/json" "errors" + "fmt" "io" "net/http" "slices" @@ -19,6 +21,7 @@ import ( "github.com/larksuite/cli/internal/core" "github.com/larksuite/cli/internal/httpmock" "github.com/larksuite/cli/internal/output" + "github.com/larksuite/cli/internal/recovery" "github.com/larksuite/cli/internal/registry" "github.com/larksuite/cli/shortcuts/common" "github.com/zalando/go-keyring" @@ -331,6 +334,63 @@ func TestAuthLoginRun_NonTerminal_NoFlags_RejectsWithHint(t *testing.T) { } } +func TestGenericUserAuthorizationStartCommandPassesLoginValidation(t *testing.T) { + const startCommand = "lark-cli auth login --recommend --no-wait --json" + if hint := recovery.UserAuthorization().String(); !strings.Contains(hint, startCommand) { + t.Fatalf("generic recovery = %q, want executable start command %q", hint, startCommand) + } + + f, stdout, _, reg := cmdutil.TestFactory(t, &core.CliConfig{ + ProfileName: "default", + AppID: "cli_test", + AppSecret: "secret", + Brand: core.BrandFeishu, + }) + reg.Register(&httpmock.Stub{ + Method: "POST", + URL: larkauth.PathDeviceAuthorization, + Body: map[string]interface{}{ + "device_code": "device-code", + "user_code": "user-code", + "verification_uri": "https://example.com/verify", + "verification_uri_complete": "https://example.com/verify?code=123", + "expires_in": 240, + "interval": 5, + }, + }) + + err := authLoginRun(&LoginOptions{ + Factory: f, + Ctx: context.Background(), + Recommend: true, + NoWait: true, + JSON: true, + }) + if err != nil { + t.Fatalf("generic recovery start command failed before returning a verification URL: %v", err) + } + var payload map[string]interface{} + if err := json.Unmarshal(stdout.Bytes(), &payload); err != nil { + t.Fatalf("decode login response: %v\nstdout=%s", err, stdout.String()) + } + if got := payload["verification_url"]; got != "https://example.com/verify?code=123" { + t.Fatalf("verification_url = %#v, want mocked URL", got) + } + hint, ok := payload["hint"].(string) + if !ok { + t.Fatalf("hint = %#v, want string", payload["hint"]) + } + if strings.Contains(hint, "lark-cli auth login --no-wait --json") { + t.Errorf("successful start response recommends an invalid optionless retry: %q", hint) + } + for _, want := range []string{"same `--scope`, `--domain`, or `--recommend` selection", "any `--exclude` values", "`--no-wait --json`"} { + if !strings.Contains(hint, want) { + t.Errorf("hint = %q, want executable fresh-login guidance containing %q", hint, want) + } + } + reg.Verify(t) +} + func TestEnsureRequestedScopesGranted(t *testing.T) { issue := ensureRequestedScopesGranted("im:message:send im:message:reply", "im:message:reply", getLoginMsg("en"), nil) if issue == nil { @@ -1050,7 +1110,9 @@ func TestAuthLoginRun_NoWaitJSONHintIncludesRawURLGuidance(t *testing.T) { "YOU must execute", "lark-cli auth login --device-code ", "Do NOT cache", - "lark-cli auth login --no-wait --json", + "same `--scope`, `--domain`, or `--recommend` selection", + "any `--exclude` values", + "`--no-wait --json`", } { if !strings.Contains(hint, want) { t.Fatalf("hint missing %q, got:\n%s", want, hint) @@ -1059,6 +1121,7 @@ func TestAuthLoginRun_NoWaitJSONHintIncludesRawURLGuidance(t *testing.T) { for _, unwanted := range []string{ "Then immediately execute", "Do not instruct the user to run this command themselves", + "lark-cli auth login --no-wait --json", } { if strings.Contains(hint, unwanted) { t.Fatalf("hint should not contain %q, got:\n%s", unwanted, hint) @@ -1066,6 +1129,68 @@ func TestAuthLoginRun_NoWaitJSONHintIncludesRawURLGuidance(t *testing.T) { } } +func TestNoWaitAgentHint_DefaultBytesStable(t *testing.T) { + const wantSHA256 = "bd1000350f418a4353807c45c68e1ee073127366bf9d8dd8a0a0f797e0adf8b7" + if got := fmt.Sprintf("%x", sha256.Sum256([]byte(noWaitAgentHint(recovery.RenderContext{})))); got != wantSHA256 { + t.Fatalf("default no-wait hint digest = %s, want legacy %s", got, wantSHA256) + } +} + +func TestAuthLoginRun_NoWaitJSONHintPreservesExplicitProfile(t *testing.T) { + f, stdout, _, reg := cmdutil.TestFactory(t, &core.CliConfig{ + ProfileName: "team-beta", + AppID: "cli_test", + AppSecret: "secret", + Brand: core.BrandFeishu, + }) + f.Invocation.Profile = "team-beta" + + reg.Register(&httpmock.Stub{ + Method: "POST", + URL: larkauth.PathDeviceAuthorization, + Body: map[string]interface{}{ + "device_code": "device-code", + "user_code": "user-code", + "verification_uri": "https://example.com/verify", + "verification_uri_complete": "https://example.com/verify?code=123", + "expires_in": 240, + "interval": 5, + }, + }) + + if err := authLoginRun(&LoginOptions{ + Factory: f, + Ctx: context.Background(), + Scope: "im:message:send", + NoWait: true, + JSON: true, + }); err != nil { + t.Fatalf("authLoginRun() error = %v", err) + } + + var data map[string]interface{} + if err := json.NewDecoder(strings.NewReader(stdout.String())).Decode(&data); err != nil { + t.Fatalf("Decode(stdout) error = %v, stdout=%q", err, stdout.String()) + } + hint, _ := data["hint"].(string) + for _, want := range []string{ + "`lark-cli auth login --profile='team-beta' --device-code `", + "rerun `lark-cli auth login --profile='team-beta'` with the same", + } { + if !strings.Contains(hint, want) { + t.Errorf("profile-aware no-wait JSON hint missing %q: %s", want, hint) + } + } + for _, stale := range []string{ + "`lark-cli auth login --device-code `", + "rerun `lark-cli auth login` with the same", + } { + if strings.Contains(hint, stale) { + t.Errorf("profile-aware no-wait JSON hint retained stale command %q: %s", stale, hint) + } + } +} + func TestAuthLoginRun_JSONWriteFailure_DeviceAuthorizationReturnsWriterError(t *testing.T) { f, _, _, reg := cmdutil.TestFactory(t, &core.CliConfig{ ProfileName: "default", diff --git a/cmd/build.go b/cmd/build.go index e6a1a29c9..cbd7267ff 100644 --- a/cmd/build.go +++ b/cmd/build.go @@ -7,6 +7,7 @@ import ( "context" "io" "io/fs" + "strings" "github.com/larksuite/cli/cmd/api" "github.com/larksuite/cli/cmd/auth" @@ -224,9 +225,9 @@ func buildInternalWithConfig(ctx context.Context, inv cmdutil.InvocationContext, } f.SkillContent = embeddedSkillContent runtime := &buildRuntime{Factory: f} - runtime.recovery = recovery.NewProjector(func() *surface.Plan { + runtime.recovery = recovery.NewProjectorWithContext(func() *surface.Plan { return runtime.surface - }) + }, recovery.RenderContext{Profile: inv.Profile}) f.Recovery = runtime.recovery rootCmd := &cobra.Command{ Use: "lark-cli", @@ -276,7 +277,9 @@ func buildInternalWithConfig(ctx context.Context, inv cmdutil.InvocationContext, rootCmd.AddCommand(doctor.NewCmdDoctorWithRecovery(f, runtime.recovery)) rootCmd.AddCommand(whoami.NewCmdWhoamiWithRecovery(f, runtime.recovery)) rootCmd.AddCommand(api.NewCmdApiWithContext(ctx, f, nil)) - rootCmd.AddCommand(schema.NewCmdSchema(f, nil)) + rootCmd.AddCommand(schema.NewCmdSchemaWithVisibility(f, func(path []string) bool { + return runtime.surface.CanReference(surface.CommandID(strings.Join(path, "/"))) + }, nil)) rootCmd.AddCommand(completion.NewCmdCompletion(f)) rootCmd.AddCommand(cmdupdate.NewCmdUpdate(f)) rootCmd.AddCommand(cmdevent.NewCmdEvents(f)) @@ -352,6 +355,7 @@ func buildInternalWithConfig(ctx context.Context, inv cmdutil.InvocationContext, } f.SkillContent = skillResolution.Content runtime.skillReferences = skillResolution.References + f.SkillReferences = skillResolution.References // Install hooks only on business commands. The concealment-specific help // command is attached afterwards, preserving Cobra's historical contract diff --git a/cmd/error_auth_hint.go b/cmd/error_auth_hint.go index 2d24a5c15..3d0d3f102 100644 --- a/cmd/error_auth_hint.go +++ b/cmd/error_auth_hint.go @@ -4,104 +4,33 @@ package cmd import ( - "fmt" "strings" "github.com/spf13/cobra" - "github.com/larksuite/cli/errs" "github.com/larksuite/cli/internal/apicatalog" - internalauth "github.com/larksuite/cli/internal/auth" "github.com/larksuite/cli/internal/cmdutil" "github.com/larksuite/cli/internal/core" - "github.com/larksuite/cli/internal/errclass" "github.com/larksuite/cli/internal/recovery" "github.com/larksuite/cli/internal/registry" "github.com/larksuite/cli/shortcuts" shortcutcommon "github.com/larksuite/cli/shortcuts/common" ) -// rootErrorPresenter owns the final command-facing error transformation for -// one Cobra tree. Producers report typed facts and optional semantic recovery; -// this boundary clones, completes, and projects them without exposing the -// build-local surface plan to business packages. -type rootErrorPresenter struct { - f *cmdutil.Factory - projector *recovery.Projector -} - -func newRootErrorPresenter(f *cmdutil.Factory, projector *recovery.Projector) *rootErrorPresenter { - return &rootErrorPresenter{f: f, projector: projector} -} - -func (p *rootErrorPresenter) Present(err error) error { - if err == nil || errs.IsRaw(err) { - return err +// presentRootError uses the same build-local presenter as shortcut result +// sinks, adding only the root command's lazy declared-scope resolver. +func presentRootError(f *cmdutil.Factory, err error, projector *recovery.Projector) error { + identity := core.Identity("") + if f != nil { + identity = f.ResolvedIdentity } - rendered := p.projector.Render(err) - p.completePermissionRecovery(rendered) - applyNeedAuthorizationHint(p.f, rendered) - return rendered -} - -// completePermissionRecovery supplies the canonical recovery for direct -// PermissionError producers. API classification paths that already carry an -// owned structured annotation keep their rendered Hint unchanged. -func (p *rootErrorPresenter) completePermissionRecovery(err error) { - typed, ok := errs.UnwrapTypedError(err) - if !ok { - return - } - permissionErr, ok := typed.(*errs.PermissionError) //nolint:errorlint // presentation must not descend into the clone's original Cause - if !ok || permissionErr.Hint != "" { - return - } - identity := permissionErr.Identity - if identity == "" && p.f != nil { - identity = string(p.f.ResolvedIdentity) - } - if identity == "" { - identity = string(core.AsUser) - } - hint := errclass.PermissionRecovery( - permissionErr.MissingScopes, - identity, - permissionErr.Subtype, - permissionErr.ConsoleURL, - ) - permissionErr.Hint = p.projector.RenderHint(hint) -} - -// applyNeedAuthorizationHint augments a typed *errs.AuthenticationError with a -// "current command requires scope(s): X, Y" hint when the underlying error is -// a need_user_authorization signal AND the current command declares scopes -// locally (via shortcut registration or service-method metadata). Existing -// Hint text is preserved; scopes are appended on a new line. -func applyNeedAuthorizationHint(f *cmdutil.Factory, err error) { - if err == nil || f == nil { - return - } - if !internalauth.IsNeedUserAuthorizationError(err) { - return - } - typed, ok := errs.UnwrapTypedError(err) - if !ok { - return - } - authErr, ok := typed.(*errs.AuthenticationError) //nolint:errorlint // enrich only the presented clone, never a nested producer Cause - if !ok { - return - } - scopes := resolveDeclaredScopesForCurrentCommand(f) - if len(scopes) == 0 { - return - } - scopeHint := fmt.Sprintf("current command requires scope(s): %s", strings.Join(scopes, ", ")) - if authErr.Hint == "" { - authErr.Hint = scopeHint - return - } - authErr.Hint += "\n" + scopeHint + return f.PresentError(err, cmdutil.ErrorPresentationOptions{ + Projector: projector, + Identity: identity, + DeclaredScopes: func() []string { + return resolveDeclaredScopesForCurrentCommand(f) + }, + }) } // resolveDeclaredScopesForCurrentCommand returns the scopes declared by the diff --git a/cmd/error_presenter_test.go b/cmd/error_presenter_test.go index 4d76d17e8..3dbeadee2 100644 --- a/cmd/error_presenter_test.go +++ b/cmd/error_presenter_test.go @@ -12,6 +12,7 @@ import ( internalauth "github.com/larksuite/cli/internal/auth" "github.com/larksuite/cli/internal/cmdutil" "github.com/larksuite/cli/internal/core" + "github.com/larksuite/cli/internal/errclass" "github.com/larksuite/cli/internal/recovery" "github.com/larksuite/cli/internal/registry" "github.com/larksuite/cli/internal/surface" @@ -19,17 +20,33 @@ import ( ) func TestRootErrorPresenterCompletesDirectPermissionRecoveryWithoutMutatingProducer(t *testing.T) { + cause := errors.New("permission cause") source := errs.NewPermissionError(errs.SubtypeMissingScope, "missing scope"). WithMissingScopes("docx:document"). - WithIdentity("user") + WithIdentity("user"). + WithCause(cause) - visible := newRootErrorPresenter( + visible := presentRootError( &cmdutil.Factory{ResolvedIdentity: core.AsUser}, + source, recovery.NewProjector(nil), - ).Present(source) - visibleProblem, _ := errs.ProblemOf(visible) - if !strings.Contains(visibleProblem.Hint, `auth login --scope "docx:document"`) { - t.Fatalf("visible recovery = %q, want scoped auth login", visibleProblem.Hint) + ) + visibleProblem, ok := errs.ProblemOf(visible) + if !ok { + t.Fatalf("visible error = %T, want typed error", visible) + } + if visibleProblem.Category != errs.CategoryAuthorization { + t.Errorf("visible category = %q, want %q", visibleProblem.Category, errs.CategoryAuthorization) + } + if visibleProblem.Subtype != errs.SubtypeMissingScope { + t.Errorf("visible subtype = %q, want %q", visibleProblem.Subtype, errs.SubtypeMissingScope) + } + if !errors.Is(visible, cause) { + t.Errorf("visible error lost cause %v: %v", cause, visible) + } + const wantVisible = "run `lark-cli auth login --scope \"docx:document\" --no-wait --json` to get device_code and verification_url; present verification_url to the user exactly and end this turn; after the user confirms authorization, run `lark-cli auth login --device-code ` in a later turn to finish login" + if got, want := visibleProblem.Hint, wantVisible; got != want { + t.Fatalf("visible recovery = %q, want exact split-flow recovery %q", got, want) } if source.Hint != "" { t.Fatalf("presenter mutated producer hint: %q", source.Hint) @@ -38,10 +55,11 @@ func TestRootErrorPresenterCompletesDirectPermissionRecoveryWithoutMutatingProdu plan := surface.NewPlan(map[surface.CommandID]surface.CommandState{ surface.CommandAuthLogin: surface.CommandConcealed, }) - concealed := newRootErrorPresenter( + concealed := presentRootError( &cmdutil.Factory{ResolvedIdentity: core.AsUser}, + source, recovery.NewProjector(func() *surface.Plan { return plan }), - ).Present(source) + ) concealedProblem, _ := errs.ProblemOf(concealed) if strings.Contains(concealedProblem.Hint, "auth login") || !strings.Contains(concealedProblem.Hint, "supported authorization flow") { @@ -49,19 +67,233 @@ func TestRootErrorPresenterCompletesDirectPermissionRecoveryWithoutMutatingProdu } } -func TestRootErrorPresenterDoesNotRecommendUserLoginForBotPermission(t *testing.T) { - source := errs.NewPermissionError(errs.SubtypeMissingScope, "missing scope"). - WithMissingScopes("drive:file:download"). - WithIdentity("bot") +func TestRootErrorPresenterUsesDeclaredScopesForCanonicalPermissionRecovery(t *testing.T) { + const declaredScope = "calendar:calendar.event:read" - rendered := newRootErrorPresenter( - &cmdutil.Factory{ResolvedIdentity: core.AsBot}, - recovery.NewProjector(nil), - ).Present(source) - problem, _ := errs.ProblemOf(rendered) - if strings.Contains(problem.Hint, "auth login") || - !strings.Contains(problem.Hint, "app developer") { - t.Fatalf("bot recovery = %q", problem.Hint) + f := &cmdutil.Factory{ResolvedIdentity: core.AsUser} + root := &cobra.Command{Use: "lark-cli"} + calendar := &cobra.Command{Use: "calendar"} + agenda := &cobra.Command{Use: "+agenda"} + root.AddCommand(calendar) + calendar.AddCommand(agenda) + f.CurrentCommand = agenda + + newSource := func(t *testing.T) (error, *errs.PermissionError) { + t.Helper() + err := errclass.BuildAPIError( + map[string]any{"code": 230027, "msg": "operation unauthorized"}, + errclass.ClassifyContext{Identity: "user"}, + ) + typed, ok := errs.UnwrapTypedError(err) + if !ok { + t.Fatalf("source = %T, want typed error", err) + } + permission, ok := typed.(*errs.PermissionError) + if !ok { + t.Fatalf("source = %T, want *errs.PermissionError", err) + } + if len(permission.MissingScopes) != 0 || !strings.Contains(permission.Hint, "--recommend") { + t.Fatalf("source = %+v, want canonical generic recovery without server scope facts", permission) + } + return err, permission + } + + source, sourcePermission := newSource(t) + sourceHint := sourcePermission.Hint + visible := presentRootError(f, source, recovery.NewProjector(nil)) + presented, ok := visible.(*errs.PermissionError) + if !ok { + t.Fatalf("visible = %T, want *errs.PermissionError", visible) + } + wantVisible := errclass.PermissionRecovery( + []string{declaredScope}, + "user", + errs.SubtypeUserUnauthorized, + "", + ).String() + if presented.Hint != wantVisible { + t.Fatalf("visible recovery = %q, want declared-scope recovery %q", presented.Hint, wantVisible) + } + if len(presented.MissingScopes) != 0 { + t.Fatalf("presentation fabricated missing_scopes: %v", presented.MissingScopes) + } + if sourcePermission.Hint != sourceHint || len(sourcePermission.MissingScopes) != 0 { + t.Fatalf("presenter mutated producer: %+v", sourcePermission) + } + + const serverScope = "calendar:calendar.event:read:server" + serverSource := errclass.BuildAPIError( + map[string]any{ + "code": 99991679, + "msg": "missing scope", + "error": map[string]any{ + "permission_violations": []any{map[string]any{"subject": serverScope}}, + }, + }, + errclass.ClassifyContext{Identity: "user"}, + ) + var serverProducer *errs.PermissionError + if !errors.As(serverSource, &serverProducer) { + t.Fatalf("server source = %T, want *errs.PermissionError", serverSource) + } + serverPresentedError := presentRootError(f, serverSource, recovery.NewProjector(nil)) + serverPresented, ok := serverPresentedError.(*errs.PermissionError) + if !ok { + t.Fatalf("server presented = %T, want *errs.PermissionError", serverPresentedError) + } + wantServer := errclass.PermissionRecovery( + []string{serverScope}, + "user", + errs.SubtypeMissingScope, + "", + ).String() + if serverPresented.Hint != wantServer { + t.Fatalf("server recovery = %q, want authoritative server scope %q", serverPresented.Hint, wantServer) + } + if len(serverPresented.MissingScopes) != 1 || serverPresented.MissingScopes[0] != serverScope { + t.Fatalf("presented missing_scopes = %v, want [%s]", serverPresented.MissingScopes, serverScope) + } + if len(serverProducer.MissingScopes) != 1 || serverProducer.MissingScopes[0] != serverScope { + t.Fatalf("presenter mutated server producer: %+v", serverProducer) + } + + plan := surface.NewPlan(map[surface.CommandID]surface.CommandState{ + surface.CommandAuthLogin: surface.CommandConcealed, + }) + concealedSource, _ := newSource(t) + concealed := presentRootError(f, concealedSource, recovery.NewProjector(func() *surface.Plan { return plan })) + concealedPermission, ok := concealed.(*errs.PermissionError) + if !ok { + t.Fatalf("concealed = %T, want *errs.PermissionError", concealed) + } + wantConcealed := errclass.PermissionRecovery( + []string{declaredScope}, + "user", + errs.SubtypeUserUnauthorized, + "", + ).Render(plan) + if concealedPermission.Hint != wantConcealed { + t.Fatalf("concealed recovery = %q, want declared-scope fallback %q", concealedPermission.Hint, wantConcealed) + } + if strings.Contains(concealedPermission.Hint, "auth login") || !strings.Contains(concealedPermission.Hint, declaredScope) { + t.Fatalf("concealed recovery leaked a command or lost scope context: %q", concealedPermission.Hint) + } + + custom := errs.NewPermissionError(errs.SubtypeUserUnauthorized, "permission denied"). + WithIdentity("user"). + WithHint("ask the tenant admin to review the resource policy") + customPresented := presentRootError(f, custom, recovery.NewProjector(nil)) + customProblem, _ := errs.ProblemOf(customPresented) + if got, want := customProblem.Hint, custom.Hint; got != want { + t.Fatalf("custom recovery = %q, want producer guidance %q", got, want) + } +} + +func TestRootErrorPresenterPreservesPermissionGuidanceWhenAuthLoginIsConcealed(t *testing.T) { + const authorizationFallback = "obtain or refresh a user credential through this distribution's supported authorization flow, have the user complete authorization, then retry\ncurrent command requires scope(s): im:message" + tests := []struct { + name string + subtype errs.Subtype + wantHint string + }{ + { + name: "token scope insufficient", + subtype: errs.SubtypeTokenScopeInsufficient, + wantHint: "check the token's granted scopes; " + authorizationFallback, + }, + { + name: "user unauthorized", + subtype: errs.SubtypeUserUnauthorized, + wantHint: authorizationFallback + "; if re-auth does not help, the operation may be blocked by external-chat or admin policy", + }, + } + + plan := surface.NewPlan(map[surface.CommandID]surface.CommandState{ + surface.CommandAuthLogin: surface.CommandConcealed, + }) + projector := recovery.NewProjector(func() *surface.Plan { return plan }) + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + cause := errors.New("permission cause") + source := errs.NewPermissionError(tt.subtype, "permission denied"). + WithMissingScopes("im:message"). + WithIdentity("user"). + WithCause(cause) + + rendered := presentRootError( + &cmdutil.Factory{ResolvedIdentity: core.AsUser}, + source, + projector, + ) + presented, ok := rendered.(*errs.PermissionError) + if !ok { + t.Fatalf("rendered error = %T, want *errs.PermissionError", rendered) + } + problem, ok := errs.ProblemOf(rendered) + if !ok { + t.Fatalf("ProblemOf(%T) failed: %v", rendered, rendered) + } + if problem.Category != errs.CategoryAuthorization || problem.Subtype != tt.subtype { + t.Errorf("problem = %s/%s, want authorization/%s", problem.Category, problem.Subtype, tt.subtype) + } + if got := presented.Hint; got != tt.wantHint { + t.Fatalf("concealed recovery = %q, want exact joined recovery %q", got, tt.wantHint) + } + if strings.Contains(presented.Hint, "auth login") { + t.Fatalf("concealed recovery leaks unavailable auth login target: %q", presented.Hint) + } + if presented.Message != source.Message || presented.Identity != "user" || + len(presented.MissingScopes) != 1 || presented.MissingScopes[0] != "im:message" { + t.Fatalf("presented machine fields = %+v, source = %+v", presented, source) + } + if !errors.Is(rendered, cause) { + t.Fatalf("rendered error lost cause %v: %v", cause, rendered) + } + if source.Hint != "" { + t.Fatalf("presenter mutated producer hint: %q", source.Hint) + } + }) + } +} + +func TestRootErrorPresenterDoesNotRecommendUserLoginForBotPermission(t *testing.T) { + tests := []struct { + subtype errs.Subtype + want string + }{ + {subtype: errs.SubtypeMissingScope, want: "app developer"}, + {subtype: errs.SubtypeTokenScopeInsufficient, want: "token's granted scopes"}, + {subtype: errs.SubtypeUserUnauthorized, want: "required bot permissions"}, + {subtype: errs.SubtypePermissionDenied, want: "this bot"}, + } + for _, tt := range tests { + t.Run(string(tt.subtype), func(t *testing.T) { + source := errs.NewPermissionError(tt.subtype, "bot permission failure"). + WithMissingScopes("drive:file:download"). + WithIdentity("bot") + + rendered := presentRootError( + &cmdutil.Factory{ResolvedIdentity: core.AsBot}, + source, + recovery.NewProjector(nil), + ) + problem, ok := errs.ProblemOf(rendered) + if !ok { + t.Fatalf("rendered error = %T, want typed permission error", rendered) + } + for _, forbidden := range []string{"auth login", "verification_url", "device_code", "user credential"} { + if strings.Contains(strings.ToLower(problem.Hint), forbidden) { + t.Errorf("bot recovery %q contains user OAuth guidance %q", problem.Hint, forbidden) + } + } + if !strings.Contains(problem.Hint, tt.want) { + t.Errorf("bot recovery = %q, want guidance containing %q", problem.Hint, tt.want) + } + if source.Hint != "" || source.Identity != "bot" || len(source.MissingScopes) != 1 { + t.Errorf("presenter mutated producer: %+v", source) + } + }) } } @@ -73,10 +305,11 @@ func TestRootErrorPresenterDoesNotMutateNestedPermissionCause(t *testing.T) { WithHint("retry the operation"). WithCause(inner) - rendered := newRootErrorPresenter( + rendered := presentRootError( &cmdutil.Factory{ResolvedIdentity: core.AsUser}, + outer, recovery.NewProjector(nil), - ).Present(outer) + ) if inner.Hint != "" { t.Fatalf("presenter mutated nested producer hint: %q", inner.Hint) @@ -99,7 +332,7 @@ func TestRootErrorPresenterDoesNotMutateNestedAuthenticationCause(t *testing.T) WithHint("retry the operation"). WithCause(source) - rendered := newRootErrorPresenter(f, recovery.NewProjector(nil)).Present(outer) + rendered := presentRootError(f, outer, recovery.NewProjector(nil)) if got := inner.Hint; got != originalHint { t.Fatalf("presenter mutated nested authentication hint: got %q want %q", got, originalHint) diff --git a/cmd/presentation_test.go b/cmd/presentation_test.go index eced5a083..9cdc2bd58 100644 --- a/cmd/presentation_test.go +++ b/cmd/presentation_test.go @@ -20,6 +20,7 @@ import ( "github.com/larksuite/cli/internal/core" "github.com/larksuite/cli/internal/deprecation" "github.com/larksuite/cli/internal/output" + "github.com/larksuite/cli/internal/recovery" "github.com/larksuite/cli/internal/skillscheck" "github.com/larksuite/cli/internal/surface" "github.com/larksuite/cli/internal/update" @@ -438,6 +439,58 @@ func TestRecoveryRenderingUsesExactBuildLocalSurfaceAndDoesNotMutate(t *testing. } } +func TestRecoveryRenderingKeepsExplicitProfilesIsolatedAcrossBuilds(t *testing.T) { + tmpHome(t) + platform.ResetForTesting() + t.Cleanup(platform.ResetForTesting) + + alphaInvocation := buildInvocationForTest(t) + alphaInvocation.Profile = "alpha" + alphaRuntime, _, _ := buildInternal(context.Background(), alphaInvocation, WithoutPlugins()) + + betaInvocation := buildInvocationForTest(t) + betaInvocation.Profile = "beta" + betaRuntime, _, _ := buildInternal(context.Background(), betaInvocation, WithoutPlugins()) + + source := recovery.Attach( + errs.NewPermissionError(errs.SubtypeMissingScope, "missing scope"). + WithMissingScopes("docx:document"). + WithIdentity("user"), + recovery.UserAuthorization("docx:document"), + ) + sourceProblem, _ := errs.ProblemOf(source) + if strings.Contains(sourceProblem.Hint, "--profile") { + t.Fatalf("producer hint unexpectedly owns an invocation profile: %q", sourceProblem.Hint) + } + + assertProfile := func(name string, runtime *buildRuntime, want, unwanted string) { + t.Helper() + rendered := runtime.recovery.Render(source) + problem, ok := errs.ProblemOf(rendered) + if !ok { + t.Fatalf("%s rendered error = %T, want typed error", name, rendered) + } + for _, command := range []string{ + "lark-cli auth login --profile='" + want + "' --scope \"docx:document\" --no-wait --json", + "lark-cli auth login --profile='" + want + "' --device-code ", + } { + if !strings.Contains(problem.Hint, command) { + t.Errorf("%s recovery missing %q: %q", name, command, problem.Hint) + } + } + if strings.Contains(problem.Hint, "--profile='"+unwanted+"'") { + t.Errorf("%s recovery leaked profile %q: %q", name, unwanted, problem.Hint) + } + } + + assertProfile("alpha before beta", alphaRuntime, "alpha", "beta") + assertProfile("beta", betaRuntime, "beta", "alpha") + assertProfile("alpha after beta", alphaRuntime, "alpha", "beta") + if strings.Contains(sourceProblem.Hint, "--profile") { + t.Fatalf("build-local rendering mutated source hint: %q", sourceProblem.Hint) + } +} + func TestConcurrentBuildsKeepIndependentSurfacePlans(t *testing.T) { tmpHome(t) registerRestriction(t, []string{"config/init"}, nil) diff --git a/cmd/root.go b/cmd/root.go index d50c7d2ec..6e3cdcfe6 100644 --- a/cmd/root.go +++ b/cmd/root.go @@ -281,7 +281,7 @@ func handleRootError( // dynamic enrichment operate on a concrete clone, never the producer's // reusable error value. if !errs.IsRaw(err) { - renderedErr = newRootErrorPresenter(f, projector).Present(err) + renderedErr = presentRootError(f, err, projector) } // Staged dispatch: capture the typed exit code BEFORE attempting the diff --git a/cmd/root_test.go b/cmd/root_test.go index 246590ddc..096c404de 100644 --- a/cmd/root_test.go +++ b/cmd/root_test.go @@ -6,6 +6,7 @@ package cmd import ( "bytes" "encoding/json" + "errors" "fmt" "io" "strings" @@ -23,7 +24,9 @@ import ( "github.com/larksuite/cli/internal/core" "github.com/larksuite/cli/internal/deprecation" "github.com/larksuite/cli/internal/output" + "github.com/larksuite/cli/internal/recovery" "github.com/larksuite/cli/internal/registry" + "github.com/larksuite/cli/internal/surface" ) // TestPersistentPreRunE_AuthCheckDisabledAnnotations verifies that @@ -506,8 +509,8 @@ func TestHandleRootError_TypedAuthErrorWithLegacyCausePreserved(t *testing.T) { } // TestApplyNeedAuthorizationHint_ServiceMethodUsesLocalScopesWhenNoUAT pins -// that a typed AuthenticationError carrying the need_user_authorization marker gets a -// declared-scopes Hint appended when the current command is a registered +// that a typed AuthenticationError carrying the need_user_authorization marker +// gets executable scoped recovery when the current command is a registered // service method. func TestApplyNeedAuthorizationHint_ServiceMethodUsesLocalScopesWhenNoUAT(t *testing.T) { t.Setenv("LARKSUITE_CLI_CONFIG_DIR", t.TempDir()) @@ -541,17 +544,53 @@ func TestApplyNeedAuthorizationHint_ServiceMethodUsesLocalScopesWhenNoUAT(t *tes resourceCmd.AddCommand(methodCmd) f.CurrentCommand = methodCmd - authErr := newAuthErrorWithNeedAuthMarker() - applyNeedAuthorizationHint(f, authErr) + source := internalauth.NewNeedUserAuthorizationError("u_service") + var authErr *errs.AuthenticationError + if !errors.As(source, &authErr) { + t.Fatalf("source = %T, want *errs.AuthenticationError", source) + } + originalHint := authErr.Hint + rendered := presentRootError(f, source, recovery.NewProjector(nil)) + problem, ok := errs.ProblemOf(rendered) + if !ok { + t.Fatalf("rendered error = %T, want typed error", rendered) + } - if authErr.Category != errs.CategoryAuthentication { - t.Errorf("Category = %q, want authentication", authErr.Category) + if problem.Category != errs.CategoryAuthentication { + t.Errorf("Category = %q, want authentication", problem.Category) } - if !strings.Contains(authErr.Message, "need_user_authorization") { - t.Errorf("Message should preserve need_user_authorization marker; got %q", authErr.Message) + if problem.Subtype != errs.SubtypeTokenMissing { + t.Errorf("Subtype = %q, want %q", problem.Subtype, errs.SubtypeTokenMissing) } - if !strings.Contains(authErr.Hint, "current command requires scope(s): calendar:calendar.event:create") { - t.Errorf("expected declared-scope hint, got %q", authErr.Hint) + if !errors.Is(rendered, authErr.Cause) { + t.Errorf("rendered error lost need-authorization cause %v: %v", authErr.Cause, rendered) + } + if !strings.Contains(problem.Message, "need_user_authorization") { + t.Errorf("Message should preserve need_user_authorization marker; got %q", problem.Message) + } + if !strings.Contains(problem.Hint, `auth login --scope "calendar:calendar.event:create" --no-wait --json`) { + t.Errorf("expected scoped two-turn recovery, got %q", problem.Hint) + } + if authErr.Hint != originalHint || !strings.Contains(authErr.Hint, "--recommend --no-wait --json") { + t.Errorf("presenter mutated producer's generic recovery: before %q, after %q", originalHint, authErr.Hint) + } + + concealedPlan := surface.NewPlan(map[surface.CommandID]surface.CommandState{ + surface.CommandAuthLogin: surface.CommandConcealed, + }) + concealed := presentRootError(f, internalauth.NewNeedUserAuthorizationError("u_service"), recovery.NewProjector(func() *surface.Plan { + return concealedPlan + })) + concealedProblem, ok := errs.ProblemOf(concealed) + if !ok { + t.Fatalf("concealed rendered error = %T, want typed error", concealed) + } + wantFallback := recovery.UserAuthorization("calendar:calendar.event:create").Render(concealedPlan) + if concealedProblem.Hint != wantFallback { + t.Errorf("concealed recovery = %q, want fallback %q", concealedProblem.Hint, wantFallback) + } + if strings.Contains(concealedProblem.Hint, "auth login") { + t.Errorf("concealed recovery leaked auth command: %q", concealedProblem.Hint) } } @@ -573,10 +612,20 @@ func TestApplyNeedAuthorizationHint_ShortcutUsesDeclaredScopesWhenNoUAT(t *testi f.CurrentCommand = shortcutCmd authErr := newAuthErrorWithNeedAuthMarker() - applyNeedAuthorizationHint(f, authErr) + rendered := presentRootError(f, authErr, recovery.NewProjector(nil)) + problem, ok := errs.ProblemOf(rendered) + if !ok { + t.Fatalf("rendered error = %T, want typed error", rendered) + } + if problem.Category != errs.CategoryAuthentication { + t.Errorf("Category = %q, want %q", problem.Category, errs.CategoryAuthentication) + } + if problem.Subtype != errs.SubtypeUnknown { + t.Errorf("Subtype = %q, want %q", problem.Subtype, errs.SubtypeUnknown) + } - if !strings.Contains(authErr.Hint, "current command requires scope(s): docx:document:create") { - t.Errorf("expected shortcut scope hint, got %q", authErr.Hint) + if !strings.Contains(problem.Hint, `auth login --scope "docx:document:create" --no-wait --json`) { + t.Errorf("expected shortcut scoped recovery, got %q", problem.Hint) } } @@ -598,15 +647,25 @@ func TestApplyNeedAuthorizationHint_ShortcutIncludesConditionalScopes(t *testing f.CurrentCommand = shortcutCmd authErr := newAuthErrorWithNeedAuthMarker() - applyNeedAuthorizationHint(f, authErr) + rendered := presentRootError(f, authErr, recovery.NewProjector(nil)) + problem, ok := errs.ProblemOf(rendered) + if !ok { + t.Fatalf("rendered error = %T, want typed error", rendered) + } + if problem.Category != errs.CategoryAuthentication { + t.Errorf("Category = %q, want %q", problem.Category, errs.CategoryAuthentication) + } + if problem.Subtype != errs.SubtypeUnknown { + t.Errorf("Subtype = %q, want %q", problem.Subtype, errs.SubtypeUnknown) + } - if !strings.Contains(authErr.Hint, "current command requires scope(s): drive:drive.metadata:readonly, drive:file:download") { - t.Errorf("expected conditional scope hint for drive +status, got %q", authErr.Hint) + if !strings.Contains(problem.Hint, `auth login --scope "drive:drive.metadata:readonly drive:file:download" --no-wait --json`) { + t.Errorf("expected conditional scoped recovery for drive +status, got %q", problem.Hint) } } // TestApplyNeedAuthorizationHint_AppendsExistingHint pins that the -// declared-scopes guidance is appended (separated by newline) when the typed +// declared-scope recovery is appended (separated by newline) when the typed // AuthenticationError already carries a Hint from elsewhere. func TestApplyNeedAuthorizationHint_AppendsExistingHint(t *testing.T) { t.Setenv("LARKSUITE_CLI_CONFIG_DIR", t.TempDir()) @@ -625,10 +684,26 @@ func TestApplyNeedAuthorizationHint_AppendsExistingHint(t *testing.T) { authErr := newAuthErrorWithNeedAuthMarker() authErr.Hint = "existing hint" - applyNeedAuthorizationHint(f, authErr) + rendered := presentRootError(f, authErr, recovery.NewProjector(nil)) + problem, ok := errs.ProblemOf(rendered) + if !ok { + t.Fatalf("rendered error = %T, want typed error", rendered) + } + if problem.Category != errs.CategoryAuthentication { + t.Errorf("Category = %q, want %q", problem.Category, errs.CategoryAuthentication) + } + if problem.Subtype != errs.SubtypeUnknown { + t.Errorf("Subtype = %q, want %q", problem.Subtype, errs.SubtypeUnknown) + } + if !errors.Is(rendered, authErr.Cause) { + t.Errorf("rendered error lost need-authorization cause %v: %v", authErr.Cause, rendered) + } - want := "existing hint\ncurrent command requires scope(s): docx:document:create" - if authErr.Hint != want { - t.Errorf("expected appended hint %q, got %q", want, authErr.Hint) + want := "existing hint\n" + recovery.UserAuthorization("docx:document:create").String() + if problem.Hint != want { + t.Errorf("expected appended hint %q, got %q", want, problem.Hint) + } + if authErr.Hint != "existing hint" { + t.Errorf("presenter mutated producer hint: %q", authErr.Hint) } } diff --git a/cmd/schema/schema.go b/cmd/schema/schema.go index 3b27967e0..b4ead42b3 100644 --- a/cmd/schema/schema.go +++ b/cmd/schema/schema.go @@ -13,12 +13,24 @@ import ( "github.com/larksuite/cli/internal/apicatalog" "github.com/larksuite/cli/internal/cmdutil" "github.com/larksuite/cli/internal/core" + "github.com/larksuite/cli/internal/meta" "github.com/larksuite/cli/internal/output" "github.com/larksuite/cli/internal/registry" "github.com/larksuite/cli/internal/schema" "github.com/spf13/cobra" ) +// CommandVisibility reports whether one canonical generated-command path is +// referenceable in the current build. Paths use the same segments as +// apicatalog.MethodRef.CommandPath (for example +// ["mail", "user_mailbox.messages", "list"]). A nil visibility keeps the +// complete schema catalog. +// +// The callback is deliberately command-facing rather than policy-facing: +// cmd/schema only consumes the final build-local presentation surface and does +// not know why a command is or is not referenceable. +type CommandVisibility func(path []string) bool + // SchemaOptions holds all inputs for the schema command. type SchemaOptions struct { Factory *cmdutil.Factory @@ -30,8 +42,20 @@ type SchemaOptions struct { Args []string } -// NewCmdSchema creates the schema command. If runF is non-nil it is called instead of schemaRun (test hook). +// NewCmdSchema creates the schema command. If runF is non-nil it is called instead of the default runner (test hook). func NewCmdSchema(f *cmdutil.Factory, runF func(*SchemaOptions) error) *cobra.Command { + return NewCmdSchemaWithVisibility(f, nil, runF) +} + +// NewCmdSchemaWithVisibility creates the schema command projected through one +// build-local command surface. Existing callers should use NewCmdSchema; the +// root builder uses this form so schema execution and completion share the +// exact presentation plan captured by that Cobra tree. +func NewCmdSchemaWithVisibility( + f *cmdutil.Factory, + visibility CommandVisibility, + runF func(*SchemaOptions) error, +) *cobra.Command { opts := &SchemaOptions{Factory: f} cmd := &cobra.Command{ @@ -44,7 +68,7 @@ func NewCmdSchema(f *cmdutil.Factory, runF func(*SchemaOptions) error) *cobra.Co if runF != nil { return runF(opts) } - return schemaRun(opts) + return schemaRunWithVisibility(opts, visibility) }, } cmdutil.DisableAuthCheck(cmd) @@ -59,7 +83,7 @@ func NewCmdSchema(f *cmdutil.Factory, runF func(*SchemaOptions) error) *cobra.Co _ = cmd.Flags().MarkHidden("json") _ = cmd.Flags().MarkHidden("as") - cmd.ValidArgsFunction = completeSchemaPath(f) + cmd.ValidArgsFunction = completeSchemaPath(f, visibility) cmdutil.SetRisk(cmd, cmdutil.RiskRead) return cmd @@ -68,10 +92,14 @@ func NewCmdSchema(f *cmdutil.Factory, runF func(*SchemaOptions) error) *cobra.Co // completeSchemaPath is a thin adapter over the schema catalog's Complete. // It uses the same source as schema execution so completion candidates match // what `schema` can resolve. -func completeSchemaPath(f *cmdutil.Factory) func(*cobra.Command, []string, string) ([]string, cobra.ShellCompDirective) { +func completeSchemaPath( + f *cmdutil.Factory, + visibility CommandVisibility, +) func(*cobra.Command, []string, string) ([]string, cobra.ShellCompDirective) { return func(cmd *cobra.Command, args []string, toComplete string) ([]string, cobra.ShellCompDirective) { mode := f.ResolveStrictMode(cmd.Context()) - completions, noSpace := registry.SchemaCatalog().Complete(args, toComplete, registry.FilterForStrictMode(mode)) + catalog := projectSchemaCatalog(registry.SchemaCatalog(), visibility) + completions, noSpace := catalog.Complete(args, toComplete, registry.FilterForStrictMode(mode)) directive := cobra.ShellCompDirectiveNoFileComp if noSpace { directive |= cobra.ShellCompDirectiveNoSpace @@ -80,25 +108,44 @@ func completeSchemaPath(f *cmdutil.Factory) func(*cobra.Command, []string, strin } } -func schemaRun(opts *SchemaOptions) error { +func schemaRunWithVisibility(opts *SchemaOptions, visibility CommandVisibility) error { out := opts.Factory.IOStreams.Out mode := opts.Factory.ResolveStrictMode(opts.Ctx) - return runSchema(out, apicatalog.ParsePath(opts.Args), mode) + return runSchemaWithVisibility(out, apicatalog.ParsePath(opts.Args), mode, visibility) } -// runSchema resolves the path through the schema catalog and renders the +// runSchemaWithVisibility resolves the path through the schema catalog and renders the // matching envelope(s). The catalog owns navigation (Resolve + MethodRefs) and // schema owns rendering (Envelope/Envelopes); this adapter only chooses the // output shape — a single resolved method renders as one envelope object, // anything broader as an array — and maps resolve failures to hints. -func runSchema(out io.Writer, parts []string, mode core.StrictMode) error { - catalog := registry.SchemaCatalog() +func runSchemaWithVisibility( + out io.Writer, + parts []string, + mode core.StrictMode, + visibility CommandVisibility, +) error { + return runSchemaCatalog(out, parts, mode, registry.SchemaCatalog(), visibility) +} + +func runSchemaCatalog( + out io.Writer, + parts []string, + mode core.StrictMode, + catalog apicatalog.Catalog, + visibility CommandVisibility, +) error { + // Test the source catalog before presentation projection. A distribution + // that intentionally conceals every generated method still has metadata; + // bare `schema` should render an empty list rather than claim metadata is + // unavailable. if len(catalog.Services()) == 0 { // No embedded metadata and the runtime fallback is empty too: offline // with a cold cache, remote meta off, or an unwritable cache dir. return errs.NewValidationError(errs.SubtypeFailedPrecondition, "No API metadata available"). WithHint("this binary has no embedded API metadata; run any command with network access to the open platform once so metadata can be fetched and cached") } + catalog = projectSchemaCatalog(catalog, visibility) target, err := catalog.Resolve(parts) if err != nil { return resolveError(err) @@ -117,6 +164,112 @@ func runSchema(out io.Writer, parts []string, mode core.StrictMode) error { return nil } +// projectSchemaCatalog produces the metadata view corresponding to one final +// command surface. It lives in cmd/schema so apicatalog remains a policy-free +// navigation module. Resolve, broad listings, and Complete all consume the +// same projected Catalog, which also prevents resolve-error candidate hints +// from naming concealed resources or methods. +// +// Unchanged branches retain their original maps. A parent is removed when +// projection removed its last reachable method, so a fully concealed service +// cannot survive as an empty schema namespace. Originally-empty, unaffected +// metadata remains unchanged for backward compatibility. +func projectSchemaCatalog(catalog apicatalog.Catalog, visibility CommandVisibility) apicatalog.Catalog { + if visibility == nil { + return catalog + } + + services := make([]meta.Service, 0, len(catalog.Services())) + changed := false + for _, service := range catalog.Services() { + servicePath := []string{service.Name} + if !visibility(servicePath) { + changed = true + continue + } + + resources, resourceChanged, hasVisibleMethod := projectSchemaResources( + service.Resources, + servicePath, + visibility, + ) + if resourceChanged && !hasVisibleMethod { + changed = true + continue + } + if resourceChanged { + service.Resources = resources + changed = true + } + services = append(services, service) + } + if !changed { + return catalog + } + return apicatalog.New(catalog.Source(), services) +} + +func projectSchemaResources( + resources map[string]meta.Resource, + parentPath []string, + visibility CommandVisibility, +) (projected map[string]meta.Resource, changed, hasVisibleMethod bool) { + projected = make(map[string]meta.Resource, len(resources)) + for name, resource := range resources { + resourcePath := appendPath(parentPath, name) + if !visibility(resourcePath) { + changed = true + continue + } + + methods := make(map[string]meta.Method, len(resource.Methods)) + resourceChanged := false + resourceHasVisibleMethod := false + for methodName, method := range resource.Methods { + if !visibility(appendPath(resourcePath, methodName)) { + resourceChanged = true + continue + } + methods[methodName] = method + resourceHasVisibleMethod = true + } + + subResources, subChanged, subHasVisibleMethod := projectSchemaResources( + resource.Resources, + resourcePath, + visibility, + ) + resourceChanged = resourceChanged || subChanged + resourceHasVisibleMethod = resourceHasVisibleMethod || subHasVisibleMethod + + if resourceChanged && !resourceHasVisibleMethod { + // Projection removed the final method below this resource. Keeping + // the empty group would still reveal a concealed schema namespace. + changed = true + continue + } + if resourceChanged { + resource.Methods = methods + resource.Resources = subResources + changed = true + } + projected[name] = resource + hasVisibleMethod = hasVisibleMethod || resourceHasVisibleMethod + } + + if !changed { + return resources, false, hasVisibleMethod + } + return projected, true, hasVisibleMethod +} + +func appendPath(parent []string, segment string) []string { + path := make([]string, len(parent)+1) + copy(path, parent) + path[len(parent)] = segment + return path +} + // resolveError maps a catalog *ResolveError to a typed *errs.ValidationError // (CategoryValidation drives the exit code; Hint promotes to the envelope), // preserving the historical message + hint text. diff --git a/cmd/schema/schema_test.go b/cmd/schema/schema_test.go index 7e1762c8e..653b92c5b 100644 --- a/cmd/schema/schema_test.go +++ b/cmd/schema/schema_test.go @@ -4,14 +4,18 @@ package schema import ( + "bytes" "encoding/json" "errors" + "reflect" "strings" "testing" "github.com/larksuite/cli/errs" + "github.com/larksuite/cli/internal/apicatalog" "github.com/larksuite/cli/internal/cmdutil" "github.com/larksuite/cli/internal/core" + "github.com/larksuite/cli/internal/meta" ) func TestSchemaCmd_FlagParsing(t *testing.T) { @@ -252,6 +256,157 @@ func TestSchemaCmd_UnknownMethod_TypedValidation(t *testing.T) { } } -// Completion candidate generation (dotted + space forms, strict-mode filtering, -// dotted-resource handling) now lives in internal/apicatalog and is covered by -// apicatalog's TestComplete. cmd/schema only adapts catalog.Complete to cobra. +// Base completion navigation (dotted + space forms, strict-mode filtering, +// dotted-resource handling) lives in internal/apicatalog. The tests below pin +// cmd/schema's build-local surface projection around that navigator. + +func TestSchemaSurfaceProjectionFiltersExecutionListingAndCompletion(t *testing.T) { + catalog := schemaSurfaceCatalog() + visible := func(path []string) bool { + return strings.Join(path, "/") != "mail/user_mailbox.messages/list" + } + + var out bytes.Buffer + if err := runSchemaCatalog(&out, nil, core.StrictModeOff, catalog, visible); err != nil { + t.Fatalf("broad schema failed: %v", err) + } + var envelopes []map[string]interface{} + if err := json.Unmarshal(out.Bytes(), &envelopes); err != nil { + t.Fatalf("broad schema output is not JSON: %v\n%s", err, out.String()) + } + names := make(map[string]bool, len(envelopes)) + for _, envelope := range envelopes { + name, _ := envelope["name"].(string) + names[name] = true + } + if names["mail user_mailbox.messages list"] { + t.Error("broad schema retained concealed mail messages list") + } + for _, want := range []string{"mail user_mailbox.messages get", "im messages list"} { + if !names[want] { + t.Errorf("broad schema lost visible method %q: %v", want, names) + } + } + + out.Reset() + err := runSchemaCatalog( + &out, + []string{"mail", "user_mailbox", "messages", "list"}, + core.StrictModeOff, + catalog, + visible, + ) + if err == nil { + t.Fatal("concealed exact method unexpectedly resolved") + } + var validationErr *errs.ValidationError + if !errors.As(err, &validationErr) || validationErr.Subtype != errs.SubtypeInvalidArgument { + t.Fatalf("concealed exact method error = %T %v, want validation/invalid_argument", err, err) + } + if strings.Contains(validationErr.Hint, "list") || !strings.Contains(validationErr.Hint, "get") { + t.Errorf("resolve candidates were not surface-projected: %q", validationErr.Hint) + } + if out.Len() != 0 { + t.Errorf("concealed exact method wrote schema output: %s", out.String()) + } + + projected := projectSchemaCatalog(catalog, visible) + if got, _ := projected.Complete(nil, "mail.user_mailbox.messages.l", nil); len(got) != 0 { + t.Errorf("dotted completion exposed concealed method: %v", got) + } + if got, _ := projected.Complete(nil, "mail.user_mailbox.messages.g", nil); !reflect.DeepEqual(got, []string{"mail.user_mailbox.messages.get"}) { + t.Errorf("dotted completion lost visible sibling: %v", got) + } + if got, _ := projected.Complete([]string{"mail", "user_mailbox", "messages"}, "l", nil); len(got) != 0 { + t.Errorf("space completion exposed concealed method: %v", got) + } + if got, _ := projected.Complete([]string{"mail", "user_mailbox", "messages"}, "g", nil); !reflect.DeepEqual(got, []string{"get"}) { + t.Errorf("space completion lost visible sibling: %v", got) + } +} + +func TestSchemaSurfaceProjectionDropsServiceWhenGlobConcealsAllDescendants(t *testing.T) { + catalog := schemaSurfaceCatalog() + // Mirrors a policy that retains the top-level schema command and mail group + // but conceals mail/**. + visible := func(path []string) bool { + return !strings.HasPrefix(strings.Join(path, "/"), "mail/") + } + projected := projectSchemaCatalog(catalog, visible) + + if _, ok := projected.Service("mail"); ok { + t.Fatal("mail survived as an empty schema namespace after mail/** was concealed") + } + if _, ok := projected.Service("im"); !ok { + t.Fatal("unrelated visible service im was removed") + } + if got, _ := projected.Complete(nil, "ma", nil); len(got) != 0 { + t.Errorf("root dotted completion exposed concealed mail service: %v", got) + } + if got, _ := projected.Complete(nil, "im.m", nil); !reflect.DeepEqual(got, []string{"im.messages."}) { + t.Errorf("root dotted completion lost visible im service: %v", got) + } + + _, err := projected.Resolve([]string{"mail", "messages", "get"}) + var resolveErr *apicatalog.ResolveError + if !errors.As(err, &resolveErr) || resolveErr.Kind != apicatalog.ErrService { + t.Fatalf("concealed mail resolve error = %T %v, want unknown service", err, err) + } + if strings.Contains(strings.Join(resolveErr.Candidates, ","), "mail") { + t.Errorf("unknown-service candidates exposed concealed mail: %v", resolveErr.Candidates) + } +} + +func TestSchemaSurfaceProjectionPreservesDefaultAndDeniedVisibleCatalog(t *testing.T) { + catalog := schemaSurfaceCatalog() + allVisible := func([]string) bool { return true } + + var defaultOut, projectedOut bytes.Buffer + if err := runSchemaCatalog(&defaultOut, nil, core.StrictModeOff, catalog, nil); err != nil { + t.Fatalf("default schema failed: %v", err) + } + if err := runSchemaCatalog(&projectedOut, nil, core.StrictModeOff, catalog, allVisible); err != nil { + t.Fatalf("all-visible schema failed: %v", err) + } + if defaultOut.String() != projectedOut.String() { + t.Errorf("all-referenceable surface changed default schema output\ndefault: %s\nprojected: %s", defaultOut.String(), projectedOut.String()) + } +} + +func schemaSurfaceCatalog() apicatalog.Catalog { + service := func(name string, methods map[string]interface{}) meta.Service { + resourceName := "messages" + if name == "mail" { + resourceName = "user_mailbox.messages" + } + return meta.ServiceFromMap(map[string]interface{}{ + "name": name, + "version": "v1", + "servicePath": "/open-apis/" + name + "/v1", + "resources": map[string]interface{}{ + resourceName: map[string]interface{}{ + "methods": methods, + }, + }, + }) + } + method := func(id, description string) map[string]interface{} { + return map[string]interface{}{ + "id": id, + "path": "/open-apis/fixture/v1/messages", + "httpMethod": "GET", + "description": description, + "risk": "read", + "accessTokens": []interface{}{"tenant"}, + } + } + return apicatalog.New(apicatalog.SourceEmbedded, []meta.Service{ + service("mail", map[string]interface{}{ + "get": method("mail.user_mailbox.messages.get", "visible mail method"), + "list": method("mail.user_mailbox.messages.list", "concealable mail method"), + }), + service("im", map[string]interface{}{ + "list": method("im.messages.list", "visible im method"), + }), + }) +} diff --git a/cmd/skill_customization_test.go b/cmd/skill_customization_test.go index 379ae4f40..3bc5c90ba 100644 --- a/cmd/skill_customization_test.go +++ b/cmd/skill_customization_test.go @@ -13,6 +13,7 @@ import ( "github.com/larksuite/cli/errs" "github.com/larksuite/cli/extension/platform" "github.com/larksuite/cli/internal/skillcontent" + "github.com/larksuite/cli/internal/skillpolicy" ) // withBaseSkills swaps the process-global embedded skill tree for the @@ -168,6 +169,51 @@ func TestBuildInternal_invalidSkillsOverlayGuard(t *testing.T) { } } +// Invalid host skill metadata is classified at the command boundary while +// preserving the internal sentinel for callers that inspect the cause chain. +func TestBuildInternal_invalidHostBaseGuard(t *testing.T) { + tmpHome(t) + platform.ResetForTesting() + t.Cleanup(platform.ResetForTesting) + + withBaseSkills(t, map[string]string{ + "lark-a/SKILL.md": "---\nmetadata:\n requires:\n skills: [\"../escape\"]\n---\n", + }) + platform.Register(platform.NewPlugin("acme", "1.0"). + EmbeddedSkills(&platform.SkillsOverlay{Allow: []string{"lark-a"}}).MustBuild()) + + _, root, _ := buildInternal(context.Background(), buildInvocationForTest(t)) + leaf := findRunnableLeaf(root) + if leaf == nil { + t.Fatal("no runnable leaf in command tree") + } + err := leaf.RunE(leaf, nil) + problem, ok := errs.ProblemOf(err) + if !ok { + t.Fatalf("ProblemOf(%T) failed: %v", err, err) + } + if problem.Category != errs.CategoryValidation || problem.Subtype != errs.SubtypeFailedPrecondition { + t.Errorf("problem = %s/%s, want validation/failed_precondition", problem.Category, problem.Subtype) + } + var verr *errs.ValidationError + if !errors.As(err, &verr) { + t.Fatalf("expected *errs.ValidationError, got %T %+v", err, err) + } + if verr.Param != "" { + t.Errorf("param = %q, want empty because embedded host content is not a user argument", verr.Param) + } + if !errors.Is(err, skillpolicy.ErrInvalidHostBase) { + t.Fatalf("error does not preserve ErrInvalidHostBase: %v", err) + } + const wantHint = "the wrapper's embedded base skill tree is invalid; fix the content passed to cmd.SetEmbeddedSkillContent (reason_code invalid_skills_overlay)" + if verr.Hint != wantHint { + t.Errorf("hint = %q, want host-base-specific recovery %q", verr.Hint, wantHint) + } + if strings.Contains(verr.Hint, "fix the plugin's EmbeddedSkills") { + t.Errorf("hint misattributes invalid host content to the plugin: %q", verr.Hint) + } +} + // A wrapper main that forgets to wire its embedded skill base should get the // missing host assembly step, not the same recovery hint as a misspelled // Allow/Remove name. diff --git a/errs/ERROR_CONTRACT.md b/errs/ERROR_CONTRACT.md index d1ad2c5e6..76066b0bb 100644 --- a/errs/ERROR_CONTRACT.md +++ b/errs/ERROR_CONTRACT.md @@ -44,10 +44,9 @@ Typed errors render to **stderr** as one JSON object per process exit: "subtype": "missing_scope", "code": 99991679, "message": "missing scope `calendar:event:create` for app cli_xxx", - "hint": "run lark-cli auth login --scope calendar:event:create", + "hint": "run `lark-cli auth login --scope \"calendar:event:create\" --no-wait --json` to get device_code and verification_url; present verification_url to the user exactly and end this turn; after the user confirms authorization, run `lark-cli auth login --device-code ` in a later turn to finish login", "log_id": "20260520-0a1b2c3d", - "missing_scopes": ["calendar:event:create"], - "console_url": "https://open.feishu.cn/app/cli_xxx/auth?q=..." + "missing_scopes": ["calendar:event:create"] } } ``` @@ -65,7 +64,7 @@ Typed errors render to **stderr** as one JSON object per process exit: | `error.retryable` | wire-stable | `true` when present; omitted when `false` | | `error.param` | per-Subtype-stable | single offending parameter (`ValidationError`); see **Validation parameters** | | `error.params` | per-Subtype-stable | per-parameter validation detail array (`ValidationError`); see **Validation parameters** | -| per-Subtype extension fields | per-Subtype-stable | e.g. `missing_scopes`, `console_url`, `challenge_url` | +| per-Subtype extension fields | per-Subtype-stable | e.g. `missing_scopes`, `console_url`, `challenge_url`; `console_url` is emitted for developer/admin recovery such as `app_scope_not_applied`, not user `missing_scope` | `SecurityPolicyError` renders through the same typed envelope as every other category. `error.type` is `"policy"`, `error.subtype` is one of @@ -146,6 +145,41 @@ argument validation): the latter are classified into a typed validation envelope (`invalid_argument`) and exit `2`, matching the explicit flag and subcommand guards. +### Concealed commands (`validation/command_unavailable`) + +`command_unavailable` is emitted only by a distribution that explicitly opts +into presenting plugin-restricted commands as absent. Direct invocation and +`help ` both produce a typed validation envelope and exit `2`: + +```json +{ + "ok": false, + "error": { + "type": "validation", + "subtype": "command_unavailable", + "message": "requested capability is not available in this CLI distribution" + } +} +``` + +For consumers, this subtype means the capability is not part of the current +binary's usable command surface. Do not treat it as an authentication failure, +attempt to bypass local policy, or infer that installing credentials will make +the command available. The distribution may customize `message`; branch only +on `type` and `subtype`. + +The concealed wire shape deliberately omits `param`, `policy_source`, +`rule_name`, and `reason_code`, so it does not disclose the plugin policy that +removed the capability. An in-process Go caller may still observe the original +denial through the error cause for auditing. + +This opt-in behavior does not change the other command-resolution contracts: + +- an ordinary unknown command remains `validation/invalid_argument`; +- a restricted command in the legacy visible presentation remains + `validation/failed_precondition` with its policy diagnostics; and +- the default CLI build does not emit `command_unavailable`. + ### Predicate commands (`output.BareError`) A small class of commands is **predicates**: they answer a yes/no diff --git a/extension/platform/README.md b/extension/platform/README.md index 0e0434273..68856fcc7 100644 --- a/extension/platform/README.md +++ b/extension/platform/README.md @@ -316,7 +316,9 @@ These codes remain available to in-process hosts through the wrapped rule and shipped-tree summary. Agents consuming a host that explicitly enabled concealment should match `error.type == "validation"` and `error.subtype == "command_unavailable"` instead of branching on a -rule-specific reason. +rule-specific reason. The canonical +[`validation/command_unavailable` contract](../../errs/ERROR_CONTRACT.md#concealed-commands-validationcommand_unavailable) +defines its exit code, wire fields, and consumer behavior. ## Where to go next diff --git a/extension/platform/skillsoverlay.go b/extension/platform/skillsoverlay.go index 7cdd08123..aa466397a 100644 --- a/extension/platform/skillsoverlay.go +++ b/extension/platform/skillsoverlay.go @@ -29,6 +29,11 @@ import "io/fs" // the CLI builds. Later additions or removals of top-level directories do // not change the manifest; files within an owned skill directory are read // live. Base and Overlay must contain only valid skill directories. +// A skill may declare hard dependencies under +// metadata.requires.skills in SKILL.md. Every declared dependency must be +// present in the final composed manifest; Allow is never widened and Remove +// is never overridden to satisfy one. A same-named Overlay replacement uses +// the replacement SKILL.md's dependency metadata, not the base copy's. // Declaring this asset composition is a build-integrity commitment: invalid // selection, content, ownership, or reference remaps abort the build rather // than silently falling back to host defaults. diff --git a/internal/cmdutil/error_presenter.go b/internal/cmdutil/error_presenter.go new file mode 100644 index 000000000..49873b55a --- /dev/null +++ b/internal/cmdutil/error_presenter.go @@ -0,0 +1,126 @@ +// Copyright (c) 2026 Lark Technologies Pte. Ltd. +// SPDX-License-Identifier: MIT + +package cmdutil + +import ( + "github.com/larksuite/cli/errs" + internalauth "github.com/larksuite/cli/internal/auth" + "github.com/larksuite/cli/internal/core" + "github.com/larksuite/cli/internal/errclass" + "github.com/larksuite/cli/internal/recovery" +) + +// ErrorPresentationOptions supplies invocation-specific facts to PresentError. +// DeclaredScopes is lazy because only user-authorization recovery with no +// server-reported scope facts needs command metadata resolution. +type ErrorPresentationOptions struct { + Projector *recovery.Projector + Identity core.Identity + DeclaredScopes func() []string +} + +// PresentError clones and projects a typed producer error before command-facing +// fields are copied into either a root error envelope or a result payload. The +// producer is never mutated, and all machine-readable fields are preserved. +func (f *Factory) PresentError(err error, options ErrorPresentationOptions) error { + if err == nil || errs.IsRaw(err) { + return err + } + + projector := options.Projector + if projector == nil && f != nil { + projector = f.Recovery + } + rendered := projector.Render(err) + completePermissionRecovery(f, rendered, projector, options.Identity, options.DeclaredScopes) + applyNeedAuthorizationHint(rendered, projector, options.DeclaredScopes) + return rendered +} + +func completePermissionRecovery( + f *Factory, + err error, + projector *recovery.Projector, + identity core.Identity, + declaredScopes func() []string, +) { + typed, ok := errs.UnwrapTypedError(err) + if !ok { + return + } + permissionErr, ok := typed.(*errs.PermissionError) //nolint:errorlint // presentation must not descend into the clone's original Cause + if !ok { + return + } + if permissionErr.Identity != "" { + identity = core.Identity(permissionErr.Identity) + } else if identity == "" && f != nil { + identity = f.ResolvedIdentity + } + if identity == "" { + identity = core.AsUser + } + canonical := errclass.PermissionRecovery( + permissionErr.MissingScopes, + string(identity), + permissionErr.Subtype, + permissionErr.ConsoleURL, + ) + if permissionErr.Hint != "" && + permissionErr.Hint != canonical.String() && + permissionErr.Hint != projector.RenderHint(canonical) { + return + } + + recoveryScopes := permissionErr.MissingScopes + if permissionRecoveryUsesDeclaredScopes(permissionErr, identity) && declaredScopes != nil { + if scopes := declaredScopes(); len(scopes) > 0 { + recoveryScopes = scopes + } + } + hint := errclass.PermissionRecovery( + recoveryScopes, + string(identity), + permissionErr.Subtype, + permissionErr.ConsoleURL, + ) + permissionErr.Hint = projector.RenderHint(hint) +} + +func permissionRecoveryUsesDeclaredScopes(permissionErr *errs.PermissionError, identity core.Identity) bool { + if permissionErr == nil || identity != core.AsUser || len(permissionErr.MissingScopes) > 0 { + return false + } + switch permissionErr.Subtype { + case errs.SubtypeMissingScope, errs.SubtypeTokenScopeInsufficient, errs.SubtypeUserUnauthorized: + return true + default: + return false + } +} + +func applyNeedAuthorizationHint(err error, projector *recovery.Projector, declaredScopes func() []string) { + if err == nil || declaredScopes == nil || !internalauth.IsNeedUserAuthorizationError(err) { + return + } + typed, ok := errs.UnwrapTypedError(err) + if !ok { + return + } + authErr, ok := typed.(*errs.AuthenticationError) //nolint:errorlint // enrich only the presented clone, never a nested producer Cause + if !ok { + return + } + scopes := declaredScopes() + if len(scopes) == 0 { + return + } + scopedRecovery := projector.RenderHint(recovery.UserAuthorization(scopes...)) + genericRecovery := projector.RenderHint(recovery.UserAuthorization()) + if authErr.Hint == "" || authErr.Hint == genericRecovery { + authErr.Hint = scopedRecovery + return + } + authErr.Hint += "\n" + scopedRecovery +} diff --git a/internal/cmdutil/error_presenter_test.go b/internal/cmdutil/error_presenter_test.go new file mode 100644 index 000000000..29a7f8f61 --- /dev/null +++ b/internal/cmdutil/error_presenter_test.go @@ -0,0 +1,105 @@ +// Copyright (c) 2026 Lark Technologies Pte. Ltd. +// SPDX-License-Identifier: MIT + +package cmdutil + +import ( + "errors" + "strings" + "testing" + + "github.com/larksuite/cli/errs" + "github.com/larksuite/cli/internal/core" + "github.com/larksuite/cli/internal/errclass" + "github.com/larksuite/cli/internal/recovery" + "github.com/larksuite/cli/internal/surface" +) + +func TestFactoryPresentErrorClonesAndPreservesPermissionMachineFields(t *testing.T) { + cause := errors.New("permission cause") + source := errs.NewPermissionError(errs.SubtypeMissingScope, "missing scope"). + WithCode(99991679). + WithLogID("log-123"). + WithMissingScopes("docx:document"). + WithRequestedScopes("docx:document", "drive:drive"). + WithGrantedScopes("drive:drive"). + WithIdentity("user"). + WithCause(cause) + plan := surface.NewPlan(map[surface.CommandID]surface.CommandState{ + surface.CommandAuthLogin: surface.CommandConcealed, + }) + f := &Factory{ + ResolvedIdentity: core.AsUser, + Recovery: recovery.NewProjector(func() *surface.Plan { + return plan + }), + } + + rendered := f.PresentError(source, ErrorPresentationOptions{}) + presented, ok := rendered.(*errs.PermissionError) + if !ok { + t.Fatalf("PresentError() = %T, want *errs.PermissionError", rendered) + } + if presented == source { + t.Fatal("PresentError returned the producer instead of a clone") + } + if !errors.Is(rendered, cause) { + t.Fatalf("PresentError did not preserve cause %v: %v", cause, rendered) + } + problem, ok := errs.ProblemOf(rendered) + if !ok { + t.Fatalf("PresentError() = %T, want typed problem", rendered) + } + if problem.Category != errs.CategoryAuthorization || problem.Subtype != errs.SubtypeMissingScope { + t.Fatalf("problem = %s/%s, want authorization/missing_scope", problem.Category, problem.Subtype) + } + if presented.Code != source.Code || presented.LogID != source.LogID || + presented.Identity != source.Identity || presented.Subtype != source.Subtype { + t.Fatalf("presented machine fields = %+v, source = %+v", presented, source) + } + if strings.Join(presented.MissingScopes, ",") != strings.Join(source.MissingScopes, ",") || + strings.Join(presented.RequestedScopes, ",") != strings.Join(source.RequestedScopes, ",") || + strings.Join(presented.GrantedScopes, ",") != strings.Join(source.GrantedScopes, ",") { + t.Fatalf("presented scope fields = %+v, source = %+v", presented, source) + } + if strings.Contains(presented.Hint, "auth login") || + !strings.Contains(presented.Hint, "supported authorization flow") { + t.Fatalf("presented concealed hint = %q", presented.Hint) + } + if source.Hint != "" { + t.Fatalf("PresentError mutated producer hint: %q", source.Hint) + } +} + +func TestFactoryPresentErrorRebuildsUnannotatedCanonicalHintWithInvocationContext(t *testing.T) { + canonical := errclass.PermissionHint(nil, "user", errs.SubtypeMissingScope, "") + source := errs.NewPermissionError(errs.SubtypeMissingScope, "missing scope"). + WithIdentity("user"). + WithHint("%s", canonical) + projector := recovery.NewProjectorWithContext(nil, recovery.RenderContext{Profile: "team-beta"}) + f := &Factory{ResolvedIdentity: core.AsUser, Recovery: projector} + + rendered := f.PresentError(source, ErrorPresentationOptions{ + DeclaredScopes: func() []string { return []string{"calendar:calendar.event:read"} }, + }) + presented, ok := rendered.(*errs.PermissionError) + if !ok { + t.Fatalf("PresentError() = %T, want *errs.PermissionError", rendered) + } + for _, want := range []string{ + `--profile='team-beta'`, + `--scope "calendar:calendar.event:read"`, + "--no-wait --json", + "--device-code", + } { + if !strings.Contains(presented.Hint, want) { + t.Fatalf("presented hint %q does not contain %q", presented.Hint, want) + } + } + if strings.Contains(presented.Hint, "--recommend") { + t.Fatalf("presented hint retained generic recovery: %q", presented.Hint) + } + if source.Hint != canonical { + t.Fatalf("PresentError mutated producer hint: got %q, want %q", source.Hint, canonical) + } +} diff --git a/internal/cmdutil/factory.go b/internal/cmdutil/factory.go index f5ef1b6ed..49eb556ca 100644 --- a/internal/cmdutil/factory.go +++ b/internal/cmdutil/factory.go @@ -22,6 +22,7 @@ import ( "github.com/larksuite/cli/internal/credential" "github.com/larksuite/cli/internal/keychain" "github.com/larksuite/cli/internal/recovery" + "github.com/larksuite/cli/internal/skillref" "github.com/larksuite/cli/internal/transport" ) @@ -48,8 +49,9 @@ type Factory struct { FileIOProvider fileio.Provider // file transfer provider (default: local filesystem) - SkillContent fs.FS // embedded skill tree (rooted at the skill list); nil when the build embeds no skills - Recovery *recovery.Projector // build-local recovery presentation; nil means the default fully-visible surface + SkillContent fs.FS // embedded skill tree (rooted at the skill list); nil when the build embeds no skills + SkillReferences *skillref.Resolver // build-local projection from canonical skill references to embedded content + Recovery *recovery.Projector // build-local recovery presentation; nil means the default fully-visible surface } // RenderRecoveryHint renders semantic recovery against this command tree. @@ -62,6 +64,27 @@ func (f *Factory) RenderRecoveryHint(hint recovery.Hint) string { return f.Recovery.RenderHint(hint) } +// ResolveSkillReference projects a canonical skills-read reference into this +// build's embedded skill tree. Concealed skills-read surfaces never expose a +// reference, even when the underlying content remains embedded. +func (f *Factory) ResolveSkillReference(canonical string) (string, bool) { + if f == nil || !f.Recovery.CanReference(recovery.TargetSkillsRead) { + return "", false + } + if f.SkillReferences != nil { + return f.SkillReferences.ResolveString(canonical) + } + + ref, err := skillref.Parse(canonical) + if err != nil || f.SkillContent == nil { + return "", false + } + if _, err := fs.Stat(f.SkillContent, ref.StatPath()); err != nil { + return "", false + } + return canonical, true +} + // ExternalHTTPClient returns a clone of the existing Factory client whose // requests are explicitly classified as external. The underlying client, // redirect policy, timeout, proxy configuration, and legacy transport provider diff --git a/internal/cmdutil/factory_test.go b/internal/cmdutil/factory_test.go index 97eed8097..dfb5dae12 100644 --- a/internal/cmdutil/factory_test.go +++ b/internal/cmdutil/factory_test.go @@ -8,6 +8,7 @@ import ( "errors" "strings" "testing" + "testing/fstest" "github.com/spf13/cobra" @@ -17,8 +18,64 @@ import ( "github.com/larksuite/cli/internal/credential" "github.com/larksuite/cli/internal/envvars" "github.com/larksuite/cli/internal/output" + "github.com/larksuite/cli/internal/recovery" + "github.com/larksuite/cli/internal/skillref" + "github.com/larksuite/cli/internal/surface" ) +func TestFactoryResolveSkillReference(t *testing.T) { + content := fstest.MapFS{ + "lark-doc/SKILL.md": {Data: []byte("canonical")}, + "acme-doc/SKILL.md": {Data: []byte("remapped")}, + } + from, err := skillref.Parse("lark-doc") + if err != nil { + t.Fatal(err) + } + to, err := skillref.Parse("acme-doc") + if err != nil { + t.Fatal(err) + } + resolver, err := skillref.New(content, []skillref.Mapping{{From: from, To: to}}) + if err != nil { + t.Fatal(err) + } + + t.Run("explicit remap", func(t *testing.T) { + f := &Factory{SkillContent: content, SkillReferences: resolver} + if got, ok := f.ResolveSkillReference("lark-doc"); !ok || got != "acme-doc" { + t.Fatalf("ResolveSkillReference() = %q, %v; want acme-doc, true", got, ok) + } + }) + + t.Run("identity fallback", func(t *testing.T) { + f := &Factory{SkillContent: content} + if got, ok := f.ResolveSkillReference("lark-doc"); !ok || got != "lark-doc" { + t.Fatalf("ResolveSkillReference() = %q, %v; want lark-doc, true", got, ok) + } + if got, ok := f.ResolveSkillReference("missing"); ok || got != "" { + t.Fatalf("missing ResolveSkillReference() = %q, %v; want empty, false", got, ok) + } + if got, ok := f.ResolveSkillReference("../invalid"); ok || got != "" { + t.Fatalf("invalid ResolveSkillReference() = %q, %v; want empty, false", got, ok) + } + }) + + t.Run("concealed skills read", func(t *testing.T) { + plan := surface.NewPlan(map[surface.CommandID]surface.CommandState{ + surface.CommandSkillsRead: surface.CommandConcealed, + }) + f := &Factory{ + SkillContent: content, + SkillReferences: resolver, + Recovery: recovery.NewProjector(func() *surface.Plan { return plan }), + } + if got, ok := f.ResolveSkillReference("lark-doc"); ok || got != "" { + t.Fatalf("concealed ResolveSkillReference() = %q, %v; want empty, false", got, ok) + } + }) +} + // newCmdWithAsFlag creates a cobra.Command with a --as string flag for testing. func newCmdWithAsFlag(asValue string, changed bool) *cobra.Command { cmd := &cobra.Command{Use: "test"} diff --git a/internal/errclass/classify.go b/internal/errclass/classify.go index 68f140706..4418d756e 100644 --- a/internal/errclass/classify.go +++ b/internal/errclass/classify.go @@ -318,7 +318,7 @@ func buildPermissionErrorFromFacts(p errs.Problem, missing []string, cc Classify identity = "user" } consoleURL := ConsoleURL(cc.Brand, cc.AppID, missing) - p.Message = CanonicalPermissionMessage(p.Subtype, cc.AppID, missing, p.Message) + p.Message = canonicalPermissionMessageForIdentity(p.Subtype, identity, cc.AppID, missing, p.Message) // Permission categories have authoritative recovery guidance (scopes to // grant, console URL), so the curated PermissionHint deliberately overrides // any server detail lifted into p.Hint (the opposite precedence from the @@ -331,22 +331,23 @@ func buildPermissionErrorFromFacts(p errs.Problem, missing []string, cc Classify Identity: identity, } // ConsoleURL is the developer-console deep-link an app developer follows to - // apply for a missing scope. That action only resolves SubtypeAppScopeNotApplied, - // which is bot-perspective. The other authorization subtypes route to a - // different actor: SubtypeMissingScope / SubtypeTokenScopeInsufficient / - // SubtypeUserUnauthorized recover via `lark-cli auth login`; SubtypeAppUnavailable - // / SubtypeAppDisabled require tenant admin. Carrying ConsoleURL on those - // envelopes is dead weight and risks pointing an end user at a console they - // cannot modify; the URL is still computed so the hint composer can use it - // where appropriate. + // apply for a missing scope. The typed machine field is attached only to + // SubtypeAppScopeNotApplied, whose primary recovery is developer action. + // Other authorization subtypes route according to identity: user calls may + // recover through `lark-cli auth login`, while bot calls must stay on + // token/app/bot/admin recovery paths. For bot scope failures the URL may + // still be embedded in the human hint, but carrying it as a machine field on + // every permission envelope risks pointing an end user at a console they + // cannot modify. SubtypeAppUnavailable / SubtypeAppDisabled require tenant + // admin. if p.Subtype == errs.SubtypeAppScopeNotApplied { permErr.ConsoleURL = consoleURL } return recovery.Annotate(permErr, hint) } -// CanonicalPermissionMessage returns the CLI-side canonical wording for a -// typed PermissionError, preserving the Lark official-API phrasing +// CanonicalPermissionMessage returns the default user-facing CLI wording for +// a typed PermissionError, preserving the Lark official-API phrasing // ("access denied" / "unauthorized" / "token has no permission") and // enhancing it with CLI context (app ID, missing scope list). Subtypes // outside the known set fall through to fallback so the upstream message @@ -390,14 +391,37 @@ func CanonicalPermissionMessage(subtype errs.Subtype, appID string, missing []st return fallback } +// canonicalPermissionMessageForIdentity removes user-specific claims from +// bot errors while preserving CanonicalPermissionMessage as the default user +// contract. Category, subtype, identity, and other machine fields are +// unaffected; error.message is informational by contract. +func canonicalPermissionMessageForIdentity(subtype errs.Subtype, identity, appID string, missing []string, fallback string) string { + message := CanonicalPermissionMessage(subtype, appID, missing, fallback) + if identity != "bot" { + return message + } + switch subtype { + case errs.SubtypeMissingScope: + if len(missing) > 0 { + return fmt.Sprintf("unauthorized: bot identity does not have the required scope(s): %s", strings.Join(missing, ", ")) + } + return "unauthorized: bot identity does not have the required scope" + case errs.SubtypeUserUnauthorized: + return "access denied for this bot operation" + case errs.SubtypePermissionDenied: + return "bot lacks permission for the requested resource" + default: + return message + } +} + // PermissionHint returns the canonical per-subtype recovery hint for a typed // PermissionError. The hint distinguishes authorization subtypes routing // to different recovery paths: developer console for app_scope_not_applied, -// user re-login for missing_scope / token_scope_insufficient / user_unauthorized, -// and tenant admin for app_unavailable / app_disabled. The subtype -// argument is the primary discriminator; identity is retained for the -// generic permission_denied fallback so callers that do not yet route on -// subtype still get a sensible hint. +// identity-specific user or bot recovery for missing_scope / +// token_scope_insufficient / user_unauthorized, and tenant admin for +// app_unavailable / app_disabled. The subtype and identity together select +// the actor that can resolve the failure. // // Exported so direct construction sites (cmd/service/service.go's // checkServiceScopes) can produce hints that match the dispatcher path @@ -414,6 +438,15 @@ func PermissionRecovery(missing []string, identity string, subtype errs.Subtype, return permissionRecoveryHint(missing, identity, subtype, consoleURL) } +func botScopeRecoveryHint(consoleURL string) recovery.Hint { + if consoleURL != "" { + return recovery.Join("", recovery.Text(fmt.Sprintf( + "the app developer must verify and grant the required scope(s) for the bot identity at the developer console: %s", consoleURL))) + } + return recovery.Join("", recovery.Text( + "the app developer must verify and grant the required scope(s) to the bot identity")) +} + func permissionRecoveryHint(missing []string, identity string, subtype errs.Subtype, consoleURL string) recovery.Hint { switch subtype { case errs.SubtypeAppScopeNotApplied: @@ -425,25 +458,27 @@ func permissionRecoveryHint(missing []string, identity string, subtype errs.Subt "the app developer must apply for the required scope(s) at the developer console")) case errs.SubtypeMissingScope: if identity == "bot" { - if consoleURL != "" { - return recovery.Join("", recovery.Text(fmt.Sprintf( - "the app developer must apply for the required scope(s) at the developer console: %s", consoleURL))) - } - return recovery.Join("", recovery.Text( - "the app developer must grant the required scope(s) to the bot identity")) + return botScopeRecoveryHint(consoleURL) } return recovery.UserAuthorization(missing...) case errs.SubtypeTokenScopeInsufficient: - return recovery.Join("; ", - recovery.Text("check the token's granted scopes"), - recovery.Command(recovery.TargetAuthLogin, - "run `lark-cli auth login` to refresh if the scope was added after the token was issued"), + tokenCheck := recovery.Join("", recovery.Text("check the token's granted scopes")) + if identity == "bot" { + return recovery.JoinHints("; ", tokenCheck, botScopeRecoveryHint(consoleURL)) + } + return recovery.JoinHints("; ", + tokenCheck, + recovery.UserAuthorization(missing...), ) case errs.SubtypeUserUnauthorized: - return recovery.Join("; ", - recovery.Command(recovery.TargetAuthLogin, - "run `lark-cli auth login` to re-authorize this user"), - recovery.Text("if re-auth does not help, the operation may be blocked by external-chat or admin policy"), + if identity == "bot" { + return recovery.Join("", recovery.Text( + "check that the app has the required bot permissions, is installed and available to the target tenant, and the bot can access the target resource; if those checks pass, ask the tenant admin to inspect app and resource policy restrictions")) + } + return recovery.JoinHints("; ", + recovery.UserAuthorization(missing...), + recovery.Join("", recovery.Text( + "if re-auth does not help, the operation may be blocked by external-chat or admin policy")), ) case errs.SubtypeAppUnavailable: return recovery.Join("", recovery.Text( diff --git a/internal/errclass/classify_test.go b/internal/errclass/classify_test.go index 8cb5506c9..496c54d8f 100644 --- a/internal/errclass/classify_test.go +++ b/internal/errclass/classify_test.go @@ -769,6 +769,135 @@ func TestBuildPermissionHint_AppMissingScopeRoutesToConsole(t *testing.T) { } } +// TestBuildAPIError_BotPermissionRecoveryFamily pins the complete dispatcher +// contract for bot callers across the three permission codes that can otherwise +// be mistaken for user OAuth failures. Recovery guidance must name only actors +// that can fix a bot call; user-login commands would send an agent through an +// irrelevant device-authorization round trip without repairing the bot call. +func TestBuildAPIError_BotPermissionRecoveryFamily(t *testing.T) { + const ( + appID = "cli_bot" + scope = "im:message" + ) + cases := []struct { + name string + code int + wantSubtype errs.Subtype + wantMessage string + wantMissing []string + wantHintParts []string + }{ + { + name: "99991679 missing_scope", + code: 99991679, + wantSubtype: errs.SubtypeMissingScope, + wantMessage: "unauthorized: bot identity does not have the required scope(s): " + scope, + wantMissing: []string{scope}, + wantHintParts: []string{"app developer", "developer console"}, + }, + { + name: "99991676 token_scope_insufficient", + code: 99991676, + wantSubtype: errs.SubtypeTokenScopeInsufficient, + wantMessage: "token has no permission for this operation; required scope is missing", + wantMissing: []string{scope}, + wantHintParts: []string{"token's granted scopes", "app developer", "developer console"}, + }, + { + name: "230027 user_unauthorized", + code: 230027, + wantSubtype: errs.SubtypeUserUnauthorized, + wantMessage: "access denied for this bot operation", + wantHintParts: []string{"required bot permissions", "target tenant", "target resource", "tenant admin", "policy restrictions"}, + }, + { + name: "1470403 permission_denied", + code: 1470403, + wantSubtype: errs.SubtypePermissionDenied, + wantMessage: "bot lacks permission for the requested resource", + wantHintParts: []string{"resource owner", "this bot"}, + }, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + resp := map[string]any{ + "code": tc.code, + "msg": "upstream permission failure", + } + if len(tc.wantMissing) > 0 { + resp["error"] = map[string]any{ + "permission_violations": []any{map[string]any{"subject": scope}}, + } + } + + err := errclass.BuildAPIError(resp, errclass.ClassifyContext{ + Brand: "feishu", + AppID: appID, + Identity: "bot", + }) + problem, ok := errs.ProblemOf(err) + if !ok { + t.Fatalf("ProblemOf returned !ok, err = %T", err) + } + if problem.Category != errs.CategoryAuthorization { + t.Errorf("Category = %q, want %q", problem.Category, errs.CategoryAuthorization) + } + if problem.Subtype != tc.wantSubtype { + t.Errorf("Subtype = %q, want %q", problem.Subtype, tc.wantSubtype) + } + if problem.Code != tc.code { + t.Errorf("Code = %d, want %d", problem.Code, tc.code) + } + if problem.Message != tc.wantMessage { + t.Errorf("Message = %q, want %q", problem.Message, tc.wantMessage) + } + + permission := requirePermissionError(t, err) + if permission.Identity != "bot" { + t.Errorf("Identity = %q, want bot", permission.Identity) + } + if len(permission.MissingScopes) != len(tc.wantMissing) { + t.Fatalf("MissingScopes = %v, want %v", permission.MissingScopes, tc.wantMissing) + } + for i := range tc.wantMissing { + if permission.MissingScopes[i] != tc.wantMissing[i] { + t.Errorf("MissingScopes = %v, want %v", permission.MissingScopes, tc.wantMissing) + } + } + if permission.ConsoleURL != "" { + t.Errorf("ConsoleURL = %q, want empty machine field for subtype %q", permission.ConsoleURL, tc.wantSubtype) + } + + hint := strings.ToLower(problem.Hint) + message := strings.ToLower(problem.Message) + for _, part := range tc.wantHintParts { + if !strings.Contains(hint, strings.ToLower(part)) { + t.Errorf("Hint %q missing bot recovery guidance %q", problem.Hint, part) + } + } + for _, forbidden := range []string{ + "auth login", + "--no-wait", + "verification_url", + "device_code", + "authorize or refresh", + "current user", + "re-authorize this user", + "user credential", + "target chat", + "external chats", + } { + if strings.Contains(hint, forbidden) { + t.Errorf("bot Hint %q must not contain user OAuth guidance %q", problem.Hint, forbidden) + } + if strings.Contains(message, forbidden) { + t.Errorf("bot Message %q must not contain user-specific guidance %q", problem.Message, forbidden) + } + } + }) + } +} + // TestBuildPermissionError_CanonicalMessage pins the per-subtype canonical // wording so the wire envelope's Message preserves Lark's official phrasing // ("access denied" / "unauthorized" / "token has no permission") and enhances diff --git a/internal/errclass/hint_gate_test.go b/internal/errclass/hint_gate_test.go index b447e972e..ca3662855 100644 --- a/internal/errclass/hint_gate_test.go +++ b/internal/errclass/hint_gate_test.go @@ -20,13 +20,28 @@ func TestPermissionHint_usesBuildLocalSurface(t *testing.T) { plan := surface.NewPlan(map[surface.CommandID]surface.CommandState{ surface.CommandAuthLogin: surface.CommandConcealed, }) + cause := errors.New("permission cause") for _, st := range []errs.Subtype{errs.SubtypeMissingScope, errs.SubtypeTokenScopeInsufficient, errs.SubtypeUserUnauthorized} { hint := PermissionHint([]string{"im:message"}, "user", st, "") sourceTyped := errs.NewPermissionError(st, "permission denied"). - WithHint("%s", hint) + WithHint("%s", hint). + WithCause(cause) source := recovery.Attach(sourceTyped, permissionRecoveryHint([]string{"im:message"}, "user", st, "")) rendered := recovery.Render(source, plan) + problem, ok := errs.ProblemOf(rendered) + if !ok { + t.Fatalf("%s: rendered error = %T, want typed error", st, rendered) + } + if problem.Category != errs.CategoryAuthorization { + t.Errorf("%s: rendered category = %q, want %q", st, problem.Category, errs.CategoryAuthorization) + } + if problem.Subtype != st { + t.Errorf("%s: rendered subtype = %q, want %q", st, problem.Subtype, st) + } + if !errors.Is(rendered, cause) { + t.Errorf("%s: rendered error lost cause %v: %v", st, cause, rendered) + } var concealed *errs.PermissionError if !errors.As(rendered, &concealed) { t.Fatalf("%s: rendered error = %T, want *errs.PermissionError", st, rendered) @@ -43,9 +58,29 @@ func TestPermissionHint_usesBuildLocalSurface(t *testing.T) { var visible *errs.PermissionError if !errors.As(recovery.Render(source, nil), &visible) || !strings.Contains(visible.Hint, "auth login") { t.Errorf("%s: visible render must keep auth login, got %+v", st, visible) + } else { + for _, want := range []string{ + "--no-wait --json", + "verification_url", + "auth login --device-code ", + "in a later turn", + } { + if !strings.Contains(visible.Hint, want) { + t.Errorf("%s: OAuth recovery missing %q: %q", st, want, visible.Hint) + } + } } } + tokenHint := PermissionHint([]string{"im:message"}, "user", errs.SubtypeTokenScopeInsufficient, "") + if !strings.Contains(tokenHint, "check the token's granted scopes") { + t.Errorf("token-scope recovery lost token policy guidance: %q", tokenHint) + } + userHint := PermissionHint([]string{"im:message"}, "user", errs.SubtypeUserUnauthorized, "") + if !strings.Contains(userHint, "external-chat or admin policy") { + t.Errorf("user-unauthorized recovery lost external policy guidance: %q", userHint) + } + // Non-command recovery guidance is retained under the same plan. consoleHint := PermissionHint(nil, "bot", errs.SubtypeAppScopeNotApplied, "https://example.com") consoleErr := errs.NewPermissionError(errs.SubtypeAppScopeNotApplied, "permission denied"). diff --git a/internal/recovery/context.go b/internal/recovery/context.go new file mode 100644 index 000000000..6afa3de75 --- /dev/null +++ b/internal/recovery/context.go @@ -0,0 +1,54 @@ +// Copyright (c) 2026 Lark Technologies Pte. Ltd. +// SPDX-License-Identifier: MIT + +package recovery + +import "strings" + +// RenderContext carries invocation-local facts that affect recovery command +// rendering. It belongs to one command-tree build; business error producers do +// not receive it and therefore remain independent of CLI invocation details. +type RenderContext struct { + // Profile is the explicit --profile override from this invocation. An empty + // value means recovery commands retain their historical profile-free form. + Profile string +} + +// AuthLoginCommand returns the auth-login command for this invocation. suffix +// is a code-owned argument fragment (for example "--device-code "); the +// invocation profile is always emitted as one shell-safe argv value. +func (c RenderContext) AuthLoginCommand(suffix string) string { + command := "lark-cli auth login" + if c.Profile != "" { + // The equals form keeps a leading-dash profile value attached to its flag; + // single-quote escaping prevents shell expansion or argument splitting. + command += " --profile=" + shellQuote(c.Profile) + } + if suffix != "" { + command += " " + suffix + } + return command +} + +// InlineAuthLoginCommand wraps AuthLoginCommand in a Markdown code span. The +// default command keeps the historical single-backtick bytes; unusual profile +// values containing backticks receive a longer delimiter without changing the +// shell command itself. +func (c RenderContext) InlineAuthLoginCommand(suffix string) string { + return inlineCode(c.AuthLoginCommand(suffix)) +} + +func shellQuote(value string) string { + return "'" + strings.ReplaceAll(value, "'", "'\"'\"'") + "'" +} + +func inlineCode(value string) string { + delimiter := "`" + for strings.Contains(value, delimiter) { + delimiter += "`" + } + if delimiter == "`" { + return delimiter + value + delimiter + } + return delimiter + " " + value + " " + delimiter +} diff --git a/internal/recovery/hint.go b/internal/recovery/hint.go index 5fe7fe607..4abae745e 100644 --- a/internal/recovery/hint.go +++ b/internal/recovery/hint.go @@ -43,8 +43,9 @@ const ( // target and is always retained; a command part is retained only while its // target remains referenceable in the current command surface. type Part struct { - text string - target Target + text string + target Target + renderText func(RenderContext) string } // Text returns a recovery hint part that does not point to a command. @@ -57,12 +58,17 @@ func Command(target Target, text string) Part { return Part{text: text, target: target} } +func contextualCommand(target Target, render func(RenderContext) string) Part { + return Part{target: target, renderText: render} +} + // Hint is an immutable sequence of semantic recovery parts. separator is used // only between retained non-empty parts, so filtering one action cannot leave // dangling punctuation such as a leading "; ". type Hint struct { separator string parts []Part + hints []Hint fallback string } @@ -73,6 +79,15 @@ func Join(separator string, parts ...Part) Hint { return Hint{separator: separator, parts: snapshot} } +// JoinHints composes independently renderable hints. Each child applies its +// own target filtering and fallback before the retained children are joined, +// allowing command-only recovery to keep a useful fallback alongside other +// policy guidance. +func JoinHints(separator string, hints ...Hint) Hint { + snapshot := append([]Hint(nil), hints...) + return Hint{separator: separator, hints: snapshot} +} + // WithFallback returns a copy that renders text when projection removes every // ordinary part. It is intended for command-only recovery: reduced // distributions must not retain a dead command pointer, but callers still @@ -82,22 +97,27 @@ func (h Hint) WithFallback(text string) Hint { return h } -// UserAuthorization returns the canonical user-login recovery. Business -// producers provide only the scopes they require; the command target, -// standard wording, and reduced-distribution fallback stay centralized. +// UserAuthorization returns canonical structured user-login recovery for +// producers that opt into this helper. Producers provide only the scopes they +// require; the command target, standard wording, and reduced-distribution +// fallback stay centralized. func UserAuthorization(scopes ...string) Hint { - var command string - if len(scopes) == 0 { - command = "run `lark-cli auth login` to authorize or refresh the current user" - } else { - command = fmt.Sprintf( - "run `lark-cli auth login --scope \"%s\"` to authorize or refresh the current user", - strings.Join(scopes, " "), - ) + scopes = append([]string(nil), scopes...) + fallback := "obtain or refresh a user credential through this distribution's supported authorization flow, have the user complete authorization, then retry" + if len(scopes) > 0 { + fallback += "\ncurrent command requires scope(s): " + strings.Join(scopes, ", ") } - return Join("", Command(TargetAuthLogin, command)).WithFallback( - "obtain or refresh a user credential through this distribution's supported authorization flow", - ) + return Join("", contextualCommand(TargetAuthLogin, func(context RenderContext) string { + startArgs := "--recommend --no-wait --json" + if len(scopes) > 0 { + startArgs = fmt.Sprintf("--scope \"%s\" --no-wait --json", strings.Join(scopes, " ")) + } + return fmt.Sprintf( + "run %s to get device_code and verification_url; present verification_url to the user exactly and end this turn; after the user confirms authorization, run %s in a later turn to finish login", + context.InlineAuthLoginCommand(startArgs), + context.InlineAuthLoginCommand("--device-code "), + ) + })).WithFallback(fallback) } // String returns the hint as rendered for the default, fully visible surface. @@ -107,15 +127,36 @@ func (h Hint) String() string { // Render filters command-targeted parts against plan without changing h. func (h Hint) Render(plan *surface.Plan) string { + return h.render(plan, RenderContext{}) +} + +func (h Hint) render(plan *surface.Plan, context RenderContext) string { + if len(h.hints) > 0 { + retained := make([]string, 0, len(h.hints)) + for _, child := range h.hints { + if rendered := child.render(plan, context); rendered != "" { + retained = append(retained, rendered) + } + } + if len(retained) == 0 { + return h.fallback + } + return strings.Join(retained, h.separator) + } + retained := make([]string, 0, len(h.parts)) for _, part := range h.parts { - if part.text == "" { + text := part.text + if part.renderText != nil { + text = part.renderText(context) + } + if text == "" { continue } if part.target != "" && !plan.CanReference(surface.CommandID(part.target)) { continue } - retained = append(retained, part.text) + retained = append(retained, text) } if len(retained) == 0 { return h.fallback diff --git a/internal/recovery/hint_test.go b/internal/recovery/hint_test.go index 5ea01a94f..fe4bcd0df 100644 --- a/internal/recovery/hint_test.go +++ b/internal/recovery/hint_test.go @@ -5,12 +5,100 @@ package recovery import ( "errors" + "strings" "testing" "github.com/larksuite/cli/errs" "github.com/larksuite/cli/internal/surface" ) +func TestUserAuthorizationGolden(t *testing.T) { + tests := []struct { + name string + hint Hint + visible string + concealed string + }{ + { + name: "no scopes", + hint: UserAuthorization(), + visible: "run `lark-cli auth login --recommend --no-wait --json` to get device_code and verification_url; " + + "present verification_url to the user exactly and end this turn; after the user confirms authorization, " + + "run `lark-cli auth login --device-code ` in a later turn to finish login", + concealed: "obtain or refresh a user credential through this distribution's supported authorization flow, have the user complete authorization, then retry", + }, + { + name: "multiple scopes", + hint: UserAuthorization("docx:document", "drive:drive"), + visible: "run `lark-cli auth login --scope \"docx:document drive:drive\" --no-wait --json` to get device_code and verification_url; " + + "present verification_url to the user exactly and end this turn; after the user confirms authorization, " + + "run `lark-cli auth login --device-code ` in a later turn to finish login", + concealed: "obtain or refresh a user credential through this distribution's supported authorization flow, have the user complete authorization, then retry\n" + + "current command requires scope(s): docx:document, drive:drive", + }, + } + + concealedPlan := surface.NewPlan(map[surface.CommandID]surface.CommandState{ + surface.CommandAuthLogin: surface.CommandConcealed, + }) + deniedVisiblePlan := surface.NewPlan(map[surface.CommandID]surface.CommandState{ + surface.CommandAuthLogin: surface.CommandDeniedVisible, + }) + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + if got := tt.hint.String(); got != tt.visible { + t.Fatalf("visible hint = %q, want %q", got, tt.visible) + } + if got := tt.hint.Render(deniedVisiblePlan); got != tt.visible { + t.Fatalf("denied-visible hint = %q, want %q", got, tt.visible) + } + if got := tt.hint.Render(concealedPlan); got != tt.concealed { + t.Fatalf("concealed hint = %q, want %q", got, tt.concealed) + } + for _, dead := range []string{"auth login", "verification_url", "device_code"} { + if got := tt.hint.Render(concealedPlan); strings.Contains(got, dead) { + t.Errorf("concealed hint %q contains dead command detail %q", got, dead) + } + } + }) + } +} + +func TestUserAuthorizationUsesBuildLocalProfileForBothCommands(t *testing.T) { + hint := UserAuthorization("docx:document", "drive:drive") + projector := NewProjectorWithContext(nil, RenderContext{Profile: "team-beta"}) + + want := "run `lark-cli auth login --profile='team-beta' --scope \"docx:document drive:drive\" --no-wait --json` to get device_code and verification_url; " + + "present verification_url to the user exactly and end this turn; after the user confirms authorization, " + + "run `lark-cli auth login --profile='team-beta' --device-code ` in a later turn to finish login" + if got := projector.RenderHint(hint); got != want { + t.Fatalf("profile-aware hint = %q, want %q", got, want) + } + + // The producer owns no invocation state: its default form remains byte-for-byte + // pinned by TestUserAuthorizationGolden after another build renders it. + if strings.Contains(hint.String(), "--profile") { + t.Fatalf("producer hint was polluted with build-local profile: %q", hint.String()) + } + + concealed := NewProjectorWithContext(func() *surface.Plan { + return surface.NewPlan(map[surface.CommandID]surface.CommandState{ + surface.CommandAuthLogin: surface.CommandConcealed, + }) + }, RenderContext{Profile: "team-beta"}).RenderHint(hint) + if strings.Contains(concealed, "team-beta") || strings.Contains(concealed, "auth login") { + t.Fatalf("concealed recovery leaked profile or command: %q", concealed) + } +} + +func TestRenderContextShellQuotesProfileAsOneArgument(t *testing.T) { + context := RenderContext{Profile: "team'$(touch /tmp/should-not-run)"} + want := `lark-cli auth login --profile='team'"'"'$(touch /tmp/should-not-run)' --device-code ` + if got := context.AuthLoginCommand("--device-code "); got != want { + t.Fatalf("AuthLoginCommand() = %q, want %q", got, want) + } +} + func TestHintRenderFiltersOnlyUnreferenceableTargets(t *testing.T) { hint := Join("; ", Command(TargetConfigInit, "run `lark-cli config init`"), diff --git a/internal/recovery/projector.go b/internal/recovery/projector.go index b67bdb22b..07a05cd91 100644 --- a/internal/recovery/projector.go +++ b/internal/recovery/projector.go @@ -14,7 +14,8 @@ import "github.com/larksuite/cli/internal/surface" // A nil Projector, nil callback, or nil Plan means the default fully-visible // surface. type Projector struct { - plan func() *surface.Plan + plan func() *surface.Plan + context RenderContext } // NewProjector returns a projector backed by one command tree's plan callback. @@ -22,6 +23,12 @@ func NewProjector(plan func() *surface.Plan) *Projector { return &Projector{plan: plan} } +// NewProjectorWithContext returns a projector whose presentation is scoped to +// both one command tree and one immutable invocation context. +func NewProjectorWithContext(plan func() *surface.Plan, context RenderContext) *Projector { + return &Projector{plan: plan, context: context} +} + func (p *Projector) surfacePlan() *surface.Plan { if p == nil || p.plan == nil { return nil @@ -37,10 +44,16 @@ func (p *Projector) CanReference(target Target) bool { // Render clones and projects a typed error for this command tree. func (p *Projector) Render(err error) error { - return Render(err, p.surfacePlan()) + if p == nil { + return Render(err, nil) + } + return renderWithContext(err, p.surfacePlan(), p.context) } // RenderHint projects one semantic hint for this command tree. func (p *Projector) RenderHint(hint Hint) string { - return hint.Render(p.surfacePlan()) + if p == nil { + return hint.Render(nil) + } + return hint.render(p.surfacePlan(), p.context) } diff --git a/internal/recovery/render.go b/internal/recovery/render.go index 12628ce35..0b7d3ce2c 100644 --- a/internal/recovery/render.go +++ b/internal/recovery/render.go @@ -18,6 +18,12 @@ import ( // Untyped errors and raw-passthrough errors are returned unchanged. Raw errors // intentionally bypass local presentation rewriting. func Render(err error, plan *surface.Plan) error { + return renderWithContext(err, plan, RenderContext{}) +} + +// renderWithContext is Render with build-local invocation facts used only +// while materializing structured recovery commands. +func renderWithContext(err error, plan *surface.Plan, context RenderContext) error { if err == nil || errs.IsRaw(err) { return err } @@ -35,12 +41,12 @@ func Render(err error, plan *surface.Plan) error { } if hint, ok := hintOf(err, sourceProblem); ok { if problem, ok := errs.ProblemOf(rendered); ok { - problem.Hint = projectAnnotatedText(problem.Hint, hint, plan) + problem.Hint = projectAnnotatedText(problem.Hint, hint, plan, context) } } if message, ok := messageOf(err, sourceProblem); ok { if problem, ok := errs.ProblemOf(rendered); ok { - problem.Message = projectAnnotatedText(problem.Message, message, plan) + problem.Message = projectAnnotatedText(problem.Message, message, plan, context) } } return rendered @@ -49,9 +55,9 @@ func Render(err error, plan *surface.Plan) error { // projectAnnotatedText replaces only the exact annotated recovery fragment. // Producers may enrich a typed error after annotation (for example with // rollback IDs); text around that owned fragment must survive filtering. -func projectAnnotatedText(current string, annotation Hint, plan *surface.Plan) string { +func projectAnnotatedText(current string, annotation Hint, plan *surface.Plan, context RenderContext) string { original := annotation.String() - projected := annotation.Render(plan) + projected := annotation.render(plan, context) if projected == original { return current } diff --git a/internal/skillpolicy/dependencies.go b/internal/skillpolicy/dependencies.go new file mode 100644 index 000000000..04f198b21 --- /dev/null +++ b/internal/skillpolicy/dependencies.go @@ -0,0 +1,112 @@ +// Copyright (c) 2026 Lark Technologies Pte. Ltd. +// SPDX-License-Identifier: MIT + +package skillpolicy + +import ( + "fmt" + "io/fs" + "sort" + "strings" + + "gopkg.in/yaml.v3" +) + +// skillManifest is the build-integrity metadata frozen when a skill tree is +// scanned. Runtime content within the owning skill directory remains live, but +// composition must not be able to change its dependency contract after Resolve. +type skillManifest struct { + requiredSkills []string +} + +func readSkillManifest(source fs.FS, name string) (skillManifest, error) { + data, err := fs.ReadFile(source, name+"/SKILL.md") + if err != nil { + return skillManifest{}, fmt.Errorf("cannot read SKILL.md: %w", err) + } + required, err := parseRequiredSkills(name, data) + if err != nil { + return skillManifest{}, err + } + return skillManifest{requiredSkills: required}, nil +} + +// parseRequiredSkills reads only the structured hard-dependency declaration: +// +// metadata: +// requires: +// skills: ["lark-shared"] +// +// Markdown links and prose are intentionally irrelevant. A SKILL.md without +// YAML frontmatter declares no hard dependencies; malformed frontmatter that +// purports to be structured metadata fails closed during composition. +func parseRequiredSkills(skillName string, skillMD []byte) ([]string, error) { + // A UTF-8 text file may begin with one BOM. Normalize it before checking + // the delimiter so BOM-prefixed dependency metadata cannot be skipped. + content := strings.TrimPrefix(string(skillMD), "\uFEFF") + lines := strings.Split(content, "\n") + if strings.TrimRight(lines[0], "\r") != "---" { + return nil, nil + } + + block := make([]string, 0, len(lines)) + closed := false + for _, line := range lines[1:] { + if strings.TrimRight(line, "\r") == "---" { + closed = true + break + } + block = append(block, line) + } + if !closed { + return nil, fmt.Errorf("SKILL.md frontmatter is not closed") + } + + var frontmatter struct { + Metadata struct { + Requires struct { + Skills []string `yaml:"skills"` + } `yaml:"requires"` + } `yaml:"metadata"` + } + if err := yaml.Unmarshal([]byte(strings.Join(block, "\n")), &frontmatter); err != nil { + return nil, fmt.Errorf("cannot parse SKILL.md frontmatter: %w", err) + } + + required := frontmatter.Metadata.Requires.Skills + seen := make(map[string]struct{}, len(required)) + out := make([]string, 0, len(required)) + for _, dependency := range required { + if !isSkillName(dependency) { + return nil, fmt.Errorf("required skill %q declared by %q is not a valid skill name", dependency, skillName) + } + if _, duplicate := seen[dependency]; duplicate { + continue + } + seen[dependency] = struct{}{} + out = append(out, dependency) + } + return out, nil +} + +// validateRequiredSkills checks the already-composed owner manifest. It must +// run after Base -> Allow -> Remove -> Overlay so no validation branch can +// accidentally disagree with the tree that list/read actually serves. +func validateRequiredSkills(composed *overlayFS) error { + if composed == nil { + return nil + } + names := make([]string, 0, len(composed.owner)) + for name := range composed.owner { + names = append(names, name) + } + sort.Strings(names) + for _, name := range names { + for _, dependency := range composed.owner[name].manifest.requiredSkills { + if _, present := composed.owner[dependency]; !present { + return fmt.Errorf("%w: skill %q requires skill %q, but %q is absent from the composed skill tree", ErrUnsatisfiedSkillDependency, name, dependency, dependency) + } + } + } + return nil +} diff --git a/internal/skillpolicy/overlay.go b/internal/skillpolicy/overlay.go index f59a690e8..d167ca11e 100644 --- a/internal/skillpolicy/overlay.go +++ b/internal/skillpolicy/overlay.go @@ -21,14 +21,20 @@ import ( // so the io/fs helpers route through the merge instead of hitting a // single underlying tree. type overlayFS struct { - // The manifest: top-level skill name -> owning tree, snapshotted once - // at composition. Routing and listing consult the same snapshot, so a - // top-level directory added later cannot appear through only one of - // those surfaces. Contents WITHIN a skill directory are still read live. - owner map[string]fs.FS + // The manifest: top-level skill name -> owning tree and integrity + // metadata, snapshotted once at composition. Routing, listing, and + // dependency validation consult the same snapshot, so a top-level + // directory added later cannot appear through only one of those surfaces. + // Contents WITHIN a skill directory are still read live. + owner map[string]skillOwner entries []fs.DirEntry // manifest listing, sorted by name } +type skillOwner struct { + source fs.FS + manifest skillManifest +} + var ( _ fs.FS = (*overlayFS)(nil) _ fs.ReadDirFS = (*overlayFS)(nil) @@ -49,11 +55,11 @@ func newOverlayFS(lower, upper skillTreeSnapshot, remove, allow []string) *overl } } - o := &overlayFS{owner: map[string]fs.FS{}} - for name := range upper.skills { - o.owner[name] = upper.source + o := &overlayFS{owner: map[string]skillOwner{}} + for name, manifest := range upper.skills { + o.owner[name] = skillOwner{source: upper.source, manifest: manifest} } - for name := range lower.skills { + for name, manifest := range lower.skills { if removed[name] { continue } @@ -63,7 +69,7 @@ func newOverlayFS(lower, upper skillTreeSnapshot, remove, allow []string) *overl if allowed != nil && !allowed[name] { continue } - o.owner[name] = lower.source + o.owner[name] = skillOwner{source: lower.source, manifest: manifest} } // Derive the listing from the routing manifest after composition. Keeping @@ -86,7 +92,7 @@ func (o *overlayFS) route(name string) (target fs.FS, whiteout bool) { top = name[:i] } if t, ok := o.owner[top]; ok { - return t, false + return t.source, false } return nil, true } diff --git a/internal/skillpolicy/resolver.go b/internal/skillpolicy/resolver.go index b21450c43..f0356ab0a 100644 --- a/internal/skillpolicy/resolver.go +++ b/internal/skillpolicy/resolver.go @@ -43,6 +43,12 @@ var ErrNoBaseSkillContent = errors.New("build embeds no base skill content") // can direct the integrator to the correct owner. var ErrInvalidHostBase = errors.New("host embedded skill content is invalid") +// ErrUnsatisfiedSkillDependency reports that a skill retained by the final +// composed manifest declares another skill that the manifest does not retain. +// The resolver never widens Allow or overrides Remove to repair this: an +// incomplete distribution is a build-integrity error. +var ErrUnsatisfiedSkillDependency = errors.New("composed skill tree has an unsatisfied required skill") + // Resolution is the build-local result of composing embedded skill assets. // Content serves `skills list`/`read`; References projects canonical // CLI-authored pointers onto that same tree. @@ -106,7 +112,11 @@ func ResolveWithReferences(base fs.FS, specs []PluginSkill) (Resolution, error) if lower == nil && upper == nil { content = nil } else { - content = newOverlayFS(lowerSnapshot, upperSnapshot, spec.Remove, spec.Allow) + composed := newOverlayFS(lowerSnapshot, upperSnapshot, spec.Remove, spec.Allow) + if err := validateRequiredSkills(composed); err != nil { + return Resolution{}, fmt.Errorf("plugin %q skill spec: %w", owner, err) + } + content = composed } refs, err := resolveReferences(content, spec.ReferenceRemaps) if err != nil { @@ -147,14 +157,14 @@ func distinctOwners(specs []PluginSkill) []string { type skillTreeSnapshot struct { source fs.FS - skills map[string]struct{} + skills map[string]skillManifest } // scanSkillTree validates and snapshots a skill tree's top level in one // pass. The returned set is the only source used by validation and overlay // composition, so a mutable FS cannot swap unvalidated names between phases. func scanSkillTree(label string, source fs.FS) (skillTreeSnapshot, error) { - snapshot := skillTreeSnapshot{source: source, skills: map[string]struct{}{}} + snapshot := skillTreeSnapshot{source: source, skills: map[string]skillManifest{}} if source == nil { return snapshot, nil } @@ -177,7 +187,11 @@ func scanSkillTree(label string, source fs.FS) (skillTreeSnapshot, error) { if !ok { return snapshot, fmt.Errorf("%s: skill %q is missing SKILL.md", label, name) } - snapshot.skills[name] = struct{}{} + manifest, err := readSkillManifest(source, name) + if err != nil { + return snapshot, fmt.Errorf("%s: skill %q has invalid metadata: %w", label, name, err) + } + snapshot.skills[name] = manifest } return snapshot, nil } diff --git a/internal/skillpolicy/resolver_test.go b/internal/skillpolicy/resolver_test.go index 32dfd2212..6287f1d00 100644 --- a/internal/skillpolicy/resolver_test.go +++ b/internal/skillpolicy/resolver_test.go @@ -35,6 +35,14 @@ func baseTree() fstest.MapFS { }) } +func baseTreeWithRequiredShared() fstest.MapFS { + return skillFS(map[string]string{ + "lark-a/SKILL.md": "---\nmetadata:\n requires:\n skills: [\"lark-shared\"]\n---\nbase a", + "lark-b/SKILL.md": "base b", + "lark-shared/SKILL.md": "base shared", + }) +} + // topLevel returns the sorted top-level skill names of fsys. func topLevel(t *testing.T, fsys fs.FS) []string { t.Helper() @@ -283,6 +291,158 @@ func TestResolve_Allow_KeepsOnlyListed(t *testing.T) { } } +func TestResolve_AllowMissingRequiredSkillFailsClosed(t *testing.T) { + _, err := resolveContent(baseTreeWithRequiredShared(), []PluginSkill{{ + PluginName: "acme", + SkillsOverlay: &platform.SkillsOverlay{ + Allow: []string{"lark-a"}, + }, + }}) + if !errors.Is(err, ErrUnsatisfiedSkillDependency) { + t.Fatalf("err = %v, want ErrUnsatisfiedSkillDependency", err) + } + for _, want := range []string{"lark-a", "lark-shared"} { + if !strings.Contains(err.Error(), want) { + t.Errorf("error %q does not identify %q", err, want) + } + } +} + +func TestResolve_AllowIncludingRequiredSkillSucceeds(t *testing.T) { + got := mustResolve(t, baseTreeWithRequiredShared(), &platform.SkillsOverlay{ + Allow: []string{"lark-a", "lark-shared"}, + }) + if want := []string{"lark-a", "lark-shared"}; !slices.Equal(topLevel(t, got), want) { + t.Fatalf("top level = %v, want %v", topLevel(t, got), want) + } +} + +func TestResolve_UTF8BOMFrontmatterRequiredSkillPresentSucceeds(t *testing.T) { + base := skillFS(map[string]string{ + "lark-a/SKILL.md": "\uFEFF---\nmetadata:\n requires:\n skills: [\"lark-shared\"]\n---\nbase a", + "lark-shared/SKILL.md": "base shared", + }) + + got := mustResolve(t, base, &platform.SkillsOverlay{ + Allow: []string{"lark-a", "lark-shared"}, + }) + if want := []string{"lark-a", "lark-shared"}; !slices.Equal(topLevel(t, got), want) { + t.Fatalf("top level = %v, want %v", topLevel(t, got), want) + } +} + +func TestResolve_UTF8BOMFrontmatterMissingRequiredSkillFailsClosed(t *testing.T) { + base := skillFS(map[string]string{ + "lark-a/SKILL.md": "\uFEFF---\nmetadata:\n requires:\n skills: [\"lark-shared\"]\n---\nbase a", + "lark-shared/SKILL.md": "base shared", + }) + + _, err := resolveContent(base, []PluginSkill{{ + PluginName: "acme", + SkillsOverlay: &platform.SkillsOverlay{ + Allow: []string{"lark-a"}, + }, + }}) + if !errors.Is(err, ErrUnsatisfiedSkillDependency) { + t.Fatalf("err = %v, want ErrUnsatisfiedSkillDependency", err) + } + for _, want := range []string{"lark-a", "lark-shared"} { + if !strings.Contains(err.Error(), want) { + t.Errorf("error %q does not identify %q", err, want) + } + } +} + +func TestResolve_RemoveRequiredSkillFailsClosed(t *testing.T) { + _, err := resolveContent(baseTreeWithRequiredShared(), []PluginSkill{{ + PluginName: "acme", + SkillsOverlay: &platform.SkillsOverlay{ + Remove: []string{"lark-shared"}, + }, + }}) + if !errors.Is(err, ErrUnsatisfiedSkillDependency) { + t.Fatalf("err = %v, want ErrUnsatisfiedSkillDependency", err) + } +} + +func TestResolve_OverlayReplacementUsesReplacementDependencies(t *testing.T) { + overlay := skillFS(map[string]string{ + "lark-a/SKILL.md": "replacement a without dependencies", + }) + got := mustResolve(t, baseTreeWithRequiredShared(), &platform.SkillsOverlay{ + Allow: []string{"lark-a"}, + Overlay: overlay, + }) + if want := []string{"lark-a"}; !slices.Equal(topLevel(t, got), want) { + t.Fatalf("top level = %v, want %v", topLevel(t, got), want) + } +} + +func TestResolve_OverlayReplacementMissingOwnDependencyFailsClosed(t *testing.T) { + overlay := skillFS(map[string]string{ + "lark-a/SKILL.md": "---\nmetadata:\n requires:\n skills: [\"acme-runtime\"]\n---\nreplacement a", + }) + _, err := resolveContent(baseTreeWithRequiredShared(), []PluginSkill{{ + PluginName: "acme", + SkillsOverlay: &platform.SkillsOverlay{ + Allow: []string{"lark-a"}, + Overlay: overlay, + }, + }}) + if !errors.Is(err, ErrUnsatisfiedSkillDependency) { + t.Fatalf("err = %v, want ErrUnsatisfiedSkillDependency", err) + } + if !strings.Contains(err.Error(), "acme-runtime") { + t.Fatalf("error does not identify replacement dependency: %v", err) + } +} + +func TestResolve_DoesNotInferDependenciesFromMarkdownLinks(t *testing.T) { + base := skillFS(map[string]string{ + "lark-a/SKILL.md": "Read [missing](../lark-missing/SKILL.md) when useful.", + }) + got := mustResolve(t, base, &platform.SkillsOverlay{Allow: []string{"lark-a"}}) + if want := []string{"lark-a"}; !slices.Equal(topLevel(t, got), want) { + t.Fatalf("top level = %v, want %v", topLevel(t, got), want) + } +} + +func TestResolve_UnclosedFrontmatterFailsClosed(t *testing.T) { + base := skillFS(map[string]string{ + "lark-a/SKILL.md": "---\nmetadata:\n requires:\n skills: [\"lark-shared\"]\nbase a", + }) + _, err := resolveContent(base, []PluginSkill{{ + PluginName: "acme", + SkillsOverlay: &platform.SkillsOverlay{Allow: []string{"lark-a"}}, + }}) + if !errors.Is(err, ErrInvalidHostBase) { + t.Fatalf("err = %v, want ErrInvalidHostBase", err) + } + for _, want := range []string{"lark-a", "invalid metadata", "frontmatter is not closed"} { + if !strings.Contains(err.Error(), want) { + t.Errorf("error %q does not identify %q", err, want) + } + } +} + +func TestResolve_InvalidRequiredSkillNameFailsClosed(t *testing.T) { + base := skillFS(map[string]string{ + "lark-a/SKILL.md": "---\nmetadata:\n requires:\n skills: [\"../escape\"]\n---\nbase a", + }) + _, err := resolveContent(base, []PluginSkill{{ + PluginName: "acme", + SkillsOverlay: &platform.SkillsOverlay{Allow: []string{"lark-a"}}, + }}) + if !errors.Is(err, ErrInvalidHostBase) { + t.Fatalf("err = %v, want ErrInvalidHostBase", err) + } + for _, want := range []string{"lark-a", "invalid metadata", "../escape"} { + if !strings.Contains(err.Error(), want) { + t.Errorf("error %q does not identify %q", err, want) + } + } +} + // Remove wins over Allow, mirroring Rule's Deny-over-Allow. func TestResolve_RemoveWinsOverAllow(t *testing.T) { got := mustResolve(t, baseTree(), &platform.SkillsOverlay{ diff --git a/shortcuts/common/mcp_client.go b/shortcuts/common/mcp_client.go index 817f13d6a..b6e87f529 100644 --- a/shortcuts/common/mcp_client.go +++ b/shortcuts/common/mcp_client.go @@ -62,6 +62,10 @@ func normalizeMCPToolResult(raw interface{}) (map[string]interface{}, error) { } func DoMCPCall(ctx context.Context, httpClient *http.Client, toolName string, args map[string]interface{}, accessToken string, mcpEndpoint string, isBot bool) (interface{}, error) { + identity := string(core.AsUser) + if isBot { + identity = string(core.AsBot) + } body := map[string]interface{}{ "jsonrpc": "2.0", "id": uuid.NewString(), @@ -100,7 +104,7 @@ func DoMCPCall(ctx context.Context, httpClient *http.Client, toolName string, ar return nil, errs.NewNetworkError(errs.SubtypeNetworkTransport, "failed to read MCP response: %v", err).WithCause(err) } if resp.StatusCode >= 400 { - return nil, classifyMCPHTTPError(resp.StatusCode, resp.Status, respBody) + return nil, classifyMCPHTTPError(resp.StatusCode, resp.Status, respBody, identity) } var data map[string]interface{} @@ -111,34 +115,66 @@ func DoMCPCall(ctx context.Context, httpClient *http.Client, toolName string, ar } if errObj, ok := data["error"]; ok { - return nil, classifyMCPPayloadError(errObj) + return nil, classifyMCPPayloadError(errObj, identity) } return UnwrapMCPResult(data["result"]), nil } -func classifyMCPHTTPError(statusCode int, status string, body []byte) error { +func classifyMCPHTTPError(statusCode int, status string, body []byte, identity string) error { var payload map[string]interface{} if err := json.Unmarshal(body, &payload); err == nil { - if errObj, ok := payload["error"]; ok { - return classifyMCPPayloadError(errObj) + code, msg, hasBusinessError := extractMCPBusinessError(payload) + if hasBusinessError { + if _, known := errclass.LookupCodeMeta(code); known { + classified := errclass.BuildAPIError(payload, errclass.ClassifyContext{Identity: identity}) + return withMCPAuthenticationRecovery(classified, identity) + } } - if code, msg, ok := extractMCPBusinessError(payload); ok { + if errObj, ok := payload["error"]; ok { + if statusCode == http.StatusUnauthorized && !hasKnownMCPErrorCode(errObj) { + return newMCPHTTPAuthenticationError(statusCode, status, body, identity) + } + return classifyMCPPayloadError(errObj, identity) + } + if hasBusinessError { + if statusCode == http.StatusUnauthorized { + return newMCPHTTPAuthenticationError(statusCode, status, body, identity) + } return errs.NewAPIError(errs.SubtypeUnknown, "MCP HTTP %d %s: [%d] %s", statusCode, status, code, msg).WithCode(code) } } - bodyText := TruncateStr(strings.TrimSpace(string(body)), mcpErrorBodyLimit) if statusCode == http.StatusUnauthorized { - return errs.NewAuthenticationError(errs.SubtypeTokenInvalid, "MCP HTTP %d %s: %s", statusCode, status, bodyText).WithCode(statusCode) + return newMCPHTTPAuthenticationError(statusCode, status, body, identity) } + bodyText := TruncateStr(strings.TrimSpace(string(body)), mcpErrorBodyLimit) if statusCode >= 500 { return errs.NewNetworkError(errs.SubtypeNetworkServer, "MCP HTTP %d %s: %s", statusCode, status, bodyText).WithCode(statusCode) } return errs.NewAPIError(errs.SubtypeUnknown, "MCP HTTP %d %s: %s", statusCode, status, bodyText).WithCode(statusCode) } -func classifyMCPPayloadError(errObj interface{}) error { +func hasKnownMCPErrorCode(errObj interface{}) bool { + errMap, ok := errObj.(map[string]interface{}) + if !ok { + return false + } + code, ok := util.ToFloat64(errMap["code"]) + if !ok { + return false + } + _, known := errclass.LookupCodeMeta(int(code)) + return known +} + +func newMCPHTTPAuthenticationError(statusCode int, status string, body []byte, identity string) error { + bodyText := TruncateStr(strings.TrimSpace(string(body)), mcpErrorBodyLimit) + err := errs.NewAuthenticationError(errs.SubtypeTokenInvalid, "MCP HTTP %d %s: %s", statusCode, status, bodyText).WithCode(statusCode) + return withMCPAuthenticationRecovery(err, identity) +} + +func classifyMCPPayloadError(errObj interface{}, identity string) error { if errMap, ok := errObj.(map[string]interface{}); ok { msg := GetString(errMap, "message") if msg == "" { @@ -149,39 +185,48 @@ func classifyMCPPayloadError(errObj interface{}) error { // codes become typed (Authentication / Permission / ...) rather // than generic APIError. Falls back to APIError for unknown codes. payload := map[string]any{"code": int(code), "msg": msg, "error": errMap} - if classified := errclass.BuildAPIError(payload, errclass.ClassifyContext{}); classified != nil { - return classified + if classified := errclass.BuildAPIError(payload, errclass.ClassifyContext{Identity: identity}); classified != nil { + return withMCPAuthenticationRecovery(classified, identity) } return errs.NewAPIError(errs.SubtypeUnknown, "MCP: [%.0f] %s", code, msg).WithCode(int(code)) } if msg != "" { - return classifyMCPMessageError(fmt.Sprintf("MCP: %s", msg)) + return classifyMCPMessageError(fmt.Sprintf("MCP: %s", msg), identity) } } if msg, ok := errObj.(string); ok && strings.TrimSpace(msg) != "" { - return classifyMCPMessageError(fmt.Sprintf("MCP: %s", msg)) + return classifyMCPMessageError(fmt.Sprintf("MCP: %s", msg), identity) } return errs.NewAPIError(errs.SubtypeUnknown, "MCP returned an error response") } -func classifyMCPMessageError(msg string) error { +func classifyMCPMessageError(msg, identity string) error { lower := strings.ToLower(msg) switch { case strings.Contains(lower, "unauthorized"), strings.Contains(lower, "access token"), strings.Contains(lower, "token invalid"), strings.Contains(lower, "token expired"): - return recovery.Attach( - errs.NewAuthenticationError(errs.SubtypeTokenInvalid, "%s", msg), - recovery.UserAuthorization(), - ) + return withMCPAuthenticationRecovery( + errs.NewAuthenticationError(errs.SubtypeTokenInvalid, "%s", msg), identity) default: return errs.NewAPIError(errs.SubtypeUnknown, "%s", msg) } } +func withMCPAuthenticationRecovery(err error, identity string) error { + authErr, ok := err.(*errs.AuthenticationError) //nolint:errorlint // enrich only fresh direct MCP classifier errors, never a wrapped cause + if !ok || authErr.Hint != "" { + return err + } + if identity == string(core.AsBot) { + return authErr.WithHint("configure valid app credentials for the bot identity") + } + return recovery.Attach(authErr, recovery.UserAuthorization()) +} + func extractMCPBusinessError(payload map[string]interface{}) (int, string, bool) { code, ok := util.ToFloat64(payload["code"]) if !ok || code == 0 { diff --git a/shortcuts/common/mcp_client_test.go b/shortcuts/common/mcp_client_test.go index bbfc1101c..788ede865 100644 --- a/shortcuts/common/mcp_client_test.go +++ b/shortcuts/common/mcp_client_test.go @@ -22,21 +22,118 @@ func (f roundTripFunc) RoundTrip(req *http.Request) (*http.Response, error) { } func TestDoMCPCallUnauthorizedHTTPError(t *testing.T) { - t.Parallel() + for _, tt := range []struct { + name string + isBot bool + wantHint string + forbidden string + }{ + {name: "user", wantHint: "auth login --recommend --no-wait --json", forbidden: "bot identity"}, + {name: "bot", isBot: true, wantHint: "valid app credentials for the bot identity", forbidden: "auth login"}, + } { + t.Run(tt.name, func(t *testing.T) { + client := &http.Client{Transport: roundTripFunc(func(*http.Request) (*http.Response, error) { + return &http.Response{ + StatusCode: http.StatusUnauthorized, + Status: "401 Unauthorized", + Body: io.NopCloser(strings.NewReader("unauthorized")), + }, nil + })} - client := &http.Client{ - Transport: roundTripFunc(func(*http.Request) (*http.Response, error) { - return &http.Response{ - StatusCode: http.StatusUnauthorized, - Status: "401 Unauthorized", - Body: io.NopCloser(strings.NewReader("unauthorized")), - }, nil - }), + _, err := DoMCPCall(context.Background(), client, "fetch-doc", map[string]interface{}{"doc_id": "doc_1"}, "token", "https://example.com/mcp", tt.isBot) + if got := output.ExitCodeOf(err); got != output.ExitAuth { + t.Fatalf("expected auth exit code (%d), got %d", output.ExitAuth, got) + } + var authErr *errs.AuthenticationError + if !errors.As(err, &authErr) { + t.Fatalf("error = %T %v, want *errs.AuthenticationError", err, err) + } + if authErr.Subtype != errs.SubtypeTokenInvalid || authErr.Code != http.StatusUnauthorized { + t.Errorf("authentication error = %+v, want token_invalid code 401", authErr) + } + if !strings.Contains(authErr.Hint, tt.wantHint) || strings.Contains(authErr.Hint, tt.forbidden) { + t.Errorf("hint = %q, want %q and no %q", authErr.Hint, tt.wantHint, tt.forbidden) + } + }) + } +} + +func TestDoMCPCallUnauthorizedHTTPUnknownStructuredErrorUsesAuthRecovery(t *testing.T) { + payloads := []struct { + name string + body string + wantCode string + }{ + {name: "top-level", body: `{"code":987654321,"msg":"unknown MCP auth failure"}`, wantCode: "987654321"}, + {name: "JSON-RPC", body: `{"error":{"code":-32001,"message":"unknown MCP auth failure"}}`, wantCode: "-32001"}, + } + identities := []struct { + name string + isBot bool + wantHint string + forbidden string + }{ + {name: "user", wantHint: "auth login --recommend --no-wait --json", forbidden: "bot identity"}, + {name: "bot", isBot: true, wantHint: "valid app credentials for the bot identity", forbidden: "auth login"}, } - _, err := DoMCPCall(context.Background(), client, "fetch-doc", map[string]interface{}{"doc_id": "doc_1"}, "uat-token", "https://example.com/mcp", false) - if got := output.ExitCodeOf(err); got != output.ExitAuth { - t.Fatalf("expected auth exit code (%d), got %d", output.ExitAuth, got) + for _, payload := range payloads { + for _, identity := range identities { + t.Run(payload.name+"/"+identity.name, func(t *testing.T) { + client := &http.Client{Transport: roundTripFunc(func(*http.Request) (*http.Response, error) { + return &http.Response{ + StatusCode: http.StatusUnauthorized, + Status: "401 Unauthorized", + Body: io.NopCloser(strings.NewReader(payload.body)), + }, nil + })} + + _, err := DoMCPCall(context.Background(), client, "fetch-doc", nil, "token", "https://example.com/mcp", identity.isBot) + if got := output.ExitCodeOf(err); got != output.ExitAuth { + t.Fatalf("exit code = %d, want auth exit code %d", got, output.ExitAuth) + } + var authErr *errs.AuthenticationError + if !errors.As(err, &authErr) { + t.Fatalf("error = %T %v, want *errs.AuthenticationError", err, err) + } + if authErr.Subtype != errs.SubtypeTokenInvalid || authErr.Code != http.StatusUnauthorized { + t.Errorf("authentication error = %+v, want token_invalid code 401", authErr) + } + if !strings.Contains(authErr.Message, payload.wantCode) { + t.Errorf("message = %q, want upstream code %s preserved as diagnostic context", authErr.Message, payload.wantCode) + } + if !strings.Contains(authErr.Hint, identity.wantHint) || strings.Contains(authErr.Hint, identity.forbidden) { + t.Errorf("hint = %q, want %q and no %q", authErr.Hint, identity.wantHint, identity.forbidden) + } + }) + } + } +} + +func TestDoMCPCallUnauthorizedHTTPKnownNestedCodeKeepsBusinessClassification(t *testing.T) { + client := &http.Client{Transport: roundTripFunc(func(*http.Request) (*http.Response, error) { + return &http.Response{ + StatusCode: http.StatusUnauthorized, + Status: "401 Unauthorized", + Body: io.NopCloser(strings.NewReader( + `{"error":{"code":99991679,"message":"missing scope","permission_violations":[{"subject":"drive:file:read"}]}}`, + )), + }, nil + })} + + _, err := DoMCPCall(context.Background(), client, "fetch-doc", nil, "token", "https://example.com/mcp", true) + var permission *errs.PermissionError + if !errors.As(err, &permission) { + t.Fatalf("error = %T %v, want *errs.PermissionError", err, err) + } + if permission.Subtype != errs.SubtypeMissingScope || permission.Code != 99991679 || permission.Identity != "bot" { + t.Fatalf("permission error = %+v, want bot missing_scope code 99991679", permission) + } + if len(permission.MissingScopes) != 1 || permission.MissingScopes[0] != "drive:file:read" { + t.Errorf("missing_scopes = %v, want [drive:file:read]", permission.MissingScopes) + } + if strings.Contains(permission.Hint, "auth login") || !strings.Contains(permission.Hint, "app developer") { + t.Errorf("bot hint = %q, want developer recovery without user OAuth", permission.Hint) } } @@ -64,6 +161,164 @@ func TestDoMCPCallJSONRPCErrorUsesLarkClassification(t *testing.T) { if !errors.As(err, &authErr) { t.Fatalf("expected *errs.AuthenticationError, got %T: %v", err, err) } + if !strings.Contains(authErr.Hint, "auth login") { + t.Fatalf("user MCP authentication recovery = %q, want user login", authErr.Hint) + } +} + +func TestDoMCPCallPermissionErrorKeepsCallingIdentity(t *testing.T) { + tests := []struct { + name string + body string + subtype errs.Subtype + wantScope string + wantBotHint string + }{ + { + name: "missing scope", + body: `{"error":{"code":99991679,"message":"missing scope","permission_violations":[{"subject":"drive:file:read"}]}}`, + subtype: errs.SubtypeMissingScope, + wantScope: "drive:file:read", + wantBotHint: "app developer", + }, + { + name: "token scope insufficient", + body: `{"error":{"code":99991676,"message":"token scope insufficient","permission_violations":[{"subject":"drive:file:read"}]}}`, + subtype: errs.SubtypeTokenScopeInsufficient, + wantScope: "drive:file:read", + wantBotHint: "token's granted scopes", + }, + { + name: "user unauthorized code", + body: `{"error":{"code":230027,"message":"operation unauthorized"}}`, + subtype: errs.SubtypeUserUnauthorized, + wantBotHint: "required bot permissions", + }, + } + identities := []struct { + name string + isBot bool + }{ + {name: "user"}, + {name: "bot", isBot: true}, + } + + for _, tt := range tests { + for _, identity := range identities { + t.Run(tt.name+"/"+identity.name, func(t *testing.T) { + client := &http.Client{Transport: roundTripFunc(func(*http.Request) (*http.Response, error) { + return &http.Response{ + StatusCode: http.StatusOK, + Status: "200 OK", + Body: io.NopCloser(strings.NewReader(tt.body)), + }, nil + })} + + _, err := DoMCPCall(context.Background(), client, "fetch-doc", nil, "token", "https://example.com/mcp", identity.isBot) + var permission *errs.PermissionError + if !errors.As(err, &permission) { + t.Fatalf("error = %T %v, want *errs.PermissionError", err, err) + } + if permission.Subtype != tt.subtype || permission.Code == 0 { + t.Errorf("permission error = %+v, want subtype %q and upstream code", permission, tt.subtype) + } + if permission.Identity != identity.name { + t.Errorf("identity = %q, want %q", permission.Identity, identity.name) + } + if tt.wantScope != "" && (len(permission.MissingScopes) != 1 || permission.MissingScopes[0] != tt.wantScope) { + t.Errorf("missing_scopes = %v, want [%s]", permission.MissingScopes, tt.wantScope) + } + if identity.isBot { + for _, forbidden := range []string{"auth login", "verification_url", "device_code", "user authorization"} { + if strings.Contains(strings.ToLower(permission.Hint+"\n"+permission.Message), forbidden) { + t.Errorf("bot error contains user OAuth guidance %q: %+v", forbidden, permission) + } + } + if !strings.Contains(permission.Hint, tt.wantBotHint) { + t.Errorf("bot hint = %q, want %q", permission.Hint, tt.wantBotHint) + } + } else if !strings.Contains(permission.Hint, "auth login") { + t.Errorf("user hint = %q, want user OAuth recovery", permission.Hint) + } + }) + } + } +} + +func TestDoMCPCallMessageOnlyAuthorizationRecoveryUsesCallingIdentity(t *testing.T) { + for _, tt := range []struct { + name string + isBot bool + wantHint string + forbidden string + }{ + {name: "user", wantHint: "auth login", forbidden: "bot identity"}, + {name: "bot", isBot: true, wantHint: "valid app credentials for the bot identity", forbidden: "auth login"}, + } { + t.Run(tt.name, func(t *testing.T) { + client := &http.Client{Transport: roundTripFunc(func(*http.Request) (*http.Response, error) { + return &http.Response{ + StatusCode: http.StatusOK, + Status: "200 OK", + Body: io.NopCloser(strings.NewReader(`{"error":"unauthorized"}`)), + }, nil + })} + + _, err := DoMCPCall(context.Background(), client, "fetch-doc", nil, "token", "https://example.com/mcp", tt.isBot) + var authErr *errs.AuthenticationError + if !errors.As(err, &authErr) { + t.Fatalf("error = %T %v, want *errs.AuthenticationError", err, err) + } + if !strings.Contains(authErr.Hint, tt.wantHint) || strings.Contains(authErr.Hint, tt.forbidden) { + t.Errorf("hint = %q, want %q and no %q", authErr.Hint, tt.wantHint, tt.forbidden) + } + }) + } +} + +func TestDoMCPCallHTTPBusinessErrorKeepsBotIdentity(t *testing.T) { + client := &http.Client{Transport: roundTripFunc(func(*http.Request) (*http.Response, error) { + return &http.Response{ + StatusCode: http.StatusForbidden, + Status: "403 Forbidden", + Body: io.NopCloser(strings.NewReader( + `{"code":99991679,"msg":"missing scope","error":{"permission_violations":[{"subject":"drive:file:read"}]}}`, + )), + }, nil + })} + + _, err := DoMCPCall(context.Background(), client, "fetch-doc", nil, "token", "https://example.com/mcp", true) + var permission *errs.PermissionError + if !errors.As(err, &permission) { + t.Fatalf("error = %T %v, want *errs.PermissionError", err, err) + } + if permission.Identity != "bot" || permission.Subtype != errs.SubtypeMissingScope || + len(permission.MissingScopes) != 1 || permission.MissingScopes[0] != "drive:file:read" { + t.Fatalf("permission error = %+v, want bot missing_scope with drive:file:read", permission) + } + if strings.Contains(permission.Hint, "auth login") || !strings.Contains(permission.Hint, "app developer") { + t.Errorf("bot hint = %q, want developer recovery without user OAuth", permission.Hint) + } +} + +func TestDoMCPCallHTTPUnknownBusinessErrorPreservesFallback(t *testing.T) { + client := &http.Client{Transport: roundTripFunc(func(*http.Request) (*http.Response, error) { + return &http.Response{ + StatusCode: http.StatusBadRequest, + Status: "400 Bad Request", + Body: io.NopCloser(strings.NewReader(`{"code":987654321,"msg":"unknown MCP failure"}`)), + }, nil + })} + + _, err := DoMCPCall(context.Background(), client, "fetch-doc", nil, "token", "https://example.com/mcp", true) + var apiErr *errs.APIError + if !errors.As(err, &apiErr) { + t.Fatalf("error = %T %v, want *errs.APIError", err, err) + } + if apiErr.Subtype != errs.SubtypeUnknown || apiErr.Code != 987654321 || + !strings.Contains(apiErr.Message, "MCP HTTP 400 400 Bad Request: [987654321] unknown MCP failure") { + t.Errorf("unknown MCP error fallback changed: %+v", apiErr) + } } func TestDoMCPCallSetsHeadersAndUnwrapsResult(t *testing.T) { diff --git a/shortcuts/common/runner.go b/shortcuts/common/runner.go index 9492669aa..e12224f0c 100644 --- a/shortcuts/common/runner.go +++ b/shortcuts/common/runner.go @@ -36,20 +36,21 @@ import ( // RuntimeContext provides helpers for shortcut execution. type RuntimeContext struct { - ctx context.Context // from cmd.Context(), propagated through the call chain - Config *core.CliConfig - Cmd *cobra.Command - Format string - JqExpr string // --jq expression; empty = no filter - outputErrOnce sync.Once // guards first-error capture in Out()/OutFormat() - outputErr error // deferred error from jq filtering; written at most once - botOnly bool // set by framework for bot-only shortcuts - resolvedAs core.Identity // effective identity resolved by framework - Factory *cmdutil.Factory // injected by framework - apiClientFunc func() (*client.APIClient, error) // sync.OnceValues; initialized in newRuntimeContext - botInfoFunc func() (*BotInfo, error) // sync.OnceValues; lazy bot identity from /bot/v3/info - larkSDK *lark.Client // eagerly initialized in mountDeclarative - stdinConsumed bool // set when an Input flag has consumed stdin (`-`); guards against a second flag also using `-` within the same call + ctx context.Context // from cmd.Context(), propagated through the call chain + Config *core.CliConfig + Cmd *cobra.Command + Format string + JqExpr string // --jq expression; empty = no filter + outputErrOnce sync.Once // guards first-error capture in Out()/OutFormat() + outputErr error // deferred error from jq filtering; written at most once + botOnly bool // set by framework for bot-only shortcuts + resolvedAs core.Identity // effective identity resolved by framework + declaredScopes []string // shortcut-declared scopes for the resolved identity + Factory *cmdutil.Factory // injected by framework + apiClientFunc func() (*client.APIClient, error) // sync.OnceValues; initialized in newRuntimeContext + botInfoFunc func() (*BotInfo, error) // sync.OnceValues; lazy bot identity from /bot/v3/info + larkSDK *lark.Client // eagerly initialized in mountDeclarative + stdinConsumed bool // set when an Input flag has consumed stdin (`-`); guards against a second flag also using `-` within the same call } // ── Identity ── @@ -70,6 +71,20 @@ func (ctx *RuntimeContext) As() core.Identity { return core.AsUser } +// PresentError renders a typed producer error for this command tree before a +// shortcut copies its fields into a result payload. +func (ctx *RuntimeContext) PresentError(err error) error { + if ctx == nil { + return err + } + return ctx.Factory.PresentError(err, cmdutil.ErrorPresentationOptions{ + Identity: ctx.As(), + DeclaredScopes: func() []string { + return slices.Clone(ctx.declaredScopes) + }, + }) +} + // IsBot returns true if current identity is bot. func (ctx *RuntimeContext) IsBot() bool { return ctx.As().IsBot() @@ -1011,7 +1026,15 @@ func checkShortcutScopes(f *cmdutil.Factory, ctx context.Context, as core.Identi func newRuntimeContext(cmd *cobra.Command, f *cmdutil.Factory, s *Shortcut, config *core.CliConfig, as core.Identity, botOnly bool) (*RuntimeContext, error) { ctx := cmd.Context() ctx = cmdutil.ContextWithShortcut(ctx, s.Service+":"+s.Command, uuid.New().String()) - rctx := &RuntimeContext{ctx: ctx, Config: config, Cmd: cmd, botOnly: botOnly, resolvedAs: as, Factory: f} + rctx := &RuntimeContext{ + ctx: ctx, + Config: config, + Cmd: cmd, + botOnly: botOnly, + resolvedAs: as, + Factory: f, + } + rctx.declaredScopes = s.DeclaredScopesForIdentity(string(rctx.As())) rctx.apiClientFunc = sync.OnceValues(func() (*client.APIClient, error) { return f.NewAPIClientWithConfig(config) }) diff --git a/shortcuts/common/runner_error_presenter_test.go b/shortcuts/common/runner_error_presenter_test.go new file mode 100644 index 000000000..d2839a3ef --- /dev/null +++ b/shortcuts/common/runner_error_presenter_test.go @@ -0,0 +1,137 @@ +// Copyright (c) 2026 Lark Technologies Pte. Ltd. +// SPDX-License-Identifier: MIT + +package common + +import ( + "context" + "errors" + "reflect" + "strings" + "testing" + + "github.com/larksuite/cli/errs" + "github.com/larksuite/cli/internal/core" + "github.com/larksuite/cli/internal/errclass" + "github.com/larksuite/cli/internal/recovery" + "github.com/larksuite/cli/internal/surface" +) + +func TestRuntimeContextPresentErrorUsesResolvedShortcutDeclaredScopes(t *testing.T) { + const ( + userScope = "calendar:calendar.event:read" + botScope = "calendar:calendar.event:read:bot" + ) + tests := []struct { + name string + identity string + concealed bool + wantScope string + }{ + {name: "user visible", identity: "user", wantScope: userScope}, + {name: "user concealed", identity: "user", concealed: true, wantScope: userScope}, + {name: "bot keeps bot recovery", identity: "bot"}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + f := newTestFactory() + var plan *surface.Plan + if tt.concealed { + plan = surface.NewPlan(map[surface.CommandID]surface.CommandState{ + surface.CommandAuthLogin: surface.CommandConcealed, + }) + f.Recovery = recovery.NewProjector(func() *surface.Plan { return plan }) + } + + var source *errs.PermissionError + var presented *errs.PermissionError + shortcut := &Shortcut{ + Service: "test", + Command: "+present-error", + AuthTypes: []string{"user", "bot"}, + ConditionalUserScopes: []string{userScope}, + ConditionalBotScopes: []string{botScope}, + Execute: func(_ context.Context, runtime *RuntimeContext) error { + err := errclass.BuildAPIError( + map[string]any{"code": 230027, "msg": "operation unauthorized"}, + errclass.ClassifyContext{Identity: string(runtime.As())}, + ) + if !errors.As(err, &source) { + t.Fatalf("source = %T, want *errs.PermissionError", err) + } + rendered := runtime.PresentError(err) + permission, ok := rendered.(*errs.PermissionError) + if !ok { + t.Fatalf("presented = %T, want *errs.PermissionError", rendered) + } + presented = permission + return nil + }, + } + cmd := newTestShortcutCmd(shortcut, f) + if err := cmd.Flags().Set("as", tt.identity); err != nil { + t.Fatal(err) + } + if err := runShortcut(cmd, f, shortcut, false); err != nil { + t.Fatalf("runShortcut() error = %v", err) + } + + if source == nil || presented == nil { + t.Fatal("shortcut did not present its error") + } + if source == presented || source.Identity != tt.identity || presented.Identity != tt.identity { + t.Fatalf("identity/clone mismatch: source=%+v presented=%+v", source, presented) + } + if len(source.MissingScopes) != 0 || len(presented.MissingScopes) != 0 { + t.Fatalf("presentation fabricated missing_scopes: source=%v presented=%v", source.MissingScopes, presented.MissingScopes) + } + + want := errclass.PermissionRecovery(nil, tt.identity, errs.SubtypeUserUnauthorized, "").Render(plan) + if tt.wantScope != "" { + want = errclass.PermissionRecovery([]string{tt.wantScope}, tt.identity, errs.SubtypeUserUnauthorized, "").Render(plan) + } + if presented.Hint != want { + t.Fatalf("presented recovery = %q, want %q", presented.Hint, want) + } + if tt.identity == "bot" { + if strings.Contains(presented.Hint, "auth login") || strings.Contains(presented.Hint, botScope) { + t.Fatalf("bot recovery used user/scoped OAuth guidance: %q", presented.Hint) + } + } else if tt.concealed { + if strings.Contains(presented.Hint, "auth login") || !strings.Contains(presented.Hint, userScope) { + t.Fatalf("concealed user recovery leaked command or lost scope: %q", presented.Hint) + } + } else if !strings.Contains(presented.Hint, `auth login --scope "`+userScope+`"`) { + t.Fatalf("visible user recovery did not use declared scope: %q", presented.Hint) + } + }) + } +} + +func TestNewRuntimeContextUsesEffectiveBotOnlyIdentityForDeclaredScopes(t *testing.T) { + shortcut := &Shortcut{ + Service: "test", + Command: "+bot-only", + AuthTypes: []string{"bot"}, + UserScopes: []string{"user:scope"}, + BotScopes: []string{"bot:scope"}, + Execute: func(context.Context, *RuntimeContext) error { return nil }, + } + f := newTestFactory() + config, err := f.Config() + if err != nil { + t.Fatal(err) + } + cmd := newTestShortcutCmd(shortcut, f) + runtime, err := newRuntimeContext(cmd, f, shortcut, config, core.AsUser, true) + if err != nil { + t.Fatalf("newRuntimeContext() error = %v", err) + } + if runtime.As() != core.AsBot { + t.Fatalf("runtime.As() = %q, want bot", runtime.As()) + } + if got, want := runtime.declaredScopes, []string{"bot:scope"}; !reflect.DeepEqual(got, want) { + t.Fatalf("declared scopes = %v, want effective bot scopes %v", got, want) + } +} diff --git a/shortcuts/common/skill_references.go b/shortcuts/common/skill_references.go new file mode 100644 index 000000000..0f7d43461 --- /dev/null +++ b/shortcuts/common/skill_references.go @@ -0,0 +1,42 @@ +// Copyright (c) 2026 Lark Technologies Pte. Ltd. +// SPDX-License-Identifier: MIT + +package common + +import ( + "github.com/larksuite/cli/internal/affordance" + "github.com/larksuite/cli/internal/cmdmeta" + "github.com/larksuite/cli/internal/meta" +) + +// ResolveAffordanceSkillReferences returns the current command's related skill +// references after applying this build's skill overlay, remaps, and command +// presentation. It is the execution-time counterpart of help rendering: an +// error producer can offer the same version-matched guidance without copying +// canonical paths or publishing a `skills read` command that this distribution +// cannot execute. +func (ctx *RuntimeContext) ResolveAffordanceSkillReferences() []string { + if ctx == nil || ctx.Cmd == nil || ctx.Factory == nil { + return nil + } + service, methodID, ok := cmdmeta.AffordanceRef(ctx.Cmd) + if !ok { + return nil + } + raw, ok := affordance.For(service, methodID) + if !ok { + return nil + } + parsed, ok := (meta.Method{Affordance: raw}).ParsedAffordance() + if !ok { + return nil + } + + resolved := make([]string, 0, len(parsed.Skills)) + for _, canonical := range parsed.Skills { + if ref, ok := ctx.Factory.ResolveSkillReference(canonical); ok { + resolved = append(resolved, ref) + } + } + return resolved +} diff --git a/shortcuts/doc/docs_create_test.go b/shortcuts/doc/docs_create_test.go index f320d8818..1c08fdd63 100644 --- a/shortcuts/doc/docs_create_test.go +++ b/shortcuts/doc/docs_create_test.go @@ -6,9 +6,11 @@ package doc import ( "bytes" "encoding/json" + "errors" "strings" "testing" + "github.com/larksuite/cli/errs" "github.com/larksuite/cli/internal/cmdutil" "github.com/larksuite/cli/internal/core" "github.com/larksuite/cli/internal/httpmock" @@ -281,6 +283,21 @@ func TestDocsCreateRejectsLegacyV1Flags(t *testing.T) { if err == nil { t.Fatal("expected legacy v1 flags to be rejected") } + problem, ok := errs.ProblemOf(err) + if !ok { + t.Fatalf("error = %T, want typed problem", err) + } + if problem.Category != errs.CategoryValidation || problem.Subtype != errs.SubtypeInvalidArgument { + t.Fatalf("problem = %s/%s, want validation/invalid_argument", problem.Category, problem.Subtype) + } + var validationErr *errs.ValidationError + if !errors.As(err, &validationErr) { + t.Fatalf("error = %T, want *errs.ValidationError", err) + } + if got, want := validationErr.Param, "--markdown"; got != want { + t.Fatalf("param = %q, want %q", got, want) + } + presented := problem.Message + "\n" + problem.Hint for _, want := range []string{ "docs +create is v2-only", "the old v1 interface has been shut down", @@ -288,7 +305,7 @@ func TestDocsCreateRejectsLegacyV1Flags(t *testing.T) { "--markdown -> use --content with --doc-format markdown", "lark-cli docs +create --help", } { - if !strings.Contains(err.Error(), want) { + if !strings.Contains(presented, want) { t.Fatalf("error missing %q: %v", want, err) } } diff --git a/shortcuts/doc/docs_fetch_v2_test.go b/shortcuts/doc/docs_fetch_v2_test.go index f04daceb9..2b3d9fa2c 100644 --- a/shortcuts/doc/docs_fetch_v2_test.go +++ b/shortcuts/doc/docs_fetch_v2_test.go @@ -935,8 +935,13 @@ func TestDocsFetchRejectsLegacyFlags(t *testing.T) { t.Fatal("expected v2-only validation error") } assertValidationContract(t, err, errs.SubtypeInvalidArgument, "--offset") + problem, ok := errs.ProblemOf(err) + if !ok { + t.Fatalf("error = %T, want typed problem", err) + } + presented := problem.Message + "\n" + problem.Hint for _, want := range tt.want { - if !strings.Contains(err.Error(), want) { + if !strings.Contains(presented, want) { t.Fatalf("error missing %q: %v", want, err) } } diff --git a/shortcuts/doc/docs_update_test.go b/shortcuts/doc/docs_update_test.go index f9e1e477c..aa6b5ea0d 100644 --- a/shortcuts/doc/docs_update_test.go +++ b/shortcuts/doc/docs_update_test.go @@ -235,8 +235,23 @@ func TestDocsUpdateRejectsLegacyFlags(t *testing.T) { if err == nil { t.Fatal("expected v2-only validation error") } + problem, ok := errs.ProblemOf(err) + if !ok { + t.Fatalf("error = %T, want typed problem", err) + } + if problem.Category != errs.CategoryValidation || problem.Subtype != errs.SubtypeInvalidArgument { + t.Fatalf("problem = %s/%s, want validation/invalid_argument", problem.Category, problem.Subtype) + } + var validationErr *errs.ValidationError + if !errors.As(err, &validationErr) { + t.Fatalf("error = %T, want *errs.ValidationError", err) + } + if got, want := validationErr.Param, "--mode"; got != want { + t.Fatalf("param = %q, want %q", got, want) + } + presented := problem.Message + "\n" + problem.Hint for _, want := range tt.want { - if !strings.Contains(err.Error(), want) { + if !strings.Contains(presented, want) { t.Fatalf("error missing %q: %v", want, err) } } diff --git a/shortcuts/doc/v2_only.go b/shortcuts/doc/v2_only.go index 83bafcec2..575032ceb 100644 --- a/shortcuts/doc/v2_only.go +++ b/shortcuts/doc/v2_only.go @@ -4,9 +4,11 @@ package doc import ( + "fmt" "strings" "github.com/larksuite/cli/errs" + "github.com/larksuite/cli/internal/recovery" "github.com/larksuite/cli/shortcuts/common" ) @@ -81,20 +83,36 @@ func validateDocsV2Only(runtime *common.RuntimeContext, shortcut string, legacyF if len(replacements) > 0 { detail += "; " + strings.Join(replacements, "; ") } - return docsV2OnlyError(shortcut, detail, used[0]) + return docsV2OnlyError(runtime, shortcut, detail, used[0]) } -func docsV2OnlyError(shortcut, detail, param string) error { +func docsV2OnlyError(runtime *common.RuntimeContext, shortcut, detail, param string) error { helpCommand := "lark-cli docs " + shortcut + " --help" err := errs.NewValidationError( errs.SubtypeInvalidArgument, - "docs %s is v2-only; %s. Run `%s` for the latest command flags", + "docs %s is v2-only; %s", shortcut, detail, - helpCommand, ) if param != "" { err = err.WithParam(param) } - return err + + parts := []recovery.Part{ + recovery.Text(fmt.Sprintf("run `%s` for the latest command flags", helpCommand)), + } + if runtime != nil { + if refs := runtime.ResolveAffordanceSkillReferences(); len(refs) > 0 { + commands := make([]string, 0, len(refs)) + for _, ref := range refs { + commands = append(commands, "`lark-cli skills read "+ref+"`") + } + parts = append(parts, recovery.Command( + recovery.TargetSkillsRead, + "read the version-matched embedded guidance before retrying: "+strings.Join(commands, ", ")+ + "; do not inspect another local SKILL.md copy", + )) + } + } + return recovery.Attach(err, recovery.Join("; ", parts...)) } diff --git a/shortcuts/doc/v2_only_test.go b/shortcuts/doc/v2_only_test.go index eb78e2ad8..06587c8de 100644 --- a/shortcuts/doc/v2_only_test.go +++ b/shortcuts/doc/v2_only_test.go @@ -4,9 +4,20 @@ package doc import ( + "context" + "errors" "strings" "testing" + "testing/fstest" + "github.com/larksuite/cli/errs" + "github.com/larksuite/cli/internal/affordance" + "github.com/larksuite/cli/internal/cmdmeta" + "github.com/larksuite/cli/internal/cmdutil" + "github.com/larksuite/cli/internal/core" + "github.com/larksuite/cli/internal/recovery" + "github.com/larksuite/cli/internal/skillref" + "github.com/larksuite/cli/internal/surface" "github.com/larksuite/cli/shortcuts/common" "github.com/spf13/cobra" ) @@ -23,21 +34,68 @@ func TestValidateDocsV2OnlyIgnoresAPIVersionValues(t *testing.T) { } func TestValidateDocsV2OnlyRejectsChangedLegacyFlags(t *testing.T) { - runtime := docsV2OnlyTestRuntime(t, "", true) + runtime := docsV2OnlyTestRuntimeWithSkills(t, true, nil, "lark-doc") err := validateDocsV2Only(runtime, "+update", []docsLegacyFlag{{Name: "mode", Replacement: "use --command"}}) if err == nil { t.Fatal("expected changed legacy flag to be rejected") } + problem, ok := errs.ProblemOf(err) + if !ok { + t.Fatalf("error = %T, want typed problem", err) + } + if problem.Category != errs.CategoryValidation || problem.Subtype != errs.SubtypeInvalidArgument { + t.Fatalf("problem = %s/%s, want validation/invalid_argument", problem.Category, problem.Subtype) + } + if got, want := problem.Message, "docs +update is v2-only; the old v1 interface has been shut down; legacy v1 flag(s) --mode are no longer supported; --mode -> use --command"; got != want { + t.Fatalf("message = %q, want %q", got, want) + } + var validationErr *errs.ValidationError + if !errors.As(err, &validationErr) { + t.Fatalf("error = %T, want *errs.ValidationError", err) + } + if got, want := validationErr.Param, "--mode"; got != want { + t.Fatalf("param = %q, want %q", got, want) + } + if got, want := problem.Hint, "run `lark-cli docs +update --help` for the latest command flags; read the version-matched embedded guidance before retrying: `lark-cli skills read lark-doc`, `lark-cli skills read lark-doc/references/lark-doc-update.md`, `lark-cli skills read lark-doc/references/lark-doc-xml.md`, `lark-cli skills read lark-doc/references/lark-doc-md.md`; do not inspect another local SKILL.md copy"; got != want { + t.Fatalf("hint = %q, want %q", got, want) + } +} + +func TestValidateDocsV2OnlyOmitsConcealedSkillsReadRecovery(t *testing.T) { + plan := surface.NewPlan(map[surface.CommandID]surface.CommandState{ + surface.CommandSkillsRead: surface.CommandConcealed, + }) + runtime := docsV2OnlyTestRuntimeWithSkills(t, true, plan, "lark-doc") + err := validateDocsV2Only(runtime, "+update", []docsLegacyFlag{{Name: "mode", Replacement: "use --command"}}) + problem, ok := errs.ProblemOf(err) + if !ok { + t.Fatalf("error = %T, want typed problem", err) + } + if got, want := problem.Hint, "run `lark-cli docs +update --help` for the latest command flags"; got != want { + t.Fatalf("hint = %q, want %q", got, want) + } +} + +func TestValidateDocsV2OnlyUsesRemappedSkillReferences(t *testing.T) { + runtime := docsV2OnlyTestRuntimeWithSkills(t, true, nil, "acme-doc") + err := validateDocsV2Only(runtime, "+update", []docsLegacyFlag{{Name: "mode", Replacement: "use --command"}}) + problem, ok := errs.ProblemOf(err) + if !ok { + t.Fatalf("error = %T, want typed problem", err) + } for _, want := range []string{ - "the old v1 interface has been shut down", - "legacy v1 flag(s) --mode are no longer supported", - "--mode -> use --command", - "lark-cli docs +update --help", + "lark-cli skills read acme-doc", + "lark-cli skills read acme-doc/references/lark-doc-update.md", + "lark-cli skills read acme-doc/references/lark-doc-xml.md", + "lark-cli skills read acme-doc/references/lark-doc-md.md", } { - if !strings.Contains(err.Error(), want) { - t.Fatalf("error missing %q: %v", want, err) + if !strings.Contains(problem.Hint, want) { + t.Fatalf("hint missing %q: %s", want, problem.Hint) } } + if strings.Contains(problem.Hint, "skills read lark-doc") { + t.Fatalf("hint retained canonical skill name after remap: %s", problem.Hint) + } } func docsV2OnlyTestRuntime(t *testing.T, apiVersion string, legacyMode bool) *common.RuntimeContext { @@ -58,3 +116,60 @@ func docsV2OnlyTestRuntime(t *testing.T, apiVersion string, legacyMode bool) *co } return common.TestNewRuntimeContext(cmd, nil) } + +func docsV2OnlyTestRuntimeWithSkills(t *testing.T, legacyMode bool, plan *surface.Plan, runtimeSkill string) *common.RuntimeContext { + t.Helper() + + cmd := &cobra.Command{Use: "+update"} + cmd.Flags().String("api-version", "", "") + cmd.Flags().String("mode", "", "") + if legacyMode { + if err := cmd.Flags().Set("mode", "overwrite"); err != nil { + t.Fatalf("set mode: %v", err) + } + } + cmdmeta.SetAffordanceRef(cmd, "docs", "+update") + affordance.SetSource(fstest.MapFS{ + "docs.md": {Data: []byte(`# docs +> skill: lark-doc + +## +update +### Skills +- lark-doc/references/lark-doc-update.md +- lark-doc/references/lark-doc-xml.md +- lark-doc/references/lark-doc-md.md +`)}, + }) + t.Cleanup(func() { affordance.SetSource(nil) }) + + content := fstest.MapFS{ + runtimeSkill + "/SKILL.md": {Data: []byte("skill")}, + runtimeSkill + "/references/lark-doc-update.md": {Data: []byte("update")}, + runtimeSkill + "/references/lark-doc-xml.md": {Data: []byte("xml")}, + runtimeSkill + "/references/lark-doc-md.md": {Data: []byte("markdown")}, + } + var mappings []skillref.Mapping + if runtimeSkill != "lark-doc" { + from, err := skillref.Parse("lark-doc") + if err != nil { + t.Fatal(err) + } + to, err := skillref.Parse(runtimeSkill) + if err != nil { + t.Fatal(err) + } + mappings = append(mappings, skillref.Mapping{From: from, To: to}) + } + resolver, err := skillref.New(content, mappings) + if err != nil { + t.Fatalf("skillref.New(): %v", err) + } + factory := &cmdutil.Factory{ + SkillContent: content, + SkillReferences: resolver, + Recovery: recovery.NewProjector(func() *surface.Plan { + return plan + }), + } + return common.TestNewRuntimeContextForAPI(context.Background(), cmd, &core.CliConfig{}, factory, core.AsUser) +} diff --git a/shortcuts/task/tasklist_add_task.go b/shortcuts/task/tasklist_add_task.go index 6f868b7ed..330baceec 100644 --- a/shortcuts/task/tasklist_add_task.go +++ b/shortcuts/task/tasklist_add_task.go @@ -73,17 +73,18 @@ var AddTaskToTasklist = common.Shortcut{ data, err := callTaskAPITyped(runtime, http.MethodPost, "/open-apis/task/v2/tasks/"+url.PathEscape(taskId)+"/add_tasklist", params, body) if err != nil { + presented := runtime.PresentError(err) failDetail := map[string]interface{}{ "guid": taskId, } - if p, ok := errs.ProblemOf(err); ok { + if p, ok := errs.ProblemOf(presented); ok { failDetail["type"] = string(p.Subtype) failDetail["code"] = p.Code failDetail["message"] = p.Message failDetail["hint"] = p.Hint } else { failDetail["type"] = "api_error" - failDetail["message"] = err.Error() + failDetail["message"] = presented.Error() } failed = append(failed, failDetail) } else { diff --git a/shortcuts/task/tasklist_add_task_test.go b/shortcuts/task/tasklist_add_task_test.go index 5a19c6d23..060089850 100644 --- a/shortcuts/task/tasklist_add_task_test.go +++ b/shortcuts/task/tasklist_add_task_test.go @@ -4,6 +4,7 @@ package task import ( + "encoding/json" "errors" "strings" "testing" @@ -11,8 +12,79 @@ import ( "github.com/larksuite/cli/errs" "github.com/larksuite/cli/internal/httpmock" "github.com/larksuite/cli/internal/output" + "github.com/larksuite/cli/internal/recovery" + "github.com/larksuite/cli/internal/surface" ) +func TestAddTaskToTasklist_UserMissingScopeProjectsInlineHint(t *testing.T) { + tests := []struct { + name string + plan *surface.Plan + }{ + {name: "visible"}, + { + name: "concealed", + plan: surface.NewPlan(map[surface.CommandID]surface.CommandState{ + surface.CommandAuthLogin: surface.CommandConcealed, + }), + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + f, stdout, _, reg := taskShortcutTestFactory(t) + f.Recovery = recovery.NewProjector(func() *surface.Plan { return tt.plan }) + reg.Register(&httpmock.Stub{ + Method: "POST", + URL: "/open-apis/task/v2/tasks/task-scope/add_tasklist", + Body: map[string]interface{}{ + "code": 99991679, + "msg": "missing scope", + "error": map[string]interface{}{ + "permission_violations": []interface{}{ + map[string]interface{}{"subject": "task:task:write"}, + }, + }, + }, + }) + + s := AddTaskToTasklist + s.AuthTypes = []string{"bot", "user"} + err := runMountedTaskShortcut(t, s, []string{ + "+tasklist-task-add", "--tasklist-id", "tl-123", "--task-id", "task-scope", + "--as", "user", "--format", "json", + }, f, stdout) + var partial *output.PartialFailureError + if !errors.As(err, &partial) { + t.Fatalf("err = %T, want *output.PartialFailureError: %v", err, err) + } + + var envelope struct { + OK bool `json:"ok"` + Data struct { + Failed []map[string]interface{} `json:"failed_tasks"` + } `json:"data"` + } + if err := json.Unmarshal(stdout.Bytes(), &envelope); err != nil { + t.Fatalf("unmarshal stdout: %v\n%s", err, stdout.String()) + } + if envelope.OK || len(envelope.Data.Failed) != 1 { + t.Fatalf("envelope = %#v, want ok:false with one failure", envelope) + } + failed := envelope.Data.Failed[0] + if got, want := failed["type"], string(errs.SubtypeMissingScope); got != want { + t.Errorf("failed type = %v, want %v", got, want) + } + if got, want := failed["hint"], recovery.UserAuthorization("task:task:write").Render(tt.plan); got != want { + t.Errorf("failed hint = %q, want %q", got, want) + } + if tt.plan != nil && strings.Contains(failed["hint"].(string), "auth login") { + t.Errorf("concealed hint leaked auth command: %q", failed["hint"]) + } + }) + } +} + func TestAddTaskToTasklist_Success(t *testing.T) { f, stdout, _, reg := taskShortcutTestFactory(t) warmTenantToken(t, f, reg) diff --git a/shortcuts/task/tasklist_create.go b/shortcuts/task/tasklist_create.go index 0923bba49..3360aec82 100644 --- a/shortcuts/task/tasklist_create.go +++ b/shortcuts/task/tasklist_create.go @@ -115,7 +115,7 @@ var CreateTasklist = common.Shortcut{ if tErr != nil { summary, _ := tDef["summary"].(string) - failedTasks = append(failedTasks, buildTaskCreateFailure(idx, summary, tErr)) + failedTasks = append(failedTasks, buildTaskCreateFailure(runtime, idx, summary, tErr)) return } @@ -186,19 +186,20 @@ var CreateTasklist = common.Shortcut{ }, } -func buildTaskCreateFailure(index int, summary string, err error) map[string]interface{} { +func buildTaskCreateFailure(runtime *common.RuntimeContext, index int, summary string, err error) map[string]interface{} { + presented := runtime.PresentError(err) failDetail := map[string]interface{}{ "index": index, "summary": summary, } - if p, ok := errs.ProblemOf(err); ok { + if p, ok := errs.ProblemOf(presented); ok { failDetail["type"] = string(p.Subtype) failDetail["code"] = p.Code failDetail["message"] = p.Message failDetail["hint"] = p.Hint } else { failDetail["type"] = "api_error" - failDetail["message"] = err.Error() + failDetail["message"] = presented.Error() } return failDetail } diff --git a/shortcuts/task/tasklist_create_test.go b/shortcuts/task/tasklist_create_test.go index 16abe4b7c..205f3f4d1 100644 --- a/shortcuts/task/tasklist_create_test.go +++ b/shortcuts/task/tasklist_create_test.go @@ -5,6 +5,7 @@ package task import ( "bytes" + "encoding/json" "errors" "strings" "testing" @@ -12,8 +13,91 @@ import ( "github.com/larksuite/cli/errs" "github.com/larksuite/cli/internal/httpmock" "github.com/larksuite/cli/internal/output" + "github.com/larksuite/cli/internal/recovery" + "github.com/larksuite/cli/internal/surface" ) +func TestCreateTasklist_UserMissingScopeProjectsInlineHint(t *testing.T) { + tests := []struct { + name string + plan *surface.Plan + }{ + {name: "visible"}, + { + name: "concealed", + plan: surface.NewPlan(map[surface.CommandID]surface.CommandState{ + surface.CommandAuthLogin: surface.CommandConcealed, + }), + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + f, stdout, _, reg := taskShortcutTestFactory(t) + f.Recovery = recovery.NewProjector(func() *surface.Plan { return tt.plan }) + reg.Register(&httpmock.Stub{ + Method: "POST", + URL: "/open-apis/task/v2/tasklists", + Body: map[string]interface{}{ + "code": 0, + "msg": "success", + "data": map[string]interface{}{ + "tasklist": map[string]interface{}{"guid": "tl-new", "name": "My List"}, + }, + }, + }) + reg.Register(&httpmock.Stub{ + Method: "POST", + URL: "/open-apis/task/v2/tasks", + BodyFilter: func(body []byte) bool { return bytes.Contains(body, []byte("bad-task")) }, + Body: map[string]interface{}{ + "code": 99991679, + "msg": "missing scope", + "error": map[string]interface{}{ + "permission_violations": []interface{}{ + map[string]interface{}{"subject": "task:task:write"}, + }, + }, + }, + }) + + s := CreateTasklist + s.AuthTypes = []string{"bot", "user"} + err := runMountedTaskShortcut(t, s, []string{ + "+tasklist-create", "--name", "My List", "--data", `[{"summary":"bad-task"}]`, + "--as", "user", "--format", "json", + }, f, stdout) + var partial *output.PartialFailureError + if !errors.As(err, &partial) { + t.Fatalf("err = %T, want *output.PartialFailureError: %v", err, err) + } + + var envelope struct { + OK bool `json:"ok"` + Data struct { + Failed []map[string]interface{} `json:"failed_tasks"` + } `json:"data"` + } + if err := json.Unmarshal(stdout.Bytes(), &envelope); err != nil { + t.Fatalf("unmarshal stdout: %v\n%s", err, stdout.String()) + } + if envelope.OK || len(envelope.Data.Failed) != 1 { + t.Fatalf("envelope = %#v, want ok:false with one failure", envelope) + } + failed := envelope.Data.Failed[0] + if got, want := failed["type"], string(errs.SubtypeMissingScope); got != want { + t.Errorf("failed type = %v, want %v", got, want) + } + if got, want := failed["hint"], recovery.UserAuthorization("task:task:write").Render(tt.plan); got != want { + t.Errorf("failed hint = %q, want %q", got, want) + } + if tt.plan != nil && strings.Contains(failed["hint"].(string), "auth login") { + t.Errorf("concealed hint leaked auth command: %q", failed["hint"]) + } + }) + } +} + // TestCreateTasklist_PartialFailure exercises the batch sub-task path: the // tasklist is created (code 0), then two sub-tasks are created concurrently — // one succeeds, one fails with a typed API error. The command returns the typed @@ -164,7 +248,7 @@ func TestCreateTasklist_PartialFailurePrettyOutput(t *testing.T) { "Failed tasks:", "Index", "bad-task", - "user lacks permission", + "bot lacks permission", } { if !strings.Contains(out, want) { t.Errorf("pretty output missing %q; got:\n%s", want, out) diff --git a/shortcuts/vc/vc_calendar_event_recovery_test.go b/shortcuts/vc/vc_calendar_event_recovery_test.go new file mode 100644 index 000000000..562e236f6 --- /dev/null +++ b/shortcuts/vc/vc_calendar_event_recovery_test.go @@ -0,0 +1,156 @@ +// Copyright (c) 2026 Lark Technologies Pte. Ltd. +// SPDX-License-Identifier: MIT + +package vc + +import ( + "encoding/json" + "errors" + "strings" + "testing" + "time" + + keyring "github.com/zalando/go-keyring" + + "github.com/larksuite/cli/internal/auth" + "github.com/larksuite/cli/internal/cmdutil" + "github.com/larksuite/cli/internal/httpmock" + "github.com/larksuite/cli/internal/output" + "github.com/larksuite/cli/internal/recovery" + "github.com/larksuite/cli/internal/surface" + "github.com/larksuite/cli/shortcuts/common" +) + +func TestCalendarEventResolutionMissingScopeProjectsInlineHint(t *testing.T) { + // Prime the package test credential cache before subtests isolate keyring and + // data directories for their real stored-user-token setup. + warmTokenCache(t) + + const ( + calendarID = "cal_scope" + instanceID = "evt_scope" + missingScope = "calendar:calendar.event:read" + wantError = "unauthorized: user authorization does not cover the required scope(s): " + missingScope + ) + + sinks := []struct { + name string + shortcut common.Shortcut + command string + resultKey string + }{ + {name: "notes", shortcut: VCNotes, command: "+notes", resultKey: "notes"}, + {name: "recording", shortcut: VCRecording, command: "+recording", resultKey: "recordings"}, + } + projections := []struct { + name string + plan *surface.Plan + }{ + {name: "visible"}, + { + name: "concealed", + plan: surface.NewPlan(map[surface.CommandID]surface.CommandState{ + surface.CommandAuthLogin: surface.CommandConcealed, + }), + }, + } + + for _, sink := range sinks { + for _, projection := range projections { + t.Run(sink.name+"/"+projection.name, func(t *testing.T) { + keyring.MockInit() + t.Setenv("HOME", t.TempDir()) + t.Setenv("LARKSUITE_CLI_DATA_DIR", t.TempDir()) + t.Setenv("LARKSUITE_CLI_CONFIG_DIR", t.TempDir()) + + cfg := defaultConfig() + now := time.Now() + stored := &auth.StoredUAToken{ + UserOpenId: cfg.UserOpenId, + AppId: cfg.AppID, + AccessToken: "test-user-access-token", + RefreshToken: "test-refresh-token", + ExpiresAt: now.Add(time.Hour).UnixMilli(), + RefreshExpiresAt: now.Add(24 * time.Hour).UnixMilli(), + GrantedAt: now.Add(-time.Hour).UnixMilli(), + Scope: strings.Join([]string{ + "vc:note:read", + "vc:record:readonly", + "vc:meeting.meetingevent:read", + "calendar:calendar:read", + missingScope, + }, " "), + } + if err := auth.SetStoredToken(stored); err != nil { + t.Fatalf("SetStoredToken() error = %v", err) + } + t.Cleanup(func() { _ = auth.RemoveStoredToken(cfg.AppID, cfg.UserOpenId) }) + + f, stdout, _, registry := cmdutil.TestFactory(t, cfg) + f.Recovery = recovery.NewProjector(func() *surface.Plan { return projection.plan }) + registry.Register(primaryCalendarStub(calendarID)) + registry.Register(&httpmock.Stub{ + Method: "POST", + URL: "/open-apis/calendar/v4/calendars/" + calendarID + "/events/mget_instance_relation_info", + Body: map[string]interface{}{ + "code": 99991679, + "msg": "missing scope", + "error": map[string]interface{}{ + "permission_violations": []interface{}{ + map[string]interface{}{"subject": missingScope}, + }, + }, + }, + }) + + err := mountAndRun(t, sink.shortcut, []string{ + sink.command, + "--calendar-event-ids", instanceID, + "--as", "user", + "--format", "json", + }, f, stdout) + var partial *output.PartialFailureError + if !errors.As(err, &partial) || partial.Code != output.ExitAPI { + t.Fatalf("exit error = %T %v, want PartialFailureError(%d)", err, err, output.ExitAPI) + } + registry.Verify(t) + + var envelope struct { + OK bool `json:"ok"` + Data map[string]interface{} `json:"data"` + } + if err := json.Unmarshal(stdout.Bytes(), &envelope); err != nil { + t.Fatalf("unmarshal stdout: %v\n%s", err, stdout.String()) + } + if envelope.OK { + t.Fatalf("envelope.ok = true, want false: %s", stdout.String()) + } + items, ok := envelope.Data[sink.resultKey].([]interface{}) + if !ok || len(items) != 1 { + t.Fatalf("data.%s = %#v, want one result", sink.resultKey, envelope.Data[sink.resultKey]) + } + result, ok := items[0].(map[string]interface{}) + if !ok { + t.Fatalf("result = %T, want object", items[0]) + } + if got := result["calendar_event_id"]; got != instanceID { + t.Errorf("calendar_event_id = %#v, want %q", got, instanceID) + } + if got := result["error"]; got != wantError { + t.Errorf("error = %#v, want unchanged %q", got, wantError) + } + hint, ok := result["hint"].(string) + if !ok { + t.Fatalf("hint = %#v, want string", result["hint"]) + } + wantHint := recovery.UserAuthorization(missingScope).Render(projection.plan) + if hint != wantHint { + t.Errorf("hint = %q, want %q", hint, wantHint) + } + if projection.plan != nil && strings.Contains(hint, "auth login") { + t.Errorf("concealed hint leaked auth command: %q", hint) + } + }) + } + } +} diff --git a/shortcuts/vc/vc_notes.go b/shortcuts/vc/vc_notes.go index d5d4a4372..3c36c01ad 100644 --- a/shortcuts/vc/vc_notes.go +++ b/shortcuts/vc/vc_notes.go @@ -170,6 +170,19 @@ func resolveMeetingIDsFromCalendarEvent(runtime *common.RuntimeContext, instance return result, nil } +// calendarEventResolutionFailure preserves the established calendar_event_id +// and error fields while presenting the typed error before it is embedded in a +// partial-result envelope. Existing messages remain byte-compatible, while an +// additive hint receives centralized command-surface projection. +func calendarEventResolutionFailure(runtime *common.RuntimeContext, instanceID string, err error) map[string]any { + presented := runtime.PresentError(err) + result := map[string]any{"calendar_event_id": instanceID, "error": presented.Error()} + if problem, ok := errs.ProblemOf(presented); ok && problem.Hint != "" { + result["hint"] = problem.Hint + } + return result +} + // extractStringSlice extracts a []string from a JSON array field in a map. func extractStringSlice(m map[string]any, key string) []string { raw, _ := m[key].([]any) @@ -191,7 +204,7 @@ func fetchNoteByCalendarEventID(ctx context.Context, runtime *common.RuntimeCont relInfo, err := resolveMeetingIDsFromCalendarEvent(runtime, instanceID, calendarID, true) if err != nil { - return map[string]any{"calendar_event_id": instanceID, "error": err.Error()} + return calendarEventResolutionFailure(runtime, instanceID, err) } result := map[string]any{"calendar_event_id": instanceID} @@ -382,8 +395,9 @@ func fetchNoteByMinuteToken(ctx context.Context, runtime *common.RuntimeContext, data, err := runtime.CallAPITyped(http.MethodGet, fmt.Sprintf("/open-apis/minutes/v1/minutes/%s", validate.EncodePathSegment(minuteToken)), nil, nil) if err != nil { err = minutesReadError(err, minuteToken) - result := map[string]any{"minute_token": minuteToken, "error": err.Error()} - if p, ok := errs.ProblemOf(err); ok && p.Hint != "" { + presented := runtime.PresentError(err) + result := map[string]any{"minute_token": minuteToken, "error": presented.Error()} + if p, ok := errs.ProblemOf(presented); ok && p.Hint != "" { result["hint"] = p.Hint } return result diff --git a/shortcuts/vc/vc_notes_test.go b/shortcuts/vc/vc_notes_test.go index d87ae13ad..6618562ea 100644 --- a/shortcuts/vc/vc_notes_test.go +++ b/shortcuts/vc/vc_notes_test.go @@ -22,10 +22,76 @@ import ( "github.com/larksuite/cli/internal/core" "github.com/larksuite/cli/internal/httpmock" "github.com/larksuite/cli/internal/output" + "github.com/larksuite/cli/internal/recovery" + "github.com/larksuite/cli/internal/surface" "github.com/larksuite/cli/shortcuts/common" "github.com/larksuite/cli/shortcuts/note" ) +func TestNotes_UserMissingScopeProjectsInlineHintWithoutChangingExit(t *testing.T) { + tests := []struct { + name string + plan *surface.Plan + }{ + {name: "visible"}, + { + name: "concealed", + plan: surface.NewPlan(map[surface.CommandID]surface.CommandState{ + surface.CommandAuthLogin: surface.CommandConcealed, + }), + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Setenv("LARKSUITE_CLI_CONFIG_DIR", t.TempDir()) + f, stdout, _, reg := cmdutil.TestFactory(t, defaultConfig()) + f.Recovery = recovery.NewProjector(func() *surface.Plan { return tt.plan }) + reg.Register(&httpmock.Stub{ + Method: "GET", + URL: "/open-apis/minutes/v1/minutes/tokscope", + Body: map[string]interface{}{ + "code": 99991679, + "msg": "missing scope", + "error": map[string]interface{}{ + "permission_violations": []interface{}{ + map[string]interface{}{"subject": "minutes:minutes:readonly"}, + }, + }, + }, + }) + + // minute_token itself remains a usable routing payload, so preserve the + // command's established ok:true / nil-error behavior. + if err := mountAndRun(t, VCNotes, []string{ + "+notes", "--minute-tokens", "tokscope", "--as", "user", "--format", "json", + }, f, stdout); err != nil { + t.Fatalf("unexpected exit behavior change: %v", err) + } + + var envelope struct { + OK bool `json:"ok"` + Data struct { + Notes []map[string]interface{} `json:"notes"` + } `json:"data"` + } + if err := json.Unmarshal(stdout.Bytes(), &envelope); err != nil { + t.Fatalf("unmarshal stdout: %v\n%s", err, stdout.String()) + } + if !envelope.OK || len(envelope.Data.Notes) != 1 { + t.Fatalf("envelope = %#v, want ok:true with one note", envelope) + } + note := envelope.Data.Notes[0] + if got, want := note["hint"], recovery.UserAuthorization("minutes:minutes:readonly").Render(tt.plan); got != want { + t.Errorf("note hint = %q, want %q", got, want) + } + if tt.plan != nil && strings.Contains(note["hint"].(string), "auth login") { + t.Errorf("concealed hint leaked auth command: %q", note["hint"]) + } + }) + } +} + // --------------------------------------------------------------------------- // helpers // --------------------------------------------------------------------------- diff --git a/shortcuts/vc/vc_recording.go b/shortcuts/vc/vc_recording.go index 173fbe29a..67384493c 100644 --- a/shortcuts/vc/vc_recording.go +++ b/shortcuts/vc/vc_recording.go @@ -183,7 +183,7 @@ var VCRecording = common.Shortcut{ fmt.Fprintf(errOut, "%s resolving calendar_event_id=%s ...\n", recordingLogPrefix, sanitizeLogValue(instanceID)) relInfo, resolveErr := resolveMeetingIDsFromCalendarEvent(runtime, instanceID, calendarID, false) if resolveErr != nil { - results = append(results, map[string]any{"calendar_event_id": instanceID, "error": resolveErr.Error()}) + results = append(results, calendarEventResolutionFailure(runtime, instanceID, resolveErr)) continue } found := false diff --git a/skills/lark-doc/SKILL.md b/skills/lark-doc/SKILL.md index 478be8bb9..dba530dc3 100644 --- a/skills/lark-doc/SKILL.md +++ b/skills/lark-doc/SKILL.md @@ -5,6 +5,7 @@ description: "飞书云文档（Docx / Wiki 文档）：读取和编辑飞书文 metadata: requires: bins: ["lark-cli"] + skills: ["lark-shared"] cliHelp: "lark-cli docs --help;lark-cli mindnotes --help" --- diff --git a/skills/lark-shared/SKILL.md b/skills/lark-shared/SKILL.md index 5b1c94e24..527b84bd9 100644 --- a/skills/lark-shared/SKILL.md +++ b/skills/lark-shared/SKILL.md @@ -80,8 +80,8 @@ LARKSUITE_CLI_NO_UPDATE_NOTIFIER=1 LARKSUITE_CLI_NO_SKILLS_NOTIFIER=1 lark-cli a #### User 身份（`--as user`） ```bash -lark-cli auth login --domain # 按业务域授权 -lark-cli auth login --scope "" # 按具体 scope 授权（推荐,符合最小权限原则） +lark-cli auth login --domain --no-wait --json # 按业务域发起授权 +lark-cli auth login --scope "" --no-wait --json # 按具体 scope 发起授权（推荐，符合最小权限原则） ``` **规则**：auth login 必须指定范围（`--domain` 或 `--scope`）。多次 login 的 scope 会累积（增量授权）。 @@ -124,7 +124,7 @@ lark-cli auth login --device-code - **你必须亲自执行 `--device-code` 命令**，不要指示用户自行执行 - **不要在同一轮中展示 URL 后立刻执行 `--device-code`**，这会导致用户看不到 URL -- **禁止缓存 `verification_url` 或 `device_code`**：每次需要授权时，必须重新执行 `lark-cli auth login --no-wait --json` 生成新的链接。不要将授权链接和 device code 存入上下文供后续复用 +- **禁止缓存 `verification_url` 或 `device_code`**：每次需要重新发起授权时，必须沿用所需的 `--scope`、`--domain` 或 `--recommend` 选择以及任何 `--exclude` 值，并附加 `--no-wait --json` 生成新的链接。不要复用已过期的授权链接或 device code ## 更新检查 diff --git a/tests/cli_e2e/docs/docs_update_dryrun_test.go b/tests/cli_e2e/docs/docs_update_dryrun_test.go index 5219fa9f5..9dcc69d88 100644 --- a/tests/cli_e2e/docs/docs_update_dryrun_test.go +++ b/tests/cli_e2e/docs/docs_update_dryrun_test.go @@ -11,6 +11,7 @@ import ( clie2e "github.com/larksuite/cli/tests/cli_e2e" "github.com/stretchr/testify/require" + "github.com/tidwall/gjson" ) func TestDocs_DryRunDefaultsToV2OpenAPI(t *testing.T) { @@ -225,3 +226,36 @@ func TestDocs_CreateTitleDryRunPrependsContent(t *testing.T) { require.Equal(t, "markdown", clie2e.DryRunGet(out, "api.0.body.format").String(), "stdout:\n%s", out) require.Equal(t, "Dry Run & Title\n## Body", clie2e.DryRunGet(out, "api.0.body.content").String(), "stdout:\n%s", out) } + +func TestDocsUpdateDryRunLegacyFlagReturnsCurrentEmbeddedGuidance(t *testing.T) { + t.Setenv("LARKSUITE_CLI_CONFIG_DIR", t.TempDir()) + t.Setenv("LARKSUITE_CLI_APP_ID", "app") + t.Setenv("LARKSUITE_CLI_APP_SECRET", "secret") + t.Setenv("LARKSUITE_CLI_BRAND", "feishu") + + ctx, cancel := context.WithTimeout(context.Background(), 30*time.Second) + t.Cleanup(cancel) + + result, err := clie2e.RunCmd(ctx, clie2e.Request{ + Args: []string{ + "docs", "+update", + "--doc", "doxcnDryRunE2E", + "--mode", "overwrite", + "--content", "

hello

", + "--dry-run", + }, + DefaultAs: "bot", + }) + require.NoError(t, err) + result.AssertExitCode(t, 2) + require.Empty(t, result.Stdout, "validate-stage failure must not write to stdout") + + require.Equal(t, "validation", gjson.Get(result.Stderr, "error.type").String(), result.Stderr) + require.Equal(t, "invalid_argument", gjson.Get(result.Stderr, "error.subtype").String(), result.Stderr) + require.Equal(t, "--mode", gjson.Get(result.Stderr, "error.param").String(), result.Stderr) + require.Equal(t, + "run `lark-cli docs +update --help` for the latest command flags; read the version-matched embedded guidance before retrying: `lark-cli skills read lark-doc`, `lark-cli skills read lark-doc/references/lark-doc-update.md`, `lark-cli skills read lark-doc/references/lark-doc-xml.md`, `lark-cli skills read lark-doc/references/lark-doc-md.md`; do not inspect another local SKILL.md copy", + gjson.Get(result.Stderr, "error.hint").String(), + result.Stderr, + ) +} diff --git a/tests/plugin_e2e/harness.go b/tests/plugin_e2e/harness.go index eeb954c7f..3024fb92f 100644 --- a/tests/plugin_e2e/harness.go +++ b/tests/plugin_e2e/harness.go @@ -32,6 +32,8 @@ import ( "sync" "testing" "time" + + "github.com/larksuite/cli/internal/vfs" ) // cleanTree is the git-archived, committed-only source tree of the repo under @@ -128,12 +130,15 @@ func buildForkWithMain(t *testing.T, name, pluginSrc, mainSrc string) string { if err := os.MkdirAll(filepath.Join(mod, "plugin"), 0o755); err != nil { t.Fatalf("mkdir customer module: %v", err) } - for _, name := range []string{"lark-a", "lark-b", "lark-doc"} { - if err := os.MkdirAll(filepath.Join(mod, "skills", name), 0o755); err != nil { + for _, name := range []string{"lark-a", "lark-b", "lark-doc", "lark-shared"} { + if err := vfs.MkdirAll(filepath.Join(mod, "skills", name), 0o755); err != nil { t.Fatalf("mkdir customer skill %q: %v", name, err) } - writeFile(t, filepath.Join(mod, "skills", name, "SKILL.md"), - "---\nname: "+name+"\ndescription: plugin e2e base skill\n---\n") + skillMD := "---\nname: " + name + "\ndescription: plugin e2e base skill\n---\n" + if name == "lark-doc" { + skillMD = "---\nname: lark-doc\ndescription: plugin e2e base skill\nmetadata:\n requires:\n skills: [\"lark-shared\"]\n---\n" + } + writeFile(t, filepath.Join(mod, "skills", name, "SKILL.md"), skillMD) } if err := os.MkdirAll(filepath.Join(mod, "skills", "lark-doc", "references"), 0o755); err != nil { t.Fatalf("mkdir customer lark-doc references: %v", err) diff --git a/tests/plugin_e2e/restrict_test.go b/tests/plugin_e2e/restrict_test.go index dcab210f9..afcb56904 100644 --- a/tests/plugin_e2e/restrict_test.go +++ b/tests/plugin_e2e/restrict_test.go @@ -170,6 +170,101 @@ func TestConcealedForkProjectsSchemaFromGeneratedMethodHelp(t *testing.T) { } } +// TestConcealedForkProjectsRetainedSchemaCatalog pins the cross-surface case +// that a schema command retained by the distribution must not enumerate a +// generated service subtree concealed by the same build. This differs from +// TestConcealedForkProjectsSchemaFromGeneratedMethodHelp: that test conceals +// schema itself, while this one keeps schema executable and conceals only +// mail/**. +func TestConcealedForkProjectsRetainedSchemaCatalog(t *testing.T) { + bin := buildConcealedFork(t, "concealed-schema-mail", schemaMailConcealPlugin) + + hidden := runWithSeededCatalog( + t, + bin, + schemaProjectionCatalogJSON, + "schema", "mail.user_mailbox.messages.get", + ) + if hidden.exit != 2 || !gjson.Valid(hidden.stderr) { + t.Fatalf("concealed schema lookup exit=%d stdout=%s stderr=%s", hidden.exit, hidden.stdout, hidden.stderr) + } + if got := gjson.Get(hidden.stderr, "error.subtype").String(); got != "invalid_argument" { + t.Errorf("concealed schema subtype=%q want invalid_argument; stderr=%s", got, hidden.stderr) + } + if strings.Contains(hidden.stdout+hidden.stderr, "hidden mail schema method") { + t.Errorf("concealed exact lookup exposed method metadata: stdout=%s stderr=%s", hidden.stdout, hidden.stderr) + } + + broad := runWithSeededCatalog(t, bin, schemaProjectionCatalogJSON, "schema") + if broad.exit != 0 || !gjson.Valid(broad.stdout) { + t.Fatalf("broad schema exit=%d stdout=%s stderr=%s", broad.exit, broad.stdout, broad.stderr) + } + if strings.Contains(broad.stdout, "mail user_mailbox.messages get") || strings.Contains(broad.stdout, "hidden mail schema method") { + t.Errorf("broad schema exposed concealed mail method: %s", broad.stdout) + } + if !strings.Contains(broad.stdout, "im widgets get") { + t.Errorf("broad schema lost visible im method: %s", broad.stdout) + } + + visible := runWithSeededCatalog( + t, + bin, + schemaProjectionCatalogJSON, + "schema", "im.widgets.get", + ) + if visible.exit != 0 || !strings.Contains(visible.stdout, "im widgets get") { + t.Fatalf("visible schema lookup exit=%d stdout=%s stderr=%s", visible.exit, visible.stdout, visible.stderr) + } + + completionCases := []struct { + name string + args []string + concealed string + visible string + }{ + { + name: "dotted service", + args: []string{"__complete", "schema", ""}, + concealed: "mail.", + visible: "im.", + }, + { + name: "dotted descendant", + args: []string{"__complete", "schema", "mail."}, + concealed: "mail.user_mailbox.messages.", + }, + { + name: "space descendant", + args: []string{"__complete", "schema", "mail", ""}, + concealed: "user_mailbox.messages", + }, + { + name: "visible dotted descendant", + args: []string{"__complete", "schema", "im."}, + visible: "im.widgets.", + }, + { + name: "visible space descendant", + args: []string{"__complete", "schema", "im", ""}, + visible: "widgets", + }, + } + for _, tc := range completionCases { + t.Run(tc.name, func(t *testing.T) { + res := runWithSeededCatalog(t, bin, schemaProjectionCatalogJSON, tc.args...) + if res.exit != 0 { + t.Fatalf("completion exit=%d stdout=%s stderr=%s", res.exit, res.stdout, res.stderr) + } + if tc.concealed != "" && strings.Contains(res.stdout, tc.concealed) { + t.Errorf("completion exposed concealed candidate %q: %s", tc.concealed, res.stdout) + } + if tc.visible != "" && !strings.Contains(res.stdout, tc.visible) { + t.Errorf("completion lost visible candidate %q: %s", tc.visible, res.stdout) + } + }) + } +} + func TestConcealedForkHelpRejectsDescendantOfConcealedParent(t *testing.T) { bin := buildConcealedFork(t, "concealed-readonly", readonlyPlugin) assertUnavailableEnvelope(t, run(t, bin, "help", "auth", "login")) @@ -357,6 +452,82 @@ func init() { } ` +// schemaMailConcealPlugin deliberately keeps the schema tool while concealing +// only the generated mail subtree. It is the minimal distribution shape that +// catches schema escaping the build-local command surface. +const schemaMailConcealPlugin = `// Code generated by plugin_e2e; DO NOT EDIT. +package plugin + +import "github.com/larksuite/cli/extension/platform" + +func init() { + platform.Register( + platform.NewPlugin("schema-mail-conceal", "0.1.0"). + Restrict(&platform.Rule{ + Name: "retain-schema-hide-mail", + Allow: []string{"schema", "mail", "mail/**", "im", "im/**"}, + Deny: []string{"mail/**"}, + MaxRisk: platform.RiskHighRiskWrite, + AllowUnannotated: true, + }). + MustBuild()) +} +` + +const schemaProjectionCatalogJSON = `{ + "version": "9.9.9", + "services": [ + { + "name": "mail", + "version": "v1", + "title": "mail projection fixture", + "description": "mail service concealed from schema", + "servicePath": "/open-apis/mail/v1", + "resources": { + "user_mailbox.messages": { + "methods": { + "get": { + "id": "mail.user_mailbox.messages.get", + "path": "/open-apis/mail/v1/user_mailboxes/:user_mailbox_id/messages/:message_id", + "httpMethod": "GET", + "description": "hidden mail schema method", + "risk": "read", + "accessTokens": ["tenant"], + "parameters": { + "id": {"type": "string", "location": "path", "required": true} + } + } + } + } + } + }, + { + "name": "im", + "version": "v1", + "title": "im projection fixture", + "description": "visible control service", + "servicePath": "/open-apis/im/v1", + "resources": { + "widgets": { + "methods": { + "get": { + "id": "im.widgets.get", + "path": "/open-apis/im/v1/widgets/:id", + "httpMethod": "GET", + "description": "visible im schema method", + "risk": "read", + "accessTokens": ["tenant"], + "parameters": { + "id": {"type": "string", "location": "path", "required": true} + } + } + } + } + } + } + ] +}` + // multiRulePlugin registers two scope-exclusive Restrict rules (im-only, // docs-only). A command outside both domains (e.g. the top-level "schema" // command, itself read-risk and already proven to hit domain_not_allowed diff --git a/tests/plugin_e2e/skills_test.go b/tests/plugin_e2e/skills_test.go index 46aed3a3e..c54f8ea1e 100644 --- a/tests/plugin_e2e/skills_test.go +++ b/tests/plugin_e2e/skills_test.go @@ -6,6 +6,7 @@ package plugin_e2e import ( "os" "path/filepath" + "regexp" "strings" "testing" @@ -32,6 +33,36 @@ func init() { } ` +const incompleteSkillDependencyPlugin = `// Code generated by plugin_e2e; DO NOT EDIT. +package plugin + +import "github.com/larksuite/cli/extension/platform" + +func init() { + platform.Register( + platform.NewPlugin("incomplete-skill-dependency", "0.1.0"). + EmbeddedSkills(&platform.SkillsOverlay{ + Allow: []string{"lark-doc"}, + }). + MustBuild()) +} +` + +const completeSkillDependencyPlugin = `// Code generated by plugin_e2e; DO NOT EDIT. +package plugin + +import "github.com/larksuite/cli/extension/platform" + +func init() { + platform.Register( + platform.NewPlugin("complete-skill-dependency", "0.1.0"). + EmbeddedSkills(&platform.SkillsOverlay{ + Allow: []string{"lark-doc", "lark-shared"}, + }). + MustBuild()) +} +` + const replacementSkillTreePlugin = `// Code generated by plugin_e2e; DO NOT EDIT. package plugin @@ -156,13 +187,13 @@ func TestForkMainWiresEmbeddedSkillsWithoutOverlay(t *testing.T) { if res.exit != 0 || !gjson.Valid(res.stdout) { t.Fatalf("skills list exit=%d stdout=%s stderr=%s", res.exit, res.stdout, res.stderr) } - if got := gjson.Get(res.stdout, "count").Int(); got != 3 { - t.Fatalf("skills count=%d want 3; stdout=%s", got, res.stdout) + if got := gjson.Get(res.stdout, "count").Int(); got != 4 { + t.Fatalf("skills count=%d want 4; stdout=%s", got, res.stdout) } - if got := gjson.Get(res.stdout, "skills.#.name").Array(); len(got) != 3 || + if got := gjson.Get(res.stdout, "skills.#.name").Array(); len(got) != 4 || got[0].String() != "lark-a" || got[1].String() != "lark-b" || - got[2].String() != "lark-doc" { - t.Fatalf("skill names=%v want [lark-a lark-b lark-doc]; stdout=%s", got, res.stdout) + got[2].String() != "lark-doc" || got[3].String() != "lark-shared" { + t.Fatalf("skill names=%v want [lark-a lark-b lark-doc lark-shared]; stdout=%s", got, res.stdout) } docsHelp := run(t, cli, "docs", "--help") @@ -233,6 +264,58 @@ func TestForkSkillsAllowRemoveComposeAgainstEmbeddedBase(t *testing.T) { } } +func TestForkSkillsMissingRequiredSkillUsesTypedStartupGuard(t *testing.T) { + bin := buildFork(t, "incomplete-skill-dependency", incompleteSkillDependencyPlugin) + res := run(t, bin, "skills", "list") + if res.exit != 2 || !gjson.Valid(res.stderr) { + t.Fatalf("skills list exit=%d stdout=%s stderr=%s", res.exit, res.stdout, res.stderr) + } + if got := gjson.Get(res.stderr, "error.type").String(); got != "validation" { + t.Errorf("error.type=%q want validation; stderr=%s", got, res.stderr) + } + if got := gjson.Get(res.stderr, "error.subtype").String(); got != "failed_precondition" { + t.Errorf("error.subtype=%q want failed_precondition; stderr=%s", got, res.stderr) + } + hint := gjson.Get(res.stderr, "error.hint").String() + reasonMatches := regexp.MustCompile(`\breason_code ([a-z0-9_]+)\b`).FindAllStringSubmatch(hint, -1) + if len(reasonMatches) != 1 || reasonMatches[0][1] != "invalid_skills_overlay" { + t.Errorf("error.hint reason_codes=%v want exactly [invalid_skills_overlay]; hint=%q", reasonMatches, hint) + } + + message := gjson.Get(res.stderr, "error.message").String() + affectedSkills := make(map[string]struct{}) + for _, match := range regexp.MustCompile(`\bskill "([^"]+)"`).FindAllStringSubmatch(message, -1) { + affectedSkills[match[1]] = struct{}{} + } + if len(affectedSkills) != 2 { + t.Errorf("affected skills=%v want exactly [lark-doc lark-shared]; message=%q", affectedSkills, message) + } else { + for _, want := range []string{"lark-doc", "lark-shared"} { + if _, ok := affectedSkills[want]; !ok { + t.Errorf("affected skills=%v missing %q; message=%q", affectedSkills, want, message) + } + } + } +} + +func TestForkSkillsIncludingRequiredSkillSucceeds(t *testing.T) { + bin := buildFork(t, "complete-skill-dependency", completeSkillDependencyPlugin) + list := run(t, bin, "skills", "list") + if list.exit != 0 || !gjson.Valid(list.stdout) { + t.Fatalf("skills list exit=%d stdout=%s stderr=%s", list.exit, list.stdout, list.stderr) + } + if got := gjson.Get(list.stdout, "skills.#.name").Array(); len(got) != 2 || + got[0].String() != "lark-doc" || got[1].String() != "lark-shared" { + t.Fatalf("skill names=%v want [lark-doc lark-shared]; stdout=%s", got, list.stdout) + } + for _, name := range []string{"lark-doc", "lark-shared"} { + read := run(t, bin, "skills", "read", name) + if read.exit != 0 || !strings.Contains(read.stdout, "name: "+name) { + t.Errorf("skill %q unreadable: exit=%d stdout=%s stderr=%s", name, read.exit, read.stdout, read.stderr) + } + } +} + func TestForkSkillsBaseReplacementAndReferenceRemapWithoutHostBase(t *testing.T) { bin := buildForkWithMain( t, @@ -271,6 +354,44 @@ func TestForkSkillsBaseReplacementAndReferenceRemapWithoutHostBase(t *testing.T) t.Fatalf("remapped pointer unreadable: exit=%d stdout=%s stderr=%s", read.exit, read.stdout, read.stderr) } + + configDir := t.TempDir() + writeFile(t, filepath.Join(configDir, "config.json"), + `{"apps":[{"appId":"cli_plugin_e2e","appSecret":"secret","brand":"feishu","users":[]}]}`) + legacyEnv := append(baseEnv(), + "LARKSUITE_CLI_NO_UPDATE_NOTIFIER=1", + "LARKSUITE_CLI_NO_SKILLS_NOTIFIER=1", + "LARKSUITE_CLI_CONFIG_DIR="+configDir, + "LARKSUITE_CLI_REMOTE_META=off", + ) + legacy := runWithEnv(t, bin, legacyEnv, + "docs", "+update", "--doc", "doccn-plugin-e2e", "--mode", "replace", "--as", "user") + if legacy.exit != 2 || !gjson.Valid(legacy.stderr) { + t.Fatalf("remapped v2-only error: exit=%d stdout=%s stderr=%s", + legacy.exit, legacy.stdout, legacy.stderr) + } + if got := gjson.Get(legacy.stderr, "error.subtype").String(); got != "invalid_argument" { + t.Fatalf("remapped v2-only subtype=%q want invalid_argument; stderr=%s", + got, legacy.stderr) + } + if got := gjson.Get(legacy.stderr, "error.param").String(); got != "--mode" { + t.Fatalf("remapped v2-only param=%q want --mode; stderr=%s", + got, legacy.stderr) + } + hint := gjson.Get(legacy.stderr, "error.hint").String() + for _, want := range []string{ + "`lark-cli skills read acme-docx`", + "`lark-cli skills read acme-docx/references/lark-doc-update.md`", + "`lark-cli skills read acme-docx/references/lark-doc-xml.md`", + "`lark-cli skills read acme-docx/references/lark-doc-md.md`", + } { + if !strings.Contains(hint, want) { + t.Errorf("remapped v2-only hint missing %q: %q", want, hint) + } + } + if strings.Contains(hint, "lark-cli skills read lark-doc") { + t.Errorf("remapped v2-only hint leaked canonical skill reference: %q", hint) + } } func TestForkRemovingDocsSkillDropsPointersButKeepsStandaloneGuidance(t *testing.T) {