Give the batch commands one exit status and one JSON document

README.md

@@ -267,15 +267,27 @@ to draw nothing at all.
 
 ## For agents
 
-An agent driving this program should know four things:
-
-1. **`--json` on everything.** The payload is the API's own. Errors come back as
-   `{"error": "...", "causes": [...]}` on stdout in JSON mode, and the exit status is
-   non-zero.
-2. **stdout is data, stderr is commentary.** `--quiet` removes the commentary entirely.
-3. **`--yes` is required** for anything destructive selected by filter, because there is no
+An agent driving this program should know five things:
+
+1. **`--json` on everything.** The payload is the API's own. A command that failed
+   outright answers with `{"error": "...", "causes": [...]}` on stdout and a non-zero
+   exit status; a batch that ran answers as item 2 describes. Either way a command that
+   answered writes exactly one document to stdout, whether it worked or not — never a
+   summary with an error appended after it.
+2. **The exit status of a batch says how much got through.** `upload`, `download` and
+   `edit` exit **0** when nothing was refused, **1** when nothing got through, and **3**
+   when some items succeeded and some were refused. (2 is left to clap, which uses it for
+   a command line it could not parse.) A batch that ran writes one document either way —
+   `failed` and `failures` are in it, as they are not in a `--dry-run`, which lists a
+   plan rather than an outcome — and a run that was not wholly successful
+   adds `error` to that document rather than a second one after it. So `$?` says whether
+   to look and the summary says at what; there is no `causes` key on a batch, because the
+   run itself did not fail, some of its items did. A batch with nothing to do says so on
+   stderr and writes no document at all.
+3. **stdout is data, stderr is commentary.** `--quiet` removes the commentary entirely.
+4. **`--yes` is required** for anything destructive selected by filter, because there is no
    terminal to answer a prompt. Naming ids explicitly never prompts.
-4. **`IMOGEN_TOKEN` and `IMOGEN_SERVER`** authenticate a single invocation without touching
+5. **`IMOGEN_TOKEN` and `IMOGEN_SERVER`** authenticate a single invocation without touching
    the credentials file.
 
 ```bash

src/commands/assets.rs

@@ -512,22 +512,28 @@ pub async fn edit(ctx: &Context, args: &EditArgs) -> Result<()> {
         }
     }
 
-    if ctx.out.is_json() {
-        return ctx.out.json(&json!({
-            "updated": updated.len(),
-            "failed": failures.len(),
-            "items": updated,
-            "failures": failures,
-        }));
-    }
-    for failure in &failures {
-        ctx.out.warn(failure.message());
+    let report = json!({
+        "updated": updated.len(),
+        "failed": failures.len(),
+        "items": updated,
+        "failures": failures,
+    });
+
+    if !ctx.out.is_json() {
+        for failure in &failures {
+            ctx.out.warn(failure.message());
+        }
+        // A run that got nothing done does not close with a green line saying so: the
+        // sentence `main` is about to print is the whole of what happened.
+        if !updated.is_empty() {
+            ctx.out.note(ctx.out.paint(
+                &format!("Edited {}.", output::plural(updated.len(), "photograph")),
+                GREEN,
+            ));
+        }
     }
-    ctx.out.note(ctx.out.paint(
-        &format!("Edited {}.", output::plural(updated.len(), "photograph")),
-        GREEN,
-    ));
-    Ok(())
+
+    crate::commands::finish_batch(ctx, report, failures.len(), updated.len(), "photograph")
 }
 
 fn build_patch(args: &EditArgs) -> Result<AssetUpdate> {

src/commands/download.rs

@@ -99,39 +99,44 @@ pub async fn download(ctx: &Context, args: &DownloadArgs) -> Result<()> {
         }
     }
 
-    if ctx.out.is_json() {
-        return ctx.out.json(&json!({
-            "written": written.len(),
-            "skipped": skipped,
-            "failed": failures.len(),
-            "items": written,
-            "failures": failures,
-        }));
-    }
-    for failure in &failures {
-        ctx.out.warn(failure.message());
-    }
-    let bytes: u64 = written
-        .iter()
-        .filter_map(|item| item["bytes"].as_u64())
-        .sum();
-    ctx.out.note(ctx.out.paint(
-        &format!(
-            "Wrote {}, {}{}.",
-            output::plural(written.len(), "file"),
-            output::bytes(bytes),
-            if skipped > 0 {
-                format!(", {skipped} already there")
-            } else {
-                String::new()
-            }
-        ),
-        GREEN,
-    ));
-    if !failures.is_empty() {
-        bail!("{} failed", output::plural(failures.len(), "file"));
+    let report = json!({
+        "written": written.len(),
+        "skipped": skipped,
+        "failed": failures.len(),
+        "items": written,
+        "failures": failures,
+    });
+
+    if !ctx.out.is_json() {
+        for failure in &failures {
+            ctx.out.warn(failure.message());
+        }
+        // A run that got nothing through does not close with a green line saying so: the
+        // sentence `main` is about to print is the whole of what happened.
+        if written.len() + skipped > 0 {
+            let bytes: u64 = written
+                .iter()
+                .filter_map(|item| item["bytes"].as_u64())
+                .sum();
+            ctx.out.note(ctx.out.paint(
+                &format!(
+                    "Wrote {}, {}{}.",
+                    output::plural(written.len(), "file"),
+                    output::bytes(bytes),
+                    if skipped > 0 {
+                        format!(", {skipped} already there")
+                    } else {
+                        String::new()
+                    }
+                ),
+                GREEN,
+            ));
+        }
     }
-    Ok(())
+
+    // A file that was already there was not fetched, but it is on disk, which is what the
+    // caller asked for: it counts as got through rather than as a failure.
+    crate::commands::finish_batch(ctx, report, failures.len(), written.len() + skipped, "file")
 }
 
 async fn resolve(ctx: &Context, args: &DownloadArgs) -> Result<Vec<Asset>> {

src/commands/mod.rs

@@ -1,3 +1,8 @@
+use anyhow::Result;
+
+use crate::context::Context;
+use crate::errors::BatchFailure;
+
 pub mod account;
 pub mod admin;
 pub mod albums;
@@ -7,3 +12,25 @@ pub mod people;
 pub mod session;
 pub mod share;
 pub mod upload;
+
+/// How a batch ends, for the three commands that run one.
+///
+/// A person has already had the per-item warnings and the summary line by the time this
+/// is called. What is left is stdout and `$?`, and the two are decided together: in
+/// `--json` exactly one document reaches stdout — `summary` when nothing was refused, and
+/// otherwise that same summary carried out through `main` by `BatchFailure`. Never both.
+pub fn finish_batch(
+    ctx: &Context,
+    summary: serde_json::Value,
+    failed: usize,
+    succeeded: usize,
+    noun: &str,
+) -> Result<()> {
+    if failed == 0 {
+        if ctx.out.is_json() {
+            ctx.out.json(&summary)?;
+        }
+        return Ok(());
+    }
+    Err(BatchFailure::new(failed, succeeded, noun, summary).into())
+}

src/commands/upload.rs

@@ -196,40 +196,42 @@ pub async fn upload(ctx: &Context, args: &UploadArgs) -> Result<()> {
         }
     }
 
-    if ctx.out.is_json() {
-        ctx.out.json(&json!({
-            "uploaded": uploaded.len() - duplicates,
-            "duplicates": duplicates,
-            "failed": failures.len(),
-            "addedToAlbums": added,
-            "items": uploaded,
-            "failures": failures,
-        }))?;
-    } else {
+    let summary = json!({
+        "uploaded": uploaded.len() - duplicates,
+        "duplicates": duplicates,
+        "failed": failures.len(),
+        "addedToAlbums": added,
+        "items": uploaded,
+        "failures": failures,
+    });
+
+    if !ctx.out.is_json() {
         for failure in &failures {
             ctx.out.warn(failure.message());
         }
-        let summary = format!(
-            "Uploaded {}{}{}.",
-            output::plural(uploaded.len() - duplicates, "file"),
-            if duplicates > 0 {
-                format!(", {duplicates} already there")
-            } else {
-                String::new()
-            },
-            if added > 0 {
-                format!(", {added} filed into albums")
-            } else {
-                String::new()
-            }
-        );
-        ctx.out.note(ctx.out.paint(&summary, GREEN));
+        // A run that got nothing through does not close with a green line saying so: the
+        // sentence `main` is about to print is the whole of what happened.
+        if !uploaded.is_empty() {
+            let sentence = format!(
+                "Uploaded {}{}{}.",
+                output::plural(uploaded.len() - duplicates, "file"),
+                if duplicates > 0 {
+                    format!(", {duplicates} already there")
+                } else {
+                    String::new()
+                },
+                if added > 0 {
+                    format!(", {added} filed into albums")
+                } else {
+                    String::new()
+                }
+            );
+            ctx.out.note(ctx.out.paint(&sentence, GREEN));
+        }
     }
 
-    if !failures.is_empty() {
-        bail!("{} failed", output::plural(failures.len(), "file"));
-    }
-    Ok(())
+    // A file the server already had is not a file that failed, so it counts as got through.
+    crate::commands::finish_batch(ctx, summary, failures.len(), uploaded.len(), "file")
 }
 
 /// One line of the `--report` JSONL: what became of one file, in the API's own field

src/errors.rs

@@ -102,6 +102,85 @@ impl Failure {
     }
 }
 
+/// What `$?` says. A run that produced nothing exits `FAILED`; a run in which some items
+/// got through and some did not exits `PARTIAL`, because "nothing worked" and "most of it
+/// worked" call for different things from the script that asked. 2 is deliberately not
+/// used: clap already exits with it for a command line it could not parse.
+pub const EXIT_FAILED: i32 = 1;
+pub const EXIT_PARTIAL: i32 = 3;
+
+/// The end of a batch in which the server refused some of the items.
+///
+/// The summary is not thrown away by the failure: the run happened, and what happened to
+/// each item is the answer. So it travels with the error and `main` writes it once — as
+/// the single JSON document the contract promises, or as the sentence that closes a
+/// table. A command that wrote that document itself and *then* returned an error put two
+/// documents on the same stdout, which no ordinary parser will read:
+/// ergofobe/imogen-cli#27.
+#[derive(Debug)]
+pub struct BatchFailure {
+    message: String,
+    everything: bool,
+    report: serde_json::Value,
+}
+
+impl BatchFailure {
+    pub fn new(failed: usize, succeeded: usize, noun: &str, report: serde_json::Value) -> Self {
+        let everything = succeeded == 0;
+        let message = if everything {
+            format!("{} failed", crate::output::plural(failed, noun))
+        } else {
+            format!(
+                "{failed} of {} failed",
+                crate::output::plural(failed + succeeded, noun)
+            )
+        };
+        let mut failure = Self {
+            message,
+            everything,
+            report,
+        };
+        // The one document says both what the run did and that it did not all work, so a
+        // reader looking only for `error` is not misled by a summary full of items.
+        if let Some(object) = failure.report.as_object_mut() {
+            object.insert(
+                "error".to_string(),
+                serde_json::Value::String(failure.message.clone()),
+            );
+        }
+        failure
+    }
+
+    pub fn exit_code(&self) -> i32 {
+        if self.everything {
+            EXIT_FAILED
+        } else {
+            EXIT_PARTIAL
+        }
+    }
+
+    /// The one document `--json` puts on stdout for a failed batch.
+    pub fn report(&self) -> &serde_json::Value {
+        &self.report
+    }
+}
+
+impl std::fmt::Display for BatchFailure {
+    fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
+        f.write_str(&self.message)
+    }
+}
+
+impl std::error::Error for BatchFailure {}
+
+/// The batch failure anywhere in the chain, looked for the way `details_in` looks for a
+/// rejection: a command that added context leaves this underneath it.
+pub fn batch_in(error: &anyhow::Error) -> Option<&BatchFailure> {
+    error
+        .chain()
+        .find_map(|cause| cause.downcast_ref::<BatchFailure>())
+}
+
 /// A rejection as the server sends one, for the tests of everything that renders it.
 #[cfg(test)]
 pub fn rejection(details: &[(&str, &[&str])]) -> imogen_sdk::Error {
@@ -192,6 +271,47 @@ mod tests {
         assert_eq!(failure.message().lines().count(), 1);
     }
 
+    /// A run that got nothing done and a run that got most of it done ask different
+    /// things of the script that called, so they do not share a status.
+    #[test]
+    fn a_batch_that_got_nowhere_is_told_apart_from_one_that_got_part_way() {
+        let nothing = BatchFailure::new(3, 0, "file", serde_json::json!({ "failed": 3 }));
+        assert_eq!(nothing.exit_code(), EXIT_FAILED);
+        assert_eq!(nothing.to_string(), "3 files failed");
+
+        let some = BatchFailure::new(1, 4, "file", serde_json::json!({ "failed": 1 }));
+        assert_eq!(some.exit_code(), EXIT_PARTIAL);
+        assert_eq!(some.to_string(), "1 of 5 files failed");
+    }
+
+    /// The document `--json` writes for a failed batch is the summary of the run, with
+    /// the sentence added so a reader looking only for `error` is not misled by it.
+    #[test]
+    fn the_failed_batchs_document_is_the_summary_plus_the_sentence() {
+        let failure = BatchFailure::new(
+            1,
+            1,
+            "photograph",
+            serde_json::json!({ "updated": 1, "failed": 1 }),
+        );
+        let report = failure.report();
+        assert_eq!(report["updated"], 1);
+        assert_eq!(report["failed"], 1);
+        assert_eq!(report["error"], "1 of 2 photographs failed");
+    }
+
+    /// Found the way a rejection is found, so a command that adds context on the way out
+    /// does not cost the run its exit status.
+    #[test]
+    fn the_batch_survives_a_commands_own_context() {
+        let error = anyhow::Error::new(BatchFailure::new(1, 1, "file", serde_json::json!({})))
+            .context("while uploading");
+        assert_eq!(
+            batch_in(&error).map(BatchFailure::exit_code),
+            Some(EXIT_PARTIAL)
+        );
+    }
+
     #[test]
     fn a_download_failure_is_named_by_the_asset_it_was_fetching() {
         let error = rejection(&[("variant", &["Unknown variant"])]);