diff --git a/internal/errclass/codemeta_spark.go b/internal/errclass/codemeta_spark.go index ff8556fb1f..ce6a24ed98 100644 --- a/internal/errclass/codemeta_spark.go +++ b/internal/errclass/codemeta_spark.go @@ -23,6 +23,10 @@ var sparkCodeMeta = map[int]CodeMeta{ 3344039: {Category: errs.CategoryAPI, Subtype: errs.SubtypeInvalidParameters}, // invalid role description 3344040: {Category: errs.CategoryAPI, Subtype: errs.SubtypeInvalidParameters}, // unsupported member type 3344041: {Category: errs.CategoryAPI, Subtype: errs.SubtypeInvalidParameters}, // invalid member ID + + 400002465: {Category: errs.CategoryValidation, Subtype: errs.SubtypeFailedPrecondition}, // app has no database yet + 500002759: {Category: errs.CategoryValidation, Subtype: errs.SubtypeFailedPrecondition}, // app has no database yet, pre-4xx renumber + 400002469: {Category: errs.CategoryAPI, Subtype: errs.SubtypeNotFound}, // table does not exist } func init() { mergeCodeMeta(sparkCodeMeta, "spark") } diff --git a/internal/errclass/codemeta_spark_test.go b/internal/errclass/codemeta_spark_test.go index ab583aeb5d..343ad056d7 100644 --- a/internal/errclass/codemeta_spark_test.go +++ b/internal/errclass/codemeta_spark_test.go @@ -8,6 +8,7 @@ import ( "testing" "github.com/larksuite/cli/errs" + "github.com/larksuite/cli/internal/output" ) func TestLookupCodeMetaSparkRoleCodes(t *testing.T) { @@ -30,6 +31,9 @@ func TestLookupCodeMetaSparkRoleCodes(t *testing.T) { {3344039, errs.CategoryAPI, errs.SubtypeInvalidParameters}, {3344040, errs.CategoryAPI, errs.SubtypeInvalidParameters}, {3344041, errs.CategoryAPI, errs.SubtypeInvalidParameters}, + {400002465, errs.CategoryValidation, errs.SubtypeFailedPrecondition}, + {500002759, errs.CategoryValidation, errs.SubtypeFailedPrecondition}, + {400002469, errs.CategoryAPI, errs.SubtypeNotFound}, } for _, tt := range tests { @@ -57,3 +61,36 @@ func TestLookupCodeMetaSparkRoleCodes(t *testing.T) { }) } } + +// TestSparkNoDatabaseCodesExitCode pins the exit code these codes route to; +// classifying them as Validation moves it from 1 to 2. +func TestSparkNoDatabaseCodesExitCode(t *testing.T) { + for _, code := range []int{400002465, 500002759} { + err := BuildAPIError(map[string]any{ + "code": code, + "msg": "get workspace id failed by app id", + }, ClassifyContext{Identity: "user"}) + if got := output.ExitCodeOf(err); got != 2 { + t.Errorf("code %d exit = %d, want 2 (validation: fix state, do not retry)", code, got) + } + } +} + +// TestSparkTableNotFoundLeavesHintToCaller keeps Hint empty here: the Apps layer +// only fills its command-scoped hint when the classifier left one empty. +func TestSparkTableNotFoundLeavesHintToCaller(t *testing.T) { + err := BuildAPIError(map[string]any{ + "code": 400002469, + "msg": "数据表格不存在", + }, ClassifyContext{Identity: "user"}) + p, ok := errs.ProblemOf(err) + if !ok { + t.Fatalf("BuildAPIError = %#v, want typed problem", err) + } + if p.Hint != "" { + t.Errorf("Hint = %q, want empty so the command-scoped hint still applies", p.Hint) + } + if got := output.ExitCodeOf(err); got != 1 { + t.Errorf("exit = %d, want 1 (unchanged: an ordinary API lookup failure)", got) + } +} diff --git a/shortcuts/apps/common.go b/shortcuts/apps/common.go index cd0572b976..fb10e6eecb 100644 --- a/shortcuts/apps/common.go +++ b/shortcuts/apps/common.go @@ -22,14 +22,22 @@ const apiBasePath = "/open-apis/spark/v1" // lark-apps SKILL.md ("app_id 获取"); the hint stays lean and does not repeat it. const appIDListHint = "verify --app-id is correct and you have access to the app; list your apps with `lark-cli apps +list`" -// appNoDatabaseCode is the Spark business code returned when a db command runs -// against an app that has not initialized a database yet. The raw server -// message for this code carries internal workspace terminology, so the CLI +// appNoDatabaseCode / appNoDatabaseLegacyCode are the Spark business codes seen +// when a db command runs against an app that has not initialized a database yet. +// The raw server message carries internal workspace terminology, so the CLI // rewrites it into a user-facing explanation and attaches a recoverable // cloud-development next step (see appNoDatabaseMessage / appNoDatabaseHint). -// The numeric code — not the message text — is the stable discriminator an -// agent harness keys on to enter the recovery flow. -const appNoDatabaseCode = 500002759 +// +// Two codes, not one: the server renumbered this case from 500002759 to +// 400002465 when the domain moved its client-class errors into the 4xx band. +// Keying on a single literal made the recovery flow disappear silently on the +// day that shipped — no compile error, no failing unit test (they assert against +// the same constant), and the dry-run E2E never reaches a live server. Hence +// isAppNoDatabaseError matches code OR message; see that function. +const ( + appNoDatabaseCode = 400002465 // current + appNoDatabaseLegacyCode = 500002759 // pre-4xx renumber; kept so older servers still match +) // appNoDatabaseMessage is the user-facing explanation for appNoDatabaseCode. // It deliberately drops internal workspace / db-branch terms. @@ -48,10 +56,10 @@ const appNoDatabaseHint = "ask the user whether to add a database through Miaoda // is filled in. Mirrors drive.appendDriveExportRecoveryHint. err==nil and // untyped errors pass through unchanged. // -// Special-case appNoDatabaseCode (500002759, db command on an app with no -// database yet): rewrite the message to a user-facing explanation and force the +// Special-case the "app has no database yet" failure (see isAppNoDatabaseError): +// rewrite the message to a user-facing explanation and force the // cloud-development recovery hint, since the raw upstream message uses internal -// terms and any generic hint would be less actionable. This code is only +// terms and any generic hint would be less actionable. That failure is only // produced by db endpoints, so the override is safe to check for every apps // command that funnels through here. func withAppsHint(err error, hint string) error { @@ -60,7 +68,7 @@ func withAppsHint(err error, hint string) error { } // p points at the embedded Problem, so the mutation is reflected in err. if p, ok := errs.ProblemOf(err); ok { - if p.Code == appNoDatabaseCode { + if isAppNoDatabaseError(p) { p.Message = appNoDatabaseMessage p.Hint = appNoDatabaseHint return err @@ -73,6 +81,47 @@ func withAppsHint(err error, hint string) error { return err } +// appNoDatabaseMessageMarkers are lowercase substrings of the raw server message +// for the no-database failure, used as a fallback when the business code is not +// one the CLI knows. They quote the server's internal vocabulary — "db branch", +// "workspace id ... app id" — which is exactly why the message gets rewritten for +// users. +// +// Deliberately narrow. A looser marker such as "workspace" alone would swallow +// neighbouring db failures that need their own hint; "no db branch" in particular +// must not also match env-pull's "invalid db branch" case +// (isEnvPullDevDBNotInitializedError). Widen only with a test proving the +// neighbours still pass through. +var appNoDatabaseMessageMarkers = []string{ + "get workspace id failed by app id", + "no db branch", +} + +// isAppNoDatabaseError reports whether a typed failure is "this app has no +// database yet", matching on business code OR raw server message. +// +// Why both: the code is the precise signal but not a stable one — the server has +// already renumbered this case once (500002759 → 400002465), and a code-only +// check fails open, silently dropping the recovery flow with nothing in CI to +// catch it. The message is the reverse trade: it survives renumbering but breaks +// on rewording or localization. Requiring either to match means one channel +// changing degrades nothing, and only a simultaneous change of both regresses. +func isAppNoDatabaseError(p *errs.Problem) bool { + if p == nil { + return false + } + if p.Code == appNoDatabaseCode || p.Code == appNoDatabaseLegacyCode { + return true + } + message := strings.ToLower(p.Message) + for _, marker := range appNoDatabaseMessageMarkers { + if strings.Contains(message, marker) { + return true + } + } + return false +} + // validateRealAppID checks that --app-id is a real app ID (app_ prefix). // meta_token values are rejected with a hint to resolve via +get first. func validateRealAppID(appID string) error { diff --git a/shortcuts/apps/common_test.go b/shortcuts/apps/common_test.go index e4b09cfe16..8d964ef7f1 100644 --- a/shortcuts/apps/common_test.go +++ b/shortcuts/apps/common_test.go @@ -92,6 +92,95 @@ func TestWithAppsHint(t *testing.T) { } }) + // The server has renumbered this case before (500002759 → 400002465). Both the + // legacy code and an unrecognized future code must still enter the recovery + // flow, so detection is code-OR-message rather than a single literal. + // assertClassificationIntact checks the discriminators the error envelope keys + // on. Rewriting Message/Hint must never change how the failure is classified, + // and the helper must hand back the same error value so the cause chain + // survives — a replacement error would otherwise pass a Message-only check. + assertClassificationIntact := func(t *testing.T, label string, in error, out error, wantSubtype errs.Subtype, wantCode int) { + t.Helper() + if out != in { + t.Fatalf("%s: returned a different error value: got %p, want original %p", label, out, in) + } + p, ok := errs.ProblemOf(out) + if !ok { + t.Fatalf("%s: returned error is not typed: %T", label, out) + } + if p.Category != errs.CategoryAPI { + t.Errorf("%s: Category = %q, want %q", label, p.Category, errs.CategoryAPI) + } + if p.Subtype != wantSubtype { + t.Errorf("%s: Subtype = %q, want %q", label, p.Subtype, wantSubtype) + } + if p.Code != wantCode { + t.Errorf("%s: Code = %d, want %d", label, p.Code, wantCode) + } + } + + t.Run("legacy no-database code still enters the recovery flow", func(t *testing.T) { + in := errs.NewAPIError(errs.SubtypeNotFound, "workspace has no db branch"). + WithCode(appNoDatabaseLegacyCode) + out := withAppsHint(in, "generic db hint") + p, _ := errs.ProblemOf(out) + if p.Message != appNoDatabaseMessage || p.Hint != appNoDatabaseHint { + t.Errorf("legacy code not detected: Message=%q Hint=%q", p.Message, p.Hint) + } + assertClassificationIntact(t, "legacy code", in, out, errs.SubtypeNotFound, appNoDatabaseLegacyCode) + }) + + t.Run("unknown code is detected by server message", func(t *testing.T) { + // Codes the CLI has never seen: only the raw message identifies the case. + // A concrete subtype (not Unknown) proves classification survives the rewrite. + for _, msg := range []string{ + "get workspace id failed by app id", + "Get Workspace Id Failed By App Id", // case-insensitive + "workspace ws_x has no db branch", // substring, not whole-string + } { + in := errs.NewAPIError(errs.SubtypeNotFound, msg).WithCode(999999999) + out := withAppsHint(in, "generic db hint") + p, _ := errs.ProblemOf(out) + if p.Message != appNoDatabaseMessage || p.Hint != appNoDatabaseHint { + t.Errorf("message %q not detected: Message=%q Hint=%q", msg, p.Message, p.Hint) + } + assertClassificationIntact(t, "message "+msg, in, out, errs.SubtypeNotFound, 999999999) + } + }) + + t.Run("no-database rewrite preserves the wrapped cause", func(t *testing.T) { + // The recovery flow replaces Message wholesale; if it ever swapped the error + // value instead of mutating in place, callers would silently lose errors.Is. + cause := errors.New("upstream transport failure") + in := errs.NewAPIError(errs.SubtypeNotFound, "workspace has no db branch"). + WithCode(appNoDatabaseCode).WithCause(cause) + out := withAppsHint(in, "generic db hint") + if !errors.Is(out, cause) { + t.Errorf("cause chain lost: errors.Is(out, cause) = false") + } + p, _ := errs.ProblemOf(out) + if p.Message != appNoDatabaseMessage { + t.Errorf("Message = %q, want %q", p.Message, appNoDatabaseMessage) + } + }) + + t.Run("unrelated failure keeps the caller hint", func(t *testing.T) { + // Guards against an over-broad matcher hijacking neighbouring db errors: + // "invalid db branch" (env-pull's dev-branch case) must NOT be swallowed. + for _, msg := range []string{"invalid db branch: dev", "数据表格不存在", "permission denied"} { + in := errs.NewAPIError(errs.SubtypeNotFound, msg).WithCode(400002469) + out := withAppsHint(in, "generic db hint") + p, _ := errs.ProblemOf(out) + if p.Message != msg { + t.Errorf("message %q was rewritten to %q; matcher is too broad", msg, p.Message) + } + if p.Hint != "generic db hint" { + t.Errorf("message %q got hint %q, want the caller hint", msg, p.Hint) + } + assertClassificationIntact(t, "unrelated "+msg, in, out, errs.SubtypeNotFound, 400002469) + } + }) + t.Run("no-database code overrides even a preexisting upstream hint", func(t *testing.T) { // An upstream hint must NOT win here: the recovery flow is more actionable. in := errs.NewAPIError(errs.SubtypeUnknown, "internal msg"). @@ -106,3 +195,13 @@ func TestWithAppsHint(t *testing.T) { } }) } + +// TestIsAppNoDatabaseError_NilProblem covers the defensive nil guard, which +// withAppsHint itself cannot reach (ProblemOf returns ok=false for an untyped +// error, so the predicate is never called with nil from there). The guard exists +// because the predicate is package-level and a future caller could pass nil. +func TestIsAppNoDatabaseError_NilProblem(t *testing.T) { + if isAppNoDatabaseError(nil) { + t.Error("isAppNoDatabaseError(nil) = true, want false") + } +}