From c5dc3d384cf8f4ce84380d8333ce2969155adc0e Mon Sep 17 00:00:00 2001 From: Jim Phillips <5315024+ergofobe@users.noreply.github.com> Date: Tue, 15 Sep 2026 18:04:33 -0400 Subject: [PATCH 1/4] Refuse argument combinations that used to resolve to something arbitrary MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two places where the CLI was handed a contradiction or a non-reference and picked an answer instead of saying no. An empty `--to` resolved to the first named person: `find_person` matched with `contains`, and `contains("")` is true of every name. In a script, `--to "$PERSON"` with the variable unset moved faces onto whoever happened to be listed first, exit 0. A `trim().is_empty()` bail before the lookup closes it for all five commands that share it — `people show`, `name`, `hide`, `merge` and `reassign` — and refuses before anything reaches the wire. While there: `find_person` gained the exact-name tier `find_album` already has, so somebody called "Al" is reachable by their whole name rather than refused as ambiguous with "Alice". The precise string a person typed now wins over a longer one it happens to start. `--location` and `--clear-location` were not declared mutually exclusive, so the clear branch overwrote the patch the set branch had just built — losing the value the user asked to set, which is the wrong way round for a tie-break. Declared in clap with `conflicts_with`, following `people reassign`'s `--to` / `--unassign`, so it is clap's own message and not a hand-rolled check. Closes #30 Closes #32 Co-Authored-By: Claude Opus 5 --- src/cli.rs | 24 +++++++++++++++++- src/commands/assets.rs | 1 + src/commands/people.rs | 56 +++++++++++++++++++++++++++++++++++++++--- src/context.rs | 17 +++++++++++++ 4 files changed, 94 insertions(+), 4 deletions(-) diff --git a/src/cli.rs b/src/cli.rs index 2c5be10..9220760 100644 --- a/src/cli.rs +++ b/src/cli.rs @@ -487,7 +487,7 @@ pub struct EditArgs { pub reset_captured_at: bool, /// Set where it was taken: lat,lon[,altitude] - #[arg(long)] + #[arg(long, conflicts_with = "clear_location")] pub location: Option, /// Forget where it was taken @@ -932,6 +932,28 @@ mod tests { ); } + #[test] + fn setting_and_clearing_a_location_are_not_both_accepted() { + // Taking both used to let the clear win silently, which loses the value the + // person actually asked to set. + assert!( + Cli::try_parse_from([ + "imogen", + "edit", + "asset-1", + "--location", + "50.1,-5.5", + "--clear-location" + ]) + .is_err(), + "a location cannot both be set and forgotten" + ); + assert!( + Cli::try_parse_from(["imogen", "edit", "asset-1", "--location", "50.1,-5.5"]).is_ok() + ); + assert!(Cli::try_parse_from(["imogen", "edit", "asset-1", "--clear-location"]).is_ok()); + } + #[test] fn a_manifest_and_paths_are_not_both_accepted() { // Taking both would leave it unclear which metadata applied to what. diff --git a/src/commands/assets.rs b/src/commands/assets.rs index 90b3cc7..c0ea67d 100644 --- a/src/commands/assets.rs +++ b/src/commands/assets.rs @@ -559,6 +559,7 @@ fn build_patch(args: &EditArgs) -> Result { if let Some(location) = &args.location { patch.location = Some(Some(parse_location(location)?)); } + // clap has already refused both at once, so neither branch can overwrite the other. if args.clear_location { patch.location = Some(None); } diff --git a/src/commands/people.rs b/src/commands/people.rs index d1bfbd0..86f84ab 100644 --- a/src/commands/people.rs +++ b/src/commands/people.rs @@ -286,7 +286,14 @@ mod tests { const PEOPLE: &str = r#"{"items":[ {"id":"person-1","name":"Alice","coverFaceId":null,"photoCount":3,"hidden":false}, - {"id":"person-2","name":"Bob","coverFaceId":null,"photoCount":1,"hidden":false} + {"id":"person-2","name":"Bob","coverFaceId":null,"photoCount":1,"hidden":false}, + {"id":"person-3","name":"Al","coverFaceId":null,"photoCount":2,"hidden":false} + ]}"#; + + /// A library with nobody to be ambiguous with, which is where an empty reference does + /// its damage quietly rather than colliding with a second match. + const ONE_PERSON: &str = r#"{"items":[ + {"id":"person-1","name":"Alice","coverFaceId":null,"photoCount":3,"hidden":false} ]}"#; fn person(id: &str, name: Option<&str>) -> Person { @@ -372,6 +379,44 @@ mod tests { assert!(sent["personId"].is_null()); } + #[tokio::test] + async fn an_empty_destination_is_refused_before_anything_moves() { + // `--to "$PERSON"` with the variable unset. `contains("")` is true of every name, + // so this used to resolve to the only person in the library and move faces onto + // them, exit 0. + for reference in ["", " "] { + let stub = stub_returning(ONE_PERSON).await; + let error = reassign( + &context(&stub.base_url), + &["face-1".into()], + Some(reference), + ) + .await + .expect_err("an empty --to is not a person"); + assert!( + error.to_string().contains("Name somebody"), + "said {error} instead of naming the problem" + ); + assert!( + stub.calls().is_empty(), + "nothing should reach the wire for a reference that cannot name anybody" + ); + } + } + + #[tokio::test] + async fn an_exact_name_beats_the_longer_one_it_is_a_prefix_of() { + // "Al" is somebody's whole name as well as the start of "Alice". Refusing it as + // ambiguous left that person unreachable by the name they actually have. + let stub = stub().await; + reassign(&context(&stub.base_url), &["face-1".into()], Some("Al")) + .await + .unwrap(); + + let sent: serde_json::Value = serde_json::from_str(&stub.calls()[1].1).unwrap(); + assert_eq!(sent["personId"], "person-3"); + } + fn context(server: &str) -> Context { let global = GlobalArgs { server: None, @@ -412,6 +457,11 @@ mod tests { /// makes and to record what it was asked. The SDK's own conformance suite does the /// same rather than depending on a server framework nothing else here needs. async fn stub() -> Stub { + stub_returning(PEOPLE).await + } + + /// The same stub over a library of the caller's choosing. + async fn stub_returning(people: &'static str) -> Stub { let listener = TcpListener::bind("127.0.0.1:0").await.unwrap(); let base_url = format!("http://{}", listener.local_addr().unwrap()); let calls: Arc>> = Arc::new(Mutex::new(Vec::new())); @@ -431,8 +481,8 @@ mod tests { "HTTP/1.1 204 No Content\r\nConnection: close\r\n\r\n".to_string() } else { format!( - "HTTP/1.1 200 OK\r\nContent-Type: application/json\r\nContent-Length: {}\r\nConnection: close\r\n\r\n{PEOPLE}", - PEOPLE.len() + "HTTP/1.1 200 OK\r\nContent-Type: application/json\r\nContent-Length: {}\r\nConnection: close\r\n\r\n{people}", + people.len() ) }; recorded.lock().unwrap().push((path, body)); diff --git a/src/context.rs b/src/context.rs index 69de761..ce96f3d 100644 --- a/src/context.rs +++ b/src/context.rs @@ -230,12 +230,29 @@ impl Context { } } + /// A person by id, or by enough of their name to be unambiguous — the same three + /// tiers as `find_album`, and for the same reason: somebody called "Al" should be + /// reachable by their whole name rather than lost to "Alice" starting with it. pub async fn find_person(&self, reference: &str) -> Result { + // `contains("")` is true of every name, so an unset `$PERSON` in a script would + // otherwise resolve to whoever happened to be listed first — and `merge` and + // `reassign` move data. Refused before the lookup, so nothing reaches the wire. + if reference.trim().is_empty() { + bail!("Name somebody by id or name — an empty reference cannot pick anybody"); + } let people = self.client.people.list(true).await?; if let Some(exact) = people.iter().find(|person| person.id == reference) { return Ok(exact.clone()); } let lowered = reference.to_lowercase(); + if let Some(named) = people.iter().find(|person| { + person + .name + .as_deref() + .is_some_and(|name| name.to_lowercase() == lowered) + }) { + return Ok(named.clone()); + } let matches: Vec<&Person> = people .iter() .filter(|person| { From fce9fb3a641437986260d8976c9a646f84f70911 Mon Sep 17 00:00:00 2001 From: Jim Phillips <5315024+ergofobe@users.noreply.github.com> Date: Tue, 15 Sep 2026 18:07:12 -0400 Subject: [PATCH 2/4] Refuse a shared name, and the other two contradictory pairs on `edit` MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review of the first commit found that the exact-name tier it added had reintroduced the bug it was closing. Person names are not unique — `people merge` exists because grouping produces two clusters for one person — so `find_person` taking the first whole-name match moved faces onto whichever the server happened to list first. It now takes that shortcut only when exactly one person carries the name, and otherwise falls through to the ambiguity refusal that was already there. `--location` was also only one of three contradictory pairs on `imogen edit`. `--captured-at` with `--reset-captured-at` reached the server as both, and the server takes the reset branch and drops the date the person typed; `-d` with `--clear-description` sent a null. Both now carry `conflicts_with` alongside `--location`'s, so `build_patch` has no branch that can overwrite another. Co-Authored-By: Claude Opus 5 --- src/cli.rs | 37 +++++++++++++++++++++++++++++++++++-- src/commands/assets.rs | 3 ++- src/commands/people.rs | 21 +++++++++++++++++++++ src/context.rs | 9 +++++++-- 4 files changed, 65 insertions(+), 5 deletions(-) diff --git a/src/cli.rs b/src/cli.rs index 9220760..af71c23 100644 --- a/src/cli.rs +++ b/src/cli.rs @@ -471,7 +471,7 @@ pub struct EditArgs { pub unarchive: bool, /// Set the description - #[arg(long, short = 'd')] + #[arg(long, short = 'd', conflicts_with = "clear_description")] pub description: Option, /// Remove the description @@ -479,7 +479,7 @@ pub struct EditArgs { pub clear_description: bool, /// Correct the capture time, as ISO-8601 or YYYY-MM-DD - #[arg(long)] + #[arg(long, conflicts_with = "reset_captured_at")] pub captured_at: Option, /// Discard a capture-time correction and go back to the imported date @@ -954,6 +954,39 @@ mod tests { assert!(Cli::try_parse_from(["imogen", "edit", "asset-1", "--clear-location"]).is_ok()); } + #[test] + fn setting_and_clearing_the_other_editable_fields_are_not_both_accepted() { + // The same contradiction as --location, in the two sibling pairs of the same + // command. --captured-at with --reset-captured-at reached the server as both, and + // the server drops the date; -d with --clear-description sent a null. + assert!( + Cli::try_parse_from([ + "imogen", + "edit", + "asset-1", + "--captured-at", + "2020-01-01", + "--reset-captured-at" + ]) + .is_err(), + "a capture time cannot both be corrected and reset" + ); + assert!( + Cli::try_parse_from([ + "imogen", + "edit", + "asset-1", + "-d", + "a caption", + "--clear-description" + ]) + .is_err(), + "a description cannot both be written and removed" + ); + assert!(Cli::try_parse_from(["imogen", "edit", "asset-1", "--reset-captured-at"]).is_ok()); + assert!(Cli::try_parse_from(["imogen", "edit", "asset-1", "-d", "a caption"]).is_ok()); + } + #[test] fn a_manifest_and_paths_are_not_both_accepted() { // Taking both would leave it unclear which metadata applied to what. diff --git a/src/commands/assets.rs b/src/commands/assets.rs index c0ea67d..2d68241 100644 --- a/src/commands/assets.rs +++ b/src/commands/assets.rs @@ -544,6 +544,8 @@ fn build_patch(args: &EditArgs) -> Result { if args.unarchive { patch.archived = Some(false); } + // Each setting flag below is declared to conflict with the clearing one that follows + // it, so clap has already refused the pair and neither can overwrite the other. if let Some(description) = &args.description { patch.description = Some(Some(description.clone())); } @@ -559,7 +561,6 @@ fn build_patch(args: &EditArgs) -> Result { if let Some(location) = &args.location { patch.location = Some(Some(parse_location(location)?)); } - // clap has already refused both at once, so neither branch can overwrite the other. if args.clear_location { patch.location = Some(None); } diff --git a/src/commands/people.rs b/src/commands/people.rs index 86f84ab..d50492e 100644 --- a/src/commands/people.rs +++ b/src/commands/people.rs @@ -296,6 +296,13 @@ mod tests { {"id":"person-1","name":"Alice","coverFaceId":null,"photoCount":3,"hidden":false} ]}"#; + /// Two clusters carrying the same whole name, which is the state `people merge` exists + /// to resolve and therefore one an exact-name match has to cope with. + const TWO_ALS: &str = r#"{"items":[ + {"id":"person-1","name":"Al","coverFaceId":null,"photoCount":3,"hidden":false}, + {"id":"person-2","name":"Al","coverFaceId":null,"photoCount":1,"hidden":false} + ]}"#; + fn person(id: &str, name: Option<&str>) -> Person { Person { id: id.into(), @@ -417,6 +424,20 @@ mod tests { assert_eq!(sent["personId"], "person-3"); } + #[tokio::test] + async fn a_name_two_people_share_is_still_refused() { + // Person names are not unique. An exact match that took the first of them would be + // the same arbitrary resolution as the empty reference, just harder to notice. + let stub = stub_returning(TWO_ALS).await; + let error = reassign(&context(&stub.base_url), &["face-1".into()], Some("Al")) + .await + .expect_err("two people are called Al"); + assert!( + error.to_string().contains("use an id instead"), + "said {error} instead of refusing" + ); + } + fn context(server: &str) -> Context { let global = GlobalArgs { server: None, diff --git a/src/context.rs b/src/context.rs index ce96f3d..3aac678 100644 --- a/src/context.rs +++ b/src/context.rs @@ -245,12 +245,17 @@ impl Context { return Ok(exact.clone()); } let lowered = reference.to_lowercase(); - if let Some(named) = people.iter().find(|person| { + // Only when it is somebody's whole name and nobody else's. Person names are not + // unique — `people merge` exists because grouping produces two clusters for one + // person — so two called "Al" fall through to the ambiguity refusal below rather + // than resolving to whichever the server listed first. + let mut whole_name = people.iter().filter(|person| { person .name .as_deref() .is_some_and(|name| name.to_lowercase() == lowered) - }) { + }); + if let (Some(named), None) = (whole_name.next(), whole_name.next()) { return Ok(named.clone()); } let matches: Vec<&Person> = people From fbe4ae358542f669ccc05dba9607a741192ca408 Mon Sep 17 00:00:00 2001 From: Jim Phillips <5315024+ergofobe@users.noreply.github.com> Date: Tue, 15 Sep 2026 18:10:28 -0400 Subject: [PATCH 3/4] Trim a person reference once, so the guard and the search agree MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The empty guard tested `reference.trim()` and the lookup then searched the untrimmed string, so `--to "$(cat name.txt)"` — the same shell interpolation the guard is about — arrived as "Alice\n", passed the guard and matched nobody. Co-Authored-By: Claude Opus 5 --- src/commands/people.rs | 18 ++++++++++++++++++ src/context.rs | 5 ++++- 2 files changed, 22 insertions(+), 1 deletion(-) diff --git a/src/commands/people.rs b/src/commands/people.rs index d50492e..aece7b5 100644 --- a/src/commands/people.rs +++ b/src/commands/people.rs @@ -438,6 +438,24 @@ mod tests { ); } + #[tokio::test] + async fn a_name_with_a_stray_newline_still_finds_them() { + // The same shell interpolation the empty guard is about: `--to "$(cat name.txt)"` + // arrives as "Alice\n". Refusing an empty reference on its trimmed form and then + // searching on the untrimmed one would report nobody called "Alice\n". + let stub = stub().await; + reassign( + &context(&stub.base_url), + &["face-1".into()], + Some("Alice\n"), + ) + .await + .unwrap(); + + let sent: serde_json::Value = serde_json::from_str(&stub.calls()[1].1).unwrap(); + assert_eq!(sent["personId"], "person-1"); + } + fn context(server: &str) -> Context { let global = GlobalArgs { server: None, diff --git a/src/context.rs b/src/context.rs index 3aac678..15404c9 100644 --- a/src/context.rs +++ b/src/context.rs @@ -237,7 +237,10 @@ impl Context { // `contains("")` is true of every name, so an unset `$PERSON` in a script would // otherwise resolve to whoever happened to be listed first — and `merge` and // `reassign` move data. Refused before the lookup, so nothing reaches the wire. - if reference.trim().is_empty() { + // Trimmed once, so the guard and the search agree: the shell interpolation this + // is about is also where a trailing newline comes from. + let reference = reference.trim(); + if reference.is_empty() { bail!("Name somebody by id or name — an empty reference cannot pick anybody"); } let people = self.client.people.list(true).await?; From 1a0a8833fa07610892acfa3ea57cfd75f3211ce5 Mon Sep 17 00:00:00 2001 From: Jim Phillips <5315024+ergofobe@users.noreply.github.com> Date: Tue, 15 Sep 2026 18:14:01 -0400 Subject: [PATCH 4/4] Correct the tier comment, and assert both sides of each pair alone The doc comment claimed `find_person` had the same three tiers as `find_album`; after this branch they differ, and saying otherwise would tell the next reader `find_album` is guarded when it is not. The sibling-pair test asserted only one side of each pair in isolation, so a `conflicts_with` naming the wrong id could have stopped `--captured-at` or `--clear-description` parsing on its own without failing anything. Co-Authored-By: Claude Opus 5 --- src/cli.rs | 7 +++++++ src/context.rs | 8 +++++--- 2 files changed, 12 insertions(+), 3 deletions(-) diff --git a/src/cli.rs b/src/cli.rs index af71c23..7e49822 100644 --- a/src/cli.rs +++ b/src/cli.rs @@ -983,8 +983,15 @@ mod tests { .is_err(), "a description cannot both be written and removed" ); + // Both sides of each pair alone, so a `conflicts_with` naming the wrong id cannot + // stop a legitimate single flag parsing without a test noticing. assert!(Cli::try_parse_from(["imogen", "edit", "asset-1", "--reset-captured-at"]).is_ok()); + assert!( + Cli::try_parse_from(["imogen", "edit", "asset-1", "--captured-at", "2020-01-01"]) + .is_ok() + ); assert!(Cli::try_parse_from(["imogen", "edit", "asset-1", "-d", "a caption"]).is_ok()); + assert!(Cli::try_parse_from(["imogen", "edit", "asset-1", "--clear-description"]).is_ok()); } #[test] diff --git a/src/context.rs b/src/context.rs index 15404c9..bca1fa5 100644 --- a/src/context.rs +++ b/src/context.rs @@ -230,9 +230,11 @@ impl Context { } } - /// A person by id, or by enough of their name to be unambiguous — the same three - /// tiers as `find_album`, and for the same reason: somebody called "Al" should be - /// reachable by their whole name rather than lost to "Alice" starting with it. + /// A person by id, by their whole name when exactly one person carries it, or by + /// enough of it to be unambiguous. The whole-name tier is why somebody called "Al" is + /// reachable at all rather than lost to "Alice" starting with it. `find_album` has the + /// same three tiers but neither of the guards below: it takes the first exact name + /// without checking it is unique, and accepts an empty reference. pub async fn find_person(&self, reference: &str) -> Result { // `contains("")` is true of every name, so an unset `$PERSON` in a script would // otherwise resolve to whoever happened to be listed first — and `merge` and