fix: error messages

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",

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;
+}

script/command-executor.ts

@@ -748,7 +748,24 @@ function deploymentErrors(command: cli.IDeploymentErrorsCommand): Promise<void>
 
   return sdk
     .getDeploymentErrors(command.appName, command.deploymentName)
-    .then((result: DeploymentErrorsResult): void => printDeploymentErrors(command, result))
+    .then(
+      (result: DeploymentErrorsResult): Promise<void> | 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.

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}`));
     }

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(<CodePushError>{
-              message: body.message,
-              statusCode: this.getErrorStatus(err, res),
-            });
-          } else {
-            reject(<CodePushError>{
-              message: res.text,
-              statusCode: this.getErrorStatus(err, res),
-            });
-          }
+          reject(<CodePushError>{
+            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<any>): 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 <CodePushError>{ message, statusCode: res.status };
+  return <CodePushError>{ message: messageFromResponseText(res.text), statusCode: res.status };
 }
 
 export = AccountManager;

test/cli.ts

@@ -694,17 +694,56 @@ 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);
       assert.ok(String(log.args[0][0]).startsWith("No failed updates have been reported"));
       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,

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.

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(() => {