From 6cb3cdda5514cca6e95a048b11f2afd992a059e7 Mon Sep 17 00:00:00 2001 From: Martin Helmich Date: Thu, 2 Jul 2026 11:25:32 +0200 Subject: [PATCH] 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); } }