From 981c44d9d671f1559ef1491d4b7a3eb5843602ce Mon Sep 17 00:00:00 2001 From: Ryan Ciehanski Date: Tue, 29 Sep 2026 00:22:41 -0500 Subject: [PATCH 1/2] fix(cli): report server errors plainly, and do not mistake a typo for a clean deployment --- script/api-errors.ts | 15 ++++++++++++ script/command-executor.ts | 19 ++++++++++++++- script/command-parser.ts | 11 +++++++-- script/management-sdk.ts | 27 ++++++-------------- test/cli.ts | 41 ++++++++++++++++++++++++++++++- test/command-parser.ts | 10 ++++++++ test/management-sdk.ts | 50 ++++++++++++++++++++++++++++++++++++++ 7 files changed, 150 insertions(+), 23 deletions(-) create mode 100644 script/api-errors.ts diff --git a/script/api-errors.ts b/script/api-errors.ts new file mode 100644 index 0000000..61a8695 --- /dev/null +++ b/script/api-errors.ts @@ -0,0 +1,15 @@ +/** + * The API answers errors with `{"message": "..."}`. Printing the envelope makes every refusal, an + * expired key or a read-only key among them, read like a bug in the CLI rather than an answer from + * the server. Anything that is not that shape is passed through untouched. + */ +export function messageFromResponseText(text: string): string { + if (!text) return text; + try { + const parsed = JSON.parse(text); + if (parsed && typeof parsed.message === "string" && parsed.message) return parsed.message; + } catch { + /* not JSON */ + } + return text; +} diff --git a/script/command-executor.ts b/script/command-executor.ts index a5fd65e..ce0555b 100644 --- a/script/command-executor.ts +++ b/script/command-executor.ts @@ -748,7 +748,24 @@ function deploymentErrors(command: cli.IDeploymentErrorsCommand): Promise return sdk .getDeploymentErrors(command.appName, command.deploymentName) - .then((result: DeploymentErrorsResult): void => printDeploymentErrors(command, result)) + .then( + (result: DeploymentErrorsResult): Promise | void => { + // The reports route answers an unknown app with an empty list, not a 404, so "no failures" and + // "no such app" look identical. Only an empty result pays for the check. + if (!result.entries.length) { + return sdk.getDeployment(command.appName, command.deploymentName).then( + (): void => printDeploymentErrors(command, result), + (): never => { + throw new Error( + `There is no "${command.deploymentName}" deployment of an app named "${command.appName}". ` + + `Run "dpctl deployment ls ${command.appName}" to see the deployments this app has.` + ); + } + ); + } + printDeploymentErrors(command, result); + } + ) .catch((error: any): void => { // Failure reports come from an account-level endpoint, so an app-scoped key gets a bare 403. // Say why, rather than letting it read as a permissions bug. diff --git a/script/command-parser.ts b/script/command-parser.ts index d358650..d588522 100644 --- a/script/command-parser.ts +++ b/script/command-parser.ts @@ -22,6 +22,13 @@ let lastFailMessage: string | undefined; let wasHelpShown = false; /** True when yargs already printed why the arguments were rejected, so callers need not add their own. */ +// yargs stringifies a .check callback that returns false, so its message is the source of +// `(argv) => isValidCommand`. The check is still doing its job (help, exit 1); only its wording is +// useless, and a real complaint about the same input is reported alongside it. +function isCheckFailure(msg: string): boolean { + return msg.startsWith("Argument check failed:"); +} + export function failureReported(): boolean { return !!lastFailMessage; } @@ -161,7 +168,7 @@ function addCommonConfiguration(yargs: yargs.Argv): void { .fail((msg: string) => { // yargs runs this handler once per nesting level (root, category, subcommand, ...), so the // same message arrives several times for a single mistake. Print each one once. - if (msg && msg !== lastFailMessage) { + if (msg && msg !== lastFailMessage && !isCheckFailure(msg)) { lastFailMessage = msg; console.error(chalk.red(`[Error] ${msg}`)); } @@ -1392,7 +1399,7 @@ yargs // A bare `dpctl` also lands here (yargs demands a command), and that is not a mistake to report: it // gets the greeting. Anything else typed something wrong and wants the reason, not the banner. const typedSomething = process.argv.slice(2).length > 0; - if (typedSomething && msg && msg !== lastFailMessage) { + if (typedSomething && msg && msg !== lastFailMessage && !isCheckFailure(msg)) { lastFailMessage = msg; console.error(chalk.red(`[Error] ${msg}`)); } diff --git a/script/management-sdk.ts b/script/management-sdk.ts index 3e9e9b8..2512558 100644 --- a/script/management-sdk.ts +++ b/script/management-sdk.ts @@ -9,6 +9,7 @@ import superagent = require("superagent"); import * as recursiveFs from "recursive-fs"; import * as yazl from "yazl"; import slash = require("slash"); +import { messageFromResponseText } from "./api-errors"; import Promise = Q.Promise; @@ -805,17 +806,10 @@ class AccountManager { }); } } else { - if (body) { - reject({ - message: body.message, - statusCode: this.getErrorStatus(err, res), - }); - } else { - reject({ - message: res.text, - statusCode: this.getErrorStatus(err, res), - }); - } + reject({ + message: messageFromResponseText(res.text), + statusCode: this.getErrorStatus(err, res), + }); } }); }); @@ -837,7 +831,8 @@ class AccountManager { } private getErrorMessage(error: Error, response: superagent.Response): string { - return response && response.text ? response.text : error.message; + const text = response && response.text; + return text ? messageFromResponseText(text) : error.message; } private attachCredentials(request: superagent.Request): void { @@ -860,13 +855,7 @@ class AccountManager { } function expoRouteError(res: superagent.Response): CodePushError { - let message: string = res.text; - try { - message = JSON.parse(res.text).message || message; - } catch { - /* not JSON; keep the raw text */ - } - return { message, statusCode: res.status }; + return { message: messageFromResponseText(res.text), statusCode: res.status }; } export = AccountManager; diff --git a/test/cli.ts b/test/cli.ts index 642ef38..38ece5c 100644 --- a/test/cli.ts +++ b/test/cli.ts @@ -694,10 +694,11 @@ describe("CLI", () => { var command: cli.IDeploymentErrorsCommand = { type: cli.CommandType.deploymentErrors, appName: "a", - deploymentName: "Nope", + deploymentName: "Production", format: "table", limit: 50, }; + sandbox.stub(cmdexec.sdk, "getDeploymentErrors").callsFake(() => Q({ entries: [], truncated: false })); cmdexec.execute(command).done((): void => { sinon.assert.calledOnce(log); @@ -705,6 +706,44 @@ describe("CLI", () => { done(); }); }); + + it("deploymentErrors tells a mistyped name apart from a clean deployment", (done: Mocha.Done): void => { + // The reports route answers an unknown app with an empty list, so without a check a typo reads as + // an all-clear, which is the wrong answer to give a CI job asking whether a release is failing. + var command: cli.IDeploymentErrorsCommand = { + type: cli.CommandType.deploymentErrors, + appName: "a", + deploymentName: "NoSuchDeployment", + format: "table", + limit: 50, + }; + sandbox.stub(cmdexec.sdk, "getDeploymentErrors").callsFake(() => Q({ entries: [], truncated: false })); + + cmdexec.execute(command).done( + (): void => done(new Error("Should have rejected")), + (error: any): void => { + assert.ok(/no "NoSuchDeployment" deployment of an app named "a"/.test(error.message), error.message); + sinon.assert.notCalled(log); + done(); + } + ); + }); + + it("deploymentErrors does not pay for the check when there are failures to show", (done: Mocha.Done): void => { + var command: cli.IDeploymentErrorsCommand = { + type: cli.CommandType.deploymentErrors, + appName: "a", + deploymentName: "Production", + format: "json", + limit: 50, + }; + var getDeployment: sinon.SinonSpy = sandbox.spy(cmdexec.sdk, "getDeployment"); + + cmdexec.execute(command).done((): void => { + sinon.assert.notCalled(getDeployment); + done(); + }); + }); it("accessKeyRemove removes access key", (done: Mocha.Done): void => { var command: cli.IAccessKeyRemoveCommand = { type: cli.CommandType.accessKeyRemove, diff --git a/test/command-parser.ts b/test/command-parser.ts index 6aefc0e..18eca28 100644 --- a/test/command-parser.ts +++ b/test/command-parser.ts @@ -197,6 +197,16 @@ describe("command line", function () { assert.strictEqual(parse("webhook", "update", "abc-123", "--name", "x").enabled, null); }); + it("a bare command category reports one error, not the source of a check callback", () => { + ["org", "app", "webhook", "deployment", "access-key"].forEach((category: string) => { + const result = run(category); + const errors = result.output.split("\n").filter((line: string) => line.indexOf("[Error]") >= 0); + assert.strictEqual(errors.length, 1, `${category} printed ${errors.length} errors:\n${errors.join("\n")}`); + assert.ok(!/Argument check failed/.test(result.output), `${category} leaked the check callback`); + assert.strictEqual(result.status, 1, `${category} should still exit 1`); + }); + }); + it("each command is registered once", () => { // A command registered twice silently wins with its last builder, so its newest options vanish // from the parse while still showing up in the source. diff --git a/test/management-sdk.ts b/test/management-sdk.ts index 31dc4f9..59c642f 100644 --- a/test/management-sdk.ts +++ b/test/management-sdk.ts @@ -5,6 +5,7 @@ import * as assert from "assert"; import * as Q from "q"; import AccountManager = require("../script/management-sdk"); +import { messageFromResponseText } from "../script/api-errors"; var request = require("superagent"); @@ -420,6 +421,55 @@ describe("Management SDK", () => { }, rejectHandler); }); + it("unwraps the API's error envelope, and leaves anything else alone", () => { + assert.strictEqual(messageFromResponseText('{"message":"This access key is read-only."}'), "This access key is read-only."); + assert.strictEqual(messageFromResponseText("upstream connect error"), "upstream connect error"); + // Shapes that are JSON but carry no usable message stay as they were, rather than becoming + // "undefined" or an empty error. + assert.strictEqual(messageFromResponseText('{"error":"nope"}'), '{"error":"nope"}'); + assert.strictEqual(messageFromResponseText('{"message":""}'), '{"message":""}'); + assert.strictEqual(messageFromResponseText('{"message":123}'), '{"message":123}'); + assert.strictEqual(messageFromResponseText(""), ""); + }); + + it("unwraps the envelope on the branch a real 4xx takes", () => { + // superagent reports a 4xx as an error, so production rejects through getCodePushError, not + // through the body-parsing branch below. superagent-mock cannot produce that combination (an + // error AND a response body), so this reaches the method directly. + const error: any = new Error("Forbidden"); + const response: any = { status: 403, text: '{"message":"This access key is read-only."}' }; + const built = (manager as any).getCodePushError(error, response); + assert.strictEqual(built.message, "This access key is read-only."); + assert.strictEqual(built.statusCode, 403); + + // No body at all: the transport error is still what gets reported. + const offline: any = new Error("connect ECONNREFUSED"); + assert.strictEqual((manager as any).getCodePushError(offline, undefined).message, "connect ECONNREFUSED"); + }); + + it("reports the server's message, not the JSON envelope it arrived in", (done: Mocha.Done) => { + mockReturn(JSON.stringify({ message: "This access key is read-only." }), 403, {}, /*throwOnError=*/ false); + manager.addApp("MyApp").then( + () => done(new Error("Should have rejected")), + (error: any) => { + assert.strictEqual(error.message, "This access key is read-only."); + assert.strictEqual(error.statusCode, 403); + done(); + } + ); + }); + + it("passes a non-JSON error body through untouched", (done: Mocha.Done) => { + mockReturn("upstream connect error", 502, {}, /*throwOnError=*/ false); + manager.addApp("MyApp").then( + () => done(new Error("Should have rejected")), + (error: any) => { + assert.strictEqual(error.message, "upstream connect error"); + done(); + } + ); + }); + it("identifies itself as dpctl, so a login is not a nameless CLI row", (done: Mocha.Done) => { mockReturn(JSON.stringify({ apps: [] }), 200); manager.getApps().done(() => { From 27273c9e5548aece88c8dad179b8f667a435dd3e Mon Sep 17 00:00:00 2001 From: Ryan Ciehanski Date: Tue, 29 Sep 2026 00:22:46 -0500 Subject: [PATCH 2/2] chore: release 1.2.1 --- package.json | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/package.json b/package.json index 58fc2c1..0dd7293 100644 --- a/package.json +++ b/package.json @@ -1,6 +1,6 @@ { "name": "@deploypulseio/dpctl", - "version": "1.2.0", + "version": "1.2.1", "description": "Management CLI for the DeployPulse CodePush service", "keywords": [ "deploypulse",