CLI: edit rejects unknown options instead of writing an unchanged copy (#133) (#227)

A misspelled option such as `--rotat` fell through the argument loop's
`_ => {}`, so `pdfcraft-cli edit` wrote a byte-identical copy of the input
and exited 0: an apparently successful edit that did nothing.

Every argument is now parsed before the file is opened. An unknown `--option`
fails with the list of accepted ones, an option without its value is refused,
and a second positional argument is reported. The documented options behave
as before (`--out`, `--password`, `--full` and the edits), and nothing is
written for a rejected command line.

Co-authored-by: William Stock <[email protected]>
Co-authored-by: Claude Fable 5.1 <[email protected]>
Co-authored-by: Brandon Thomas <[email protected]>

apps/pdfcraft-cli/src/main.rs

@@ -204,18 +204,44 @@ fn page_list(s: &str) -> Result<Vec<usize>, String> {
     s.split(',').map(|p| p.trim().parse::<usize>().ok().filter(|n| *n > 0).map(|n| n - 1).ok_or(format!("bad page number {p:?}"))).collect()
 }
 
+/// The options `edit` understands, for its "unknown option" message.
+const EDIT_OPTIONS: &str = "--out, --password, --full, --rotate, --delete, --move, --insert-blank, --title, --author";
+
 /// Apply page and metadata edits through the engine and save (incrementally unless `--full`).
+///
+/// Every argument is parsed before the file is opened: an option this command does not know
+/// (a typo like `--rotat`) is an error, not something to skip — skipping it would write an
+/// unchanged copy and exit 0, which looks like a successful edit (#133).
 fn edit(args: &[String]) -> Result<(), CliError> {
     use pdfcraft_engine::{Edit, Session};
-    let path = *positional(args).first().ok_or("edit: missing file")?;
-    let out = flag(args, "--out").ok_or("edit: missing --out")?;
-    let mut session = Session::new();
-    let id = session.open(path, Some(path.to_string()), read(path)?, flag(args, "--password")).map_err(|e| e.to_string())?;
+    let mut path: Option<&str> = None;
+    let mut out: Option<&str> = None;
+    let mut password: Option<&str> = None;
+    let mut full = false;
     let mut edits = Vec::new();
     let mut i = 0;
-    while i < args.len() {
-        let value = args.get(i + 1).map(String::as_str).unwrap_or("");
-        match args[i].as_str() {
+    while let Some(arg) = args.get(i).map(String::as_str) {
+        // Options that take no value, and the input file.
+        match arg {
+            "--full" => {
+                full = true;
+                i += 1;
+                continue;
+            }
+            a if a.starts_with("--") => {}
+            a => {
+                if path.is_some() {
+                    return Err(format!("edit: unexpected argument {a:?} (one input file, then options)").into());
+                }
+                path = Some(a);
+                i += 1;
+                continue;
+            }
+        }
+        let value = args.get(i + 1).map(String::as_str).ok_or_else(|| format!("edit: {arg} needs a value"))?;
+        match arg {
+            "--out" => out = Some(value),
+            "--password" => password = Some(value),
             "--rotate" => {
                 let (pages, deg) = value.split_once(':').ok_or("--rotate PAGES:DEGREES")?;
                 edits.push(Edit::RotatePages { pages: page_list(pages)?, degrees: deg.parse().map_err(|_| "bad degrees")? });
@@ -232,14 +258,18 @@ fn edit(args: &[String]) -> Result<(), CliError> {
             }
             "--title" => edits.push(Edit::SetInfo { key: "Title".into(), value: value.into() }),
             "--author" => edits.push(Edit::SetInfo { key: "Author".into(), value: value.into() }),
-            _ => {}
+            other => return Err(format!("edit: unknown option {other:?} (expected one of: {EDIT_OPTIONS})").into()),
         }
-        i += 1;
+        i += 2;
     }
+    let path = path.ok_or("edit: missing file")?;
+    let out = out.ok_or("edit: missing --out")?;
+    let mut session = Session::new();
+    let id = session.open(path, Some(path.to_string()), read(path)?, password).map_err(|e| e.to_string())?;
     for e in edits {
         session.apply(id, e).map_err(|e| e.to_string())?;
     }
-    let bytes = if args.iter().any(|a| a == "--full") { session.save_full_bytes(id) } else { session.save_bytes(id) }.map_err(|e| e.to_string())?;
+    let bytes = if full { session.save_full_bytes(id) } else { session.save_bytes(id) }.map_err(|e| e.to_string())?;
     std::fs::write(out, bytes.as_slice()).map_err(|e| format!("{out}: {e}").into())
 }
 

apps/pdfcraft-cli/tests/edit.rs

@@ -0,0 +1,93 @@
+//! `edit` must reject options it does not know instead of writing an unchanged copy (#133).
+
+use std::path::PathBuf;
+use std::process::Command;
+
+const BIN: &str = env!("CARGO_BIN_EXE_pdfcraft-cli");
+
+/// A one-page 200×300 pt PDF.
+fn fixture() -> Vec<u8> {
+    let objs: Vec<Vec<u8>> = vec![
+        b"<< /Type /Catalog /Pages 2 0 R >>".to_vec(),
+        b"<< /Type /Pages /Kids [3 0 R] /Count 1 /MediaBox [0 0 200 300] >>".to_vec(),
+        b"<< /Type /Page /Parent 2 0 R >>".to_vec(),
+    ];
+    let mut out = b"%PDF-1.7\n".to_vec();
+    let mut offsets = Vec::new();
+    for (i, o) in objs.iter().enumerate() {
+        offsets.push(out.len());
+        out.extend_from_slice(format!("{} 0 obj\n", i + 1).as_bytes());
+        out.extend_from_slice(o);
+        out.extend_from_slice(b"\nendobj\n");
+    }
+    let xref = out.len();
+    out.extend_from_slice(format!("xref\n0 {}\n0000000000 65535 f \n", objs.len() + 1).as_bytes());
+    for o in offsets {
+        out.extend_from_slice(format!("{o:010} 00000 n \n").as_bytes());
+    }
+    out.extend_from_slice(format!("trailer\n<< /Size {} /Root 1 0 R >>\nstartxref\n{xref}\n%%EOF\n", objs.len() + 1).as_bytes());
+    out
+}
+
+fn tmp(test: &str, name: &str) -> PathBuf {
+    let d = std::env::temp_dir().join(format!("pdfcraft-cli-edit-{test}-{}", std::process::id()));
+    std::fs::create_dir_all(&d).unwrap();
+    d.join(name)
+}
+
+/// The page rotation `pdfcraft-cli info` reports for the first page, via the inspector.
+fn first_page_rotation(path: &std::path::Path) -> u16 {
+    let bytes = std::sync::Arc::new(std::fs::read(path).unwrap());
+    pdfcraft_render::inspect(bytes, None).unwrap().pages[0].rotation
+}
+
+#[test]
+fn a_misspelled_option_fails_and_writes_nothing() {
+    let input = tmp("typo", "in.pdf");
+    std::fs::write(&input, fixture()).unwrap();
+    let output = tmp("typo", "typo.pdf");
+    let out = Command::new(BIN).args(["edit", input.to_str().unwrap(), "--out", output.to_str().unwrap(), "--rotat", "1:90"]).output().unwrap();
+    assert!(!out.status.success(), "an unknown option must not exit 0");
+    let stderr = String::from_utf8_lossy(&out.stderr);
+    assert!(stderr.contains("unknown option") && stderr.contains("--rotat"), "the option must be named: {stderr}");
+    assert!(stderr.contains("--rotate"), "the accepted options must be listed: {stderr}");
+    assert!(!output.exists(), "no output may be written for a rejected command line");
+}
+
+#[test]
+fn an_option_without_its_value_fails() {
+    let input = tmp("novalue", "in.pdf");
+    std::fs::write(&input, fixture()).unwrap();
+    let output = tmp("novalue", "out.pdf");
+    let out = Command::new(BIN).args(["edit", input.to_str().unwrap(), "--out", output.to_str().unwrap(), "--title"]).output().unwrap();
+    assert!(!out.status.success());
+    assert!(String::from_utf8_lossy(&out.stderr).contains("--title needs a value"));
+    assert!(!output.exists());
+}
+
+#[test]
+fn a_second_input_file_fails() {
+    let input = tmp("stray", "in.pdf");
+    std::fs::write(&input, fixture()).unwrap();
+    let output = tmp("stray", "out.pdf");
+    let out = Command::new(BIN).args(["edit", input.to_str().unwrap(), "other.pdf", "--out", output.to_str().unwrap()]).output().unwrap();
+    assert!(!out.status.success());
+    assert!(String::from_utf8_lossy(&out.stderr).contains("unexpected argument \"other.pdf\""));
+    assert!(!output.exists());
+}
+
+#[test]
+fn the_documented_options_still_edit_and_save() {
+    let input = tmp("ok", "in.pdf");
+    std::fs::write(&input, fixture()).unwrap();
+    for (name, extra) in [("incremental.pdf", Vec::new()), ("full.pdf", vec!["--full"])] {
+        let output = tmp("ok", name);
+        let mut args = vec!["edit", input.to_str().unwrap(), "--out", output.to_str().unwrap(), "--rotate", "1:90", "--title", "Rotated"];
+        args.extend(extra);
+        let out = Command::new(BIN).args(&args).output().unwrap();
+        assert!(out.status.success(), "{name}: {}", String::from_utf8_lossy(&out.stderr));
+        assert_eq!(first_page_rotation(&output), 90, "{name}");
+        let info = pdfcraft_render::inspect(std::sync::Arc::new(std::fs::read(&output).unwrap()), None).unwrap();
+        assert_eq!(info.title.as_deref(), Some("Rotated"), "{name}");
+    }
+}