Skip to content

Refuse argument combinations that used to resolve to something arbitrary - #37

Merged
ergofobe merged 4 commits into
mainfrom
issue-30-32-arg-refusals
Sep 15, 2026
Merged

ergofobe merged 4 commits into
mainfrom
issue-30-32-arg-refusals

Conversation

@ergofobe

@ergofobe ergofobe commented Sep 15, 2026 •

Copy link
Copy Markdown
Owner

Two arguments the CLI accepted and then quietly picked an answer for. Same shape, same
area, one diff: a contradiction or a non-reference is now refused rather than resolved.

#30 — an empty --to resolved to the first named person

Context::find_person matched names with contains, and contains("") is true of every
name. --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. The reference is trimmed once and the search uses
the trimmed form, so the guard and the lookup agree about what is empty.

#32 — --location and --clear-location together silently cleared

The two branches in build_patch wrote the same field, so the clear overwrote the set —
losing the value the user actually asked for, which is the wrong way round for a tie-break.
Now declared in clap with conflicts_with, following people reassign's --to /
--unassign, so the refusal is clap's own message rather than a hand-rolled check:

$ imogen edit asset-1 --location 50.1,-5.5 --clear-location
error: the argument '--location <LOCATION>' cannot be used with '--clear-location'
exit 2

--location turned out to be one of three identical pairs in the same function.
--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 too, so build_patch has no branch that can overwrite
another. --favorite/--no-favorite and --archive/--unarchive deliberately keep
overrides_with: last-wins on a repeatable boolean is a POSIX idiom and is deterministic,
not the arbitrary resolution #32 is about.

The substring question #30 raised

#30 asked whether a substring match is right for a destructive operation at all. Decided:
keep substring matching, but give find_person the exact-name tier find_album already
has
— and only when the name is unique. Somebody called "Al" was previously unreachable by
their own whole name, because contains matched both "Al" and "Alice" and it was refused as
ambiguous. Now the precise string wins over a longer one it happens to start. Person names
are not unique — people merge exists because grouping produces two clusters for one
person — so two people called "Al" fall through to the ambiguity refusal rather than
resolving to whichever the server listed first.

Not done: restricting merge and reassign to exact matches only. That would make a name
that works for people show fail on people reassign, and a reference whose meaning
depends on the verb is harder to reason about than one rule everywhere. Ambiguity is already
refused, ids remain the machine path, and the empty case — the one that actually resolved to
something arbitrary — is closed.

Tests

Written first, every one watched fail:

  • an_empty_destination_is_refused_before_anything_moves — "" and " " through
    people reassign against a one-person library, which is where the bug was quiet.
    Before: expect_err panicked with () — the reassign succeeded, faces moved onto
    Alice. Also asserts nothing reached the wire.
  • a_name_two_people_share_is_still_refused — two clusters both called "Al". Before the
    uniqueness check: expect_err panicked with (), faces moved onto the first.
  • an_exact_name_beats_the_longer_one_it_is_a_prefix_of — before: "Al" matches several people; use an id instead.
  • a_name_with_a_stray_newline_still_finds_them — before: Nobody called "Alice\n".
  • setting_and_clearing_a_location_are_not_both_accepted and
    setting_and_clearing_the_other_editable_fields_are_not_both_accepted — each pair is
    refused, each flag alone still parses.

cargo test --locked && cargo fmt --check && cargo clippy --all-targets --locked -- -D warnings && cargo build --release --locked
→ test result: ok. 134 passed; 0 failed; 2 ignored, fmt and clippy clean, release built.

Nothing on the wire changes; no version bump.

Found, not fixed

Both in Context, adjacent to this diff, neither blocking it, and they want fixing together:

  • find_album has the same empty-reference hole, with a worse blast radius. --album ""
    survives QueryArgs::is_empty() (it is Some(""), not None), so to_filter calls
    find_album(""); contains("") matches every album and a single-album library resolves to
    it silently. That reaches imogen trash --album "$ALBUM" with the variable unset, exit 0.
  • album_or_create swallows every find_album error and creates. It matches on Err(_),
    so an ambiguous name — not just a missing one — creates a second album under that name
    instead of refusing. This is why the guard above cannot be added on its own: find_album("")
    refusing would make album_or_create start creating an album literally named "".
  • find_album's exact-name tier is not checked for uniqueness either, and album names
    have no unique index — album_or_create actively manufactures duplicates by creating on
    any Err. With two albums called "Holiday", imogen trash --album Holiday resolves to
    whichever the server listed first.
  • find_album does not trim, so --album "$(cat name.txt)" still reports no album of
    that name where --to now works.
  • imogen album update -d "new text" --clear-description is --location and --clear-location together silently clear #32's contradiction in a
    different command; AlbumArgs::Update declares no conflicts_with either.

Does not touch #25, #27 or #34 — no overlap with #34's two seams (errors.rs, upload.rs);
this diff touches neither file.

Closes #30
Closes #32

🤖 Generated with Claude Code

ergofobe and others added 4 commits September 15, 2026 18:04
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 <noreply@anthropic.com>
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 <noreply@anthropic.com>
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 <noreply@anthropic.com>
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 <noreply@anthropic.com>
@ergofobe
ergofobe merged commit ca009f3 into main Sep 15, 2026
2 checks passed
@ergofobe
ergofobe deleted the issue-30-32-arg-refusals branch September 15, 2026 22:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

--location and --clear-location together silently clear An empty --to resolves to the first named person instead of failing

1 participant