From 0b5b5d8ac175802c30ac84d65c048b0d6b1a4425 Mon Sep 17 00:00:00 2001 From: Jim Phillips <5315024+ergofobe@users.noreply.github.com> Date: Tue, 15 Sep 2026 22:39:56 -0400 Subject: [PATCH 1/5] Give the batch commands one exit status and one JSON document `upload`, `download` and `edit` each ran a batch and each decided for itself what a refusal meant. An edit the server refused entirely printed "Edited 0 photographs." and exited 0. `download --json` returned before the failure was raised and exited 0 where the same run in prose exited 1. `upload --json` wrote its summary and then let the failure write a second document to the same stdout, which no ordinary parser will read. One rule now, in `finish_batch`: - Nothing refused exits 0; nothing got through exits 1; some of each exits 3. "Nothing worked" and "most of it worked" ask different things of the script that called, and 2 is left to clap's usage error. - `--json` puts exactly one document on stdout whatever happened. On a failure the command writes nothing there: the summary travels out with `BatchFailure` and `main` writes it once, with the sentence added under `error` so a reader looking only for that key is not misled. This is a behaviour change for scripts. A run that today exits 0 because every edit was refused, or because a download failed under `--json`, now exits non-zero, and a partly refused run exits 3 rather than 1. The tests run the real program against a stub server, because `$?` and the bytes on stdout only exist once there is a process; stdout is parsed as a stream and the documents are counted, since a second one is invisible to a grep. Closes #27 Co-Authored-By: Claude Opus 5 --- README.md | 15 +- src/commands/assets.rs | 32 ++-- src/commands/download.rs | 65 ++++---- src/commands/mod.rs | 27 ++++ src/commands/upload.rs | 26 ++-- src/errors.rs | 120 +++++++++++++++ src/main.rs | 21 ++- tests/batch_exit_status.rs | 302 +++++++++++++++++++++++++++++++++++++ 8 files changed, 539 insertions(+), 69 deletions(-) create mode 100644 tests/batch_exit_status.rs diff --git a/README.md b/README.md index c73f46f..92135b5 100644 --- a/README.md +++ b/README.md @@ -267,15 +267,20 @@ to draw nothing at all. ## For agents -An agent driving this program should know four things: +An agent driving this program should know five 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 + non-zero. Exactly one JSON document reaches stdout, whatever happened. +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. The summary names `failed` and lists + `failures` either way, so `$?` says whether to look and the document says at what. (2 + is left to clap, which uses it for a command line it could not parse.) +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 diff --git a/src/commands/assets.rs b/src/commands/assets.rs index 2d68241..672eb49 100644 --- a/src/commands/assets.rs +++ b/src/commands/assets.rs @@ -512,22 +512,24 @@ 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()); + } + 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 { diff --git a/src/commands/download.rs b/src/commands/download.rs index ebf6593..382afbf 100644 --- a/src/commands/download.rs +++ b/src/commands/download.rs @@ -99,39 +99,40 @@ 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()); + } + 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> { diff --git a/src/commands/mod.rs b/src/commands/mod.rs index aa2ff9e..6cb6563 100644 --- a/src/commands/mod.rs +++ b/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()) +} diff --git a/src/commands/upload.rs b/src/commands/upload.rs index f94768d..e1e1392 100644 --- a/src/commands/upload.rs +++ b/src/commands/upload.rs @@ -196,16 +196,16 @@ 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 report = 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()); } @@ -226,10 +226,8 @@ pub async fn upload(ctx: &Context, args: &UploadArgs) -> Result<()> { ctx.out.note(ctx.out.paint(&summary, 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, report, failures.len(), uploaded.len(), "file") } /// One line of the `--report` JSONL: what became of one file, in the API's own field diff --git a/src/errors.rs b/src/errors.rs index 50a883e..954ff48 100644 --- a/src/errors.rs +++ b/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::()) +} + /// 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"])]); diff --git a/src/main.rs b/src/main.rs index a1c5608..2abe159 100644 --- a/src/main.rs +++ b/src/main.rs @@ -16,7 +16,7 @@ use clap::{CommandFactory, Parser}; use crate::cli::{Cli, Command}; use crate::context::Context; -use crate::errors::{detail_lines, details_in, Details}; +use crate::errors::{batch_in, detail_lines, details_in, Details, EXIT_FAILED}; use crate::output::{Output, RED}; fn main() { @@ -36,10 +36,19 @@ fn main() { let out = Output::new(cli.global.json, cli.global.no_color, cli.global.quiet); if let Err(error) = runtime.block_on(run(&cli)) { report(&out, &error); - std::process::exit(1); + std::process::exit(exit_code(&error)); } } +/// A batch that got some of its work done is worth telling apart from one that got none, +/// because the two ask different things of the script that ran it. Everything else is an +/// ordinary failure. +fn exit_code(error: &anyhow::Error) -> i32 { + batch_in(error) + .map(|batch| batch.exit_code()) + .unwrap_or(EXIT_FAILED) +} + /// Rust ignores SIGPIPE so that a write to a closed pipe surfaces as an error rather than /// killing the process. For a tool built to be piped — `imogen ls --ids | head` — that /// turns an ordinary early exit into a panic, so the default is put back. @@ -60,7 +69,13 @@ fn restore_sigpipe() {} fn report(out: &Output, error: &anyhow::Error) { let details = details_in(error); if out.is_json() { - let _ = out.json(&error_json(error, details)); + // A batch ran to completion and carries the report of what it did; anything else + // has only the failure to describe. Either way this is the one document on + // stdout — the command wrote nothing there: ergofobe/imogen-cli#27. + let _ = match batch_in(error) { + Some(batch) => out.json(batch.report()), + None => out.json(&error_json(error, details)), + }; return; } eprintln!("{}", out.paint(&format!("error: {error}"), RED)); diff --git a/tests/batch_exit_status.rs b/tests/batch_exit_status.rs new file mode 100644 index 0000000..4ab1cc0 --- /dev/null +++ b/tests/batch_exit_status.rs @@ -0,0 +1,302 @@ +//! What a script sees when a batch partly fails: the exit status, and the one document +//! `--json` promises. ergofobe/imogen-cli#27. +//! +//! These run the real program against a stub that answers the few requests a batch makes, +//! because the two things under test — `$?` and the bytes on stdout — only exist once +//! there is a process. A second JSON document is invisible to a grep and obvious to a +//! parser, so stdout is parsed as a stream and the documents are counted. + +use std::io::{BufRead, BufReader, Read, Write}; +use std::net::{TcpListener, TcpStream}; +use std::path::Path; +use std::process::{Command, Output}; +use std::sync::atomic::{AtomicUsize, Ordering}; +use std::sync::Arc; + +/// Complete success, every item refused, and some of each. The statuses the program hands +/// back for the last two are what a script reads. +const EXIT_OK: i32 = 0; +const EXIT_FAILED: i32 = 1; +const EXIT_PARTIAL: i32 = 3; + +// --- the stub ------------------------------------------------------------------------- + +struct Server { + base: String, +} + +type Answer = dyn Fn(&str, &str) -> (u16, String) + Send + Sync + 'static; + +impl Server { + /// Answers every request with whatever `answer` makes of its method and path, on a + /// port the operating system picks. One request per connection, closed afterwards, so + /// the stub never has to understand keep-alive. + fn start(answer: impl Fn(&str, &str) -> (u16, String) + Send + Sync + 'static) -> Self { + let listener = TcpListener::bind("127.0.0.1:0").expect("a port"); + let base = format!("http://{}", listener.local_addr().expect("an address")); + let answer: Arc = Arc::new(answer); + std::thread::spawn(move || { + for stream in listener.incoming().flatten() { + let answer = answer.clone(); + std::thread::spawn(move || handle(stream, answer.as_ref())); + } + }); + Self { base } + } +} + +fn handle(stream: TcpStream, answer: &Answer) { + let mut reader = BufReader::new(match stream.try_clone() { + Ok(clone) => clone, + Err(_) => return, + }); + + let mut request_line = String::new(); + if reader.read_line(&mut request_line).unwrap_or(0) == 0 { + return; + } + let mut words = request_line.split_whitespace(); + let method = words.next().unwrap_or_default().to_string(); + let target = words.next().unwrap_or_default().to_string(); + + let mut length = 0usize; + let mut chunked = false; + loop { + let mut line = String::new(); + if reader.read_line(&mut line).unwrap_or(0) == 0 { + break; + } + if line.trim().is_empty() { + break; + } + if let Some((name, value)) = line.split_once(':') { + match name.trim().to_ascii_lowercase().as_str() { + "content-length" => length = value.trim().parse().unwrap_or(0), + "transfer-encoding" => chunked = value.trim().eq_ignore_ascii_case("chunked"), + _ => {} + } + } + } + + // The body is drained before answering, so an upload is never writing into a socket + // that has already been closed underneath it. + if chunked { + drain_chunked(&mut reader); + } else if length > 0 { + let mut body = vec![0u8; length]; + let _ = reader.read_exact(&mut body); + } + + let path = target.split('?').next().unwrap_or(&target); + let (status, payload) = answer(&method, path); + let mut stream = stream; + let _ = write!( + stream, + "HTTP/1.1 {status} \r\nContent-Type: application/json\r\nContent-Length: {}\r\nConnection: close\r\n\r\n", + payload.len() + ); + let _ = stream.write_all(payload.as_bytes()); + let _ = stream.flush(); +} + +fn drain_chunked(reader: &mut BufReader) { + loop { + let mut header = String::new(); + if reader.read_line(&mut header).unwrap_or(0) == 0 { + return; + } + let size = usize::from_str_radix(header.trim(), 16).unwrap_or(0); + let mut chunk = vec![0u8; size + 2]; + if reader.read_exact(&mut chunk).is_err() || size == 0 { + return; + } + } +} + +// --- what the stub says --------------------------------------------------------------- + +fn rejected() -> (u16, String) { + ( + 400, + r#"{"error":{"code":"validation_failed","message":"The request did not match what this endpoint expects","details":{"capturedAt":["Invalid date"]}}}"# + .to_string(), + ) +} + +fn not_found() -> (u16, String) { + ( + 404, + r#"{"error":{"code":"not_found","message":"No such photograph"}}"#.to_string(), + ) +} + +fn asset(id: &str) -> String { + format!( + r#"{{"id":"{id}","ownerId":"owner-1","type":"image","status":"ready", + "originalFilename":"harbour.jpg","mimeType":"image/jpeg","checksum":"abc", + "sizeBytes":12,"width":null,"height":null,"duration":null, + "capturedAt":"2024-06-01T09:30:00.000Z","capturedAtIsExact":true, + "capturedAtOriginal":null,"capturedAtOriginalIsExact":null, + "createdAt":"2024-06-01T09:30:00.000Z","updatedAt":"2024-06-01T09:30:00.000Z", + "deletedAt":null,"favorite":false,"archived":false,"description":null, + "exif":null,"location":null,"placeholderColor":null,"livePhotoVideoId":null, + "deviceAssetId":null}}"# + ) +} + +fn uploaded(id: &str) -> (u16, String) { + ( + 201, + format!(r#"{{"asset":{},"duplicate":false}}"#, asset(id)), + ) +} + +// --- running the program --------------------------------------------------------------- + +fn imogen(server: &str, home: &Path, args: &[&str]) -> Output { + Command::new(env!("CARGO_BIN_EXE_imogen")) + .env("IMOGEN_CONFIG", home.join("config.json")) + .env("NO_COLOR", "1") + .env_remove("IMOGEN_PROFILE") + .arg("--server") + .arg(server) + .arg("--token") + .arg("test-token") + .args(args) + .output() + .expect("the program runs") +} + +/// Everything on stdout, read the way `jq` reads it. A run that wrote a summary and then +/// an error wrote two documents, and this is where that shows up. +fn documents(output: &Output) -> Vec { + serde_json::Deserializer::from_slice(&output.stdout) + .into_iter::() + .collect::, _>>() + .unwrap_or_else(|error| { + panic!( + "stdout is not a stream of JSON documents: {error}\n{}", + String::from_utf8_lossy(&output.stdout) + ) + }) +} + +fn photograph(directory: &Path, name: &str) -> String { + let path = directory.join(name); + std::fs::write(&path, b"not really a photograph").expect("a file"); + path.display().to_string() +} + +// --- the tests -------------------------------------------------------------------------- + +/// An edit the server refused every part of used to print "Edited 0 photographs." and +/// exit 0, so a script that submitted a hundred edits and had a hundred refused saw +/// success. +#[test] +fn an_edit_the_server_refused_entirely_fails() { + let home = tempfile::tempdir().expect("a directory"); + let server = Server::start(|_method, _path| rejected()); + + let prose = imogen( + &server.base, + home.path(), + &["edit", "asset-1", "asset-2", "--favorite"], + ); + assert_eq!(prose.status.code(), Some(EXIT_FAILED)); + + let json = imogen( + &server.base, + home.path(), + &["--json", "edit", "asset-1", "asset-2", "--favorite"], + ); + assert_eq!(json.status.code(), Some(EXIT_FAILED)); + let documents = documents(&json); + assert_eq!(documents.len(), 1, "{documents:?}"); + assert_eq!(documents[0]["failed"], 2); + assert_eq!(documents[0]["updated"], 0); +} + +/// `upload --json` wrote its summary and then let the failure write a second document to +/// the same stdout, which no ordinary parser will read. +#[test] +fn a_partly_refused_upload_writes_one_document() { + let home = tempfile::tempdir().expect("a directory"); + let attempt = Arc::new(AtomicUsize::new(0)); + let server = Server::start(move |_method, _path| { + // One file through, one refused — whichever order the concurrent uploads arrive in. + match attempt.fetch_add(1, Ordering::SeqCst) { + 0 => rejected(), + _ => uploaded("asset-1"), + } + }); + + let first = photograph(home.path(), "one.jpg"); + let second = photograph(home.path(), "two.jpg"); + let output = imogen( + &server.base, + home.path(), + &["--json", "upload", &first, &second], + ); + + let documents = documents(&output); + assert_eq!(documents.len(), 1, "{documents:?}"); + assert_eq!(documents[0]["failed"], 1); + assert_eq!(documents[0]["uploaded"], 1); + assert_eq!( + documents[0]["failures"][0]["details"]["capturedAt"][0], + "Invalid date" + ); + assert_eq!(output.status.code(), Some(EXIT_PARTIAL)); +} + +/// The same run reported differently depending on which audience asked: the JSON branch +/// returned before the failure was raised, so `--json` exited 0 where prose exited 1. +#[test] +fn a_download_reports_the_same_failure_to_both_audiences() { + let home = tempfile::tempdir().expect("a directory"); + let server = Server::start(|_method, path| { + if path.ends_with("/download") { + return not_found(); + } + (200, asset("asset-1")) + }); + let out = home.path().join("out"); + + let prose = imogen( + &server.base, + home.path(), + &["download", "asset-1", "-o", &out.display().to_string()], + ); + let json = imogen( + &server.base, + home.path(), + &[ + "--json", + "download", + "asset-1", + "-o", + &out.display().to_string(), + ], + ); + + assert_eq!(json.status.code(), prose.status.code()); + assert_eq!(json.status.code(), Some(EXIT_FAILED)); + let documents = documents(&json); + assert_eq!(documents.len(), 1, "{documents:?}"); + assert_eq!(documents[0]["failed"], 1); +} + +/// The rule only earns its keep if a run that worked still says so. +#[test] +fn a_batch_that_worked_still_exits_zero() { + let home = tempfile::tempdir().expect("a directory"); + let server = Server::start(|_method, _path| uploaded("asset-1")); + + let only = photograph(home.path(), "one.jpg"); + let output = imogen(&server.base, home.path(), &["--json", "upload", &only]); + + assert_eq!(output.status.code(), Some(EXIT_OK)); + let documents = documents(&output); + assert_eq!(documents.len(), 1, "{documents:?}"); + assert_eq!(documents[0]["failed"], 0); +} From e8ffa7cf3cf3a6eddcc2350eb30acf05bbd3e81c Mon Sep 17 00:00:00 2001 From: Jim Phillips <5315024+ergofobe@users.noreply.github.com> Date: Tue, 15 Sep 2026 22:43:01 -0400 Subject: [PATCH 2/5] Say in the README what a batch's one document holds MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The bullet above it promises `{"error", "causes"}` for a `--json` error, and a batch failure's document is the run's summary with `error` added — no `causes`, because the run did not fail, some of its items did. A reader keying off `causes` had no way to know that from the page. Co-Authored-By: Claude Opus 5 --- README.md | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/README.md b/README.md index 92135b5..64febeb 100644 --- a/README.md +++ b/README.md @@ -274,9 +274,12 @@ An agent driving this program should know five things: non-zero. Exactly one JSON document reaches stdout, whatever happened. 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. The summary names `failed` and lists - `failures` either way, so `$?` says whether to look and the document says at what. (2 - is left to clap, which uses it for a command line it could not parse.) + when some items succeeded and some were refused. (2 is left to clap, which uses it for + a command line it could not parse.) Their one document is the summary of the run + either way — `failed` and `failures` are always there — and a run that was not wholly + successful adds `error` to it rather than a second document 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. 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. From d9dcd2cae70ee29cfef36c94ba1dd335df821d98 Mon Sep 17 00:00:00 2001 From: Jim Phillips <5315024+ergofobe@users.noreply.github.com> Date: Tue, 15 Sep 2026 22:46:04 -0400 Subject: [PATCH 3/5] Stop a run that got nothing through closing in green MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `edit` printed "Edited 0 photographs." in green and then, now that the run is non-zero, `error: 2 photographs failed` under it — the success line contradicting the exit status. `upload` and `download` had the same shape. All three now leave the closing line out when nothing got through: the warnings and the sentence `main` prints are the whole of what happened. Also: the README claimed one document reaches stdout whatever happened, which an empty batch does not do — it says so on stderr and writes nothing. And the partly-refused upload test picked its victim by request ordinal, so a preflight the program learns to make one day would have become the refused request; it keys on the endpoint instead. Co-Authored-By: Claude Opus 5 --- README.md | 14 ++++++++------ src/commands/assets.rs | 12 ++++++++---- src/commands/download.rs | 38 +++++++++++++++++++++----------------- src/commands/upload.rs | 34 +++++++++++++++++++--------------- tests/batch_exit_status.rs | 22 ++++++++++++++++------ 5 files changed, 72 insertions(+), 48 deletions(-) diff --git a/README.md b/README.md index 64febeb..ac73638 100644 --- a/README.md +++ b/README.md @@ -271,15 +271,17 @@ An agent driving this program should know five 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. Exactly one JSON document reaches stdout, whatever happened. + non-zero. 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.) Their one document is the summary of the run - either way — `failed` and `failures` are always there — and a run that was not wholly - successful adds `error` to it rather than a second document 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 command line it could not parse.) A batch that ran writes one document either way — + `failed` and `failures` are always in it — 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. diff --git a/src/commands/assets.rs b/src/commands/assets.rs index 672eb49..54fb553 100644 --- a/src/commands/assets.rs +++ b/src/commands/assets.rs @@ -523,10 +523,14 @@ pub async fn edit(ctx: &Context, args: &EditArgs) -> Result<()> { for failure in &failures { ctx.out.warn(failure.message()); } - ctx.out.note(ctx.out.paint( - &format!("Edited {}.", output::plural(updated.len(), "photograph")), - GREEN, - )); + // 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, + )); + } } crate::commands::finish_batch(ctx, report, failures.len(), updated.len(), "photograph") diff --git a/src/commands/download.rs b/src/commands/download.rs index 382afbf..fd94ad4 100644 --- a/src/commands/download.rs +++ b/src/commands/download.rs @@ -111,23 +111,27 @@ pub async fn download(ctx: &Context, args: &DownloadArgs) -> Result<()> { 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, - )); + // 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, + )); + } } // A file that was already there was not fetched, but it is on disk, which is what the diff --git a/src/commands/upload.rs b/src/commands/upload.rs index e1e1392..5208c0e 100644 --- a/src/commands/upload.rs +++ b/src/commands/upload.rs @@ -209,21 +209,25 @@ pub async fn upload(ctx: &Context, args: &UploadArgs) -> Result<()> { 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 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 file the server already had is not a file that failed, so it counts as got through. diff --git a/tests/batch_exit_status.rs b/tests/batch_exit_status.rs index 4ab1cc0..de92239 100644 --- a/tests/batch_exit_status.rs +++ b/tests/batch_exit_status.rs @@ -203,6 +203,13 @@ fn an_edit_the_server_refused_entirely_fails() { &["edit", "asset-1", "asset-2", "--favorite"], ); assert_eq!(prose.status.code(), Some(EXIT_FAILED)); + // Nothing was edited, so nothing closes the run by saying how much was. + let commentary = String::from_utf8_lossy(&prose.stderr); + assert!(!commentary.contains("Edited 0"), "{commentary}"); + assert!( + commentary.contains("capturedAt: Invalid date"), + "{commentary}" + ); let json = imogen( &server.base, @@ -221,13 +228,16 @@ fn an_edit_the_server_refused_entirely_fails() { #[test] fn a_partly_refused_upload_writes_one_document() { let home = tempfile::tempdir().expect("a directory"); - let attempt = Arc::new(AtomicUsize::new(0)); - let server = Server::start(move |_method, _path| { - // One file through, one refused — whichever order the concurrent uploads arrive in. - match attempt.fetch_add(1, Ordering::SeqCst) { - 0 => rejected(), - _ => uploaded("asset-1"), + let uploads = Arc::new(AtomicUsize::new(0)); + let server = Server::start(move |method, path| { + // One file through, one refused — whichever order the concurrent uploads arrive + // in. Counted by endpoint rather than by request, so a preflight the program + // learns to make one day does not quietly become the refused one. + let upload = method == "POST" && path == "/api/v1/assets"; + if upload && uploads.fetch_add(1, Ordering::SeqCst) == 0 { + return rejected(); } + uploaded("asset-1") }); let first = photograph(home.path(), "one.jpg"); From c8d341ffaf9ecc0785c6a4ef0cd4044e479756e1 Mon Sep 17 00:00:00 2001 From: Jim Phillips <5315024+ergofobe@users.noreply.github.com> Date: Tue, 15 Sep 2026 22:48:37 -0400 Subject: [PATCH 4/5] Stop calling the upload summary what the JSONL handle is called MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two `report`s in one function, the file handle and the JSON answer. The JSONL writes all happen before the shadow, so nothing was wrong today — it is the next edit that would find out. The answer is `summary`, which is what `finish_batch` calls its parameter, and the prose line is the `sentence`. The README's guarantee also covered `--dry-run`, which lists a plan and has no `failed` to give. Co-Authored-By: Claude Opus 5 --- README.md | 3 ++- src/commands/upload.rs | 8 ++++---- 2 files changed, 6 insertions(+), 5 deletions(-) diff --git a/README.md b/README.md index ac73638..5cdb6d7 100644 --- a/README.md +++ b/README.md @@ -277,7 +277,8 @@ An agent driving this program should know five things: `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 always in it — and a run that was not wholly successful + `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 diff --git a/src/commands/upload.rs b/src/commands/upload.rs index 5208c0e..5602c65 100644 --- a/src/commands/upload.rs +++ b/src/commands/upload.rs @@ -196,7 +196,7 @@ pub async fn upload(ctx: &Context, args: &UploadArgs) -> Result<()> { } } - let report = json!({ + let summary = json!({ "uploaded": uploaded.len() - duplicates, "duplicates": duplicates, "failed": failures.len(), @@ -212,7 +212,7 @@ pub async fn upload(ctx: &Context, args: &UploadArgs) -> Result<()> { // 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 summary = format!( + let sentence = format!( "Uploaded {}{}{}.", output::plural(uploaded.len() - duplicates, "file"), if duplicates > 0 { @@ -226,12 +226,12 @@ pub async fn upload(ctx: &Context, args: &UploadArgs) -> Result<()> { String::new() } ); - ctx.out.note(ctx.out.paint(&summary, GREEN)); + ctx.out.note(ctx.out.paint(&sentence, GREEN)); } } // A file the server already had is not a file that failed, so it counts as got through. - crate::commands::finish_batch(ctx, report, failures.len(), uploaded.len(), "file") + 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 From 5c82996b690beb869a5c5aa821bd7d3d6f5b351e Mon Sep 17 00:00:00 2001 From: Jim Phillips <5315024+ergofobe@users.noreply.github.com> Date: Tue, 15 Sep 2026 22:50:26 -0400 Subject: [PATCH 5/5] Stop the two agent bullets contradicting each other One promised `causes` on every JSON error; the other said a batch has none. The first now says which kind of failure it describes. Co-Authored-By: Claude Opus 5 --- README.md | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/README.md b/README.md index 5cdb6d7..25ff35b 100644 --- a/README.md +++ b/README.md @@ -269,10 +269,11 @@ to draw nothing at all. An agent driving this program should know five 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. A command that answered writes exactly one document to stdout, whether it - worked or not — never a summary with an error appended after it. +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