From 6cb3cdda5514cca6e95a048b11f2afd992a059e7 Mon Sep 17 00:00:00 2001 From: Martin Helmich Date: Thu, 2 Jul 2026 11:25:32 +0200 Subject: [PATCH 1/3] fix(app): surface uninstall failures instead of reporting false success DeleteBaseCommand caught errors thrown by deleteResource() but then still called process.complete(), which made the SimpleProcessRenderer print "Process completed successfully" even though the deletion had failed. The command therefore reported success (and appeared to exit 0) when e.g. `app uninstall` was rejected with HTTP 412. The catch branch now calls process.error() so the failure is surfaced, and keeps ux.exit(1) to guarantee a non-zero exit code for scripts. Additionally, `app uninstall` now maps the HTTP 412 that the API returns for an app with a linked primary database to an actionable message instead of a raw Axios error, while passing all other errors through unchanged. Closes #1970 --- src/commands/app/uninstall.test.ts | 44 +++++++++ src/commands/app/uninstall.ts | 34 ++++++- .../basecommands/DeleteBaseCommand.test.tsx | 97 +++++++++++++++++++ src/lib/basecommands/DeleteBaseCommand.tsx | 3 +- 4 files changed, 172 insertions(+), 6 deletions(-) create mode 100644 src/commands/app/uninstall.test.ts create mode 100644 src/lib/basecommands/DeleteBaseCommand.test.tsx diff --git a/src/commands/app/uninstall.test.ts b/src/commands/app/uninstall.test.ts new file mode 100644 index 000000000..146241f2d --- /dev/null +++ b/src/commands/app/uninstall.test.ts @@ -0,0 +1,44 @@ +import { describe, expect, it } from "@jest/globals"; +import { AxiosError, AxiosHeaders } from "axios"; +import { mapUninstallError } from "./uninstall.js"; + +function axiosErrorWithStatus(status: number): AxiosError { + const headers = new AxiosHeaders(); + const config = { headers }; + return new AxiosError( + `Request failed with status code ${status}`, + "ERR_BAD_RESPONSE", + // eslint-disable-next-line @typescript-eslint/no-explicit-any + config as any, + {}, + // eslint-disable-next-line @typescript-eslint/no-explicit-any + { status, data: {}, statusText: "", headers, config } as any, + ); +} + +describe("mapUninstallError", () => { + it("maps an HTTP 412 to a linked-primary-database hint", () => { + const original = axiosErrorWithStatus(412); + const mapped = mapUninstallError(original); + + expect(mapped).toBeInstanceOf(Error); + expect((mapped as Error).message).toContain("cannot uninstall"); + expect((mapped as Error).message).toContain("primary database"); + // The original error is preserved as the cause and not swallowed. + expect((mapped as Error).cause).toBe(original); + }); + + it("passes through other Axios errors unchanged", () => { + const original = axiosErrorWithStatus(409); + const mapped = mapUninstallError(original); + + expect(mapped).toBe(original); + }); + + it("passes through non-Axios errors unchanged", () => { + const original = new Error("something else"); + const mapped = mapUninstallError(original); + + expect(mapped).toBe(original); + }); +}); diff --git a/src/commands/app/uninstall.ts b/src/commands/app/uninstall.ts index d25e0bdce..779a0e325 100644 --- a/src/commands/app/uninstall.ts +++ b/src/commands/app/uninstall.ts @@ -1,7 +1,28 @@ import { assertStatus } from "@mittwald/api-client-commons"; +import axios from "axios"; import { DeleteBaseCommand } from "../../lib/basecommands/DeleteBaseCommand.js"; import { appInstallationArgs } from "../../lib/resources/app/flags.js"; +/** + * Maps an error raised while uninstalling an app installation to a more + * actionable one. The API answers with an HTTP 412 (Precondition Failed) when + * the app still has a linked _primary_ database; in that case we surface a + * clear hint instead of a raw Axios error. All other errors are passed through + * unchanged so they are not accidentally swallowed. + */ +export function mapUninstallError(err: unknown): unknown { + if (axios.isAxiosError(err) && err.response?.status === 412) { + return new Error( + "cannot uninstall: app has a linked primary database — " + + "unlink or repurpose it first (a primary database must be " + + "repurposed to custom or cache before it can be unlinked)", + { cause: err }, + ); + } + + return err; +} + export default class Uninstall extends DeleteBaseCommand { static description = "Uninstall an app"; static resourceName = "app installation"; @@ -12,10 +33,15 @@ export default class Uninstall extends DeleteBaseCommand { protected async deleteResource(): Promise { const appInstallationId = await this.withAppInstallationId(Uninstall); - const response = await this.apiClient.app.uninstallAppinstallation({ - appInstallationId, - }); - assertStatus(response, 204); + try { + const response = await this.apiClient.app.uninstallAppinstallation({ + appInstallationId, + }); + + assertStatus(response, 204); + } catch (err) { + throw mapUninstallError(err); + } } } diff --git a/src/lib/basecommands/DeleteBaseCommand.test.tsx b/src/lib/basecommands/DeleteBaseCommand.test.tsx new file mode 100644 index 000000000..347bab0a4 --- /dev/null +++ b/src/lib/basecommands/DeleteBaseCommand.test.tsx @@ -0,0 +1,97 @@ +import { beforeEach, describe, expect, it, jest } from "@jest/globals"; + +/* + * Regression test for the bug where a failing deletion (e.g. an app uninstall + * rejected with HTTP 412) still printed "Process completed successfully" and + * exited with code 0. The command must instead surface the error via + * process.error() and exit with a non-zero code. + */ + +const fakeStep = { + complete: jest.fn(), + error: jest.fn(), +}; + +const fakeProcess = { + start: jest.fn(), + addStep: jest.fn(() => fakeStep), + addConfirmation: jest.fn(async () => true), + addInfo: jest.fn(), + complete: jest.fn(async (_summary: unknown) => {}), + error: jest.fn(async (_err: unknown) => {}), +}; + +const makeProcessRenderer = jest.fn(() => fakeProcess); + +jest.unstable_mockModule("../../rendering/process/process_flags.js", () => ({ + __esModule: true, + makeProcessRenderer, + processFlags: {}, +})); + +const { DeleteBaseCommand } = await import("./DeleteBaseCommand.js"); + +class TestDelete extends DeleteBaseCommand { + static resourceName = "test resource"; + public shouldThrow: unknown = undefined; + + protected async deleteResource(): Promise { + if (this.shouldThrow !== undefined) { + throw this.shouldThrow; + } + } +} + +function makeInstance(): TestDelete { + const instance = Object.create(TestDelete.prototype) as TestDelete; + // eslint-disable-next-line @typescript-eslint/no-explicit-any + (instance as any).flags = { force: true }; + return instance; +} + +// eslint-disable-next-line @typescript-eslint/no-explicit-any +async function runExec(instance: TestDelete): Promise { + try { + // eslint-disable-next-line @typescript-eslint/no-explicit-any + await (instance as any).exec(); + return undefined; + } catch (err) { + return err; + } +} + +describe("DeleteBaseCommand", () => { + beforeEach(() => { + jest.clearAllMocks(); + }); + + it("reports success and does not exit non-zero when deletion succeeds", async () => { + const instance = makeInstance(); + + const thrown = await runExec(instance); + + expect(fakeStep.complete).toHaveBeenCalled(); + expect(fakeProcess.complete).toHaveBeenCalledTimes(1); + expect(fakeProcess.error).not.toHaveBeenCalled(); + // no ux.exit() -> no thrown ExitError + expect(thrown).toBeUndefined(); + }); + + it("surfaces the error and exits non-zero when deletion fails", async () => { + const instance = makeInstance(); + const failure = new Error("boom"); + instance.shouldThrow = failure; + + const thrown = await runExec(instance); + + // Must NOT falsely report success ... + expect(fakeProcess.complete).not.toHaveBeenCalled(); + // ... must surface the actual error ... + expect(fakeStep.error).toHaveBeenCalledWith(failure); + expect(fakeProcess.error).toHaveBeenCalledWith(failure); + // ... and must exit with a non-zero code (ux.exit(1) throws an ExitError). + expect(thrown).toBeDefined(); + // eslint-disable-next-line @typescript-eslint/no-explicit-any + expect((thrown as any).oclif?.exit).toBe(1); + }); +}); diff --git a/src/lib/basecommands/DeleteBaseCommand.tsx b/src/lib/basecommands/DeleteBaseCommand.tsx index 129d2c3d6..8215b5ad8 100644 --- a/src/lib/basecommands/DeleteBaseCommand.tsx +++ b/src/lib/basecommands/DeleteBaseCommand.tsx @@ -7,7 +7,6 @@ import { processFlags, } from "../../rendering/process/process_flags.js"; import { Success } from "../../rendering/react/components/Success.js"; -import { Text } from "ink"; import React from "react"; export abstract class DeleteBaseCommand< @@ -56,7 +55,7 @@ export abstract class DeleteBaseCommand< ); } catch (err) { deletingStep.error(err); - process.complete(Failed to delete {resourceName}); + await process.error(err); ux.exit(1); } } From 5d87fd8b696336799930fecd9954668d1b554688 Mon Sep 17 00:00:00 2001 From: Lars Bergmann Date: Thu, 23 Jul 2026 11:18:43 +0200 Subject: [PATCH 2/3] handle error without monkeypatching axios client --- src/commands/app/uninstall.test.ts | 54 ++++++++++++++++++---------- src/commands/app/uninstall.ts | 56 +++++++++++++++++++++++++++--- 2 files changed, 86 insertions(+), 24 deletions(-) diff --git a/src/commands/app/uninstall.test.ts b/src/commands/app/uninstall.test.ts index 146241f2d..8c66effa0 100644 --- a/src/commands/app/uninstall.test.ts +++ b/src/commands/app/uninstall.test.ts @@ -1,41 +1,57 @@ import { describe, expect, it } from "@jest/globals"; -import { AxiosError, AxiosHeaders } from "axios"; import { mapUninstallError } from "./uninstall.js"; -function axiosErrorWithStatus(status: number): AxiosError { - const headers = new AxiosHeaders(); - const config = { headers }; - return new AxiosError( - `Request failed with status code ${status}`, - "ERR_BAD_RESPONSE", - // eslint-disable-next-line @typescript-eslint/no-explicit-any - config as any, - {}, - // eslint-disable-next-line @typescript-eslint/no-explicit-any - { status, data: {}, statusText: "", headers, config } as any, - ); +function apiErrorLike(options: { + message?: string; + status: number; + type?: string; +}) { + return { + response: { + data: { + message: options.message, + type: options.type, + }, + status: options.status, + }, + }; } describe("mapUninstallError", () => { - it("maps an HTTP 412 to a linked-primary-database hint", () => { - const original = axiosErrorWithStatus(412); + it("maps matching HTTP 412 primary-database preconditions", () => { + const original = apiErrorLike({ + status: 412, + type: "PreconditionFailed", + message: "App has a linked primary database", + }); const mapped = mapUninstallError(original); expect(mapped).toBeInstanceOf(Error); expect((mapped as Error).message).toContain("cannot uninstall"); expect((mapped as Error).message).toContain("primary database"); - // The original error is preserved as the cause and not swallowed. + // The original error is preserved as the cause. expect((mapped as Error).cause).toBe(original); }); - it("passes through other Axios errors unchanged", () => { - const original = axiosErrorWithStatus(409); + it("passes through unrelated HTTP 412 errors unchanged", () => { + const original = apiErrorLike({ + status: 412, + type: "PreconditionFailed", + message: "Some other precondition failed", + }); const mapped = mapUninstallError(original); expect(mapped).toBe(original); }); - it("passes through non-Axios errors unchanged", () => { + it("passes through non-412 API errors unchanged", () => { + const original = apiErrorLike({ status: 409, type: "Conflict" }); + const mapped = mapUninstallError(original); + + expect(mapped).toBe(original); + }); + + it("passes through unrelated errors unchanged", () => { const original = new Error("something else"); const mapped = mapUninstallError(original); diff --git a/src/commands/app/uninstall.ts b/src/commands/app/uninstall.ts index 779a0e325..6cf4aafbf 100644 --- a/src/commands/app/uninstall.ts +++ b/src/commands/app/uninstall.ts @@ -1,17 +1,63 @@ import { assertStatus } from "@mittwald/api-client-commons"; -import axios from "axios"; import { DeleteBaseCommand } from "../../lib/basecommands/DeleteBaseCommand.js"; import { appInstallationArgs } from "../../lib/resources/app/flags.js"; +interface APIErrorBody { + message?: string; + type?: string; +} + +interface APIErrorLike { + response?: { + data?: unknown; + status?: number; + }; +} + +function getAPIErrorDetails(err: unknown): { + body: APIErrorBody | undefined; + status: number; +} | null { + if (typeof err !== "object" || err === null) { + return null; + } + + const response = (err as APIErrorLike).response; + if (typeof response?.status !== "number") { + return null; + } + + const body = + typeof response.data === "object" && response.data !== null + ? (response.data as APIErrorBody) + : undefined; + + return { + status: response.status, + body, + }; +} + +function isPrimaryDatabasePreconditionError( + body: APIErrorBody | undefined, +): boolean { + const signature = `${body?.type ?? ""} ${body?.message ?? ""}`.toLowerCase(); + return signature.includes("primary") && signature.includes("database"); +} + /** * Maps an error raised while uninstalling an app installation to a more * actionable one. The API answers with an HTTP 412 (Precondition Failed) when - * the app still has a linked _primary_ database; in that case we surface a - * clear hint instead of a raw Axios error. All other errors are passed through - * unchanged so they are not accidentally swallowed. + * the app still has a linked _primary_ database. We therefore require both the + * precondition status and a matching error signature before rewriting the + * message, so unrelated 412 cases are not accidentally swallowed. */ export function mapUninstallError(err: unknown): unknown { - if (axios.isAxiosError(err) && err.response?.status === 412) { + const details = getAPIErrorDetails(err); + if ( + details?.status === 412 && + isPrimaryDatabasePreconditionError(details.body) + ) { return new Error( "cannot uninstall: app has a linked primary database — " + "unlink or repurpose it first (a primary database must be " + From ba61f5cf0b599b688de397dd3c9fb629ca64c532 Mon Sep 17 00:00:00 2001 From: Lars Bergmann Date: Thu, 23 Jul 2026 11:24:51 +0200 Subject: [PATCH 3/3] extract api error check into shared tool --- src/commands/app/uninstall.ts | 51 +++-------------------- src/lib/error/apiError.test.ts | 74 ++++++++++++++++++++++++++++++++++ src/lib/error/apiError.ts | 66 ++++++++++++++++++++++++++++++ 3 files changed, 145 insertions(+), 46 deletions(-) create mode 100644 src/lib/error/apiError.test.ts create mode 100644 src/lib/error/apiError.ts diff --git a/src/commands/app/uninstall.ts b/src/commands/app/uninstall.ts index 6cf4aafbf..7e0f9652c 100644 --- a/src/commands/app/uninstall.ts +++ b/src/commands/app/uninstall.ts @@ -1,50 +1,8 @@ import { assertStatus } from "@mittwald/api-client-commons"; import { DeleteBaseCommand } from "../../lib/basecommands/DeleteBaseCommand.js"; +import { matchesAPIError } from "../../lib/error/apiError.js"; import { appInstallationArgs } from "../../lib/resources/app/flags.js"; -interface APIErrorBody { - message?: string; - type?: string; -} - -interface APIErrorLike { - response?: { - data?: unknown; - status?: number; - }; -} - -function getAPIErrorDetails(err: unknown): { - body: APIErrorBody | undefined; - status: number; -} | null { - if (typeof err !== "object" || err === null) { - return null; - } - - const response = (err as APIErrorLike).response; - if (typeof response?.status !== "number") { - return null; - } - - const body = - typeof response.data === "object" && response.data !== null - ? (response.data as APIErrorBody) - : undefined; - - return { - status: response.status, - body, - }; -} - -function isPrimaryDatabasePreconditionError( - body: APIErrorBody | undefined, -): boolean { - const signature = `${body?.type ?? ""} ${body?.message ?? ""}`.toLowerCase(); - return signature.includes("primary") && signature.includes("database"); -} - /** * Maps an error raised while uninstalling an app installation to a more * actionable one. The API answers with an HTTP 412 (Precondition Failed) when @@ -53,10 +11,11 @@ function isPrimaryDatabasePreconditionError( * message, so unrelated 412 cases are not accidentally swallowed. */ export function mapUninstallError(err: unknown): unknown { - const details = getAPIErrorDetails(err); if ( - details?.status === 412 && - isPrimaryDatabasePreconditionError(details.body) + matchesAPIError(err, { + status: 412, + keywords: ["primary", "database"], + }) ) { return new Error( "cannot uninstall: app has a linked primary database — " + diff --git a/src/lib/error/apiError.test.ts b/src/lib/error/apiError.test.ts new file mode 100644 index 000000000..3aa5d50e5 --- /dev/null +++ b/src/lib/error/apiError.test.ts @@ -0,0 +1,74 @@ +import { describe, expect, it } from "@jest/globals"; +import { getAPIErrorDetails, matchesAPIError } from "./apiError.js"; + +function apiErrorLike(options: { + message?: string; + status: number; + type?: string; +}) { + return { + response: { + data: { + message: options.message, + type: options.type, + }, + status: options.status, + }, + }; +} + +describe("getAPIErrorDetails", () => { + it("extracts status and body from API-like errors", () => { + const details = getAPIErrorDetails( + apiErrorLike({ + status: 412, + type: "PreconditionFailed", + message: "primary database is linked", + }), + ); + + expect(details).toEqual({ + status: 412, + body: { + type: "PreconditionFailed", + message: "primary database is linked", + }, + }); + }); + + it("returns null for non-API errors", () => { + expect(getAPIErrorDetails(new Error("no response"))).toBeNull(); + }); +}); + +describe("matchesAPIError", () => { + it("matches by status and keywords", () => { + const err = apiErrorLike({ + status: 412, + type: "PreconditionFailed", + message: "App has linked primary database", + }); + + expect( + matchesAPIError(err, { + status: 412, + keywords: ["primary", "database"], + }), + ).toBe(true); + }); + + it("does not match when keywords are missing", () => { + const err = apiErrorLike({ + status: 412, + type: "PreconditionFailed", + message: "Some other precondition failed", + }); + + expect( + matchesAPIError(err, { + status: 412, + keywords: ["primary", "database"], + }), + ).toBe(false); + }); +}); diff --git a/src/lib/error/apiError.ts b/src/lib/error/apiError.ts new file mode 100644 index 000000000..d3e0c1ab4 --- /dev/null +++ b/src/lib/error/apiError.ts @@ -0,0 +1,66 @@ +interface APIErrorBody { + message?: string; + type?: string; +} + +interface APIErrorLike { + response?: { + data?: unknown; + status?: number; + }; +} + +export interface APIErrorDetails { + body: APIErrorBody | undefined; + status: number; +} + +export function getAPIErrorDetails(err: unknown): APIErrorDetails | null { + if (typeof err !== "object" || err === null) { + return null; + } + + const response = (err as APIErrorLike).response; + if (typeof response?.status !== "number") { + return null; + } + + const body = + typeof response.data === "object" && response.data !== null + ? (response.data as APIErrorBody) + : undefined; + + return { + status: response.status, + body, + }; +} + +export function matchesAPIError( + err: unknown, + options: { + keywords?: string[]; + status?: number; + }, +): boolean { + const details = getAPIErrorDetails(err); + if (details === null) { + return false; + } + + if (options.status !== undefined && details.status !== options.status) { + return false; + } + + if (!options.keywords || options.keywords.length === 0) { + return true; + } + + const signature = `${details.body?.type ?? ""} ${details.body?.message ?? ""}` + .toLowerCase() + .trim(); + + return options.keywords.every((keyword) => + signature.includes(keyword.toLowerCase()), + ); +}