Skip to content

Commit b60ea07

Browse files
committed
feat: emit typed error envelopes for minutes and vc commands
Failures from the minutes and video-conference commands now carry a stable error category and subtype instead of free-form text, so callers can tell apart invalid input, missing permissions, and network or service failures without scraping messages. Errors raised at the remote API boundary keep their current form until the typed request path is in place.
1 parent 6d7f8ba commit b60ea07

21 files changed

Lines changed: 905 additions & 61 deletions

shortcuts/minutes/minutes_download.go

Lines changed: 38 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,7 @@ import (
1717
"strings"
1818
"time"
1919

20+
"github.com/larksuite/cli/errs"
2021
"github.com/larksuite/cli/extension/fileio"
2122
"github.com/larksuite/cli/internal/output"
2223
"github.com/larksuite/cli/internal/validate"
@@ -54,21 +55,21 @@ var MinutesDownload = common.Shortcut{
5455
Validate: func(ctx context.Context, runtime *common.RuntimeContext) error {
5556
tokens := common.SplitCSV(runtime.Str("minute-tokens"))
5657
if len(tokens) == 0 {
57-
return output.ErrValidation("--minute-tokens is required")
58+
return errs.NewValidationError(errs.SubtypeInvalidArgument, "--minute-tokens is required").WithParam("--minute-tokens")
5859
}
5960
if len(tokens) > maxBatchSize {
60-
return output.ErrValidation("--minute-tokens: too many tokens (%d), maximum is %d", len(tokens), maxBatchSize)
61+
return errs.NewValidationError(errs.SubtypeInvalidArgument, "--minute-tokens: too many tokens (%d), maximum is %d", len(tokens), maxBatchSize).WithParam("--minute-tokens")
6162
}
6263
for _, token := range tokens {
6364
if !validMinuteToken.MatchString(token) {
64-
return output.ErrValidation("invalid minute token %q: must contain only lowercase alphanumeric characters (e.g. obcnq3b9jl72l83w4f149w9c)", token)
65+
return errs.NewValidationError(errs.SubtypeInvalidArgument, "invalid minute token %q: must contain only lowercase alphanumeric characters (e.g. obcnq3b9jl72l83w4f149w9c)", token).WithParam("--minute-tokens")
6566
}
6667
}
6768
// Cheap checks first, then path-safety resolution.
6869
out := runtime.Str("output")
6970
outDir := runtime.Str("output-dir")
7071
if out != "" && outDir != "" {
71-
return output.ErrValidation("--output and --output-dir cannot both be set")
72+
return errs.NewValidationError(errs.SubtypeInvalidArgument, "--output and --output-dir cannot both be set").WithParam("--output")
7273
}
7374
if out != "" {
7475
if err := common.ValidateSafePath(runtime.FileIO(), out); err != nil {
@@ -112,15 +113,15 @@ var MinutesDownload = common.Shortcut{
112113
explicitOutputPath = ""
113114
case statErr == nil && !fi.IsDir():
114115
if !single {
115-
return output.ErrValidation("--output %q is a file; batch mode expects a directory (use --output-dir)", explicitOutputPath)
116+
return errs.NewValidationError(errs.SubtypeInvalidArgument, "--output %q is a file; batch mode expects a directory (use --output-dir)", explicitOutputPath).WithParam("--output")
116117
}
117118
case errors.Is(statErr, fs.ErrNotExist):
118119
if !single {
119120
explicitOutputDir = explicitOutputPath
120121
explicitOutputPath = ""
121122
}
122123
default:
123-
return output.Errorf(output.ExitAPI, "io_error", "cannot access --output %q: %s", explicitOutputPath, statErr)
124+
return errs.NewInternalError(errs.SubtypeFileIO, "cannot access --output %q: %s", explicitOutputPath, statErr).WithCause(statErr)
124125
}
125126
}
126127

@@ -151,7 +152,7 @@ var MinutesDownload = common.Shortcut{
151152
// download URLs originate from the trusted Lark API, not user input.
152153
baseClient, err := runtime.Factory.HttpClient()
153154
if err != nil {
154-
return output.ErrNetwork("failed to get HTTP client: %s", err)
155+
return errs.NewNetworkError(errs.SubtypeNetworkTransport, "failed to get HTTP client: %s", err).WithCause(err)
155156
}
156157
clonedClient := *baseClient
157158
clonedClient.Timeout = disableClientTimeout
@@ -235,6 +236,7 @@ var MinutesDownload = common.Shortcut{
235236
if single {
236237
r := results[0]
237238
if r.Error != "" {
239+
// TODO(errs-migrate): surface batch partial failures via runtime.OutPartialFailure
238240
return output.ErrAPI(0, r.Error, nil)
239241
}
240242
if urlOnly {
@@ -264,6 +266,7 @@ var MinutesDownload = common.Shortcut{
264266

265267
runtime.OutFormat(map[string]interface{}{"downloads": results}, &output.Meta{Count: len(results)}, nil)
266268
if successCount == 0 && len(results) > 0 {
269+
// TODO(errs-migrate): surface batch partial failures via runtime.OutPartialFailure
267270
return output.ErrAPI(0, fmt.Sprintf("all %d downloads failed", len(results)), nil)
268271
}
269272
return nil
@@ -272,6 +275,7 @@ var MinutesDownload = common.Shortcut{
272275

273276
// fetchDownloadURL retrieves the pre-signed download URL for a minute token.
274277
func fetchDownloadURL(ctx context.Context, runtime *common.RuntimeContext, minuteToken string) (string, error) {
278+
// TODO(errs-migrate): route through runtime.CallAPITyped once the typed API boundary is available
275279
data, err := runtime.DoAPIJSON(http.MethodGet,
276280
fmt.Sprintf("/open-apis/minutes/v1/minutes/%s/media", validate.EncodePathSegment(minuteToken)),
277281
nil, nil)
@@ -280,7 +284,7 @@ func fetchDownloadURL(ctx context.Context, runtime *common.RuntimeContext, minut
280284
}
281285
downloadURL := common.GetString(data, "download_url")
282286
if downloadURL == "" {
283-
return "", output.Errorf(output.ExitAPI, "api_error", "API returned empty download_url for %s", minuteToken)
287+
return "", errs.NewInternalError(errs.SubtypeInvalidResponse, "API returned empty download_url for %s", minuteToken)
284288
}
285289
return downloadURL, nil
286290
}
@@ -302,26 +306,26 @@ type downloadOpts struct {
302306
// Filename resolution: opts.outputPath > Content-Disposition filename > Content-Type ext > <token>.media.
303307
func downloadMediaFile(ctx context.Context, client *http.Client, downloadURL, minuteToken string, opts downloadOpts) (*downloadResult, error) {
304308
if err := validate.ValidateDownloadSourceURL(ctx, downloadURL); err != nil {
305-
return nil, output.ErrValidation("blocked download URL: %s", err)
309+
return nil, errs.NewValidationError(errs.SubtypeInvalidArgument, "blocked download URL: %s", err).WithCause(err)
306310
}
307311

308312
req, err := http.NewRequestWithContext(ctx, http.MethodGet, downloadURL, nil)
309313
if err != nil {
310-
return nil, output.ErrNetwork("invalid download URL: %s", err)
314+
return nil, errs.NewNetworkError(errs.SubtypeNetworkTransport, "invalid download URL: %s", err).WithCause(err)
311315
}
312316

313317
resp, err := client.Do(req)
314318
if err != nil {
315-
return nil, output.ErrNetwork("download failed: %s", err)
319+
return nil, errs.NewNetworkError(errs.SubtypeNetworkTransport, "download failed: %s", err).WithCause(err)
316320
}
317321
defer resp.Body.Close()
318322

319323
if resp.StatusCode < 200 || resp.StatusCode >= 300 {
320324
body, _ := io.ReadAll(io.LimitReader(resp.Body, 4096))
321325
if len(body) > 0 {
322-
return nil, output.ErrNetwork("download failed: HTTP %d: %s", resp.StatusCode, strings.TrimSpace(string(body)))
326+
return nil, errs.NewNetworkError(errs.SubtypeNetworkTransport, "download failed: HTTP %d: %s", resp.StatusCode, strings.TrimSpace(string(body)))
323327
}
324-
return nil, output.ErrNetwork("download failed: HTTP %d", resp.StatusCode)
328+
return nil, errs.NewNetworkError(errs.SubtypeNetworkTransport, "download failed: HTTP %d", resp.StatusCode)
325329
}
326330

327331
// resolve output path
@@ -340,7 +344,8 @@ func downloadMediaFile(ctx context.Context, client *http.Client, downloadURL, mi
340344

341345
if !opts.overwrite {
342346
if _, statErr := opts.fio.Stat(outputPath); statErr == nil {
343-
return nil, output.ErrValidation("output file already exists: %s (use --overwrite to replace)", outputPath)
347+
// TODO(errs-migrate): use errs.SubtypeFailedPrecondition once that subtype is available
348+
return nil, errs.NewValidationError(errs.SubtypeInvalidArgument, "output file already exists: %s (use --overwrite to replace)", outputPath)
344349
}
345350
}
346351

@@ -349,7 +354,7 @@ func downloadMediaFile(ctx context.Context, client *http.Client, downloadURL, mi
349354
ContentLength: resp.ContentLength,
350355
}, resp.Body)
351356
if err != nil {
352-
return nil, common.WrapSaveErrorByCategory(err, "io")
357+
return nil, minutesSaveError(err)
353358
}
354359
resolvedPath, err := opts.fio.ResolvePath(outputPath)
355360
if err != nil || resolvedPath == "" {
@@ -417,3 +422,21 @@ func extFromContentType(contentType string) string {
417422
}
418423
return ""
419424
}
425+
426+
// minutesSaveError maps a FileIO.Save error to a typed error: path
427+
// validation failures are validation errors; mkdir/write failures are
428+
// internal file-I/O errors.
429+
func minutesSaveError(err error) error {
430+
if err == nil {
431+
return nil
432+
}
433+
var me *fileio.MkdirError
434+
switch {
435+
case errors.Is(err, fileio.ErrPathValidation):
436+
return errs.NewValidationError(errs.SubtypeInvalidArgument, "unsafe output path: %s", err).WithCause(err)
437+
case errors.As(err, &me):
438+
return errs.NewInternalError(errs.SubtypeFileIO, "cannot create parent directory: %s", err).WithCause(err)
439+
default:
440+
return errs.NewInternalError(errs.SubtypeFileIO, "cannot create file: %s", err).WithCause(err)
441+
}
442+
}

shortcuts/minutes/minutes_download_test.go

Lines changed: 185 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,7 @@ import (
77
"bytes"
88
"context"
99
"encoding/json"
10+
"errors"
1011
"net/http"
1112
"os"
1213
"strings"
@@ -15,6 +16,7 @@ import (
1516

1617
"github.com/spf13/cobra"
1718

19+
"github.com/larksuite/cli/errs"
1820
"github.com/larksuite/cli/internal/cmdutil"
1921
"github.com/larksuite/cli/internal/core"
2022
"github.com/larksuite/cli/internal/httpmock"
@@ -694,3 +696,186 @@ func TestDownload_Batch_DryRun(t *testing.T) {
694696
t.Errorf("dry-run should show tokens, got: %s", out)
695697
}
696698
}
699+
700+
// ---------------------------------------------------------------------------
701+
// Typed-error lock tests
702+
// ---------------------------------------------------------------------------
703+
704+
// TestDownload_TypedErr_ValidationInvalidArgument verifies that an invalid
705+
// minute token format (passing cobra's required check but failing our regex)
706+
// returns a *errs.ValidationError with SubtypeInvalidArgument and the expected
707+
// Param. This locks site :64 (invalid minute token %q).
708+
func TestDownload_TypedErr_ValidationInvalidArgument(t *testing.T) {
709+
f, _, _, _ := cmdutil.TestFactory(t, defaultConfig())
710+
err := mountAndRun(t, MinutesDownload, []string{
711+
"+download", "--minute-tokens", "INVALID***TOKEN", "--as", "user",
712+
}, f, nil)
713+
if err == nil {
714+
t.Fatal("expected error, got nil")
715+
}
716+
var ve *errs.ValidationError
717+
if !errors.As(err, &ve) {
718+
t.Fatalf("expected *errs.ValidationError, got %T: %v", err, err)
719+
}
720+
if ve.Subtype != errs.SubtypeInvalidArgument {
721+
t.Errorf("Subtype = %q, want %q", ve.Subtype, errs.SubtypeInvalidArgument)
722+
}
723+
if ve.Param != "--minute-tokens" {
724+
t.Errorf("Param = %q, want %q", ve.Param, "--minute-tokens")
725+
}
726+
}
727+
728+
// TestDownload_TypedErr_NetworkTransport_HttpError verifies that a non-2xx
729+
// download response from downloadMediaFile returns a *errs.NetworkError with
730+
// SubtypeNetworkTransport.
731+
//
732+
// Note: in the end-to-end single-token Execute path the error is captured into
733+
// results[i].Error (string) and re-surfaced as an aggregate error that is not
734+
// yet typed. We therefore call downloadMediaFile directly via a probe shortcut
735+
// to assert the typed shape at the source.
736+
func TestDownload_TypedErr_NetworkTransport_HttpError(t *testing.T) {
737+
chdir(t, t.TempDir())
738+
739+
var capturedErr error
740+
probe := common.Shortcut{
741+
Service: "minutes",
742+
Command: "+probe-dl",
743+
AuthTypes: []string{"bot"},
744+
Execute: func(ctx context.Context, rctx *common.RuntimeContext) error {
745+
client, err := rctx.Factory.HttpClient()
746+
if err != nil {
747+
return err
748+
}
749+
_, capturedErr = downloadMediaFile(ctx, client,
750+
"https://example.com/presigned/download", "tok001",
751+
downloadOpts{fio: rctx.FileIO(), outputPath: "out.mp4"})
752+
return nil
753+
},
754+
}
755+
756+
f, _, _, reg := cmdutil.TestFactory(t, defaultConfig())
757+
reg.Register(&httpmock.Stub{
758+
URL: "example.com/presigned/download",
759+
Status: 503,
760+
RawBody: []byte("Service Unavailable"),
761+
})
762+
763+
if err := mountAndRun(t, probe, []string{"+probe-dl", "--as", "bot"}, f, nil); err != nil {
764+
t.Fatalf("probe shortcut should not error: %v", err)
765+
}
766+
if capturedErr == nil {
767+
t.Fatal("expected downloadMediaFile to return an error for HTTP 503, got nil")
768+
}
769+
var ne *errs.NetworkError
770+
if !errors.As(capturedErr, &ne) {
771+
t.Fatalf("expected *errs.NetworkError, got %T: %v", capturedErr, capturedErr)
772+
}
773+
if ne.Subtype != errs.SubtypeNetworkTransport {
774+
t.Errorf("Subtype = %q, want %q", ne.Subtype, errs.SubtypeNetworkTransport)
775+
}
776+
if !strings.Contains(ne.Error(), "503") {
777+
t.Errorf("error message should contain status code 503, got: %v", ne)
778+
}
779+
}
780+
781+
// TestDownload_TypedErr_InternalInvalidResponse verifies that fetchDownloadURL
782+
// returns *errs.InternalError with SubtypeInvalidResponse when the API
783+
// response contains an empty download_url field.
784+
//
785+
// Note: the end-to-end Execute path wraps this error in an aggregate error at
786+
// the single-token result aggregation step, which is not yet typed. The
787+
// typed assertion is therefore made at the fetchDownloadURL call site directly,
788+
// using a thin custom shortcut that returns the raw error.
789+
func TestDownload_TypedErr_InternalInvalidResponse(t *testing.T) {
790+
var capturedErr error
791+
probe := common.Shortcut{
792+
Service: "minutes",
793+
Command: "+probe-download-url",
794+
AuthTypes: []string{"bot"},
795+
Execute: func(ctx context.Context, rctx *common.RuntimeContext) error {
796+
_, capturedErr = fetchDownloadURL(ctx, rctx, "tok001")
797+
// Always return nil so mountAndRun doesn't swallow the error type.
798+
return nil
799+
},
800+
}
801+
802+
f, _, _, reg := cmdutil.TestFactory(t, defaultConfig())
803+
reg.Register(&httpmock.Stub{
804+
Method: "GET",
805+
URL: "/open-apis/minutes/v1/minutes/tok001/media",
806+
Status: 200,
807+
Body: map[string]interface{}{
808+
"code": 0, "msg": "ok",
809+
"data": map[string]interface{}{"download_url": ""},
810+
},
811+
})
812+
813+
if err := mountAndRun(t, probe, []string{"+probe-download-url", "--as", "bot"}, f, nil); err != nil {
814+
t.Fatalf("probe shortcut should not error: %v", err)
815+
}
816+
if capturedErr == nil {
817+
t.Fatal("expected fetchDownloadURL to return an error for empty download_url, got nil")
818+
}
819+
var ie *errs.InternalError
820+
if !errors.As(capturedErr, &ie) {
821+
t.Fatalf("expected *errs.InternalError, got %T: %v", capturedErr, capturedErr)
822+
}
823+
if ie.Subtype != errs.SubtypeInvalidResponse {
824+
t.Errorf("Subtype = %q, want %q", ie.Subtype, errs.SubtypeInvalidResponse)
825+
}
826+
if !strings.Contains(ie.Error(), "download_url") {
827+
t.Errorf("error message should mention download_url, got: %v", ie)
828+
}
829+
}
830+
831+
// TestDownload_TypedErr_OverwriteProtection verifies that the overwrite guard
832+
// in downloadMediaFile returns *errs.ValidationError with SubtypeInvalidArgument
833+
// (it will move to a failed-precondition subtype once that subtype is available).
834+
//
835+
// Note: in the end-to-end single-token Execute path this error is captured into
836+
// results[i].Error (string) and re-surfaced as an aggregate error that is not
837+
// yet typed. We call downloadMediaFile directly via a probe shortcut so the typed
838+
// shape is asserted at the source.
839+
func TestDownload_TypedErr_OverwriteProtection(t *testing.T) {
840+
chdir(t, t.TempDir())
841+
if err := os.WriteFile("existing.mp4", []byte("old"), 0644); err != nil {
842+
t.Fatalf("setup failed: %v", err)
843+
}
844+
845+
var capturedErr error
846+
probe := common.Shortcut{
847+
Service: "minutes",
848+
Command: "+probe-overwrite",
849+
AuthTypes: []string{"bot"},
850+
Execute: func(ctx context.Context, rctx *common.RuntimeContext) error {
851+
client, err := rctx.Factory.HttpClient()
852+
if err != nil {
853+
return err
854+
}
855+
_, capturedErr = downloadMediaFile(ctx, client,
856+
"https://example.com/presigned/download", "tok001",
857+
downloadOpts{fio: rctx.FileIO(), outputPath: "existing.mp4", overwrite: false})
858+
return nil
859+
},
860+
}
861+
862+
f, _, _, reg := cmdutil.TestFactory(t, defaultConfig())
863+
reg.Register(downloadStub("example.com/presigned/download", []byte("new-content"), "video/mp4"))
864+
865+
if err := mountAndRun(t, probe, []string{"+probe-overwrite", "--as", "bot"}, f, nil); err != nil {
866+
t.Fatalf("probe shortcut should not error: %v", err)
867+
}
868+
if capturedErr == nil {
869+
t.Fatal("expected downloadMediaFile to return an error for existing file without overwrite, got nil")
870+
}
871+
var ve *errs.ValidationError
872+
if !errors.As(capturedErr, &ve) {
873+
t.Fatalf("expected *errs.ValidationError, got %T: %v", capturedErr, capturedErr)
874+
}
875+
if ve.Subtype != errs.SubtypeInvalidArgument {
876+
t.Errorf("Subtype = %q, want %q", ve.Subtype, errs.SubtypeInvalidArgument)
877+
}
878+
if !strings.Contains(ve.Error(), "exists") {
879+
t.Errorf("error message should mention exists, got: %v", ve)
880+
}
881+
}

0 commit comments

Comments
 (0)