diff --git a/api/create_admission.go b/api/create_admission.go index c383803..12c346e 100644 --- a/api/create_admission.go +++ b/api/create_admission.go @@ -195,17 +195,47 @@ func (s *server) resolveCreateAdmission(ctx context.Context, principal principal switch response.Status { case "", extensionStatusResolved: case extensionStatusUnresolved: - return newAdmissionError(http.StatusConflict, "preset inputs are unresolved", map[string]any{"error": "preset_create_unresolved"}, errors.New("preset inputs are unresolved")) + return newAdmissionError( + http.StatusConflict, + "preset inputs are unresolved", + presetCreatePublicError(response.Status, body.RequestID, body.PresetID).responseData(), + errors.New("preset inputs are unresolved"), + ) case extensionStatusForbidden: - return newAdmissionError(http.StatusForbidden, "preset create resolution is forbidden", map[string]any{"error": "preset_create_forbidden"}, errors.New("preset create resolution is forbidden")) + return newAdmissionError( + http.StatusForbidden, + "preset create resolution is forbidden", + presetCreatePublicError(response.Status, body.RequestID, body.PresetID).responseData(), + errors.New("preset create resolution is forbidden"), + ) case extensionStatusAmbiguous: - return newAdmissionError(http.StatusConflict, "preset inputs are ambiguous", map[string]any{"error": "preset_create_ambiguous"}, errors.New("preset inputs are ambiguous")) + return newAdmissionError( + http.StatusConflict, + "preset inputs are ambiguous", + presetCreatePublicError(response.Status, body.RequestID, body.PresetID).responseData(), + errors.New("preset inputs are ambiguous"), + ) case extensionStatusInvalid: - return newAdmissionError(http.StatusBadRequest, "preset inputs are invalid", map[string]any{"error": "preset_create_invalid"}, errors.New("preset inputs are invalid")) + return newAdmissionError( + http.StatusBadRequest, + "preset inputs are invalid", + presetCreatePublicError(response.Status, body.RequestID, body.PresetID).responseData(), + errors.New("preset inputs are invalid"), + ) case extensionStatusUnavailable: - return newAdmissionError(http.StatusServiceUnavailable, "preset create resolution is unavailable", map[string]any{"error": "preset_create_unavailable"}, errors.New("preset create resolution is unavailable")) + return newAdmissionError( + http.StatusServiceUnavailable, + "preset create resolution is unavailable", + presetCreatePublicError(response.Status, body.RequestID, body.PresetID).responseData(), + errors.New("preset create resolution is unavailable"), + ) default: - return newAdmissionError(http.StatusServiceUnavailable, "preset create resolution returned an unsupported status", nil, fmt.Errorf("unsupported preset create status %q", response.Status)) + return newAdmissionError( + http.StatusServiceUnavailable, + "preset create resolution returned an unsupported status", + presetCreatePublicError(response.Status, body.RequestID, body.PresetID).responseData(), + fmt.Errorf("unsupported preset create status %q", response.Status), + ) } } if selectedClass != nil { @@ -224,6 +254,69 @@ func (s *server) resolveCreateAdmission(ctx context.Context, principal principal return nil } +func presetCreatePublicError(status extensionResolverStatus, requestID, presetID string) publicError { + subject := map[string]string{} + if trimmedPresetID := strings.TrimSpace(presetID); trimmedPresetID != "" { + subject["presetId"] = trimmedPresetID + } + switch status { + case extensionStatusUnresolved: + return createPublicError( + publicErrorCodeIdentityUnresolved, + "This request could not be linked to the required preset inputs yet.", + false, + requestID, + subject, + nil, + ) + case extensionStatusForbidden: + return createPublicError( + publicErrorCodePolicyForbidden, + "This request is not allowed to use the preset create resolver.", + false, + requestID, + subject, + nil, + ) + case extensionStatusAmbiguous: + return createPublicError( + publicErrorCodeIdentityAmbiguous, + "This request matched more than one possible preset input state.", + false, + requestID, + subject, + nil, + ) + case extensionStatusInvalid: + return createPublicError( + publicErrorCodeResolverInvalid, + "This request included invalid preset inputs.", + false, + requestID, + subject, + nil, + ) + case extensionStatusUnavailable: + return createPublicError( + publicErrorCodeResolverUnavailable, + "The preset create resolver is temporarily unavailable.", + true, + requestID, + subject, + nil, + ) + default: + return createPublicError( + publicErrorCodeInternalError, + "The preset create resolver returned an unsupported result.", + true, + requestID, + subject, + nil, + ) + } +} + type presetCreateMutationResult struct { serviceAccountResolved bool runtimePolicyResolved bool diff --git a/api/create_admission_test.go b/api/create_admission_test.go index 271293a..1df7308 100644 --- a/api/create_admission_test.go +++ b/api/create_admission_test.go @@ -245,6 +245,72 @@ func TestCreateSpritzRejectsPresetInputsWithoutMatchingResolver(t *testing.T) { } } +func TestCreateSpritzReturnsStructuredPublicErrorForUnresolvedPresetInputs(t *testing.T) { + s := newCreateSpritzTestServer(t) + resolver := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("Content-Type", "application/json") + _ = json.NewEncoder(w).Encode(map[string]any{ + "status": "unresolved", + }) + })) + defer resolver.Close() + configurePresetResolverTestServer(s, resolver.URL, "") + + e := echo.New() + secured := e.Group("", s.authMiddleware()) + secured.POST("/api/spritzes", s.createSpritz) + + body := []byte(`{ + "name":"zeno-lake", + "presetId":"zeno", + "presetInputs":{"agentId":"ag-123"}, + "requestId":"create-unresolved-1", + "spec":{} + }`) + req := httptest.NewRequest(http.MethodPost, "/api/spritzes", bytes.NewReader(body)) + req.Header.Set(echo.HeaderContentType, echo.MIMEApplicationJSON) + req.Header.Set("X-Spritz-User-Id", "user-1") + rec := httptest.NewRecorder() + + e.ServeHTTP(rec, req) + + if rec.Code != http.StatusConflict { + t.Fatalf("expected status 409, got %d: %s", rec.Code, rec.Body.String()) + } + + var payload map[string]any + if err := json.Unmarshal(rec.Body.Bytes(), &payload); err != nil { + t.Fatalf("failed to decode response json: %v", err) + } + if payload["status"] != "fail" { + t.Fatalf("expected jsend fail status, got %#v", payload["status"]) + } + data := payload["data"].(map[string]any) + publicError, ok := data["error"].(map[string]any) + if !ok { + t.Fatalf("expected structured public error, got %#v", data["error"]) + } + if publicError["code"] != "identity.unresolved" { + t.Fatalf("expected identity.unresolved code, got %#v", publicError["code"]) + } + if publicError["operation"] != "spritz.create" { + t.Fatalf("expected spritz.create operation, got %#v", publicError["operation"]) + } + if publicError["requestId"] != "create-unresolved-1" { + t.Fatalf("expected requestId create-unresolved-1, got %#v", publicError["requestId"]) + } + if publicError["retryable"] != false { + t.Fatalf("expected retryable false, got %#v", publicError["retryable"]) + } + subject, ok := publicError["subject"].(map[string]any) + if !ok { + t.Fatalf("expected subject payload, got %#v", publicError["subject"]) + } + if subject["presetId"] != "zeno" { + t.Fatalf("expected presetId zeno, got %#v", subject["presetId"]) + } +} + func TestCreateSpritzRejectsMissingRequiredResolvedFieldFromInstanceClass(t *testing.T) { s := newCreateSpritzTestServer(t) s.presets = presetCatalog{ diff --git a/api/external_owner_resolution.go b/api/external_owner_resolution.go index 331b9ca..98d56b0 100644 --- a/api/external_owner_resolution.go +++ b/api/external_owner_resolution.go @@ -95,12 +95,13 @@ type externalOwnerConfig struct { } type externalOwnerResolutionError struct { - status int - code string - message string - provider string - tenant string - subject string + status int + code string + message string + requestID string + provider string + tenant string + subject string } type externalOwnerResolverRequest struct { @@ -288,12 +289,13 @@ func (c externalOwnerConfig) resolve(ctx context.Context, principal principal, r policy, ok := c.policyForPrincipal(principal) if !ok { return externalOwnerResolution{}, externalOwnerResolutionError{ - status: http.StatusForbidden, - code: "external_identity_forbidden", - message: "external identity resolution is not allowed for this principal", - provider: strings.TrimSpace(ref.Provider), - tenant: strings.TrimSpace(ref.Tenant), - subject: strings.TrimSpace(ref.Subject), + status: http.StatusForbidden, + code: "external_identity_forbidden", + message: "external identity resolution is not allowed for this principal", + requestID: strings.TrimSpace(requestID), + provider: strings.TrimSpace(ref.Provider), + tenant: strings.TrimSpace(ref.Tenant), + subject: strings.TrimSpace(ref.Subject), } } normalized, err := normalizeExternalOwnerRef(ref) @@ -302,45 +304,49 @@ func (c externalOwnerConfig) resolve(ctx context.Context, principal principal, r } if _, ok := policy.AllowedProviders[normalized.Provider]; !ok { return externalOwnerResolution{}, externalOwnerResolutionError{ - status: http.StatusForbidden, - code: "external_identity_forbidden", - message: "provider is not allowed for this principal", - provider: normalized.Provider, - tenant: normalized.Tenant, - subject: normalized.Subject, + status: http.StatusForbidden, + code: "external_identity_forbidden", + message: "provider is not allowed for this principal", + requestID: strings.TrimSpace(requestID), + provider: normalized.Provider, + tenant: normalized.Tenant, + subject: normalized.Subject, } } tenantRequired := policy.requiresTenant(normalized.Provider) if normalized.Tenant == "" && tenantRequired { return externalOwnerResolution{}, externalOwnerResolutionError{ - status: http.StatusForbidden, - code: "external_identity_forbidden", - message: "tenant is required for this principal", - provider: normalized.Provider, - tenant: normalized.Tenant, - subject: normalized.Subject, + status: http.StatusForbidden, + code: "external_identity_forbidden", + message: "tenant is required for this principal", + requestID: strings.TrimSpace(requestID), + provider: normalized.Provider, + tenant: normalized.Tenant, + subject: normalized.Subject, } } if len(policy.AllowedTenants) > 0 { if normalized.Tenant != "" { if _, ok := policy.AllowedTenants[normalized.Tenant]; !ok { return externalOwnerResolution{}, externalOwnerResolutionError{ - status: http.StatusForbidden, - code: "external_identity_forbidden", - message: "tenant is not allowed for this principal", - provider: normalized.Provider, - tenant: normalized.Tenant, - subject: normalized.Subject, + status: http.StatusForbidden, + code: "external_identity_forbidden", + message: "tenant is not allowed for this principal", + requestID: strings.TrimSpace(requestID), + provider: normalized.Provider, + tenant: normalized.Tenant, + subject: normalized.Subject, } } } else if tenantRequired { return externalOwnerResolution{}, externalOwnerResolutionError{ - status: http.StatusForbidden, - code: "external_identity_forbidden", - message: "tenant is required for this principal", - provider: normalized.Provider, - tenant: normalized.Tenant, - subject: normalized.Subject, + status: http.StatusForbidden, + code: "external_identity_forbidden", + message: "tenant is required for this principal", + requestID: strings.TrimSpace(requestID), + provider: normalized.Provider, + tenant: normalized.Tenant, + subject: normalized.Subject, } } } @@ -348,12 +354,13 @@ func (c externalOwnerConfig) resolve(ctx context.Context, principal principal, r resolution, err := c.resolver.ResolveExternalOwner(ctx, policy, principal, normalized, requestID) if err != nil { return externalOwnerResolution{}, externalOwnerResolutionError{ - status: http.StatusServiceUnavailable, - code: "external_identity_resolution_unavailable", - message: "external identity resolution is unavailable", - provider: normalized.Provider, - tenant: normalized.Tenant, - subject: normalized.Subject, + status: http.StatusServiceUnavailable, + code: "external_identity_resolution_unavailable", + message: "external identity resolution is unavailable", + requestID: strings.TrimSpace(requestID), + provider: normalized.Provider, + tenant: normalized.Tenant, + subject: normalized.Subject, } } @@ -368,50 +375,55 @@ func (c externalOwnerConfig) resolve(ctx context.Context, principal principal, r case externalOwnerResolved: if strings.TrimSpace(resolution.OwnerID) == "" { return externalOwnerResolution{}, externalOwnerResolutionError{ - status: http.StatusServiceUnavailable, - code: "external_identity_resolution_unavailable", - message: "external identity resolution returned an invalid owner", - provider: normalized.Provider, - tenant: normalized.Tenant, - subject: normalized.Subject, + status: http.StatusServiceUnavailable, + code: "external_identity_resolution_unavailable", + message: "external identity resolution returned an invalid owner", + requestID: strings.TrimSpace(requestID), + provider: normalized.Provider, + tenant: normalized.Tenant, + subject: normalized.Subject, } } return resolution, nil case externalOwnerUnresolved: return externalOwnerResolution{}, externalOwnerResolutionError{ - status: http.StatusConflict, - code: "external_identity_unresolved", - message: "external identity is unresolved", - provider: normalized.Provider, - tenant: normalized.Tenant, - subject: normalized.Subject, + status: http.StatusConflict, + code: "external_identity_unresolved", + message: "external identity is unresolved", + requestID: strings.TrimSpace(requestID), + provider: normalized.Provider, + tenant: normalized.Tenant, + subject: normalized.Subject, } case externalOwnerForbidden: return externalOwnerResolution{}, externalOwnerResolutionError{ - status: http.StatusForbidden, - code: "external_identity_forbidden", - message: "external identity resolution is forbidden", - provider: normalized.Provider, - tenant: normalized.Tenant, - subject: normalized.Subject, + status: http.StatusForbidden, + code: "external_identity_forbidden", + message: "external identity resolution is forbidden", + requestID: strings.TrimSpace(requestID), + provider: normalized.Provider, + tenant: normalized.Tenant, + subject: normalized.Subject, } case externalOwnerAmbiguous: return externalOwnerResolution{}, externalOwnerResolutionError{ - status: http.StatusConflict, - code: "external_identity_ambiguous", - message: "external identity is ambiguous", - provider: normalized.Provider, - tenant: normalized.Tenant, - subject: normalized.Subject, + status: http.StatusConflict, + code: "external_identity_ambiguous", + message: "external identity is ambiguous", + requestID: strings.TrimSpace(requestID), + provider: normalized.Provider, + tenant: normalized.Tenant, + subject: normalized.Subject, } default: return externalOwnerResolution{}, externalOwnerResolutionError{ - status: http.StatusServiceUnavailable, - code: "external_identity_resolution_unavailable", - message: "external identity resolution is unavailable", - provider: normalized.Provider, - tenant: normalized.Tenant, - subject: normalized.Subject, + status: http.StatusServiceUnavailable, + code: "external_identity_resolution_unavailable", + message: "external identity resolution is unavailable", + requestID: strings.TrimSpace(requestID), + provider: normalized.Provider, + tenant: normalized.Tenant, + subject: normalized.Subject, } } } @@ -554,16 +566,61 @@ func (e externalOwnerResolutionError) Error() string { } func (e externalOwnerResolutionError) responseData() map[string]any { - data := map[string]any{ - "message": e.message, - "error": e.code, - "identity": map[string]string{ - "provider": e.provider, - "subject": e.subject, - }, + identity := map[string]string{ + "provider": e.provider, + "subject": e.subject, } if strings.TrimSpace(e.tenant) != "" { - data["identity"].(map[string]string)["tenant"] = e.tenant + identity["tenant"] = e.tenant + } + payload := externalOwnerPublicError(e).responseData() + payload["identity"] = identity + return payload +} + +func externalOwnerPublicError(err externalOwnerResolutionError) publicError { + safeDetails := map[string]any{ + "provider": err.provider, + } + if tenant := strings.TrimSpace(err.tenant); tenant != "" { + safeDetails["tenant"] = tenant + } + switch err.code { + case "external_identity_unresolved": + return createPublicError( + publicErrorCodeIdentityUnresolved, + "This request could not be linked to an owner account yet.", + false, + err.requestID, + nil, + safeDetails, + ) + case "external_identity_forbidden": + return createPublicError( + publicErrorCodeIdentityForbidden, + "This identity is not allowed to create an instance in this deployment.", + false, + err.requestID, + nil, + safeDetails, + ) + case "external_identity_ambiguous": + return createPublicError( + publicErrorCodeIdentityAmbiguous, + "This request matched more than one possible owner account.", + false, + err.requestID, + nil, + safeDetails, + ) + default: + return createPublicError( + publicErrorCodeResolverUnavailable, + "External identity resolution is temporarily unavailable.", + true, + err.requestID, + nil, + safeDetails, + ) } - return data } diff --git a/api/jsend.go b/api/jsend.go index 02bdeff..778d845 100644 --- a/api/jsend.go +++ b/api/jsend.go @@ -22,7 +22,8 @@ func writeJSendSuccess(c echo.Context, status int, payload any) error { func writeJSendFail(c echo.Context, status int, message string) error { return c.JSON(status, jsendResponse{ - Status: "fail", + Status: "fail", + Message: message, Data: map[string]string{ "message": message, }, @@ -30,17 +31,19 @@ func writeJSendFail(c echo.Context, status int, message string) error { } func writeJSendFailData(c echo.Context, status int, payload any) error { + message := jsendErrorMessage(payload, status) if status >= 500 { return c.JSON(status, jsendResponse{ Status: "error", - Message: jsendErrorMessage(payload, status), + Message: message, Code: status, Data: payload, }) } return c.JSON(status, jsendResponse{ - Status: "fail", - Data: payload, + Status: "fail", + Message: message, + Data: payload, }) } @@ -56,6 +59,9 @@ func writeError(c echo.Context, status int, message string) error { } func jsendErrorMessage(payload any, status int) string { + if message := publicErrorMessage(payload); message != "" { + return message + } if data, ok := payload.(map[string]any); ok { if message, ok := data["message"].(string); ok && message != "" { return message diff --git a/api/main_create_external_owner_test.go b/api/main_create_external_owner_test.go index 46b6d5e..60ae686 100644 --- a/api/main_create_external_owner_test.go +++ b/api/main_create_external_owner_test.go @@ -167,8 +167,21 @@ func TestCreateSpritzReturnsTypedFailureWhenExternalOwnerIsUnresolved(t *testing t.Fatalf("expected jsend fail status, got %#v", payload["status"]) } data := payload["data"].(map[string]any) - if data["error"] != "external_identity_unresolved" { - t.Fatalf("expected unresolved error code, got %#v", data["error"]) + publicError, ok := data["error"].(map[string]any) + if !ok { + t.Fatalf("expected structured public error, got %#v", data["error"]) + } + if publicError["code"] != "identity.unresolved" { + t.Fatalf("expected identity.unresolved code, got %#v", publicError["code"]) + } + if publicError["operation"] != "spritz.create" { + t.Fatalf("expected spritz.create operation, got %#v", publicError["operation"]) + } + if publicError["retryable"] != false { + t.Fatalf("expected retryable false, got %#v", publicError["retryable"]) + } + if publicError["requestId"] != nil { + t.Fatalf("expected requestId to be omitted when absent, got %#v", publicError["requestId"]) } identity := data["identity"].(map[string]any) if identity["provider"] != "msteams" { diff --git a/api/provisioning.go b/api/provisioning.go index 58c9f44..f0287eb 100644 --- a/api/provisioning.go +++ b/api/provisioning.go @@ -318,12 +318,13 @@ func (s *server) resolveCreateOwner(ctx context.Context, body *createRequest, pr } if !s.externalOwners.enabled() { return spritzv1.SpritzOwner{}, nil, externalOwnerResolutionError{ - status: http.StatusForbidden, - code: "external_identity_forbidden", - message: "external identity resolution is not configured", - provider: normalizedRef.Provider, - tenant: normalizedRef.Tenant, - subject: normalizedRef.Subject, + status: http.StatusForbidden, + code: "external_identity_forbidden", + message: "external identity resolution is not configured", + requestID: strings.TrimSpace(body.RequestID), + provider: normalizedRef.Provider, + tenant: normalizedRef.Tenant, + subject: normalizedRef.Subject, } } resolution, err := s.externalOwners.resolve(ctx, principal, normalizedRef, body.RequestID) diff --git a/api/public_error.go b/api/public_error.go new file mode 100644 index 0000000..16aee22 --- /dev/null +++ b/api/public_error.go @@ -0,0 +1,100 @@ +package main + +import "strings" + +type publicErrorCode string + +const ( + publicErrorCodeStateInvalid publicErrorCode = "state.invalid" + publicErrorCodeStateExpired publicErrorCode = "state.expired" + publicErrorCodeAuthDenied publicErrorCode = "auth.denied" + publicErrorCodeAuthFailed publicErrorCode = "auth.failed" + publicErrorCodeIdentityUnresolved publicErrorCode = "identity.unresolved" + publicErrorCodeIdentityForbidden publicErrorCode = "identity.forbidden" + publicErrorCodeIdentityAmbiguous publicErrorCode = "identity.ambiguous" + publicErrorCodePolicyForbidden publicErrorCode = "policy.forbidden" + publicErrorCodeResolverInvalid publicErrorCode = "resolver.invalid" + publicErrorCodeResolverUnavailable publicErrorCode = "resolver.unavailable" + publicErrorCodeRegistryConflict publicErrorCode = "registry.conflict" + publicErrorCodeRuntimeUnavailable publicErrorCode = "runtime.unavailable" + publicErrorCodeInternalError publicErrorCode = "internal.error" +) + +type publicErrorOperation string + +const ( + publicErrorOperationSpritzCreate publicErrorOperation = "spritz.create" + publicErrorOperationChannelInstall publicErrorOperation = "channel.install" +) + +type publicErrorAction struct { + Type string `json:"type,omitempty"` + Label string `json:"label,omitempty"` + Href string `json:"href,omitempty"` +} + +type publicError struct { + Code publicErrorCode `json:"code"` + Operation publicErrorOperation `json:"operation"` + Title string `json:"title,omitempty"` + Message string `json:"message"` + Retryable bool `json:"retryable"` + RequestID string `json:"requestId,omitempty"` + Action *publicErrorAction `json:"action,omitempty"` + Subject map[string]string `json:"subject,omitempty"` + SafeDetails map[string]any `json:"safeDetails,omitempty"` +} + +func (e publicError) responseData() map[string]any { + data := map[string]any{ + "message": e.Message, + "error": e, + } + if requestID := strings.TrimSpace(e.RequestID); requestID != "" { + data["requestId"] = requestID + } + return data +} + +func createPublicError( + code publicErrorCode, + message string, + retryable bool, + requestID string, + subject map[string]string, + safeDetails map[string]any, +) publicError { + return publicError{ + Code: code, + Operation: publicErrorOperationSpritzCreate, + Message: message, + Retryable: retryable, + RequestID: strings.TrimSpace(requestID), + Subject: subject, + SafeDetails: safeDetails, + } +} + +func publicErrorMessage(payload any) string { + switch typed := payload.(type) { + case publicError: + return strings.TrimSpace(typed.Message) + case *publicError: + if typed == nil { + return "" + } + return strings.TrimSpace(typed.Message) + case map[string]any: + if message, ok := typed["message"].(string); ok && strings.TrimSpace(message) != "" { + return strings.TrimSpace(message) + } + if nested, ok := typed["error"]; ok { + return publicErrorMessage(nested) + } + case map[string]string: + if message, ok := typed["message"]; ok && strings.TrimSpace(message) != "" { + return strings.TrimSpace(message) + } + } + return "" +} diff --git a/cli/src/index.ts b/cli/src/index.ts index 5fda7ec..b6a0be4 100644 --- a/cli/src/index.ts +++ b/cli/src/index.ts @@ -736,6 +736,52 @@ function isJSend(payload: any): payload is { status: string; data?: any; message return payload && typeof payload === 'object' && typeof payload.status === 'string'; } +function structuredPublicError(value: unknown): { code?: string; message?: string } | null { + if (!value || typeof value !== 'object') return null; + const record = value as Record; + const code = + typeof record.code === 'string' && record.code.trim() ? record.code.trim() : undefined; + const message = + typeof record.message === 'string' && record.message.trim() + ? record.message.trim() + : undefined; + if (!code && !message) return null; + return { code, message }; +} + +function jsendErrorCode(jsend: { status: string; data?: any; message?: string } | null): string | undefined { + if (!jsend || !jsend.data || typeof jsend.data !== 'object') return undefined; + const direct = structuredPublicError(jsend.data.error); + if (direct?.code) return direct.code; + if (typeof jsend.data.error === 'string' && jsend.data.error.trim()) return jsend.data.error.trim(); + const nested = structuredPublicError(jsend.data); + return nested?.code; +} + +function jsendErrorMessage(jsend: { status: string; data?: any; message?: string } | null, fallback: string): string { + const direct = jsend ? structuredPublicError(jsend.data?.error) : null; + const nested = jsend ? structuredPublicError(jsend.data) : null; + return ( + direct?.message || + nested?.message || + (jsend && (jsend.message || jsend.data?.message || jsend.data?.error)) || + fallback + ); +} + +function hasExternalOwnerIdentity(data: unknown): boolean { + if (!data || typeof data !== 'object') return false; + const identity = (data as Record).identity; + if (!identity || typeof identity !== 'object') return false; + const record = identity as Record; + return ( + typeof record.provider === 'string' && + record.provider.trim().length > 0 && + typeof record.subject === 'string' && + record.subject.trim().length > 0 + ); +} + async function authHeaders(): Promise> { const { profile } = await resolveProfile({ allowFlag: true }); const token = argValue('--token') || process.env.SPRITZ_BEARER_TOKEN || profile?.bearerToken; @@ -796,15 +842,9 @@ async function request(path: string, init?: RequestInit) { } const jsend = isJSend(data) ? data : null; if (!res.ok || (res.ok && jsend && jsend.status !== 'success')) { - const errorCode = - (jsend && typeof jsend.data?.error === 'string' ? jsend.data.error : undefined) || - undefined; + const errorCode = jsendErrorCode(jsend); const errorData = jsend?.data; - const message = - (jsend && (jsend.message || jsend.data?.message || jsend.data?.error)) || - text || - res.statusText || - 'Request failed'; + const message = jsendErrorMessage(jsend, text || res.statusText || 'Request failed'); throw new SpritzRequestError(message, { statusCode: res.status, code: errorCode, data: errorData }); } if (res.status === 204) return null; @@ -839,11 +879,7 @@ async function internalRequest(path: string, init?: RequestInit) { } const jsend = isJSend(data) ? data : null; if (!res.ok || (res.ok && jsend && jsend.status !== 'success')) { - const message = - (jsend && (jsend.message || jsend.data?.message || jsend.data?.error)) || - text || - res.statusText || - 'Request failed'; + const message = jsendErrorMessage(jsend, text || res.statusText || 'Request failed'); throw new SpritzRequestError(message, { statusCode: res.status, data: jsend?.data }); } if (res.status === 204) return null; @@ -1421,7 +1457,11 @@ async function main() { body: JSON.stringify(body), }); } catch (error) { - if (error instanceof SpritzRequestError && error.code === 'external_identity_unresolved') { + if ( + error instanceof SpritzRequestError && + error.code === 'identity.unresolved' && + hasExternalOwnerIdentity(error.data) + ) { throw new Error(unresolvedExternalOwnerMessage(error, guidance)); } throw error; diff --git a/cli/test/provisioner-create.test.ts b/cli/test/provisioner-create.test.ts index db19d4b..914711e 100644 --- a/cli/test/provisioner-create.test.ts +++ b/cli/test/provisioner-create.test.ts @@ -299,8 +299,13 @@ test('create explains unresolved external owners with connect-account guidance', res.end(JSON.stringify({ status: 'fail', data: { - message: 'external identity is unresolved', - error: 'external_identity_unresolved', + message: 'This request could not be linked to an owner account yet.', + error: { + code: 'identity.unresolved', + operation: 'spritz.create', + message: 'This request could not be linked to an owner account yet.', + retryable: false, + }, identity: { provider: 'discord', subject: '123456789012345678', @@ -354,6 +359,71 @@ test('create explains unresolved external owners with connect-account guidance', assert.match(stderr, /--owner-provider and --owner-subject/i); }); +test('create preserves preset-input unresolved messages without external-owner guidance', async (t) => { + const server = http.createServer((req, res) => { + const chunks: Buffer[] = []; + req.on('data', (chunk) => chunks.push(Buffer.from(chunk))); + req.on('end', () => { + res.writeHead(409, { 'Content-Type': 'application/json' }); + res.end(JSON.stringify({ + status: 'fail', + data: { + message: 'This request could not be linked to the required preset inputs yet.', + error: { + code: 'identity.unresolved', + operation: 'spritz.create', + message: 'This request could not be linked to the required preset inputs yet.', + retryable: false, + subject: { + presetId: 'openclaw', + }, + }, + }, + })); + }); + }); + await new Promise((resolve) => server.listen(0, '127.0.0.1', resolve)); + t.after(() => { + server.close(); + }); + const address = server.address(); + assert.ok(address && typeof address === 'object'); + + const child = spawn( + process.execPath, + [ + '--import', + 'tsx', + cliPath, + 'create', + '--preset', + 'openclaw', + '--preset-inputs', + '{"agentId":"ag-123"}', + ], + { + env: { + ...process.env, + SPRITZ_API_URL: `http://127.0.0.1:${address.port}/api`, + SPRITZ_BEARER_TOKEN: 'service-token', + SPRITZ_CONFIG_DIR: mkdtempSync(path.join(os.tmpdir(), 'spz-config-')), + SPRITZ_OWNER_ID: 'user-123', + }, + stdio: ['ignore', 'pipe', 'pipe'], + }, + ); + + let stderr = ''; + child.stderr.on('data', (chunk) => { + stderr += chunk.toString(); + }); + + const exitCode = await new Promise((resolve) => child.on('exit', resolve)); + assert.notEqual(exitCode, 0, 'spz create should fail for unresolved preset inputs'); + assert.match(stderr, /required preset inputs/i); + assert.doesNotMatch(stderr, /connect their account/i); +}); + test('create without owner input guides agent callers toward external owner flags', async () => { const child = spawn( process.execPath, diff --git a/docs/2026-04-03-unified-public-error-architecture.md b/docs/2026-04-03-unified-public-error-architecture.md new file mode 100644 index 0000000..d74eca0 --- /dev/null +++ b/docs/2026-04-03-unified-public-error-architecture.md @@ -0,0 +1,432 @@ +--- +date: 2026-04-03 +author: Onur Solmaz +title: Unified Public Error Architecture +tags: [spritz, error-handling, api, ui, architecture] +--- + +## Overview + +Goal in plain English: + +If a user tries to install a channel app, create an instance, open a runtime, +or start a session and something expected goes wrong, Spritz should always say: + +- what failed +- what the user can do next +- whether retry makes sense +- which request ID to share with support + +It should not leak a raw proxy error, a vague upstream failure, or a generic +operation-local code that hides the real category of the problem. + +The long-term goal is one canonical public error model and one app-controlled +rendering model across all user-visible Spritz flows. + +Related docs: + +- [Channel Install Result Surface](2026-04-02-channel-install-result-surface.md) +- [External Identity Resolution API Architecture](2026-03-12-external-identity-resolution-api-architecture.md) +- [Unified Extension Framework Architecture](2026-03-19-unified-extension-framework-architecture.md) + +## Problem + +Spritz currently has the start of the right model, but not the full system. + +What already exists: + +- install flow now has a typed result surface +- the shared channel gateway can redirect to a Spritz-controlled result page +- some internal APIs already produce typed states such as `resolved`, + `unresolved`, `forbidden`, `ambiguous`, `invalid`, and `unavailable` + +What is still fragmented: + +- create-time resolver failures still collapse into operation-local codes such + as `preset_create_unresolved` +- browser flows and API flows do not share one explicit public error contract +- the same logical failure can look different depending on which component hit + it first +- internal causes are not consistently separated from user-facing meaning + +This creates three recurring problems: + +1. users get vague or misleading errors +2. adapters and UIs re-invent error handling per flow +3. operators have to infer product meaning from low-level logs + +## Goals + +- define one canonical public error contract for Spritz +- keep browser-visible failures app-controlled +- make API, gateway, and UI surfaces render from the same typed reason model +- separate user-facing meaning from internal cause chains +- keep deployment-specific branding and copy overrides possible without letting + deployments invent new semantics ad hoc + +## Non-Goals + +- exposing raw upstream payloads to end users +- replacing all existing transport envelopes immediately +- moving deployment-specific business policy into Spritz core +- requiring every deployment to use the same copy or visual design + +## Core Decision + +Every user-visible Spritz flow should terminate in a Spritz-owned public error +contract. + +That includes: + +- channel install +- instance create +- runtime bootstrap +- chat/session startup +- channel message delivery when the user is expected to take an action + +The core pattern is: + +1. a backend, resolver, hook, or adapter produces an internal result +2. Spritz normalizes that result into one canonical public error shape +3. the caller surface renders that normalized error +4. internal causes remain in logs, traces, and operator metadata only + +## Canonical Public Error Contract + +Spritz should define a reusable public error object, independent of the +transport used to deliver it. + +Recommended shape: + +```json +{ + "code": "identity.unresolved", + "operation": "spritz.create", + "title": "Account could not be linked", + "message": "This request could not be linked to an owner account yet.", + "retryable": false, + "action": { + "type": "link_account", + "label": "Link account" + }, + "requestId": "req_123", + "subject": { + "presetId": "tcdev" + } +} +``` + +Required fields: + +- `code`: stable machine-readable reason +- `operation`: stable machine-readable operation such as `channel.install` or + `spritz.create` +- `message`: safe default user-facing description +- `retryable`: whether immediate retry is sensible +- `requestId`: correlation identifier shown to the user and logs + +Recommended fields: + +- `title` +- `action` +- `subject` +- `docsUrl` + +Rules: + +- `code` is the stable contract +- `message` and `title` are safe defaults, not the source of truth +- `operation` tells the UI which flow failed without inventing a new error + taxonomy per flow +- the same logical reason should keep the same `code` across flows + +## Canonical Reason Taxonomy + +Spritz should use generic, cross-flow reason codes instead of operation-local +codes such as `preset_create_unresolved`. + +Recommended initial taxonomy: + +- `state.invalid` +- `state.expired` +- `auth.denied` +- `auth.failed` +- `identity.unresolved` +- `identity.forbidden` +- `identity.ambiguous` +- `policy.forbidden` +- `resolver.invalid` +- `resolver.unavailable` +- `binding.unavailable` +- `registry.conflict` +- `runtime.unavailable` +- `internal.error` + +The important rule is that `code` describes the failure category, while +`operation` describes where it happened. + +Example mappings: + +- install callback could not link identity: + - `operation=channel.install` + - `code=identity.unresolved` +- create-time preset resolver could not resolve owner: + - `operation=spritz.create` + - `code=identity.unresolved` +- runtime binding backend is temporarily unreachable: + - `operation=spritz.create` + - `code=resolver.unavailable` + +This avoids creating parallel taxonomies such as: + +- `external_identity_unresolved` +- `preset_create_unresolved` +- `runtime_binding_unavailable` + +when the public meaning is actually the same category with different +operations. + +## Internal Cause Chain + +The public error should not carry the full internal cause chain. + +Spritz should keep an internal error record for logs and traces with fields +such as: + +- `requestId` +- `operation` +- `publicCode` +- `component` +- `resolverId` +- `upstreamStatus` +- `retryable` +- `cause` +- `details` + +Example: + +```json +{ + "requestId": "req_123", + "operation": "spritz.create", + "publicCode": "identity.unresolved", + "component": "preset-resolver", + "resolverId": "preset-create-resolver", + "upstreamStatus": "unresolved", + "cause": "owner lookup returned no matching account" +} +``` + +Rules: + +- internal logs may contain raw cause data +- public responses must not +- every public error should be traceable to one request ID + +## Transport And Rendering Model + +### API responses + +Spritz should keep its existing response envelope style if desired, but public +errors should be embedded in a consistent structured payload rather than +operation-specific ad hoc objects. + +For example: + +```json +{ + "status": "fail", + "data": { + "error": { + "code": "identity.unresolved", + "operation": "spritz.create", + "message": "This request could not be linked to an owner account yet.", + "retryable": false, + "requestId": "req_123" + } + } +} +``` + +The current `jsend.go` helpers are too small to express this well. Spritz +should add explicit writers for typed public errors rather than relying on +free-form `message` strings or flow-local `error` keys. + +### Browser flows + +Browser-visible flows should always terminate in an app-controlled surface. + +Examples: + +- install callback redirects to `/install/result` +- create UI can render inline from the same error contract +- flows that cannot render inline may redirect to `/result` with a stable error + payload reference or query-state key + +Rules: + +- expected failures must not fall through to raw proxy pages +- browser copy must come from `operation + code` +- request ID must always be visible + +### CLI and agent flows + +CLI and agent clients should render the same structured contract in text form. + +That means: + +- a clear one-line summary +- next action if one exists +- request ID + +They should not need operation-specific parsing logic for each backend path. + +## Extension And Adapter Contract + +Resolvers and hooks should continue returning operation-local status, but they +should also be able to provide a canonical public reason. + +Recommended extension response fields: + +```json +{ + "status": "unresolved", + "code": "identity.unresolved", + "retryable": false, + "safeDetails": { + "provider": "slack" + } +} +``` + +Rules: + +- `status` remains the resolver-local execution result +- `code` becomes the public semantic reason +- `safeDetails` may be shown to the UI or gateway +- raw upstream bodies stay out of the public payload + +If a resolver does not provide a `code`, Spritz core should map the local +status to a default public code. + +## Flow-Specific Behavior + +### Install + +Install already has the right direction: + +- normalize backend outcomes +- redirect to a Spritz-owned result surface +- render copy from typed codes + +The next step is to move install onto the shared public error contract instead +of keeping a separate install-only shape long term. + +### Create + +Create is the biggest current gap. + +Today, create admission still returns operation-local codes such as +`preset_create_unresolved`. + +The desired model is: + +- normalize resolver output into canonical `code` +- preserve `operation=spritz.create` +- render the same structure in API, UI, and CLI +- keep resolver IDs and low-level causes in logs only + +### Runtime bootstrap and session startup + +These flows often fail after create succeeds. + +They should still use the same public error contract: + +- `operation=runtime.bootstrap` +- `operation=acp.connect` +- `operation=channel.deliver` + +The user-facing error model should not depend on whether the failure happened +before or after the instance resource existed. + +## Scope Split + +### Spritz owns + +- canonical public reason codes +- public error object shape +- default titles, messages, and retryability +- request ID propagation +- app-controlled result surfaces +- mapping local resolver statuses into public reasons + +### Deployments own + +- branding +- copy overrides +- flow-specific action targets such as a deployment login page or account-link + page +- low-level integration implementation + +Deployments may override presentation, but they should not invent their own +semantic error categories outside the Spritz contract. + +## Phased Rollout + +### Phase 1: Canonical error primitives + +- add a reusable public error type in core +- add typed writers in the API layer +- define the first stable reason taxonomy + +### Phase 2: Resolver-aware normalization + +- let extension responses carry optional canonical `code` +- add default mappings from existing statuses to public reasons +- emit structured operator logs with `requestId`, `operation`, and `publicCode` + +### Phase 3: Create flow adoption + +- replace create-time `preset_create_*` public responses with canonical public + errors +- update UI and CLI create surfaces to render from the shared contract + +### Phase 4: Runtime and session adoption + +- apply the same model to bootstrap, session startup, and channel delivery +- ensure browser-visible failures stay app-controlled + +### Phase 5: Install convergence + +- move install result flow onto the same shared public error object +- keep the install result route, but make it consume the canonical contract + +## Validation + +Minimum validation for this architecture: + +- install, create, and runtime flows all expose `code`, `operation`, + `retryable`, and `requestId` +- expected failures never surface as raw proxy pages in browser flows +- the same logical failure category uses the same `code` across flows +- operator logs include the internal cause chain for every public error +- CLI and browser surfaces can render without flow-specific parsing hacks + +## Holy Grail + +The holy grail is simple: + +- one public error model +- one rendering model +- many internal causes + +If a user sees an error in Spritz, it should behave like a large production +platform: + +- clear +- typed +- correlated +- actionable +- controlled by the application surface, not by infrastructure accidents + +That is the standard every major user-visible Spritz flow should meet. diff --git a/integrations/slack-gateway/gateway_test.go b/integrations/slack-gateway/gateway_test.go index 886ba9d..2157c8b 100644 --- a/integrations/slack-gateway/gateway_test.go +++ b/integrations/slack-gateway/gateway_test.go @@ -302,8 +302,8 @@ func TestOAuthCallbackRedirectsToControlledRetryableErrorWhenBackendUpsertFails( if got := redirectURL.Query().Get("status"); got != "error" { t.Fatalf("expected error status, got %q", got) } - if got := redirectURL.Query().Get("code"); got != "installation_registry_unavailable" { - t.Fatalf("expected installation registry unavailable code, got %q", got) + if got := redirectURL.Query().Get("code"); got != "resolver.unavailable" { + t.Fatalf("expected resolver.unavailable code, got %q", got) } resultReq := httptest.NewRequest(http.MethodGet, redirectURL.RequestURI(), nil) @@ -402,8 +402,8 @@ func TestOAuthCallbackRedirectsToControlledOwnerResolutionError(t *testing.T) { if err != nil { t.Fatalf("parse callback redirect: %v", err) } - if got := redirectURL.Query().Get("code"); got != "external_identity_unresolved" { - t.Fatalf("expected external identity unresolved code, got %q", got) + if got := redirectURL.Query().Get("code"); got != "identity.unresolved" { + t.Fatalf("expected identity.unresolved code, got %q", got) } resultReq := httptest.NewRequest(http.MethodGet, redirectURL.RequestURI(), nil) @@ -460,8 +460,8 @@ func TestOAuthCallbackRedirectsDeniedProviderAuthToControlledResult(t *testing.T if err != nil { t.Fatalf("parse callback redirect: %v", err) } - if got := redirectURL.Query().Get("code"); got != "provider_authorization_denied" { - t.Fatalf("expected provider authorization denied code, got %q", got) + if got := redirectURL.Query().Get("code"); got != "auth.denied" { + t.Fatalf("expected auth.denied code, got %q", got) } } diff --git a/integrations/slack-gateway/install_result.go b/integrations/slack-gateway/install_result.go index abcbb56..3ccf1c3 100644 --- a/integrations/slack-gateway/install_result.go +++ b/integrations/slack-gateway/install_result.go @@ -18,26 +18,30 @@ const ( installResultStatusError installResultStatus = "error" ) +const installResultOperationChannelInstall = "channel.install" + type installResultCode string const ( - installResultCodeInstalled installResultCode = "installed" - installResultCodeInstallStateInvalid installResultCode = "install_state_invalid" - installResultCodeInstallStateExpired installResultCode = "install_state_expired" - installResultCodeProviderAuthorizationDenied installResultCode = "provider_authorization_denied" - installResultCodeProviderAuthorizationFailed installResultCode = "provider_authorization_failed" - installResultCodeExternalIdentityUnresolved installResultCode = "external_identity_unresolved" - installResultCodeExternalIdentityForbidden installResultCode = "external_identity_forbidden" - installResultCodeExternalIdentityAmbiguous installResultCode = "external_identity_ambiguous" - installResultCodeInstallationConflict installResultCode = "installation_conflict" - installResultCodeInstallationRegistryUnavailable installResultCode = "installation_registry_unavailable" - installResultCodeRuntimeBindingUnavailable installResultCode = "runtime_binding_unavailable" - installResultCodeInternalError installResultCode = "internal_error" + installResultCodeInstalled installResultCode = "installed" + installResultCodeStateInvalid installResultCode = "state.invalid" + installResultCodeStateExpired installResultCode = "state.expired" + installResultCodeAuthDenied installResultCode = "auth.denied" + installResultCodeAuthFailed installResultCode = "auth.failed" + installResultCodeIdentityUnresolved installResultCode = "identity.unresolved" + installResultCodeIdentityForbidden installResultCode = "identity.forbidden" + installResultCodeIdentityAmbiguous installResultCode = "identity.ambiguous" + installResultCodeRegistryConflict installResultCode = "registry.conflict" + installResultCodeResolverUnavailable installResultCode = "resolver.unavailable" + installResultCodeRuntimeUnavailable installResultCode = "runtime.unavailable" + installResultCodeInternalError installResultCode = "internal.error" ) type installResult struct { Status installResultStatus Code installResultCode + Operation string + Retryable bool Provider string RequestID string TeamID string @@ -170,6 +174,12 @@ func (g *slackGateway) redirectToInstallResult(w http.ResponseWriter, r *http.Re query.Set("status", string(result.Status)) query.Set("code", string(result.Code)) query.Set("provider", firstNonEmpty(result.Provider, slackProvider)) + if operation := strings.TrimSpace(result.Operation); operation != "" { + query.Set("operation", operation) + } + if result.Retryable { + query.Set("retryable", "true") + } if requestID := strings.TrimSpace(result.RequestID); requestID != "" { query.Set("requestId", requestID) } @@ -187,7 +197,7 @@ func installResultDescriptorFor(code installResultCode, installURL string) insta Title: "Slack workspace connected", Message: "The shared Slack app is installed and ready. You can close this tab.", } - case installResultCodeInstallStateExpired: + case installResultCodeStateExpired: return installResultDescriptor{ Title: "Install link expired", Message: "This install link expired before it completed. Start the install again.", @@ -195,7 +205,7 @@ func installResultDescriptorFor(code installResultCode, installURL string) insta ActionLabel: "Start install again", ActionHref: installURL, } - case installResultCodeInstallStateInvalid: + case installResultCodeStateInvalid: return installResultDescriptor{ Title: "Install link is invalid", Message: "This install callback could not be verified. Start the install again.", @@ -203,7 +213,7 @@ func installResultDescriptorFor(code installResultCode, installURL string) insta ActionLabel: "Start install again", ActionHref: installURL, } - case installResultCodeProviderAuthorizationDenied: + case installResultCodeAuthDenied: return installResultDescriptor{ Title: "Slack authorization was cancelled", Message: "The Slack install did not finish because authorization was denied or cancelled.", @@ -211,7 +221,7 @@ func installResultDescriptorFor(code installResultCode, installURL string) insta ActionLabel: "Start install again", ActionHref: installURL, } - case installResultCodeProviderAuthorizationFailed: + case installResultCodeAuthFailed: return installResultDescriptor{ Title: "Slack authorization failed", Message: "The Slack install did not complete successfully. Please try again.", @@ -219,7 +229,7 @@ func installResultDescriptorFor(code installResultCode, installURL string) insta ActionLabel: "Start install again", ActionHref: installURL, } - case installResultCodeExternalIdentityUnresolved: + case installResultCodeIdentityUnresolved: return installResultDescriptor{ Title: "Install could not be linked", Message: "This Slack install could not be linked to an owner account yet. Link the expected account, then start the install again.", @@ -227,28 +237,28 @@ func installResultDescriptorFor(code installResultCode, installURL string) insta ActionLabel: "Start install again", ActionHref: installURL, } - case installResultCodeExternalIdentityForbidden: + case installResultCodeIdentityForbidden: return installResultDescriptor{ Title: "Install is not allowed", Message: "This Slack identity is not allowed to complete the install for this deployment.", ActionLabel: "Start install again", ActionHref: installURL, } - case installResultCodeExternalIdentityAmbiguous: + case installResultCodeIdentityAmbiguous: return installResultDescriptor{ Title: "Install owner is ambiguous", Message: "This Slack install matched more than one possible owner. Resolve the account mapping, then start the install again.", ActionLabel: "Start install again", ActionHref: installURL, } - case installResultCodeInstallationConflict: + case installResultCodeRegistryConflict: return installResultDescriptor{ Title: "Install conflicts with existing state", Message: "This workspace already has conflicting install state. Resolve the existing binding, then start the install again.", ActionLabel: "Start install again", ActionHref: installURL, } - case installResultCodeInstallationRegistryUnavailable: + case installResultCodeResolverUnavailable: return installResultDescriptor{ Title: "Install could not be completed", Message: "The install service is temporarily unavailable. Please try again shortly.", @@ -256,7 +266,7 @@ func installResultDescriptorFor(code installResultCode, installURL string) insta ActionLabel: "Start install again", ActionHref: installURL, } - case installResultCodeRuntimeBindingUnavailable: + case installResultCodeRuntimeUnavailable: return installResultDescriptor{ Title: "Install is still being prepared", Message: "The workspace binding is not ready yet. Please try again shortly.", @@ -278,18 +288,38 @@ func installResultDescriptorFor(code installResultCode, installURL string) insta func normalizeInstallResultCode(raw string) installResultCode { switch installResultCode(strings.TrimSpace(raw)) { case installResultCodeInstalled, - installResultCodeInstallStateInvalid, - installResultCodeInstallStateExpired, - installResultCodeProviderAuthorizationDenied, - installResultCodeProviderAuthorizationFailed, - installResultCodeExternalIdentityUnresolved, - installResultCodeExternalIdentityForbidden, - installResultCodeExternalIdentityAmbiguous, - installResultCodeInstallationConflict, - installResultCodeInstallationRegistryUnavailable, - installResultCodeRuntimeBindingUnavailable, + installResultCodeStateInvalid, + installResultCodeStateExpired, + installResultCodeAuthDenied, + installResultCodeAuthFailed, + installResultCodeIdentityUnresolved, + installResultCodeIdentityForbidden, + installResultCodeIdentityAmbiguous, + installResultCodeRegistryConflict, + installResultCodeResolverUnavailable, + installResultCodeRuntimeUnavailable, installResultCodeInternalError: return installResultCode(strings.TrimSpace(raw)) + case "install_state_invalid": + return installResultCodeStateInvalid + case "install_state_expired": + return installResultCodeStateExpired + case "provider_authorization_denied": + return installResultCodeAuthDenied + case "provider_authorization_failed": + return installResultCodeAuthFailed + case "external_identity_unresolved": + return installResultCodeIdentityUnresolved + case "external_identity_forbidden": + return installResultCodeIdentityForbidden + case "external_identity_ambiguous": + return installResultCodeIdentityAmbiguous + case "installation_conflict": + return installResultCodeRegistryConflict + case "installation_registry_unavailable": + return installResultCodeResolverUnavailable + case "runtime_binding_unavailable": + return installResultCodeRuntimeUnavailable default: return installResultCodeInternalError } @@ -306,29 +336,29 @@ func classifyInstallUpsertError(err error) installResultCode { return code } if payload.Status == "unresolved" && payload.Field == "ownerRef" { - return installResultCodeExternalIdentityUnresolved + return installResultCodeIdentityUnresolved } if payload.Status == "forbidden" && payload.Field == "ownerRef" { - return installResultCodeExternalIdentityForbidden + return installResultCodeIdentityForbidden } if payload.Status == "ambiguous" && payload.Field == "ownerRef" { - return installResultCodeExternalIdentityAmbiguous + return installResultCodeIdentityAmbiguous } if payload.Status == "ambiguous" { - return installResultCodeInstallationConflict + return installResultCodeRegistryConflict } if payload.Status == "unavailable" { - return installResultCodeInstallationRegistryUnavailable + return installResultCodeResolverUnavailable } } switch statusErr.statusCode { case http.StatusServiceUnavailable: - return installResultCodeInstallationRegistryUnavailable + return installResultCodeResolverUnavailable case http.StatusConflict: - return installResultCodeInstallationConflict + return installResultCodeRegistryConflict default: if statusErr.statusCode >= http.StatusInternalServerError { - return installResultCodeInstallationRegistryUnavailable + return installResultCodeResolverUnavailable } return installResultCodeInternalError } @@ -338,6 +368,8 @@ func (g *slackGateway) handleInstallResult(w http.ResponseWriter, r *http.Reques result := installResult{ Status: installResultStatus(firstNonEmpty(r.URL.Query().Get("status"), string(installResultStatusError))), Code: normalizeInstallResultCode(firstNonEmpty(r.URL.Query().Get("code"), string(installResultCodeInternalError))), + Operation: strings.TrimSpace(r.URL.Query().Get("operation")), + Retryable: strings.EqualFold(strings.TrimSpace(r.URL.Query().Get("retryable")), "true"), Provider: firstNonEmpty(r.URL.Query().Get("provider"), slackProvider), RequestID: strings.TrimSpace(r.URL.Query().Get("requestId")), TeamID: strings.TrimSpace(r.URL.Query().Get("teamId")), diff --git a/integrations/slack-gateway/install_result_test.go b/integrations/slack-gateway/install_result_test.go index 2e593aa..64b1b16 100644 --- a/integrations/slack-gateway/install_result_test.go +++ b/integrations/slack-gateway/install_result_test.go @@ -13,7 +13,7 @@ func TestClassifyInstallUpsertErrorPreservesTypedBackendAvailabilityCodes(t *tes body: `{"error":"runtime_binding_unavailable"}`, } - if got := classifyInstallUpsertError(err); got != installResultCodeRuntimeBindingUnavailable { + if got := classifyInstallUpsertError(err); got != installResultCodeRuntimeUnavailable { t.Fatalf("expected runtime binding unavailable, got %q", got) } } @@ -26,7 +26,7 @@ func TestClassifyInstallUpsertErrorMapsLegacyOwnerRefUnresolvedPayloads(t *testi body: `{"status":"unresolved","field":"ownerRef"}`, } - if got := classifyInstallUpsertError(err); got != installResultCodeExternalIdentityUnresolved { + if got := classifyInstallUpsertError(err); got != installResultCodeIdentityUnresolved { t.Fatalf("expected external identity unresolved, got %q", got) } } diff --git a/integrations/slack-gateway/slack_oauth.go b/integrations/slack-gateway/slack_oauth.go index 7a48f4c..53bebac 100644 --- a/integrations/slack-gateway/slack_oauth.go +++ b/integrations/slack-gateway/slack_oauth.go @@ -83,13 +83,14 @@ func (g *slackGateway) handleOAuthCallback(w http.ResponseWriter, r *http.Reques "request_id", requestID, ) - resultCode := installResultCodeInstallStateInvalid + resultCode := installResultCodeStateInvalid if strings.Contains(strings.ToLower(err.Error()), "expired") { - resultCode = installResultCodeInstallStateExpired + resultCode = installResultCodeStateExpired } g.redirectToInstallResult(w, r, installResult{ Status: installResultStatusError, Code: resultCode, + Operation: installResultOperationChannelInstall, Provider: slackProvider, RequestID: requestID, }) @@ -104,13 +105,14 @@ func (g *slackGateway) handleOAuthCallback(w http.ResponseWriter, r *http.Reques "request_id", requestID, ) - resultCode := installResultCodeProviderAuthorizationFailed + resultCode := installResultCodeAuthFailed if providerError == "access_denied" { - resultCode = installResultCodeProviderAuthorizationDenied + resultCode = installResultCodeAuthDenied } g.redirectToInstallResult(w, r, installResult{ Status: installResultStatusError, Code: resultCode, + Operation: installResultOperationChannelInstall, Provider: slackProvider, RequestID: requestID, }) @@ -125,7 +127,8 @@ func (g *slackGateway) handleOAuthCallback(w http.ResponseWriter, r *http.Reques ) g.redirectToInstallResult(w, r, installResult{ Status: installResultStatusError, - Code: installResultCodeProviderAuthorizationFailed, + Code: installResultCodeAuthFailed, + Operation: installResultOperationChannelInstall, Provider: slackProvider, RequestID: requestID, }) @@ -144,7 +147,8 @@ func (g *slackGateway) handleOAuthCallback(w http.ResponseWriter, r *http.Reques ) g.redirectToInstallResult(w, r, installResult{ Status: installResultStatusError, - Code: installResultCodeProviderAuthorizationFailed, + Code: installResultCodeAuthFailed, + Operation: installResultOperationChannelInstall, Provider: slackProvider, RequestID: requestID, }) @@ -166,6 +170,7 @@ func (g *slackGateway) handleOAuthCallback(w http.ResponseWriter, r *http.Reques g.redirectToInstallResult(w, r, installResult{ Status: installResultStatusError, Code: classifyInstallUpsertError(err), + Operation: installResultOperationChannelInstall, Provider: slackProvider, RequestID: requestID, TeamID: installation.TeamID, @@ -183,6 +188,7 @@ func (g *slackGateway) handleOAuthCallback(w http.ResponseWriter, r *http.Reques g.redirectToInstallResult(w, r, installResult{ Status: installResultStatusSuccess, Code: installResultCodeInstalled, + Operation: installResultOperationChannelInstall, Provider: slackProvider, RequestID: requestID, TeamID: installation.TeamID, diff --git a/ui/src/lib/api.ts b/ui/src/lib/api.ts index 6b41528..3d0d4a0 100644 --- a/ui/src/lib/api.ts +++ b/ui/src/lib/api.ts @@ -345,6 +345,22 @@ function normalizeErrorMessage(value: unknown): string { return normalizeHtmlErrorText(text); } +function structuredPublicError(value: unknown): Record | null { + return value && typeof value === 'object' ? (value as Record) : null; +} + +function jsendPublicErrorMessage(jsend: Record | null): string { + if (!jsend) return ''; + const data = structuredPublicError(jsend.data); + const directError = structuredPublicError(data?.error); + return ( + normalizeErrorMessage(directError?.message) || + normalizeErrorMessage(data?.message) || + normalizeErrorMessage(jsend.message) || + normalizeErrorMessage(data?.error) + ); +} + export async function request(path: string, options: RequestOptions = {}): Promise { const headers = new Headers(options.headers || {}); const token = getAuthToken(); @@ -361,9 +377,7 @@ export async function request(path: string, options: RequestOptions if (res.ok && jsend && jsend.status !== 'success') { const message = - normalizeErrorMessage(jsend.message) || - normalizeErrorMessage((jsend.data as Record)?.message) || - normalizeErrorMessage((jsend.data as Record)?.error) || + jsendPublicErrorMessage(jsend as Record) || normalizeErrorMessage(text) || res.statusText || 'Request failed'; @@ -375,10 +389,7 @@ export async function request(path: string, options: RequestOptions if (!res.ok) { const message = - (jsend && - (normalizeErrorMessage(jsend.message) || - normalizeErrorMessage((jsend.data as Record)?.message) || - normalizeErrorMessage((jsend.data as Record)?.error))) || + (jsend && jsendPublicErrorMessage(jsend as Record)) || normalizeErrorMessage((data as Record)?.error) || normalizeErrorMessage((data as Record)?.message) || normalizeErrorMessage(text) ||