Skip to content

An empty --to resolves to the first named person instead of failing #30

Closed

Description

@ergofobe

Symptom

Context::find_person (src/context.rs) matches names case-insensitively with contains, and
does not reject an empty reference. So an empty string matches the first named person rather
than failing.

In a script, --to "$PERSON" with PERSON unset or empty silently reassigns faces onto whichever
person happens to be listed first, instead of refusing.

The lookup is shared, so this affects people show, people name, people hide, people merge
and now people reassign — including two commands that move data.

Reproduction

imogen people reassign <face-id> --to ""

Observed: faces are moved onto an arbitrary person, exit 0. Expected: a refusal naming the problem.

Expected

find_person bails on an empty or whitespace-only reference before it searches. One
reference.trim().is_empty() guard closes it for all five commands at once.

Worth considering at the same time whether a substring match is right for a destructive
operation at all — --to "Al" matching "Alice" is convenient for people show and rather less so
for merge and reassign — but the empty case is the bug and can be fixed on its own.

Affected repos

Only this one. Nothing on the wire changes.

Context

Found by review while implementing #5 (people reassign). Pre-existing; the new command made it
reachable from a data-moving path for the second time.

Activity

  1. added
    bugSomething isn't working
    triagedHome repo decided, labelled, ready to bundle
    on Sep 12, 2026
  2. added a commit that references this issue on Sep 15, 2026
    ca009f3
  3. ergofobe commented on Sep 15, 2026

    @ergofobe
    OwnerAuthor

    Fixed in #37 (squashed to main).

    Context::find_person now bails on an empty or whitespace-only reference before the lookup, so all five commands that share it refuse — and refuse before anything reaches the wire. The reference is trimmed once and searched in its trimmed form, so the guard and the search agree about what is empty.

    Reproduced first as a test against a one-person library, which is where the bug was quiet: reassign(…, Some("")) succeeded and moved the faces onto the only person, exit 0.

    On the substring question you raised: kept substring matching, and added the exact-name tier find_album already has — but only when exactly one person carries the name. Somebody called "Al" was previously unreachable by their own whole name, since contains matched both "Al" and "Alice" and it was refused as ambiguous; the precise string now wins over a longer one it starts. Because person names are not unique — people merge exists because grouping produces two clusters for one person — 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. A name that works for people show failing on people reassign makes a reference whose meaning depends on the verb, which is harder to reason about than one rule everywhere. Ambiguity is already refused and ids remain the machine path.

    Noted in the PR under ## Found, not fixed: find_album has the same empty-reference hole with a worse blast radius (imogen trash --album ""), its exact-name tier is not checked for uniqueness either, and album_or_create matches on Err(_) so an ambiguous name creates a duplicate instead of refusing. Those three want fixing together — guarding find_album alone would make album_or_create start creating an album named "".

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    bugSomething isn't workingtriagedHome repo decided, labelled, ready to bundle

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions