Skip to content

Refuse an album reference that cannot pick one album - #42

Merged
ergofobe merged 4 commits into
mainfrom
issue-38-empty-album
Sep 16, 2026
Merged

ergofobe merged 4 commits into
mainfrom
issue-38-empty-album

Conversation

@ergofobe

@ergofobe ergofobe commented Sep 16, 2026 •

Copy link
Copy Markdown
Owner

Closes #38.

imogen trash --album "$ALBUM" with the variable unset trashed a library's worth of
photographs and exited 0: contains("") is true of every name, so an empty reference
resolved to whichever album the server listed first. find_album did not trim either, so
" " behaved the same way.

The three defects #38 names are fixed together, because the empty-reference guard cannot
land on its own:

  • album_or_create matched on Err(_), so a refused empty reference would have made
    an album literally called "" — and an ambiguous name was already making a duplicate
    rather than asking which of the two was meant.
  • The exact-name tier was not uniqueness-checked. Album names have no unique index, so
    two called "Holiday" resolved to whichever the server listed first. The same shortcut was
    attempted for people in the PR closing An empty --to resolves to the first named person instead of failing #30 and reintroduced the bug it was closing; the
    tier now requires the name to be unique, exactly as find_person's whole-name tier does.

Both lookups now go through albums_for, which trims the reference and refuses an empty
one before anything reaches the wire — the trim and the guard live together so neither can
be done without the other — and then through named_album, the exact tier: an id, or a
name exactly one album carries. The stored name is compared trimmed as well, so an album
called " Trip " is still the album somebody filing into "Trip" means.

What the two do with a name nothing carries exactly is where they part, and review changed
this after the first commit:

  • find_album widens to partial names, as a lookup should.
  • album_or_create makes that name. Filing into an album is a name, not a search:
    settling for "Holiday 2024" when asked for "Holiday" puts photographs somewhere nobody
    named, and refusing "Holiday" as ambiguous between "Holiday 2024" and "Holiday 2025"
    would leave an album of that name impossible to make.

A transport failure listing the albums is no longer mistaken for "not there yet" either,
and both ambiguity refusals name each album with its id — two albums called "Holiday"
printed as "Holiday, Holiday" said nothing.

Tests

Thirteen in src/context.rs, against a stub answering GET/POST /api/v1/albums and
recording what it was asked. Those that fail on main:

  • an empty and a whitespace-only --album are refused, and nothing reaches the wire
  • an empty reference makes no album either (on main it creates one called " ")
  • a name two albums share is refused rather than resolved, and the refusal names the ids
  • an ambiguous name adds no third album of that name
  • a partial name matching several albums names them by id too
  • filing into an album does not settle for a longer name
  • a library that cannot be listed makes nothing
  • a new album is created under the trimmed name

And the regression guards, which pass on main and pin what the fix must not break: an
exact name still beats the longer one it is a prefix of, a name with a stray newline still
finds its album, an album whose stored name is padded is still the album of that name, and
a new name is still creatable beside the names it is part of.

Verify

cargo test --locked && cargo fmt --check && cargo clippy --all-targets --locked -- -D warnings && cargo build --release --locked — green (153 tests).

Found, not fixed

🤖 Generated with Claude Code

ergofobe and others added 3 commits September 15, 2026 23:04
`imogen trash --album "$ALBUM"` with the variable unset trashed the library:
`contains("")` is true of every name, so an empty reference resolved to
whichever album the server listed first and `trash` was handed a filter
matching all of it, exit 0. `find_album` also did not trim, so "   " did the
same.

The guard cannot land alone. `album_or_create` matched on `Err(_)`, treating
every failure as "not there yet", so a refused empty reference would have made
an album called "" — and an ambiguous name was already making a duplicate
rather than asking which album was meant. And the exact-name tier took the
first album of that name without checking it was the only one; album names have
no unique index, so two called "Holiday" resolved arbitrarily. The same
shortcut was attempted for people while closing #30 and reintroduced the bug it
was closing.

So all three together, in one lookup the two callers share: `look_up_album`
refuses an empty or ambiguous reference and answers `Ok(None)` only for a name
nothing carries, which is the one case `album_or_create` may answer by creating
one. The whole-name tier now requires the name to be unique, as `find_person`'s
does, and the reference is trimmed once so the guard and the search agree — a
name arriving as "Holiday\n" from a shell still finds it.

Closes #38

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Review of the lookup above: routing `album_or_create` through the partial-name
tier makes a name impossible to have. With "Holiday 2024" and "Holiday 2025" in
the library, `imogen upload --album Holiday` now refused as ambiguous, where
before it made the album — and with only "Holiday 2024" there it quietly filed
the photographs into that one instead, which is the same class of defect as #38
itself: a reference that picked an album nobody named.

Filing into an album is a name, not a search, so the two callers now share only
the exact tier — `named_album`: an id, or a name exactly one album carries.
`find_album` widens to partial names after it, as a lookup should;
`album_or_create` makes the name instead. An id that matches nothing is still a
new album's name there, which is what `--album` promises.

The refusal for two albums of one name lists their ids rather than the same
name twice, and says an id is the way out, as `find_person`'s does.

Also pins the failure `Err(_)` swallowed: a library that cannot be listed at all
is not a library saying there is no such album, and must not become a new one.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Review of the two commits above. `albums_to_search` guarded an empty reference
but left trimming to its callers, so a third caller passing a raw `--album`
value would reintroduce #38 with nothing to catch it. Both now live in
`albums_for`, which hands back the trimmed reference alongside the albums: the
two cannot be done separately.

The partial-name refusal still listed names alone, so two albums called
"Holiday" printed as "Holiday, Holiday" — the very thing the whole-name refusal
was changed to avoid one tier above. Both now name each album with its id and
say an id is the way past.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Third review round. With the reference trimmed and the name not, an album
called " Trip " — nothing stops this program, the web interface or an import
from writing one — stopped answering to "Trip", and `imogen upload --album Trip`
made a second album indistinguishable from the first in a listing. The partial
tier used to cover this by accident; the exact tier has to do it on purpose.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@ergofobe
ergofobe merged commit 4a3cc7a into main Sep 16, 2026
2 checks passed
@ergofobe
ergofobe deleted the issue-38-empty-album branch September 16, 2026 03:15
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.

An empty --album matches every album, and trashing by album acts on all of them

1 participant