DEVX-70: preserve GraphQL error classification in CLI envelope

[asingh9-godaddy]

Server-side mutations on app-registry-api classify failures via errors[0].extensions.code (for example VALIDATION_ERROR, AUTH_ERROR). Previously every GraphQL failure in application* services was wrapped in NetworkError, so the CLI JSON envelope reported NETWORK_ERROR with a misleading "verify environment connectivity" fix string.

• Add mapGraphQLError helper in services/applications.ts that inspects ClientError extensions.code and returns ValidationError or AuthenticationError; falls back to NetworkError for unclassified / transport errors (behavior unchanged for those cases).
• Replace all 9 catch blocks in services/applications.ts with the helper (create / update / get / getWithRelease / list / archive / enable / disable / createRelease).
• Add 4 TDD tests covering VALIDATION_ERROR, AUTH_ERROR, unknown extensions.code, and missing extensions.code paths.

[wcole1-godaddy]

Nice refactor — the defensive wire parsing and test coverage are genuinely good. A few things I'd push on before merge, mostly about simplification.

Main feedback: the taxonomy is over-classed

ForbiddenError, NotFoundError, ConflictError, and RateLimitError are structurally identical — same fields, same ApiErrorContext mixin, only _tag differs. Nothing pattern-matches on these tags in a type-driven way; every consumer is the switch in fromTaggedError that just maps _tag → code + fix string. The tag is carrying data, not behavior.

Collapsing them into one ServerError with a discriminant would remove ~80 lines and make "add a new server classification" a two-line change instead of a five-file change:

export type ServerErrorKind =
  | "VALIDATION"
  | "FORBIDDEN"
  | "NOT_FOUND"
  | "CONFLICT"
  | "RATE_LIMITED";

export class ServerError extends Data.TaggedError("ServerError")<
  {
    readonly kind: ServerErrorKind;
    readonly message: string;
    readonly userMessage: string;
    readonly fields?: ReadonlyArray<ErrorField>;
  } & ApiErrorContext
> {}

This also resolves the asymmetry where ValidationError is the only classified error that doesn't carry ApiErrorContext. Keep ValidationError only for client-side arktype input failures — "we rejected your input before sending" is a genuinely different semantic than "the server rejected your request," and conflating them is the root of that asymmetry.

fromTaggedError then becomes a single case with a small kind → fix lookup table colocated with the ServerErrorKind union.

Other issues

pickPrimaryError invariant is conventional, not structural. It's called twice inside mapGraphQLError (once for message, once for extensions) and the "both helpers must refer to the same entry" invariant — the thing the bug fix was about — relies on both callers using the same helper. Pick once at the top of mapGraphQLError, pass the entry down. Can't accidentally break the invariant if there's only one primary.

endsWith("_NOT_FOUND") / endsWith("_CONFLICT") is a fragile contract with the server. POLICY_NOT_FOUND_IN_CACHE would mis-classify. The CLI is also mirroring server taxonomy from @repo/errors by hand — either import that package (ideal) or extract an explicit const CODE_MAP: Record<string, ServerErrorKind> table. Easier to review, easier to keep in sync.

safeGraphQLUserMessage is now doing redundant work. Its status-based branches (401 → "Run godaddy auth login", 403 → "Access denied...") overlap with the new classified-error branches, which already own user-facing guidance via fix strings. After classification, a ForbiddenError doesn't need the status branch to also produce a 403 message. Reduce it to primary?.message ?? "An unexpected error occurred" and keep the status-based canned strings only on the NetworkError fallback path (where classification didn't happen).

ValidationError drops ApiErrorContext in the current implementation. Intentional-looking, but every other branch preserves ctx. If you take the ServerError collapse above, this disappears for free. If not, add a comment explaining why.

ErrorField.code is typed as required string but filtered through typeof === "string". One is wrong. Given the defensive posture everywhere else in this file, I'd relax the type to match runtime reality.

ServerExtensions alias. primary?.extensions as ServerExtensions is a cast, not a check — extensions is Record<string, unknown> on the wire. The runtime guards right after the cast are what actually enforce the shape. Drop the alias and read extensions?.code directly; one less type to maintain.

Envelope code divergences.

• RateLimitError → envelope "RATE_LIMITED" but server wire code is "RATE_LIMIT_EXCEEDED". Consider aligning so agents that speak the server taxonomy don't have to translate.
• AuthenticationError → errorCode() returns "AUTH_ERROR" but envelope overrides to "AUTH_REQUIRED" (pre-existing, not yours). The new classes are consistent; the old one is the outlier. Worth a comment or alignment.

Import style inconsistency. applications.ts uses ./graphql-error; presigned-url.ts uses @/services/graphql-error. Pick one.

Test clarity. expect(err.status).toBe(200) on error paths reads oddly without context — one line noting "GraphQL conventionally returns HTTP 200 with an errors[] body" would age well.

Verdict

Shape of the refactor is right — shared mapper, HTTP context preserved on fallback, field-level classification, the pickPrimaryError bug fix. The over-classing is the main thing worth fixing before merge; everything else is polish. Happy to review again after the collapse.

[wcole1-godaddy]

already commented

[cblecken-godaddy]

Review: DEVX-70 — Preserve GraphQL error classification in CLI envelope

Strengths

• Good extraction pattern — 9 identical catch blocks in applications.ts collapsed to catch: mapGraphQLError, and presigned-url.ts gets the same treatment. Net -76 lines from services.
• Defensive wire-shape handling — toErrorArray, Array.isArray(fields), typeof code === "string" guards throughout. The pickPrimaryError approach correctly handles multi-error responses where the classified entry isn't at index 0.
• Strong test coverage — 20+ test cases including parameterized tests for field-level codes, multi-error regression, malformed fields, transport failures, and end-to-end envelope verification. The presigned-url.test.ts additions are a nice regression guard.
• Correct reclassification — arktype parse failures in createApplicationEffect/createReleaseEffect changed from NetworkError to ValidationError, which is the right semantic.

Minor suggestion

safeGraphQLUserMessage still uses HTTP status codes — The function checks err.response.status for 401/403/404/5xx, but mapGraphQLError classifies based on extensions.code. This means the userMessage can contradict the error class — e.g., a 200-status response with extensions.code: "FORBIDDEN" would produce a ForbiddenError with userMessage: "An unexpected error occurred" (falls through all status checks). Not a bug per se, but the user message could be more helpful by aligning with the classification rather than HTTP status.

Verdict

The core logic is correct, well-tested, and meaningfully improves the CLI's error reporting. LGTM.

[asarkar2-godaddy]

LG