From 69e89e12d9afa29522f320dffc5a3ac78f166024 Mon Sep 17 00:00:00 2001 From: Andrey Markelov Date: Thu, 25 Jun 2026 14:07:30 -0700 Subject: [PATCH] Add structured JSON error details Add optional error.details to JSON error responses for stable machine-readable remediation context. Populate auth remediation fields, path conflicts, and Dropbox API summaries while omitting details for generic local failures. Document the optional details contract and extend schema/tests to keep it covered. --- README.md | 2 + cmd/auth.go | 50 +++++++++++--- cmd/get.go | 2 +- cmd/json_contract_test.go | 3 + cmd/json_output.go | 6 +- cmd/mkdir.go | 4 +- cmd/output.go | 99 ++++++++++++++++++++++++--- cmd/output_test.go | 70 ++++++++++++++++++- cmd/put.go | 6 +- docs/json-schema/v1/README.md | 3 + docs/json-schema/v1/error.schema.json | 5 ++ 11 files changed, 221 insertions(+), 29 deletions(-) diff --git a/README.md b/README.md index e17e254..88e3a1f 100644 --- a/README.md +++ b/README.md @@ -191,6 +191,8 @@ JSON error responses use stable `error.code` values: | `unknown_flag` | Cobra could not resolve a flag. | | `command_failed` | Fallback for failures without a more specific stable code. | +JSON errors may also include an optional `error.details` object when dbxcli has reliable machine-readable context. Current detail keys include `path` for known path conflicts; `token_type`, `login_command`, and `env_var` for auth remediation; and `api_summary` for Dropbox API errors. Generic local failures omit `error.details`. + Successful JSON responses for migrated commands return `ok: true`, `schema_version: "1"`, `command`, an `input` object, a `results` array, and a `warnings` array. Result payloads are command-specific. Public top-level schemas and the command contract catalog live under [docs/json-schema/v1](docs/json-schema/v1/). If a multi-target or recursive command fails after some side effects have already happened, dbxcli returns a JSON error envelope and does not include partial success results. For commands such as `mkdir`, each result reports what happened to the requested path: ```json diff --git a/cmd/auth.go b/cmd/auth.go index 4dd9fce..84c3276 100644 --- a/cmd/auth.go +++ b/cmd/auth.go @@ -238,10 +238,11 @@ func getAccessToken(tokType string, domain string, force bool) (string, string, if !force && credential.shouldRefresh(time.Now()) { credential, err = refreshStoredCredential(tokType, domain, credential) if err != nil { + details := authTokenDetails(tokType) if jsonErrorCode(err) == jsonErrorCodeAppKeyRequired { - return "", "", appKeyRequiredErrorf("refresh saved Dropbox credentials: %w; run %q again", err, loginCommand(tokType)) + return "", "", appKeyRequiredErrorfWithDetails("refresh saved Dropbox credentials: %w; run %q again", details, err, loginCommand(tokType)) } - return "", "", authRefreshFailedErrorf("refresh saved Dropbox credentials: %w; run %q again", err, loginCommand(tokType)) + return "", "", authRefreshFailedErrorfWithDetails("refresh saved Dropbox credentials: %w; run %q again", details, err, loginCommand(tokType)) } tokens[tokType] = credential if err = writeTokens(filePath, tokenMap); err != nil { @@ -264,7 +265,26 @@ func loginCommand(tokType string) string { } func missingAccessTokenError(tokType string) error { - return authRequiredErrorf("no saved Dropbox credentials; run %q first or set %s", loginCommand(tokType), envAccessToken) + return authRequiredErrorfWithDetails("no saved Dropbox credentials; run %q first or set %s", authTokenDetails(tokType), loginCommand(tokType), envAccessToken) +} + +func authTokenDetails(tokType string) map[string]any { + return map[string]any{ + "token_type": authTokenTypeName(tokType), + "login_command": loginCommand(tokType), + "env_var": envAccessToken, + } +} + +func authTokenTypeName(tokType string) string { + switch tokType { + case tokenTeamAccess: + return "team-access" + case tokenTeamManage: + return "team-manage" + default: + return "personal" + } } func appCredentialsName(tokType string) string { @@ -289,7 +309,9 @@ func ensureOAuthAppCredentials(tokType string) error { } creds.Key = strings.TrimSpace(creds.Key) if creds.Key == "" { - return appKeyRequiredError("Dropbox app key is required") + return appKeyRequiredErrorWithDetails("Dropbox app key is required", map[string]any{ + "token_type": authTokenTypeName(tokType), + }) } setOAuthCredentials(tokType, creds.Key) @@ -327,13 +349,19 @@ func requestAccessCredential(tokType string, domain string) (storedCredential, e } token, err := exchangeAuthorizationCode(context.Background(), conf, code, verifier) if err != nil { - return storedCredential{}, authExchangeFailedErrorf("exchange authorization code: %w", err) + return storedCredential{}, authExchangeFailedErrorfWithDetails("exchange authorization code: %w", map[string]any{ + "token_type": authTokenTypeName(tokType), + }, err) } if token == nil || token.AccessToken == "" { - return storedCredential{}, authExchangeFailedError("authorization did not return an access token") + return storedCredential{}, authExchangeFailedErrorWithDetails("authorization did not return an access token", map[string]any{ + "token_type": authTokenTypeName(tokType), + }) } if token.RefreshToken == "" { - return storedCredential{}, authExchangeFailedError("authorization did not return a refresh token") + return storedCredential{}, authExchangeFailedErrorWithDetails("authorization did not return a refresh token", map[string]any{ + "token_type": authTokenTypeName(tokType), + }) } return storedCredentialFromOAuthToken(token, conf.ClientID), nil } @@ -344,7 +372,9 @@ func refreshStoredCredential(tokType string, domain string, credential storedCre appKey = oauthCredentials(tokType) } if strings.TrimSpace(appKey) == "" { - return storedCredential{}, appKeyRequiredError("saved credentials cannot be refreshed without a Dropbox app key") + return storedCredential{}, appKeyRequiredErrorWithDetails("saved credentials cannot be refreshed without a Dropbox app key", map[string]any{ + "token_type": authTokenTypeName(tokType), + }) } token, err := refreshOAuthToken(context.Background(), oauthConfigWithAppKey(appKey, domain), credential.oauthToken()) @@ -352,7 +382,9 @@ func refreshStoredCredential(tokType string, domain string, credential storedCre return storedCredential{}, err } if token == nil || token.AccessToken == "" { - return storedCredential{}, authRefreshFailedErrorf("token refresh did not return an access token") + return storedCredential{}, authRefreshFailedErrorfWithDetails("token refresh did not return an access token", map[string]any{ + "token_type": authTokenTypeName(tokType), + }) } refreshed := storedCredentialFromOAuthToken(token, appKey) diff --git a/cmd/get.go b/cmd/get.go index cc85286..9bb1ab6 100644 --- a/cmd/get.go +++ b/cmd/get.go @@ -345,7 +345,7 @@ func ensureLocalDirectoryResult(source, target string, metadata files.IsMetadata status := getStatusCreated if info, err := os.Stat(target); err == nil { if !info.IsDir() { - return getResult{}, pathConflictErrorf("path exists and is not a folder: %s", target) + return getResult{}, pathConflictErrorWithPath(target, "path exists and is not a folder: %s", target) } status = getStatusExisting } else if !os.IsNotExist(err) { diff --git a/cmd/json_contract_test.go b/cmd/json_contract_test.go index dff641f..0e6b4b6 100644 --- a/cmd/json_contract_test.go +++ b/cmd/json_contract_test.go @@ -192,6 +192,9 @@ func TestPublicJSONSchemaFiles(t *testing.T) { errorSchema := schema.Properties["error"] codeSchema := errorSchema.Properties["code"] assertStringSliceEqual(t, tt.file+" error code enum", codeSchema.Enum, expectedJSONErrorCodes()) + if _, ok := errorSchema.Properties["details"]; !ok { + t.Fatalf("%s error schema missing details property", tt.file) + } } }) } diff --git a/cmd/json_output.go b/cmd/json_output.go index 62d55fc..5f9210c 100644 --- a/cmd/json_output.go +++ b/cmd/json_output.go @@ -15,8 +15,9 @@ type jsonErrorResponse struct { } type jsonError struct { - Message string `json:"message"` - Code string `json:"code"` + Message string `json:"message"` + Code string `json:"code"` + Details map[string]any `json:"details,omitempty"` } type jsonWarning struct { @@ -56,6 +57,7 @@ func newJSONErrorResponse(cmd *cobra.Command, err error) jsonErrorResponse { Error: jsonError{ Message: err.Error(), Code: jsonErrorCode(err), + Details: jsonErrorDetails(err), }, Warnings: emptyJSONWarnings(), } diff --git a/cmd/mkdir.go b/cmd/mkdir.go index 7aa3154..be21b08 100644 --- a/cmd/mkdir.go +++ b/cmd/mkdir.go @@ -76,7 +76,7 @@ func mkdir(cmd *cobra.Command, args []string) (err error) { } status = mkdirStatusExisting case ok && (conflictTag == files.WriteConflictErrorFile || conflictTag == files.WriteConflictErrorFileAncestor): - return pathConflictErrorf("path exists and is not a folder: %s", dst) + return pathConflictErrorWithPath(dst, "path exists and is not a folder: %s", dst) case ok: return err case isConflictError(err): @@ -112,7 +112,7 @@ func existingFolderMetadata(dbx files.Client, dst string) (*files.FolderMetadata } folder, ok := metadata.(*files.FolderMetadata) if !ok || folder == nil { - return nil, pathConflictErrorf("path exists and is not a folder: %s", dst) + return nil, pathConflictErrorWithPath(dst, "path exists and is not a folder: %s", dst) } return folder, nil } diff --git a/cmd/output.go b/cmd/output.go index df64871..d231fc5 100644 --- a/cmd/output.go +++ b/cmd/output.go @@ -37,9 +37,15 @@ type jsonCodedError interface { JSONErrorCode() string } +type jsonDetailedError interface { + error + JSONErrorDetails() map[string]any +} + type codedError struct { - code string - err error + code string + err error + details map[string]any } func (e codedError) Error() string { @@ -54,13 +60,18 @@ func (e codedError) JSONErrorCode() string { return e.code } -func newCodedError(code string, err error) error { +func (e codedError) JSONErrorDetails() map[string]any { + return cloneJSONErrorDetails(e.details) +} + +func newCodedError(code string, err error, details ...map[string]any) error { if err == nil { return nil } return codedError{ - code: code, - err: err, + code: code, + err: err, + details: mergeJSONErrorDetails(details...), } } @@ -72,34 +83,52 @@ func invalidArgumentsErrorf(format string, args ...any) error { return newCodedError(jsonErrorCodeInvalidArguments, fmt.Errorf(format, args...)) } -func pathConflictErrorf(format string, args ...any) error { - return newCodedError(jsonErrorCodePathConflict, fmt.Errorf(format, args...)) +func pathConflictErrorWithPath(path string, format string, args ...any) error { + return newCodedError(jsonErrorCodePathConflict, fmt.Errorf(format, args...), map[string]any{ + "path": path, + }) } func authRequiredErrorf(format string, args ...any) error { return newCodedError(jsonErrorCodeAuthRequired, fmt.Errorf(format, args...)) } +func authRequiredErrorfWithDetails(format string, details map[string]any, args ...any) error { + return newCodedError(jsonErrorCodeAuthRequired, fmt.Errorf(format, args...), details) +} + func appKeyRequiredError(message string) error { return newCodedError(jsonErrorCodeAppKeyRequired, errors.New(message)) } -func appKeyRequiredErrorf(format string, args ...any) error { - return newCodedError(jsonErrorCodeAppKeyRequired, fmt.Errorf(format, args...)) +func appKeyRequiredErrorWithDetails(message string, details map[string]any) error { + return newCodedError(jsonErrorCodeAppKeyRequired, errors.New(message), details) +} + +func appKeyRequiredErrorfWithDetails(format string, details map[string]any, args ...any) error { + return newCodedError(jsonErrorCodeAppKeyRequired, fmt.Errorf(format, args...), details) } func authExchangeFailedError(message string) error { return newCodedError(jsonErrorCodeAuthExchangeFailed, errors.New(message)) } -func authExchangeFailedErrorf(format string, args ...any) error { - return newCodedError(jsonErrorCodeAuthExchangeFailed, fmt.Errorf(format, args...)) +func authExchangeFailedErrorWithDetails(message string, details map[string]any) error { + return newCodedError(jsonErrorCodeAuthExchangeFailed, errors.New(message), details) +} + +func authExchangeFailedErrorfWithDetails(format string, details map[string]any, args ...any) error { + return newCodedError(jsonErrorCodeAuthExchangeFailed, fmt.Errorf(format, args...), details) } func authRefreshFailedErrorf(format string, args ...any) error { return newCodedError(jsonErrorCodeAuthRefreshFailed, fmt.Errorf(format, args...)) } +func authRefreshFailedErrorfWithDetails(format string, details map[string]any, args ...any) error { + return newCodedError(jsonErrorCodeAuthRefreshFailed, fmt.Errorf(format, args...), details) +} + func commandOutput(cmd *cobra.Command) *output.Renderer { if cmd == nil { return output.New(nil, nil, output.FormatText) @@ -275,6 +304,54 @@ func jsonErrorCode(err error) string { } } +func jsonErrorDetails(err error) map[string]any { + details := make(map[string]any) + + var detailed jsonDetailedError + if errors.As(err, &detailed) { + for key, value := range detailed.JSONErrorDetails() { + details[key] = value + } + } + + if summary, ok := dropboxAPIErrorSummary(err); ok { + details["api_summary"] = summary + } else if summary, ok := dropboxAPISummaryFromMessage(err.Error()); ok { + details["api_summary"] = summary + } + + if len(details) == 0 { + return nil + } + return details +} + +func cloneJSONErrorDetails(details map[string]any) map[string]any { + if len(details) == 0 { + return nil + } + cloned := make(map[string]any, len(details)) + for key, value := range details { + cloned[key] = value + } + return cloned +} + +func mergeJSONErrorDetails(details ...map[string]any) map[string]any { + merged := make(map[string]any) + for _, detail := range details { + for key, value := range detail { + if value != nil { + merged[key] = value + } + } + } + if len(merged) == 0 { + return nil + } + return merged +} + func dropboxAPIJSONErrorCode(err error) string { var rateLimitErr dropboxauth.RateLimitAPIError var rateLimitErrPtr *dropboxauth.RateLimitAPIError diff --git a/cmd/output_test.go b/cmd/output_test.go index fa2a831..8b24e44 100644 --- a/cmd/output_test.go +++ b/cmd/output_test.go @@ -305,12 +305,80 @@ func TestRenderCommandErrorWritesJSONErrorToStdout(t *testing.T) { if got.Error.Code != "command_failed" { t.Fatalf("code = %q, want command_failed", got.Error.Code) } + if got.Error.Details != nil { + t.Fatalf("details = %+v, want nil", got.Error.Details) + } if len(got.Warnings) != 0 { t.Fatalf("warnings = %+v, want empty", got.Warnings) } if !strings.Contains(rendered, `"warnings":[]`) { t.Fatalf("output = %q, want warnings array", rendered) } + if strings.Contains(rendered, `"details"`) { + t.Fatalf("output = %q, want details omitted for generic error", rendered) + } +} + +func TestRenderCommandErrorIncludesCodedDetails(t *testing.T) { + var stdout bytes.Buffer + var stderr bytes.Buffer + cmd := &cobra.Command{Use: "mkdir"} + cmd.SetOut(&stdout) + cmd.SetErr(&stderr) + cmd.Flags().String(outputFlag, "json", "") + + renderCommandError(cmd, pathConflictErrorWithPath("/file", "path exists and is not a folder: %s", "/file")) + + if got := stderr.String(); got != "" { + t.Fatalf("stderr = %q, want empty", got) + } + got := decodeJSONErrorResponse(t, stdout.String()) + if got.Error.Code != jsonErrorCodePathConflict { + t.Fatalf("code = %q, want %q", got.Error.Code, jsonErrorCodePathConflict) + } + if got.Error.Details["path"] != "/file" { + t.Fatalf("details = %+v, want path", got.Error.Details) + } +} + +func TestRenderCommandErrorIncludesDropboxAPISummaryDetails(t *testing.T) { + var stdout bytes.Buffer + var stderr bytes.Buffer + cmd := &cobra.Command{Use: "ls"} + cmd.SetOut(&stdout) + cmd.SetErr(&stderr) + cmd.Flags().String(outputFlag, "json", "") + + err := fmt.Errorf("get metadata: %w", files.GetMetadataAPIError{APIError: dropbox.APIError{ErrorSummary: "path/not_found/"}}) + renderCommandError(cmd, err) + + if got := stderr.String(); got != "" { + t.Fatalf("stderr = %q, want empty", got) + } + got := decodeJSONErrorResponse(t, stdout.String()) + if got.Error.Code != jsonErrorCodeNotFound { + t.Fatalf("code = %q, want %q", got.Error.Code, jsonErrorCodeNotFound) + } + if got.Error.Details["api_summary"] != "path/not_found/" { + t.Fatalf("details = %+v, want api_summary", got.Error.Details) + } +} + +func TestJSONErrorDetailsIncludesAuthRemediation(t *testing.T) { + got := newJSONErrorResponse(&cobra.Command{Use: "account"}, missingAccessTokenError(tokenPersonal)) + + if got.Error.Code != jsonErrorCodeAuthRequired { + t.Fatalf("code = %q, want %q", got.Error.Code, jsonErrorCodeAuthRequired) + } + for key, want := range map[string]any{ + "token_type": tokenPersonal, + "login_command": "dbxcli login", + "env_var": envAccessToken, + } { + if got.Error.Details[key] != want { + t.Fatalf("details = %+v, want %s=%v", got.Error.Details, key, want) + } + } } func TestRenderCommandErrorWritesUnsupportedStructuredOutputAsJSON(t *testing.T) { @@ -621,7 +689,7 @@ func TestJSONErrorCodeUsesCodedErrors(t *testing.T) { }{ { name: "path conflict", - err: pathConflictErrorf("path exists and is not a folder: /file"), + err: pathConflictErrorWithPath("/file", "path exists and is not a folder: /file"), want: jsonErrorCodePathConflict, }, { diff --git a/cmd/put.go b/cmd/put.go index 87e998c..d45801a 100644 --- a/cmd/put.go +++ b/cmd/put.go @@ -600,7 +600,7 @@ func checkPutDestination(dbx files.Client, dst string, ifExists string) (putDest return putDestinationUpload, nil, nil } if _, ok := meta.(*files.FolderMetadata); ok { - return putDestinationUpload, nil, pathConflictErrorf("destination %q is a folder", dst) + return putDestinationUpload, nil, pathConflictErrorWithPath(dst, "destination %q is a folder", dst) } return actionForExistingDestination(dst, ifExists, meta) } @@ -610,7 +610,7 @@ func actionForExistingDestination(dst string, ifExists string, metadata files.Is case putIfExistsSkip: return putDestinationSkip, metadata, nil case putIfExistsFail: - return putDestinationUpload, nil, pathConflictErrorf("destination %q already exists", dst) + return putDestinationUpload, nil, pathConflictErrorWithPath(dst, "destination %q already exists", dst) default: return putDestinationUpload, nil, nil } @@ -869,7 +869,7 @@ func putDirectoryConflictError(dst string, err error) error { case ok && conflictTag == files.WriteConflictErrorFolder: return nil case ok && (conflictTag == files.WriteConflictErrorFile || conflictTag == files.WriteConflictErrorFileAncestor): - return pathConflictErrorf("path exists and is not a folder: %s", dst) + return pathConflictErrorWithPath(dst, "path exists and is not a folder: %s", dst) case ok: return err case isConflictError(err): diff --git a/docs/json-schema/v1/README.md b/docs/json-schema/v1/README.md index 70370fc..d795783 100644 --- a/docs/json-schema/v1/README.md +++ b/docs/json-schema/v1/README.md @@ -26,6 +26,9 @@ Error responses always include: - `command`: command path when available, or `dbxcli` for root/pre-parse errors - `error.message`: human-readable error text - `error.code`: stable machine-readable error code +- `error.details`: optional machine-readable context, included only when + dbxcli has reliable structured details such as `path`, `token_type`, + `login_command`, `env_var`, or Dropbox `api_summary` - `warnings`: machine-actionable warnings, or `[]` Command-specific `input` and `result` payload contracts are listed in diff --git a/docs/json-schema/v1/error.schema.json b/docs/json-schema/v1/error.schema.json index 3b560d2..f6b6eb3 100644 --- a/docs/json-schema/v1/error.schema.json +++ b/docs/json-schema/v1/error.schema.json @@ -51,6 +51,11 @@ "unknown_flag", "command_failed" ] + }, + "details": { + "type": "object", + "description": "Optional machine-readable context for stable error handling. Present only when dbxcli has reliable structured details.", + "additionalProperties": true } } },